Deny capturing late-bound ty/const params in nested opaques #132832
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
Nov 10, 2024
| @@ -5,6 +5,7 @@ trait Trait<T> {} | |||
|
|
|||
| fn f() -> impl for<T> Trait<impl Trait<T>> {} | |||
| //~^ ERROR nested `impl Trait` is not allowed | |||
| //~| ERROR the trait bound `(): Trait<impl Trait<T>>` is not satisfied | |||
| //~| ERROR the trait bound `(): Trait<impl Trait<{type error}>>` is not satisfied | |||
There was a problem hiding this comment.
This error message sucks, but also it's the same error message we'd get from something like impl Trait<impl Trait<Missing>>, and really has nothing to do with this test other than now we begin resolving the late-bound T to error since it crosses a late-bound scope.
The predicate itself doesn't reference errors, but when we render an opaque using its bounds, we'll render {type error}. We could suppress it, but also I don't think it needs to be fixed here.
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 16, 2024
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#131717 (Stabilize `const_atomic_from_ptr`) - rust-lang#132134 (Remove `ResultsVisitable`) - rust-lang#132449 (mark is_val_statically_known intrinsic as stably const-callable) - rust-lang#132569 (rustdoc search: allow queries to end in an empty path segment) - rust-lang#132787 (Unify FnKind between AST visitors and make WalkItemKind more straight forward) - rust-lang#132832 (Deny capturing late-bound ty/const params in nested opaques) - rust-lang#133097 (Opt out TaKO8Ki from review rotation for now) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 17, 2024
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#131717 (Stabilize `const_atomic_from_ptr`) - rust-lang#132134 (Remove `ResultsVisitable`) - rust-lang#132449 (mark is_val_statically_known intrinsic as stably const-callable) - rust-lang#132569 (rustdoc search: allow queries to end in an empty path segment) - rust-lang#132787 (Unify FnKind between AST visitors and make WalkItemKind more straight forward) - rust-lang#132832 (Deny capturing late-bound ty/const params in nested opaques) - rust-lang#133097 (Opt out TaKO8Ki from review rotation for now) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 17, 2024
Rollup merge of rust-lang#132832 - compiler-errors:late-ty, r=cjgillot Deny capturing late-bound ty/const params in nested opaques First, this reverts a7f6095. I can't exactly remember why I approved this specific bit of rust-lang#132466; specifically, I don't know that the purpose of that commit is, and afaict we will never have an opaque that captures late-bound params through a const because opaques can't be used inside of anon consts. Am I missing something `@cjgillot?` Since I can't see a case where this matters, and no tests seem to fail. The second commit adds a `deny_late_regions: bool` to distinguish `Scope::LateBoundary` which should deny *any* late-bound params or just ty/consts. Then, when resolving opaques we wrap ourselves in a `Scope::LateBoundary { deny_late_regions: false }` so that we deny late-bound ty/const, which fixes a bunch of ICEs that all vaguely look like `impl for<T> Trait<Assoc = impl OtherTrait<T>>`. I guess this could be achieved other ways; for example, with a different scope kind, or maybe we could just reuse `Scope::Opaque`. But this seems a bit more verbose. I'm open to feedback anyways. Fixes rust-lang#131535 Fixes rust-lang#131637 Fixes rust-lang#132530 I opted to remove those crashes tests ^ without adding them as regular tests, since they're basically triggering uninteresting late-bound ICEs far off in the trait solver, and the reason that existing tests such as `tests/ui/type-alias-impl-trait/non-lifetime-binder-in-constraint.rs` don't ICE are kinda just coincidental (i.e. due to a missing impl block). I don't really feel motivated to add random permutations to tests just to exercise non-lifetime binders. r? cjgillot
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.
First, this reverts a7f6095. I can't exactly remember why I approved this specific bit of #132466; specifically, I don't know that the purpose of that commit is, and afaict we will never have an opaque that captures late-bound params through a const because opaques can't be used inside of anon consts. Am I missing something @cjgillot? Since I can't see a case where this matters, and no tests seem to fail.
The second commit adds a
deny_late_regions: boolto distinguishScope::LateBoundarywhich should deny any late-bound params or just ty/consts. Then, when resolving opaques we wrap ourselves in aScope::LateBoundary { deny_late_regions: false }so that we deny late-bound ty/const, which fixes a bunch of ICEs that all vaguely look likeimpl for<T> Trait<Assoc = impl OtherTrait<T>>.I guess this could be achieved other ways; for example, with a different scope kind, or maybe we could just reuse
Scope::Opaque. But this seems a bit more verbose. I'm open to feedback anyways.Fixes #131535
Fixes #131637
Fixes #132530
I opted to remove those crashes tests ^ without adding them as regular tests, since they're basically triggering uninteresting late-bound ICEs far off in the trait solver, and the reason that existing tests such as
tests/ui/type-alias-impl-trait/non-lifetime-binder-in-constraint.rsdon't ICE are kinda just coincidental (i.e. due to a missing impl block). I don't really feel motivated to add random permutations to tests just to exercise non-lifetime binders.r? cjgillot