Merge pull request #2340 from phansch/newline_after_attributes

Warn on empty lines after outer attributes

Author: oli-obk

clippy_lints/src/attrs.rs

@@ -5,7 +5,7 @@ use rustc::lint::*;
 use rustc::hir::*;
 use rustc::ty::{self, TyCtxt};
 use semver::Version;
-use syntax::ast::{Attribute, Lit, LitKind, MetaItemKind, NestedMetaItem, NestedMetaItemKind};
+use syntax::ast::{Attribute, AttrStyle, Lit, LitKind, MetaItemKind, NestedMetaItem, NestedMetaItemKind};
 use syntax::codemap::Span;
 use utils::{in_macro, match_def_path, opt_def_id, paths, snippet_opt, span_lint, span_lint_and_then};
 
@@ -78,12 +78,44 @@ declare_lint! {
     "use of `#[deprecated(since = \"x\")]` where x is not semver"
 }
 
+/// **What it does:** Checks for empty lines after outer attributes
+///
+/// **Why is this bad?**
+/// Most likely the attribute was meant to be an inner attribute using a '!'.
+/// If it was meant to be an outer attribute, then the following item
+/// should not be separated by empty lines.
+///
+/// **Known problems:** None
+///
+/// **Example:**
+/// ```rust
+/// // Bad
+/// #[inline(always)]
+///
+/// fn not_quite_good_code(..) { ... }
+///
+/// // Good (as inner attribute)
+/// #![inline(always)]
+///
+/// fn this_is_fine_too(..) { ... }
+///
+/// // Good (as outer attribute)
+/// #[inline(always)]
+/// fn this_is_fine(..) { ... }
+///
+/// ```
+declare_lint! {
+    pub EMPTY_LINE_AFTER_OUTER_ATTR,
+    Warn,
+    "empty line after outer attribute"
+}
+
 #[derive(Copy, Clone)]
 pub struct AttrPass;
 
 impl LintPass for AttrPass {
     fn get_lints(&self) -> LintArray {
-        lint_array!(INLINE_ALWAYS, DEPRECATED_SEMVER, USELESS_ATTRIBUTE)
+        lint_array!(INLINE_ALWAYS, DEPRECATED_SEMVER, USELESS_ATTRIBUTE, EMPTY_LINE_AFTER_OUTER_ATTR)
     }
 }
 
@@ -171,7 +203,7 @@ fn is_relevant_item(tcx: TyCtxt, item: &Item) -> bool {
     if let ItemFn(_, _, _, _, _, eid) = item.node {
         is_relevant_expr(tcx, tcx.body_tables(eid), &tcx.hir.body(eid).value)
     } else {
-        false
+        true
     }
 }
 
