AST validation: Improve handling of inherent impls nested within functions and anon consts #122004
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
…tions and anon consts
fmease
added
beta-nominated
compiler-errors
approved these changes
Mar 4, 2024
There was a problem hiding this comment.
i'd love if you left a detailed explanation in the pr, and maybe something in comments at the new call site. change looks fine to me, though.
for the others reading this pr, fmease and i discussed that we'd rather have the whole ast validator rewritten, since this visitor has really turned into spaghetti. i'm happy to r+ this just so we don't affect user-facing code.
| if let &Const::Yes(span) = constness { | ||
| self.dcx().emit_err(error(span, "`const`", true)); | ||
| } | ||
| self.with_in_trait_impl(None, |this| { |
There was a problem hiding this comment.
It's not entirely clear why we're setting this for an inherent impl. Please leave detailed explanation for why.
fmease
added
S-waiting-on-author
compiler-errors
approved these changes
Mar 7, 2024
|
@bors r+ |
bors
added
S-waiting-on-bors
rustbot
added
the
beta-accepted
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 7, 2024
…llaumeGomez Rollup of 10 pull requests Successful merges: - rust-lang#119888 (Stabilize the `#[diagnostic]` namespace and `#[diagnostic::on_unimplemented]` attribute) - rust-lang#121089 (Remove `feed_local_def_id`) - rust-lang#122004 (AST validation: Improve handling of inherent impls nested within functions and anon consts) - rust-lang#122087 (Add missing background color for top-level rust documentation page and increase contrast by setting text color to black) - rust-lang#122136 (Include all library files in artifact summary on CI) - rust-lang#122137 (Don't pass a break scope to `Builder::break_for_else`) - rust-lang#122138 (Record mtime in bootstrap's LLVM linker script) - rust-lang#122141 (sync (try_)instantiate_mir_and_normalize_erasing_regions implementation) - rust-lang#122142 (cleanup rustc_infer) - rust-lang#122147 (Make `std::os::unix::ucred` module private) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 8, 2024
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`
Merged
cuviper
removed
the
beta-nominated
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 15, 2024
[beta] backports - AST validation: Improve handling of inherent impls nested within functions and anon consts rust-lang#122004 - Downgrade const eval dangling ptr in final to future incompat lint rust-lang#122204 r? cuviper
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.
Minimal fix for issue #121607 extracted from PR #120698 for ease of backporting and since I'd like to improve PR #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 #120698 sort of does that already but there's still room for improvement.
Fixes #89342.
Fixes [after beta-backport] #121607.
Partially addresses #119924 (#120698 aims to fully fix it).
Explainer
The last commit of PR #119505 regressed the code found in issue #121607.
Previously we would reject visibilities on associated items with
visibility_not_permittedif we were in a trait (by checking the parameterctxtofvisit_assoc_itemwhich was 100% accurate) or if we were in a trait impl (by checking a flag calledin_trait_impltracked inAstValidatorwhich was/is only accurate if the visitor methods correctly updated it which wasn't actually the case giving rise to the old open issue #89342).In PR #119505, I moved even more state into the
AstValidatorby generalizing the flagin_trait_impltotrait_or_trait_implto be able to report more precise diagnostics (modeling Trait | TraitImpl). However since we/I didn't updatetrait_or_trait_implin all places to reflect reality (similar to us not updatingin_trait_implbefore), this lead to #121607 (comment) getting wrongfully rejected. Since PR #119505 we reject visibilities if the “globally tracked” (wrt. toAstValidator)outer_trait_or_trait_implisSome.Crucially, when visiting an inherent impl, I never reset
outer_trait_or_trait_implback toNoneleading us to believe thatbarin the stack [trait Foo>fn foo>impl Bar>pub fn bar] (from the MCVE) was an inherent associated item (we sawtrait Foobut notimpl Barbefore it).The old open issue #89342 is caused by the aforementioned issue of us never updating
in_trait_implprior to my PR #119505 /outer_trait_or_traitafter my PR. Stack: [impl Default for Foo>{>impl Foo>pub const X] (we only sawimpl Default for Foobut not theimpl Foobefore it).This PR is only meant to be a hot fix. I plan on completely rewriting
AstValidatorfrom 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 fromAstValidatorinto the AST lowering pass/stage/visitor to follow the Parse, don't validate “pattern”.r? @compiler-errors