diff --git a/clippy_dev/src/main.rs b/clippy_dev/src/main.rs index 22a2f2ca79c7..58b4f87e8727 100644 --- a/clippy_dev/src/main.rs +++ b/clippy_dev/src/main.rs @@ -4,6 +4,7 @@ use clap::{App, Arg, SubCommand}; use clippy_dev::*; mod fmt; +mod new_lint; mod stderr_length_check; #[derive(PartialEq)] @@ -51,6 +52,47 @@ fn main() { .help("Checks that util/dev update_lints has been run. Used on CI."), ), ) + .subcommand( + SubCommand::with_name("new_lint") + .about("Create new lint and run util/dev update_lints") + .arg( + Arg::with_name("pass") + .short("p") + .long("pass") + .help("Specify whether the lint runs during the early or late pass") + .takes_value(true) + .possible_values(&["early", "late"]) + .required(true), + ) + .arg( + Arg::with_name("name") + .short("n") + .long("name") + .help("Name of the new lint in snake case, ex: fn_too_long") + .takes_value(true) + .required(true), + ) + .arg( + Arg::with_name("category") + .short("c") + .long("category") + .help("What category the lint belongs to") + .default_value("nursery") + .possible_values(&[ + "style", + "correctness", + "complexity", + "perf", + "pedantic", + "restriction", + "cargo", + "nursery", + "internal", + "internal_warn", + ]) + .takes_value(true), + ), + ) .arg( Arg::with_name("limit-stderr-length") .long("limit-stderr-length") @@ -75,6 +117,16 @@ fn main() { update_lints(&UpdateMode::Change); } }, + ("new_lint", Some(matches)) => { + match new_lint::create( + matches.value_of("pass"), + matches.value_of("name"), + matches.value_of("category"), + ) { + Ok(_) => update_lints(&UpdateMode::Change), + Err(e) => eprintln!("Unable to create lint: {}", e), + } + }, _ => {}, } } diff --git a/clippy_dev/src/new_lint.rs b/clippy_dev/src/new_lint.rs new file mode 100644 index 000000000000..c5733be45eba --- /dev/null +++ b/clippy_dev/src/new_lint.rs @@ -0,0 +1,182 @@ +use std::fs::{File, OpenOptions}; +use std::io; +use std::io::prelude::*; +use std::io::ErrorKind; +use std::path::{Path, PathBuf}; + +pub fn create(pass: Option<&str>, lint_name: Option<&str>, category: Option<&str>) -> Result<(), io::Error> { + let pass = pass.expect("`pass` argument is validated by clap"); + let lint_name = lint_name.expect("`name` argument is validated by clap"); + let category = category.expect("`category` argument is validated by clap"); + + match open_files(lint_name) { + Ok((mut test_file, mut lint_file)) => { + let (pass_type, pass_import, context_import) = match pass { + "early" => ("EarlyLintPass", "use syntax::ast::*;", "EarlyContext"), + "late" => ("LateLintPass", "use rustc_hir::*;", "LateContext"), + _ => { + unreachable!("`pass_type` should only ever be `early` or `late`!"); + }, + }; + + let camel_case_name = to_camel_case(lint_name); + + if let Err(e) = test_file.write_all(get_test_file_contents(lint_name).as_bytes()) { + return Err(io::Error::new( + ErrorKind::Other, + format!("Could not write to test file: {}", e), + )); + }; + + if let Err(e) = lint_file.write_all( + get_lint_file_contents( + pass_type, + lint_name, + &camel_case_name, + category, + pass_import, + context_import, + ) + .as_bytes(), + ) { + return Err(io::Error::new( + ErrorKind::Other, + format!("Could not write to lint file: {}", e), + )); + } + Ok(()) + }, + Err(e) => Err(io::Error::new( + ErrorKind::Other, + format!("Unable to create lint: {}", e), + )), + } +} + +fn open_files(lint_name: &str) -> Result<(File, File), io::Error> { + let project_root = project_root()?; + + let test_file_path = project_root.join("tests").join("ui").join(format!("{}.rs", lint_name)); + let lint_file_path = project_root + .join("clippy_lints") + .join("src") + .join(format!("{}.rs", lint_name)); + + if Path::new(&test_file_path).exists() { + return Err(io::Error::new( + ErrorKind::AlreadyExists, + format!("test file {:?} already exists", test_file_path), + )); + } + if Path::new(&lint_file_path).exists() { + return Err(io::Error::new( + ErrorKind::AlreadyExists, + format!("lint file {:?} already exists", lint_file_path), + )); + } + + let test_file = OpenOptions::new().write(true).create_new(true).open(test_file_path)?; + let lint_file = OpenOptions::new().write(true).create_new(true).open(lint_file_path)?; + + Ok((test_file, lint_file)) +} + +fn project_root() -> Result { + let current_dir = std::env::current_dir()?; + for path in current_dir.ancestors() { + let result = std::fs::read_to_string(path.join("Cargo.toml")); + if let Err(err) = &result { + if err.kind() == io::ErrorKind::NotFound { + continue; + } + } + + let content = result?; + if content.contains("[package]\nname = \"clippy\"") { + return Ok(path.to_path_buf()); + } + } + Err(io::Error::new(ErrorKind::Other, "Unable to find project root")) +} + +fn to_camel_case(name: &str) -> String { + name.split('_') + .map(|s| { + if s.is_empty() { + String::from("") + } else { + [&s[0..1].to_uppercase(), &s[1..]].concat() + } + }) + .collect() +} + +fn get_test_file_contents(lint_name: &str) -> String { + format!( + "#![warn(clippy::{})] + +fn main() {{ + // test code goes here +}} +", + lint_name + ) +} + +fn get_lint_file_contents( + pass_type: &str, + lint_name: &str, + camel_case_name: &str, + category: &str, + pass_import: &str, + context_import: &str, +) -> String { + format!( + "use rustc::lint::{{LintArray, LintPass, {type}, {context_import}}}; +use rustc_session::{{declare_lint_pass, declare_tool_lint}}; +{pass_import} + +declare_clippy_lint! {{ + /// **What it does:** + /// + /// **Why is this bad?** + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// // example code + /// ``` + pub {name_upper}, + {category}, + \"default lint description\" +}} + +declare_lint_pass!({name_camel} => [{name_upper}]); + +impl {type} for {name_camel} {{}} +", + type=pass_type, + name_upper=lint_name.to_uppercase(), + name_camel=camel_case_name, + category=category, + pass_import=pass_import, + context_import=context_import + ) +} + +#[test] +fn test_camel_case() { + let s = "a_lint"; + let s2 = to_camel_case(s); + assert_eq!(s2, "ALint"); + + let name = "a_really_long_new_lint"; + let name2 = to_camel_case(name); + assert_eq!(name2, "AReallyLongNewLint"); + + let name3 = "lint__name"; + let name4 = to_camel_case(name3); + assert_eq!(name4, "LintName"); +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 576a4a8cd7c2..c1fe51fbf8af 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1090,6 +1090,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&utils::internal_lints::LINT_WITHOUT_LINT_PASS), LintId::of(&utils::internal_lints::OUTER_EXPN_EXPN_DATA), LintId::of(&utils::internal_lints::PRODUCE_ICE), + LintId::of(&utils::internal_lints::DEFAULT_LINT), ]); store.register_group(true, "clippy::all", Some("clippy"), vec![ diff --git a/clippy_lints/src/utils/internal_lints.rs b/clippy_lints/src/utils/internal_lints.rs index 825e796382b5..dcedaedc27f2 100644 --- a/clippy_lints/src/utils/internal_lints.rs +++ b/clippy_lints/src/utils/internal_lints.rs @@ -13,10 +13,10 @@ use rustc_hir::*; use rustc_lint::{EarlyContext, EarlyLintPass, LateContext, LateLintPass}; use rustc_session::declare_tool_lint; use rustc_session::{declare_lint_pass, impl_lint_pass}; -use rustc_span::source_map::Span; +use rustc_span::source_map::{Span, Spanned}; use rustc_span::symbol::SymbolStr; use syntax::ast; -use syntax::ast::{Crate as AstCrate, ItemKind, Name}; +use syntax::ast::{Crate as AstCrate, ItemKind, LitKind, Name}; use syntax::visit::FnKind; declare_clippy_lint! { @@ -121,6 +121,29 @@ declare_clippy_lint! { "this message should not appear anywhere as we ICE before and don't emit the lint" } +declare_clippy_lint! { + /// **What it does:** Checks for cases of an auto-generated lint without an updated description, + /// i.e. `default lint description`. + /// + /// **Why is this bad?** Indicates that the lint is not finished. + /// + /// **Known problems:** None + /// + /// **Example:** + /// Bad: + /// ```rust,ignore + /// declare_lint! { pub COOL_LINT, nursery, "default lint description" } + /// ``` + /// + /// Good: + /// ```rust,ignore + /// declare_lint! { pub COOL_LINT, nursery, "a great new lint" } + /// ``` + pub DEFAULT_LINT, + internal, + "found 'default lint description' in a lint declaration" +} + declare_lint_pass!(ClippyLintsInternal => [CLIPPY_LINTS_INTERNAL]); impl EarlyLintPass for ClippyLintsInternal { @@ -163,12 +186,34 @@ pub struct LintWithoutLintPass { registered_lints: FxHashSet, } -impl_lint_pass!(LintWithoutLintPass => [LINT_WITHOUT_LINT_PASS]); +impl_lint_pass!(LintWithoutLintPass => [DEFAULT_LINT, LINT_WITHOUT_LINT_PASS]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LintWithoutLintPass { fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx Item<'_>) { - if let hir::ItemKind::Static(ref ty, Mutability::Not, _) = item.kind { + if let hir::ItemKind::Static(ref ty, Mutability::Not, body_id) = item.kind { if is_lint_ref_type(cx, ty) { + let expr = &cx.tcx.hir().body(body_id).value; + if_chain! { + if let ExprKind::AddrOf(_, _, ref inner_exp) = expr.kind; + if let ExprKind::Struct(_, ref fields, _) = inner_exp.kind; + let field = fields.iter() + .find(|f| f.ident.as_str() == "desc") + .expect("lints must have a description field"); + if let ExprKind::Lit(Spanned { + node: LitKind::Str(ref sym, _), + .. + }) = field.expr.kind; + if sym.as_str() == "default lint description"; + + then { + span_lint( + cx, + DEFAULT_LINT, + item.span, + &format!("the lint `{}` has the default lint description", item.ident.name), + ); + } + } self.declared_lints.insert(item.ident.name, item.span); } } else if is_expn_of(item.span, "impl_lint_pass").is_some() diff --git a/doc/adding_lints.md b/doc/adding_lints.md index 45b80a6b0c12..6fd052893bfc 100644 --- a/doc/adding_lints.md +++ b/doc/adding_lints.md @@ -9,6 +9,7 @@ because that's clearly a non-descriptive name. - [Adding a new lint](#adding-a-new-lint) - [Setup](#setup) + - [Getting Started](#getting-started) - [Testing](#testing) - [Rustfix tests](#rustfix-tests) - [Edition 2018 tests](#edition-2018-tests) @@ -31,6 +32,19 @@ which can change rapidly. Make sure you're working near rust-clippy's master, and use the `setup-toolchain.sh` script to configure the appropriate toolchain for the Clippy directory. +### Getting Started + +There is a bit of boilerplate code that needs to be set up when creating a new +lint. Fortunately, you can use the clippy dev tools to handle this for you. We +are naming our new lint `foo_functions` (lints are generally written in snake +case), and we don't need type information so it will have an early pass type +(more on this later on). To get started on this lint you can run +`./util/dev new_lint --name=foo_functions --pass=early --category=pedantic` +(category will default to nursery if not provided). This command will create +two files: `tests/ui/foo_functions.rs` and `clippy_lints/src/foo_functions.rs`, +as well as run `./util/dev update_lints` to register the new lint. Next, we'll +open up these files and add our lint! + ### Testing Let's write some tests first that we can execute while we iterate on our lint. @@ -41,11 +55,9 @@ we want to check. The output of Clippy is compared against a `.stderr` file. Note that you don't have to create this file yourself, we'll get to generating the `.stderr` files further down. -We start by creating the test file at `tests/ui/foo_functions.rs`. It doesn't -really matter what the file is called, but it's a good convention to name it -after the lint it is testing, so `foo_functions.rs` it is. +We start by opening the test file created at `tests/ui/foo_functions.rs`. -Inside the file we put some examples to get started: +Update the file with some examples to get started: ```rust #![warn(clippy::foo_functions)] @@ -90,8 +102,8 @@ Once we are satisfied with the output, we need to run `tests/ui/update-all-references.sh` to update the `.stderr` file for our lint. Please note that, we should run `TESTNAME=foo_functions cargo uitest` every time before running `tests/ui/update-all-references.sh`. -Running `TESTNAME=foo_functions cargo uitest` should pass then. When we -commit our lint, we need to commit the generated `.stderr` files, too. +Running `TESTNAME=foo_functions cargo uitest` should pass then. When we commit +our lint, we need to commit the generated `.stderr` files, too. ### Rustfix tests @@ -121,26 +133,42 @@ With tests in place, let's have a look at implementing our lint now. ### Lint declaration -We start by creating a new file in the `clippy_lints` crate. That's the crate -where all the lint code is. We are going to call the file -`clippy_lints/src/foo_functions.rs` and import some initial things we need: +Let's start by opening the new file created in the `clippy_lints` crate +at `clippy_lints/src/foo_functions.rs`. That's the crate where all the +lint code is. This file has already imported some initial things we will need: ```rust -use rustc::lint::{LintArray, LintPass, EarlyLintPass}; +use rustc::lint::{LintArray, LintPass, EarlyLintPass, EarlyContext}; use rustc_session::{declare_lint_pass, declare_tool_lint}; +use syntax::ast::*; ``` -The next step is to provide a lint declaration. Lints are declared using the -[`declare_clippy_lint!`][declare_clippy_lint] macro: +The next step is to update the lint declaration. Lints are declared using the +[`declare_clippy_lint!`][declare_clippy_lint] macro, and we just need to update +the auto-generated lint declaration to have a real description, something like this: ```rust declare_clippy_lint! { + /// **What it does:** + /// + /// **Why is this bad?** + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// // example code + /// ``` pub FOO_FUNCTIONS, pedantic, "function named `foo`, which is not a descriptive name" } ``` +* The section of lines prefixed with `///` constitutes the lint documentation +section. This is the default documentation style and will be displayed at +https://rust-lang.github.io/rust-clippy/master/index.html. * `FOO_FUNCTIONS` is the name of our lint. Be sure to follow the [lint naming guidelines][lint_naming] here when naming your lint. In short, the name should state the thing that is being checked for and read well when used with @@ -150,8 +178,8 @@ state the thing that is being checked for and read well when used with * The last part should be a text that explains what exactly is wrong with the code -With our lint declaration done, we will now make sure that it is assigned to a -lint pass: +The rest of this file contains an empty implementation for our lint pass, +which in this case is `EarlyLintPass` and should look like this: ```rust // clippy_lints/src/foo_functions.rs @@ -166,12 +194,9 @@ impl EarlyLintPass for FooFunctions {} Don't worry about the `name` method here. As long as it includes the name of the lint pass it should be fine. -Next we need to run `util/dev update_lints` to register the lint in various -places, mainly in `clippy_lints/src/lib.rs`. - -While `update_lints` automates some things, it doesn't automate everything. We -will have to register our lint pass manually in the `register_plugins` function -in `clippy_lints/src/lib.rs`: +The new lint automation runs `update_lints`, which automates some things, but it +doesn't automate everything. We will have to register our lint pass manually in +the `register_plugins` function in `clippy_lints/src/lib.rs`: ```rust reg.register_early_lint_pass(box foo_functions::FooFunctions); @@ -195,14 +220,9 @@ In short, the `LateLintPass` has access to type information while the `EarlyLintPass`. The `EarlyLintPass` is also faster. However linting speed hasn't really been a concern with Clippy so far. -Since we don't need type information for checking the function name, we are -going to use the `EarlyLintPass`. It has to be imported as well, changing our -imports to: - -```rust -use rustc::lint::{LintArray, LintPass, EarlyLintPass, EarlyContext}; -use rustc::{declare_tool_lint, lint_array}; -``` +Since we don't need type information for checking the function name, we used +`--pass=early` when running the new lint automation and all the imports were +added accordingly. ### Emitting a lint diff --git a/tests/ui/default_lint.rs b/tests/ui/default_lint.rs new file mode 100644 index 000000000000..988a7b866dea --- /dev/null +++ b/tests/ui/default_lint.rs @@ -0,0 +1,28 @@ +#![deny(clippy::internal)] +#![feature(rustc_private)] + +#[macro_use] +extern crate rustc; +#[macro_use] +extern crate rustc_session; +extern crate rustc_lint; +use rustc_lint::{LintArray, LintPass}; + +declare_tool_lint! { + pub clippy::TEST_LINT, + Warn, + "", + report_in_external_macro: true +} + +declare_tool_lint! { + pub clippy::TEST_LINT_DEFAULT, + Warn, + "default lint description", + report_in_external_macro: true +} + +declare_lint_pass!(Pass => [TEST_LINT]); +declare_lint_pass!(Pass2 => [TEST_LINT_DEFAULT]); + +fn main() {} diff --git a/tests/ui/default_lint.stderr b/tests/ui/default_lint.stderr new file mode 100644 index 000000000000..21fd86ec6aad --- /dev/null +++ b/tests/ui/default_lint.stderr @@ -0,0 +1,21 @@ +error: the lint `TEST_LINT_DEFAULT` has the default lint description + --> $DIR/default_lint.rs:18:1 + | +LL | / declare_tool_lint! { +LL | | pub clippy::TEST_LINT_DEFAULT, +LL | | Warn, +LL | | "default lint description", +LL | | report_in_external_macro: true +LL | | } + | |_^ + | +note: lint level defined here + --> $DIR/default_lint.rs:1:9 + | +LL | #![deny(clippy::internal)] + | ^^^^^^^^^^^^^^^^ + = note: `#[deny(clippy::default_lint)]` implied by `#[deny(clippy::internal)]` + = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) + +error: aborting due to previous error +