Skip to content

Commit

Permalink
Auto merge of #73461 - calebzulawski:validate-attribute-placement, r=…
Browse files Browse the repository at this point in the history 
…matthewjasper

Validate built-in attribute placement

Closes #54584, closes #47725, closes #54044.

I've changed silently ignoring some incorrectly placed attributes to errors.  I'm not sure what the policy is since this can theoretically break code (should they be warnings instead? does it warrant a crater run?).
  • Loading branch information
@bors
bors committed Sep 12, 2020
2 parents 9891908 + 82cb379 commit dbb73f8
Show file tree
Hide file tree
Showing 18 changed files with 1,156 additions and 462 deletions.
197 changes: 183 additions & 14 deletions compiler/rustc_passes/src/check_attr.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,26 @@ impl CheckAttrVisitor<'tcx> {
} else if self.tcx.sess.check_name(attr, sym::marker) {
self.check_marker(attr, span, target)
} else if self.tcx.sess.check_name(attr, sym::target_feature) {
self.check_target_feature(attr, span, target)
self.check_target_feature(hir_id, attr, span, target)
} else if self.tcx.sess.check_name(attr, sym::track_caller) {
self.check_track_caller(&attr.span, attrs, span, target)
} else if self.tcx.sess.check_name(attr, sym::doc) {
self.check_doc_alias(attr, hir_id, target)
} else if self.tcx.sess.check_name(attr, sym::no_link) {
self.check_no_link(&attr, span, target)
} else if self.tcx.sess.check_name(attr, sym::export_name) {
self.check_export_name(&attr, span, target)
} else {
// lint-only checks
if self.tcx.sess.check_name(attr, sym::cold) {
self.check_cold(hir_id, attr, span, target);
} else if self.tcx.sess.check_name(attr, sym::link_name) {
self.check_link_name(hir_id, attr, span, target);
} else if self.tcx.sess.check_name(attr, sym::link_section) {
self.check_link_section(hir_id, attr, span, target);
} else if self.tcx.sess.check_name(attr, sym::no_mangle) {
self.check_no_mangle(hir_id, attr, span, target);
}
true
};
}
Expand Down Expand Up @@ -109,12 +123,12 @@ impl CheckAttrVisitor<'tcx> {
lint.build("`#[inline]` is ignored on constants")
.warn(
"this was previously accepted by the compiler but is \
being phased out; it will become a hard error in \
a future release!",
being phased out; it will become a hard error in \
a future release!",
)
.note(
"see issue #65833 <https://github.com/rust-lang/rust/issues/65833> \
for more information",
for more information",
)
.emit();
});
Expand Down Expand Up @@ -153,7 +167,7 @@ impl CheckAttrVisitor<'tcx> {
.emit();
false
}
Target::Fn | Target::Method(..) | Target::ForeignFn => true,
Target::Fn | Target::Method(..) | Target::ForeignFn | Target::Closure => true,
_ => {
struct_span_err!(
self.tcx.sess,
Expand Down Expand Up @@ -202,10 +216,31 @@ impl CheckAttrVisitor<'tcx> {
}

/// Checks if the `#[target_feature]` attribute on `item` is valid. Returns `true` if valid.
fn check_target_feature(&self, attr: &Attribute, span: &Span, target: Target) -> bool {
fn check_target_feature(
&self,
hir_id: HirId,
attr: &Attribute,
span: &Span,
target: Target,
) -> bool {
match target {
Target::Fn
| Target::Method(MethodKind::Trait { body: true } | MethodKind::Inherent) => true,
// FIXME: #[target_feature] was previously erroneously allowed on statements and some
// crates used this, so only emit a warning.
Target::Statement => {
self.tcx.struct_span_lint_hir(UNUSED_ATTRIBUTES, hir_id, attr.span, |lint| {
lint.build("attribute should be applied to a function")
.warn(
"this was previously accepted by the compiler but is \
being phased out; it will become a hard error in \
a future release!",
)
.span_label(*span, "not a function")
.emit();
});
true
}
_ => {
self.tcx
.sess
Expand Down Expand Up @@ -277,6 +312,136 @@ impl CheckAttrVisitor<'tcx> {
true
}

/// Checks if `#[cold]` is applied to a non-function. Returns `true` if valid.
fn check_cold(&self, hir_id: HirId, attr: &Attribute, span: &Span, target: Target) {
match target {
Target::Fn | Target::Method(..) | Target::ForeignFn | Target::Closure => {}
_ => {
// FIXME: #[cold] was previously allowed on non-functions and some crates used
// this, so only emit a warning.
self.tcx.struct_span_lint_hir(UNUSED_ATTRIBUTES, hir_id, attr.span, |lint| {
lint.build("attribute should be applied to a function")
.warn(
"this was previously accepted by the compiler but is \
being phased out; it will become a hard error in \
a future release!",
)
.span_label(*span, "not a function")
.emit();
});
}
}
}

/// Checks if `#[link_name]` is applied to an item other than a foreign function or static.
fn check_link_name(&self, hir_id: HirId, attr: &Attribute, span: &Span, target: Target) {
match target {
Target::ForeignFn | Target::ForeignStatic => {}
_ => {
// FIXME: #[cold] was previously allowed on non-functions/statics and some crates
// used this, so only emit a warning.
self.tcx.struct_span_lint_hir(UNUSED_ATTRIBUTES, hir_id, attr.span, |lint| {
let mut diag =
lint.build("attribute should be applied to a foreign function or static");
diag.warn(
"this was previously accepted by the compiler but is \
being phased out; it will become a hard error in \
a future release!",
);

// See issue #47725
if let Target::ForeignMod = target {
if let Some(value) = attr.value_str() {
diag.span_help(
attr.span,
&format!(r#"try `#[link(name = "{}")]` instead"#, value),
);
} else {
diag.span_help(attr.span, r#"try `#[link(name = "...")]` instead"#);
}
}

diag.span_label(*span, "not a foreign function or static");
diag.emit();
});
}
}
}

/// Checks if `#[no_link]` is applied to an `extern crate`. Returns `true` if valid.
fn check_no_link(&self, attr: &Attribute, span: &Span, target: Target) -> bool {
if target == Target::ExternCrate {
true
} else {
self.tcx
.sess
.struct_span_err(attr.span, "attribute should be applied to an `extern crate` item")
.span_label(*span, "not an `extern crate` item")
.emit();
false
}
}

/// Checks if `#[export_name]` is applied to a function or static. Returns `true` if valid.
fn check_export_name(&self, attr: &Attribute, span: &Span, target: Target) -> bool {
match target {
Target::Static | Target::Fn | Target::Method(..) => true,
_ => {
self.tcx
.sess
.struct_span_err(
attr.span,
"attribute should be applied to a function or static",
)
.span_label(*span, "not a function or static")
.emit();
false
}
}
}

/// Checks if `#[link_section]` is applied to a function or static.
fn check_link_section(&self, hir_id: HirId, attr: &Attribute, span: &Span, target: Target) {
match target {
Target::Static | Target::Fn | Target::Method(..) => {}
_ => {
// FIXME: #[link_section] was previously allowed on non-functions/statics and some
// crates used this, so only emit a warning.
self.tcx.struct_span_lint_hir(UNUSED_ATTRIBUTES, hir_id, attr.span, |lint| {
lint.build("attribute should be applied to a function or static")
.warn(
"this was previously accepted by the compiler but is \
being phased out; it will become a hard error in \
a future release!",
)
.span_label(*span, "not a function or static")
.emit();
});
}
}
}

/// Checks if `#[no_mangle]` is applied to a function or static.
fn check_no_mangle(&self, hir_id: HirId, attr: &Attribute, span: &Span, target: Target) {
match target {
Target::Static | Target::Fn | Target::Method(..) => {}
_ => {
// FIXME: #[no_mangle] was previously allowed on non-functions/statics and some
// crates used this, so only emit a warning.
self.tcx.struct_span_lint_hir(UNUSED_ATTRIBUTES, hir_id, attr.span, |lint| {
lint.build("attribute should be applied to a function or static")
.warn(
"this was previously accepted by the compiler but is \
being phased out; it will become a hard error in \
a future release!",
)
.span_label(*span, "not a function or static")
.emit();
});
}
}
}

/// Checks if the `#[repr]` attributes on `item` are valid.
fn check_repr(
&self,
Expand Down Expand Up @@ -321,7 +486,11 @@ impl CheckAttrVisitor<'tcx> {
}
sym::simd => {
is_simd = true;
if target != Target::Struct { ("a", "struct") } else { continue }
if target != Target::Struct {
("a", "struct")
} else {
continue;
}
}
sym::transparent => {
is_transparent = true;
Expand Down Expand Up @@ -358,7 +527,11 @@ impl CheckAttrVisitor<'tcx> {
| sym::isize
| sym::usize => {
int_reprs += 1;
if target != Target::Enum { ("an", "enum") } else { continue }
if target != Target::Enum {
("an", "enum")
} else {
continue;
}
}
_ => continue,
};
Expand Down Expand Up @@ -421,10 +594,8 @@ impl CheckAttrVisitor<'tcx> {
fn check_stmt_attributes(&self, stmt: &hir::Stmt<'_>) {
// When checking statements ignore expressions, they will be checked later
if let hir::StmtKind::Local(ref l) = stmt.kind {
self.check_attributes(l.hir_id, &l.attrs, &stmt.span, Target::Statement, None);
for attr in l.attrs.iter() {
if self.tcx.sess.check_name(attr, sym::inline) {
self.check_inline(l.hir_id, attr, &stmt.span, Target::Statement);
}
if self.tcx.sess.check_name(attr, sym::repr) {
self.emit_repr_error(
attr.span,
Expand All @@ -442,10 +613,8 @@ impl CheckAttrVisitor<'tcx> {
hir::ExprKind::Closure(..) => Target::Closure,
_ => Target::Expression,
};
self.check_attributes(expr.hir_id, &expr.attrs, &expr.span, target, None);
for attr in expr.attrs.iter() {
if self.tcx.sess.check_name(attr, sym::inline) {
self.check_inline(expr.hir_id, attr, &expr.span, target);
}
if self.tcx.sess.check_name(attr, sym::repr) {
self.emit_repr_error(
attr.span,
Expand Down
30 changes: 11 additions & 19 deletions compiler/rustc_typeck/src/collect.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2525,10 +2525,17 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, id: DefId) -> CodegenFnAttrs {
codegen_fn_attrs.export_name = Some(s);
}
} else if tcx.sess.check_name(attr, sym::target_feature) {
if !tcx.features().target_feature_11 {
check_target_feature_safe_fn(tcx, id, attr.span);
} else if let Some(local_id) = id.as_local() {
if tcx.fn_sig(id).unsafety() == hir::Unsafety::Normal {
if !tcx.is_closure(id) && tcx.fn_sig(id).unsafety() == hir::Unsafety::Normal {
if !tcx.features().target_feature_11 {
let mut err = feature_err(
&tcx.sess.parse_sess,
sym::target_feature_11,
attr.span,
"`#[target_feature(..)]` can only be applied to `unsafe` functions",
);
err.span_label(tcx.def_span(id), "not an `unsafe` function");
err.emit();
} else if let Some(local_id) = id.as_local() {
check_target_feature_trait_unsafe(tcx, local_id, attr.span);
}
}
Expand Down Expand Up @@ -2785,21 +2792,6 @@ fn check_link_name_xor_ordinal(
}
}

/// Checks the function annotated with `#[target_feature]` is unsafe,
/// reporting an error if it isn't.
fn check_target_feature_safe_fn(tcx: TyCtxt<'_>, id: DefId, attr_span: Span) {
if tcx.is_closure(id) || tcx.fn_sig(id).unsafety() == hir::Unsafety::Normal {
let mut err = feature_err(
&tcx.sess.parse_sess,
sym::target_feature_11,
attr_span,
"`#[target_feature(..)]` can only be applied to `unsafe` functions",
);
err.span_label(tcx.def_span(id), "not an `unsafe` function");
err.emit();
}
}

/// Checks the function annotated with `#[target_feature]` is not a safe
/// trait method implementation, reporting an error if it is.
fn check_target_feature_trait_unsafe(tcx: TyCtxt<'_>, id: LocalDefId, attr_span: Span) {
Expand Down
5 changes: 1 addition & 4 deletions src/test/ui/check-static-recursion-foreign.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// run-pass

#![allow(dead_code)]
// Static recursion check shouldn't fail when given a foreign item (#18279)

// aux-build:check_static_recursion_foreign_helper.rs
Expand All @@ -15,12 +14,10 @@ extern crate libc;

use libc::c_int;

#[link_name = "check_static_recursion_foreign_helper"]
extern "C" {
#[allow(dead_code)]
static test_static: c_int;
}

static B: &'static c_int = unsafe { &test_static };
pub static B: &'static c_int = unsafe { &test_static };

pub fn main() {}
Loading

0 comments on commit dbb73f8

Please sign in to comment.