Skip to content

Rework illformed attribute check for unparsed attributes #143963

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Rework illformed attribute check for unparsed attributes #143963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3321,6 +3321,7 @@ dependencies = [
"rustc_hir",
"rustc_lexer",
"rustc_macros",
"rustc_parse",
"rustc_session",
"rustc_span",
"thin-vec",
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_attr_parsing/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ rustc_fluent_macro = { path = "../rustc_fluent_macro" }
rustc_hir = { path = "../rustc_hir" }
rustc_lexer = { path = "../rustc_lexer" }
rustc_macros = { path = "../rustc_macros" }
rustc_parse = { path = "../rustc_parse" }
rustc_session = { path = "../rustc_session" }
rustc_span = { path = "../rustc_span" }
thin-vec = "0.2.12"
Expand Down
15 changes: 13 additions & 2 deletions compiler/rustc_attr_parsing/src/attributes/stability.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,12 @@ pub(crate) fn parse_stability<S: Stage>(
let mut feature = None;
let mut since = None;

for param in args.list()?.mixed() {
let ArgParser::List(list) = args else {
cx.expected_list(cx.attr_span);
return None;
};

for param in list.mixed() {
let param_span = param.span();
let Some(param) = param.meta_item() else {
cx.emit_err(session_diagnostics::UnsupportedLiteral {
Expand Down Expand Up @@ -312,7 +317,13 @@ pub(crate) fn parse_unstability<S: Stage>(
let mut is_soft = false;
let mut implied_by = None;
let mut old_name = None;
for param in args.list()?.mixed() {

let ArgParser::List(list) = args else {
cx.expected_list(cx.attr_span);
return None;
};

for param in list.mixed() {
let Some(param) = param.meta_item() else {
cx.emit_err(session_diagnostics::UnsupportedLiteral {
span: param.span(),
Expand Down
117 changes: 111 additions & 6 deletions compiler/rustc_attr_parsing/src/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ use private::Sealed;
use rustc_ast::{self as ast, LitKind, MetaItemLit, NodeId};
use rustc_attr_data_structures::AttributeKind;
use rustc_attr_data_structures::lints::{AttributeLint, AttributeLintKind};
use rustc_errors::{DiagCtxtHandle, Diagnostic};
use rustc_feature::{AttributeTemplate, Features};
use rustc_errors::{Applicability, DiagCtxtHandle, Diagnostic};
use rustc_feature::{AttributeTemplate, BUILTIN_ATTRIBUTE_MAP, BuiltinAttribute, Features};
use rustc_hir::{AttrArgs, AttrItem, AttrPath, Attribute, HashIgnoredAttrId, HirId};
use rustc_parse::validate_attr::{is_attr_template_compatible, parse_meta};
use rustc_session::Session;
use rustc_span::{DUMMY_SP, ErrorGuaranteed, Span, Symbol, sym};

Expand Down Expand Up @@ -197,7 +198,7 @@ mod private {
#[allow(private_interfaces)]
pub trait Stage: Sized + 'static + Sealed {
type Id: Copy;
const SHOULD_EMIT_LINTS: bool;
const IS_LATE: bool;

fn parsers() -> &'static group_type!(Self);

Expand All @@ -212,7 +213,7 @@ pub trait Stage: Sized + 'static + Sealed {
#[allow(private_interfaces)]
impl Stage for Early {
type Id = NodeId;
const SHOULD_EMIT_LINTS: bool = false;
const IS_LATE: bool = false;

fn parsers() -> &'static group_type!(Self) {
&early::ATTRIBUTE_PARSERS
Expand All @@ -234,7 +235,7 @@ impl Stage for Early {
#[allow(private_interfaces)]
impl Stage for Late {
type Id = HirId;
const SHOULD_EMIT_LINTS: bool = true;
const IS_LATE: bool = true;

fn parsers() -> &'static group_type!(Self) {
&late::ATTRIBUTE_PARSERS
Expand Down Expand Up @@ -284,7 +285,7 @@ impl<'f, 'sess: 'f, S: Stage> SharedContext<'f, 'sess, S> {
/// must be delayed until after HIR is built. This method will take care of the details of
/// that.
pub(crate) fn emit_lint(&mut self, lint: AttributeLintKind, span: Span) {
if !S::SHOULD_EMIT_LINTS {
if !S::IS_LATE {
return;
}
let id = self.target_id;
Expand Down Expand Up @@ -740,12 +741,25 @@ impl<'sess, S: Stage> AttributeParser<'sess, S> {
ast::AttrKind::Normal(n) => {
attr_paths.push(PathParser::Ast(&n.item.path));

// Parse attribute using new infra
let parser = MetaItemParser::from_attr(n, self.dcx());
let path = parser.path();
let args = parser.args();
let parts = path.segments().map(|i| i.name).collect::<Vec<_>>();

if let Some(accepts) = S::parsers().0.get(parts.as_slice()) {
//FIXME we call this to generate a few of the errors (such as invalid literals) that the new parses does not generate yet
//Remove this when possible

// rustc_dummy is not checked unless it is of the `Eq` arguments form
let skip_parse_meta_check = attr.has_name(sym::rustc_dummy)
&& !matches!(n.item.args, rustc_ast::AttrArgs::Eq { .. });
if !skip_parse_meta_check
&& let Err(err) = parse_meta(&self.sess.psess, attr)
{
err.emit();
}

for (template, accept) in accepts {
let mut cx: AcceptContext<'_, 'sess, S> = AcceptContext {
shared: SharedContext {
Expand Down Expand Up @@ -777,6 +791,46 @@ impl<'sess, S: Stage> AttributeParser<'sess, S> {
// "attribute {path} wasn't parsed and isn't a know tool attribute",
// );

if S::IS_LATE
&& !attr.has_name(sym::cfg_trace)
&& !attr.has_name(sym::cfg_attr_trace)
{
let builtin_attr_info = attr
.ident()
.and_then(|ident| BUILTIN_ATTRIBUTE_MAP.get(&ident.name));
if let Some(BuiltinAttribute { name, template, .. }) = builtin_attr_info
{
match parse_meta(&self.sess.psess, attr) {
// Don't check safety again, we just did that
Ok(meta) => {
if !is_attr_template_compatible(&template, &meta.kind) {
self.emit_malformed_unparsed_attribute(
attr.style,
meta.span,
*name,
*template,
target_id,
&mut emit_lint,
);
}
}
Err(err) => {
err.emit();
}
}
} else {
if let rustc_ast::AttrArgs::Eq { .. } = n.item.args {
// All key-value attributes are restricted to meta-item syntax.
match parse_meta(&self.sess.psess, attr) {
Ok(_) => {}
Err(err) => {
err.emit();
}
}
}
}
}

attributes.push(Attribute::Unparsed(Box::new(AttrItem {
path: AttrPath::from_ast(&n.item.path),
args: self.lower_attr_args(&n.item.args, lower_span),
Expand Down Expand Up @@ -809,6 +863,57 @@ impl<'sess, S: Stage> AttributeParser<'sess, S> {
attributes
}

fn emit_malformed_unparsed_attribute(
&self,
style: ast::AttrStyle,
span: Span,
name: Symbol,
template: AttributeTemplate,
target_id: S::Id,
emit_lint: &mut impl FnMut(AttributeLint<S::Id>),
) {
// Some of previously accepted forms were used in practice,
// report them as warnings for now.
let should_warn = |name| matches!(name, sym::doc | sym::link | sym::test | sym::bench);

let error_msg = format!("malformed `{name}` attribute input");
let mut suggestions = vec![];
let inner = if style == ast::AttrStyle::Inner { "!" } else { "" };
if template.word {
suggestions.push(format!("#{inner}[{name}]"));
}
if let Some(descr) = template.list {
suggestions.push(format!("#{inner}[{name}({descr})]"));
}
suggestions.extend(template.one_of.iter().map(|&word| format!("#{inner}[{name}({word})]")));
if let Some(descr) = template.name_value_str {
suggestions.push(format!("#{inner}[{name} = \"{descr}\"]"));
}
if should_warn(name) {
emit_lint(AttributeLint {
id: target_id,
span,
kind: AttributeLintKind::IllFormedAttributeInput { suggestions },
});
} else {
suggestions.sort();
self.sess
.dcx()
.struct_span_err(span, error_msg)
.with_span_suggestions(
span,
if suggestions.len() == 1 {
"must be of the form"
} else {
"the following are the possible correct uses"
},
suggestions,
Applicability::HasPlaceholders,
)
.emit();
}
}

/// Returns whether there is a parser for an attribute with this name
pub fn is_parsed_attribute(path: &[Symbol]) -> bool {
Late::parsers().0.contains_key(path)
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_builtin_macros/src/cfg_accessible.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ impl MultiItemModifier for Expander {
ast::AttrStyle::Outer,
sym::cfg_accessible,
template,
true,
);

let Some(path) = validate_input(ecx, meta_item) else {
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_builtin_macros/src/derive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ impl MultiItemModifier for Expander {
ast::AttrStyle::Outer,
sym::derive,
template,
true,
);

let mut resolutions = match &meta_item.kind {
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_builtin_macros/src/util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ pub(crate) fn check_builtin_macro_attribute(ecx: &ExtCtxt<'_>, meta_item: &MetaI
AttrStyle::Outer,
name,
template,
true,
);
}

Expand Down
93 changes: 3 additions & 90 deletions compiler/rustc_parse/src/validate_attr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use rustc_ast::{
Path, Safety,
};
use rustc_errors::{Applicability, DiagCtxtHandle, FatalError, PResult};
use rustc_feature::{AttributeSafety, AttributeTemplate, BUILTIN_ATTRIBUTE_MAP, BuiltinAttribute};
use rustc_feature::{AttributeSafety, AttributeTemplate, BUILTIN_ATTRIBUTE_MAP};
use rustc_session::errors::report_lit_error;
use rustc_session::lint::BuiltinLintDiag;
use rustc_session::lint::builtin::{ILL_FORMED_ATTRIBUTE_INPUT, UNSAFE_ATTR_OUTSIDE_UNSAFE};
Expand All @@ -26,34 +26,6 @@ pub fn check_attr(psess: &ParseSess, attr: &Attribute, id: NodeId) {

let builtin_attr_safety = builtin_attr_info.map(|x| x.safety);
check_attribute_safety(psess, builtin_attr_safety, attr, id);

// Check input tokens for built-in and key-value attributes.
match builtin_attr_info {
// `rustc_dummy` doesn't have any restrictions specific to built-in attributes.
Some(BuiltinAttribute { name, template, .. }) if *name != sym::rustc_dummy => {
match parse_meta(psess, attr) {
// Don't check safety again, we just did that
Ok(meta) => {
check_builtin_meta_item(psess, &meta, attr.style, *name, *template, false)
}
Err(err) => {
err.emit();
}
}
}
_ => {
let attr_item = attr.get_normal_item();
if let AttrArgs::Eq { .. } = attr_item.args {
// All key-value attributes are restricted to meta-item syntax.
match parse_meta(psess, attr) {
Ok(_) => {}
Err(err) => {
err.emit();
}
}
}
}
}
}

pub fn parse_meta<'a>(psess: &'a ParseSess, attr: &Attribute) -> PResult<'a, MetaItem> {
Expand Down Expand Up @@ -141,7 +113,7 @@ pub(super) fn check_cfg_attr_bad_delim(psess: &ParseSess, span: DelimSpan, delim
}

/// Checks that the given meta-item is compatible with this `AttributeTemplate`.
fn is_attr_template_compatible(template: &AttributeTemplate, meta: &ast::MetaItemKind) -> bool {
pub fn is_attr_template_compatible(template: &AttributeTemplate, meta: &ast::MetaItemKind) -> bool {
let is_one_allowed_subword = |items: &[MetaItemInner]| match items {
[item] => item.is_word() && template.one_of.iter().any(|&word| item.has_name(word)),
_ => false,
Expand Down Expand Up @@ -262,70 +234,11 @@ pub fn check_builtin_meta_item(
style: ast::AttrStyle,
name: Symbol,
template: AttributeTemplate,
deny_unsafety: bool,
) {
if !is_attr_template_compatible(&template, &meta.kind) {
// attrs with new parsers are locally validated so excluded here
if matches!(
name,
sym::inline
| sym::export_stable
| sym::ffi_const
| sym::ffi_pure
| sym::rustc_std_internal_symbol
| sym::may_dangle
| sym::rustc_as_ptr
| sym::rustc_pub_transparent
| sym::rustc_const_stable_indirect
| sym::rustc_force_inline
| sym::rustc_confusables
| sym::rustc_skip_during_method_dispatch
| sym::rustc_pass_by_value
| sym::rustc_deny_explicit_impl
| sym::rustc_do_not_implement_via_object
| sym::rustc_coinductive
| sym::const_trait
| sym::rustc_specialization_trait
| sym::rustc_unsafe_specialization_marker
| sym::rustc_allow_incoherent_impl
| sym::rustc_coherence_is_core
| sym::marker
| sym::fundamental
| sym::rustc_paren_sugar
| sym::type_const
| sym::repr
| sym::align
| sym::deprecated
| sym::optimize
| sym::cold
| sym::target_feature
| sym::rustc_allow_const_fn_unstable
| sym::naked
| sym::no_mangle
| sym::non_exhaustive
| sym::omit_gdb_pretty_printer_section
| sym::path
| sym::ignore
| sym::must_use
| sym::track_caller
| sym::link_name
| sym::link_ordinal
| sym::export_name
| sym::rustc_macro_transparency
| sym::link_section
| sym::rustc_layout_scalar_valid_range_start
| sym::rustc_layout_scalar_valid_range_end
| sym::no_implicit_prelude
| sym::automatically_derived
) {
return;
}
emit_malformed_attribute(psess, style, meta.span, name, template);
}

if deny_unsafety {
deny_builtin_meta_unsafety(psess.dcx(), meta.unsafety, &meta.path);
}
deny_builtin_meta_unsafety(psess.dcx(), meta.unsafety, &meta.path);
}

fn emit_malformed_attribute(
Expand Down
Loading
Loading