From f1f1ec3d3a5b6f8cde4e7e6140dc6af07d2dc5fc Mon Sep 17 00:00:00 2001 From: Urgau Date: Fri, 27 Sep 2024 15:41:29 +0200 Subject: [PATCH] Lint against some unexpected target cfgs with the help of the Cargo lints infra --- src/cargo/core/workspace.rs | 3 +- src/cargo/util/lints.rs | 131 +++++++++++++++++++++++++++++- src/doc/src/reference/unstable.md | 2 +- tests/testsuite/cfg.rs | 101 +++++++++++++++++++---- 4 files changed, 220 insertions(+), 17 deletions(-) diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index b2f78df61c1b..8e91bac4a20b 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -24,7 +24,7 @@ use crate::sources::{PathSource, SourceConfigMap, CRATES_IO_INDEX, CRATES_IO_REG use crate::util::edit_distance; use crate::util::errors::{CargoResult, ManifestError}; use crate::util::interning::InternedString; -use crate::util::lints::{analyze_cargo_lints_table, check_im_a_teapot}; +use crate::util::lints::{analyze_cargo_lints_table, check_im_a_teapot, unexpected_target_cfgs}; use crate::util::toml::{read_manifest, InheritableFields}; use crate::util::{ context::CargoResolverConfig, context::ConfigRelativePath, context::IncompatibleRustVersions, @@ -1219,6 +1219,7 @@ impl<'gctx> Workspace<'gctx> { self.gctx, )?; check_im_a_teapot(pkg, &path, &cargo_lints, &mut error_count, self.gctx)?; + unexpected_target_cfgs(self, pkg, &path, &mut error_count, self.gctx)?; if error_count > 0 { Err(crate::util::errors::AlreadyPrintedError::new(anyhow!( "encountered {error_count} errors(s) while running lints" diff --git a/src/cargo/util/lints.rs b/src/cargo/util/lints.rs index 99531bfbd7ba..fe745930ec42 100644 --- a/src/cargo/util/lints.rs +++ b/src/cargo/util/lints.rs @@ -1,8 +1,11 @@ -use crate::core::{Edition, Feature, Features, Manifest, Package}; +use crate::core::compiler::{CompileKind, TargetInfo}; +use crate::core::{Edition, Feature, Features, Manifest, Package, Workspace}; use crate::{CargoResult, GlobalContext}; use annotate_snippets::{Level, Snippet}; +use cargo_platform::{Cfg, ExpectedValues, Platform}; use cargo_util_schemas::manifest::{TomlLintLevel, TomlToolLints}; use pathdiff::diff_paths; +use std::borrow::Cow; use std::fmt::Display; use std::ops::Range; use std::path::Path; @@ -605,6 +608,132 @@ fn output_unknown_lints( Ok(()) } +pub fn unexpected_target_cfgs( + ws: &Workspace<'_>, + pkg: &Package, + path: &Path, + error_count: &mut usize, + gctx: &GlobalContext, +) -> CargoResult<()> { + let manifest = pkg.manifest(); + + // FIXME: We should get the lint level from `[lints.rust.unexpected_cfgs]` + let lint_level = LintLevel::Warn; + + let rustc = gctx.load_global_rustc(Some(ws))?; + // FIXME: While it doesn't doesn't really matter for `--print=check-cfg`, we should + // still pass the actual requested targets instead of an empty array so that the + // target info can always be de-duplicated. + // FIXME: Move the target info creation before any linting is done and re-use it for + // all the packages. + let compile_kinds = CompileKind::from_requested_targets(gctx, &[])?; + let target_info = TargetInfo::new(gctx, &compile_kinds, &rustc, CompileKind::Host)?; + + let Some(global_check_cfg) = target_info.check_cfg() else { + return Ok(()); + }; + + if !global_check_cfg.exhaustive { + return Ok(()); + } + + // FIXME: If the `[lints.rust.unexpected_cfgs.check-cfg]` config is set we should + // re-fetch the check-cfg informations with those extra args + + for dep in pkg.summary().dependencies() { + let Some(platform) = dep.platform() else { + continue; + }; + let Platform::Cfg(cfg_expr) = platform else { + continue; + }; + + cfg_expr.walk(|cfg| -> CargoResult<()> { + let (name, value) = match cfg { + Cfg::Name(name) => (name, None), + Cfg::KeyPair(name, value) => (name, Some(value.to_string())), + }; + + match global_check_cfg.expecteds.get(name) { + Some(ExpectedValues::Some(values)) if !values.contains(&value) => { + let level = lint_level.to_diagnostic_level(); + if lint_level == LintLevel::Forbid || lint_level == LintLevel::Deny { + *error_count += 1; + } + + let value = match value { + Some(value) => Cow::from(format!("`{value}`")), + None => Cow::from("(none)"), + }; + + // Retrieving the span can fail if our pretty-priting doesn't match what the + // user wrote: `unix="foo"` and `unix = "foo"`, for that reason we don't fail + // and just produce a slightly worth diagnostic. + if let Some(span) = get_span(manifest.document(), &["target", &*format!("cfg({cfg_expr})")], false) { + let manifest_path = rel_cwd_manifest_path(path, gctx); + let title = format!( + "unexpected `cfg` condition value: {value} for `{cfg}`" + ); + let diag = level.title(&*title).snippet( + Snippet::source(manifest.contents()) + .origin(&manifest_path) + .annotation(level.span(span)) + .fold(true) + ); + gctx.shell().print_message(diag)?; + } else { + let title = format!( + "unexpected `cfg` condition value: {value} for `{cfg}` in `[target.'cfg({cfg_expr})']`" + ); + let help_where = format!("occurred in `{path}`", path = path.display()); + let diag = level.title(&*title).footer(Level::Help.title(&*help_where)); + gctx.shell().print_message(diag)?; + } + } + None => { + let level = lint_level.to_diagnostic_level(); + if lint_level == LintLevel::Forbid || lint_level == LintLevel::Deny { + *error_count += 1; + } + + let for_cfg = match value { + Some(_value) => Cow::from(format!(" for `{cfg}`")), + None => Cow::from(""), + }; + + // Retrieving the span can fail if our pretty-priting doesn't match what the + // user wrote: `unix="foo"` and `unix = "foo"`, for that reason we don't fail + // and just produce a slightly worth diagnostic. + if let Some(span) = get_span(manifest.document(), &["target", &*format!("cfg({cfg_expr})")], false) { + let manifest_path = rel_cwd_manifest_path(path, gctx); + let title = format!( + "unexpected `cfg` condition name: {name}{for_cfg}" + ); + let diag = level.title(&*title).snippet( + Snippet::source(manifest.contents()) + .origin(&manifest_path) + .annotation(level.span(span)) + .fold(true) + ); + gctx.shell().print_message(diag)?; + } else { + let title = format!( + "unexpected `cfg` condition name: {name}{for_cfg} in `[target.'cfg({cfg_expr})']`" + ); + let help_where = format!("occurred in `{path}`", path = path.display()); + let diag = level.title(&*title).footer(Level::Help.title(&*help_where)); + gctx.shell().print_message(diag)?; + } + } + _ => { /* not unexpected */ } + } + Ok(()) + })?; + } + + Ok(()) +} + #[cfg(test)] mod tests { use itertools::Itertools; diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index 8305f14fce1f..55ff7825ab34 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -1757,7 +1757,7 @@ This feature checks for unexpected cfgs in `[target.'cfg(...)']` entries, based on `rustc --print=check-cfg`. ```sh -cargo check -Zcheck-target-cfgs +cargo check -Zcargo-lints -Zcheck-target-cfgs ``` It follows the lint Rust `unexpected_cfgs` lint configuration: diff --git a/tests/testsuite/cfg.rs b/tests/testsuite/cfg.rs index 15d20a05bffd..f0d40aaadce1 100644 --- a/tests/testsuite/cfg.rs +++ b/tests/testsuite/cfg.rs @@ -564,10 +564,39 @@ fn unexpected_cfgs_target() { .file("c/src/lib.rs", "") .build(); - p.cargo("check -Zcheck-target-cfgs") + p.cargo("check -Zcargo-lints -Zcheck-target-cfgs") .masquerade_as_nightly_cargo(&["requires -Zcheck-target-cfgs"]) - // FIXME: We should warn on multiple cfgs .with_stderr_data(str![[r#" +[WARNING] unexpected `cfg` condition name: foo + --> Cargo.toml:11:25 + | +11 | [target."cfg(any(foo, all(bar)))".dependencies] + | ------------------------- + | +[WARNING] unexpected `cfg` condition name: bar + --> Cargo.toml:11:25 + | +11 | [target."cfg(any(foo, all(bar)))".dependencies] + | ------------------------- + | +[WARNING] unexpected `cfg` condition value: `` for `windows = ""` + --> Cargo.toml:18:25 + | +18 | [target.'cfg(not(windows = ""))'.dependencies] + | ------------------------ + | +[WARNING] unexpected `cfg` condition value: `zoo` for `unix = "zoo"` + --> Cargo.toml:14:25 + | +14 | [target.'cfg(unix = "zoo")'.dependencies] + | ------------------- + | +[WARNING] unexpected `cfg` condition value: `zoo` for `unix = "zoo"` + --> Cargo.toml:14:25 + | +14 | [target.'cfg(unix = "zoo")'.dependencies] + | ------------------- + | [LOCKING] 2 packages to latest compatible versions [CHECKING] b v0.0.1 ([ROOT]/foo/b) [CHECKING] a v0.0.1 ([ROOT]/foo) @@ -614,10 +643,28 @@ fn unexpected_cfgs_target_with_lint() { .file("b/src/lib.rs", "") .build(); - p.cargo("check -Zcheck-target-cfgs") + p.cargo("check -Zcargo-lints -Zcheck-target-cfgs") .masquerade_as_nightly_cargo(&["requires -Zcheck-target-cfgs"]) - // FIXME: We should warn on multiple cfgs + // FIXME: We should not warn on `cfg(foo = "foo")` but we currently do .with_stderr_data(str![[r#" +[WARNING] unexpected `cfg` condition name: bar + --> Cargo.toml:14:25 + | +14 | [target."cfg(bar)".dependencies] + | ---------- + | +[WARNING] unexpected `cfg` condition name: foo for `foo = "foo"` + --> Cargo.toml:11:25 + | +11 | [target.'cfg(foo = "foo")'.dependencies] # should not warn here + | ------------------ + | +[WARNING] unexpected `cfg` condition name: foo + --> Cargo.toml:8:25 + | +8 | [target."cfg(foo)".dependencies] # should not warn here + | ---------- + | [LOCKING] 1 package to latest compatible version [CHECKING] a v0.0.1 ([ROOT]/foo) [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s @@ -638,10 +685,10 @@ fn unexpected_cfgs_target_diagnostics() { edition = "2015" authors = [] - [target."cfg(target_pointer_width)".dependencies] + [target."cfg(target_pointer_width)".dependencies] # expect (none) as value b = { path = 'b' } - [target.'cfg( all(foo , bar))'.dependencies] + [target.'cfg( all(foo , bar))'.dependencies] # no snippet due to weird formatting b = { path = 'b' } "#, ) @@ -650,9 +697,19 @@ fn unexpected_cfgs_target_diagnostics() { .file("b/src/lib.rs", "") .build(); - p.cargo("check -Zcheck-target-cfgs") + p.cargo("check -Zcargo-lints -Zcheck-target-cfgs") .masquerade_as_nightly_cargo(&["requires -Zcheck-target-cfgs"]) .with_stderr_data(str![[r#" +[WARNING] unexpected `cfg` condition name: foo in `[target.'cfg(all(foo, bar))']` + = [HELP] occurred in `[ROOT]/foo/Cargo.toml` +[WARNING] unexpected `cfg` condition name: bar in `[target.'cfg(all(foo, bar))']` + = [HELP] occurred in `[ROOT]/foo/Cargo.toml` +[WARNING] unexpected `cfg` condition value: (none) for `target_pointer_width` + --> Cargo.toml:8:25 + | +8 | [target."cfg(target_pointer_width)".dependencies] # expect (none) as value + | --------------------------- + | [LOCKING] 1 package to latest compatible version [CHECKING] a v0.0.1 ([ROOT]/foo) [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s @@ -685,10 +742,16 @@ fn unexpected_cfgs_target_lint_level_allow() { .file("b/src/lib.rs", "") .build(); - p.cargo("check -Zcheck-target-cfgs") + p.cargo("check -Zcargo-lints -Zcheck-target-cfgs") .masquerade_as_nightly_cargo(&["requires -Zcheck-target-cfgs"]) - // FIXME: We should warn on multiple cfgs + // FIXME: We shouldn't warn any target cfgs because of the level="allow" .with_stderr_data(str![[r#" +[WARNING] unexpected `cfg` condition name: foo + --> Cargo.toml:8:25 + | +8 | [target."cfg(foo)".dependencies] + | ---------- + | [LOCKING] 1 package to latest compatible version [CHECKING] a v0.0.1 ([ROOT]/foo) [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s @@ -721,9 +784,15 @@ fn unexpected_cfgs_target_lint_level_deny() { .file("b/src/lib.rs", "") .build(); - p.cargo("check -Zcheck-target-cfgs") + p.cargo("check -Zcargo-lints -Zcheck-target-cfgs") .masquerade_as_nightly_cargo(&["requires -Zcheck-target-cfgs"]) .with_stderr_data(str![[r#" +[WARNING] unexpected `cfg` condition name: foo + --> Cargo.toml:8:25 + | +8 | [target."cfg(foo)".dependencies] + | ---------- + | [LOCKING] 1 package to latest compatible version [CHECKING] a v0.0.1 ([ROOT]/foo) [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s @@ -759,9 +828,16 @@ fn unexpected_cfgs_target_cfg_any() { .file("b/src/lib.rs", "") .build(); - p.cargo("check -Zcheck-target-cfgs") + p.cargo("check -Zcargo-lints -Zcheck-target-cfgs") .masquerade_as_nightly_cargo(&["requires -Zcheck-target-cfgs"]) + // FIXME: We shouldn't be linting `cfg(foo)` because of the `cfg(any())` .with_stderr_data(str![[r#" +[WARNING] unexpected `cfg` condition name: foo + --> Cargo.toml:8:25 + | +8 | [target."cfg(foo)".dependencies] + | ---------- + | [LOCKING] 1 package to latest compatible version [CHECKING] a v0.0.1 ([ROOT]/foo) [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s @@ -814,9 +890,6 @@ fn no_unexpected_cfgs_target() { p.cargo("check -Zcargo-lints") .masquerade_as_nightly_cargo(&["requires -Zcargo-lints"]) .with_stderr_data(str![[r#" -[LOCKING] 1 package to latest compatible version -[CHECKING] b v0.0.1 ([ROOT]/foo/b) -[CHECKING] a v0.0.1 ([ROOT]/foo) [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s "#]])