Don't synthesize host effect params for trait associated functions marked const #119505

fmease · 2024-01-02T00:01:56Z

r? fee1-dead or compiler

fmease · 2024-01-02T00:06:44Z

compiler/rustc_ast_lowering/src/item.rs

+        // 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 };


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).

Can we not just do both (delay span bug + recover as Const::No)?

compiler/rustc_ast_passes/src/ast_validation.rs

tests/ui/rfcs/rfc-2632-const-trait-impl/effects/trait-fn-const.rs

fee1-dead

overall LGTM, had one suggestion for the diagnostic.

tests/ui/rfcs/rfc-2632-const-trait-impl/effects/trait-fn-const.stderr

fee1-dead

r=me after nit

compiler/rustc_ast_passes/messages.ftl

No reason why this needs to be a `bug!()`.

fmease · 2024-01-02T12:55:05Z

@bors r=fee1-dead rollup

bors · 2024-01-02T12:55:07Z

📌 Commit aa79904 has been approved by fee1-dead

It is now in the queue for this repository.

bors · 2024-01-02T12:55:08Z

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

…, r=fee1-dead Don't synthesize host effect params for trait associated functions marked const Fixes rust-lang#113378. r? fee1-dead or compiler

Rollup of 21 pull requests Successful merges: - rust-lang#119086 (Query panic!() to useful diagnostic) - rust-lang#119239 (Remove unnecessary arm in `check_expr_yield`) - rust-lang#119298 (suppress change-tracker warnings in CI containers) - rust-lang#119319 (Document that File does not buffer reads/writes) - rust-lang#119434 (rc: Take *const T in is_dangling) - rust-lang#119444 (Rename `TyCtxt::is_closure` to `TyCtxt::is_closure_or_coroutine`) - rust-lang#119474 (Update tracking issue of naked_functions) - rust-lang#119476 (Pretty-print always-const trait predicates correctly) - rust-lang#119477 (rustdoc ui: adjust tooltip z-index to be above sidebar) - rust-lang#119479 (Remove two unused feature gates from rustc_query_impl) - rust-lang#119487 (Minor improvements in comment on `freshen.rs`) - rust-lang#119492 (Update books) - rust-lang#119494 (Deny defaults for higher-ranked generic parameters) - rust-lang#119498 (Update deadlinks of `strict_provenance` lints) - rust-lang#119505 (Don't synthesize host effect params for trait associated functions marked const) - rust-lang#119510 (Report I/O errors from rmeta encoding with emit_fatal) - rust-lang#119512 (Mark myself as back from leave) - rust-lang#119514 (coverage: Avoid a query stability hazard in `function_coverage_map`) - rust-lang#119523 (llvm: Allow `noundef` in codegen tests) - rust-lang#119534 (Update `thread_local` examples to use `local_key_cell_methods`) - rust-lang#119544 (Fix: Properly set vendor in i686-win7-windows-msvc target) r? `@ghost` `@rustbot` modify labels: rollup

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

…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.

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.

…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`

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](https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/) “pattern”. --- r? `@compiler-errors`

rustbot assigned fee1-dead Jan 2, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 2, 2024

fmease commented Jan 2, 2024

View reviewed changes

fmease added F-const_trait_impl `#![feature(const_trait_impl)]` F-effects `#![feature(effects)]` labels Jan 2, 2024

fmease force-pushed the no-host-param-for-trait-fns branch from 9a4ac5e to 0e333c6 Compare January 2, 2024 00:09

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 Jan 2, 2024

This comment was marked as resolved.

Sign in to view

fmease force-pushed the no-host-param-for-trait-fns branch from 484cacc to cab9f8c Compare January 2, 2024 00:14

fmease commented Jan 2, 2024

View reviewed changes

compiler/rustc_ast_passes/src/ast_validation.rs Outdated Show resolved Hide resolved

fmease commented Jan 2, 2024

