Auto merge of #13837 - Muscraft:only-underscores-lint-names, r=epage

fix(lints): Remove ability to specify `-` in lint name

In a recent Cargo Team meeting, it was discussed whether our lint should use `-` or `_` and if we should rewrite the wrong form to the correct one. It was decided that Cargo would use `_` for lint names and would not convert `-` to `_` automatically; instead, we would warn about an "unknown lint" and mention the similarly named lint with `_`, if found.

The decision to ise `_` was made because it is the canonical representation, as well as [RFC #344](https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints) specifies:
> Use snake case in the same way you would for function names.

This PR implements these changes.

Note: This adds an `unknown_lints` lint, that tries to mirror [the lint `rustc` has with the same name](https://doc.rust-lang.org/rustc/lints/listing/warn-by-default.html#unknown-lints).

Author: bors

src/cargo/core/workspace.rs

@@ -1161,7 +1161,6 @@ impl<'gctx> Workspace<'gctx> {
             .cloned()
             .unwrap_or_default()
             .into_iter()
-            .map(|(k, v)| (k.replace('-', "_"), v))
             .collect();
 
         for (path, maybe_pkg) in &self.packages.packages {
@@ -1214,10 +1213,6 @@ impl<'gctx> Workspace<'gctx> {
             .get("cargo")
             .cloned()
             .unwrap_or(manifest::TomlToolLints::default());
-        let normalized_lints = cargo_lints
-            .into_iter()
-            .map(|(name, lint)| (name.replace('-', "_"), lint))
-            .collect();
 
         // We should only be using workspace lints if the `[lints]` table is
         // present in the manifest, and `workspace` is set to `true`
@@ -1242,7 +1237,7 @@ impl<'gctx> Workspace<'gctx> {
         analyze_cargo_lints_table(
             pkg,
             &path,
-            &normalized_lints,
+            &cargo_lints,
             ws_cargo_lints,
             ws_contents,
             ws_document,
@@ -1252,23 +1247,23 @@ impl<'gctx> Workspace<'gctx> {
         check_im_a_teapot(
             pkg,
             &path,
-            &normalized_lints,
+            &cargo_lints,
             ws_cargo_lints,
             &mut error_count,
             self.gctx,
         )?;
         check_implicit_features(
             pkg,
             &path,
-            &normalized_lints,
+            &cargo_lints,
             ws_cargo_lints,
             &mut error_count,
             self.gctx,
         )?;
         unused_dependencies(
             pkg,
             &path,
-            &normalized_lints,
+            &cargo_lints,
             ws_cargo_lints,
             &mut error_count,
             self.gctx,

src/cargo/util/lints.rs

@@ -29,45 +29,68 @@ pub fn analyze_cargo_lints_table(
     let manifest = pkg.manifest();
     let manifest_path = rel_cwd_manifest_path(path, gctx);
     let ws_path = rel_cwd_manifest_path(ws_path, gctx);
-
-    for lint_name in pkg_lints
+    let mut unknown_lints = Vec::new();
+    for (lint_name, specified_in) in pkg_lints
         .keys()
-        .chain(ws_lints.map(|l| l.keys()).unwrap_or_default())
+        .map(|name| (name, SpecifiedIn::Package))
+        .chain(
+            ws_lints
+                .map(|l| l.keys())
+                .unwrap_or_default()
+                .map(|name| (name, SpecifiedIn::Workspace)),
+        )
     {
-        if let Some((name, default_level, edition_lint_opts, feature_gate)) =
+        let Some((name, default_level, edition_lint_opts, feature_gate)) =
             find_lint_or_group(lint_name)
-        {
-            let (_, reason, _) = level_priority(
-                name,
-                *default_level,
-                *edition_lint_opts,
-                pkg_lints,
-                ws_lints,
-                manifest.edition(),
-            );
-
-            // Only run analysis on user-specified lints
-            if !reason.is_user_specified() {
-                continue;
-            }
+        else {
+            unknown_lints.push((lint_name, specified_in));
+            continue;
+        };
 
-            // Only run this on lints that are gated by a feature
-            if let Some(feature_gate) = feature_gate {
-                verify_feature_enabled(
-                    name,
-                    feature_gate,
-                    reason,
-                    manifest,
-                    &manifest_path,
-                    ws_contents,
-                    ws_document,
-                    &ws_path,
-                    &mut error_count,
-                    gctx,
-                )?;
-            }
+        let (_, reason, _) = level_priority(
+            name,
+            *default_level,
+            *edition_lint_opts,
+            pkg_lints,
+            ws_lints,
+            manifest.edition(),
+        );
+
+        // Only run analysis on user-specified lints
+        if !reason.is_user_specified() {
+            continue;
+        }
+
+        // Only run this on lints that are gated by a feature
+        if let Some(feature_gate) = feature_gate {
+            verify_feature_enabled(
+                name,
+                feature_gate,
+                reason,
+                manifest,
+                &manifest_path,
+                ws_contents,
+                ws_document,
+                &ws_path,
+                &mut error_count,
+                gctx,
+            )?;
         }
     }
+
+    output_unknown_lints(
+        unknown_lints,
+        manifest,
+        &manifest_path,
+        pkg_lints,
+        ws_lints,
+        ws_contents,
+        ws_document,
+        &ws_path,
+        &mut error_count,
+        gctx,
+    )?;
+
     if error_count > 0 {
         Err(anyhow::anyhow!(
             "encountered {error_count} errors(s) while verifying lints",
@@ -117,31 +140,21 @@ fn verify_feature_enabled(
     gctx: &GlobalContext,
 ) -> CargoResult<()> {
     if !manifest.unstable_features().is_enabled(feature_gate) {
-        let dash_name = lint_name.replace("_", "-");
         let dash_feature_name = feature_gate.name().replace("_", "-");
-        let title = format!("use of unstable lint `{}`", dash_name);
+        let title = format!("use of unstable lint `{}`", lint_name);
         let label = format!(
             "this is behind `{}`, which is not enabled",
             dash_feature_name
         );
-        let second_title = format!("`cargo::{}` was inherited", dash_name);
+        let second_title = format!("`cargo::{}` was inherited", lint_name);
         let help = format!(
             "consider adding `cargo-features = [\"{}\"]` to the top of the manifest",
             dash_feature_name
         );
         let message = match reason {
             LintLevelReason::Package => {
-                let span = get_span(
-                    manifest.document(),
-                    &["lints", "cargo", dash_name.as_str()],
-                    false,
-                )
-                .or(get_span(
-                    manifest.document(),
-                    &["lints", "cargo", lint_name],
-                    false,
-                ))
-                .unwrap();
+                let span =
+                    get_span(manifest.document(), &["lints", "cargo", lint_name], false).unwrap();
 
                 Level::Error
                     .title(&title)
@@ -155,15 +168,10 @@ fn verify_feature_enabled(
             }
             LintLevelReason::Workspace => {
                 let lint_span = get_span(
-                    ws_document,
-                    &["workspace", "lints", "cargo", dash_name.as_str()],
-                    false,
-                )
-                .or(get_span(
                     ws_document,
                     &["workspace", "lints", "cargo", lint_name],
                     false,
-                ))
+                )
                 .unwrap();
                 let inherit_span_key =
                     get_span(manifest.document(), &["lints", "workspace"], false).unwrap();
@@ -395,6 +403,11 @@ impl LintLevelReason {
     }
 }
 
+enum SpecifiedIn {
+    Package,
+    Workspace,
+}
+
 fn level_priority(
     name: &str,
     default_level: LintLevel,
@@ -588,6 +601,120 @@ pub fn check_implicit_features(
     Ok(())
 }
 
+const UNKNOWN_LINTS: Lint = Lint {
+    name: "unknown_lints",
+    desc: "unknown lint",
+    groups: &[],
+    default_level: LintLevel::Warn,
+    edition_lint_opts: None,
+    feature_gate: None,
+};
+
+fn output_unknown_lints(
+    unknown_lints: Vec<(&String, SpecifiedIn)>,
+    manifest: &Manifest,
+    manifest_path: &str,
+    pkg_lints: &TomlToolLints,
+    ws_lints: Option<&TomlToolLints>,
+    ws_contents: &str,
+    ws_document: &ImDocument<String>,
+    ws_path: &str,
+    error_count: &mut usize,
+    gctx: &GlobalContext,
+) -> CargoResult<()> {
+    let (lint_level, reason) = UNKNOWN_LINTS.level(
+        pkg_lints,
+        ws_lints,
+        manifest.edition(),
+        manifest.unstable_features(),
+    );
+    if lint_level == LintLevel::Allow {
+        return Ok(());
+    }
+
+    let level = lint_level.to_diagnostic_level();
+    let mut emitted_source = None;
+    for (lint_name, specified_in) in unknown_lints {
+        if lint_level == LintLevel::Forbid || lint_level == LintLevel::Deny {
+            *error_count += 1;
+        }
+        let title = format!("{}: `{lint_name}`", UNKNOWN_LINTS.desc);
+        let second_title = format!("`cargo::{}` was inherited", lint_name);
+        let underscore_lint_name = lint_name.replace("-", "_");
+        let matching = if let Some(lint) = LINTS.iter().find(|l| l.name == underscore_lint_name) {
+            Some((lint.name, "lint"))
+        } else if let Some(group) = LINT_GROUPS.iter().find(|g| g.name == underscore_lint_name) {
+            Some((group.name, "group"))
+        } else {
+            None
+        };
+        let help =
+            matching.map(|(name, kind)| format!("there is a {kind} with a similar name: `{name}`"));
+
+        let mut message = match specified_in {
+            SpecifiedIn::Package => {
+                let span =
+                    get_span(manifest.document(), &["lints", "cargo", lint_name], false).unwrap();
+
+                level.title(&title).snippet(
+                    Snippet::source(manifest.contents())
+                        .origin(&manifest_path)
+                        .annotation(Level::Error.span(span))
+                        .fold(true),
+                )
+            }
+            SpecifiedIn::Workspace => {
+                let lint_span = get_span(
+                    ws_document,
+                    &["workspace", "lints", "cargo", lint_name],
+                    false,
+                )
+                .unwrap();
+                let inherit_span_key =
+                    get_span(manifest.document(), &["lints", "workspace"], false).unwrap();
+                let inherit_span_value =
+                    get_span(manifest.document(), &["lints", "workspace"], true).unwrap();
+
+                level
+                    .title(&title)
+                    .snippet(
+                        Snippet::source(ws_contents)
+                            .origin(&ws_path)
+                            .annotation(Level::Error.span(lint_span))
+                            .fold(true),
+                    )
+                    .footer(
+                        Level::Note.title(&second_title).snippet(
+                            Snippet::source(manifest.contents())
+                                .origin(&manifest_path)
+                                .annotation(
+                                    Level::Note
+                                        .span(inherit_span_key.start..inherit_span_value.end),
+                                )
+                                .fold(true),
+                        ),
+                    )
+            }
+        };
+
+        if emitted_source.is_none() {
+            emitted_source = Some(format!(
+                "`cargo::{}` is set to `{lint_level}` {reason}",
+                UNKNOWN_LINTS.name
+            ));
+            message = message.footer(Level::Note.title(emitted_source.as_ref().unwrap()));
+        }
+
+        if let Some(help) = help.as_ref() {
+            message = message.footer(Level::Help.title(help));
+        }
+
+        gctx.shell().print_message(message)?;
+    }
+
+    Ok(())
+}
+
 const UNUSED_OPTIONAL_DEPENDENCY: Lint = Lint {
     name: "unused_optional_dependency",
     desc: "unused optional dependency",

tests/testsuite/lints/implicit_features/edition_2021_warn/mod.rs

@@ -27,7 +27,7 @@ baz = { version = "0.1.0", optional = true }
 target-dep = { version = "0.1.0", optional = true }
 
 [lints.cargo]
-implicit-features = "warn"
+implicit_features = "warn"
 "#,
         )
         .file("src/lib.rs", "")

tests/testsuite/lints/implicit_features/edition_2024/mod.rs

@@ -25,7 +25,7 @@ baz = { version = "0.1.0", optional = true }
 baz = ["dep:baz"]
 
 [lints.cargo]
-unused-optional-dependency = "allow"
+unused_optional_dependency = "allow"
 "#,
         )
         .file("src/lib.rs", "")

tests/testsuite/lints/mod.rs

@@ -1,2 +1,3 @@
 mod implicit_features;
+mod unknown_lints;
 mod unused_optional_dependencies;

tests/testsuite/lints/unknown_lints/default/mod.rs

@@ -0,0 +1,33 @@
+use cargo_test_support::prelude::*;
+use cargo_test_support::str;
+use cargo_test_support::{file, project};
+
+#[cargo_test]
+fn case() {
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+[package]
+name = "foo"
+version = "0.0.1"
+edition = "2015"
+authors = []
+
+[lints.cargo]
+this-lint-does-not-exist = "warn"
+"#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    snapbox::cmd::Command::cargo_ui()
+        .masquerade_as_nightly_cargo(&["cargo-lints"])
+        .current_dir(p.root())
+        .arg("check")
+        .arg("-Zcargo-lints")
+        .assert()
+        .success()
+        .stdout_matches(str![""])
+        .stderr_matches(file!["stderr.term.svg"]);
+}