Skip to content

add test to reproduce #137687 and fix it by converting #[crate_name] to a new-style attribute parser #137729

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions compiler/rustc_attr_parsing/src/attributes/crate_level.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
use rustc_feature::{AttributeTemplate, template};
use rustc_hir::attrs::AttributeKind;
use rustc_span::{Symbol, sym};

use super::{AttributeOrder, OnDuplicate, SingleAttributeParser};
use crate::context::{AcceptContext, Stage};
use crate::parser::ArgParser;

pub(crate) struct CrateNameParser;

impl<S: Stage> SingleAttributeParser<S> for CrateNameParser {
const PATH: &[Symbol] = &[sym::crate_name];
const ATTRIBUTE_ORDER: AttributeOrder = AttributeOrder::KeepOutermost;
const ON_DUPLICATE: OnDuplicate<S> = OnDuplicate::WarnButFutureError;
const TEMPLATE: AttributeTemplate = template!(NameValueStr: "name");

fn convert(cx: &mut AcceptContext<'_, '_, S>, args: &ArgParser<'_>) -> Option<AttributeKind> {
let ArgParser::NameValue(n) = args else {
cx.expected_name_value(cx.attr_span, None);
return None;
};

let Some(name) = n.value_as_str() else {
cx.expected_string_literal(n.value_span, Some(n.value_as_lit()));
return None;
};

Some(AttributeKind::CrateName { name, name_span: n.value_span, style: cx.attr_style })
}
}
1 change: 1 addition & 0 deletions compiler/rustc_attr_parsing/src/attributes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ pub(crate) mod cfg;
pub(crate) mod cfg_old;
pub(crate) mod codegen_attrs;
pub(crate) mod confusables;
pub(crate) mod crate_level;
pub(crate) mod deprecation;
pub(crate) mod dummy;
pub(crate) mod inline;
Expand Down
6 changes: 1 addition & 5 deletions compiler/rustc_attr_parsing/src/attributes/util.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use rustc_ast::attr::{AttributeExt, first_attr_value_str_by_name};
use rustc_ast::attr::AttributeExt;
use rustc_feature::is_builtin_attr_name;
use rustc_hir::RustcVersion;
use rustc_span::{Symbol, sym};
Expand All @@ -23,10 +23,6 @@ pub fn is_builtin_attr(attr: &impl AttributeExt) -> bool {
attr.is_doc_comment() || attr.ident().is_some_and(|ident| is_builtin_attr_name(ident.name))
}

pub fn find_crate_name(attrs: &[impl AttributeExt]) -> Option<Symbol> {
first_attr_value_str_by_name(attrs, sym::crate_name)
}

pub fn is_doc_alias_attrs_contain_symbol<'tcx, T: AttributeExt + 'tcx>(
attrs: impl Iterator<Item = &'tcx T>,
symbol: Symbol,
Expand Down
56 changes: 52 additions & 4 deletions compiler/rustc_attr_parsing/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@ use std::ops::{Deref, DerefMut};
use std::sync::LazyLock;

use private::Sealed;
use rustc_ast::{self as ast, LitKind, MetaItemLit, NodeId};
use rustc_ast::{self as ast, AttrStyle, LitKind, MetaItemLit, NodeId};
use rustc_errors::{DiagCtxtHandle, Diagnostic};
use rustc_feature::{AttributeTemplate, Features};
use rustc_hir::attrs::AttributeKind;
use rustc_hir::lints::{AttributeLint, AttributeLintKind};
use rustc_hir::{AttrArgs, AttrItem, AttrPath, Attribute, HashIgnoredAttrId, HirId};
use rustc_session::Session;
use rustc_session::lint::BuiltinLintDiag;
use rustc_span::{DUMMY_SP, ErrorGuaranteed, Span, Symbol, sym};

use crate::attributes::allow_unstable::{
Expand All @@ -22,6 +23,7 @@ use crate::attributes::codegen_attrs::{
TargetFeatureParser, TrackCallerParser, UsedParser,
};
use crate::attributes::confusables::ConfusablesParser;
use crate::attributes::crate_level::CrateNameParser;
use crate::attributes::deprecation::DeprecationParser;
use crate::attributes::dummy::DummyParser;
use crate::attributes::inline::{InlineParser, RustcForceInlineParser};
Expand Down Expand Up @@ -59,6 +61,7 @@ use crate::attributes::traits::{
};
use crate::attributes::transparency::TransparencyParser;
use crate::attributes::{AttributeParser as _, Combine, Single, WithoutArgs};
use crate::lints::lint_name;
use crate::parser::{ArgParser, MetaItemParser, PathParser};
use crate::session_diagnostics::{AttributeParseError, AttributeParseErrorReason, UnknownMetaItem};

Expand Down Expand Up @@ -157,6 +160,7 @@ attribute_parsers!(

// tidy-alphabetical-start
Single<CoverageParser>,
Single<CrateNameParser>,
Single<DeprecationParser>,
Single<DummyParser>,
Single<ExportNameParser>,
Expand Down Expand Up @@ -301,6 +305,9 @@ pub struct AcceptContext<'f, 'sess, S: Stage> {
/// The span of the attribute currently being parsed
pub(crate) attr_span: Span,

/// Whether it is an inner or outer attribute
pub(crate) attr_style: AttrStyle,

/// The expected structure of the attribute.
///
/// Used in reporting errors to give a hint to users what the attribute *should* look like.
Expand Down Expand Up @@ -382,6 +389,7 @@ impl<'f, 'sess: 'f, S: Stage> AcceptContext<'f, 'sess, S> {
i.kind.is_bytestr().then(|| self.sess().source_map().start_point(i.span))
}),
},
attr_style: self.attr_style,
})
}

