Instantiate closure synthetic substs in root universe #112399
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
compiler-errors
commented
Jun 7, 2023
| @@ -189,6 +189,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |||
| pub fn errors_reported_since_creation(&self) -> bool { | |||
| self.tcx.sess.err_count() > self.err_count_on_creation | |||
| } | |||
|
|
|||
| pub fn next_root_ty_var(&self, origin: TypeVariableOrigin) -> Ty<'tcx> { | |||
There was a problem hiding this comment.
I put this in FnCtxt since I want to discourage it from being used in non-typeck places. I could also probably leave a FIXME to remove this once we model universes more faithfully. Also, I could give this a scarier name.
There was a problem hiding this comment.
think it's fine for this to not be too scary given that misusing it accepts strictly less code, would like some short comment about what this means though
rust-cloud-vms
bot
force-pushed
the
closure-substs-root-universe
branch
from
f0aa647
to
e3b499f
Compare
lcnr
reviewed
Jun 8, 2023
| @@ -189,6 +189,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |||
| pub fn errors_reported_since_creation(&self) -> bool { | |||
| self.tcx.sess.err_count() > self.err_count_on_creation | |||
| } | |||
|
|
|||
| pub fn next_root_ty_var(&self, origin: TypeVariableOrigin) -> Ty<'tcx> { | |||
There was a problem hiding this comment.
think it's fine for this to not be too scary given that misusing it accepts strictly less code, would like some short comment about what this means though
lcnr
added
S-waiting-on-author
|
hitting this issue after the PR lands is incredibly difficult as the types of all locals are gathered at the start of typeck, so they all live in the root universe. Can always go back to the underlying issue if it crops up later via crater or sth @bors r+ rollup |
bors
added
S-waiting-on-bors
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jun 16, 2023
…t-universe, r=lcnr Instantiate closure synthetic substs in root universe In the UI test example, we end up generalizing an associated type (something like `<Map<Option<i32>, [closure upvars=?0]> as IntoIterator>::Item` generalizes into `<Map<Option<i32>, [closure upvars=?1]> as IntoIterator>::Item`) then assigning it to itself, emitting an alias-relate goal. This trivially holds via one of the normalizes-to candidates, instead of relating substs, so when closure analysis eventually sets `?0` to the actual upvars, `?1` never gets constrained. This ends up being reported as an ambiguity error during writeback. Instead, we can take advantage of the fact that we *know* the closure substs live in the root universe. This will prevent them being generalized, since they always can be named, and the alias-relate above never gets emitted at all. We can probably do this to a handful of other `next_ty_var` calls in typeck for variables that are clearly associated with the body of the program, but I wanted to limit this for now. Eventually, if we end up representing universes more faithfully like a tree or whatever, we can remove this and turn it back to just a call to `next_ty_var`. Note: This is incredibly order-dependent -- we need to be assigning a type variable that was created *before* the closure substs, and we also need to actually have an unnormalized type at the time of the assignment. This currently seems easiest to trigger during call argument analysis just due to the fact that we instantiate the call's substs, normalize, THEN check args. r? `@lcnr`
Dylan-DPC
added a commit
to Dylan-DPC/rust
that referenced
this pull request
Jun 16, 2023
…t-universe, r=lcnr Instantiate closure synthetic substs in root universe In the UI test example, we end up generalizing an associated type (something like `<Map<Option<i32>, [closure upvars=?0]> as IntoIterator>::Item` generalizes into `<Map<Option<i32>, [closure upvars=?1]> as IntoIterator>::Item`) then assigning it to itself, emitting an alias-relate goal. This trivially holds via one of the normalizes-to candidates, instead of relating substs, so when closure analysis eventually sets `?0` to the actual upvars, `?1` never gets constrained. This ends up being reported as an ambiguity error during writeback. Instead, we can take advantage of the fact that we *know* the closure substs live in the root universe. This will prevent them being generalized, since they always can be named, and the alias-relate above never gets emitted at all. We can probably do this to a handful of other `next_ty_var` calls in typeck for variables that are clearly associated with the body of the program, but I wanted to limit this for now. Eventually, if we end up representing universes more faithfully like a tree or whatever, we can remove this and turn it back to just a call to `next_ty_var`. Note: This is incredibly order-dependent -- we need to be assigning a type variable that was created *before* the closure substs, and we also need to actually have an unnormalized type at the time of the assignment. This currently seems easiest to trigger during call argument analysis just due to the fact that we instantiate the call's substs, normalize, THEN check args. r? ``@lcnr``
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 16, 2023
Rollup of 7 pull requests Successful merges: - rust-lang#112163 (fix: inline `predicate_may_hold_fatal` and remove expect call in it) - rust-lang#112399 (Instantiate closure synthetic substs in root universe) - rust-lang#112443 (Opportunistically resolve regions in new solver) - rust-lang#112535 (reorder attributes to make miri-test-libstd work again) - rust-lang#112579 (Fix building libstd documentation on FreeBSD.) - rust-lang#112639 (Fix `dead_code_cgu` computation) - rust-lang#112642 (Handle interpolated literal errors) r? `@ghost` `@rustbot` modify labels: rollup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the UI test example, we end up generalizing an associated type (something like
<Map<Option<i32>, [closure upvars=?0]> as IntoIterator>::Itemgeneralizes into<Map<Option<i32>, [closure upvars=?1]> as IntoIterator>::Item) then assigning it to itself, emitting an alias-relate goal. This trivially holds via one of the normalizes-to candidates, instead of relating substs, so when closure analysis eventually sets?0to the actual upvars,?1never gets constrained. This ends up being reported as an ambiguity error during writeback.Instead, we can take advantage of the fact that we know the closure substs live in the root universe. This will prevent them being generalized, since they always can be named, and the alias-relate above never gets emitted at all.
We can probably do this to a handful of other
next_ty_varcalls in typeck for variables that are clearly associated with the body of the program, but I wanted to limit this for now. Eventually, if we end up representing universes more faithfully like a tree or whatever, we can remove this and turn it back to just a call tonext_ty_var.Note: This is incredibly order-dependent -- we need to be assigning a type variable that was created before the closure substs, and we also need to actually have an unnormalized type at the time of the assignment. This currently seems easiest to trigger during call argument analysis just due to the fact that we instantiate the call's substs, normalize, THEN check args.
r? @lcnr