Check all repr hints together when checking for mis-applied attributes

Robin Kruppe · Robin Kruppe · commit 03936115eff4 · 2018-01-01T22:05:29.000+01:00
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.
diff --git 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,63 +67,67 @@ 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
                     }
                 }
                 "simd" => {
                     is_simd = true;
                     if target != Target::Struct {
-                        ("attribute should be applied to struct",
-                         "a struct")
+                        ("a", "struct")
                     } else {
                         continue
                     }
                 }
                 "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,24 +137,27 @@ 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();
         }
 
         // Warn on repr(u8, u16), repr(C, simd), and c-like-enum-repr(C, u8)
         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/ui/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
@@ -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
@@ -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
@@ -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
@@ -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 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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
@@ -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)]
+   |        ^^
+

Original file line number	Diff line number	Diff line change
`@@ -1,15 +1,15 @@`
`1`	`1`	error: use of SIMD type `LocalSimd` in FFI is highly experimental and may result in invalid code
`2`		`- --> $DIR/feature-gate-simd-ffi.rs:20:17`
	`2`	`+ --> $DIR/feature-gate-simd-ffi.rs:19:17`
`3`	`3`	`\|`
`4`		`-20 \| fn baz() -> LocalSimd; //~ ERROR use of SIMD type`
	`4`	`+19 \| fn baz() -> LocalSimd; //~ ERROR use of SIMD type`
`5`	`5`	`\| ^^^^^^^^^`
`6`	`6`	`\|`
`7`	`7`	`= help: add #![feature(simd_ffi)] to the crate attributes to enable`
`8`	`8`
`9`	`9`	error: use of SIMD type `LocalSimd` in FFI is highly experimental and may result in invalid code
`10`		`- --> $DIR/feature-gate-simd-ffi.rs:21:15`
	`10`	`+ --> $DIR/feature-gate-simd-ffi.rs:20:15`
`11`	`11`	`\|`
`12`		`-21 \| fn qux(x: LocalSimd); //~ ERROR use of SIMD type`
	`12`	`+20 \| fn qux(x: LocalSimd); //~ ERROR use of SIMD type`
`13`	`13`	`\| ^^^^^^^^^`
`14`	`14`	`\|`
`15`	`15`	`= help: add #![feature(simd_ffi)] to the crate attributes to enable`