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 diff --git a/clippy_lints/src/attrs.rs b/clippy_lints/src/attrs.rs index 340d82dfa61..553a98f682d 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. @@ -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" } @@ -276,6 +280,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..e3a7fc851b1 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -1086,3 +1086,36 @@ 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![]; + + let mut nest_level = 0; + + for line in lines.into_iter() { + if line.contains("/*") { + nest_level += 1; + continue; + } else if line.contains("*/") { + nest_level -= 1; + continue; + } + + if nest_level == 0 { + 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..30063dac0a4 100644 --- a/tests/ui/empty_line_after_outer_attribute.rs +++ b/tests/ui/empty_line_after_outer_attribute.rs @@ -79,4 +79,16 @@ pub enum FooFighter { Bar4 } +// 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 new file mode 100644 index 00000000000..375df057544 --- /dev/null +++ b/tests/without_block_comments.rs @@ -0,0 +1,29 @@ +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!["/* 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"]); +} 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