Auto merge of #5614 - ebroto:test_cargo_lints, r=flip1995

Test cargo lints

changelog: Add infrastructure to test cargo lints

Closes #5603

clippy_dev/src/new_lint.rs

@@ -1,91 +1,110 @@
 use crate::clippy_project_root;
-use std::fs::{File, OpenOptions};
-use std::io;
+use std::fs::{self, OpenOptions};
 use std::io::prelude::*;
-use std::io::ErrorKind;
-use std::path::Path;
+use std::io::{self, ErrorKind};
+use std::path::{Path, PathBuf};
 
-/// Creates files required to implement and test a new lint and runs `update_lints`.
-///
-/// # Errors
-///
-/// This function errors, if the files couldn't be created
-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");
+struct LintData<'a> {
+    pass: &'a str,
+    name: &'a str,
+    category: &'a str,
+    project_root: PathBuf,
+}
 
-    match open_files(lint_name) {
-        Ok((mut test_file, mut lint_file)) => {
-            let (pass_type, pass_lifetimes, pass_import, context_import) = match pass {
-                "early" => ("EarlyLintPass", "", "use rustc_ast::ast::*;", "EarlyContext"),
-                "late" => ("LateLintPass", "<'_, '_>", "use rustc_hir::*;", "LateContext"),
-                _ => {
-                    unreachable!("`pass_type` should only ever be `early` or `late`!");
-                },
-            };
+trait Context {
+    fn context<C: AsRef<str>>(self, text: C) -> Self;
+}
 
-            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,
-                    pass_lifetimes,
-                    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),
-        )),
+impl<T> Context for io::Result<T> {
+    fn context<C: AsRef<str>>(self, text: C) -> Self {
+        match self {
+            Ok(t) => Ok(t),
+            Err(e) => {
+                let message = format!("{}: {}", text.as_ref(), e);
+                Err(io::Error::new(ErrorKind::Other, message))
+            },
+        }
     }
 }
 
-fn open_files(lint_name: &str) -> Result<(File, File), io::Error> {
-    let project_root = clippy_project_root();
+/// Creates the files required to implement and test a new lint and runs `update_lints`.
+///
+/// # Errors
+///
+/// This function errors out if the files couldn't be created or written to.
+pub fn create(pass: Option<&str>, lint_name: Option<&str>, category: Option<&str>) -> io::Result<()> {
+    let lint = LintData {
+        pass: pass.expect("`pass` argument is validated by clap"),
+        name: lint_name.expect("`name` argument is validated by clap"),
+        category: category.expect("`category` argument is validated by clap"),
+        project_root: clippy_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));
+    create_lint(&lint).context("Unable to create lint implementation")?;
+    create_test(&lint).context("Unable to create a test for the new lint")
+}
 
