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: bevyengine/bevy#14591 (comment)

Author: weiznich

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)]

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,
     }
 }
 

compiler/rustc_ast/src/mut_visit.rs

@@ -631,7 +631,7 @@ fn walk_local<T: MutVisitor>(vis: &mut T, local: &mut P<Local>) {
 }
 
 fn walk_attribute<T: MutVisitor>(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 {

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;

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,
                 }],
             );
         }

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) {

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);
+}

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,

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);

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::<Vec<_>>();
 
         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>,

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<A> Foo for (A,) {}
+
+#[diagnostic::do_not_recommend]
+impl<A, B> Foo for (A, B) {}
+
+#[diagnostic::do_not_recommend]
+impl<A, B, C> 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
+}

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`.