diff --git a/Cargo.lock b/Cargo.lock index f7727eca4d0..9fb84c15aec 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -351,6 +351,7 @@ dependencies = [ "gix", "gix-transport", "glob", + "heck", "hex", "hmac", "home 0.5.12 (registry+https://github.com/rust-lang/crates.io-index)", @@ -525,7 +526,7 @@ dependencies = [ [[package]] name = "cargo-util-schemas" -version = "0.12.0" +version = "0.12.1" dependencies = [ "jiff", "schemars", @@ -2291,6 +2292,12 @@ dependencies = [ "stable_deref_trait", ] +[[package]] +name = "heck" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" + [[package]] name = "hex" version = "0.4.3" diff --git a/Cargo.toml b/Cargo.toml index 392758a17db..195190aec26 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,7 +34,7 @@ cargo-platform = { path = "crates/cargo-platform", version = "0.3.0" } cargo-test-macro = { version = "0.4.9", path = "crates/cargo-test-macro" } cargo-test-support = { version = "0.10.0", path = "crates/cargo-test-support" } cargo-util = { version = "0.2.27", path = "crates/cargo-util" } -cargo-util-schemas = { version = "0.12.0", path = "crates/cargo-util-schemas" } +cargo-util-schemas = { version = "0.12.1", path = "crates/cargo-util-schemas" } cargo_metadata = "0.23.1" clap = "4.5.53" clap_complete = { version = "4.5.64", features = ["unstable-dynamic"] } @@ -54,6 +54,7 @@ git2-curl = "0.21.0" gix = { version = "0.77.0", default-features = false, features = ["progress-tree", "parallel", "dirwalk", "status"] } glob = "0.3.3" handlebars = { version = "6.4.0", features = ["dir_source"] } +heck = "0.5.0" hex = "0.4.3" hmac = "0.12.1" home = "0.5.12" @@ -176,6 +177,7 @@ git2.workspace = true git2-curl.workspace = true gix.workspace = true glob.workspace = true +heck.workspace = true hex.workspace = true hmac.workspace = true home.workspace = true diff --git a/crates/cargo-util-schemas/Cargo.toml b/crates/cargo-util-schemas/Cargo.toml index 1149ae0e50d..10eca27fcad 100644 --- a/crates/cargo-util-schemas/Cargo.toml +++ b/crates/cargo-util-schemas/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cargo-util-schemas" -version = "0.12.0" +version = "0.12.1" rust-version = "1.92" # MSRV:1 edition.workspace = true license.workspace = true diff --git a/crates/cargo-util-schemas/src/manifest/mod.rs b/crates/cargo-util-schemas/src/manifest/mod.rs index bca5e2362c4..c08ea423d62 100644 --- a/crates/cargo-util-schemas/src/manifest/mod.rs +++ b/crates/cargo-util-schemas/src/manifest/mod.rs @@ -1796,7 +1796,7 @@ impl<'de> de::Deserialize<'de> for VecStringOrBool { } } -#[derive(Clone)] +#[derive(Clone, PartialEq, Eq)] pub struct PathValue(pub PathBuf); impl fmt::Debug for PathValue { diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 0de5aad4f2e..2a1b8e28555 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -25,6 +25,7 @@ use crate::lints::analyze_cargo_lints_table; use crate::lints::rules::blanket_hint_mostly_unused; use crate::lints::rules::check_im_a_teapot; use crate::lints::rules::implicit_minimum_version_req; +use crate::lints::rules::non_kebab_case_bin; use crate::ops; use crate::ops::lockfile::LOCKFILE_NAME; use crate::sources::{CRATES_IO_INDEX, CRATES_IO_REGISTRY, PathSource, SourceConfigMap}; @@ -1354,6 +1355,14 @@ impl<'gctx> Workspace<'gctx> { &mut run_error_count, self.gctx, )?; + non_kebab_case_bin( + self, + pkg.into(), + &path, + &cargo_lints, + &mut run_error_count, + self.gctx, + )?; if run_error_count > 0 { let plural = if run_error_count == 1 { "" } else { "s" }; diff --git a/src/cargo/lints/mod.rs b/src/cargo/lints/mod.rs index cc23938e204..a520d58173e 100644 --- a/src/cargo/lints/mod.rs +++ b/src/cargo/lints/mod.rs @@ -218,40 +218,85 @@ pub struct TomlSpan { pub value: Range, } -pub fn get_key_value<'doc>( +#[derive(Copy, Clone)] +pub enum TomlIndex<'i> { + Key(&'i str), + Offset(usize), +} + +impl<'i> TomlIndex<'i> { + fn as_key(&self) -> Option<&'i str> { + match self { + TomlIndex::Key(key) => Some(key), + TomlIndex::Offset(_) => None, + } + } +} + +pub trait AsIndex { + fn as_index<'i>(&'i self) -> TomlIndex<'i>; +} + +impl AsIndex for TomlIndex<'_> { + fn as_index<'i>(&'i self) -> TomlIndex<'i> { + match self { + TomlIndex::Key(key) => TomlIndex::Key(key), + TomlIndex::Offset(offset) => TomlIndex::Offset(*offset), + } + } +} + +impl AsIndex for &str { + fn as_index<'i>(&'i self) -> TomlIndex<'i> { + TomlIndex::Key(self) + } +} + +impl AsIndex for usize { + fn as_index<'i>(&'i self) -> TomlIndex<'i> { + TomlIndex::Offset(*self) + } +} + +pub fn get_key_value<'doc, 'i>( document: &'doc toml::Spanned>, - path: &[&str], + path: &[impl AsIndex], ) -> Option<( &'doc toml::Spanned>, &'doc toml::Spanned>, )> { - let mut table = document.get_ref(); - let mut iter = path.into_iter().peekable(); - while let Some(key) = iter.next() { - let key_s: &str = key.as_ref(); - let (key, item) = table.get_key_value(key_s)?; - if iter.peek().is_none() { - return Some((key, item)); - } - if let Some(next_table) = item.get_ref().as_table() { - table = next_table; - } - if iter.peek().is_some() { - if let Some(array) = item.get_ref().as_array() { - let next = iter.next().unwrap(); - return array.iter().find_map(|item| match item.get_ref() { - toml::de::DeValue::String(s) if s == next => Some((key, item)), - _ => None, - }); + let table = document.get_ref(); + let mut iter = path.into_iter(); + let index0 = iter.next()?.as_index(); + let key0 = index0.as_key()?; + let (mut current_key, mut current_item) = table.get_key_value(key0)?; + + while let Some(index) = iter.next() { + match index.as_index() { + TomlIndex::Key(key) => { + if let Some(table) = current_item.get_ref().as_table() { + (current_key, current_item) = table.get_key_value(key)?; + } else if let Some(array) = current_item.get_ref().as_array() { + current_item = array.iter().find(|item| match item.get_ref() { + toml::de::DeValue::String(s) => s == key, + _ => false, + })?; + } else { + return None; + } + } + TomlIndex::Offset(offset) => { + let array = current_item.get_ref().as_array()?; + current_item = array.get(offset)?; } } } - None + Some((current_key, current_item)) } -pub fn get_key_value_span( +pub fn get_key_value_span<'i>( document: &toml::Spanned>, - path: &[&str], + path: &[impl AsIndex], ) -> Option { get_key_value(document, path).map(|(k, v)| TomlSpan { key: k.span(), diff --git a/src/cargo/lints/rules/mod.rs b/src/cargo/lints/rules/mod.rs index da6975a8c5c..ebcbea8bfd6 100644 --- a/src/cargo/lints/rules/mod.rs +++ b/src/cargo/lints/rules/mod.rs @@ -1,16 +1,19 @@ mod blanket_hint_mostly_unused; mod im_a_teapot; mod implicit_minimum_version_req; +mod non_kebab_case_bin; mod unknown_lints; pub use blanket_hint_mostly_unused::blanket_hint_mostly_unused; pub use im_a_teapot::check_im_a_teapot; pub use implicit_minimum_version_req::implicit_minimum_version_req; +pub use non_kebab_case_bin::non_kebab_case_bin; pub use unknown_lints::output_unknown_lints; pub const LINTS: &[crate::lints::Lint] = &[ blanket_hint_mostly_unused::LINT, implicit_minimum_version_req::LINT, im_a_teapot::LINT, + non_kebab_case_bin::LINT, unknown_lints::LINT, ]; diff --git a/src/cargo/lints/rules/non_kebab_case_bin.rs b/src/cargo/lints/rules/non_kebab_case_bin.rs new file mode 100644 index 00000000000..cc66fa15d72 --- /dev/null +++ b/src/cargo/lints/rules/non_kebab_case_bin.rs @@ -0,0 +1,262 @@ +use std::path::Path; + +use annotate_snippets::AnnotationKind; +use annotate_snippets::Group; +use annotate_snippets::Level; +use annotate_snippets::Origin; +use annotate_snippets::Patch; +use annotate_snippets::Snippet; +use cargo_util_schemas::manifest::TomlToolLints; + +use crate::CargoResult; +use crate::GlobalContext; +use crate::core::Package; +use crate::core::Workspace; +use crate::lints::AsIndex; +use crate::lints::Lint; +use crate::lints::LintLevel; +use crate::lints::LintLevelReason; +use crate::lints::STYLE; +use crate::lints::get_key_value_span; +use crate::lints::rel_cwd_manifest_path; + +pub const LINT: Lint = Lint { + name: "non_kebab_case_bin", + desc: "binaries should have a kebab-case name", + primary_group: &STYLE, + edition_lint_opts: None, + feature_gate: None, + docs: Some( + r#" +### What it does + +Detect binary names, explicit and implicit, that are not kebab-case + +### Why it is bad + +Kebab-case binary names is a common convention among command line tools. + +### Drawbacks + +It would be disruptive to existing users to change the binary name. + +A binary may need to conform to externally controlled conventions which can include a different naming convention. + +GUI applications may wish to choose a more user focused naming convention, like "Title Case" or "Sentence case". + +### Example + +```toml +[[bin]] +name = "foo_bar" +``` + +Should be written as: + +```toml +[[bin]] +name = "foo-bar" +``` +"#, + ), +}; + +pub fn non_kebab_case_bin( + ws: &Workspace<'_>, + pkg: &Package, + manifest_path: &Path, + cargo_lints: &TomlToolLints, + error_count: &mut usize, + gctx: &GlobalContext, +) -> CargoResult<()> { + let (lint_level, reason) = LINT.level( + cargo_lints, + pkg.manifest().edition(), + pkg.manifest().unstable_features(), + ); + + if lint_level == LintLevel::Allow { + return Ok(()); + } + + let manifest_path = rel_cwd_manifest_path(manifest_path, gctx); + + lint_package( + ws, + pkg, + &manifest_path, + lint_level, + reason, + error_count, + gctx, + ) +} + +pub fn lint_package( + ws: &Workspace<'_>, + pkg: &Package, + manifest_path: &str, + lint_level: LintLevel, + reason: LintLevelReason, + error_count: &mut usize, + gctx: &GlobalContext, +) -> CargoResult<()> { + let manifest = pkg.manifest(); + + for (i, bin) in manifest.normalized_toml().bin.iter().flatten().enumerate() { + let Some(original_name) = bin.name.as_deref() else { + continue; + }; + let kebab_case = heck::ToKebabCase::to_kebab_case(original_name); + if kebab_case == original_name { + continue; + } + + let document = manifest.document(); + let contents = manifest.contents(); + let level = lint_level.to_diagnostic_level(); + let emitted_source = LINT.emitted_source(lint_level, reason); + + let mut primary_source = ws.target_dir().as_path_unlocked().to_owned(); + // Elide profile/platform as we don't have that context + primary_source.push("..."); + primary_source.push(""); + let mut primary_source = primary_source.display().to_string(); + let primary_span_start = primary_source.len(); + let primary_span_end = primary_span_start + original_name.len(); + primary_source.push_str(original_name); + primary_source.push_str(std::env::consts::EXE_SUFFIX); + let mut primary_group = + level + .primary_title(LINT.desc) + .element(Snippet::source(&primary_source).annotation( + AnnotationKind::Primary.span(primary_span_start..primary_span_end), + )); + if i == 0 { + primary_group = primary_group.element(Level::NOTE.message(emitted_source)); + } + let mut report = vec![primary_group]; + + if let Some((i, _target)) = manifest + .original_toml() + .iter() + .flat_map(|m| m.bin.iter().flatten()) + .enumerate() + .find(|(_i, t)| t.name.as_deref() == Some(original_name)) + { + let mut help = Group::with_title( + Level::HELP + .secondary_title("to change the binary name to kebab case, convert `bin.name`"), + ); + if let Some(document) = document + && let Some(contents) = contents + && let Some(span) = get_key_value_span( + document, + &["bin".as_index(), i.as_index(), "name".as_index()], + ) + { + help = help.element( + Snippet::source(contents) + .path(manifest_path) + .patch(Patch::new(span.value, format!("\"{kebab_case}\""))), + ); + } else { + help = help.element(Origin::path(manifest_path)); + } + report.push(help); + } else if is_default_main(bin.path.as_ref()) + && manifest + .original_toml() + .iter() + .flat_map(|m| m.bin.iter().flatten()) + .all(|t| t.path != bin.path) + && manifest + .original_toml() + .and_then(|t| t.package.as_ref()) + .map(|p| p.name.is_some()) + .unwrap_or(false) + { + // Showing package in case this is done before first publish to fix the problem at the + // root + let help_package_name = + "to change the binary name to kebab case, convert `package.name`"; + // Including `[[bin]]` in case it is already published. + // Preferring it over moving the file to avoid having to get into moving the + // files it `mod`s + let help_bin_table = "to change the binary name to kebab case, specify `bin.name`"; + if let Some(document) = document + && let Some(contents) = contents + && let Some(span) = get_key_value_span(document, &["package", "name"]) + { + report.push( + Level::HELP.secondary_title(help_package_name).element( + Snippet::source(contents) + .path(manifest_path) + .patch(Patch::new(span.value, format!("\"{kebab_case}\""))), + ), + ); + report.push( + Level::HELP.secondary_title(help_bin_table).element( + Snippet::source(contents) + .path(manifest_path) + .patch(Patch::new( + contents.len()..contents.len(), + format!( + r#" +[[bin]] +name = "{kebab_case}" +path = "src/main.rs""# + ), + )), + ), + ); + } else { + report.push( + Level::HELP + .secondary_title(help_package_name) + .element(Origin::path(manifest_path)), + ); + report.push( + Level::HELP + .secondary_title(help_bin_table) + .element(Origin::path(manifest_path)), + ); + } + } else { + let path = bin + .path + .as_ref() + .expect("normalized have a path") + .0 + .as_path(); + let display_path = path.as_os_str().to_string_lossy(); + let end = display_path.len() - if display_path.ends_with(".rs") { 3 } else { 0 }; + let start = path + .parent() + .map(|p| { + let p = p.as_os_str().to_string_lossy(); + // Account for trailing slash that was removed + p.len() + if p.is_empty() { 0 } else { 1 } + }) + .unwrap_or(0); + let help = Level::HELP + .secondary_title("to change the binary name to kebab case, convert the file stem") + .element(Snippet::source(display_path).patch(Patch::new(start..end, kebab_case))); + report.push(help); + } + + if lint_level.is_error() { + *error_count += 1; + } + gctx.shell().print_report(&report, lint_level.force())?; + } + + Ok(()) +} + +fn is_default_main(path: Option<&cargo_util_schemas::manifest::PathValue>) -> bool { + let Some(path) = path else { + return false; + }; + path.0 == std::path::Path::new("src/main.rs") +} diff --git a/src/doc/src/reference/lints.md b/src/doc/src/reference/lints.md index cba73cc20bd..4ba5fe0a55f 100644 --- a/src/doc/src/reference/lints.md +++ b/src/doc/src/reference/lints.md @@ -25,6 +25,7 @@ These lints are all set to the 'allow' level by default. These lints are all set to the 'warn' level by default. - [`blanket_hint_mostly_unused`](#blanket_hint_mostly_unused) +- [`non_kebab_case_bin`](#non_kebab_case_bin) - [`unknown_lints`](#unknown_lints) ## `blanket_hint_mostly_unused` @@ -108,6 +109,42 @@ serde = "1.0.219" ``` +## `non_kebab_case_bin` +Group: `style` + +Level: `warn` + +### What it does + +Detect binary names, explicit and implicit, that are not kebab-case + +### Why it is bad + +Kebab-case binary names is a common convention among command line tools. + +### Drawbacks + +It would be disruptive to existing users to change the binary name. + +A binary may need to conform to externally controlled conventions which can include a different naming convention. + +GUI applications may wish to choose a more user focused naming convention, like "Title Case" or "Sentence case". + +### Example + +```toml +[[bin]] +name = "foo_bar" +``` + +Should be written as: + +```toml +[[bin]] +name = "foo-bar" +``` + + ## `unknown_lints` Group: `suspicious` diff --git a/tests/testsuite/lints/mod.rs b/tests/testsuite/lints/mod.rs index ea2ddeb7137..22f0a047860 100644 --- a/tests/testsuite/lints/mod.rs +++ b/tests/testsuite/lints/mod.rs @@ -7,6 +7,7 @@ mod blanket_hint_mostly_unused; mod error; mod implicit_minimum_version_req; mod inherited; +mod non_kebab_case_bin; mod unknown_lints; mod warning; diff --git a/tests/testsuite/lints/non_kebab_case_bin.rs b/tests/testsuite/lints/non_kebab_case_bin.rs new file mode 100644 index 00000000000..79103a22e35 --- /dev/null +++ b/tests/testsuite/lints/non_kebab_case_bin.rs @@ -0,0 +1,173 @@ +use crate::prelude::*; +use cargo_test_support::project; +use cargo_test_support::str; + +#[cargo_test] +fn bin_name_explicit() { + let p = project() + .file( + "Cargo.toml", + r#" +[package] +name = "foo" +version = "0.0.1" +edition = "2015" +authors = [] + +[[bin]] +name = "foo_bar" +path = "src/main.rs" + +[lints.cargo] +non_kebab_case_bin = "warn" +"#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + p.cargo("check -Zcargo-lints") + .masquerade_as_nightly_cargo(&["cargo-lints"]) + .with_stderr_data(str![[r#" +[WARNING] binaries should have a kebab-case name + | +1 | [ROOT]/foo/target/.../foo_bar[EXE] + | [..]^^^^^^^ + | + = [NOTE] `cargo::non_kebab_case_bin` is set to `warn` in `[lints]` +[HELP] to change the binary name to kebab case, convert `bin.name` + --> Cargo.toml:9:8 + | +9 - name = "foo_bar" +9 + name = "foo-bar" + | +[CHECKING] foo v0.0.1 ([ROOT]/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]) + .run(); +} + +#[cargo_test] +fn bin_name_from_package() { + let p = project() + .file( + "Cargo.toml", + r#" +[package] +name = "foo_bar" +version = "0.0.1" +edition = "2015" +authors = [] + +[lints.cargo] +non_kebab_case_bin = "warn" +"#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + p.cargo("check -Zcargo-lints") + .masquerade_as_nightly_cargo(&["cargo-lints"]) + .with_stderr_data(str![[r#" +[WARNING] binaries should have a kebab-case name + | + 1 | [ROOT]/foo/target/.../foo_bar[EXE] + | [..]^^^^^^^ + | + = [NOTE] `cargo::non_kebab_case_bin` is set to `warn` in `[lints]` +[HELP] to change the binary name to kebab case, convert `package.name` + --> Cargo.toml:3:8 + | + 3 - name = "foo_bar" + 3 + name = "foo-bar" + | +[HELP] to change the binary name to kebab case, specify `bin.name` + --> Cargo.toml:9:29 + | + 9 ~ non_kebab_case_bin = "warn" +10 + [[bin]] +11 + name = "foo-bar" +12 + path = "src/main.rs" + | +[CHECKING] foo_bar v0.0.1 ([ROOT]/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]) + .run(); +} + +#[cargo_test] +fn bin_name_from_path() { + let p = project() + .file( + "Cargo.toml", + r#" +[package] +name = "foo" +version = "0.0.1" +edition = "2015" +authors = [] + +[lints.cargo] +non_kebab_case_bin = "warn" +"#, + ) + .file("src/bin/foo_bar.rs", "fn main() {}") + .build(); + + p.cargo("check -Zcargo-lints") + .masquerade_as_nightly_cargo(&["cargo-lints"]) + .with_stderr_data(str![[r#" +[WARNING] binaries should have a kebab-case name + | +1 | [ROOT]/foo/target/.../foo_bar[EXE] + | [..]^^^^^^^ + | + = [NOTE] `cargo::non_kebab_case_bin` is set to `warn` in `[lints]` +[HELP] to change the binary name to kebab case, convert the file stem + | +1 - src/bin/foo_bar.rs +1 + src/bin/foo-bar.rs + | +[CHECKING] foo v0.0.1 ([ROOT]/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]) + .run(); +} + +#[cargo_test(nightly, reason = "-Zscript is unstable")] +fn bin_name_from_script_name() { + let p = cargo_test_support::project() + .file( + "foo_bar", + r#" +--- +[lints.cargo] +non_kebab_case_bin = "warn" +--- +fn main() {}"#, + ) + .build(); + + p.cargo("check -Zcargo-lints -Zscript --manifest-path foo_bar") + .masquerade_as_nightly_cargo(&["cargo-lints", "script"]) + .with_stderr_data(str![[r#" +[WARNING] `package.edition` is unspecified, defaulting to `[..]` +[WARNING] binaries should have a kebab-case name + | +1 | [ROOT]/home/.cargo/build/[HASH]/target/.../foo_bar[EXE] + | [..]^^^^^^^ + | + = [NOTE] `cargo::non_kebab_case_bin` is set to `warn` in `[lints]` +[HELP] to change the binary name to kebab case, convert the file stem + | +1 - foo_bar +1 + foo-bar + | +[CHECKING] foo_bar v0.0.0 ([ROOT]/foo/foo_bar) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]) + .run(); +}