Skip to content

Fix ICE with elided lifetimes in return type of foreign functions #43651

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 16, 2017

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Aug 5, 2017

cc #43567

This is for a preliminary crater/cargobomb run.
Lifetime elision in foreign functions now works exactly like in other functions or function-like entities.

If the breakage is significant, I'll have to partially revert #43543 (all the stuff that was required for dealing with late bound lifetimes in this position).

r? @eddyb

@eddyb
Copy link
Member

eddyb commented Aug 5, 2017

Wouldn't this break &T -> &U? Because foreign functions don't have true elision atm, their return type is treated like arguments. Can you also make this entire branch be => None?

@petrochenkov
Copy link
Contributor Author

Wouldn't this break &T -> &U?

Indeed. I've updated the PR with fix for the underlying elision problem.

@eddyb eddyb added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Aug 5, 2017
@eddyb
Copy link
Member

eddyb commented Aug 5, 2017

@bors try

@bors
Copy link
Collaborator

bors commented Aug 5, 2017

⌛ Trying commit 7704762 with merge 9a9f38a...

bors added a commit that referenced this pull request Aug 5, 2017
Fix ICE with elided lifetimes in return type of foreign functions

cc #43567

This is for a preliminary crater/cargobomb run.
Lifetime elision still produces a late-bound lifetime, but it is caught by the same check as catches `type F = for<'a> fn() -> &'a u8` and reported as an error.

If the breakage is significant, I'll have to partially revert #43543 (all the stuff that was required for dealing with late bound lifetimes in this position).

r? @eddyb
@eddyb
Copy link
Member

eddyb commented Aug 5, 2017

When the try build completes, we can run cargobomb on it.

@bors
Copy link
Collaborator

bors commented Aug 5, 2017

☀️ Test successful - status-travis
State: approved= try=True

@eddyb
Copy link
Member

eddyb commented Aug 5, 2017

cc @tomprince @frewsxcv @Mark-Simulacrum Cargobomb run needed for this PR, try done.

@Manishearth
Copy link
Member

\o/ thanks for this!

I think most of the issues in the wild were with the case of an unbound lifetime. I submitted PRs to fix those (but there may be newer crates since then)

@aidanhs
Copy link
Member

aidanhs commented Aug 10, 2017

Triage update: cargobomb in progress

@tomprince
Copy link
Member

Cargobomb results

@eddyb
Copy link
Member

eddyb commented Aug 15, 2017

All regressions are known spurious tests.
@bors r+

@bors
Copy link
Collaborator

bors commented Aug 15, 2017

📌 Commit 7704762 has been approved by eddyb

@Manishearth
Copy link
Member

Mostly timeouts and some panics -- why is this causing timeouts?

The one build failure is ... weird

@eddyb
Copy link
Member

eddyb commented Aug 15, 2017

@Manishearth It's not, I've looked at all of those tests in the past and they're just badly written tests, they'll fail randomly.

@Manishearth
Copy link
Member

lol. I should fix that. what about the build failure?

@Manishearth
Copy link
Member

would be nice if cargobomb had a "known timeout" functionality

@eddyb
Copy link
Member

eddyb commented Aug 15, 2017

@Manishearth You can try building the build failure test with a compiler that has this PR applied, if you really want to check. I'd really expect to see a compilation error if it's from this PR.

@aidanhs
Copy link
Member

aidanhs commented Aug 15, 2017

If they're known bad tests they should probably go in the blacklist, I only saw a couple in there - https://github.com/rust-lang-nursery/cargobomb/blob/master/blacklist.md

@eddyb
Copy link
Member

eddyb commented Aug 15, 2017

@aidanhs Ah I didn't realize there was a blacklist.

@Manishearth
Copy link
Member

@eddyb yeah, i trust this PR isn't changing behavior

@bors
Copy link
Collaborator

bors commented Aug 16, 2017

⌛ Testing commit 7704762 with merge e40dc66...

bors added a commit that referenced this pull request Aug 16, 2017
Fix ICE with elided lifetimes in return type of foreign functions

cc #43567

This is for a preliminary crater/cargobomb run.
Lifetime elision in foreign functions now works exactly like in other functions or function-like entities.

If the breakage is significant, I'll have to partially revert #43543 (all the stuff that was required for dealing with late bound lifetimes in this position).

r? @eddyb
@bors
Copy link
Collaborator

bors commented Aug 16, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing e40dc66 to master...

@petrochenkov petrochenkov deleted the foreign-life branch August 26, 2017 00:19
@est31 est31 mentioned this pull request Sep 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-crater Status: Waiting on a crater run to be completed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants