-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Don't create dummy if val has escaping bounds var #105694
Conversation
r? @estebank (rustbot has picked a reviewer for you, use r? to override) |
@bors r+ rollup |
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#105481 (Start improving monomorphization items stats) - rust-lang#105674 (Point at method chains on `E0271` errors) - rust-lang#105679 (Suggest constraining type parameter with `Clone`) - rust-lang#105694 (Don't create dummy if val has escaping bounds var) - rust-lang#105727 (Tweak output for bare `dyn Trait` in arguments) - rust-lang#105739 (Migrate Jump to def links background to CSS variable) - rust-lang#105743 (`SimplifiedType` cleanups) - rust-lang#105758 (Move `TypeckResults` to separate module) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
self.param_env, | ||
predicate, | ||
)); | ||
if !ct.has_escaping_bound_vars() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this fixes the ICE this is probably unsound around inline consts and const generics.
We should not skip logic if there are excess bound vars outside of possibly diagnostics. The right fix here would be to find out where the binder was skipped and stop doing that. This will require wrappig some function arguments in binders at call sites to the current function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah... this is "ok" because it can't cause unsoundness at present and fixing it in the way I described would cause errors about things having to live for 'static.
Please add a comment explaining why not adding these obligations is ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm not really sure how I feel about this change overall. I don't like adding yet another place that we're not registering WF with bound vars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm willing to work on another solution, but I am not sure where to start for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could keep the binders around, but then we'd do higher ranked trait solving which just tells us that all lifetimes must be 'static. So basically turning the ICE into an error
Skips creating/pushing obligations if val has escaping bounds vars.
Fixes #105689