View reviewed changes

tests/ui/rfcs/rfc-2632-const-trait-impl/effects/trait-fn-const.rs Outdated Show resolved Hide resolved

fmease force-pushed the no-host-param-for-trait-fns branch from cab9f8c to 50d96c7 Compare January 2, 2024 05:03

fee1-dead approved these changes Jan 2, 2024

View reviewed changes

tests/ui/rfcs/rfc-2632-const-trait-impl/effects/trait-fn-const.stderr Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

fmease force-pushed the no-host-param-for-trait-fns branch from 50d96c7 to 9fc4575 Compare January 2, 2024 05:46

fee1-dead approved these changes Jan 2, 2024

View reviewed changes

compiler/rustc_ast_passes/messages.ftl Outdated Show resolved Hide resolved

fmease added 4 commits January 2, 2024 13:49

Don't synthesize host effect params for trait assoc fns marked const

ba86034

Turn a bug!() into a span_delay_bug()

8f546aa

No reason why this needs to be a `bug!()`.

E0379: Make diagnostic more precise

ae8e401

E0379: Provide suggestions

aa79904

fmease force-pushed the no-host-param-for-trait-fns branch from 9fc4575 to aa79904 Compare January 2, 2024 12:50

bors added S-waiting-on-bors 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 Jan 2, 2024

This was referenced Jan 2, 2024

Rollup of 13 pull requests #119516

Closed

Don't synthesize host effect args inside trait object types #119540

Merged

fmease mentioned this pull request Jan 3, 2024

ICE: const fn in const trait #117478

Closed

This was referenced Jan 3, 2024

Rollup of 21 pull requests #119547

Closed

Rollup of 21 pull requests #119549

Merged

bors merged commit 8bce6fc into rust-lang:master Jan 3, 2024
11 checks passed

rustbot added this to the 1.77.0 milestone Jan 3, 2024

fmease deleted the no-host-param-for-trait-fns branch January 3, 2024 20:57

fmease mentioned this pull request Jan 13, 2024

AST validation doesn't correctly deal with impls nested within associated functions #119924

Open

matthiaskrgr mentioned this pull request Jan 30, 2024

ICE: ConstContext::Maybe must have host effect param during mir_const_qualif #120503

Open

nnethercote mentioned this pull request Feb 22, 2024

ICE: parent also has host effect param? #121418

Closed

lukas-code mentioned this pull request Feb 25, 2024

regression: visibility qualifiers are not permitted #121607

Closed

fmease mentioned this pull request Mar 7, 2024

AST validation: Improve handling of inherent impls nested within functions and anon consts #122004

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't synthesize host effect params for trait associated functions marked const #119505

Don't synthesize host effect params for trait associated functions marked const #119505

fmease commented Jan 2, 2024 •

edited

Loading

fmease Jan 2, 2024

compiler-errors Jan 2, 2024 •

edited

Loading

fmease Jan 2, 2024

This comment was marked as resolved.

fee1-dead left a comment

This comment has been minimized.

fee1-dead left a comment

fmease commented Jan 2, 2024

bors commented Jan 2, 2024

bors commented Jan 2, 2024

Don't synthesize host effect params for trait associated functions marked const #119505

Don't synthesize host effect params for trait associated functions marked const #119505

Conversation

fmease commented Jan 2, 2024 • edited Loading

fmease Jan 2, 2024

Choose a reason for hiding this comment

compiler-errors Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

fmease Jan 2, 2024

Choose a reason for hiding this comment

This comment was marked as resolved.

fee1-dead left a comment

Choose a reason for hiding this comment

This comment has been minimized.

fee1-dead left a comment

Choose a reason for hiding this comment

fmease commented Jan 2, 2024

bors commented Jan 2, 2024

bors commented Jan 2, 2024

fmease commented Jan 2, 2024 •

edited

Loading

compiler-errors Jan 2, 2024 •

edited

Loading