From b7929cafe188b691a8e41021d3883d1a93dbb313 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Thu, 29 Mar 2018 21:14:53 +0200 Subject: [PATCH 1/3] Fix false positive in empty_line_after_outer_attr Before, when you had a block comment between an attribute and the following item like this: ```rust \#[crate_type = "lib"] /* */ pub struct Rust; ``` It would cause a false positive on the lint, because there is an empty line inside the block comment. This makes sure that basic block comments are detected and removed from the snippet that was created before. --- clippy_lints/src/attrs.rs | 4 ++- clippy_lints/src/utils/mod.rs | 34 ++++++++++++++++++++ tests/ui/empty_line_after_outer_attribute.rs | 7 ++++ tests/without_block_comments.rs | 20 ++++++++++++ 4 files changed, 64 insertions(+), 1 deletion(-) create mode 100644 tests/without_block_comments.rs diff --git a/clippy_lints/src/attrs.rs b/clippy_lints/src/attrs.rs index 340d82dfa61d..1de64683e883 100644 --- a/clippy_lints/src/attrs.rs +++ b/clippy_lints/src/attrs.rs @@ -7,7 +7,7 @@ use rustc::ty::{self, TyCtxt}; use semver::Version; use syntax::ast::{Attribute, AttrStyle, Lit, LitKind, MetaItemKind, NestedMetaItem, NestedMetaItemKind}; use syntax::codemap::Span; -use utils::{in_macro, last_line_of_span, match_def_path, opt_def_id, paths, snippet_opt, span_lint, span_lint_and_then}; +use utils::{in_macro, last_line_of_span, match_def_path, opt_def_id, paths, snippet_opt, span_lint, span_lint_and_then, without_block_comments}; /// **What it does:** Checks for items annotated with `#[inline(always)]`, /// unless the annotated function is empty or simply panics. @@ -276,6 +276,8 @@ fn check_attrs(cx: &LateContext, span: Span, name: &Name, attrs: &[Attribute]) { if let Some(snippet) = snippet_opt(cx, end_of_attr_to_item) { let lines = snippet.split('\n').collect::>(); + let lines = without_block_comments(lines); + if lines.iter().filter(|l| l.trim().is_empty()).count() > 2 { span_lint( cx, diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 125602319f7b..e3202eed6797 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -1086,3 +1086,37 @@ pub fn clip(tcx: TyCtxt, u: u128, ity: ast::UintTy) -> u128 { let amt = 128 - bits; (u << amt) >> amt } + +/// Remove block comments from the given Vec of lines +/// +/// # Examples +/// +/// ```rust,ignore +/// without_block_comments(vec!["/*", "foo", "*/"]); +/// // => vec![] +/// +/// without_block_comments(vec!["bar", "/*", "foo", "*/"]); +/// // => vec!["bar"] +/// ``` +pub fn without_block_comments(lines: Vec<&str>) -> Vec<&str> { + let mut without = vec![]; + + // naive approach for block comments + let mut inside_comment = false; + + for line in lines.into_iter() { + if line.contains("/*") { + inside_comment = true; + continue; + } else if line.contains("*/") { + inside_comment = false; + continue; + } + + if !inside_comment { + without.push(line); + } + } + + without +} diff --git a/tests/ui/empty_line_after_outer_attribute.rs b/tests/ui/empty_line_after_outer_attribute.rs index 16eb95abbcbf..99e55b2760dc 100644 --- a/tests/ui/empty_line_after_outer_attribute.rs +++ b/tests/ui/empty_line_after_outer_attribute.rs @@ -79,4 +79,11 @@ pub enum FooFighter { Bar4 } +// This should not produce a warning because there is a comment in between +#[crate_type = "lib"] +/* + +*/ +pub struct S; + fn main() { } diff --git a/tests/without_block_comments.rs b/tests/without_block_comments.rs new file mode 100644 index 000000000000..525a357bdc75 --- /dev/null +++ b/tests/without_block_comments.rs @@ -0,0 +1,20 @@ +extern crate clippy_lints; +use clippy_lints::utils::without_block_comments; + +#[test] +fn test_lines_without_block_comments() { + let result = without_block_comments(vec!["/*", "", "*/"]); + println!("result: {:?}", result); + assert!(result.is_empty()); + + let result = without_block_comments( + vec!["", "/*", "", "*/", "#[crate_type = \"lib\"]", "/*", "", "*/", ""] + ); + assert_eq!(result, vec!["", "#[crate_type = \"lib\"]", ""]); + + let result = without_block_comments(vec!["/* rust", "", "*/"]); + assert!(result.is_empty()); + + let result = without_block_comments(vec!["foo", "bar", "baz"]); + assert_eq!(result, vec!["foo", "bar", "baz"]); +} From bb4af196beb20d485b2b56dff8fa023f6ee00a56 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 30 Mar 2018 11:28:37 +0200 Subject: [PATCH 2/3] Move empty_line_after_outer_attribute to nursery From the clippy side it's difficult to detect empty lines between an attributes and the following item because empty lines and comments are not part of the AST. The parsing currently works for basic cases but is not perfect and can cause false positives. Maybe libsyntax 2.0 will fix some of the problems around attributes but comments will probably be never part of the AST so we would still have to do some manual parsing. --- clippy_lints/src/attrs.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/attrs.rs b/clippy_lints/src/attrs.rs index 1de64683e883..553a98f682de 100644 --- a/clippy_lints/src/attrs.rs +++ b/clippy_lints/src/attrs.rs @@ -85,7 +85,11 @@ declare_clippy_lint! { /// If it was meant to be an outer attribute, then the following item /// should not be separated by empty lines. /// -/// **Known problems:** None +/// **Known problems:** Can cause false positives. +/// +/// From the clippy side it's difficult to detect empty lines between an attributes and the +/// following item because empty lines and comments are not part of the AST. The parsing +/// currently works for basic cases but is not perfect. /// /// **Example:** /// ```rust @@ -105,7 +109,7 @@ declare_clippy_lint! { /// ``` declare_clippy_lint! { pub EMPTY_LINE_AFTER_OUTER_ATTR, - style, + nursery, "empty line after outer attribute" } From db1ec446160ef990675082f3208616de3157a91f Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 30 Mar 2018 12:36:30 +0200 Subject: [PATCH 3/3] Handle nested block comments --- clippy_lints/src/utils/mod.rs | 9 ++++----- tests/ui/empty_line_after_outer_attribute.rs | 7 ++++++- tests/without_block_comments.rs | 9 +++++++++ 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index e3202eed6797..e3a7fc851b15 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -1101,19 +1101,18 @@ pub fn clip(tcx: TyCtxt, u: u128, ity: ast::UintTy) -> u128 { pub fn without_block_comments(lines: Vec<&str>) -> Vec<&str> { let mut without = vec![]; - // naive approach for block comments - let mut inside_comment = false; + let mut nest_level = 0; for line in lines.into_iter() { if line.contains("/*") { - inside_comment = true; + nest_level += 1; continue; } else if line.contains("*/") { - inside_comment = false; + nest_level -= 1; continue; } - if !inside_comment { + if nest_level == 0 { without.push(line); } } diff --git a/tests/ui/empty_line_after_outer_attribute.rs b/tests/ui/empty_line_after_outer_attribute.rs index 99e55b2760dc..30063dac0a46 100644 --- a/tests/ui/empty_line_after_outer_attribute.rs +++ b/tests/ui/empty_line_after_outer_attribute.rs @@ -79,11 +79,16 @@ pub enum FooFighter { Bar4 } -// This should not produce a warning because there is a comment in between +// This should not produce a warning because the empty line is inside a block comment #[crate_type = "lib"] /* */ pub struct S; +// This should not produce a warning +#[crate_type = "lib"] +/* test */ +pub struct T; + fn main() { } diff --git a/tests/without_block_comments.rs b/tests/without_block_comments.rs index 525a357bdc75..375df0575449 100644 --- a/tests/without_block_comments.rs +++ b/tests/without_block_comments.rs @@ -15,6 +15,15 @@ fn test_lines_without_block_comments() { let result = without_block_comments(vec!["/* rust", "", "*/"]); assert!(result.is_empty()); + let result = without_block_comments(vec!["/* one-line comment */"]); + assert!(result.is_empty()); + + let result = without_block_comments(vec!["/* nested", "/* multi-line", "comment", "*/", "test", "*/"]); + assert!(result.is_empty()); + + let result = without_block_comments(vec!["/* nested /* inline /* comment */ test */ */"]); + assert!(result.is_empty()); + let result = without_block_comments(vec!["foo", "bar", "baz"]); assert_eq!(result, vec!["foo", "bar", "baz"]); }