Address review comments

weiznich · compiler-errors · weiznich · commit e0d4bc06bebe · 2023-09-08T09:40:39.000+02:00
Co-authored-by: Michael Goulet &lt;michael@errs.io&gt;
diff --git a/compiler/rustc_ast/src/attr/mod.rs b/compiler/rustc_ast/src/attr/mod.rs
@@ -99,7 +99,7 @@ impl Attribute {
         }
     }
 
-    pub fn is_path(&self, name: &[Symbol]) -> bool {
+    pub fn path_matches(&self, name: &[Symbol]) -> bool {
         match &self.kind {
             AttrKind::Normal(normal) => {
                 normal.item.path.segments.len() == name.len()
@@ -109,7 +109,7 @@ impl Attribute {
                         .segments
                         .iter()
                         .zip(name)
-                        .all(|(s, n)| s.ident == Ident::with_dummy_span(*n))
+                        .all(|(s, n)| s.args.is_none() && s.ident.name == *n)
             }
             AttrKind::DocComment(..) => false,
         }
diff --git a/compiler/rustc_hir_analysis/messages.ftl b/compiler/rustc_hir_analysis/messages.ftl
@@ -57,9 +57,6 @@ hir_analysis_copy_impl_on_type_with_dtor =
     the trait `Copy` cannot be implemented for this type; the type has a destructor
     .label = `Copy` not allowed on types with destructors
 
-hir_analysis_diagnostic_on_unimplemented_only_for_traits =
-    `#[diagnostic::on_unimplemented]` can only be applied to trait definitions
-
 hir_analysis_drop_impl_negative = negative `Drop` impls are not supported
 
 hir_analysis_drop_impl_on_wrong_item =
diff --git a/compiler/rustc_hir_analysis/src/check/check.rs b/compiler/rustc_hir_analysis/src/check/check.rs
@@ -16,7 +16,6 @@ use rustc_infer::infer::outlives::env::OutlivesEnvironment;
 use rustc_infer::infer::{RegionVariableOrigin, TyCtxtInferExt};
 use rustc_infer::traits::{Obligation, TraitEngineExt as _};
 use rustc_lint_defs::builtin::REPR_TRANSPARENT_EXTERNAL_PRIVATE_FIELDS;
-use rustc_macros::LintDiagnostic;
 use rustc_middle::hir::nested_filter;
 use rustc_middle::middle::stability::EvalResult;
 use rustc_middle::traits::DefiningAnchor;
@@ -28,9 +27,7 @@ use rustc_middle::ty::{
     self, AdtDef, ParamEnv, RegionKind, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable,
     TypeVisitableExt,
 };
-use rustc_session::lint::builtin::{
-    UNINHABITED_STATIC, UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES, UNSUPPORTED_CALLING_CONVENTIONS,
-};
+use rustc_session::lint::builtin::{UNINHABITED_STATIC, UNSUPPORTED_CALLING_CONVENTIONS};
 use rustc_span::symbol::sym;
 use rustc_span::{self, Span};
 use rustc_target::abi::FieldIdx;
@@ -43,10 +40,6 @@ use rustc_type_ir::fold::TypeFoldable;
 
 use std::ops::ControlFlow;
 
-#[derive(LintDiagnostic)]
-#[diag(hir_analysis_diagnostic_on_unimplemented_only_for_traits)]
-pub struct DiagnosticOnUnimplementedOnlyForTraits;
-
 pub fn check_abi(tcx: TyCtxt<'_>, hir_id: hir::HirId, span: Span, abi: Abi) {
     match tcx.sess.target.is_abi_supported(abi) {
         Some(true) => (),
@@ -574,22 +567,7 @@ fn check_item_type(tcx: TyCtxt<'_>, id: hir::ItemId) {
         tcx.def_path_str(id.owner_id)
     );
     let _indenter = indenter();
-    let def_kind = tcx.def_kind(id.owner_id);
-    if !matches!(def_kind, DefKind::Trait) {
-        let item_def_id = id.owner_id.to_def_id();
-        if let Some(attr) = tcx
-            .get_attrs_by_path(item_def_id, Box::new([sym::diagnostic, sym::on_unimplemented]))
-            .next()
-        {
-            tcx.emit_spanned_lint(
-                UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES,
-                tcx.hir().local_def_id_to_hir_id(item_def_id.expect_local()),
-                vec![attr.span],
-                DiagnosticOnUnimplementedOnlyForTraits,
-            );
-        }
-    }
-    match def_kind {
+    match tcx.def_kind(id.owner_id) {
         DefKind::Static(..) => {
             tcx.ensure().typeck(id.owner_id.def_id);
             maybe_check_static_with_link_section(tcx, id.owner_id.def_id);
diff --git a/compiler/rustc_middle/src/ty/mod.rs b/compiler/rustc_middle/src/ty/mod.rs
@@ -2410,18 +2410,17 @@ impl<'tcx> TyCtxt<'tcx> {
         }
     }
 
-    pub fn get_attrs_by_path(
+    pub fn get_attrs_by_path<'attr>(
         self,
-        did: impl Into<DefId>,
-        attr: Box<[Symbol]>,
-    ) -> impl Iterator<Item = &'tcx ast::Attribute> {
-        let did: DefId = did.into();
-        let s = attr[0];
-        let filter_fn = move |a: &&ast::Attribute| a.is_path(&attr);
+        did: DefId,
+        attr: &'attr [Symbol],
+    ) -> impl Iterator<Item = &'tcx ast::Attribute> + 'attr
+    where
+        'tcx: 'attr,
+    {
+        let filter_fn = move |a: &&ast::Attribute| a.path_matches(&attr);
         if let Some(did) = did.as_local() {
             self.hir().attrs(self.hir().local_def_id_to_hir_id(did)).iter().filter(filter_fn)
-        } else if cfg!(debug_assertions) && rustc_feature::is_builtin_only_local(s) {
-            bug!("tried to access the `only_local` attribute `{}` from an extern crate", s);
         } else {
             self.item_attrs(did).iter().filter(filter_fn)
         }
diff --git a/compiler/rustc_passes/messages.ftl b/compiler/rustc_passes/messages.ftl
@@ -141,6 +141,9 @@ passes_deprecated_annotation_has_no_effect =
 passes_deprecated_attribute =
     deprecated attribute must be paired with either stable or unstable attribute
 
+passes_diagnostic_diagnostic_on_unimplemented_only_for_traits =
+    `#[diagnostic::on_unimplemented]` can only be applied to trait definitions
+
 passes_diagnostic_item_first_defined =
     the diagnostic item is first defined here
 
diff --git a/compiler/rustc_passes/src/check_attr.rs b/compiler/rustc_passes/src/check_attr.rs
@@ -16,6 +16,7 @@ use rustc_hir::{
     self, FnSig, ForeignItem, HirId, Item, ItemKind, TraitItem, CRATE_HIR_ID, CRATE_OWNER_ID,
 };
 use rustc_hir::{MethodKind, Target, Unsafety};
+use rustc_macros::LintDiagnostic;
 use rustc_middle::hir::nested_filter;
 use rustc_middle::middle::resolve_bound_vars::ObjectLifetimeDefault;
 use rustc_middle::query::Providers;
@@ -24,7 +25,7 @@ use rustc_middle::ty::error::{ExpectedFound, TypeError};
 use rustc_middle::ty::{self, TyCtxt};
 use rustc_session::lint::builtin::{
     CONFLICTING_REPR_HINTS, INVALID_DOC_ATTRIBUTES, INVALID_MACRO_EXPORT_ARGUMENTS,
-    UNUSED_ATTRIBUTES,
+    UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES, UNUSED_ATTRIBUTES,
 };
 use rustc_session::parse::feature_err;
 use rustc_span::symbol::{kw, sym, Symbol};
@@ -36,6 +37,10 @@ use rustc_trait_selection::traits::ObligationCtxt;
 use std::cell::Cell;
 use std::collections::hash_map::Entry;
 
+#[derive(LintDiagnostic)]
+#[diag(passes_diagnostic_diagnostic_on_unimplemented_only_for_traits)]
+pub struct DiagnosticOnUnimplementedOnlyForTraits;
+
 pub(crate) fn target_from_impl_item<'tcx>(
     tcx: TyCtxt<'tcx>,
     impl_item: &hir::ImplItem<'_>,
@@ -104,6 +109,9 @@ impl CheckAttrVisitor<'_> {
         let mut seen = FxHashMap::default();
         let attrs = self.tcx.hir().attrs(hir_id);
         for attr in attrs {
+            if attr.path_matches(&[sym::diagnostic, sym::on_unimplemented]) {
+                self.check_diagnostic_on_unimplemented(attr.span, hir_id, target);
+            }
             match attr.name_or_empty() {
                 sym::do_not_recommend => self.check_do_not_recommend(attr.span, target),
                 sym::inline => self.check_inline(hir_id, attr, span, target),
@@ -284,6 +292,18 @@ impl CheckAttrVisitor<'_> {
         }
     }
 
+    /// Checks if `#[diagnostic::on_unimplemented]` is applied to a trait definition
+    fn check_diagnostic_on_unimplemented(&self, attr_span: Span, hir_id: HirId, target: Target) {
+        if !matches!(target, Target::Trait) {
+            self.tcx.emit_spanned_lint(
+                UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES,
+                hir_id,
+                attr_span,
+                DiagnosticOnUnimplementedOnlyForTraits,
+            );
+        }
+    }
+
     /// Checks if an `#[inline]` is applied to a function or a closure. Returns `true` if valid.
     fn check_inline(&self, hir_id: HirId, attr: &Attribute, span: Span, target: Target) -> bool {
         match target {
diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/on_unimplemented.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/on_unimplemented.rs
@@ -411,7 +411,7 @@ impl<'tcx> OnUnimplementedDirective {
                 && label.is_none()
                 && note.is_none()
                 && !is_diagnostic_namespace_variant
-            // disallow filters for now
+            // FIXME(diagnostic_namespace): disallow filters for now
             {
                 if let Some(items) = item.meta_item_list() {
                     match Self::parse(
@@ -423,7 +423,7 @@ impl<'tcx> OnUnimplementedDirective {
                         is_diagnostic_namespace_variant,
                     ) {
                         Ok(Some(subcommand)) => subcommands.push(subcommand),
-                        Ok(None) => unreachable!(
+                        Ok(None) => bug!(
                             "This cannot happen for now as we only reach that if `is_diagnostic_namespace_variant` is false"
                         ),
                         Err(reported) => errored = Some(reported),
@@ -476,11 +476,7 @@ impl<'tcx> OnUnimplementedDirective {
         let Some(attr) = tcx.get_attr(item_def_id, sym::rustc_on_unimplemented).or_else(|| {
             if tcx.features().diagnostic_namespace {
                 is_diagnostic_namespace_variant = true;
-                tcx.get_attrs_by_path(
-                    item_def_id,
-                    Box::new([sym::diagnostic, sym::on_unimplemented]),
-                )
-                .next()
+                tcx.get_attrs_by_path(item_def_id, &[sym::diagnostic, sym::on_unimplemented]).next()
             } else {
                 None
             }
@@ -510,7 +506,7 @@ impl<'tcx> OnUnimplementedDirective {
                 tcx.emit_spanned_lint(
                     UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES,
                     tcx.hir().local_def_id_to_hir_id(item_def_id.expect_local()),
-                    vec![attr.span],
+                    attr.span,
                     NoValueInOnUnimplementedLint,
                 );
                 Ok(None)
@@ -519,7 +515,7 @@ impl<'tcx> OnUnimplementedDirective {
             tcx.emit_spanned_lint(
                 UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES,
                 tcx.hir().local_def_id_to_hir_id(item_def_id.expect_local()),
-                vec![attr.span],
+                attr.span,
                 NoValueInOnUnimplementedLint,
             );
             Ok(None)
diff --git a/tests/ui/diagnostic_namespace/on_unimplemented/do_not_fail_parsing_on_invalid_options_1.stderr b/tests/ui/diagnostic_namespace/on_unimplemented/do_not_fail_parsing_on_invalid_options_1.stderr
@@ -1,16 +1,16 @@
-warning: malformed `on_unimplemented` attribute
-  --> $DIR/do_not_fail_parsing_on_invalid_options_1.rs:3:32
-   |
-LL | #[diagnostic::on_unimplemented(unsupported = "foo")]
-   |                                ^^^^^^^^^^^^^^^^^^^
-   |
-   = note: `#[warn(unknown_or_malformed_diagnostic_attributes)]` on by default
-
 warning: `#[diagnostic::on_unimplemented]` can only be applied to trait definitions
   --> $DIR/do_not_fail_parsing_on_invalid_options_1.rs:8:1
    |
 LL | #[diagnostic::on_unimplemented(message = "Baz")]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `#[warn(unknown_or_malformed_diagnostic_attributes)]` on by default
+
+warning: malformed `on_unimplemented` attribute
+  --> $DIR/do_not_fail_parsing_on_invalid_options_1.rs:3:32
+   |
+LL | #[diagnostic::on_unimplemented(unsupported = "foo")]
+   |                                ^^^^^^^^^^^^^^^^^^^
 
 warning: malformed `on_unimplemented` attribute
   --> $DIR/do_not_fail_parsing_on_invalid_options_1.rs:12:50

Original file line number	Diff line number	Diff line change
`@@ -99,7 +99,7 @@ impl Attribute {`
`99`	`99`	`}`
`100`	`100`	`}`
`101`	`101`
`102`		`- pub fn is_path(&self, name: &[Symbol]) -> bool {`
	`102`	`+ pub fn path_matches(&self, name: &[Symbol]) -> bool {`
`103`	`103`	`match &self.kind {`
`104`	`104`	`AttrKind::Normal(normal) => {`
`105`	`105`	`normal.item.path.segments.len() == name.len()`
`@@ -109,7 +109,7 @@ impl Attribute {`
`109`	`109`	`.segments`
`110`	`110`	`.iter()`
`111`	`111`	`.zip(name)`
`112`		`- .all(\|(s, n)\| s.ident == Ident::with_dummy_span(*n))`
	`112`	`+ .all(\|(s, n)\| s.args.is_none() && s.ident.name == *n)`
`113`	`113`	`}`
`114`	`114`	`AttrKind::DocComment(..) => false,`
`115`	`115`	`}`