Skip to content

Commit

Permalink
Rollup merge of #122004 - fmease:astvalidator-min-fix, r=compiler-errors
Browse files Browse the repository at this point in the history
AST validation: Improve handling of inherent impls nested within functions and anon consts

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 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 isn'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](https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/) “pattern”.

---

r? `@compiler-errors`
  • Loading branch information
GuillaumeGomez authored Mar 7, 2024
2 parents 4de78d2 + 7d428db commit 2e3bde2
Show file tree
Hide file tree
Showing 4 changed files with 102 additions and 27 deletions.
57 changes: 30 additions & 27 deletions compiler/rustc_ast_passes/src/ast_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -929,35 +929,38 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
only_trait: only_trait.then_some(()),
};

self.visibility_not_permitted(
&item.vis,
errors::VisibilityNotPermittedNote::IndividualImplItems,
);
if let &Unsafe::Yes(span) = unsafety {
self.dcx().emit_err(errors::InherentImplCannotUnsafe {
span: self_ty.span,
annotation_span: span,
annotation: "unsafe",
self_ty: self_ty.span,
});
}
if let &ImplPolarity::Negative(span) = polarity {
self.dcx().emit_err(error(span, "negative", false));
}
if let &Defaultness::Default(def_span) = defaultness {
self.dcx().emit_err(error(def_span, "`default`", true));
}
if let &Const::Yes(span) = constness {
self.dcx().emit_err(error(span, "`const`", true));
}
self.with_in_trait_impl(None, |this| {
this.visibility_not_permitted(
&item.vis,
errors::VisibilityNotPermittedNote::IndividualImplItems,
);
if let &Unsafe::Yes(span) = unsafety {
this.dcx().emit_err(errors::InherentImplCannotUnsafe {
span: self_ty.span,
annotation_span: span,
annotation: "unsafe",
self_ty: self_ty.span,
});
}
if let &ImplPolarity::Negative(span) = polarity {
this.dcx().emit_err(error(span, "negative", false));
}
if let &Defaultness::Default(def_span) = defaultness {
this.dcx().emit_err(error(def_span, "`default`", true));
}
if let &Const::Yes(span) = constness {
this.dcx().emit_err(error(span, "`const`", true));
}

self.visit_vis(&item.vis);
self.visit_ident(item.ident);
self.with_tilde_const(Some(DisallowTildeConstContext::Impl(item.span)), |this| {
this.visit_generics(generics)
this.visit_vis(&item.vis);
this.visit_ident(item.ident);
this.with_tilde_const(
Some(DisallowTildeConstContext::Impl(item.span)),
|this| this.visit_generics(generics),
);
this.visit_ty(self_ty);
walk_list!(this, visit_assoc_item, items, AssocCtxt::Impl);
});
self.visit_ty(self_ty);
walk_list!(self, visit_assoc_item, items, AssocCtxt::Impl);
walk_list!(self, visit_attribute, &item.attrs);
return; // Avoid visiting again.
}
Expand Down
35 changes: 35 additions & 0 deletions tests/ui/parser/impls-nested-within-anon-consts-semantic.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Regression test for issue #89342 and for part of #119924.
//@ check-pass

struct Expr<const N: u32>;

trait Trait0 {
fn required(_: Expr<{
struct Type;

impl Type {
// This visibility qualifier used to get rejected.
pub fn perform() {}
}

0
}>);
}

trait Trait1 {}

impl Trait1 for ()
where
[(); {
struct Type;

impl Type {
// This visibility qualifier used to get rejected.
pub const STORE: Self = Self;
}

0
}]:
{}

fn main() {}
15 changes: 15 additions & 0 deletions tests/ui/parser/impls-nested-within-fns-semantic-0.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Regression test for #121607 and for part of issue #119924.
//@ check-pass

trait Trait {
fn provided() {
pub struct Type;

impl Type {
// This visibility qualifier used to get rejected.
pub fn perform() {}
}
}
}

fn main() {}
22 changes: 22 additions & 0 deletions tests/ui/parser/impls-nested-within-fns-semantic-1.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Regression test for part of issue #119924.
//@ check-pass

#![feature(const_trait_impl, effects)]

#[const_trait]
trait Trait {
fn required();
}

impl const Trait for () {
fn required() {
pub struct Type;

impl Type {
// This visibility qualifier used to get rejected.
pub fn perform() {}
}
}
}

fn main() {}

0 comments on commit 2e3bde2

Please sign in to comment.