@@ -230,6 +262,27 @@ fn check_attrs(cx: &LateContext, span: Span, name: &Name, attrs: &[Attribute]) {
     }
 
     for attr in attrs {
+        if attr.style == AttrStyle::Outer {
+            if !is_present_in_source(cx, attr.span) {
+                return;
+            }
+
+            let attr_to_item_span = Span::new(attr.span.lo(), span.lo(), span.ctxt());
+
+            if let Some(snippet) = snippet_opt(cx, attr_to_item_span) {
+                let lines = snippet.split('\n').collect::<Vec<_>>();
+                if lines.iter().filter(|l| l.trim().is_empty()).count() > 1 {
+                    span_lint(
+                        cx,
+                        EMPTY_LINE_AFTER_OUTER_ATTR,
+                        attr_to_item_span,
+                        &format!("Found an empty line after an outer attribute. Perhaps you forgot to add a '!' to make it an inner attribute?")
+                        );
+
+                }
+            }
+        }
+
         if let Some(ref values) = attr.meta_item_list() {
             if values.len() != 1 || attr.name().map_or(true, |n| n != "inline") {
                 continue;
@@ -270,3 +323,17 @@ fn is_word(nmi: &NestedMetaItem, expected: &str) -> bool {
         false
     }
 }
+
+// If the snippet is empty, it's an attribute that was inserted during macro
+// expansion and we want to ignore those, because they could come from external
+// sources that the user has no control over.
+// For some reason these attributes don't have any expansion info on them, so
+// we have to check it this way until there is a better way.
+fn is_present_in_source(cx: &LateContext, span: Span) -> bool {
+    if let Some(snippet) = snippet_opt(cx, span) {
+        if snippet.is_empty() {
+            return false;
+        }
+    }
+    true
+}

clippy_lints/src/lib.rs

@@ -440,6 +440,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         attrs::DEPRECATED_SEMVER,
         attrs::INLINE_ALWAYS,
         attrs::USELESS_ATTRIBUTE,
+        attrs::EMPTY_LINE_AFTER_OUTER_ATTR,
         bit_mask::BAD_BIT_MASK,
         bit_mask::INEFFECTIVE_BIT_MASK,
         bit_mask::VERBOSE_BIT_MASK,

tests/ui/empty_line_after_outer_attribute.rs

@@ -0,0 +1,56 @@
+
+#![warn(empty_line_after_outer_attr)]
+
+// This should produce a warning
+#[crate_type = "lib"]
+
+/// some comment
+fn with_one_newline_and_comment() { assert!(true) }
+
+// This should not produce a warning
+#[crate_type = "lib"]
+/// some comment
+fn with_no_newline_and_comment() { assert!(true) }
+
+
+// This should produce a warning
+#[crate_type = "lib"]
+
+fn with_one_newline() { assert!(true) }
+
+// This should produce a warning, too
+#[crate_type = "lib"]
+
+
+fn with_two_newlines() { assert!(true) }
+
+
+// This should produce a warning
+#[crate_type = "lib"]
+
+enum Baz {
+    One,
+    Two
+}
+
+// This should produce a warning
+#[crate_type = "lib"]
+
+struct Foo {
+    one: isize,
+    two: isize
+}
+
+// This should produce a warning
+#[crate_type = "lib"]
+
+mod foo {
+}
+
+// This should not produce a warning
+#[allow(non_camel_case_types)]
+#[allow(missing_docs)]
+#[allow(missing_docs)]
+fn three_attributes() { assert!(true) }
+
+fn main() { }

tests/ui/empty_line_after_outer_attribute.stderr

@@ -0,0 +1,54 @@
+error: Found an empty line after an outer attribute. Perhaps you forgot to add a '!' to make it an inner attribute?
+ --> $DIR/empty_line_after_outer_attribute.rs:5:1
+  |
+5 | / #[crate_type = "lib"]
+6 | |
+7 | | /// some comment
+8 | | fn with_one_newline_and_comment() { assert!(true) }
+  | |_
+  |
+  = note: `-D empty-line-after-outer-attr` implied by `-D warnings`
+
+error: Found an empty line after an outer attribute. Perhaps you forgot to add a '!' to make it an inner attribute?
+  --> $DIR/empty_line_after_outer_attribute.rs:17:1
+   |
+17 | / #[crate_type = "lib"]
+18 | |
+19 | | fn with_one_newline() { assert!(true) }
+   | |_
+
+error: Found an empty line after an outer attribute. Perhaps you forgot to add a '!' to make it an inner attribute?
+  --> $DIR/empty_line_after_outer_attribute.rs:22:1
+   |
+22 | / #[crate_type = "lib"]
+23 | |
+24 | |
+25 | | fn with_two_newlines() { assert!(true) }
+   | |_
+
+error: Found an empty line after an outer attribute. Perhaps you forgot to add a '!' to make it an inner attribute?
+  --> $DIR/empty_line_after_outer_attribute.rs:29:1
+   |
+29 | / #[crate_type = "lib"]
+30 | |
+31 | | enum Baz {
+   | |_
+
+error: Found an empty line after an outer attribute. Perhaps you forgot to add a '!' to make it an inner attribute?
+  --> $DIR/empty_line_after_outer_attribute.rs:37:1
+   |
+37 | / #[crate_type = "lib"]
+38 | |
+39 | | struct Foo {
+   | |_
+
+error: Found an empty line after an outer attribute. Perhaps you forgot to add a '!' to make it an inner attribute?
+  --> $DIR/empty_line_after_outer_attribute.rs:45:1
+   |
+45 | / #[crate_type = "lib"]
+46 | |
+47 | | mod foo {
+   | |_
+
+error: aborting due to 6 previous errors
+

tests/ui/inline_fn_without_body.rs

@@ -11,7 +11,6 @@ trait Foo {
     #[inline(always)]fn always_inline();
 
     #[inline(never)]
-
     fn never_inline();
 
     #[inline]

tests/ui/inline_fn_without_body.stderr

@@ -19,8 +19,7 @@ error: use of `#[inline]` on trait method `never_inline` which has no body
    |
 13 |       #[inline(never)]
    |  _____-^^^^^^^^^^^^^^^
-14 | |
-15 | |     fn never_inline();
+14 | |     fn never_inline();
    | |____- help: remove
 
 error: aborting due to 3 previous errors