-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Cannot use visibility modified in associated item declared inside const argument. #89342
Labels
A-associated-items
Area: Associated items (types, constants & functions)
C-bug
Category: This is a bug.
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
Comments
ChrisDenton
added
the
needs-triage-legacy
Old issue that were never triaged. Remove this label once the issue has been sufficiently triaged.
label
Jul 16, 2023
fmease
added
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
and removed
needs-triage-legacy
Old issue that were never triaged. Remove this label once the issue has been sufficiently triaged.
labels
Jan 23, 2024
fmease
added
the
A-associated-items
Area: Associated items (types, constants & functions)
label
Jan 26, 2024
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this issue
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`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
A-associated-items
Area: Associated items (types, constants & functions)
C-bug
Category: This is a bug.
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
I tried this code:
I expected the code to compile
Instead, the compiler errored with this message:
Meta
rustc --version --verbose
:The text was updated successfully, but these errors were encountered: