Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Return err instead of ICE #95085

Merged
merged 1 commit into from
Mar 21, 2022
Merged

Return err instead of ICE #95085

merged 1 commit into from
Mar 21, 2022

Conversation

ouz-a
Copy link
Contributor

@ouz-a ouz-a commented Mar 18, 2022

Having escaping_bound_vars results in ICE when trying to create ty::Binder::dummy, to avoid it we return err like the line above. I think this requires a more sophisticated fix, I would love to investigate if mentorship is available 🤓

Fixes #95023 and #85350

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 18, 2022
@rust-highfive
Copy link
Collaborator

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 18, 2022
@jackh726
Copy link
Member

This isn't the right fix for this. Roughly, when we see an impl like impl Fn(&isize) for Error {}, that should be impl<'a> Fn<(&'a isze,)> for Error { type Output = (); }. What's actually happening is we are that elided lifetime in the function arg as late-bound, becuse we enter "fn-like elision" here.

So, our final impl_trait_ref really should be Error: Fn<(&'_ isize, )>. Alternatively, We could in theory make impl_trait_ref return a PolyTraitRef and have it return for<'a> Error: Fn(&'a isize,)>, but that's more invasive and not really correct.

See #85477 for a previous (abandoned) attempt at this. And my comment for solutions.

I think probably the simplest option is either to 1) Check if the self ty is an error as in #85477 2) Treat this lifetime as 'static:

Here is resolve lifetime in scope for an impl. Interestingly, we are storing late-bound lifetimes here but probably shouldn't be. (You can try to remove that line and see if anything fails; would be good to know.) But, we can probably add a allow_late_bound field to Scope::Binder, then here only break when that is true (and delay_span_bug otherwise)

@jackh726
Copy link
Member

r? @jackh726

@rust-highfive rust-highfive assigned jackh726 and unassigned davidtwco Mar 18, 2022
@ouz-a
Copy link
Contributor Author

ouz-a commented Mar 18, 2022 •
edited
Loading

Thanks for the great review I was not happy with the fix itself 😅 , I will try to implement new solution as you suggested.

@jackh726
Copy link
Member

Let me know if you have questions. I'm around on Zulip most of the time.

jackh726
jackh726 requested changes Mar 19, 2022
View reviewed changes
compiler/rustc_resolve/src/late/lifetimes.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/late/lifetimes.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/late/lifetimes.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/late/lifetimes.rs Outdated Show resolved Hide resolved
jackh726
jackh726 reviewed Mar 20, 2022
View reviewed changes
compiler/rustc_resolve/src/late/lifetimes.rs Outdated
Scope::Binder { hir_id, allow_late_bound: true, .. } => {
break *hir_id;
}
Scope::Binder { allow_late_bound: false, .. } => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can just be in the same match arm as Scope::ObjectLifetimeDefault etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How ? Scope::ObjectLifetimeDefault does a completely different thing, that would cause an infinite loop I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I mean Root/Body

jackh726
jackh726 approved these changes Mar 21, 2022
View reviewed changes
Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just a couple more things! Also, can you squash the commits?

compiler/rustc_resolve/src/late/lifetimes.rs Show resolved Hide resolved
src/tools/tidy/src/ui_tests.rs Show resolved Hide resolved
@jackh726
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 21, 2022

📌 Commit be566f1 has been approved by jackh726

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 21, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 21, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 21, 2022
@bors
Auto merge of rust-lang#95180 - matthiaskrgr:rollup-ai1ch2s, r=matthi…
3c17c84 
…askrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#95074 (Refactor: use `format-args-capture` and remove unnecessary nested if blocks in some parts of `rust_passes`)
 - rust-lang#95085 (Return err instead of ICE)
 - rust-lang#95116 (Add needs-* directives to many tests)
 - rust-lang#95129 (Remove animation on source sidebar)
 - rust-lang#95166 (Update the unstable book with the new `values()` form of check-cfg)
 - rust-lang#95175 (move `adt_const_params`  to its own tracking issue)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e41e510 into rust-lang:master Mar 21, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 21, 2022
This was referenced Mar 22, 2022
Closed
Closed
@ouz-a ouz-a deleted the master5 branch July 26, 2023 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackh726 jackh726 jackh726 approved these changes

Assignees

@jackh726 jackh726

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.61.0
Development

Successfully merging this pull request may close these issues.

ICE: assertion failed: !value.has_escaping_bound_vars(), compiler/rustc_middle/src/ty/sty.rs:971:9
6 participants
@ouz-a @rust-highfive @jackh726 @bors @davidtwco @rustbot