Expand All @@ -392,6 +400,7 @@ impl<'f, 'sess: 'f, S: Stage> AcceptContext<'f, 'sess, S> {
template: self.template.clone(),
attribute: self.attr_path.clone(),
reason: AttributeParseErrorReason::ExpectedIntegerLiteral,
attr_style: self.attr_style,
})
}

Expand All @@ -402,6 +411,7 @@ impl<'f, 'sess: 'f, S: Stage> AcceptContext<'f, 'sess, S> {
template: self.template.clone(),
attribute: self.attr_path.clone(),
reason: AttributeParseErrorReason::ExpectedList,
attr_style: self.attr_style,
})
}

Expand All @@ -412,6 +422,7 @@ impl<'f, 'sess: 'f, S: Stage> AcceptContext<'f, 'sess, S> {
template: self.template.clone(),
attribute: self.attr_path.clone(),
reason: AttributeParseErrorReason::ExpectedNoArgs,
attr_style: self.attr_style,
})
}

Expand All @@ -423,6 +434,7 @@ impl<'f, 'sess: 'f, S: Stage> AcceptContext<'f, 'sess, S> {
template: self.template.clone(),
attribute: self.attr_path.clone(),
reason: AttributeParseErrorReason::ExpectedIdentifier,
attr_style: self.attr_style,
})
}

Expand All @@ -435,6 +447,7 @@ impl<'f, 'sess: 'f, S: Stage> AcceptContext<'f, 'sess, S> {
template: self.template.clone(),
attribute: self.attr_path.clone(),
reason: AttributeParseErrorReason::ExpectedNameValue(name),
attr_style: self.attr_style,
})
}

Expand All @@ -446,6 +459,7 @@ impl<'f, 'sess: 'f, S: Stage> AcceptContext<'f, 'sess, S> {
template: self.template.clone(),
attribute: self.attr_path.clone(),
reason: AttributeParseErrorReason::DuplicateKey(key),
attr_style: self.attr_style,
})
}

Expand All @@ -458,6 +472,7 @@ impl<'f, 'sess: 'f, S: Stage> AcceptContext<'f, 'sess, S> {
template: self.template.clone(),
attribute: self.attr_path.clone(),
reason: AttributeParseErrorReason::UnexpectedLiteral,
attr_style: self.attr_style,
})
}

Expand All @@ -468,6 +483,7 @@ impl<'f, 'sess: 'f, S: Stage> AcceptContext<'f, 'sess, S> {
template: self.template.clone(),
attribute: self.attr_path.clone(),
reason: AttributeParseErrorReason::ExpectedSingleArgument,
attr_style: self.attr_style,
})
}

Expand All @@ -478,6 +494,7 @@ impl<'f, 'sess: 'f, S: Stage> AcceptContext<'f, 'sess, S> {
template: self.template.clone(),
attribute: self.attr_path.clone(),
reason: AttributeParseErrorReason::ExpectedAtLeastOneArgument,
attr_style: self.attr_style,
})
}

