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

regression: visibility qualifiers are not permitted #121607

Closed
Mark-Simulacrum opened this issue Feb 25, 2024 · 5 comments
Closed

regression: visibility qualifiers are not permitted #121607

Mark-Simulacrum opened this issue Feb 25, 2024 · 5 comments
Assignees
Labels
A-visibility Area: Visibility / privacy P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone

Comments

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Feb 25, 2024

[INFO] [stdout] error[E0449]: visibility qualifiers are not permitted here
[INFO] [stdout]    --> src/par_stream.rs:647:16
[INFO] [stdout]     |
[INFO] [stdout] 647 |                   pool.spawn(FnOnce!(move |pool: &P::ThreadPool| {
[INFO] [stdout]     |  ____________________________^
[INFO] [stdout] 648 | |                     let mut process_tasks = tasks.into_iter();
[INFO] [stdout] 649 | |
[INFO] [stdout] 650 | |                     let mut tasks = (0..pool.threads()).map(|_| Vec::new()).collect::<Vec<_>>();
[INFO] [stdout] ...   |
[INFO] [stdout] 719 | |                     }
[INFO] [stdout] 720 | |                 }))
[INFO] [stdout]     | |__________________^
[INFO] [stdout]     |
[INFO] [stdout]     = note: trait items always share the visibility of their trait
[INFO] [stdout]     = note: this error originates in the macro `FnOnce` (in Nightly builds, run with -Z macro-backtrace for more info)
@Mark-Simulacrum Mark-Simulacrum added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example labels Feb 25, 2024
@Mark-Simulacrum Mark-Simulacrum added this to the 1.77.0 milestone Feb 25, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 25, 2024
@lukas-code
Copy link
Member

minimized: (playground)

trait Foo {
    fn foo() {
        pub struct Bar;
        impl Bar {
            pub fn bar() {}
        }
    }
}

bisects to #119505 cc @fmease

@rustbot label -E-needs-bisection -E-needs-mcve S-has-mcve A-visibility

@rustbot rustbot added A-visibility Area: Visibility / privacy S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue and removed E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example labels Feb 25, 2024
@fmease fmease removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 26, 2024
@fmease fmease self-assigned this Feb 26, 2024
@fmease
Copy link
Member

fmease commented Feb 26, 2024

I already have a PR fixing this, namely #120698. I'd like to improve the approach though, that's why it's waiting on author atm.

I've been aware of this regression (cc #119924) but I should definitely prioritize the PR then.

@apiraino
Copy link
Contributor

apiraino commented Feb 26, 2024

WG-prioritization assigning priority (Zulip discussion)

Thanks @fmease and @compiler-errors for jumping on this!

@rustbot label -I-prioritize +P-critical

@rustbot rustbot added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Feb 26, 2024
@fmease
Copy link
Member

fmease commented Feb 29, 2024

Downgrading the priority as discussed here.

@fmease fmease added P-high High priority and removed P-critical Critical priority labels Feb 29, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue 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 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`
@fmease
Copy link
Member

fmease commented Mar 15, 2024

Fixed by #122004.
Backported in #122524.
Closing as completed.

@fmease fmease closed this as completed Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-visibility Area: Visibility / privacy P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants