Skip to content

Commit

Permalink
Auto merge of rust-lang#127543 - carbotaniuman:more_unsafe_attr_verif…
Browse files Browse the repository at this point in the history
…ication, r=estebank,traviscross

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:

- rust-lang#123757
  • Loading branch information
bors committed Aug 1, 2024
2 parents 97ac52f + 49db8a5 commit c0e3298
Show file tree
Hide file tree
Showing 22 changed files with 471 additions and 92 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
2 changes: 2 additions & 0 deletions compiler/rustc_builtin_macros/src/cfg_accessible.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,13 @@ impl MultiItemModifier for Expander {
) -> ExpandResult<Vec<Annotatable>, Annotatable> {
let template = AttributeTemplate { list: Some("path"), ..Default::default() };
validate_attr::check_builtin_meta_item(
&ecx.ecfg.features,
&ecx.sess.psess,
meta_item,
ast::AttrStyle::Outer,
sym::cfg_accessible,
template,
true,
);

let Some(path) = validate_input(ecx, meta_item) else {
Expand Down
15 changes: 3 additions & 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 @@ -38,11 +38,13 @@ impl MultiItemModifier for Expander {
let template =
AttributeTemplate { list: Some("Trait1, Trait2, ..."), ..Default::default() };
validate_attr::check_builtin_meta_item(
features,
&sess.psess,
meta_item,
ast::AttrStyle::Outer,
sym::derive,
template,
true,
);

let mut resolutions = match &meta_item.kind {
Expand All @@ -60,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 @@ -160,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
2 changes: 2 additions & 0 deletions compiler/rustc_builtin_macros/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@ pub(crate) fn check_builtin_macro_attribute(ecx: &ExtCtxt<'_>, meta_item: &MetaI
// All the built-in macro attributes are "words" at the moment.
let template = AttributeTemplate { word: true, ..Default::default() };
validate_attr::check_builtin_meta_item(
&ecx.ecfg.features,
&ecx.sess.psess,
meta_item,
AttrStyle::Outer,
name,
template,
true,
);
}

Expand Down
18 changes: 17 additions & 1 deletion compiler/rustc_expand/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ use rustc_ast::tokenstream::{
use rustc_ast::{self as ast, AttrStyle, Attribute, HasAttrs, HasTokens, MetaItem, NodeId};
use rustc_attr as attr;
use rustc_data_structures::flat_map_in_place::FlatMapInPlace;
use rustc_feature::{Features, ACCEPTED_FEATURES, REMOVED_FEATURES, UNSTABLE_FEATURES};
use rustc_feature::{
AttributeSafety, Features, ACCEPTED_FEATURES, REMOVED_FEATURES, UNSTABLE_FEATURES,
};
use rustc_lint_defs::BuiltinLintDiag;
use rustc_parse::validate_attr;
use rustc_session::parse::feature_err;
Expand Down Expand Up @@ -263,6 +265,13 @@ impl<'a> StripUnconfigured<'a> {
/// is in the original source file. Gives a compiler error if the syntax of
/// the attribute is incorrect.
pub(crate) fn expand_cfg_attr(&self, cfg_attr: &Attribute, recursive: bool) -> Vec<Attribute> {
validate_attr::check_attribute_safety(
self.features.unwrap_or(&Features::default()),
&self.sess.psess,
AttributeSafety::Normal,
&cfg_attr,
);

let Some((cfg_predicate, expanded_attrs)) =
rustc_parse::parse_cfg_attr(cfg_attr, &self.sess.psess)
else {
Expand Down Expand Up @@ -385,6 +394,13 @@ impl<'a> StripUnconfigured<'a> {
return (true, None);
}
};

validate_attr::deny_builtin_meta_unsafety(
self.features.unwrap_or(&Features::default()),
&self.sess.psess,
&meta_item,
);

(
parse_cfg(&meta_item, self.sess).map_or(true, |meta_item| {
attr::cfg_matches(meta_item, &self.sess, self.lint_node_id, self.features)
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 @@ -3183,6 +3183,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
Loading

0 comments on commit c0e3298

Please sign in to comment.