Rollup merge of #139235 - nnethercote:AstValidator-tweaks, r=compiler-errors

`AstValidator` tweaks

When I read through `AstValidator` there were several things that tripped me up, and made the code harder to understand than I would have liked. This PR addresses them. Best reviewed one commit at a time.

r? ``@davidtwco``
This commit is contained in:
Matthias Krüger 2025-04-02 19:44:13 +02:00 committed by GitHub
commit ad8db11b94
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 191 additions and 190 deletions

View File

@ -47,14 +47,14 @@ enum SelfSemantic {
}
enum TraitOrTraitImpl {
Trait { span: Span, constness: Option<Span> },
TraitImpl { constness: Const, polarity: ImplPolarity, trait_ref: Span },
Trait { span: Span, constness_span: Option<Span> },
TraitImpl { constness: Const, polarity: ImplPolarity, trait_ref_span: Span },
}
impl TraitOrTraitImpl {
fn constness(&self) -> Option<Span> {
match self {
Self::Trait { constness: Some(span), .. }
Self::Trait { constness_span: Some(span), .. }
| Self::TraitImpl { constness: Const::Yes(span), .. } => Some(*span),
_ => None,
}
@ -66,7 +66,7 @@ struct AstValidator<'a> {
features: &'a Features,
/// The span of the `extern` in an `extern { ... }` block, if any.
extern_mod: Option<Span>,
extern_mod_span: Option<Span>,
outer_trait_or_trait_impl: Option<TraitOrTraitImpl>,
@ -75,7 +75,7 @@ struct AstValidator<'a> {
/// Used to ban nested `impl Trait`, e.g., `impl Into<impl Debug>`.
/// Nested `impl Trait` _is_ allowed in associated type position,
/// e.g., `impl Iterator<Item = impl Debug>`.
outer_impl_trait: Option<Span>,
outer_impl_trait_span: Option<Span>,
disallow_tilde_const: Option<TildeConstReason>,
@ -96,17 +96,22 @@ impl<'a> AstValidator<'a> {
trait_.map(|(constness, polarity, trait_ref)| TraitOrTraitImpl::TraitImpl {
constness,
polarity,
trait_ref: trait_ref.path.span,
trait_ref_span: trait_ref.path.span,
}),
);
f(self);
self.outer_trait_or_trait_impl = old;
}
fn with_in_trait(&mut self, span: Span, constness: Option<Span>, f: impl FnOnce(&mut Self)) {
fn with_in_trait(
&mut self,
span: Span,
constness_span: Option<Span>,
f: impl FnOnce(&mut Self),
) {
let old = mem::replace(
&mut self.outer_trait_or_trait_impl,
Some(TraitOrTraitImpl::Trait { span, constness }),
Some(TraitOrTraitImpl::Trait { span, constness_span }),
);
f(self);
self.outer_trait_or_trait_impl = old;
@ -170,10 +175,10 @@ impl<'a> AstValidator<'a> {
Err(errors::WhereClauseBeforeTypeAlias { span, sugg })
}
fn with_impl_trait(&mut self, outer: Option<Span>, f: impl FnOnce(&mut Self)) {
let old = mem::replace(&mut self.outer_impl_trait, outer);
fn with_impl_trait(&mut self, outer_span: Option<Span>, f: impl FnOnce(&mut Self)) {
let old = mem::replace(&mut self.outer_impl_trait_span, outer_span);
f(self);
self.outer_impl_trait = old;
self.outer_impl_trait_span = old;
}
// Mirrors `visit::walk_ty`, but tracks relevant state.
@ -258,21 +263,22 @@ impl<'a> AstValidator<'a> {
&& let TraitOrTraitImpl::TraitImpl {
constness: Const::No,
polarity: ImplPolarity::Positive,
trait_ref,
trait_ref_span,
..
} = parent
{
Some(trait_ref.shrink_to_lo())
Some(trait_ref_span.shrink_to_lo())
} else {
None
};
let make_trait_const_sugg =
if const_trait_impl && let TraitOrTraitImpl::Trait { span, constness: None } = parent {
Some(span.shrink_to_lo())
} else {
None
};
let make_trait_const_sugg = if const_trait_impl
&& let TraitOrTraitImpl::Trait { span, constness_span: None } = parent
{
Some(span.shrink_to_lo())
} else {
None
};
let parent_constness = parent.constness();
self.dcx().emit_err(errors::TraitFnConst {
@ -448,13 +454,13 @@ impl<'a> AstValidator<'a> {
check_where_clause(where_clauses.after);
}
fn check_foreign_kind_bodyless(&self, ident: Ident, kind: &str, body: Option<Span>) {
let Some(body) = body else {
fn check_foreign_kind_bodyless(&self, ident: Ident, kind: &str, body_span: Option<Span>) {
let Some(body_span) = body_span else {
return;
};
self.dcx().emit_err(errors::BodyInExtern {
span: ident.span,
body,
body: body_span,
block: self.current_extern_span(),
kind,
});
@ -473,7 +479,7 @@ impl<'a> AstValidator<'a> {
}
fn current_extern_span(&self) -> Span {
self.sess.source_map().guess_head_span(self.extern_mod.unwrap())
self.sess.source_map().guess_head_span(self.extern_mod_span.unwrap())
}
/// An `fn` in `extern { ... }` cannot have qualifiers, e.g. `async fn`.
@ -583,9 +589,10 @@ impl<'a> AstValidator<'a> {
self.dcx().emit_err(errors::ModuleNonAscii { span: ident.span, name: ident.name });
}
fn deny_generic_params(&self, generics: &Generics, ident: Span) {
fn deny_generic_params(&self, generics: &Generics, ident_span: Span) {
if !generics.params.is_empty() {
self.dcx().emit_err(errors::AutoTraitGeneric { span: generics.span, ident });
self.dcx()
.emit_err(errors::AutoTraitGeneric { span: generics.span, ident: ident_span });
}
}
@ -605,11 +612,11 @@ impl<'a> AstValidator<'a> {
}
}
fn deny_items(&self, trait_items: &[P<AssocItem>], ident: Span) {
fn deny_items(&self, trait_items: &[P<AssocItem>], ident_span: Span) {
if !trait_items.is_empty() {
let spans: Vec<_> = trait_items.iter().map(|i| i.kind.ident().unwrap().span).collect();
let total = trait_items.first().unwrap().span.to(trait_items.last().unwrap().span);
self.dcx().emit_err(errors::AutoTraitItems { spans, total, ident });
self.dcx().emit_err(errors::AutoTraitItems { spans, total, ident: ident_span });
}
}
@ -694,7 +701,7 @@ impl<'a> AstValidator<'a> {
}
}
TyKind::ImplTrait(_, bounds) => {
if let Some(outer_impl_trait_sp) = self.outer_impl_trait {
if let Some(outer_impl_trait_sp) = self.outer_impl_trait_span {
self.dcx().emit_err(errors::NestedImplTrait {
span: ty.span,
outer: outer_impl_trait_sp,
@ -727,6 +734,19 @@ impl<'a> AstValidator<'a> {
)
}
}
// Used within `visit_item` for item kinds where we don't call `visit::walk_item`.
fn visit_attrs_vis(&mut self, attrs: &'a AttrVec, vis: &'a Visibility) {
walk_list!(self, visit_attribute, attrs);
self.visit_vis(vis);
}
// Used within `visit_item` for item kinds where we don't call `visit::walk_item`.
fn visit_attrs_vis_ident(&mut self, attrs: &'a AttrVec, vis: &'a Visibility, ident: &'a Ident) {
walk_list!(self, visit_attribute, attrs);
self.visit_vis(vis);
self.visit_ident(ident);
}
}
/// Checks that generic parameters are in the correct order,
@ -834,36 +854,33 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
self_ty,
items,
}) => {
self.visit_attrs_vis(&item.attrs, &item.vis);
self.visibility_not_permitted(
&item.vis,
errors::VisibilityNotPermittedNote::TraitImpl,
);
if let TyKind::Dummy = self_ty.kind {
// Abort immediately otherwise the `TyKind::Dummy` will reach HIR lowering,
// which isn't allowed. Not a problem for this obscure, obsolete syntax.
self.dcx().emit_fatal(errors::ObsoleteAuto { span: item.span });
}
if let (&Safety::Unsafe(span), &ImplPolarity::Negative(sp)) = (safety, polarity) {
self.dcx().emit_err(errors::UnsafeNegativeImpl {
span: sp.to(t.path.span),
negative: sp,
r#unsafe: span,
});
}
let disallowed = matches!(constness, Const::No)
.then(|| TildeConstReason::TraitImpl { span: item.span });
self.with_tilde_const(disallowed, |this| this.visit_generics(generics));
self.visit_trait_ref(t);
self.visit_ty(self_ty);
self.with_in_trait_impl(Some((*constness, *polarity, t)), |this| {
this.visibility_not_permitted(
&item.vis,
errors::VisibilityNotPermittedNote::TraitImpl,
);
if let TyKind::Dummy = self_ty.kind {
// Abort immediately otherwise the `TyKind::Dummy` will reach HIR lowering,
// which isn't allowed. Not a problem for this obscure, obsolete syntax.
this.dcx().emit_fatal(errors::ObsoleteAuto { span: item.span });
}
if let (&Safety::Unsafe(span), &ImplPolarity::Negative(sp)) = (safety, polarity)
{
this.dcx().emit_err(errors::UnsafeNegativeImpl {
span: sp.to(t.path.span),
negative: sp,
r#unsafe: span,
});
}
this.visit_vis(&item.vis);
let disallowed = matches!(constness, Const::No)
.then(|| TildeConstReason::TraitImpl { span: item.span });
this.with_tilde_const(disallowed, |this| this.visit_generics(generics));
this.visit_trait_ref(t);
this.visit_ty(self_ty);
walk_list!(this, visit_assoc_item, items, AssocCtxt::Impl { of_trait: true });
});
walk_list!(self, visit_attribute, &item.attrs);
return; // Avoid visiting again.
}
ItemKind::Impl(box Impl {
safety,
@ -883,39 +900,36 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
only_trait,
};
self.with_in_trait_impl(None, |this| {
this.visibility_not_permitted(
&item.vis,
errors::VisibilityNotPermittedNote::IndividualImplItems,
);
if let &Safety::Unsafe(span) = safety {
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_attrs_vis(&item.attrs, &item.vis);
self.visibility_not_permitted(
&item.vis,
errors::VisibilityNotPermittedNote::IndividualImplItems,
);
if let &Safety::Unsafe(span) = safety {
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));
}
this.visit_vis(&item.vis);
this.with_tilde_const(
Some(TildeConstReason::Impl { span: item.span }),
|this| this.visit_generics(generics),
);
this.visit_ty(self_ty);
self.with_tilde_const(Some(TildeConstReason::Impl { span: item.span }), |this| {
this.visit_generics(generics)
});
self.visit_ty(self_ty);
self.with_in_trait_impl(None, |this| {
walk_list!(this, visit_assoc_item, items, AssocCtxt::Impl { of_trait: false });
});
walk_list!(self, visit_attribute, &item.attrs);
return; // Avoid visiting again.
}
ItemKind::Fn(
func @ box Fn {
@ -928,6 +942,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
define_opaque: _,
},
) => {
self.visit_attrs_vis_ident(&item.attrs, &item.vis, ident);
self.check_defaultness(item.span, *defaultness);
let is_intrinsic =
@ -955,43 +970,38 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
});
}
self.visit_vis(&item.vis);
self.visit_ident(ident);
let kind = FnKind::Fn(FnCtxt::Free, &item.vis, &*func);
self.visit_fn(kind, item.span, item.id);
walk_list!(self, visit_attribute, &item.attrs);
return; // Avoid visiting again.
}
ItemKind::ForeignMod(ForeignMod { extern_span, abi, safety, .. }) => {
let old_item = mem::replace(&mut self.extern_mod_span, Some(item.span));
self.visibility_not_permitted(
&item.vis,
errors::VisibilityNotPermittedNote::IndividualForeignItems,
);
if &Safety::Default == safety {
if item.span.at_least_rust_2024() {
self.dcx().emit_err(errors::MissingUnsafeOnExtern { span: item.span });
} else {
self.lint_buffer.buffer_lint(
MISSING_UNSAFE_ON_EXTERN,
item.id,
item.span,
BuiltinLintDiag::MissingUnsafeOnExtern {
suggestion: item.span.shrink_to_lo(),
},
);
}
}
if abi.is_none() {
self.maybe_lint_missing_abi(*extern_span, item.id);
}
self.with_in_extern_mod(*safety, |this| {
let old_item = mem::replace(&mut this.extern_mod, Some(item.span));
this.visibility_not_permitted(
&item.vis,
errors::VisibilityNotPermittedNote::IndividualForeignItems,
);
if &Safety::Default == safety {
if item.span.at_least_rust_2024() {
this.dcx().emit_err(errors::MissingUnsafeOnExtern { span: item.span });
} else {
this.lint_buffer.buffer_lint(
MISSING_UNSAFE_ON_EXTERN,
item.id,
item.span,
BuiltinLintDiag::MissingUnsafeOnExtern {
suggestion: item.span.shrink_to_lo(),
},
);
}
}
if abi.is_none() {
this.maybe_lint_missing_abi(*extern_span, item.id);
}
visit::walk_item(this, item);
this.extern_mod = old_item;
});
return; // Avoid visiting again.
self.extern_mod_span = old_item;
}
ItemKind::Enum(_, def, _) => {
for variant in &def.variants {
@ -1006,34 +1016,31 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
);
}
}
visit::walk_item(self, item)
}
ItemKind::Trait(box Trait { is_auto, generics, ident, bounds, items, .. }) => {
self.visit_attrs_vis_ident(&item.attrs, &item.vis, ident);
let is_const_trait =
attr::find_by_name(&item.attrs, sym::const_trait).map(|attr| attr.span);
self.with_in_trait(item.span, is_const_trait, |this| {
if *is_auto == IsAuto::Yes {
// Auto traits cannot have generics, super traits nor contain items.
this.deny_generic_params(generics, ident.span);
this.deny_super_traits(bounds, ident.span);
this.deny_where_clause(&generics.where_clause, ident.span);
this.deny_items(items, ident.span);
}
if *is_auto == IsAuto::Yes {
// Auto traits cannot have generics, super traits nor contain items.
self.deny_generic_params(generics, ident.span);
self.deny_super_traits(bounds, ident.span);
self.deny_where_clause(&generics.where_clause, ident.span);
self.deny_items(items, ident.span);
}
// Equivalent of `visit::walk_item` for `ItemKind::Trait` that inserts a bound
// context for the supertraits.
this.visit_vis(&item.vis);
this.visit_ident(ident);
let disallowed = is_const_trait
.is_none()
.then(|| TildeConstReason::Trait { span: item.span });
this.with_tilde_const(disallowed, |this| {
this.visit_generics(generics);
walk_list!(this, visit_param_bound, bounds, BoundKind::SuperTraits)
});
// Equivalent of `visit::walk_item` for `ItemKind::Trait` that inserts a bound
// context for the supertraits.
let disallowed =
is_const_trait.is_none().then(|| TildeConstReason::Trait { span: item.span });
self.with_tilde_const(disallowed, |this| {
this.visit_generics(generics);
walk_list!(this, visit_param_bound, bounds, BoundKind::SuperTraits)
});
self.with_in_trait(item.span, is_const_trait, |this| {
walk_list!(this, visit_assoc_item, items, AssocCtxt::Trait);
});
walk_list!(self, visit_attribute, &item.attrs);
return; // Avoid visiting again
}
ItemKind::Mod(safety, ident, mod_kind) => {
if let &Safety::Unsafe(span) = safety {
@ -1045,18 +1052,16 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
{
self.check_mod_file_item_asciionly(*ident);
}
visit::walk_item(self, item)
}
ItemKind::Struct(ident, vdata, generics) => match vdata {
VariantData::Struct { fields, .. } => {
self.visit_vis(&item.vis);
self.visit_ident(ident);
self.visit_attrs_vis_ident(&item.attrs, &item.vis, ident);
self.visit_generics(generics);
// Permit `Anon{Struct,Union}` as field type.
walk_list!(self, visit_struct_field_def, fields);
walk_list!(self, visit_attribute, &item.attrs);
return;
}
_ => {}
_ => visit::walk_item(self, item),
},
ItemKind::Union(ident, vdata, generics) => {
if vdata.fields().is_empty() {
@ -1064,15 +1069,12 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
}
match vdata {
VariantData::Struct { fields, .. } => {
self.visit_vis(&item.vis);
self.visit_ident(ident);
self.visit_attrs_vis_ident(&item.attrs, &item.vis, ident);
self.visit_generics(generics);
// Permit `Anon{Struct,Union}` as field type.
walk_list!(self, visit_struct_field_def, fields);
walk_list!(self, visit_attribute, &item.attrs);
return;
}
_ => {}
_ => visit::walk_item(self, item),
}
}
ItemKind::Const(box ConstItem { defaultness, expr, .. }) => {
@ -1083,6 +1085,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
replace_span: self.ending_semi_or_hi(item.span),
});
}
visit::walk_item(self, item);
}
ItemKind::Static(box StaticItem { expr, safety, .. }) => {
self.check_item_safety(item.span, *safety);
@ -1096,6 +1099,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
replace_span: self.ending_semi_or_hi(item.span),
});
}
visit::walk_item(self, item);
}
ItemKind::TyAlias(
ty_alias @ box TyAlias { defaultness, bounds, where_clauses, ty, .. },
@ -1119,11 +1123,10 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
help: self.sess.is_nightly_build(),
});
}
visit::walk_item(self, item);
}
_ => {}
_ => visit::walk_item(self, item),
}
visit::walk_item(self, item);
}
fn visit_foreign_item(&mut self, fi: &'a ForeignItem) {
@ -1488,10 +1491,8 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
|| ctxt == AssocCtxt::Trait
|| matches!(func.sig.header.constness, Const::Yes(_)) =>
{
self.visit_vis(&item.vis);
self.visit_ident(&func.ident);
self.visit_attrs_vis_ident(&item.attrs, &item.vis, &func.ident);
let kind = FnKind::Fn(FnCtxt::Assoc(ctxt), &item.vis, &*func);
walk_list!(self, visit_attribute, &item.attrs);
self.visit_fn(kind, item.span, item.id);
}
AssocItemKind::Type(_) => {
@ -1596,7 +1597,7 @@ fn deny_equality_constraints(
generics.where_clause.span
} else {
let mut span = predicate_span;
let mut prev: Option<Span> = None;
let mut prev_span: Option<Span> = None;
let mut preds = generics.where_clause.predicates.iter().peekable();
// Find the predicate that shouldn't have been in the where bound list.
while let Some(pred) = preds.next() {
@ -1606,12 +1607,12 @@ fn deny_equality_constraints(
if let Some(next) = preds.peek() {
// This is the first predicate, remove the trailing comma as well.
span = span.with_hi(next.span.lo());
} else if let Some(prev) = prev {
} else if let Some(prev_span) = prev_span {
// Remove the previous comma as well.
span = span.with_lo(prev.hi());
span = span.with_lo(prev_span.hi());
}
}
prev = Some(pred.span);
prev_span = Some(pred.span);
}
span
};
@ -1686,10 +1687,10 @@ pub fn check_crate(
let mut validator = AstValidator {
sess,
features,
extern_mod: None,
extern_mod_span: None,
outer_trait_or_trait_impl: None,
has_proc_macro_decls: false,
outer_impl_trait: None,
outer_impl_trait_span: None,
disallow_tilde_const: Some(TildeConstReason::Item),
extern_mod_safety: None,
lint_buffer: lints,

View File

@ -43,6 +43,21 @@ LL - #[coverage = "off"]
LL + #[coverage(on)]
|
error: malformed `coverage` attribute input
--> $DIR/name-value.rs:26:1
|
LL | #[coverage = "off"]
| ^^^^^^^^^^^^^^^^^^^
|
help: the following are the possible correct uses
|
LL - #[coverage = "off"]
LL + #[coverage(off)]
|
LL - #[coverage = "off"]
LL + #[coverage(on)]
|
error: malformed `coverage` attribute input
--> $DIR/name-value.rs:29:5
|
@ -59,7 +74,7 @@ LL + #[coverage(on)]
|
error: malformed `coverage` attribute input
--> $DIR/name-value.rs:26:1
--> $DIR/name-value.rs:35:1
|
LL | #[coverage = "off"]
| ^^^^^^^^^^^^^^^^^^^
@ -104,7 +119,7 @@ LL + #[coverage(on)]
|
error: malformed `coverage` attribute input
--> $DIR/name-value.rs:35:1
--> $DIR/name-value.rs:50:1
|
LL | #[coverage = "off"]
| ^^^^^^^^^^^^^^^^^^^
@ -148,21 +163,6 @@ LL - #[coverage = "off"]
LL + #[coverage(on)]
|
error: malformed `coverage` attribute input
--> $DIR/name-value.rs:50:1
|
LL | #[coverage = "off"]
| ^^^^^^^^^^^^^^^^^^^
|
help: the following are the possible correct uses
|
LL - #[coverage = "off"]
LL + #[coverage(off)]
|
LL - #[coverage = "off"]
LL + #[coverage(on)]
|
error: malformed `coverage` attribute input
--> $DIR/name-value.rs:64:1
|

View File

@ -37,6 +37,19 @@ LL | #[coverage(off)]
LL | #[coverage(on)]
| ++++
error: malformed `coverage` attribute input
--> $DIR/word-only.rs:26:1
|
LL | #[coverage]
| ^^^^^^^^^^^
|
help: the following are the possible correct uses
|
LL | #[coverage(off)]
| +++++
LL | #[coverage(on)]
| ++++
error: malformed `coverage` attribute input
--> $DIR/word-only.rs:29:5
|
@ -51,7 +64,7 @@ LL | #[coverage(on)]
| ++++
error: malformed `coverage` attribute input
--> $DIR/word-only.rs:26:1
--> $DIR/word-only.rs:35:1
|
LL | #[coverage]
| ^^^^^^^^^^^
@ -90,7 +103,7 @@ LL | #[coverage(on)]
| ++++
error: malformed `coverage` attribute input
--> $DIR/word-only.rs:35:1
--> $DIR/word-only.rs:50:1
|
LL | #[coverage]
| ^^^^^^^^^^^
@ -128,19 +141,6 @@ LL | #[coverage(off)]
LL | #[coverage(on)]
| ++++
error: malformed `coverage` attribute input
--> $DIR/word-only.rs:50:1
|
LL | #[coverage]
| ^^^^^^^^^^^
|
help: the following are the possible correct uses
|
LL | #[coverage(off)]
| +++++
LL | #[coverage(on)]
| ++++
error: malformed `coverage` attribute input
--> $DIR/word-only.rs:64:1
|