Fix validation when lowering ? trait bounds by compiler-errors · Pull Request #132209 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation15 Commits2 Checks6 Files changed
Conversation
This file contains hidden or 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
[ Show hidden characters]({{ revealButtonHref }})
Pass the unlowered (rustc_hir) polarity to lower_poly_trait_ref.
This allows us to actually validate that generic args are actually valid on ?Trait paths. This actually regressed in #113671 because that PR changed the behavior where we were inadvertently re-lowering paths as BoundPolarity::Positive, which was also coincidentally the only place we were enforcing the generics on ?Trait paths were correct.
r? @Nadrieril
rustbot has assigned @Nadrieril.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
HIR ty lowering was modified
cc @fmease
Funny enough, this code passes on nightly:
fn uwu<T: ?Sized<i32>>() {}
| @@ -681,11 +681,20 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ { |
|---|
| Some(self_ty), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^(a handful of lines above) we were probably also allowing ?std::ops::Fn<T> but i dont care to write a test for that.
Improve lowering of poly trait refs Fix validation when lowering ? trait bounds, improve spans for const trait bounds
Frankly I am of half a mind to say that we don't need to do the annoying and time-consuming dance of cratering the regression for (2.). If this actually ended up being "used" anywhere in the wild, then we will obviously catch this in the next beta crater and deal with it then, or I can crater this after it's landed.
The span change I can approve; the validation change looks good but I don't know that code, @fmease any objections?
Actually, I'll just split the span change out and just r=Nadrieril on that one first, since it's really kinda unrelated. My fault for putting this into one PR rbh.
jieyouxu added a commit to jieyouxu/rust that referenced this pull request
… r=Nadrieril
Pass constness with span into lower_poly_trait_ref
Gives us a span to point at for ~const/const on non-const traits.
Split from rust-lang#132209. r? Nadrieril
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#132227 - compiler-errors:better-const-span, r=Nadrieril
Pass constness with span into lower_poly_trait_ref
Gives us a span to point at for ~const/const on non-const traits.
Split from rust-lang#132209. r? Nadrieril
Fix validation when lowering Fix validation when lowering ? trait bounds, improve spans for const trait bounds? trait bounds
This actually regressed in #113671 because that PR changed the behavior where we were inadvertently re-lowering paths as BoundPolarity::Positive, which was also coincidentally the only place we were enforcing the generics on ?Trait paths were correct.
Omg, this is insane! 🙏 Thanks for catching this!
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have one concern but otherwise looks pretty good to me!
Comment on lines 688 to 710
| // No-op. |
|---|
| return arg_count; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this early return mean we don't validate associated item constraints which get lowered below this point in lower_assoc_item_constraint? E.g., isn't it still the case that ?Sized<Undefined = (), Undefined = ()> gets accepted under this PR?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:'( yeah
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so I made it so that we at least validate these associated type bounds, but not outright deny them. We may want to do that later.
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
📌 Commit 06a49b6 has been approved by fmease
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
Fix validation when lowering ? trait bounds
Pass the unlowered (rustc_hir) polarity to lower_poly_trait_ref.
This allows us to actually validate that generic args are actually valid on ?Trait paths. This actually regressed in rust-lang#113671 because that PR changed the behavior where we were inadvertently re-lowering paths as BoundPolarity::Positive, which was also coincidentally the only place we were enforcing the generics on ?Trait paths were correct.
bors added a commit to rust-lang-ci/rust that referenced this pull request
…iaskrgr
Rollup of 5 pull requests
Successful merges:
- rust-lang#131168 (Fix
target_osformipsel-sony-psx) - rust-lang#132209 (Fix validation when lowering
?trait bounds) - rust-lang#132357 (Improve missing_abi lint)
- rust-lang#132385 (compiler: Move
rustc_target::spec::abi::Abitorustc_abi::ExternAbi) - rust-lang#132417 (macOS: Document the difference between Clang's
-darwinand-macosxtargets)
r? @ghost
@rustbot modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#132209 - compiler-errors:modifiers, r=fmease
Fix validation when lowering ? trait bounds
Pass the unlowered (rustc_hir) polarity to lower_poly_trait_ref.
This allows us to actually validate that generic args are actually valid on ?Trait paths. This actually regressed in rust-lang#113671 because that PR changed the behavior where we were inadvertently re-lowering paths as BoundPolarity::Positive, which was also coincidentally the only place we were enforcing the generics on ?Trait paths were correct.
Labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.