From 03936115eff4c1aa2be1ceeea87238ec7c822d90 Mon Sep 17 00:00:00 2001 From: Robin Kruppe Date: Mon, 1 Jan 2018 21:42:12 +0100 Subject: [PATCH] Check all repr hints together when checking for mis-applied attributes Fixes #47094 Besides fixing that bug, this change has a user-visible effect on the spans in the "incompatible repr hints" warning and another error: they now point at `foo` and/or `bar` in `repr(foo, bar)` instead of the whole attribute. This is sometimes more precise (e.g., `#[repr(C, packed)]` on an enum points at the `packed`) but sometimes not. I moved a compile-fail test to a ui test to illustrate how it now looks in the common case of only one attribute. --- src/librustc/hir/check_attr.rs | 82 ++++++++++--------- .../{compile-fail => ui}/attr-usage-repr.rs | 1 - src/test/ui/attr-usage-repr.stderr | 42 ++++++++++ src/test/ui/feature-gate-simd-ffi.rs | 1 - src/test/ui/feature-gate-simd-ffi.stderr | 8 +- src/test/ui/issue-47094.rs | 26 ++++++ src/test/ui/issue-47094.stderr | 14 ++++ 7 files changed, 130 insertions(+), 44 deletions(-) rename src/test/{compile-fail => ui}/attr-usage-repr.rs (98%) create mode 100644 src/test/ui/attr-usage-repr.stderr create mode 100644 src/test/ui/issue-47094.rs create mode 100644 src/test/ui/issue-47094.stderr diff --git a/src/librustc/hir/check_attr.rs b/src/librustc/hir/check_attr.rs index 003255f87966f..7792726006859 100644 --- a/src/librustc/hir/check_attr.rs +++ b/src/librustc/hir/check_attr.rs @@ -47,14 +47,15 @@ struct CheckAttrVisitor<'a> { impl<'a> CheckAttrVisitor<'a> { /// Check any attribute. - fn check_attribute(&self, attr: &ast::Attribute, item: &ast::Item, target: Target) { - if let Some(name) = attr.name() { - match &*name.as_str() { - "inline" => self.check_inline(attr, item, target), - "repr" => self.check_repr(attr, item, target), - _ => (), + fn check_attributes(&self, item: &ast::Item, target: Target) { + for attr in &item.attrs { + if let Some(name) = attr.name() { + if name == "inline" { + self.check_inline(attr, item, target) + } } } + self.check_repr(item, target); } /// Check if an `#[inline]` is applied to a function. @@ -66,45 +67,51 @@ impl<'a> CheckAttrVisitor<'a> { } } - /// Check if an `#[repr]` attr is valid. - fn check_repr(&self, attr: &ast::Attribute, item: &ast::Item, target: Target) { - let words = match attr.meta_item_list() { - Some(words) => words, - None => { - return; - } - }; + /// Check if the `#[repr]` attributes on `item` are valid. + fn check_repr(&self, item: &ast::Item, target: Target) { + // Extract the names of all repr hints, e.g., [foo, bar, align] for: + // ``` + // #[repr(foo)] + // #[repr(bar, align(8))] + // ``` + let hints: Vec<_> = item.attrs + .iter() + .filter(|attr| match attr.name() { + Some(name) => name == "repr", + None => false, + }) + .filter_map(|attr| attr.meta_item_list()) + .flat_map(|hints| hints) + .collect(); let mut int_reprs = 0; let mut is_c = false; let mut is_simd = false; - for word in words { - - let name = match word.name() { - Some(word) => word, - None => continue, + for hint in &hints { + let name = if let Some(name) = hint.name() { + name + } else { + // Invalid repr hint like repr(42). We don't check for unrecognized hints here + // (libsyntax does that), so just ignore it. + continue; }; - let (message, label) = match &*name.as_str() { + let (article, allowed_targets) = match &*name.as_str() { "C" => { is_c = true; if target != Target::Struct && target != Target::Union && target != Target::Enum { - ("attribute should be applied to struct, enum or union", - "a struct, enum or union") + ("a", "struct, enum or union") } else { continue } } "packed" => { - // Do not increment conflicting_reprs here, because "packed" - // can be used to modify another repr hint if target != Target::Struct && target != Target::Union { - ("attribute should be applied to struct or union", - "a struct or union") + ("a", "struct or union") } else { continue } @@ -112,8 +119,7 @@ impl<'a> CheckAttrVisitor<'a> { "simd" => { is_simd = true; if target != Target::Struct { - ("attribute should be applied to struct", - "a struct") + ("a", "struct") } else { continue } @@ -121,8 +127,7 @@ impl<'a> CheckAttrVisitor<'a> { "align" => { if target != Target::Struct && target != Target::Union { - ("attribute should be applied to struct or union", - "a struct or union") + ("a", "struct or union") } else { continue } @@ -132,16 +137,16 @@ impl<'a> CheckAttrVisitor<'a> { "isize" | "usize" => { int_reprs += 1; if target != Target::Enum { - ("attribute should be applied to enum", - "an enum") + ("an", "enum") } else { continue } } _ => continue, }; - struct_span_err!(self.sess, attr.span, E0517, "{}", message) - .span_label(item.span, format!("not {}", label)) + struct_span_err!(self.sess, hint.span, E0517, + "attribute should be applied to {}", allowed_targets) + .span_label(item.span, format!("not {} {}", article, allowed_targets)) .emit(); } @@ -149,7 +154,10 @@ impl<'a> CheckAttrVisitor<'a> { if (int_reprs > 1) || (is_simd && is_c) || (int_reprs == 1 && is_c && is_c_like_enum(item)) { - span_warn!(self.sess, attr.span, E0566, + // Just point at all repr hints. This is not ideal, but tracking precisely which ones + // are at fault is a huge hassle. + let spans: Vec<_> = hints.iter().map(|hint| hint.span).collect(); + span_warn!(self.sess, spans, E0566, "conflicting representation hints"); } } @@ -158,9 +166,7 @@ impl<'a> CheckAttrVisitor<'a> { impl<'a> Visitor<'a> for CheckAttrVisitor<'a> { fn visit_item(&mut self, item: &'a ast::Item) { let target = Target::from_item(item); - for attr in &item.attrs { - self.check_attribute(attr, item, target); - } + self.check_attributes(item, target); visit::walk_item(self, item); } } diff --git a/src/test/compile-fail/attr-usage-repr.rs b/src/test/ui/attr-usage-repr.rs similarity index 98% rename from src/test/compile-fail/attr-usage-repr.rs rename to src/test/ui/attr-usage-repr.rs index c0bfd3690c859..db5cd47fe0efa 100644 --- a/src/test/compile-fail/attr-usage-repr.rs +++ b/src/test/ui/attr-usage-repr.rs @@ -8,7 +8,6 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -#![allow(dead_code)] #![feature(attr_literals)] #![feature(repr_simd)] diff --git a/src/test/ui/attr-usage-repr.stderr b/src/test/ui/attr-usage-repr.stderr new file mode 100644 index 0000000000000..b9c012630e9f3 --- /dev/null +++ b/src/test/ui/attr-usage-repr.stderr @@ -0,0 +1,42 @@ +error[E0517]: attribute should be applied to struct, enum or union + --> $DIR/attr-usage-repr.rs:14:8 + | +14 | #[repr(C)] //~ ERROR: attribute should be applied to struct, enum or union + | ^ +15 | fn f() {} + | --------- not a struct, enum or union + +error[E0517]: attribute should be applied to enum + --> $DIR/attr-usage-repr.rs:26:8 + | +26 | #[repr(i8)] //~ ERROR: attribute should be applied to enum + | ^^ +27 | struct SInt(f64, f64); + | ---------------------- not an enum + +error[E0517]: attribute should be applied to struct or union + --> $DIR/attr-usage-repr.rs:32:8 + | +32 | #[repr(align(8))] //~ ERROR: attribute should be applied to struct + | ^^^^^^^^ +33 | enum EAlign { A, B } + | -------------------- not a struct or union + +error[E0517]: attribute should be applied to struct or union + --> $DIR/attr-usage-repr.rs:35:8 + | +35 | #[repr(packed)] //~ ERROR: attribute should be applied to struct + | ^^^^^^ +36 | enum EPacked { A, B } + | --------------------- not a struct or union + +error[E0517]: attribute should be applied to struct + --> $DIR/attr-usage-repr.rs:38:8 + | +38 | #[repr(simd)] //~ ERROR: attribute should be applied to struct + | ^^^^ +39 | enum ESimd { A, B } + | ------------------- not a struct + +error: aborting due to 5 previous errors + diff --git a/src/test/ui/feature-gate-simd-ffi.rs b/src/test/ui/feature-gate-simd-ffi.rs index 31c055f229c3e..a603658d31610 100644 --- a/src/test/ui/feature-gate-simd-ffi.rs +++ b/src/test/ui/feature-gate-simd-ffi.rs @@ -13,7 +13,6 @@ #[repr(simd)] #[derive(Copy, Clone)] -#[repr(C)] struct LocalSimd(u8, u8); extern { diff --git a/src/test/ui/feature-gate-simd-ffi.stderr b/src/test/ui/feature-gate-simd-ffi.stderr index fa47e1a2903cc..ab1ebefa333ea 100644 --- a/src/test/ui/feature-gate-simd-ffi.stderr +++ b/src/test/ui/feature-gate-simd-ffi.stderr @@ -1,15 +1,15 @@ error: use of SIMD type `LocalSimd` in FFI is highly experimental and may result in invalid code - --> $DIR/feature-gate-simd-ffi.rs:20:17 + --> $DIR/feature-gate-simd-ffi.rs:19:17 | -20 | fn baz() -> LocalSimd; //~ ERROR use of SIMD type +19 | fn baz() -> LocalSimd; //~ ERROR use of SIMD type | ^^^^^^^^^ | = help: add #![feature(simd_ffi)] to the crate attributes to enable error: use of SIMD type `LocalSimd` in FFI is highly experimental and may result in invalid code - --> $DIR/feature-gate-simd-ffi.rs:21:15 + --> $DIR/feature-gate-simd-ffi.rs:20:15 | -21 | fn qux(x: LocalSimd); //~ ERROR use of SIMD type +20 | fn qux(x: LocalSimd); //~ ERROR use of SIMD type | ^^^^^^^^^ | = help: add #![feature(simd_ffi)] to the crate attributes to enable diff --git a/src/test/ui/issue-47094.rs b/src/test/ui/issue-47094.rs new file mode 100644 index 0000000000000..3ab9d4e6d5a8f --- /dev/null +++ b/src/test/ui/issue-47094.rs @@ -0,0 +1,26 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// must-compile-successfully + +#[repr(C,u8)] +enum Foo { + A, + B, +} + +#[repr(C)] +#[repr(u8)] +enum Bar { + A, + B, +} + +fn main() {} diff --git a/src/test/ui/issue-47094.stderr b/src/test/ui/issue-47094.stderr new file mode 100644 index 0000000000000..5276b881e4c6b --- /dev/null +++ b/src/test/ui/issue-47094.stderr @@ -0,0 +1,14 @@ +warning[E0566]: conflicting representation hints + --> $DIR/issue-47094.rs:13:8 + | +13 | #[repr(C,u8)] + | ^ ^^ + +warning[E0566]: conflicting representation hints + --> $DIR/issue-47094.rs:19:8 + | +19 | #[repr(C)] + | ^ +20 | #[repr(u8)] + | ^^ +