Skip to content

Commit

Permalink
Add toggle for parse_meta_item unsafe parsing
Browse files Browse the repository at this point in the history
This makes it possible for the `unsafe(...)` syntax to only be
valid at the top level, and the `NestedMetaItem`s will automatically
reject `unsafe(...)`.
  • Loading branch information
carbotaniuman committed Jul 30, 2024
1 parent d8bc876 commit 5bd483d
Show file tree
Hide file tree
Showing 17 changed files with 217 additions and 95 deletions.
3 changes: 0 additions & 3 deletions compiler/rustc_builtin_macros/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,6 @@ builtin_macros_derive_path_args_list = traits in `#[derive(...)]` don't accept a
builtin_macros_derive_path_args_value = traits in `#[derive(...)]` don't accept values
.suggestion = remove the value
builtin_macros_derive_unsafe_path = traits in `#[derive(...)]` don't accept `unsafe(...)`
.suggestion = remove the `unsafe(...)`
builtin_macros_env_not_defined = environment variable `{$var}` not defined at compile time
.cargo = Cargo sets build script variables at run time. Use `std::env::var({$var_expr})` instead
.custom = use `std::env::var({$var_expr})` to read the variable at run time
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_builtin_macros/src/cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use rustc_ast::token;
use rustc_ast::tokenstream::TokenStream;
use rustc_errors::PResult;
use rustc_expand::base::{DummyResult, ExpandResult, ExtCtxt, MacEager, MacroExpanderResult};
use rustc_parse::parser::attr::AllowLeadingUnsafe;
use rustc_span::Span;
use {rustc_ast as ast, rustc_attr as attr};

Expand Down Expand Up @@ -42,7 +43,7 @@ fn parse_cfg<'a>(cx: &ExtCtxt<'a>, span: Span, tts: TokenStream) -> PResult<'a,
return Err(cx.dcx().create_err(errors::RequiresCfgPattern { span }));
}

let cfg = p.parse_meta_item()?;
let cfg = p.parse_meta_item(AllowLeadingUnsafe::Yes)?;

let _ = p.eat(&token::Comma);

Expand Down
13 changes: 1 addition & 12 deletions compiler/rustc_builtin_macros/src/derive.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use rustc_ast as ast;
use rustc_ast::{GenericParamKind, ItemKind, MetaItemKind, NestedMetaItem, Safety, StmtKind};
use rustc_ast::{GenericParamKind, ItemKind, MetaItemKind, NestedMetaItem, StmtKind};
use rustc_expand::base::{
Annotatable, DeriveResolution, ExpandResult, ExtCtxt, Indeterminate, MultiItemModifier,
};
Expand Down Expand Up @@ -62,7 +62,6 @@ impl MultiItemModifier for Expander {
// Reject `#[derive(Debug = "value", Debug(abc))]`, but recover the
// paths.
report_path_args(sess, meta);
report_unsafe_args(sess, meta);
meta.path.clone()
})
.map(|path| DeriveResolution {
Expand Down Expand Up @@ -162,13 +161,3 @@ fn report_path_args(sess: &Session, meta: &ast::MetaItem) {
}
}
}

fn report_unsafe_args(sess: &Session, meta: &ast::MetaItem) {
match meta.unsafety {
Safety::Unsafe(span) => {
sess.dcx().emit_err(errors::DeriveUnsafePath { span });
}
Safety::Default => {}
Safety::Safe(_) => unreachable!(),
}
}
7 changes: 0 additions & 7 deletions compiler/rustc_builtin_macros/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -297,13 +297,6 @@ pub(crate) struct DerivePathArgsValue {
pub(crate) span: Span,
}

#[derive(Diagnostic)]
#[diag(builtin_macros_derive_unsafe_path)]
pub(crate) struct DeriveUnsafePath {
#[primary_span]
pub(crate) span: Span,
}

#[derive(Diagnostic)]
#[diag(builtin_macros_no_default_variant)]
#[help]
Expand Down
5 changes: 3 additions & 2 deletions compiler/rustc_interface/src/interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use rustc_middle::ty;
use rustc_middle::ty::CurrentGcx;
use rustc_middle::util::Providers;
use rustc_parse::new_parser_from_source_str;
use rustc_parse::parser::attr::AllowLeadingUnsafe;
use rustc_query_impl::QueryCtxt;
use rustc_query_system::query::print_query_stack;
use rustc_session::config::{self, Cfg, CheckCfg, ExpectedValues, Input, OutFileName};
Expand Down Expand Up @@ -67,7 +68,7 @@ pub(crate) fn parse_cfg(dcx: DiagCtxtHandle<'_>, cfgs: Vec<String>) -> Cfg {
}

match new_parser_from_source_str(&psess, filename, s.to_string()) {
Ok(mut parser) => match parser.parse_meta_item() {
Ok(mut parser) => match parser.parse_meta_item(AllowLeadingUnsafe::No) {
Ok(meta_item) if parser.token == token::Eof => {
if meta_item.path.segments.len() != 1 {
error!("argument key must be an identifier");
Expand Down Expand Up @@ -173,7 +174,7 @@ pub(crate) fn parse_check_cfg(dcx: DiagCtxtHandle<'_>, specs: Vec<String>) -> Ch
}
};

let meta_item = match parser.parse_meta_item() {
let meta_item = match parser.parse_meta_item(AllowLeadingUnsafe::Yes) {
Ok(meta_item) if parser.token == token::Eof => meta_item,
Ok(..) => expected_error(),
Err(err) => {
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_parse/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_parse/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3188,6 +3188,7 @@ pub(crate) struct DotDotRangeAttribute {
#[note]
pub struct InvalidAttrUnsafe {
#[primary_span]
#[label]
pub span: Span,
pub name: Path,
}
Expand Down
21 changes: 17 additions & 4 deletions compiler/rustc_parse/src/parser/attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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);
Expand Down Expand Up @@ -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
}
Expand Down
82 changes: 41 additions & 41 deletions compiler/rustc_parse/src/validate_attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,53 +163,53 @@ pub fn check_attribute_safety(
safety: AttributeSafety,
attr: &Attribute,
) {
if features.unsafe_attributes {
let attr_item = attr.get_normal_item();
if !features.unsafe_attributes {
return;
}

if safety == AttributeSafety::Unsafe {
if let ast::Safety::Default = attr_item.unsafety {
let path_span = attr_item.path.span;
let attr_item = attr.get_normal_item();

// 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 safety == AttributeSafety::Unsafe {
if let ast::Safety::Default = attr_item.unsafety {
let path_span = attr_item.path.span;

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(),
// 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(),
});
}
}
}

Expand Down
8 changes: 7 additions & 1 deletion tests/ui/attributes/unsafe/derive-unsafe-attributes.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
#![feature(unsafe_attributes)]

#[derive(unsafe(Debug))] //~ ERROR: traits in `#[derive(...)]` don't accept `unsafe(...)`
#[derive(unsafe(Debug))]
//~^ ERROR: expected identifier, found keyword `unsafe`
//~| ERROR: traits in `#[derive(...)]` don't accept arguments
//~| ERROR: expected identifier, found keyword `unsafe`
//~| ERROR: expected identifier, found keyword `unsafe`
//~| ERROR: cannot find derive macro `r#unsafe` in this scope
//~| ERROR: cannot find derive macro `r#unsafe` in this scope
struct Foo;

#[unsafe(derive(Debug))] //~ ERROR: is not an unsafe attribute
Expand Down
59 changes: 54 additions & 5 deletions tests/ui/attributes/unsafe/derive-unsafe-attributes.stderr
Original file line number Diff line number Diff line change
@@ -1,16 +1,65 @@
error: traits in `#[derive(...)]` don't accept `unsafe(...)`
error: expected identifier, found keyword `unsafe`
--> $DIR/derive-unsafe-attributes.rs:3:10
|
LL | #[derive(unsafe(Debug))]
| ^^^^^^
| ^^^^^^ expected identifier, found keyword
|
help: escape `unsafe` to use it as an identifier
|
LL | #[derive(r#unsafe(Debug))]
| ++

error: traits in `#[derive(...)]` don't accept arguments
--> $DIR/derive-unsafe-attributes.rs:3:16
|
LL | #[derive(unsafe(Debug))]
| ^^^^^^^ help: remove the arguments

error: `derive` is not an unsafe attribute
--> $DIR/derive-unsafe-attributes.rs:6:3
--> $DIR/derive-unsafe-attributes.rs:12:3
|
LL | #[unsafe(derive(Debug))]
| ^^^^^^
| ^^^^^^ this is not an unsafe attribute
|
= note: extraneous unsafe is not allowed in attributes

error: aborting due to 2 previous errors
error: expected identifier, found keyword `unsafe`
--> $DIR/derive-unsafe-attributes.rs:3:10
|
LL | #[derive(unsafe(Debug))]
| ^^^^^^ expected identifier, found keyword
|
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
help: escape `unsafe` to use it as an identifier
|
LL | #[derive(r#unsafe(Debug))]
| ++

error: expected identifier, found keyword `unsafe`
--> $DIR/derive-unsafe-attributes.rs:3:10
|
LL | #[derive(unsafe(Debug))]
| ^^^^^^ expected identifier, found keyword
|
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
help: escape `unsafe` to use it as an identifier
|
LL | #[derive(r#unsafe(Debug))]
| ++

error: cannot find derive macro `r#unsafe` in this scope
--> $DIR/derive-unsafe-attributes.rs:3:10
|
LL | #[derive(unsafe(Debug))]
| ^^^^^^

error: cannot find derive macro `r#unsafe` in this scope
--> $DIR/derive-unsafe-attributes.rs:3:10
|
LL | #[derive(unsafe(Debug))]
| ^^^^^^
|
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`

error: aborting due to 7 previous errors

2 changes: 1 addition & 1 deletion tests/ui/attributes/unsafe/double-unsafe-attributes.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ error: `r#unsafe` is not an unsafe attribute
--> $DIR/double-unsafe-attributes.rs:3:3
|
LL | #[unsafe(unsafe(no_mangle))]
| ^^^^^^
| ^^^^^^ this is not an unsafe attribute
|
= note: extraneous unsafe is not allowed in attributes

Expand Down
Loading

0 comments on commit 5bd483d

Please sign in to comment.