Expand All @@ -496,6 +513,7 @@ impl<'f, 'sess: 'f, S: Stage> AcceptContext<'f, 'sess, S> {
strings: false,
list: false,
},
attr_style: self.attr_style,
})
}

Expand All @@ -514,6 +532,7 @@ impl<'f, 'sess: 'f, S: Stage> AcceptContext<'f, 'sess, S> {
strings: false,
list: true,
},
attr_style: self.attr_style,
})
}

Expand All @@ -532,6 +551,7 @@ impl<'f, 'sess: 'f, S: Stage> AcceptContext<'f, 'sess, S> {
strings: true,
list: false,
},
attr_style: self.attr_style,
})
}

Expand Down Expand Up @@ -675,22 +695,48 @@ impl<'sess> AttributeParser<'sess, Early> {
target_span: Span,
target_node_id: NodeId,
features: Option<&'sess Features>,
) -> Option<Attribute> {
Self::parse_limited_should_emit(
sess,
attrs,
sym,
target_span,
target_node_id,
features,
ShouldEmit::Nothing,
)
}

/// Usually you want `parse_limited`, which defaults to no errors.
pub fn parse_limited_should_emit(
sess: &'sess Session,
attrs: &[ast::Attribute],
sym: Symbol,
target_span: Span,
target_node_id: NodeId,
features: Option<&'sess Features>,
should_emit: ShouldEmit,
) -> Option<Attribute> {
let mut p = Self {
features,
tools: Vec::new(),
parse_only: Some(sym),
sess,
stage: Early { emit_errors: ShouldEmit::Nothing },
stage: Early { emit_errors: should_emit },
};
let mut parsed = p.parse_attribute_list(
attrs,
target_span,
target_node_id,
OmitDoc::Skip,
std::convert::identity,
|_lint| {
panic!("can't emit lints here for now (nothing uses this atm)");
|AttributeLint { id, span, kind }| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Since I lack a bigger picture of the new attr parsing APIs) Is it desirable that parse_limited{,_should_emit} default to lint buffering by default? Shouldn't the caller decide that (via a callback)?

For example, what happens if someone calls one of these functions in a later compiler stage when all buffered lints "have been emitted already"? Can that even happen or are buffered lints only emitted when the session is destroyed (I don't know much about this logic of buffer_lint)?

sess.psess.buffer_lint(
lint_name(&kind),
span,
id,
BuiltinLintDiag::AttributeLint(kind),
);
},
);
assert!(parsed.len() <= 1);
Expand Down Expand Up @@ -731,6 +777,7 @@ impl<'sess> AttributeParser<'sess, Early> {
},
},
attr_span: attr.span,
attr_style: attr.style,
template,
attr_path: path.get_attribute_path(),
};
Expand Down Expand Up @@ -840,6 +887,7 @@ impl<'sess, S: Stage> AttributeParser<'sess, S> {
emit_lint: &mut emit_lint,
},
attr_span: lower_span(attr.span),
attr_style: attr.style,
template: &accept.template,
attr_path: path.get_attribute_path(),
};
Expand Down
6 changes: 2 additions & 4 deletions compiler/rustc_attr_parsing/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,8 @@ mod session_diagnostics;

pub use attributes::cfg::{CFG_TEMPLATE, EvalConfigResult, eval_config_entry, parse_cfg_attr};
pub use attributes::cfg_old::*;
pub use attributes::util::{
find_crate_name, is_builtin_attr, is_doc_alias_attrs_contain_symbol, parse_version,
};
pub use attributes::util::{is_builtin_attr, is_doc_alias_attrs_contain_symbol, parse_version};
pub use context::{AttributeParser, Early, Late, OmitDoc, ShouldEmit};
pub use lints::emit_attribute_lint;
pub use lints::{emit_attribute_lint, emit_attribute_lint_kind};

rustc_fluent_macro::fluent_messages! { "../messages.ftl" }
62 changes: 35 additions & 27 deletions compiler/rustc_attr_parsing/src/lints.rs
Original file line number Diff line number Diff line change
@@ -1,38 +1,46 @@
use rustc_errors::{DiagArgValue, LintEmitter};
use rustc_errors::{DiagArgValue, LintEmitter, SimpleLintEmitter};
use rustc_hir::HirId;
use rustc_hir::lints::{AttributeLint, AttributeLintKind};
use rustc_session::lint::Lint;

