Skip to content

Commit

Permalink
Add hard error and migration lint for unsafe attrs
Browse files Browse the repository at this point in the history
  • Loading branch information
carbotaniuman committed Jun 24, 2024
1 parent 33422e7 commit a23917c
Show file tree
Hide file tree
Showing 24 changed files with 473 additions and 53 deletions.
2 changes: 1 addition & 1 deletion compiler/rustc_ast_passes/src/ast_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -899,7 +899,7 @@ fn validate_generic_param_order(dcx: DiagCtxtHandle<'_>, generics: &[GenericPara

impl<'a> Visitor<'a> for AstValidator<'a> {
fn visit_attribute(&mut self, attr: &Attribute) {
validate_attr::check_attr(&self.session.psess, attr);
validate_attr::check_attr(&self.features, &self.session.psess, attr);
}

fn visit_ty(&mut self, ty: &'a Ty) {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_expand/src/expand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1882,7 +1882,7 @@ impl<'a, 'b> InvocationCollector<'a, 'b> {
let mut span: Option<Span> = None;
while let Some(attr) = attrs.next() {
rustc_ast_passes::feature_gate::check_attribute(attr, self.cx.sess, features);
validate_attr::check_attr(&self.cx.sess.psess, attr);
validate_attr::check_attr(features, &self.cx.sess.psess, attr);

let current_span = if let Some(sp) = span { sp.to(attr.span) } else { attr.span };
span = Some(current_span);
Expand Down
4 changes: 0 additions & 4 deletions compiler/rustc_feature/src/builtin_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1145,10 +1145,6 @@ pub fn is_valid_for_get_attr(name: Symbol) -> bool {
})
}

pub fn is_unsafe_attr(name: Symbol) -> bool {
BUILTIN_ATTRIBUTE_MAP.get(&name).is_some_and(|attr| attr.safety == AttributeSafety::Unsafe)
}

pub static BUILTIN_ATTRIBUTE_MAP: LazyLock<FxHashMap<Symbol, &BuiltinAttribute>> =
LazyLock::new(|| {
let mut map = FxHashMap::default();
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_feature/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ pub use accepted::ACCEPTED_FEATURES;
pub use builtin_attrs::AttributeDuplicates;
pub use builtin_attrs::{
deprecated_attributes, encode_cross_crate, find_gated_cfg, is_builtin_attr_name,
is_unsafe_attr, is_valid_for_get_attr, AttributeGate, AttributeTemplate, AttributeType,
is_valid_for_get_attr, AttributeGate, AttributeSafety, AttributeTemplate, AttributeType,
BuiltinAttribute, GatedCfg, BUILTIN_ATTRIBUTES, BUILTIN_ATTRIBUTE_MAP,
};
pub use removed::REMOVED_FEATURES;
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_lint/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -825,6 +825,10 @@ lint_unnameable_test_items = cannot test inner items
lint_unnecessary_qualification = unnecessary qualification
.suggestion = remove the unnecessary path segments
lint_unsafe_attr_outside_unsafe = unsafe attribute used without unsafe
.label = usage of unsafe attribute
lint_unsafe_attr_outside_unsafe_suggestion = wrap the attribute in `unsafe(...)`
lint_unsupported_group = `{$lint_group}` lint group is not supported with ´--force-warn´
lint_untranslatable_diag = diagnostics should be created using translatable messages
Expand Down
10 changes: 10 additions & 0 deletions compiler/rustc_lint/src/context/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,16 @@ pub(super) fn decorate_lint(sess: &Session, diagnostic: BuiltinLintDiag, diag: &
BuiltinLintDiag::UnusedQualifications { removal_span } => {
lints::UnusedQualifications { removal_span }.decorate_lint(diag);
}
BuiltinLintDiag::UnsafeAttrOutsideUnsafe {
attribute_name_span,
sugg_spans: (left, right),
} => {
lints::UnsafeAttrOutsideUnsafe {
span: attribute_name_span,
suggestion: lints::UnsafeAttrOutsideUnsafeSuggestion { left, right },
}
.decorate_lint(diag);
}
BuiltinLintDiag::AssociatedConstElidedLifetime {
elided,
span: lt_span,
Expand Down
21 changes: 21 additions & 0 deletions compiler/rustc_lint/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2890,3 +2890,24 @@ pub struct RedundantImportVisibility {
pub import_vis: String,
pub max_vis: String,
}

#[derive(LintDiagnostic)]
#[diag(lint_unsafe_attr_outside_unsafe)]
pub struct UnsafeAttrOutsideUnsafe {
#[label]
pub span: Span,
#[subdiagnostic]
pub suggestion: UnsafeAttrOutsideUnsafeSuggestion,
}

#[derive(Subdiagnostic)]
#[multipart_suggestion(
lint_unsafe_attr_outside_unsafe_suggestion,
applicability = "machine-applicable"
)]
pub struct UnsafeAttrOutsideUnsafeSuggestion {
#[suggestion_part(code = "unsafe(")]
pub left: Span,
#[suggestion_part(code = ")")]
pub right: Span,
}
43 changes: 43 additions & 0 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ declare_lint_pass! {
UNNAMEABLE_TYPES,
UNREACHABLE_CODE,
UNREACHABLE_PATTERNS,
UNSAFE_ATTR_OUTSIDE_UNSAFE,
UNSAFE_OP_IN_UNSAFE_FN,
UNSTABLE_NAME_COLLISIONS,
UNSTABLE_SYNTAX_PRE_EXPANSION,
Expand Down Expand Up @@ -4902,3 +4903,45 @@ declare_lint! {
reference: "issue #123743 <https://github.com/rust-lang/rust/issues/123743>",
};
}

declare_lint! {
/// The `unsafe_attr_outside_unsafe` lint detects a missing unsafe keyword
/// on attributes considered unsafe.
///
/// ### Example
///
/// ```rust
/// #![feature(unsafe_attributes)]
/// #![warn(unsafe_attr_outside_unsafe)]
///
/// #[no_mangle]
/// extern "C" fn foo() {}
///
/// fn main() {}
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// Some attributes (e.g. `no_mangle`, `export_name`, `link_section` -- see
/// [issue #82499] for a more complete list) are considered "unsafe" attributes.
/// An unsafe attribute must only be used inside unsafe(...).
///
/// This lint can automatically wrap the attributes in `unsafe(...)` , but this
/// obviously cannot verify that the preconditions of the `unsafe`
/// attributes are fulfilled, so that is still up to the user.
///
/// The lint is currently "allow" by default, but that might change in the
/// future.
///
/// [editions]: https://doc.rust-lang.org/edition-guide/
/// [issue #82499]: https://github.com/rust-lang/rust/issues/82499
pub UNSAFE_ATTR_OUTSIDE_UNSAFE,
Allow,
"detects unsafe attributes outside of unsafe",
@future_incompatible = FutureIncompatibleInfo {
reason: FutureIncompatibilityReason::EditionError(Edition::Edition2024),
reference: "issue #123757 <https://github.com/rust-lang/rust/issues/123757>",
};
}
4 changes: 4 additions & 0 deletions compiler/rustc_lint_defs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -691,6 +691,10 @@ pub enum BuiltinLintDiag {
/// The span of the unnecessarily-qualified path to remove.
removal_span: Span,
},
UnsafeAttrOutsideUnsafe {
attribute_name_span: Span,
sugg_spans: (Span, Span),
},
AssociatedConstElidedLifetime {
elided: bool,
span: Span,
Expand Down
9 changes: 9 additions & 0 deletions compiler/rustc_parse/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,10 @@ parse_inner_doc_comment_not_permitted = expected outer doc comment
.label_does_not_annotate_this = the inner doc comment doesn't annotate this {$item}
.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
.suggestion = remove the `unsafe(...)`
.note = extraneous unsafe is not allowed in attributes
parse_invalid_block_macro_segment = cannot use a `block` macro fragment here
.label = the `block` fragment is within this context
.suggestion = wrap this in another block
Expand Down Expand Up @@ -866,6 +870,11 @@ parse_unmatched_angle_brackets = {$num_extra_brackets ->
*[other] remove extra angle brackets
}
parse_unsafe_attr_outside_unsafe = unsafe attribute used without unsafe
.label = usage of unsafe attribute
parse_unsafe_attr_outside_unsafe_suggestion = wrap the attribute in `unsafe(...)`
parse_unskipped_whitespace = whitespace symbol '{$ch}' is not skipped
.label = {parse_unskipped_whitespace}
Expand Down
31 changes: 31 additions & 0 deletions compiler/rustc_parse/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2997,3 +2997,34 @@ pub(crate) struct DotDotRangeAttribute {
#[primary_span]
pub span: Span,
}

#[derive(Diagnostic)]
#[diag(parse_invalid_attr_unsafe)]
#[note]
pub struct InvalidAttrUnsafe {
#[primary_span]
pub span: Span,
pub name: Path,
}

#[derive(Diagnostic)]
#[diag(parse_unsafe_attr_outside_unsafe)]
pub struct UnsafeAttrOutsideUnsafe {
#[primary_span]
#[label]
pub span: Span,
#[subdiagnostic]
pub suggestion: UnsafeAttrOutsideUnsafeSuggestion,
}

#[derive(Subdiagnostic)]
#[multipart_suggestion(
parse_unsafe_attr_outside_unsafe_suggestion,
applicability = "machine-applicable"
)]
pub struct UnsafeAttrOutsideUnsafeSuggestion {
#[suggestion_part(code = "unsafe(")]
pub left: Span,
#[suggestion_part(code = ")")]
pub right: Span,
}
64 changes: 58 additions & 6 deletions compiler/rustc_parse/src/validate_attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,73 @@ use crate::{errors, parse_in};
use rustc_ast::token::Delimiter;
use rustc_ast::tokenstream::DelimSpan;
use rustc_ast::MetaItemKind;
use rustc_ast::{self as ast, AttrArgs, AttrArgsEq, Attribute, DelimArgs, MetaItem};
use rustc_ast::{self as ast, AttrArgs, AttrArgsEq, Attribute, DelimArgs, MetaItem, Safety};
use rustc_errors::{Applicability, FatalError, PResult};
use rustc_feature::{AttributeTemplate, BuiltinAttribute, BUILTIN_ATTRIBUTE_MAP};
use rustc_feature::{
AttributeSafety, AttributeTemplate, BuiltinAttribute, Features, BUILTIN_ATTRIBUTE_MAP,
};
use rustc_session::errors::report_lit_error;
use rustc_session::lint::builtin::ILL_FORMED_ATTRIBUTE_INPUT;
use rustc_session::lint::builtin::{ILL_FORMED_ATTRIBUTE_INPUT, UNSAFE_ATTR_OUTSIDE_UNSAFE};
use rustc_session::lint::BuiltinLintDiag;
use rustc_session::parse::ParseSess;
use rustc_span::{sym, Span, Symbol};
use rustc_span::{sym, BytePos, Span, Symbol};

pub fn check_attr(psess: &ParseSess, attr: &Attribute) {
pub fn check_attr(features: &Features, psess: &ParseSess, attr: &Attribute) {
if attr.is_doc_comment() {
return;
}

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

// Check input tokens for built-in and key-value attributes.
match attr_info {
Expand All @@ -32,7 +84,7 @@ pub fn check_attr(psess: &ParseSess, attr: &Attribute) {
}
}
}
_ if let AttrArgs::Eq(..) = attr.get_normal_item().args => {
_ if let AttrArgs::Eq(..) = attr_item.args => {
// All key-value attributes are restricted to meta-item syntax.
match parse_meta(psess, attr) {
Ok(_) => {}
Expand Down
4 changes: 0 additions & 4 deletions compiler/rustc_passes/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -384,10 +384,6 @@ passes_invalid_attr_at_crate_level =
passes_invalid_attr_at_crate_level_item =
the inner attribute doesn't annotate this {$kind}
passes_invalid_attr_unsafe = `{$name}` is not an unsafe attribute
.suggestion = remove the `unsafe(...)`
.note = extraneous unsafe is not allowed in attributes
passes_invalid_macro_export_arguments = `{$name}` isn't a valid `#[macro_export]` argument
passes_invalid_macro_export_arguments_too_many_items = `#[macro_export]` can only take 1 or 0 arguments
Expand Down
21 changes: 1 addition & 20 deletions compiler/rustc_passes/src/check_attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@ use rustc_ast::{MetaItemKind, MetaItemLit, NestedMetaItem};
use rustc_data_structures::fx::FxHashMap;
use rustc_errors::{Applicability, IntoDiagArg, MultiSpan};
use rustc_errors::{DiagCtxtHandle, StashKey};
use rustc_feature::{
is_unsafe_attr, AttributeDuplicates, AttributeType, BuiltinAttribute, BUILTIN_ATTRIBUTE_MAP,
};
use rustc_feature::{AttributeDuplicates, AttributeType, BuiltinAttribute, BUILTIN_ATTRIBUTE_MAP};
use rustc_hir::def_id::LocalModDefId;
use rustc_hir::intravisit::{self, Visitor};
use rustc_hir::{self as hir};
Expand Down Expand Up @@ -116,8 +114,6 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
let mut seen = FxHashMap::default();
let attrs = self.tcx.hir().attrs(hir_id);
for attr in attrs {
self.check_unsafe_attr(attr);

match attr.path().as_slice() {
[sym::diagnostic, sym::do_not_recommend] => {
self.check_do_not_recommend(attr.span, hir_id, target)
Expand Down Expand Up @@ -312,21 +308,6 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
true
}

/// Checks if `unsafe()` is applied to an invalid attribute.
fn check_unsafe_attr(&self, attr: &Attribute) {
if !attr.is_doc_comment() {
let attr_item = attr.get_normal_item();
if let ast::Safety::Unsafe(unsafe_span) = attr_item.unsafety {
if !is_unsafe_attr(attr.name_or_empty()) {
self.dcx().emit_err(errors::InvalidAttrUnsafe {
span: unsafe_span,
name: attr_item.path.clone(),
});
}
}
}
}

/// Checks if `#[diagnostic::on_unimplemented]` is applied to a trait definition
fn check_diagnostic_on_unimplemented(
&self,
Expand Down
11 changes: 1 addition & 10 deletions compiler/rustc_passes/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::{
};

use crate::fluent_generated as fluent;
use rustc_ast::{ast, Label};
use rustc_ast::Label;
use rustc_errors::{
codes::*, Applicability, Diag, DiagCtxtHandle, DiagSymbolList, Diagnostic, EmissionGuarantee,
Level, MultiSpan, SubdiagMessageOp, Subdiagnostic,
Expand Down Expand Up @@ -863,15 +863,6 @@ pub struct InvalidAttrAtCrateLevel {
pub item: Option<ItemFollowingInnerAttr>,
}

#[derive(Diagnostic)]
#[diag(passes_invalid_attr_unsafe)]
#[note]
pub struct InvalidAttrUnsafe {
#[primary_span]
pub span: Span,
pub name: ast::Path,
}

#[derive(Clone, Copy)]
pub struct ItemFollowingInnerAttr {
pub span: Span,
Expand Down
Loading

0 comments on commit a23917c

Please sign in to comment.