From b7929cafe188b691a8e41021d3883d1a93dbb313 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Thu, 29 Mar 2018 21:14:53 +0200 Subject: [PATCH 1/5] 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 340d82dfa61..1de64683e88 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 125602319f7..e3202eed679 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 16eb95abbcb..99e55b2760d 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 00000000000..525a357bdc7 --- /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/5] 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 1de64683e88..553a98f682d 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/5] 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 e3202eed679..e3a7fc851b1 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 99e55b2760d..30063dac0a4 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 525a357bdc7..375df057544 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"]); } From 2a52527a463f3e96e38d2eba3ece1bb56d970f5c Mon Sep 17 00:00:00 2001 From: flip1995 Date: Sat, 31 Mar 2018 17:53:24 +0200 Subject: [PATCH 4/5] Fix lintlib script --- util/lintlib.py | 58 ++++++++++++++++++++++--------------------------- 1 file changed, 26 insertions(+), 32 deletions(-) diff --git a/util/lintlib.py b/util/lintlib.py index 1fddcbdc3fa..c28177e1062 100644 --- a/util/lintlib.py +++ b/util/lintlib.py @@ -7,12 +7,11 @@ import collections import logging as log log.basicConfig(level=log.INFO, format='%(levelname)s: %(message)s') -Lint = collections.namedtuple('Lint', 'name level doc sourcefile') +Lint = collections.namedtuple('Lint', 'name level doc sourcefile group') Config = collections.namedtuple('Config', 'name ty doc default') lintname_re = re.compile(r'''pub\s+([A-Z_][A-Z_0-9]*)''') -level_re = re.compile(r'''(Forbid|Deny|Warn|Allow)''') -group_re = re.compile(r'''([a-z_][a-z_0-9]+)''') +group_re = re.compile(r'''\s*([a-z_][a-z_0-9]+)''') conf_re = re.compile(r'''define_Conf! {\n([^}]*)\n}''', re.MULTILINE) confvar_re = re.compile( r'''/// Lint: (\w+). (.*).*\n\s*\([^,]+,\s+"([^"]+)",\s+([^=\)]+)=>\s+(.*)\),''', re.MULTILINE) @@ -27,6 +26,7 @@ lint_levels = { "nursery": 'Allow', } + def parse_lints(lints, filepath): last_comment = [] comment = True @@ -57,36 +57,30 @@ def parse_lints(lints, filepath): else: last_comment = [] if not comment: - if name: - g = group_re.search(line) - if g: - group = g.group(1).lower() - level = lint_levels[group] - log.info("found %s with level %s in %s", - name, level, filepath) - lints.append(Lint(name, level, last_comment, filepath, group)) - last_comment = [] - comment = True - else: - m = lintname_re.search(line) - if m: - name = m.group(1).lower() + m = lintname_re.search(line) + + if m: + name = m.group(1).lower() + line = next(fp) + + if deprecated: + level = "Deprecated" + group = "deprecated" + else: + while True: + g = group_re.search(line) + if g: + group = g.group(1).lower() + level = lint_levels[group] + break + line = next(fp) + + log.info("found %s with level %s in %s", + name, level, filepath) + lints.append(Lint(name, level, last_comment, filepath, group)) + last_comment = [] + comment = True - if deprecated: - level = "Deprecated" - else: - while True: - m = level_re.search(line) - if m: - level = m.group(0) - break - line = next(fp) - if not clippy: - log.info("found %s with level %s in %s", - name, level, filepath) - lints.append(Lint(name, level, last_comment, filepath, "deprecated")) - last_comment = [] - comment = True if "}" in line: log.warn("Warning: missing Lint-Name in %s", filepath) comment = True From 872db029cf8e85ec130af0ba9bf851ee347c3b14 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sun, 1 Apr 2018 15:31:25 +0200 Subject: [PATCH 5/5] Improve CONTRIBUTING.md * Incremental compilation is on by default * Restructured the label overview to go from easy to more difficult labels. --- CONTRIBUTING.md | 47 +++++++++++++++++++++++------------------------ 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 61e5cd67936..ebdb88a3ba9 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -15,41 +15,41 @@ High level approach: All issues on Clippy are mentored, if you want help with a bug just ask @Manishearth, @llogiq, @mcarton or @oli-obk. -Some issues are easier than others. The [good first issue](https://github.com/rust-lang-nursery/rust-clippy/labels/good%20first%20issue) +Some issues are easier than others. The [`good first issue`](https://github.com/rust-lang-nursery/rust-clippy/labels/good%20first%20issue) label can be used to find the easy issues. If you want to work on an issue, please leave a comment so that we can assign it to you! -Issues marked [T-AST](https://github.com/rust-lang-nursery/rust-clippy/labels/T-AST) involve simple +Issues marked [`T-AST`](https://github.com/rust-lang-nursery/rust-clippy/labels/T-AST) involve simple matching of the syntax tree structure, and are generally easier than -[T-middle](https://github.com/rust-lang-nursery/rust-clippy/labels/T-middle) issues, which involve types +[`T-middle`](https://github.com/rust-lang-nursery/rust-clippy/labels/T-middle) issues, which involve types and resolved paths. -Issues marked [E-medium](https://github.com/rust-lang-nursery/rust-clippy/labels/E-medium) are generally -pretty easy too, though it's recommended you work on an E-easy issue first. They are mostly classified -as `E-medium`, since they might be somewhat involved code wise, but not difficult per-se. - -[Llogiq's blog post on lints](https://llogiq.github.io/2015/06/04/workflows.html) is a nice primer -to lint-writing, though it does get into advanced stuff. Most lints consist of an implementation of -`LintPass` with one or more of its default methods overridden. See the existing lints for examples -of this. - -T-AST issues will generally need you to match against a predefined syntax structure. To figure out +[`T-AST`](https://github.com/rust-lang-nursery/rust-clippy/labels/T-AST) issues will generally need you to match against a predefined syntax structure. To figure out how this syntax structure is encoded in the AST, it is recommended to run `rustc -Z ast-json` on an example of the structure and compare with the [nodes in the AST docs](http://manishearth.github.io/rust-internals-docs/syntax/ast/). Usually the lint will end up to be a nested series of matches and ifs, [like so](https://github.com/rust-lang-nursery/rust-clippy/blob/de5ccdfab68a5e37689f3c950ed1532ba9d652a0/src/misc.rs#L34). -T-middle issues can be more involved and require verifying types. The +[`E-medium`](https://github.com/rust-lang-nursery/rust-clippy/labels/E-medium) issues are generally +pretty easy too, though it's recommended you work on an E-easy issue first. They are mostly classified +as `E-medium`, since they might be somewhat involved code wise, but not difficult per-se. + +[`T-middle`](https://github.com/rust-lang-nursery/rust-clippy/labels/T-middle) issues can +be more involved and require verifying types. The [`ty`](http://manishearth.github.io/rust-internals-docs/rustc/ty) module contains a lot of methods that are useful, though one of the most useful would be `expr_ty` (gives the type of an AST expression). `match_def_path()` in Clippy's `utils` module can also be useful. ### Writing code -Compiling clippy can take almost a minute or more depending on your machine. -You can set the environment flag `CARGO_INCREMENTAL=1` to cut down that time to -almost a third on average, depending on the influence your change has. +Compiling clippy from scratch can take almost a minute or more depending on your machine. +However, since Rust 1.24.0 incremental compilation is enabled by default and compile times for small changes should be quick. + +[Llogiq's blog post on lints](https://llogiq.github.io/2015/06/04/workflows.html) is a nice primer +to lint-writing, though it does get into advanced stuff. Most lints consist of an implementation of +`LintPass` with one or more of its default methods overridden. See the existing lints for examples +of this. Please document your lint with a doc comment akin to the following: @@ -61,8 +61,13 @@ Please document your lint with a doc comment akin to the following: /// **Known problems:** None. (Or describe where it could go wrong.) /// /// **Example:** +/// /// ```rust -/// Insert a short example if you have one. +/// // Bad +/// Insert a short example of code that triggers the lint +/// +/// // Good +/// Insert a short example of improved code that doesn't trigger the lint /// ``` ``` @@ -80,12 +85,6 @@ If you don't want to wait for all tests to finish, you can also execute a single TESTNAME=ui/empty_line_after_outer_attr cargo test --test compile-test ``` -And you can also combine this with `CARGO_INCREMENTAL`: - -```bash -CARGO_INCREMENTAL=1 TESTNAME=ui/doc cargo test --test compile-test -``` - ### Testing manually Manually testing against an example file is useful if you have added some