-    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),
-        ));
+fn create_lint(lint: &LintData) -> io::Result<()> {
+    let (pass_type, pass_lifetimes, pass_import, context_import) = match lint.pass {
+        "early" => ("EarlyLintPass", "", "use rustc_ast::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);
+    let lint_contents = get_lint_file_contents(
+        pass_type,
+        pass_lifetimes,
+        lint.name,
+        &camel_case_name,
+        lint.category,
+        pass_import,
+        context_import,
+    );
+
+    let lint_path = format!("clippy_lints/src/{}.rs", lint.name);
+    write_file(lint.project_root.join(&lint_path), lint_contents.as_bytes())
+}
+
+fn create_test(lint: &LintData) -> io::Result<()> {
+    fn create_project_layout<P: Into<PathBuf>>(lint_name: &str, location: P, case: &str, hint: &str) -> io::Result<()> {
+        let mut path = location.into().join(case);
+        fs::create_dir(&path)?;
+        write_file(path.join("Cargo.toml"), get_manifest_contents(lint_name, hint))?;
+
+        path.push("src");
+        fs::create_dir(&path)?;
+        write_file(path.join("main.rs"), get_test_file_contents(lint_name))?;
+
+        Ok(())
     }
 
-    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)?;
+    if lint.category == "cargo" {
+        let relative_test_dir = format!("tests/ui-cargo/{}", lint.name);
+        let test_dir = lint.project_root.join(relative_test_dir);
+        fs::create_dir(&test_dir)?;
 
-    Ok((test_file, lint_file))
+        create_project_layout(lint.name, &test_dir, "fail", "Content that triggers the lint goes here")?;
+        create_project_layout(lint.name, &test_dir, "pass", "This file should not trigger the lint")
+    } else {
+        let test_path = format!("tests/ui/{}.rs", lint.name);
+        let test_contents = get_test_file_contents(lint.name);
+        write_file(lint.project_root.join(test_path), test_contents)
+    }
+}
+
+fn write_file<P: AsRef<Path>, C: AsRef<[u8]>>(path: P, contents: C) -> io::Result<()> {
+    fn inner(path: &Path, contents: &[u8]) -> io::Result<()> {
+        OpenOptions::new()
+            .write(true)
+            .create_new(true)
+            .open(path)?
+            .write_all(contents)
+    }
+
+    inner(path.as_ref(), contents.as_ref()).context(format!("writing to file: {}", path.as_ref().display()))
 }
 
 fn to_camel_case(name: &str) -> String {
@@ -112,6 +131,20 @@ fn main() {{
     )
 }
 
+fn get_manifest_contents(lint_name: &str, hint: &str) -> String {
+    format!(
+        r#"
+# {}
+
+[package]
+name = "{}"
+version = "0.1.0"
+publish = false
+"#,
+        hint, lint_name
+    )
+}
+
 fn get_lint_file_contents(
     pass_type: &str,
     pass_lifetimes: &str,

clippy_lints/src/cargo_common_metadata.rs

@@ -23,6 +23,7 @@ declare_clippy_lint! {
     /// [package]
     /// name = "clippy"
     /// version = "0.0.212"
+    /// authors = ["Someone <someone@rust-lang.org>"]
     /// description = "A bunch of helpful lints to avoid common pitfalls in Rust"
     /// repository = "https://github.com/rust-lang/rust-clippy"
     /// readme = "README.md"

clippy_lints/src/multiple_crate_versions.rs

@@ -54,7 +54,9 @@ impl LateLintPass<'_, '_> for MultipleCrateVersions {
             let group: Vec<cargo_metadata::Package> = group.collect();
 
             if group.len() > 1 {
-                let versions = group.into_iter().map(|p| p.version).join(", ");
+                let mut versions: Vec<_> = group.into_iter().map(|p| p.version).collect();
+                versions.sort();
+                let versions = versions.iter().join(", ");
 
                 span_lint(
                     cx,

doc/adding_lints.md

@@ -42,8 +42,10 @@ case), and we don't need type information so it will have an early pass type
 `cargo 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 `cargo dev update_lints` to register the new lint. Next, we'll
-open up these files and add our lint!
+as well as run `cargo dev update_lints` to register the new lint. For cargo lints,
+two project hierarchies (fail/pass) will be created by default under `tests/ui-cargo`.
+
+Next, we'll open up these files and add our lint!
 
 ## Testing
 
@@ -105,6 +107,24 @@ our lint, we need to commit the generated `.stderr` files, too. In general, you
 should only commit files changed by `tests/ui/update-all-references.sh` for the
 specific lint you are creating/editing.
 
+### Cargo lints
+
+For cargo lints, the process of testing differs in that we are interested in
+the `Cargo.toml` manifest file. We also need a minimal crate associated
+with that manifest.
+
+If our new lint is named e.g. `foo_categories`, after running `cargo dev new_lint` 
+we will find by default two new crates, each with its manifest file:
+
+* `tests/ui-cargo/foo_categories/fail/Cargo.toml`: this file should cause the new lint to raise an error.
+* `tests/ui-cargo/foo_categories/pass/Cargo.toml`: this file should not trigger the lint.
+
+If you need more cases, you can copy one of those crates (under `foo_categories`) and rename it.
+
+The process of generating the `.stderr` file is the same, and prepending the `TESTNAME`
+variable to `cargo uitest` works too, but the script to update the references
+is in another path: `tests/ui-cargo/update-all-references.sh`.
+
 ## Rustfix tests
 
 If the lint you are working on is making use of structured suggestions, the

tests/compile-test.rs

@@ -101,54 +101,133 @@ fn run_mode(cfg: &mut compiletest::Config) {
     compiletest::run_tests(&cfg);
 }
 
-#[allow(clippy::identity_conversion)]
-fn run_ui_toml_tests(config: &compiletest::Config, mut tests: Vec<tester::TestDescAndFn>) -> Result<bool, io::Error> {
-    let mut result = true;
-    let opts = compiletest::test_opts(config);
-    for dir in fs::read_dir(&config.src_base)? {
-        let dir = dir?;
-        if !dir.file_type()?.is_dir() {
-            continue;
-        }
-        let dir_path = dir.path();
-        set_var("CARGO_MANIFEST_DIR", &dir_path);
-        for file in fs::read_dir(&dir_path)? {
-            let file = file?;
-            let file_path = file.path();
-            if file.file_type()?.is_dir() {
-                continue;
-            }
-            if file_path.extension() != Some(OsStr::new("rs")) {
-                continue;
-            }
-            let paths = compiletest::common::TestPaths {
-                file: file_path,
-                base: config.src_base.clone(),
-                relative_dir: dir_path.file_name().unwrap().into(),
-            };
-            let test_name = compiletest::make_test_name(&config, &paths);
-            let index = tests
-                .iter()
-                .position(|test| test.desc.name == test_name)
-                .expect("The test should be in there");
-            result &= tester::run_tests_console(&opts, vec![tests.swap_remove(index)])?;
-        }
-    }
-    Ok(result)
-}
-
 fn run_ui_toml(config: &mut compiletest::Config) {
+    fn run_tests(config: &compiletest::Config, mut tests: Vec<tester::TestDescAndFn>) -> Result<bool, io::Error> {
+        let mut result = true;
+        let opts = compiletest::test_opts(config);
+        for dir in fs::read_dir(&config.src_base)? {
+            let dir = dir?;
+            if !dir.file_type()?.is_dir() {
+                continue;
+            }
+            let dir_path = dir.path();
+            set_var("CARGO_MANIFEST_DIR", &dir_path);
+            for file in fs::read_dir(&dir_path)? {
+                let file = file?;
+                let file_path = file.path();
+                if file.file_type()?.is_dir() {
+                    continue;
+                }
+                if file_path.extension() != Some(OsStr::new("rs")) {
+                    continue;
+                }
+                let paths = compiletest::common::TestPaths {
+                    file: file_path,
+                    base: config.src_base.clone(),
+                    relative_dir: dir_path.file_name().unwrap().into(),
+                };
+                let test_name = compiletest::make_test_name(&config, &paths);
+                let index = tests
+                    .iter()
+                    .position(|test| test.desc.name == test_name)
+                    .expect("The test should be in there");
+                result &= tester::run_tests_console(&opts, vec![tests.swap_remove(index)])?;
+            }
+        }
+        Ok(result)
+    }
+
     config.mode = TestMode::Ui;
     config.src_base = Path::new("tests").join("ui-toml").canonicalize().unwrap();
 
     let tests = compiletest::make_tests(&config);
 
-    let res = run_ui_toml_tests(&config, tests);
+    let res = run_tests(&config, tests);
     match res {
         Ok(true) => {},
         Ok(false) => panic!("Some tests failed"),
         Err(e) => {
-            println!("I/O failure during tests: {:?}", e);
+            panic!("I/O failure during tests: {:?}", e);
+        },
+    }
+}
+
+fn run_ui_cargo(config: &mut compiletest::Config) {
+    fn run_tests(
+        config: &compiletest::Config,
+        filter: &Option<String>,
+        mut tests: Vec<tester::TestDescAndFn>,
+    ) -> Result<bool, io::Error> {
+        let mut result = true;
+        let opts = compiletest::test_opts(config);
+
+        for dir in fs::read_dir(&config.src_base)? {
+            let dir = dir?;
+            if !dir.file_type()?.is_dir() {
+                continue;
+            }
+
+            // Use the filter if provided
+            let dir_path = dir.path();
+            match &filter {
+                Some(name) if !dir_path.ends_with(name) => continue,
+                _ => {},
+            }
+
+            for case in fs::read_dir(&dir_path)? {
+                let case = case?;
+                if !case.file_type()?.is_dir() {
+                    continue;
+                }
+
+                let src_path = case.path().join("src");
+                env::set_current_dir(&src_path)?;
+
+                for file in fs::read_dir(&src_path)? {
+                    let file = file?;
+                    if file.file_type()?.is_dir() {
+                        continue;
+                    }
+
+                    // Search for the main file to avoid running a test for each file in the project
+                    let file_path = file.path();
+                    match file_path.file_name().and_then(OsStr::to_str) {
+                        Some("main.rs") => {},
+                        _ => continue,
+                    }
+
+                    let paths = compiletest::common::TestPaths {
+                        file: file_path,
+                        base: config.src_base.clone(),
+                        relative_dir: src_path.strip_prefix(&config.src_base).unwrap().into(),
+                    };
+                    let test_name = compiletest::make_test_name(&config, &paths);
+                    let index = tests
+                        .iter()
+                        .position(|test| test.desc.name == test_name)
+                        .expect("The test should be in there");
+                    result &= tester::run_tests_console(&opts, vec![tests.swap_remove(index)])?;
+                }
+            }
+        }
+        Ok(result)
+    }
+
+    config.mode = TestMode::Ui;
+    config.src_base = Path::new("tests").join("ui-cargo").canonicalize().unwrap();
+
+    let tests = compiletest::make_tests(&config);
+
+    let current_dir = env::current_dir().unwrap();
+    let filter = env::var("TESTNAME").ok();
+    let res = run_tests(&config, &filter, tests);
+    env::set_current_dir(current_dir).unwrap();
+
+    match res {
+        Ok(true) => {},
+        Ok(false) => panic!("Some tests failed"),
+        Err(e) => {
+            panic!("I/O failure during tests: {:?}", e);
         },
     }
 }
@@ -165,4 +244,5 @@ fn compile_test() {
     let mut config = default_config();
     run_mode(&mut config);
     run_ui_toml(&mut config);
+    run_ui_cargo(&mut config);
 }

tests/ui-cargo/cargo_common_metadata/fail/Cargo.toml

@@ -0,0 +1,4 @@
+[package]
+name = "cargo_common_metadata"
+version = "0.1.0"
+publish = false

tests/ui-cargo/cargo_common_metadata/fail/src/main.rs

@@ -0,0 +1,3 @@
+#![warn(clippy::cargo_common_metadata)]
+
+fn main() {}

tests/ui-cargo/cargo_common_metadata/fail/src/main.stderr

@@ -0,0 +1,18 @@
+error: package `cargo_common_metadata` is missing `package.authors` metadata
+   |
+   = note: `-D clippy::cargo-common-metadata` implied by `-D warnings`
+
+error: package `cargo_common_metadata` is missing `package.description` metadata
+
+error: package `cargo_common_metadata` is missing `either package.license or package.license_file` metadata
+
+error: package `cargo_common_metadata` is missing `package.repository` metadata
+
+error: package `cargo_common_metadata` is missing `package.readme` metadata
+
+error: package `cargo_common_metadata` is missing `package.keywords` metadata
+
+error: package `cargo_common_metadata` is missing `package.categories` metadata
+
+error: aborting due to 7 previous errors
+

tests/ui-cargo/cargo_common_metadata/pass/Cargo.toml

@@ -0,0 +1,11 @@
+[package]
+name = "cargo_common_metadata"
+version = "0.1.0"
+publish = false
+authors = ["Random person from the Internet <someone@someplace.org>"]
+description = "A test package for the cargo_common_metadata lint"
+repository = "https://github.com/someone/cargo_common_metadata"
+readme = "README.md"
+license = "MIT OR Apache-2.0"
+keywords = ["metadata", "lint", "clippy"]
+categories = ["development-tools::testing"]

tests/ui-cargo/cargo_common_metadata/pass/src/main.rs

@@ -0,0 +1,3 @@
+#![warn(clippy::cargo_common_metadata)]
+
+fn main() {}

tests/ui-cargo/multiple_crate_versions/fail/Cargo.toml

@@ -0,0 +1,8 @@
+[package]
+name = "multiple_crate_versions"
+version = "0.1.0"
+publish = false
+
+[dependencies]
+ctrlc = "=3.1.0"
+ansi_term = "=0.11.0"

tests/ui-cargo/multiple_crate_versions/fail/src/main.rs

@@ -0,0 +1,3 @@
+#![warn(clippy::multiple_crate_versions)]
+
+fn main() {}

tests/ui-cargo/multiple_crate_versions/fail/src/main.stderr

@@ -0,0 +1,6 @@
+error: multiple versions for dependency `winapi`: 0.2.8, 0.3.8
+   |
+   = note: `-D clippy::multiple-crate-versions` implied by `-D warnings`
+
+error: aborting due to previous error
+

tests/ui-cargo/multiple_crate_versions/pass/Cargo.toml

@@ -0,0 +1,8 @@
+[package]
+name = "cargo_common_metadata"
+version = "0.1.0"
+publish = false
+
+[dependencies]
+regex = "1.3.7"
+serde = "1.0.110"

tests/ui-cargo/multiple_crate_versions/pass/src/main.rs

@@ -0,0 +1,3 @@
+#![warn(clippy::multiple_crate_versions)]
+
+fn main() {}

tests/ui-cargo/update-all-references.sh

@@ -0,0 +1,18 @@
+#!/bin/bash
+#
+# A script to update the references for all tests. The idea is that
+# you do a run, which will generate files in the build directory
+# containing the (normalized) actual output of the compiler. You then
+# run this script, which will copy those files over. If you find
+# yourself manually editing a foo.stderr file, you're doing it wrong.
+#
+# See all `update-references.sh`, if you just want to update a single test.
+
+if [[ "$1" == "--help" || "$1" == "-h" ]]; then
+    echo "usage: $0"
+fi
+
+BUILD_DIR=$PWD/target/debug/test_build_base
+MY_DIR=$(dirname "$0")
+cd "$MY_DIR" || exit
+find . -name '*.rs' -exec ./update-references.sh "$BUILD_DIR" {} +

tests/ui-cargo/update-references.sh

@@ -0,0 +1,38 @@
+#!/bin/bash
+
+# A script to update the references for particular tests. The idea is
+# that you do a run, which will generate files in the build directory
+# containing the (normalized) actual output of the compiler. This
+# script will then copy that output and replace the "expected output"
+# files. You can then commit the changes.
+#
+# If you find yourself manually editing a foo.stderr file, you're
+# doing it wrong.
+
+if [[ "$1" == "--help" || "$1" == "-h" || "$1" == "" || "$2" == "" ]]; then
+    echo "usage: $0 <build-directory> <relative-path-to-rs-files>"
+    echo ""
+    echo "For example:"
+    echo "   $0 ../../../build/x86_64-apple-darwin/test/ui *.rs */*.rs"
+fi
+
+MYDIR=$(dirname "$0")
+
+BUILD_DIR="$1"
+shift
+
+while [[ "$1" != "" ]]; do
+    STDERR_NAME="${1/%.rs/.stderr}"
+    STDOUT_NAME="${1/%.rs/.stdout}"
+    shift
+    if [[ -f "$BUILD_DIR"/"$STDOUT_NAME" ]] && \
+           ! (cmp -s -- "$BUILD_DIR"/"$STDOUT_NAME" "$MYDIR"/"$STDOUT_NAME"); then
+        echo updating "$MYDIR"/"$STDOUT_NAME"
+        cp "$BUILD_DIR"/"$STDOUT_NAME" "$MYDIR"/"$STDOUT_NAME"
+    fi
+    if [[ -f "$BUILD_DIR"/"$STDERR_NAME" ]] && \
+           ! (cmp -s -- "$BUILD_DIR"/"$STDERR_NAME" "$MYDIR"/"$STDERR_NAME"); then
+        echo updating "$MYDIR"/"$STDERR_NAME"
+        cp "$BUILD_DIR"/"$STDERR_NAME" "$MYDIR"/"$STDERR_NAME"
+    fi
+done

tests/ui-cargo/wildcard_dependencies/fail/Cargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "wildcard_dependencies"
+version = "0.1.0"
+publish = false
+
+[dependencies]
+regex = "*"

tests/ui-cargo/wildcard_dependencies/fail/src/main.rs

@@ -0,0 +1,3 @@
+#![warn(clippy::wildcard_dependencies)]
+
+fn main() {}

tests/ui-cargo/wildcard_dependencies/fail/src/main.stderr

@@ -0,0 +1,6 @@
+error: wildcard dependency for `regex`
+   |
+   = note: `-D clippy::wildcard-dependencies` implied by `-D warnings`
+
+error: aborting due to previous error
+

tests/ui-cargo/wildcard_dependencies/pass/Cargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "wildcard_dependencies"
+version = "0.1.0"
+publish = false
+
+[dependencies]
+regex = "1"

tests/ui-cargo/wildcard_dependencies/pass/src/main.rs

@@ -0,0 +1,3 @@
+#![warn(clippy::wildcard_dependencies)]
+
+fn main() {}