bjoernager/rust
bjoernager/rust
1
Fork 0
Code Activity

Auto merge of #127543 - carbotaniuman:more_unsafe_attr_verification, r=estebank,traviscross

Browse source 
More unsafe attr verification

This code denies unsafe on attributes such as `#[test]` and `#[ignore]`, while also changing the `MetaItem` parsing so `unsafe` in args like `#[allow(unsafe(dead_code))]` is not accidentally allowed.

Tracking:

- https://github.com/rust-lang/rust/issues/123757
This commit is contained in:
bors 2024-08-01 10:40:45 +00:00
commit c0e32983f5
22 changed files with 471 additions and 92 deletions
Download patch file Download diff file Expand all files Collapse all files

1
compiler/rustc_parse/messages.ftl
View file

@ -365,6 +365,7 @@ parse_inner_doc_comment_not_permitted = expected outer doc comment
.sugg_change_inner_to_outer = to annotate the {$item}, change the doc comment from inner to outer style
parse_invalid_attr_unsafe = `{$name}` is not an unsafe attribute
.label = this is not an unsafe attribute
.suggestion = remove the `unsafe(...)`
.note = extraneous unsafe is not allowed in attributes

1
compiler/rustc_parse/src/errors.rs
View file

@ -3183,6 +3183,7 @@ pub(crate) struct DotDotRangeAttribute {
#[note]
pub struct InvalidAttrUnsafe {
#[primary_span]
#[label]
pub span: Span,
pub name: Path,
}

21
compiler/rustc_parse/src/parser/attr.rs
View file

@ -31,6 +31,12 @@ enum OuterAttributeType {
Attribute,
}
#[derive(Clone, Copy, PartialEq, Eq)]
pub enum AllowLeadingUnsafe {
Yes,
No,
}
impl<'a> Parser<'a> {
/// Parses attributes that appear before an item.
pub(super) fn parse_outer_attributes(&mut self) -> PResult<'a, AttrWrapper> {
@ -332,7 +338,7 @@ impl<'a> Parser<'a> {
/// Parses `cfg_attr(pred, attr_item_list)` where `attr_item_list` is comma-delimited.
pub fn parse_cfg_attr(&mut self) -> PResult<'a, (ast::MetaItem, Vec<(ast::AttrItem, Span)>)> {
let cfg_predicate = self.parse_meta_item()?;
let cfg_predicate = self.parse_meta_item(AllowLeadingUnsafe::No)?;
self.expect(&token::Comma)?;
// Presumably, the majority of the time there will only be one attr.
@ -368,7 +374,10 @@ impl<'a> Parser<'a> {
/// MetaItem = SimplePath ( '=' UNSUFFIXED_LIT | '(' MetaSeq? ')' )? ;
/// MetaSeq = MetaItemInner (',' MetaItemInner)* ','? ;
/// ```
pub fn parse_meta_item(&mut self) -> PResult<'a, ast::MetaItem> {
pub fn parse_meta_item(
&mut self,
unsafe_allowed: AllowLeadingUnsafe,
) -> PResult<'a, ast::MetaItem> {
// We can't use `maybe_whole` here because it would bump in the `None`
// case, which we don't want.
if let token::Interpolated(nt) = &self.token.kind
@ -384,7 +393,11 @@ impl<'a> Parser<'a> {
}
let lo = self.token.span;
let is_unsafe = self.eat_keyword(kw::Unsafe);
let is_unsafe = if unsafe_allowed == AllowLeadingUnsafe::Yes {
self.eat_keyword(kw::Unsafe)
} else {
false
};
let unsafety = if is_unsafe {
let unsafe_span = self.prev_token.span;
self.psess.gated_spans.gate(sym::unsafe_attributes, unsafe_span);
@ -427,7 +440,7 @@ impl<'a> Parser<'a> {
Err(err) => err.cancel(), // we provide a better error below
}
match self.parse_meta_item() {
match self.parse_meta_item(AllowLeadingUnsafe::No) {
Ok(mi) => return Ok(ast::NestedMetaItem::MetaItem(mi)),
Err(err) => err.cancel(), // we provide a better error below
}

148
compiler/rustc_parse/src/validate_attr.rs
View file

@ -26,76 +26,35 @@ pub fn check_attr(features: &Features, psess: &ParseSess, attr: &Attribute) {
let attr_info = attr.ident().and_then(|ident| BUILTIN_ATTRIBUTE_MAP.get(&ident.name));
let attr_item = attr.get_normal_item();
let is_unsafe_attr = attr_info.is_some_and(|attr| attr.safety == AttributeSafety::Unsafe);
if features.unsafe_attributes {
if is_unsafe_attr {
if let ast::Safety::Default = attr_item.unsafety {
let path_span = attr_item.path.span;
// If the `attr_item`'s span is not from a macro, then just suggest
// wrapping it in `unsafe(...)`. Otherwise, we suggest putting the
// `unsafe(`, `)` right after and right before the opening and closing
// square bracket respectively.
let diag_span = if attr_item.span().can_be_used_for_suggestions() {
attr_item.span()
} else {
attr.span
.with_lo(attr.span.lo() + BytePos(2))
.with_hi(attr.span.hi() - BytePos(1))
};
if attr.span.at_least_rust_2024() {
psess.dcx().emit_err(errors::UnsafeAttrOutsideUnsafe {
span: path_span,
suggestion: errors::UnsafeAttrOutsideUnsafeSuggestion {
left: diag_span.shrink_to_lo(),
right: diag_span.shrink_to_hi(),
},
});
} else {
psess.buffer_lint(
UNSAFE_ATTR_OUTSIDE_UNSAFE,
path_span,
ast::CRATE_NODE_ID,
BuiltinLintDiag::UnsafeAttrOutsideUnsafe {
attribute_name_span: path_span,
sugg_spans: (diag_span.shrink_to_lo(), diag_span.shrink_to_hi()),
},
);
}
}
} else {
if let Safety::Unsafe(unsafe_span) = attr_item.unsafety {
psess.dcx().emit_err(errors::InvalidAttrUnsafe {
span: unsafe_span,
name: attr_item.path.clone(),
});
}
}
}
// All non-builtin attributes are considered safe
let safety = attr_info.map(|x| x.safety).unwrap_or(AttributeSafety::Normal);
check_attribute_safety(features, psess, safety, attr);
// Check input tokens for built-in and key-value attributes.
match 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) {
Ok(meta) => check_builtin_meta_item(psess, &meta, attr.style, *name, *template),
// Don't check safety again, we just did that
Ok(meta) => check_builtin_meta_item(
features, psess, &meta, attr.style, *name, *template, false,
),
Err(err) => {
err.emit();
}
}
}
_ 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();
_ => {
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();
}
}
}
}
_ => {}
}
}
@ -198,12 +157,85 @@ fn is_attr_template_compatible(template: &AttributeTemplate, meta: &ast::MetaIte
}
}
pub fn check_attribute_safety(
features: &Features,
psess: &ParseSess,
safety: AttributeSafety,
attr: &Attribute,
) {
if !features.unsafe_attributes {
return;
}
let attr_item = attr.get_normal_item();
if safety == AttributeSafety::Unsafe {
if let ast::Safety::Default = attr_item.unsafety {
let path_span = attr_item.path.span;
// If the `attr_item`'s span is not from a macro, then just suggest
// wrapping it in `unsafe(...)`. Otherwise, we suggest putting the
// `unsafe(`, `)` right after and right before the opening and closing
// square bracket respectively.
let diag_span = if attr_item.span().can_be_used_for_suggestions() {
attr_item.span()
} else {
attr.span.with_lo(attr.span.lo() + BytePos(2)).with_hi(attr.span.hi() - BytePos(1))
};
if attr.span.at_least_rust_2024() {
psess.dcx().emit_err(errors::UnsafeAttrOutsideUnsafe {
span: path_span,
suggestion: errors::UnsafeAttrOutsideUnsafeSuggestion {
left: diag_span.shrink_to_lo(),
right: diag_span.shrink_to_hi(),
},
});
} else {
psess.buffer_lint(
UNSAFE_ATTR_OUTSIDE_UNSAFE,
path_span,
ast::CRATE_NODE_ID,
BuiltinLintDiag::UnsafeAttrOutsideUnsafe {
attribute_name_span: path_span,
sugg_spans: (diag_span.shrink_to_lo(), diag_span.shrink_to_hi()),
},
);
}
}
} else {
if let Safety::Unsafe(unsafe_span) = attr_item.unsafety {
psess.dcx().emit_err(errors::InvalidAttrUnsafe {
span: unsafe_span,
name: attr_item.path.clone(),
});
}
}
}
// Called by `check_builtin_meta_item` and code that manually denies
// `unsafe(...)` in `cfg`
pub fn deny_builtin_meta_unsafety(features: &Features, psess: &ParseSess, meta: &MetaItem) {
// This only supports denying unsafety right now - making builtin attributes
// support unsafety will requite us to thread the actual `Attribute` through
// for the nice diagnostics.
if features.unsafe_attributes {
if let Safety::Unsafe(unsafe_span) = meta.unsafety {
psess
.dcx()
.emit_err(errors::InvalidAttrUnsafe { span: unsafe_span, name: meta.path.clone() });
}
}
}
pub fn check_builtin_meta_item(
features: &Features,
psess: &ParseSess,
meta: &MetaItem,
style: ast::AttrStyle,
name: Symbol,
template: AttributeTemplate,
deny_unsafety: bool,
) {
// Some special attributes like `cfg` must be checked
// before the generic check, so we skip them here.
@ -212,6 +244,10 @@ pub fn check_builtin_meta_item(
if !should_skip(name) && !is_attr_template_compatible(&template, &meta.kind) {
emit_malformed_attribute(psess, style, meta.span, name, template);
}
if deny_unsafety {
deny_builtin_meta_unsafety(features, psess, meta);
}
}
fn emit_malformed_attribute(