Auto merge of #13797 - Muscraft:cleanup-linting-system, r=epage

bors · bors · commit 70fb49899402 · 2024-04-24T16:31:40.000Z
Cleanup linting system

There are a number of problems with the current linting system, most notably that lints could run without `-Zcargo-lints` being set. This PR fixes that issue and a few others that are low-hanging fruit.
diff --git a/crates/cargo-util-schemas/src/manifest/mod.rs b/crates/cargo-util-schemas/src/manifest/mod.rs
@@ -1506,7 +1506,7 @@ pub struct TomlLintConfig {
     pub priority: i8,
 }
 
-#[derive(Serialize, Deserialize, Debug, Copy, Clone)]
+#[derive(Serialize, Deserialize, Debug, Copy, Clone, Eq, PartialEq)]
 #[serde(rename_all = "kebab-case")]
 pub enum TomlLintLevel {
     Forbid,
diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs
@@ -24,7 +24,7 @@ use crate::sources::{PathSource, CRATES_IO_INDEX, CRATES_IO_REGISTRY};
 use crate::util::edit_distance;
 use crate::util::errors::{CargoResult, ManifestError};
 use crate::util::interning::InternedString;
-use crate::util::lints::{check_implicit_features, unused_dependencies};
+use crate::util::lints::{check_im_a_teapot, check_implicit_features, unused_dependencies};
 use crate::util::toml::{read_manifest, InheritableFields};
 use crate::util::{
     context::CargoResolverConfig, context::CargoResolverPrecedence, context::ConfigRelativePath,
@@ -1150,7 +1150,9 @@ impl<'gctx> Workspace<'gctx> {
         for (path, maybe_pkg) in &self.packages.packages {
             let path = path.join("Cargo.toml");
             if let MaybePackage::Package(pkg) = maybe_pkg {
-                self.emit_lints(pkg, &path)?
+                if self.gctx.cli_unstable().cargo_lints {
+                    self.emit_lints(pkg, &path)?
+                }
             }
             let warnings = match maybe_pkg {
                 MaybePackage::Package(pkg) => pkg.manifest().warnings().warnings(),
@@ -1195,6 +1197,7 @@ impl<'gctx> Workspace<'gctx> {
             .map(|(name, lint)| (name.replace('-', "_"), lint))
             .collect();
 
+        check_im_a_teapot(pkg, &path, &normalized_lints, &mut error_count, self.gctx)?;
         check_implicit_features(pkg, &path, &normalized_lints, &mut error_count, self.gctx)?;
         unused_dependencies(pkg, &path, &normalized_lints, &mut error_count, self.gctx)?;
         if error_count > 0 {
diff --git a/src/cargo/util/lints.rs b/src/cargo/util/lints.rs
@@ -68,6 +68,13 @@ pub struct LintGroup {
     pub edition_lint_opts: Option<(Edition, LintLevel)>,
 }
 
+const TEST_DUMMY_UNSTABLE: LintGroup = LintGroup {
+    name: "test_dummy_unstable",
+    desc: "test_dummy_unstable is meant to only be used in tests",
+    default_level: LintLevel::Allow,
+    edition_lint_opts: None,
+};
+
 #[derive(Copy, Clone, Debug)]
 pub struct Lint {
     pub name: &'static str,
@@ -79,23 +86,37 @@ pub struct Lint {
 
 impl Lint {
     pub fn level(&self, lints: &TomlToolLints, edition: Edition) -> LintLevel {
+        let edition_level = self
+            .edition_lint_opts
+            .filter(|(e, _)| edition >= *e)
+            .map(|(_, l)| l);
+
+        if self.default_level == LintLevel::Forbid || edition_level == Some(LintLevel::Forbid) {
+            return LintLevel::Forbid;
+        }
+
         let level = self
             .groups
             .iter()
             .map(|g| g.name)
             .chain(std::iter::once(self.name))
             .filter_map(|n| lints.get(n).map(|l| (n, l)))
-            .max_by_key(|(n, l)| (l.priority(), std::cmp::Reverse(*n)));
+            .max_by_key(|(n, l)| {
+                (
+                    l.level() == TomlLintLevel::Forbid,
+                    l.priority(),
+                    std::cmp::Reverse(*n),
+                )
+            });
 
         match level {
             Some((_, toml_lint)) => toml_lint.level().into(),
             None => {
-                if let Some((lint_edition, lint_level)) = self.edition_lint_opts {
-                    if edition >= lint_edition {
-                        return lint_level;
-                    }
+                if let Some(level) = edition_level {
+                    level
+                } else {
+                    self.default_level
                 }
-                self.default_level
             }
         }
     }
@@ -123,7 +144,7 @@ impl Display for LintLevel {
 impl LintLevel {
     pub fn to_diagnostic_level(self) -> Level {
         match self {
-            LintLevel::Allow => Level::Note,
+            LintLevel::Allow => unreachable!("allow does not map to a diagnostic level"),
             LintLevel::Warn => Level::Warning,
             LintLevel::Deny => Level::Error,
             LintLevel::Forbid => Level::Error,
@@ -142,6 +163,61 @@ impl From<TomlLintLevel> for LintLevel {
     }
 }
 
+const IM_A_TEAPOT: Lint = Lint {
+    name: "im_a_teapot",
+    desc: "`im_a_teapot` is specified",
+    groups: &[TEST_DUMMY_UNSTABLE],
+    default_level: LintLevel::Allow,
+    edition_lint_opts: None,
+};
+
+pub fn check_im_a_teapot(
+    pkg: &Package,
+    path: &Path,
+    lints: &TomlToolLints,
+    error_count: &mut usize,
+    gctx: &GlobalContext,
+) -> CargoResult<()> {
+    let manifest = pkg.manifest();
+    let lint_level = IM_A_TEAPOT.level(lints, manifest.edition());
+    if lint_level == LintLevel::Allow {
+        return Ok(());
+    }
+
+    if manifest
+        .resolved_toml()
+        .package()
+        .is_some_and(|p| p.im_a_teapot.is_some())
+    {
+        if lint_level == LintLevel::Forbid || lint_level == LintLevel::Deny {
+            *error_count += 1;
+        }
+        let level = lint_level.to_diagnostic_level();
+        let manifest_path = rel_cwd_manifest_path(path, gctx);
+        let emitted_reason = format!("`cargo::{}` is set to `{lint_level}`", IM_A_TEAPOT.name);
+
+        let key_span = get_span(manifest.document(), &["package", "im-a-teapot"], false).unwrap();
+        let value_span = get_span(manifest.document(), &["package", "im-a-teapot"], true).unwrap();
+        let message = level
+            .title(IM_A_TEAPOT.desc)
+            .snippet(
+                Snippet::source(manifest.contents())
+                    .origin(&manifest_path)
+                    .annotation(level.span(key_span.start..value_span.end))
+                    .fold(true),
+            )
+            .footer(Level::Note.title(&emitted_reason));
+        let renderer = Renderer::styled().term_width(
+            gctx.shell()
+                .err_width()
+                .diagnostic_terminal_width()
+                .unwrap_or(annotate_snippets::renderer::DEFAULT_TERM_WIDTH),
+        );
+        writeln!(gctx.shell().err(), "{}", renderer.render(message))?;
+    }
+    Ok(())
+}
+
 /// By default, cargo will treat any optional dependency as a [feature]. As of
 /// cargo 1.60, these can be disabled by declaring a feature that activates the
 /// optional dependency as `dep:<name>` (see [RFC #3143]).
diff --git a/tests/testsuite/lints/unused_optional_dependencies/edition_2024/mod.rs b/tests/testsuite/lints/unused_optional_dependencies/edition_2024/mod.rs
@@ -33,9 +33,10 @@ target-dep = { version = "0.1.0", optional = true }
         .build();
 
     snapbox::cmd::Command::cargo_ui()
-        .masquerade_as_nightly_cargo(&["edition2024"])
+        .masquerade_as_nightly_cargo(&["cargo-lints", "edition2024"])
         .current_dir(p.root())
         .arg("check")
+        .arg("-Zcargo-lints")
         .assert()
         .success()
         .stdout_matches(str![""])
diff --git a/tests/testsuite/lints/unused_optional_dependencies/renamed_deps/mod.rs b/tests/testsuite/lints/unused_optional_dependencies/renamed_deps/mod.rs
@@ -32,9 +32,10 @@ target-dep = { version = "0.1.0", optional = true }
         .build();
 
     snapbox::cmd::Command::cargo_ui()
-        .masquerade_as_nightly_cargo(&["edition2024"])
+        .masquerade_as_nightly_cargo(&["cargo-lints", "edition2024"])
         .current_dir(p.root())
         .arg("check")
+        .arg("-Zcargo-lints")
         .assert()
         .success()
         .stdout_matches(str![""])
diff --git a/tests/testsuite/lints_table.rs b/tests/testsuite/lints_table.rs
@@ -756,14 +756,14 @@ fn cargo_lints_nightly_required() {
         .file(
             "Cargo.toml",
             r#"
-                [package]
-                name = "foo"
-                version = "0.0.1"
-                edition = "2015"
-                authors = []
-
-                [lints.cargo]
-                "unused-features" = "deny"
+[package]
+name = "foo"
+version = "0.0.1"
+edition = "2015"
+authors = []
+
+[lints.cargo]
+im-a-teapot = "warn"
             "#,
         )
         .file("src/lib.rs", "")
@@ -790,21 +790,24 @@ fn cargo_lints_no_z_flag() {
         .file(
             "Cargo.toml",
             r#"
-                [package]
-                name = "foo"
-                version = "0.0.1"
-                edition = "2015"
-                authors = []
+cargo-features = ["test-dummy-unstable"]
+
+[package]
+name = "foo"
+version = "0.0.1"
+edition = "2015"
+authors = []
+im-a-teapot = true
 
-                [lints.cargo]
-                "unused-features" = "deny"
+[lints.cargo]
+im-a-teapot = "warn"
             "#,
         )
         .file("src/lib.rs", "")
         .build();
 
     foo.cargo("check")
-        .masquerade_as_nightly_cargo(&["-Zcargo-lints"])
+        .masquerade_as_nightly_cargo(&["cargo-lints", "test-dummy-unstable"])
         .with_stderr(
             "\
 [WARNING] unused manifest key `lints.cargo` (may be supported in a future version)
@@ -819,27 +822,37 @@ consider passing `-Zcargo-lints` to enable this feature.
 
 #[cargo_test]
 fn cargo_lints_success() {
-    let foo = project()
+    let p = project()
         .file(
             "Cargo.toml",
             r#"
-                [package]
-                name = "foo"
-                version = "0.0.1"
-                edition = "2015"
-                authors = []
+cargo-features = ["test-dummy-unstable"]
 
-                [lints.cargo]
-                "unused-features" = "deny"
+[package]
+name = "foo"
+version = "0.0.1"
+edition = "2015"
+authors = []
+im-a-teapot = true
+
+[lints.cargo]
+im-a-teapot = "warn"
             "#,
         )
         .file("src/lib.rs", "")
         .build();
 
-    foo.cargo("check -Zcargo-lints")
-        .masquerade_as_nightly_cargo(&["-Zcargo-lints"])
+    p.cargo("check -Zcargo-lints")
+        .masquerade_as_nightly_cargo(&["cargo-lints", "test-dummy-unstable"])
         .with_stderr(
             "\
+warning: `im_a_teapot` is specified
+ --> Cargo.toml:9:1
+  |
+9 | im-a-teapot = true
+  | ------------------
+  |
+  = note: `cargo::im_a_teapot` is set to `warn`
 [CHECKING] foo v0.0.1 ([CWD])
 [FINISHED] [..]
 ",
@@ -849,43 +862,80 @@ fn cargo_lints_success() {
 
 #[cargo_test]
 fn cargo_lints_underscore_supported() {
-    Package::new("bar", "0.1.0").publish();
     let foo = project()
         .file(
             "Cargo.toml",
             r#"
-                [package]
-                name = "foo"
-                version = "0.0.1"
-                edition = "2021"
-                authors = []
+cargo-features = ["test-dummy-unstable"]
 
-                [lints.cargo]
-                "implicit_features" = "warn"
+[package]
+name = "foo"
+version = "0.0.1"
+edition = "2015"
+authors = []
+im-a-teapot = true
 
-                [dependencies]
-                bar = { version = "0.1.0", optional = true }
+[lints.cargo]
+im_a_teapot = "warn"
             "#,
         )
         .file("src/lib.rs", "")
         .build();
 
     foo.cargo("check -Zcargo-lints")
-        .masquerade_as_nightly_cargo(&["-Zcargo-lints"])
+        .masquerade_as_nightly_cargo(&["cargo-lints", "test-dummy-unstable"])
         .with_stderr(
             "\
-warning: implicit features for optional dependencies is deprecated and will be unavailable in the 2024 edition
-  --> Cargo.toml:12:17
-   |
-12 |                 bar = { version = \"0.1.0\", optional = true }
-   |                 ---
-   |
-   = note: `cargo::implicit_features` is set to `warn`
-[UPDATING] `dummy-registry` index
-[LOCKING] [..]
+warning: `im_a_teapot` is specified
+ --> Cargo.toml:9:1
+  |
+9 | im-a-teapot = true
+  | ------------------
+  |
+  = note: `cargo::im_a_teapot` is set to `warn`
 [CHECKING] foo v0.0.1 ([CWD])
 [FINISHED] [..]
 ",
         )
         .run();
 }
+
+#[cargo_test]
+fn forbid_not_overridden() {
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+cargo-features = ["test-dummy-unstable"]
+
+[package]
+name = "foo"
+version = "0.0.1"
+edition = "2015"
+authors = []
+im-a-teapot = true
+
+[lints.cargo]
+im-a-teapot = { level = "warn", priority = 10 }
+test-dummy-unstable = { level = "forbid", priority = -1 }
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    p.cargo("check -Zcargo-lints")
+        .masquerade_as_nightly_cargo(&["cargo-lints", "test-dummy-unstable"])
+        .with_status(101)
+        .with_stderr(
+            "\
+error: `im_a_teapot` is specified
+ --> Cargo.toml:9:1
+  |
+9 | im-a-teapot = true
+  | ^^^^^^^^^^^^^^^^^^
+  |
+  = note: `cargo::im_a_teapot` is set to `forbid`
+",
+        )
+        .run();
+}

Original file line number	Diff line number	Diff line change
`@@ -1506,7 +1506,7 @@ pub struct TomlLintConfig {`
`1506`	`1506`	`pub priority: i8,`
`1507`	`1507`	`}`
`1508`	`1508`
`1509`		`-#[derive(Serialize, Deserialize, Debug, Copy, Clone)]`
	`1509`	`+#[derive(Serialize, Deserialize, Debug, Copy, Clone, Eq, PartialEq)]`
`1510`	`1510`	`#[serde(rename_all = "kebab-case")]`
`1511`	`1511`	`pub enum TomlLintLevel {`
`1512`	`1512`	`Forbid,`