Don't synthesize host effect params for trait associated functions marked const by fmease · Pull Request #119505 · 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
Conversation14 Commits4 Checks0 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 }})
Fixes #113378.
r? fee1-dead or compiler
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
Comment on lines 1257 to 1259
| // Don't pass along the user-provided constness of trait associated functions, we don't want to |
|---|
| // synthesize a host effect param for them. We reject `const` on them during AST validation. |
| let constness = if kind == FnDeclKind::Inherent { sig.header.constness } else { Const::No }; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is strictly better than turning the bug!("parent also has host effect param? […]") into a span_delayed_bug. The latter can lead to weird extra diagnostics. E.g. if the constness found in the trait doesn't match with the one found in the trait impl, we would get diagnostics like method `f` has 0 const parameters but its trait declaration has 1 const parameter (and patching compare_impl_item to fix that would be a blatant hack).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not just do both (delay span bug + recover as Const::No)?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
Don't synthesize host effect args for trait associated functions marked const Don't synthesize host effect params for trait associated functions marked const
This comment was marked as resolved.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall LGTM, had one suggestion for the diagnostic.
This comment has been minimized.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me after nit
No reason why this needs to be a bug!().
📌 Commit aa79904 has been approved by fee1-dead
It is now in the queue for this repository.
🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.
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
fmease added a commit to fmease/rust that referenced this pull request
…, r=fee1-dead
Don't synthesize host effect params for trait associated functions marked const
Fixes rust-lang#113378.
r? fee1-dead or compiler
This was referenced
Jan 2, 2024
fmease added a commit to fmease/rust that referenced this pull request
…, r=fee1-dead
Don't synthesize host effect params for trait associated functions marked const
Fixes rust-lang#113378.
r? fee1-dead or compiler
This was referenced
Jan 3, 2024
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#119505 - fmease:no-host-param-for-trait-fns, r=fee1-dead
Don't synthesize host effect params for trait associated functions marked const
Fixes rust-lang#113378.
r? fee1-dead or compiler
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…rait, r=compiler-errors
Don't synthesize host effect args inside trait object types
While we were indeed emitting an error for ~const & const trait bounds in trait object types, we were still synthesizing host effect args for them.
Since we don't record the original trait bound modifiers for dyn-Trait in hir::TyKind::TraitObject (unlike we do for let's say impl-Trait, hir::TyKind::OpaqueTy), AstConv just assumes ty::BoundConstness::NotConst in conv_object_ty_poly_trait_ref which given <host> dyn ~const NonConstTrait resulted in us not realizing that ~const was used on a non-const trait which lead to a failed assertion in the end.
Instead of updating hir::TyKind::TraitObject to track this kind of information, just strip the user-provided constness (similar to rust-lang#119505).
Fixes rust-lang#119524.
compiler-errors added a commit to compiler-errors/rust that referenced this pull request
…rait, r=compiler-errors
Don't synthesize host effect args inside trait object types
While we were indeed emitting an error for ~const & const trait bounds in trait object types, we were still synthesizing host effect args for them.
Since we don't record the original trait bound modifiers for dyn-Trait in hir::TyKind::TraitObject (unlike we do for let's say impl-Trait, hir::TyKind::OpaqueTy), AstConv just assumes ty::BoundConstness::NotConst in conv_object_ty_poly_trait_ref which given <host> dyn ~const NonConstTrait resulted in us not realizing that ~const was used on a non-const trait which lead to a failed assertion in the end.
Instead of updating hir::TyKind::TraitObject to track this kind of information, just strip the user-provided constness (similar to rust-lang#119505).
Fixes rust-lang#119524.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…rait, r=compiler-errors
Don't synthesize host effect args inside trait object types
While we were indeed emitting an error for ~const & const trait bounds in trait object types, we were still synthesizing host effect args for them.
Since we don't record the original trait bound modifiers for dyn-Trait in hir::TyKind::TraitObject (unlike we do for let's say impl-Trait, hir::TyKind::OpaqueTy), AstConv just assumes ty::BoundConstness::NotConst in conv_object_ty_poly_trait_ref which given <host> dyn ~const NonConstTrait resulted in us not realizing that ~const was used on a non-const trait which lead to a failed assertion in the end.
Instead of updating hir::TyKind::TraitObject to track this kind of information, just strip the user-provided constness (similar to rust-lang#119505).
Fixes rust-lang#119524.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#119540 - fmease:no-effect-args-inside-dyn-trait, r=compiler-errors
Don't synthesize host effect args inside trait object types
While we were indeed emitting an error for ~const & const trait bounds in trait object types, we were still synthesizing host effect args for them.
Since we don't record the original trait bound modifiers for dyn-Trait in hir::TyKind::TraitObject (unlike we do for let's say impl-Trait, hir::TyKind::OpaqueTy), AstConv just assumes ty::BoundConstness::NotConst in conv_object_ty_poly_trait_ref which given <host> dyn ~const NonConstTrait resulted in us not realizing that ~const was used on a non-const trait which lead to a failed assertion in the end.
Instead of updating hir::TyKind::TraitObject to track this kind of information, just strip the user-provided constness (similar to rust-lang#119505).
Fixes rust-lang#119524.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request
…piler-errors
AST validation: Improve handling of inherent impls nested within functions and anon consts
Minimal fix for issue rust-lang#121607 extracted from PR rust-lang#120698 for ease of backporting and since I'd like to improve PR rust-lang#120698 in such a way that it makes AST validator truly robust against such sort of regressions (AST validator is generally beyond footgun-y atm). The current version of PR rust-lang#120698 sort of does that already but there's still room for improvement.
Fixes rust-lang#89342. Fixes [after beta-backport] rust-lang#121607. Partially addresses rust-lang#119924 (rust-lang#120698 aims to fully fix it).
Explainer
The last commit of PR rust-lang#119505 regressed issue rust-lang#121607.
Previously we would reject visibilities on associated items with visibility_not_permitted if we were in a trait (by checking the parameter ctxt of visit_assoc_item which was 100% accurate) or if we were in a trait impl (by checking a flag called in_trait_impl tracked in AstValidator which was/is only accurate if the visitor methods correctly updated it which isn't actually the case giving rise to the old open issue rust-lang#89342).
In PR rust-lang#119505, I moved even more state into the AstValidator by generalizing the flag in_trait_impl to trait_or_trait_impl to be able to report more precise diagnostics (modeling Trait | TraitImpl). However since we/I didn't update trait_or_trait_impl in all places to reflect reality (similar to us not updating in_trait_impl before), this lead to rust-lang#121607 (comment) getting wrongfully rejected. Since PR rust-lang#119505 we reject visibilities if the “globally tracked” (wrt. to AstValidator) outer_trait_or_trait_impl is Some.
Crucially, when visiting an inherent impl, I never reset outer_trait_or_trait_impl back to None leading us to believe that bar in the stack [trait Foo > fn foo > impl Bar > pub fn bar] (from the MCVE) was an inherent associated item (we saw trait Foo but not impl Bar before it).
The old open issue rust-lang#89342 is caused by the aforementioned issue of us never updating in_trait_impl prior to my PR rust-lang#119505 / outer_trait_or_trait after my PR. Stack: [impl Default for Foo > { > impl Foo > pub const X] (we only saw impl Default for Foo but not the impl Foo before it).
This PR is only meant to be a hot fix. I plan on completely rewriting AstValidator from the ground up to not rely on “globally tracked” state like this or at least make it close to impossible to forget updating it when descending into nested items (etc.). Other visitors do a way better job at that (e.g. AST lowering). I actually plan on experimenting with moving more and more logic from AstValidator into the AST lowering pass/stage/visitor to follow the Parse, don't validate “pattern”.
r? @compiler-errors
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#122004 - fmease:astvalidator-min-fix, r=compiler-errors
AST validation: Improve handling of inherent impls nested within functions and anon consts
Minimal fix for issue rust-lang#121607 extracted from PR rust-lang#120698 for ease of backporting and since I'd like to improve PR rust-lang#120698 in such a way that it makes AST validator truly robust against such sort of regressions (AST validator is generally beyond footgun-y atm). The current version of PR rust-lang#120698 sort of does that already but there's still room for improvement.
Fixes rust-lang#89342. Fixes [after beta-backport] rust-lang#121607. Partially addresses rust-lang#119924 (rust-lang#120698 aims to fully fix it).
Explainer
The last commit of PR rust-lang#119505 regressed issue rust-lang#121607.
Previously we would reject visibilities on associated items with visibility_not_permitted if we were in a trait (by checking the parameter ctxt of visit_assoc_item which was 100% accurate) or if we were in a trait impl (by checking a flag called in_trait_impl tracked in AstValidator which was/is only accurate if the visitor methods correctly updated it which isn't actually the case giving rise to the old open issue rust-lang#89342).
In PR rust-lang#119505, I moved even more state into the AstValidator by generalizing the flag in_trait_impl to trait_or_trait_impl to be able to report more precise diagnostics (modeling Trait | TraitImpl). However since we/I didn't update trait_or_trait_impl in all places to reflect reality (similar to us not updating in_trait_impl before), this lead to rust-lang#121607 (comment) getting wrongfully rejected. Since PR rust-lang#119505 we reject visibilities if the “globally tracked” (wrt. to AstValidator) outer_trait_or_trait_impl is Some.
Crucially, when visiting an inherent impl, I never reset outer_trait_or_trait_impl back to None leading us to believe that bar in the stack [trait Foo > fn foo > impl Bar > pub fn bar] (from the MCVE) was an inherent associated item (we saw trait Foo but not impl Bar before it).
The old open issue rust-lang#89342 is caused by the aforementioned issue of us never updating in_trait_impl prior to my PR rust-lang#119505 / outer_trait_or_trait after my PR. Stack: [impl Default for Foo > { > impl Foo > pub const X] (we only saw impl Default for Foo but not the impl Foo before it).
This PR is only meant to be a hot fix. I plan on completely rewriting AstValidator from the ground up to not rely on “globally tracked” state like this or at least make it close to impossible to forget updating it when descending into nested items (etc.). Other visitors do a way better job at that (e.g. AST lowering). I actually plan on experimenting with moving more and more logic from AstValidator into the AST lowering pass/stage/visitor to follow the Parse, don't validate “pattern”.
r? @compiler-errors