AST validation: Improve handling of inherent impls nested within functions and anon consts by fmease · Pull Request #122004 · rust-lang/rust (original) (raw)
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”.