From 998524f577228cc63576c253733cd6aa791a0b3d Mon Sep 17 00:00:00 2001 From: Scott Schafer Date: Thu, 21 Mar 2024 20:43:35 -0600 Subject: [PATCH] feat: Add a basic linting system --- Cargo.lock | 4 +- Cargo.toml | 2 +- src/cargo/core/workspace.rs | 31 ++++- src/cargo/util/lints.rs | 241 ++++++++++++++++++++++++++++++++++++ src/cargo/util/mod.rs | 1 + src/cargo/util/toml/mod.rs | 3 +- tests/testsuite/features.rs | 36 ++++++ tests/testsuite/lints.rs | 162 +++++++++++++++++++++++- 8 files changed, 474 insertions(+), 6 deletions(-) create mode 100644 src/cargo/util/lints.rs diff --git a/Cargo.lock b/Cargo.lock index e9da353a21e7..1d9d53e77ebf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3428,9 +3428,9 @@ dependencies = [ [[package]] name = "toml_edit" -version = "0.22.7" +version = "0.22.9" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "18769cd1cec395d70860ceb4d932812a0b4d06b1a4bb336745a4d21b9496e992" +checksum = "8e40bb779c5187258fd7aad0eb68cb8706a0a81fa712fbea808ab43c4b8374c4" dependencies = [ "indexmap", "serde", diff --git a/Cargo.toml b/Cargo.toml index 26e4e81256d6..c40923486315 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -97,7 +97,7 @@ tempfile = "3.10.1" thiserror = "1.0.57" time = { version = "0.3", features = ["parsing", "formatting", "serde"] } toml = "0.8.10" -toml_edit = { version = "0.22.7", features = ["serde"] } +toml_edit = { version = "0.22.9", features = ["serde"] } tracing = "0.1.40" # be compatible with rustc_log: https://github.com/rust-lang/rust/blob/e51e98dde6a/compiler/rustc_log/Cargo.toml#L9 tracing-chrome = "0.7.1" tracing-subscriber = { version = "0.3.18", features = ["env-filter"] } diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index e689933492ab..0b0e6d749615 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -24,10 +24,12 @@ 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; use crate::util::toml::{read_manifest, InheritableFields}; use crate::util::{context::ConfigRelativePath, Filesystem, GlobalContext, IntoUrl}; use cargo_util::paths; use cargo_util::paths::normalize_path; +use cargo_util_schemas::manifest; use cargo_util_schemas::manifest::RustVersion; use cargo_util_schemas::manifest::{TomlDependency, TomlProfiles}; use pathdiff::diff_paths; @@ -1095,11 +1097,14 @@ impl<'gctx> Workspace<'gctx> { pub fn emit_warnings(&self) -> CargoResult<()> { 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)? + } let warnings = match maybe_pkg { MaybePackage::Package(pkg) => pkg.manifest().warnings().warnings(), MaybePackage::Virtual(vm) => vm.warnings().warnings(), }; - let path = path.join("Cargo.toml"); for warning in warnings { if warning.is_critical { let err = anyhow::format_err!("{}", warning.message); @@ -1121,6 +1126,30 @@ impl<'gctx> Workspace<'gctx> { Ok(()) } + pub fn emit_lints(&self, pkg: &Package, path: &Path) -> CargoResult<()> { + let mut error_count = 0; + let lints = pkg + .manifest() + .resolved_toml() + .lints + .clone() + .map(|lints| lints.lints) + .unwrap_or(manifest::TomlLints::default()) + .get("cargo") + .cloned() + .unwrap_or(manifest::TomlToolLints::default()); + + check_implicit_features(pkg, &path, &lints, &mut error_count, self.gctx)?; + if error_count > 0 { + Err(crate::util::errors::AlreadyPrintedError::new(anyhow!( + "encountered {error_count} errors(s) while running lints" + )) + .into()) + } else { + Ok(()) + } + } + pub fn set_target_dir(&mut self, target_dir: Filesystem) { self.target_dir = Some(target_dir); } diff --git a/src/cargo/util/lints.rs b/src/cargo/util/lints.rs new file mode 100644 index 000000000000..eac5c61cffd3 --- /dev/null +++ b/src/cargo/util/lints.rs @@ -0,0 +1,241 @@ +use crate::core::FeatureValue::Dep; +use crate::core::{Edition, FeatureMap, FeatureValue, Package}; +use crate::util::interning::InternedString; +use crate::{CargoResult, GlobalContext}; +use annotate_snippets::{Level, Renderer, Snippet}; +use cargo_util_schemas::manifest::{TomlLintLevel, TomlToolLints}; +use pathdiff::diff_paths; +use std::collections::{BTreeMap, HashSet}; +use std::ops::Range; +use std::path::Path; +use toml_edit::ImDocument; + +pub fn get_span( + document: &ImDocument, + path: &[&str], + get_value: bool, +) -> Option> { + let mut table = document.as_item().as_table_like().unwrap(); + let mut iter = path.into_iter().peekable(); + while let Some(key) = iter.next() { + let (key, item) = table.get_key_value(key).unwrap(); + if iter.peek().is_none() { + return if get_value { + item.span() + } else { + let leaf_decor = key.dotted_decor(); + let leaf_prefix_span = leaf_decor.prefix().and_then(|p| p.span()); + let leaf_suffix_span = leaf_decor.suffix().and_then(|s| s.span()); + if let (Some(leaf_prefix_span), Some(leaf_suffix_span)) = + (leaf_prefix_span, leaf_suffix_span) + { + Some(leaf_prefix_span.start..leaf_suffix_span.end) + } else { + key.span() + } + }; + } + if item.is_table_like() { + table = item.as_table_like().unwrap(); + } + if item.is_array() && iter.peek().is_some() { + let array = item.as_array().unwrap(); + let next = iter.next().unwrap(); + return array.iter().find_map(|item| { + if next == &item.to_string() { + item.span() + } else { + None + } + }); + } + } + None +} + +fn manifest_path(path: &Path, gctx: &GlobalContext) -> String { + diff_paths(path, gctx.cwd()) + .unwrap_or_else(|| path.to_path_buf()) + .display() + .to_string() +} + +#[derive(Copy, Clone, Debug)] +pub struct LintGroup { + pub name: &'static str, + pub default_level: LintLevel, + pub desc: &'static str, + pub edition_lint_opts: Option<(Edition, LintLevel)>, +} + +const RUST_2024_COMPATIBILITY: LintGroup = LintGroup { + name: "rust-2024-compatibility", + default_level: LintLevel::Allow, + desc: "warn about compatibility with Rust 2024", + edition_lint_opts: Some((Edition::Edition2024, LintLevel::Deny)), +}; + +#[derive(Copy, Clone, Debug)] +pub struct Lint { + pub name: &'static str, + pub desc: &'static str, + pub groups: &'static [LintGroup], + pub default_level: LintLevel, + pub edition_lint_opts: Option<(Edition, LintLevel)>, +} + +impl Lint { + pub fn level(&self, lints: &TomlToolLints, edition: Edition) -> LintLevel { + 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))); + + 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 { + lint_level + } else { + IMPLICIT_FEATURES.default_level + } + } else { + IMPLICIT_FEATURES.default_level + } + } + } + } +} + +#[derive(Copy, Clone, Debug, PartialEq)] +pub enum LintLevel { + Allow, + Warn, + Deny, + Forbid, +} + +impl LintLevel { + pub fn to_diagnostic_level(self) -> Level { + match self { + LintLevel::Allow => Level::Note, + LintLevel::Warn => Level::Warning, + LintLevel::Deny => Level::Error, + LintLevel::Forbid => Level::Error, + } + } +} + +impl Into for TomlLintLevel { + fn into(self) -> LintLevel { + match self { + TomlLintLevel::Allow => LintLevel::Allow, + TomlLintLevel::Warn => LintLevel::Warn, + TomlLintLevel::Deny => LintLevel::Deny, + TomlLintLevel::Forbid => LintLevel::Forbid, + } + } +} + +const IMPLICIT_FEATURES: Lint = Lint { + name: "implicit-features", + desc: "warn about the use of unstable features", + groups: &[RUST_2024_COMPATIBILITY], + default_level: LintLevel::Allow, + edition_lint_opts: Some((Edition::Edition2024, LintLevel::Deny)), +}; + +pub fn check_implicit_features( + pkg: &Package, + path: &Path, + lints: &TomlToolLints, + error_count: &mut usize, + gctx: &GlobalContext, +) -> CargoResult<()> { + let lint_level = IMPLICIT_FEATURES.level(lints, pkg.manifest().edition()); + if lint_level == LintLevel::Allow { + return Ok(()); + } + + let manifest = pkg.manifest(); + let features: BTreeMap> = manifest + .resolved_toml() + .features() + .map_or(BTreeMap::new(), |f| { + f.iter() + .map(|(k, v)| { + ( + InternedString::new(k), + v.iter().map(InternedString::from).collect(), + ) + }) + .collect() + }); + let map: FeatureMap = features + .iter() + .map(|(feature, list)| { + let fvs: Vec<_> = list + .iter() + .map(|feat_value| FeatureValue::new(*feat_value)) + .collect(); + (*feature, fvs) + }) + .collect(); + + // Add implicit features for optional dependencies if they weren't + // explicitly listed anywhere. + let explicitly_listed: HashSet<_> = map + .values() + .flatten() + .filter_map(|fv| match fv { + Dep { dep_name } => Some(*dep_name), + _ => None, + }) + .collect(); + for dep in manifest.dependencies() { + let dep_name_in_toml = dep.name_in_toml(); + if !dep.is_optional() + || features.contains_key(&dep_name_in_toml) + || explicitly_listed.contains(&dep_name_in_toml) + { + continue; + } + + if features.contains_key(&dep_name_in_toml) || explicitly_listed.contains(&dep_name_in_toml) + { + continue; + } + if lint_level == LintLevel::Forbid || lint_level == LintLevel::Deny { + *error_count += 1; + } + let level = lint_level.to_diagnostic_level(); + let manifest_path = manifest_path(path, gctx); + let message = level.title("unused optional dependency").snippet( + Snippet::source(manifest.contents()) + .origin(&manifest_path) + .annotation( + level.span( + get_span( + manifest.document(), + &["dependencies", &dep_name_in_toml], + false, + ) + .unwrap(), + ), + ) + .fold(true), + ); + 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(()) +} diff --git a/src/cargo/util/mod.rs b/src/cargo/util/mod.rs index cacc88fcb281..7b32f108fa7b 100644 --- a/src/cargo/util/mod.rs +++ b/src/cargo/util/mod.rs @@ -51,6 +51,7 @@ pub mod into_url; mod into_url_with_base; mod io; pub mod job; +pub mod lints; mod lockserver; pub mod machine_message; pub mod network; diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 3898695fbe7e..f2c211944b7f 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1454,7 +1454,7 @@ fn verify_lints(lints: Option<&manifest::TomlLints>) -> CargoResult<()> { }; for (tool, lints) in lints { - let supported = ["rust", "clippy", "rustdoc"]; + let supported = ["rust", "clippy", "rustdoc", "cargo"]; if !supported.contains(&tool.as_str()) { let supported = supported.join(", "); anyhow::bail!("unsupported `{tool}` in `[lints]`, must be one of {supported}") @@ -1482,6 +1482,7 @@ fn verify_lints(lints: Option<&manifest::TomlLints>) -> CargoResult<()> { fn lints_to_rustflags(lints: &manifest::TomlLints) -> Vec { let mut rustflags = lints .iter() + .filter(|(tool, _)| *tool != "cargo") .flat_map(|(tool, lints)| { lints.iter().map(move |(name, config)| { let flag = match config.level() { diff --git a/tests/testsuite/features.rs b/tests/testsuite/features.rs index 7ffc9f88b7a2..8cad879017e0 100644 --- a/tests/testsuite/features.rs +++ b/tests/testsuite/features.rs @@ -2246,3 +2246,39 @@ fn default_features_conflicting_warning() { ) .run(); } + +#[cargo_test(nightly, reason = "edition2024 is not stable")] +fn implicit_features_2024() { + Package::new("bar", "0.1.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["edition2024"] + [package] + name = "foo" + version = "0.1.0" + edition = "2024" + + [dependencies] + bar = { version = "0.1.0", optional = true } + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("check") + .masquerade_as_nightly_cargo(&["edition2024"]) + .with_status(101) + .with_stderr( + "\ +error: unused optional dependency + --> Cargo.toml:9:17 + | +9 | bar = { version = \"0.1.0\", optional = true } + | ^^^ + | +", + ) + .run(); +} diff --git a/tests/testsuite/lints.rs b/tests/testsuite/lints.rs index 0666848304b9..6c88bf42edf3 100644 --- a/tests/testsuite/lints.rs +++ b/tests/testsuite/lints.rs @@ -111,7 +111,7 @@ fn fail_on_invalid_tool() { [..] Caused by: - unsupported `super-awesome-linter` in `[lints]`, must be one of rust, clippy, rustdoc + unsupported `super-awesome-linter` in `[lints]`, must be one of rust, clippy, rustdoc, cargo ", ) .run(); @@ -749,3 +749,163 @@ pub const Ĕ: i32 = 2; ) .run(); } + +#[cargo_test(nightly, reason = "edition2024 is not stable")] +fn warn_implicit_features() { + Package::new("bar", "0.1.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["edition2024"] + [package] + name = "foo" + version = "0.1.0" + edition = "2024" + + [dependencies] + bar = { version = "0.1.0", optional = true } + + [lints.cargo] + implicit-features = "warn" + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("check") + .masquerade_as_nightly_cargo(&["edition2024"]) + .with_status(0) + .with_stderr( + "\ +warning: unused optional dependency + --> Cargo.toml:9:17 + | +9 | bar = { version = \"0.1.0\", optional = true } + | --- + | +[UPDATING] [..] index +[LOCKING] [..] packages +[CHECKING] foo [..] +[FINISHED] [..] +", + ) + .run(); +} + +#[cargo_test(nightly, reason = "edition2024 is not stable")] +fn allow_implicit_features() { + Package::new("bar", "0.1.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["edition2024"] + [package] + name = "foo" + version = "0.1.0" + edition = "2024" + + [dependencies] + bar = { version = "0.1.0", optional = true } + + [lints.cargo] + implicit-features = "allow" + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("check") + .masquerade_as_nightly_cargo(&["edition2024"]) + .with_status(0) + .with_stderr( + "\ +[UPDATING] [..] index +[LOCKING] [..] packages +[CHECKING] foo [..] +[FINISHED] [..] +", + ) + .run(); +} + +#[cargo_test(nightly, reason = "edition2024 is not stable")] +fn rust_2024_compatibility_group_warn() { + Package::new("bar", "0.1.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["edition2024"] + [package] + name = "foo" + version = "0.1.0" + edition = "2024" + + [dependencies] + bar = { version = "0.1.0", optional = true } + + [lints.cargo] + rust-2024-compatibility = "warn" + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("check") + .masquerade_as_nightly_cargo(&["edition2024"]) + .with_status(0) + .with_stderr( + "\ +warning: unused optional dependency + --> Cargo.toml:9:17 + | +9 | bar = { version = \"0.1.0\", optional = true } + | --- + | +[UPDATING] [..] index +[LOCKING] [..] packages +[CHECKING] foo [..] +[FINISHED] [..] +", + ) + .run(); +} + +#[cargo_test(nightly, reason = "edition2024 is not stable")] +fn rust_2024_compatibility_group_allow() { + Package::new("bar", "0.1.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["edition2024"] + [package] + name = "foo" + version = "0.1.0" + edition = "2024" + + [dependencies] + bar = { version = "0.1.0", optional = true } + + [lints.cargo] + rust-2024-compatibility = "allow" + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("check") + .masquerade_as_nightly_cargo(&["edition2024"]) + .with_status(0) + .with_stderr( + "\ +[UPDATING] [..] index +[LOCKING] [..] packages +[CHECKING] foo [..] +[FINISHED] [..] +", + ) + .run(); +}