Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

fmease
Copy link
Member

@fmease fmease commented Mar 4, 2024

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_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 wasn't actually the case giving rise to the old open issue #89342).

In PR #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 #121607 (comment) getting wrongfully rejected. Since PR #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 #89342 is caused by the aforementioned issue of us never updating in_trait_impl prior to my PR #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

@fmease fmease added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 4, 2024
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not entirely clear why we're setting this for an inherent impl. Please leave detailed explanation for why.

@fmease fmease added S-waiting-on-author 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 Mar 5, 2024
@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 7, 2024

📌 Commit 7d428db has been approved by compiler-errors

It is now in the queue for this repository.

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 7, 2024
@apiraino
Copy link
Contributor

apiraino commented Mar 7, 2024

Beta backport accepted as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle.

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Mar 7, 2024
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
@bors bors merged commit 2e3bde2 into rust-lang:master Mar 8, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 8, 2024
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`
@fmease fmease deleted the astvalidator-min-fix branch March 8, 2024 09:21
@cuviper cuviper mentioned this pull request Mar 15, 2024
@cuviper cuviper modified the milestones: 1.78.0, 1.77.0 Mar 15, 2024
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 15, 2024
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
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot use visibility modified in associated item declared inside const argument.
6 participants