diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 805c38983e2..24b1832ce50 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -30,6 +30,7 @@ use crate::lints::rules::non_kebab_case_features; use crate::lints::rules::non_kebab_case_packages; use crate::lints::rules::non_snake_case_features; use crate::lints::rules::non_snake_case_packages; +use crate::lints::rules::redundant_homepage; use crate::lints::rules::redundant_readme; use crate::ops; use crate::ops::lockfile::LOCKFILE_NAME; @@ -1403,6 +1404,13 @@ impl<'gctx> Workspace<'gctx> { &mut run_error_count, self.gctx, )?; + redundant_homepage( + 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/rules/mod.rs b/src/cargo/lints/rules/mod.rs index 29527686b3e..8610abb251c 100644 --- a/src/cargo/lints/rules/mod.rs +++ b/src/cargo/lints/rules/mod.rs @@ -6,6 +6,7 @@ mod non_kebab_case_features; mod non_kebab_case_packages; mod non_snake_case_features; mod non_snake_case_packages; +mod redundant_homepage; mod redundant_readme; mod unknown_lints; @@ -17,6 +18,7 @@ pub use non_kebab_case_features::non_kebab_case_features; pub use non_kebab_case_packages::non_kebab_case_packages; pub use non_snake_case_features::non_snake_case_features; pub use non_snake_case_packages::non_snake_case_packages; +pub use redundant_homepage::redundant_homepage; pub use redundant_readme::redundant_readme; pub use unknown_lints::output_unknown_lints; @@ -29,6 +31,7 @@ pub const LINTS: &[crate::lints::Lint] = &[ non_kebab_case_packages::LINT, non_snake_case_features::LINT, non_snake_case_packages::LINT, + redundant_homepage::LINT, redundant_readme::LINT, unknown_lints::LINT, ]; diff --git a/src/cargo/lints/rules/redundant_homepage.rs b/src/cargo/lints/rules/redundant_homepage.rs new file mode 100644 index 00000000000..d2cd3763bc3 --- /dev/null +++ b/src/cargo/lints/rules/redundant_homepage.rs @@ -0,0 +1,156 @@ +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::InheritableField; +use cargo_util_schemas::manifest::TomlToolLints; + +use crate::CargoResult; +use crate::GlobalContext; +use crate::core::Package; +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: "redundant_homepage", + desc: "`package.homepage` is redundant with another manifest field", + primary_group: &STYLE, + edition_lint_opts: None, + feature_gate: None, + docs: Some( + r#" +### What it does + +Checks if the value of `package.homepage` is already covered by another field. + +See also [`package.homepage` reference documentation](manifest.md#the-homepage-field). + +### Why it is bad + +When package browsers render each link, a redundant link adds visual noise. + +### Drawbacks + +### Example + +```toml +[package] +name = "foo" +homepage = "https://github.com/rust-lang/cargo/" +repository = "https://github.com/rust-lang/cargo/" +``` + +Should be written as: + +```toml +[package] +name = "foo" +repository = "https://github.com/rust-lang/cargo/" +``` +"#, + ), +}; + +pub fn redundant_homepage( + 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(pkg, &manifest_path, lint_level, reason, error_count, gctx) +} + +pub fn lint_package( + pkg: &Package, + manifest_path: &str, + lint_level: LintLevel, + reason: LintLevelReason, + error_count: &mut usize, + gctx: &GlobalContext, +) -> CargoResult<()> { + let manifest = pkg.manifest(); + + let Some(normalized_pkg) = &manifest.normalized_toml().package else { + return Ok(()); + }; + let Some(InheritableField::Value(homepage)) = &normalized_pkg.homepage else { + return Ok(()); + }; + + let other_field = if let Some(InheritableField::Value(repository)) = &normalized_pkg.repository + && repository == homepage + { + "repository" + } else if let Some(InheritableField::Value(documentation)) = &normalized_pkg.documentation + && documentation == homepage + { + "documentation" + } else { + return Ok(()); + }; + + 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 = Group::with_title(level.primary_title(LINT.desc)); + if let Some(document) = document + && let Some(contents) = contents + && let Some(span) = get_key_value_span(document, &["package", "homepage"]) + { + let mut snippet = Snippet::source(contents) + .path(manifest_path) + .annotation(AnnotationKind::Primary.span(span.value)); + if let Some(span) = get_key_value_span(document, &["package", other_field]) { + snippet = snippet.annotation(AnnotationKind::Context.span(span.value)); + } + primary = primary.element(snippet); + } else { + primary = primary.element(Origin::path(manifest_path)); + } + primary = primary.element(Level::NOTE.message(emitted_source)); + let mut report = vec![primary]; + if let Some(document) = document + && let Some(contents) = contents + && let Some(span) = get_key_value_span(document, &["package", "homepage"]) + { + let mut help = + Group::with_title(Level::HELP.secondary_title("consider removing `package.homepage`")); + let span = span.key.start..span.value.end; + help = help.element( + Snippet::source(contents) + .path(manifest_path) + .patch(Patch::new(span, "")), + ); + report.push(help); + } + + if lint_level.is_error() { + *error_count += 1; + } + gctx.shell().print_report(&report, lint_level.force())?; + + Ok(()) +} diff --git a/src/doc/src/reference/lints.md b/src/doc/src/reference/lints.md index 95668f05f44..f7fed9c7b4f 100644 --- a/src/doc/src/reference/lints.md +++ b/src/doc/src/reference/lints.md @@ -30,6 +30,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_bins`](#non_kebab_case_bins) +- [`redundant_homepage`](#redundant_homepage) - [`redundant_readme`](#redundant_readme) - [`unknown_lints`](#unknown_lints) @@ -278,6 +279,41 @@ name = "foo-bar" ``` +## `redundant_homepage` +Group: `style` + +Level: `warn` + +### What it does + +Checks if the value of `package.homepage` is already covered by another field. + +See also [`package.homepage` reference documentation](manifest.md#the-homepage-field). + +### Why it is bad + +When package browsers render each link, a redundant link adds visual noise. + +### Drawbacks + +### Example + +```toml +[package] +name = "foo" +homepage = "https://github.com/rust-lang/cargo/" +repository = "https://github.com/rust-lang/cargo/" +``` + +Should be written as: + +```toml +[package] +name = "foo" +repository = "https://github.com/rust-lang/cargo/" +``` + + ## `redundant_readme` Group: `style` diff --git a/tests/testsuite/lints/mod.rs b/tests/testsuite/lints/mod.rs index b4474998b9c..e583691f88e 100644 --- a/tests/testsuite/lints/mod.rs +++ b/tests/testsuite/lints/mod.rs @@ -12,6 +12,7 @@ mod non_kebab_case_features; mod non_kebab_case_packages; mod non_snake_case_features; mod non_snake_case_packages; +mod redundant_homepage; mod redundant_readme; mod unknown_lints; mod warning; diff --git a/tests/testsuite/lints/redundant_homepage.rs b/tests/testsuite/lints/redundant_homepage.rs new file mode 100644 index 00000000000..4c116377e90 --- /dev/null +++ b/tests/testsuite/lints/redundant_homepage.rs @@ -0,0 +1,91 @@ +use crate::prelude::*; +use cargo_test_support::project; +use cargo_test_support::str; + +#[cargo_test] +fn with_repo() { + let p = project() + .file( + "Cargo.toml", + r#" +[package] +name = "cargo" +version = "0.0.1" +edition = "2015" +repository = "https://github.com/rust-lang/cargo/" +homepage = "https://github.com/rust-lang/cargo/" + +[lints.cargo] +redundant_homepage = "warn" +"#, + ) + .file("src/main.rs", "fn main() {}") + .file("README.md", "") + .build(); + + p.cargo("check -Zcargo-lints") + .masquerade_as_nightly_cargo(&["cargo-lints"]) + .with_stderr_data(str![[r#" +[WARNING] `package.homepage` is redundant with another manifest field + --> Cargo.toml:7:12 + | +6 | repository = "https://github.com/rust-lang/cargo/" + | ------------------------------------- +7 | homepage = "https://github.com/rust-lang/cargo/" + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = [NOTE] `cargo::redundant_homepage` is set to `warn` in `[lints]` +[HELP] consider removing `package.homepage` + | +7 - homepage = "https://github.com/rust-lang/cargo/" + | +[CHECKING] cargo v0.0.1 ([ROOT]/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]) + .run(); +} + +#[cargo_test] +fn with_docs() { + let p = project() + .file( + "Cargo.toml", + r#" +[package] +name = "cargo" +version = "0.0.1" +edition = "2015" +documentation = "https://docs.rs/cargo/latest/cargo/" +homepage = "https://docs.rs/cargo/latest/cargo/" + +[lints.cargo] +redundant_homepage = "warn" +"#, + ) + .file("src/main.rs", "fn main() {}") + .file("README.md", "") + .build(); + + p.cargo("check -Zcargo-lints") + .masquerade_as_nightly_cargo(&["cargo-lints"]) + .with_stderr_data(str![[r#" +[WARNING] `package.homepage` is redundant with another manifest field + --> Cargo.toml:7:12 + | +6 | documentation = "https://docs.rs/cargo/latest/cargo/" + | ------------------------------------- +7 | homepage = "https://docs.rs/cargo/latest/cargo/" + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = [NOTE] `cargo::redundant_homepage` is set to `warn` in `[lints]` +[HELP] consider removing `package.homepage` + | +7 - homepage = "https://docs.rs/cargo/latest/cargo/" + | +[CHECKING] cargo v0.0.1 ([ROOT]/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]) + .run(); +}