Cleanup: Move an impl-Trait check from AST validation to AST lowering #132214
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
fmease
added
the
C-cleanup
|
rustbot has assigned @petrochenkov. Use |
rustbot
added
S-waiting-on-review
fmease
commented
Oct 27, 2024
Comment on lines
-1207
to
-1214
| // `<impl Trait as OtherTrait>::Assoc` makes no sense. | ||
| struct_span_code_err!( | ||
| self.dcx(), | ||
| tcx.def_span(alias_ty.def_id), | ||
| E0667, | ||
| "`impl Trait` is not allowed in path parameters" | ||
| ) | ||
| .emit() // Already reported in an earlier stage. |
There was a problem hiding this comment.
This should've been a span_delayed_bug in the first place (before this PR). This double emission (once during AST validation, once here during HIR ty lowering) actually lead to two identical diagnostics getting emitted (indeed, even without -Zdeduplicate-diagnostics=no).
fmease
force-pushed
the
mv-impl-trait-val-paths
branch
from
9f7cad3
to
c5af024
Compare
fmease
commented
Oct 27, 2024
| LL | fn projection_is_disallowed(x: impl Iterator) -> <impl Iterator>::Item { | ||
| | ^^^^^^^^^^^^^ | ||
|
|
||
| error[E0277]: the trait bound `impl Debug: Step` is not satisfied |
There was a problem hiding this comment.
Both "impl Debug: Step diagnostics" were just useless. They obscured the more important issues (impl-Trait in a forbidden context) and plastered the terminal.
This comment has been minimized.
This comment has been minimized.
fmease
force-pushed
the
mv-impl-trait-val-paths
branch
from
c5af024
to
442f395
Compare
fmease
commented
Oct 27, 2024
There was a problem hiding this comment.
cc @compiler-errors since you assigned yourself to #126725. However, my PR just masks the bug by lowering the "bad" opaque to {type error}. I should come up with a different reproducer as the underlying bug still hasn't been fixed.
There was a problem hiding this comment.
Right, I was able to find a new reproducer for the underlying bug: #126725 (comment). I guess I will reopen #126725 once it gets closed. And I should maybe add a new crash test in this PR.
compiler-errors
approved these changes
Oct 27, 2024
bors
added
S-waiting-on-bors
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Oct 27, 2024
…compiler-errors
Cleanup: Move an impl-Trait check from AST validation to AST lowering
Namely the one that rejects `impl Trait` in qself types and non-final path segments.
There's no good reason to perform this during AST validation.
We have better infrastructure in place in the AST lowerer (`ImplTraitContext`).
This shaves off a lot of code.
We now lower `impl Trait` in bad positions to `{type error}` which allows us to
remove a special case from HIR ty lowering.
Coincidentally fixes rust-lang#126725. Well, it only *masks* it by passing `{type error}` to HIR analysis instead of a "bad" opaque. I was able to find a new reproducer for it. See the issue.
This was referenced Oct 27, 2024
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 27, 2024
…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#132043 (Simplify param handling in `resolve_bound_vars`) - rust-lang#132214 (Cleanup: Move an impl-Trait check from AST validation to AST lowering) - rust-lang#132221 (Clean up some comments on lint implementation) - rust-lang#132228 (Revert "ci update freebsd version proposal, freebsd 12 being eol.") - rust-lang#132234 (Miri subtree update) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 27, 2024
Rollup merge of rust-lang#132214 - fmease:mv-impl-trait-val-paths, r=compiler-errors Cleanup: Move an impl-Trait check from AST validation to AST lowering Namely the one that rejects `impl Trait` in qself types and non-final path segments. There's no good reason to perform this during AST validation. We have better infrastructure in place in the AST lowerer (`ImplTraitContext`). This shaves off a lot of code. We now lower `impl Trait` in bad positions to `{type error}` which allows us to remove a special case from HIR ty lowering. Coincidentally fixes rust-lang#126725. Well, it only *masks* it by passing `{type error}` to HIR analysis instead of a "bad" opaque. I was able to find a new reproducer for it. See the issue.
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.
Namely the one that rejects
impl Traitin qself types and non-final path segments.There's no good reason to perform this during AST validation.
We have better infrastructure in place in the AST lowerer (
ImplTraitContext).This shaves off a lot of code.
We now lower
impl Traitin bad positions to{type error}which allows us toremove a special case from HIR ty lowering.
Coincidentally fixes #126725. Well, it only masks it by passing
{type error}to HIR analysis instead of a "bad" opaque. I was able to find a new reproducer for it. See the issue.