use crate::session_diagnostics;

pub fn emit_attribute_lint<L: LintEmitter>(lint: &AttributeLint<HirId>, lint_emitter: L) {
let AttributeLint { id, span, kind } = lint;
pub(crate) fn lint_name(kind: &AttributeLintKind) -> &'static Lint {
use rustc_session::lint::builtin::*;
match kind {
AttributeLintKind::UnusedDuplicate { .. } => UNUSED_ATTRIBUTES,
AttributeLintKind::IllFormedAttributeInput { .. } => ILL_FORMED_ATTRIBUTE_INPUT,
AttributeLintKind::EmptyAttribute { .. } => UNUSED_ATTRIBUTES,
}
}

pub fn emit_attribute_lint_kind<L: SimpleLintEmitter>(kind: &AttributeLintKind, lint_emitter: L) {
match kind {
&AttributeLintKind::UnusedDuplicate { this, other, warning } => lint_emitter
.emit_node_span_lint(
rustc_session::lint::builtin::UNUSED_ATTRIBUTES,
*id,
*span,
session_diagnostics::UnusedDuplicate { this, other, warning },
),
&AttributeLintKind::UnusedDuplicate { this, other, warning } => {
lint_emitter.emit(session_diagnostics::UnusedDuplicate { this, other, warning })
}
AttributeLintKind::IllFormedAttributeInput { suggestions } => {
lint_emitter.emit_node_span_lint(
rustc_session::lint::builtin::ILL_FORMED_ATTRIBUTE_INPUT,
*id,
*span,
session_diagnostics::IllFormedAttributeInput {
num_suggestions: suggestions.len(),
suggestions: DiagArgValue::StrListSepByAnd(
suggestions.into_iter().map(|s| format!("`{s}`").into()).collect(),
),
},
);
lint_emitter.emit(session_diagnostics::IllFormedAttributeInput {
num_suggestions: suggestions.len(),
suggestions: DiagArgValue::StrListSepByAnd(
suggestions.into_iter().map(|s| format!("`{s}`").into()).collect(),
),
});
}
AttributeLintKind::EmptyAttribute { first_span } => {
lint_emitter.emit(session_diagnostics::EmptyAttributeList { attr_span: *first_span })
}
AttributeLintKind::EmptyAttribute { first_span } => lint_emitter.emit_node_span_lint(
rustc_session::lint::builtin::UNUSED_ATTRIBUTES,
*id,
*first_span,
session_diagnostics::EmptyAttributeList { attr_span: *first_span },
),
}
}

pub fn emit_attribute_lint<L: LintEmitter<ID = HirId>>(
lint: &AttributeLint<HirId>,
lint_emitter: L,
) {
let AttributeLint { id, span, kind } = lint;

let lint_name = lint_name(&lint.kind);

let emit = lint_emitter.prepare_node_span_lint(lint_name, *id, *span);
emit_attribute_lint_kind(kind, emit);
}
5 changes: 3 additions & 2 deletions compiler/rustc_attr_parsing/src/session_diagnostics.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::num::IntErrorKind;

use rustc_ast as ast;
use rustc_ast::{self as ast, AttrStyle};
use rustc_errors::codes::*;
use rustc_errors::{
Applicability, Diag, DiagArgValue, DiagCtxtHandle, Diagnostic, EmissionGuarantee, Level,
Expand Down Expand Up @@ -555,6 +555,7 @@ pub(crate) enum AttributeParseErrorReason {
pub(crate) struct AttributeParseError {
pub(crate) span: Span,
pub(crate) attr_span: Span,
pub(crate) attr_style: AttrStyle,
pub(crate) template: AttributeTemplate,
pub(crate) attribute: AttrPath,
pub(crate) reason: AttributeParseErrorReason,
Expand Down Expand Up @@ -690,7 +691,7 @@ impl<'a, G: EmissionGuarantee> Diagnostic<'a, G> for AttributeParseError {
}
}

let suggestions = self.template.suggestions(false, &name);
let suggestions = self.template.suggestions(self.attr_style == AttrStyle::Inner, &name);
diag.span_suggestions(
self.attr_span,
if suggestions.len() == 1 {
Expand Down
Loading
Loading