Skip to content
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.

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

Add hard error and migration lint for unsafe attrs #126177

Merged
merged 1 commit into from
Jun 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion: should this say something like "unsafe attribute block is not permitted for safe attributes"?

This comment was marked as resolved.


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
carbotaniuman marked this conversation as resolved.
Show resolved Hide resolved
.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
Loading