Don't synthesize host effect params for trait associated functions marked const by fmease · Pull Request #119505 · rust-lang/rust
rustbot
added
S-waiting-on-review
labels
Jan 2, 2024
fmease
changed the title
Don't synthesize host effect args for trait associated functions marked const
Don't synthesize host effect params for trait associated functions marked const
bors
added
S-waiting-on-bors
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.labels
Jan 2, 2024fmease added a commit to fmease/rust that referenced this pull request
Jan 2, 2024…, 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, 2024fmease added a commit to fmease/rust that referenced this pull request
Jan 3, 2024…, 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, 2024rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Jan 3, 2024Rollup 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
fmease
deleted the
no-host-param-for-trait-fns
branch
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
Jan 3, 2024…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
Jan 4, 2024…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
Jan 4, 2024…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
Jan 4, 2024Rollup 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
Mar 7, 2024…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](https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/) “pattern”. --- r? `@compiler-errors`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Mar 8, 2024Rollup 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](https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/) “pattern”. --- r? `@compiler-errors`
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