Properly consider binder vars in HasTypeFlagsVisitor
#115834
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
|
r? @jackh726 (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
@bors r+ |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 14, 2023
…iaskrgr Rollup of 4 pull requests Successful merges: - rust-lang#115772 (Improve Span in smir) - rust-lang#115832 (Fix the error message for `#![feature(no_coverage)]`) - rust-lang#115834 (Properly consider binder vars in `HasTypeFlagsVisitor`) - rust-lang#115844 (Paper over an accidental regression) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 14, 2023
Rollup merge of rust-lang#115834 - compiler-errors:binder-vars, r=jackh726 Properly consider binder vars in `HasTypeFlagsVisitor` Given a PolyTraitRef like `for<'a> Ty: Trait` (where neither `Ty` nor `Trait` mention `'a`), we do *not* return true for `.has_type_flags(TypeFlags::HAS_LATE_BOUND)`, even though binders are supposed to act as if they have late-bound vars even if they don't mention them in their bound value: 31ae3b2. This is because we use `HasTypeFlagsVisitor`, which only computes the type flags for `Ty`, `Const` and `Region` and `Predicates`, and we consequently skip any binders (and setting flags for their vars) that are not contained in one of these types. This ends up causing a problem, because when we call `TyCtxt::erase_regions` (which both erases regions *and* anonymizes bound vars), we will skip such a PolyTraitRef, not anonymizing it, and therefore not making it structurally equal to other binders. This breaks vtable computations. This PR computes the flags for all binders we enter in `HasTypeFlagsVisitor` if we're looking for `TypeFlags::HAS_LATE_BOUND` (or `TypeFlags::HAS_{RE,TY,CT}_LATE_BOUND`). Fixes rust-lang#115807
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 4, 2023
…try> Make sure that predicates with unmentioned bound vars are still considered global in the old solver In the old solver, we consider predicates with late-bound vars to not be "global": https://github.com/rust-lang/rust/blob/9c8a2694fadf3900c4d7880f6357cee60e9aa39b/compiler/rustc_trait_selection/src/traits/select/mod.rs#L1840-L1844 The implementation of `has_late_bound_vars` was modified in rust-lang#115834 so that we'd properly anonymize binders that had late-bound vars but didn't reference them. This fixed an ICE. However, this also led to a behavioral change in rust-lang#117056 (comment) for a couple of crates, which now consider `for<'a> GL33: Shader` (note the binder var that is *not* used in the predicate) to not be "global". This forces associated types to not be normalizable due to the old trait solver being dumb. This PR distinguishes types which *reference* late-bound vars and binders which *have* late-bound vars. The latter is represented with the new type flag `TypeFlags::HAS_BINDER_VARS`, which is used when we only care about knowing whether binders have vars in their bound var list (even if they're not used, like for binder anonymization). This should fix (after beta backport) the `luminance-gl` and `luminance-webgl` crates in rust-lang#117056. r? types **(priority is kinda high on a review here given beta becomes stable on November 16.)**
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 4, 2023
…try> Make sure that predicates with unmentioned bound vars are still considered global in the old solver In the old solver, we consider predicates with late-bound vars to not be "global": https://github.com/rust-lang/rust/blob/9c8a2694fadf3900c4d7880f6357cee60e9aa39b/compiler/rustc_trait_selection/src/traits/select/mod.rs#L1840-L1844 The implementation of `has_late_bound_vars` was modified in rust-lang#115834 so that we'd properly anonymize binders that had late-bound vars but didn't reference them. This fixed an ICE. However, this also led to a behavioral change in rust-lang#117056 (comment) for a couple of crates, which now consider `for<'a> GL33: Shader` (note the binder var that is *not* used in the predicate) to not be "global". This forces associated types to not be normalizable due to the old trait solver being dumb. This PR distinguishes types which *reference* late-bound vars and binders which *have* late-bound vars. The latter is represented with the new type flag `TypeFlags::HAS_BINDER_VARS`, which is used when we only care about knowing whether binders have vars in their bound var list (even if they're not used, like for binder anonymization). This should fix (after beta backport) the `luminance-gl` and `luminance-webgl` crates in rust-lang#117056. r? types **(priority is kinda high on a review here given beta becomes stable on November 16.)**
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 5, 2023
…ackh726 Make sure that predicates with unmentioned bound vars are still considered global in the old solver In the old solver, we consider predicates with late-bound vars to not be "global": https://github.com/rust-lang/rust/blob/9c8a2694fadf3900c4d7880f6357cee60e9aa39b/compiler/rustc_trait_selection/src/traits/select/mod.rs#L1840-L1844 The implementation of `has_late_bound_vars` was modified in rust-lang#115834 so that we'd properly anonymize binders that had late-bound vars but didn't reference them. This fixed an ICE. However, this also led to a behavioral change in rust-lang#117056 (comment) for a couple of crates, which now consider `for<'a> GL33: Shader` (note the binder var that is *not* used in the predicate) to not be "global". This forces associated types to not be normalizable due to the old trait solver being dumb. This PR distinguishes types which *reference* late-bound vars and binders which *have* late-bound vars. The latter is represented with the new type flag `TypeFlags::HAS_BINDER_VARS`, which is used when we only care about knowing whether binders have vars in their bound var list (even if they're not used, like for binder anonymization). This should fix (after beta backport) the `luminance-gl` and `luminance-webgl` crates in rust-lang#117056. r? types **(priority is kinda high on a review here given beta becomes stable on November 16.)**
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.
Given a PolyTraitRef like
for<'a> Ty: Trait(where neitherTynorTraitmention'a), we do not return true for.has_type_flags(TypeFlags::HAS_LATE_BOUND), even though binders are supposed to act as if they have late-bound vars even if they don't mention them in their bound value: 31ae3b2. This is because we useHasTypeFlagsVisitor, which only computes the type flags forTy,ConstandRegionandPredicates, and we consequently skip any binders (and setting flags for their vars) that are not contained in one of these types.This ends up causing a problem, because when we call
TyCtxt::erase_regions(which both erases regions and anonymizes bound vars), we will skip such a PolyTraitRef, not anonymizing it, and therefore not making it structurally equal to other binders. This breaks vtable computations.This PR computes the flags for all binders we enter in
HasTypeFlagsVisitorif we're looking forTypeFlags::HAS_LATE_BOUND(orTypeFlags::HAS_{RE,TY,CT}_LATE_BOUND).Fixes #115807