From 80827c1f749b0da9fab95f2d47417f91d52c4a46 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Tue, 9 Jan 2018 00:22:42 +0100 Subject: [PATCH 1/4] Warn on empty lines after outer attributes --- clippy_lints/src/attrs.rs | 53 ++++++++++++++++++- clippy_lints/src/lib.rs | 1 + tests/ui/empty_line_after_outer_attribute.rs | 21 ++++++++ .../empty_line_after_outer_attribute.stderr | 19 +++++++ 4 files changed, 92 insertions(+), 2 deletions(-) create mode 100644 tests/ui/empty_line_after_outer_attribute.rs create mode 100644 tests/ui/empty_line_after_outer_attribute.stderr diff --git a/clippy_lints/src/attrs.rs b/clippy_lints/src/attrs.rs index da7fff2ed93a..a96193bacf11 100644 --- a/clippy_lints/src/attrs.rs +++ b/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) } } @@ -230,6 +262,23 @@ fn check_attrs(cx: &LateContext, span: Span, name: &Name, attrs: &[Attribute]) { } for attr in attrs { + if attr.style == AttrStyle::Outer { + 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::>(); + 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; diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 62df7fd10587..723d15e87608 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -438,6 +438,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, diff --git a/tests/ui/empty_line_after_outer_attribute.rs b/tests/ui/empty_line_after_outer_attribute.rs new file mode 100644 index 000000000000..fa8958612f8c --- /dev/null +++ b/tests/ui/empty_line_after_outer_attribute.rs @@ -0,0 +1,21 @@ + +#![warn(empty_line_after_outer_attr)] + +// 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 not produce a warning +#[allow(non_camel_case_types)] +#[allow(missing_docs)] +#[allow(missing_docs)] +fn three_attributes() { assert!(true) } + +fn main() { } diff --git a/tests/ui/empty_line_after_outer_attribute.stderr b/tests/ui/empty_line_after_outer_attribute.stderr new file mode 100644 index 000000000000..04de89c60f67 --- /dev/null +++ b/tests/ui/empty_line_after_outer_attribute.stderr @@ -0,0 +1,19 @@ +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 | | fn with_one_newline() { 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:10:1 + | +10 | / #[crate_type = "lib"] +11 | | +12 | | +13 | | fn with_two_newlines() { assert!(true) } + | |_ + From 83909398d288579837922e68482f2e22b24f8406 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 19 Jan 2018 08:18:29 +0100 Subject: [PATCH 2/4] Add test case for comments between item and attr --- tests/ui/empty_line_after_outer_attribute.rs | 12 ++++++++++ .../empty_line_after_outer_attribute.stderr | 23 ++++++++++++++----- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/tests/ui/empty_line_after_outer_attribute.rs b/tests/ui/empty_line_after_outer_attribute.rs index fa8958612f8c..648a25f1dd07 100644 --- a/tests/ui/empty_line_after_outer_attribute.rs +++ b/tests/ui/empty_line_after_outer_attribute.rs @@ -1,6 +1,18 @@ #![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"] diff --git a/tests/ui/empty_line_after_outer_attribute.stderr b/tests/ui/empty_line_after_outer_attribute.stderr index 04de89c60f67..481f95443ce8 100644 --- a/tests/ui/empty_line_after_outer_attribute.stderr +++ b/tests/ui/empty_line_after_outer_attribute.stderr @@ -3,17 +3,28 @@ error: Found an empty line after an outer attribute. Perhaps you forgot to add a | 5 | / #[crate_type = "lib"] 6 | | -7 | | fn with_one_newline() { assert!(true) } +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:10:1 + --> $DIR/empty_line_after_outer_attribute.rs:17:1 | -10 | / #[crate_type = "lib"] -11 | | -12 | | -13 | | fn with_two_newlines() { assert!(true) } +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: aborting due to 3 previous errors + From aade0d563e1eb668566872d4c300a734b67f6600 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Tue, 23 Jan 2018 21:32:06 +0100 Subject: [PATCH 3/4] Make lint work on all members of ast::Item_ --- clippy_lints/src/attrs.rs | 2 +- tests/ui/empty_line_after_outer_attribute.rs | 23 ++++++++++++++++ .../empty_line_after_outer_attribute.stderr | 26 ++++++++++++++++++- tests/ui/inline_fn_without_body.rs | 1 - tests/ui/inline_fn_without_body.stderr | 3 +-- 5 files changed, 50 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/attrs.rs b/clippy_lints/src/attrs.rs index a96193bacf11..5df932a8d93e 100644 --- a/clippy_lints/src/attrs.rs +++ b/clippy_lints/src/attrs.rs @@ -203,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 } } diff --git a/tests/ui/empty_line_after_outer_attribute.rs b/tests/ui/empty_line_after_outer_attribute.rs index 648a25f1dd07..3d62a4913acf 100644 --- a/tests/ui/empty_line_after_outer_attribute.rs +++ b/tests/ui/empty_line_after_outer_attribute.rs @@ -24,6 +24,29 @@ fn with_one_newline() { assert!(true) } 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)] diff --git a/tests/ui/empty_line_after_outer_attribute.stderr b/tests/ui/empty_line_after_outer_attribute.stderr index 481f95443ce8..7c9c7b8f3495 100644 --- a/tests/ui/empty_line_after_outer_attribute.stderr +++ b/tests/ui/empty_line_after_outer_attribute.stderr @@ -26,5 +26,29 @@ error: Found an empty line after an outer attribute. Perhaps you forgot to add a 25 | | fn with_two_newlines() { assert!(true) } | |_ -error: aborting due to 3 previous errors +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 diff --git a/tests/ui/inline_fn_without_body.rs b/tests/ui/inline_fn_without_body.rs index 82e073184d35..76e50e56780c 100644 --- a/tests/ui/inline_fn_without_body.rs +++ b/tests/ui/inline_fn_without_body.rs @@ -11,7 +11,6 @@ trait Foo { #[inline(always)]fn always_inline(); #[inline(never)] - fn never_inline(); #[inline] diff --git a/tests/ui/inline_fn_without_body.stderr b/tests/ui/inline_fn_without_body.stderr index fd26013d11ea..2b466b686103 100644 --- a/tests/ui/inline_fn_without_body.stderr +++ b/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 From 3d54e56ed4898c2b01dd008f12ab41cf54710f43 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 26 Jan 2018 07:51:27 +0100 Subject: [PATCH 4/4] Add workaround for hidden outer attribute 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. --- clippy_lints/src/attrs.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/clippy_lints/src/attrs.rs b/clippy_lints/src/attrs.rs index 5df932a8d93e..939fdf1fae99 100644 --- a/clippy_lints/src/attrs.rs +++ b/clippy_lints/src/attrs.rs @@ -263,6 +263,10 @@ 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) { @@ -319,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 +}