From f518ce81b0b9c60aeeeb5dcaf002b98317050f19 Mon Sep 17 00:00:00 2001 From: Georg Semmler Date: Mon, 5 Aug 2024 08:43:56 +0200 Subject: [PATCH] Do not apply `#[do_not_recommend]` if the feature flag is not set This commit adds additional checks for the feature flag as apparently it is possible to use this on a beta compiler without feature flags. To track whether a diagnostic attribute is allowed based of the feature in certain possibly upstream crate we introduce a new boolean flag stored in each attribute. This hopefully enables future diagnostic attributes to prevent this situation, as there is now a single place that can be checked whether the attribute should be honored or not. This0PR might be a candidate for backporting to the latest beta release. Reported in the bevy issue tracker: https://github.com/bevyengine/bevy/issues/14591#issuecomment-2266213084 --- compiler/rustc_ast/src/ast.rs | 1 + compiler/rustc_ast/src/attr/mod.rs | 9 +++++- compiler/rustc_ast/src/mut_visit.rs | 2 +- compiler/rustc_ast/src/visit.rs | 2 +- compiler/rustc_ast_lowering/src/expr.rs | 1 + compiler/rustc_ast_lowering/src/lib.rs | 8 ++++- .../rustc_ast_passes/src/ast_validation.rs | 30 +++++++++++++++++++ compiler/rustc_interface/src/passes.rs | 2 ++ .../src/ich/impls_syntax.rs | 2 +- .../traits/fulfillment_errors.rs | 19 +++++------- ...ot_apply_attribute_without_feature_flag.rs | 21 +++++++++++++ ...pply_attribute_without_feature_flag.stderr | 21 +++++++++++++ 12 files changed, 102 insertions(+), 16 deletions(-) create mode 100644 tests/ui/diagnostic_namespace/do_not_recommend/do_not_apply_attribute_without_feature_flag.rs create mode 100644 tests/ui/diagnostic_namespace/do_not_recommend/do_not_apply_attribute_without_feature_flag.stderr diff --git a/compiler/rustc_ast/src/ast.rs b/compiler/rustc_ast/src/ast.rs index fc1af3fc3dd11..dc238cbd35d16 100644 --- a/compiler/rustc_ast/src/ast.rs +++ b/compiler/rustc_ast/src/ast.rs @@ -2855,6 +2855,7 @@ pub struct Attribute { /// or the construct this attribute is contained within (inner). pub style: AttrStyle, pub span: Span, + pub allowed_diagnostic_attribute: bool, } #[derive(Clone, Encodable, Decodable, Debug)] diff --git a/compiler/rustc_ast/src/attr/mod.rs b/compiler/rustc_ast/src/attr/mod.rs index 94a00ab1a047e..89c84dbbd14b0 100644 --- a/compiler/rustc_ast/src/attr/mod.rs +++ b/compiler/rustc_ast/src/attr/mod.rs @@ -566,7 +566,13 @@ pub fn mk_doc_comment( data: Symbol, span: Span, ) -> Attribute { - Attribute { kind: AttrKind::DocComment(comment_kind, data), id: g.mk_attr_id(), style, span } + Attribute { + kind: AttrKind::DocComment(comment_kind, data), + id: g.mk_attr_id(), + style, + span, + allowed_diagnostic_attribute: false, + } } pub fn mk_attr( @@ -592,6 +598,7 @@ pub fn mk_attr_from_item( id: g.mk_attr_id(), style, span, + allowed_diagnostic_attribute: false, } } diff --git a/compiler/rustc_ast/src/mut_visit.rs b/compiler/rustc_ast/src/mut_visit.rs index 8a66894a35603..e4ccf131477c6 100644 --- a/compiler/rustc_ast/src/mut_visit.rs +++ b/compiler/rustc_ast/src/mut_visit.rs @@ -631,7 +631,7 @@ fn walk_local(vis: &mut T, local: &mut P) { } fn walk_attribute(vis: &mut T, attr: &mut Attribute) { - let Attribute { kind, id: _, style: _, span } = attr; + let Attribute { kind, id: _, style: _, span, allowed_diagnostic_attribute: _ } = attr; match kind { AttrKind::Normal(normal) => { let NormalAttr { diff --git a/compiler/rustc_ast/src/visit.rs b/compiler/rustc_ast/src/visit.rs index fe07ec48f1f2b..f861244fd0f30 100644 --- a/compiler/rustc_ast/src/visit.rs +++ b/compiler/rustc_ast/src/visit.rs @@ -1214,7 +1214,7 @@ pub fn walk_vis<'a, V: Visitor<'a>>(visitor: &mut V, vis: &'a Visibility) -> V:: } pub fn walk_attribute<'a, V: Visitor<'a>>(visitor: &mut V, attr: &'a Attribute) -> V::Result { - let Attribute { kind, id: _, style: _, span: _ } = attr; + let Attribute { kind, id: _, style: _, span: _, allowed_diagnostic_attribute: _ } = attr; match kind { AttrKind::Normal(normal) => { let NormalAttr { item, tokens: _ } = &**normal; diff --git a/compiler/rustc_ast_lowering/src/expr.rs b/compiler/rustc_ast_lowering/src/expr.rs index 124fe6bd380d2..246f8dc0bfcaf 100644 --- a/compiler/rustc_ast_lowering/src/expr.rs +++ b/compiler/rustc_ast_lowering/src/expr.rs @@ -733,6 +733,7 @@ impl<'hir> LoweringContext<'_, 'hir> { id: self.tcx.sess.psess.attr_id_generator.mk_attr_id(), style: AttrStyle::Outer, span: unstable_span, + allowed_diagnostic_attribute: false, }], ); } diff --git a/compiler/rustc_ast_lowering/src/lib.rs b/compiler/rustc_ast_lowering/src/lib.rs index 224787c335beb..eb71b42cd28e8 100644 --- a/compiler/rustc_ast_lowering/src/lib.rs +++ b/compiler/rustc_ast_lowering/src/lib.rs @@ -945,7 +945,13 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> { AttrKind::DocComment(comment_kind, data) => AttrKind::DocComment(comment_kind, data), }; - Attribute { kind, id: attr.id, style: attr.style, span: self.lower_span(attr.span) } + Attribute { + kind, + id: attr.id, + style: attr.style, + span: self.lower_span(attr.span), + allowed_diagnostic_attribute: attr.allowed_diagnostic_attribute, + } } fn alias_attrs(&mut self, id: HirId, target_id: HirId) { diff --git a/compiler/rustc_ast_passes/src/ast_validation.rs b/compiler/rustc_ast_passes/src/ast_validation.rs index a353c79f12d45..5f10b872b8a9f 100644 --- a/compiler/rustc_ast_passes/src/ast_validation.rs +++ b/compiler/rustc_ast_passes/src/ast_validation.rs @@ -20,6 +20,7 @@ use std::mem; use std::ops::{Deref, DerefMut}; use itertools::{Either, Itertools}; +use mut_visit::MutVisitor; use rustc_ast::ptr::P; use rustc_ast::visit::{walk_list, AssocCtxt, BoundKind, FnCtxt, FnKind, Visitor}; use rustc_ast::*; @@ -1781,3 +1782,32 @@ pub fn check_crate( validator.has_proc_macro_decls } + +struct MarkDiagnosticAttributesAsUnstable<'a> { + features: &'a Features, +} + +impl<'a> MutVisitor for MarkDiagnosticAttributesAsUnstable<'a> { + fn visit_attribute(&mut self, at: &mut Attribute) { + if at.is_doc_comment() { + return; + } + let diagnostic_item = match at.path().as_slice() { + [sym::diagnostic, item] => *item, + _ => return, + }; + match diagnostic_item { + sym::on_unimplemented => at.allowed_diagnostic_attribute = true, + sym::do_not_recommend => { + at.allowed_diagnostic_attribute = self.features.do_not_recommend + } + _ => {} + } + } +} + +pub fn apply_diagnostic_attribute_stablilty(features: &Features, krate: &mut Crate) { + let mut visitor = MarkDiagnosticAttributesAsUnstable { features }; + + mut_visit::walk_crate(&mut visitor, krate); +} diff --git a/compiler/rustc_interface/src/passes.rs b/compiler/rustc_interface/src/passes.rs index 96a6f52d60b6c..5ea81b405aeb5 100644 --- a/compiler/rustc_interface/src/passes.rs +++ b/compiler/rustc_interface/src/passes.rs @@ -237,6 +237,8 @@ fn configure_and_expand( rustc_builtin_macros::test_harness::inject(&mut krate, sess, features, resolver) }); + rustc_ast_passes::ast_validation::apply_diagnostic_attribute_stablilty(features, &mut krate); + let has_proc_macro_decls = sess.time("AST_validation", || { rustc_ast_passes::ast_validation::check_crate( sess, diff --git a/compiler/rustc_query_system/src/ich/impls_syntax.rs b/compiler/rustc_query_system/src/ich/impls_syntax.rs index 8d7a6e4fa9b0c..49f29ccc65fc3 100644 --- a/compiler/rustc_query_system/src/ich/impls_syntax.rs +++ b/compiler/rustc_query_system/src/ich/impls_syntax.rs @@ -41,7 +41,7 @@ impl<'ctx> rustc_ast::HashStableContext for StableHashingContext<'ctx> { debug_assert!(!attr.ident().is_some_and(|ident| self.is_ignored_attr(ident.name))); debug_assert!(!attr.is_doc_comment()); - let ast::Attribute { kind, id: _, style, span } = attr; + let ast::Attribute { kind, id: _, style, span, allowed_diagnostic_attribute: _ } = attr; if let ast::AttrKind::Normal(normal) = kind { normal.item.hash_stable(self, hasher); style.hash_stable(self, hasher); diff --git a/compiler/rustc_trait_selection/src/error_reporting/traits/fulfillment_errors.rs b/compiler/rustc_trait_selection/src/error_reporting/traits/fulfillment_errors.rs index 1cee82f04ea0e..b59c66ded0ed2 100644 --- a/compiler/rustc_trait_selection/src/error_reporting/traits/fulfillment_errors.rs +++ b/compiler/rustc_trait_selection/src/error_reporting/traits/fulfillment_errors.rs @@ -1630,11 +1630,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> { .tcx .all_impls(def_id) // ignore `do_not_recommend` items - .filter(|def_id| { - !self - .tcx - .has_attrs_with_path(*def_id, &[sym::diagnostic, sym::do_not_recommend]) - }) + .filter(|def_id| self.do_not_recommend_filter(*def_id)) // Ignore automatically derived impls and `!Trait` impls. .filter_map(|def_id| self.tcx.impl_trait_header(def_id)) .filter_map(|header| { @@ -1904,12 +1900,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> { let impl_candidates = impl_candidates .into_iter() .cloned() - .filter(|cand| { - !self.tcx.has_attrs_with_path( - cand.impl_def_id, - &[sym::diagnostic, sym::do_not_recommend], - ) - }) + .filter(|cand| self.do_not_recommend_filter(cand.impl_def_id)) .collect::>(); let def_id = trait_ref.def_id(); @@ -1953,6 +1944,12 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> { report(impl_candidates, err) } + fn do_not_recommend_filter(&self, def_id: DefId) -> bool { + let do_not_recommend = + self.tcx.get_attrs_by_path(def_id, &[sym::diagnostic, sym::do_not_recommend]).next(); + !matches!(do_not_recommend, Some(a) if a.allowed_diagnostic_attribute) + } + fn report_similar_impl_candidates_for_root_obligation( &self, obligation: &PredicateObligation<'tcx>, diff --git a/tests/ui/diagnostic_namespace/do_not_recommend/do_not_apply_attribute_without_feature_flag.rs b/tests/ui/diagnostic_namespace/do_not_recommend/do_not_apply_attribute_without_feature_flag.rs new file mode 100644 index 0000000000000..5548fa2f52e17 --- /dev/null +++ b/tests/ui/diagnostic_namespace/do_not_recommend/do_not_apply_attribute_without_feature_flag.rs @@ -0,0 +1,21 @@ +#![allow(unknown_or_malformed_diagnostic_attributes)] + +trait Foo {} + +#[diagnostic::do_not_recommend] +impl Foo for (A,) {} + +#[diagnostic::do_not_recommend] +impl Foo for (A, B) {} + +#[diagnostic::do_not_recommend] +impl Foo for (A, B, C) {} + +impl Foo for i32 {} + +fn check(a: impl Foo) {} + +fn main() { + check(()); + //~^ ERROR the trait bound `(): Foo` is not satisfied +} diff --git a/tests/ui/diagnostic_namespace/do_not_recommend/do_not_apply_attribute_without_feature_flag.stderr b/tests/ui/diagnostic_namespace/do_not_recommend/do_not_apply_attribute_without_feature_flag.stderr new file mode 100644 index 0000000000000..e56af28f3fb5d --- /dev/null +++ b/tests/ui/diagnostic_namespace/do_not_recommend/do_not_apply_attribute_without_feature_flag.stderr @@ -0,0 +1,21 @@ +error[E0277]: the trait bound `(): Foo` is not satisfied + --> $DIR/do_not_apply_attribute_without_feature_flag.rs:19:11 + | +LL | check(()); + | ----- ^^ the trait `Foo` is not implemented for `()` + | | + | required by a bound introduced by this call + | + = help: the following other types implement trait `Foo`: + (A, B) + (A, B, C) + (A,) +note: required by a bound in `check` + --> $DIR/do_not_apply_attribute_without_feature_flag.rs:16:18 + | +LL | fn check(a: impl Foo) {} + | ^^^ required by this bound in `check` + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0277`.