rework handling of doc attributes on macro calls

Author: mejrs

Cargo.lock

@@ -4001,7 +4001,6 @@ name = "rustc_expand"
 version = "0.0.0"
 dependencies = [
  "rustc_ast",
- "rustc_ast_passes",
  "rustc_ast_pretty",
  "rustc_attr_ir",
  "rustc_attr_parsing",

compiler/rustc_ast_lowering/src/lib.rs

@@ -45,7 +45,7 @@ use rustc_ast::mut_visit::{self, MutVisitor};
 use rustc_ast::node_id::NodeMap;
 use rustc_ast::visit::{self, Visitor};
 use rustc_ast::{self as ast, *};
-use rustc_attr_parsing::{AttributeParser, OmitDoc, Recovery, ShouldEmit};
+use rustc_attr_parsing::{AttributeParser, Recovery, ShouldEmit};
 use rustc_data_structures::fx::FxIndexMap;
 use rustc_data_structures::sorted_map::SortedMap;
 use rustc_data_structures::stable_hash::{StableHash, StableHasher};
@@ -1231,7 +1231,6 @@ impl<'hir> LoweringContext<'_, 'hir> {
             attrs,
             target_span,
             target,
-            OmitDoc::Lower,
             |s| l.lower(s),
             |lint_id, span, kind| {
                 self.delayed_lints.push(DelayedLint {

compiler/rustc_ast_passes/src/feature_gate.rs

@@ -46,10 +46,6 @@ macro_rules! gate_multi {
     }};
 }
 
-pub fn check_attribute(attr: &ast::Attribute, sess: &Session, features: &Features) {
-    PostExpansionVisitor { sess, features }.visit_attribute(attr)
-}
-
 struct PostExpansionVisitor<'a> {
     sess: &'a Session,
 
@@ -152,33 +148,9 @@ impl<'a> PostExpansionVisitor<'a> {
 }
 
 impl<'a> Visitor<'a> for PostExpansionVisitor<'a> {
-    fn visit_attribute(&mut self, attr: &ast::Attribute) {
-        // Check unstable flavors of the `#[doc]` attribute.
-        if attr.has_name(sym::doc) {
-            for meta_item_inner in attr.meta_item_list().unwrap_or_default() {
-                macro_rules! gate_doc { ($($s:literal { $($name:ident => $feature:ident)* })*) => {
-                    $($(if meta_item_inner.has_name(sym::$name) {
-                        let msg = concat!("`#[doc(", stringify!($name), ")]` is ", $s);
-                        gate!(self, $feature, attr.span, msg);
-                    })*)*
-                }}
-
-                gate_doc!(
-                    "experimental" {
-                        cfg => doc_cfg
-                        auto_cfg => doc_cfg
-                        masked => doc_masked
-                        notable_trait => doc_notable_trait
-                    }
-                    "meant for internal use only" {
-                        attribute => rustdoc_internals
-                        keyword => rustdoc_internals
-                        fake_variadic => rustdoc_internals
-                        search_unbox => rustdoc_internals
-                    }
-                );
-            }
-        }
+    fn visit_attribute(&mut self, attr: &'a ast::Attribute) {
+        // Checked in attribute parsers, do NOT add checks here
+        visit::walk_attribute(self, attr)
     }
 
     fn visit_item(&mut self, i: &'a ast::Item) {

compiler/rustc_attr_parsing/src/attributes/doc.rs

@@ -1,17 +1,19 @@
-use rustc_ast::ast::{AttrStyle, LitKind, MetaItemLit};
+use rustc_ast::ExprKind;
+use rustc_ast::ast::{self, AttrArgs, AttrKind, AttrStyle, LitKind, MetaItemLit};
 use rustc_attr_ir::target::Target;
 use rustc_attr_ir::{
     AttributeKind, CfgEntry, CfgHideShow, DocAttribute, DocCfgHideShow, DocCfgHideShowValue,
     DocInline, HideOrShow,
 };
 use rustc_data_structures::fx::{FxHashSet, FxIndexMap, IndexEntry};
-use rustc_errors::{Applicability, msg};
+use rustc_errors::{Applicability, Diagnostic, MultiSpan};
 use rustc_feature::AttributeStability;
-use rustc_session::diagnostics::feature_err;
+use rustc_lint_defs::LintId;
 use rustc_span::{Span, Symbol, edition, sym};
 
 use super::prelude::{ALL_TARGETS, AllowedTargets};
 use super::{AcceptMapping, AttributeParser, template};
+use crate::EmitAttribute;
 use crate::context::{AcceptContext, FinalizeContext};
 use crate::diagnostics::{
     AttrCrateLevelOnly, DocAliasBadChar, DocAliasDuplicated, DocAliasEmpty, DocAliasMalformed,
@@ -21,7 +23,7 @@ use crate::diagnostics::{
     DocAutoCfgHideShowValuesMix, DocAutoCfgWrongLiteral, DocKeywordNotKeyword, DocTestLiteral,
     DocTestTakesList, DocTestUnknown, DocUnknownAny, DocUnknownInclude, DocUnknownPasses,
     DocUnknownPlugins, DocUnknownSpotlight, ExpectedNameValue, ExpectedNoArgs,
-    IllFormedAttributeInput, MalformedDoc, UnusedDuplicate,
+    IllFormedAttributeInput, InvalidExprInDocAttr, MalformedDoc, UnusedDuplicate,
 };
 use crate::parser::{
     ArgParser, MetaItemListParser, MetaItemOrLitParser, MetaItemParser, OwnedPathParser,
@@ -549,19 +551,15 @@ impl DocParser {
         }
         macro_rules! no_args_and_crate_level {
             ($ident: ident) => {{
-                no_args_and_crate_level!($ident, |span| {});
-            }};
-            ($ident: ident, |$span:ident| $extra_validation:block) => {{
                 if let Err(span) = args.as_no_args() {
                     expected_no_args(cx, span);
                     return;
                 }
-                let $span = path.span();
-                if !check_attr_crate_level(cx, $span) {
+                let span = path.span();
+                if !check_attr_crate_level(cx, span) {
                     return;
                 }
-                $extra_validation
-                self.attribute.$ident = Some($span);
+                self.attribute.$ident = Some(span);
             }};
         }
         macro_rules! string_arg_and_crate_level {
@@ -592,6 +590,12 @@ impl DocParser {
                 self.attribute.$ident = Some((s, path.span()));
             }};
         }
+        macro_rules! gated {
+            ($feature:ident $(,$notes:expr)*) => {
+                let stability = $crate::unstable!($feature $(, $notes)*);
+                cx.shared.cx.check_attribute_stability(&cx.attr_path, path.span(), stability);
+            };
+        }
 
         match path.word_sym() {
             Some(sym::alias) => self.parse_alias(cx, path, args),
@@ -606,37 +610,60 @@ impl DocParser {
             }
             Some(sym::inline) => self.parse_inline(cx, path, args, DocInline::Inline),
             Some(sym::no_inline) => self.parse_inline(cx, path, args, DocInline::NoInline),
-            Some(sym::masked) => no_args!(masked),
-            Some(sym::cfg) => self.parse_cfg(cx, args),
-            Some(sym::notable_trait) => no_args!(notable_trait),
-            Some(sym::keyword) => parse_keyword_and_attribute(
-                cx,
-                path,
-                args,
-                &mut self.attribute.keyword,
-                sym::keyword,
-            ),
-            Some(sym::attribute) => parse_keyword_and_attribute(
-                cx,
-                path,
-                args,
-                &mut self.attribute.attribute,
-                sym::attribute,
-            ),
-            Some(sym::fake_variadic) => no_args_and_not_crate_level!(fake_variadic),
-            Some(sym::search_unbox) => no_args_and_not_crate_level!(search_unbox),
-            Some(sym::rust_logo) => no_args_and_crate_level!(rust_logo, |span| {
-                if !cx.features().rustdoc_internals() {
-                    feature_err(
-                        cx.sess(),
-                        sym::rustdoc_internals,
-                        span,
-                        msg!("the `#[doc(rust_logo)]` attribute is used for Rust branding"),
-                    )
-                    .emit();
+            Some(sym::masked) => {
+                gated!(doc_masked);
+                no_args!(masked)
+            }
+            Some(sym::cfg) => {
+                gated!(doc_cfg);
+                self.parse_cfg(cx, args)
+            }
+            Some(sym::notable_trait) => {
+                gated!(doc_notable_trait);
+                no_args!(notable_trait)
+            }
+            Some(sym::keyword) => {
+                gated!(rustdoc_internals);
+                parse_keyword_and_attribute(
+                    cx,
+                    path,
+                    args,
+                    &mut self.attribute.keyword,
+                    sym::keyword,
+                )
+            }
+            Some(sym::attribute) => {
+                gated!(rustdoc_internals);
+                parse_keyword_and_attribute(
+                    cx,
+                    path,
+                    args,
+                    &mut self.attribute.attribute,
+                    sym::attribute,
+                )
+            }
+            Some(sym::fake_variadic) => {
+                gated!(rustdoc_internals);
+                no_args_and_not_crate_level!(fake_variadic)
+            }
+            Some(sym::search_unbox) => {
+                gated!(rustdoc_internals);
+                no_args_and_not_crate_level!(search_unbox)
+            }
+            Some(sym::rust_logo) => {
+                // FIXME: Only feature gated at the crate level (!!)
+                if cx.target == Target::Crate {
+                    gated!(
+                        rustdoc_internals,
+                        "the `#[doc(rust_logo)]` attribute is used for Rust branding"
+                    );
                 }
-            }),
-            Some(sym::auto_cfg) => self.parse_auto_cfg(cx, path, args),
+                no_args_and_crate_level!(rust_logo)
+            }
+            Some(sym::auto_cfg) => {
+                gated!(doc_cfg);
+                self.parse_auto_cfg(cx, path, args)
+            }
             Some(sym::test) => {
                 let Some(list) = args.as_list() else {
                     cx.emit_lint(
@@ -842,3 +869,41 @@ impl AttributeParser for DocParser {
         }
     }
 }
+
+/// Is this a `#[doc = mac!()]`?
+///
+/// Or perhaps something as spicy as this?
+/// ```ignore,_
+/// #[doc = {
+///     let a = 1;
+///     let b = 1;
+///     let sum = a + b;
+///     assert_eq!(sum, 2);
+/// }]
+/// println!();
+/// ```
+pub(crate) fn lint_non_lit_doc_attr(
+    mut emit_lint: impl FnMut(LintId, MultiSpan, EmitAttribute),
+    attr: &ast::Attribute,
+) -> bool {
+    if !attr.has_name(sym::doc) {
+        return false;
+    }
+    let AttrKind::Normal(n) = &attr.kind else { return false };
+    let AttrArgs::Eq { expr, .. } = &n.item.args else { return false };
+    if matches!(expr.kind, ExprKind::Lit(_)) {
+        return false;
+    };
+
+    let attr_span = attr.span;
+    let expr_span = expr.span;
+
+    emit_lint(
+        LintId::of(rustc_session::lint::builtin::ILL_FORMED_ATTRIBUTE_INPUT),
+        attr_span.into(),
+        EmitAttribute(Box::new(move |dcx, level, _| {
+            InvalidExprInDocAttr { attr_span, expr_span }.into_diag(dcx, level)
+        })),
+    );
+    true
+}

compiler/rustc_attr_parsing/src/context.rs

@@ -865,12 +865,6 @@ impl<'p, 'sess: 'p> DerefMut for SharedContext<'p, 'sess> {
     }
 }
 
-#[derive(PartialEq, Clone, Copy, Debug)]
-pub enum OmitDoc {
-    Lower,
-    Skip,
-}
-
 #[derive(Copy, Clone, Debug)]
 pub enum ShouldEmit {
     /// The operations will emit errors, and lints, and errors are fatal.

compiler/rustc_attr_parsing/src/diagnostics.rs

@@ -1334,6 +1334,15 @@ pub(crate) enum InvalidTargetHelp {
     UseRustcAlignStatic,
 }
 
+#[derive(Diagnostic)]
+#[diag("invalid expression in `doc` attribute on macro invocation")]
+pub(crate) struct InvalidExprInDocAttr {
+    #[primary_span]
+    pub expr_span: Span,
+    #[suggestion("remove the attribute", code = "", applicability = "machine-applicable")]
+    pub attr_span: Span,
+}
+
 #[derive(Diagnostic)]
 #[diag("invalid alignment value: {$error_part}", code = E0589)]
 pub(crate) struct InvalidAlignmentValue {

compiler/rustc_attr_parsing/src/interface.rs

@@ -17,14 +17,15 @@ use rustc_session::lint::LintId;
 use rustc_span::{DUMMY_SP, ErrorGuaranteed, Span, Symbol, sym};
 
 use crate::attributes::AttributeSafety;
+use crate::attributes::doc::lint_non_lit_doc_attr;
 use crate::context::{
     ATTRIBUTE_PARSERS, AcceptContext, FinalizeCheckContext, FinalizeCheckFn, FinalizeContext,
     FinalizeFn, FinalizeOutput, SharedContext,
 };
 use crate::diagnostics::ParsedDescription;
 use crate::parser::{AllowExprMetavar, ArgParser, PathParser, RefPathParser};
 use crate::synthetic::SyntheticAttrState;
-use crate::{AttributeTemplate, OmitDoc, ShouldEmit};
+use crate::{AttributeTemplate, ShouldEmit};
 
 pub struct EmitAttribute(
     pub  Box<
@@ -162,7 +163,6 @@ impl<'sess> AttributeParser<'sess> {
             attrs,
             target_span,
             target,
-            OmitDoc::Skip,
             std::convert::identity,
             |lint_id, span, kind| {
                 sess.psess.dyn_buffer_lint_sess(lint_id.lint, span, target_node_id, kind.0)
@@ -311,14 +311,12 @@ impl<'sess> AttributeParser<'sess> {
 
     /// Parse a list of attributes.
     ///
-    /// `target_span` is the span of the thing this list of attributes is applied to,
-    /// and when `omit_doc` is set, doc attributes are filtered out.
+    /// `target_span` is the span of the thing this list of attributes is applied to.
     pub fn parse_attribute_list(
         &mut self,
         attrs: &[ast::Attribute],
         target_span: Span,
         target: Target,
-        omit_doc: OmitDoc,
         lower_span: impl Copy + Fn(Span) -> Span,
         mut emit_lint: impl FnMut(LintId, MultiSpan, EmitAttribute),
     ) -> Vec<Attribute> {
@@ -336,23 +334,14 @@ impl<'sess> AttributeParser<'sess> {
                 }
             }
 
-            // Sometimes, for example for `#![doc = include_str!("readme.md")]`,
-            // doc still contains a non-literal. You might say, when we're lowering attributes
-            // that's expanded right? But no, sometimes, when parsing attributes on macros,
-            // we already use the lowering logic and these are still there. So, when `omit_doc`
-            // is set we *also* want to ignore these.
-            let is_doc_attribute = attr.has_name(sym::doc);
-            if omit_doc == OmitDoc::Skip && is_doc_attribute {
+            // FIXME accidentally allowed on Stable Rust
+            if target == Target::MacroCall && lint_non_lit_doc_attr(&mut emit_lint, attr) {
                 continue;
             }
 
             let attr_span = lower_span(attr.span);
             match &attr.kind {
                 ast::AttrKind::DocComment(comment_kind, symbol) => {
-                    if omit_doc == OmitDoc::Skip {
-                        continue;
-                    }
-
                     attributes.push(Attribute::Parsed(AttributeKind::DocComment {
                         style: attr.style,
                         kind: DocFragmentKind::Sugared(*comment_kind),
@@ -408,7 +397,7 @@ impl<'sess> AttributeParser<'sess> {
                         // bla
                         // blob
                         // a
-                        if is_doc_attribute
+                        if attr.has_name(sym::doc)
                             && let ArgParser::NameValue(nv) = &args
                             // If not a string key/value, it should emit an error, but to make
                             // things simpler, it's handled in `DocParser` because it's simpler to

compiler/rustc_attr_parsing/src/lib.rs

@@ -116,7 +116,7 @@ pub use attributes::cfg::{
 };
 pub use attributes::cfg_select::*;
 pub use attributes::util::{is_builtin_attr, parse_version};
-pub use context::{OmitDoc, ShouldEmit};
+pub use context::ShouldEmit;
 pub use diagnostics::ParsedDescription;
 pub use interface::{AttributeParser, EmitAttribute};
 pub use rustc_parse::parser::Recovery;