Skip to content

Commit

Permalink
Lint against some unexpected target cfgs
Browse files Browse the repository at this point in the history 
with the help of the Cargo lints infra
  • Loading branch information
@Urgau
Urgau committed Sep 29, 2024
1 parent 8e56b25 commit 1c88b85
Show file tree
Hide file tree
Showing 4 changed files with 221 additions and 17 deletions.
4 changes: 3 additions & 1 deletion src/cargo/core/workspace.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ 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, check_implicit_features, unused_dependencies,
analyze_cargo_lints_table, check_im_a_teapot, check_implicit_features, unexpected_target_cfgs,
unused_dependencies,
};
use crate::util::toml::{read_manifest, InheritableFields};
use crate::util::{
Expand Down Expand Up @@ -1238,6 +1239,7 @@ impl<'gctx> Workspace<'gctx> {
check_im_a_teapot(pkg, &path, &cargo_lints, &mut error_count, self.gctx)?;
check_implicit_features(pkg, &path, &cargo_lints, &mut error_count, self.gctx)?;
unused_dependencies(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"
Expand Down
131 changes: 130 additions & 1 deletion src/cargo/util/lints.rs
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
use crate::core::compiler::{CompileKind, TargetInfo};
use crate::core::dependency::DepKind;
use crate::core::FeatureValue::Dep;
use crate::core::{Edition, Feature, FeatureValue, Features, Manifest, Package};
use crate::core::{Edition, Feature, FeatureValue, Features, Manifest, Package, Workspace};
use crate::util::interning::InternedString;
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::collections::HashSet;
use std::fmt::Display;
use std::ops::Range;
Expand Down Expand Up @@ -880,6 +883,132 @@ pub fn unused_dependencies(
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;
Expand Down
2 changes: 1 addition & 1 deletion src/doc/src/reference/unstable.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1741,7 +1741,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:
Expand Down
101 changes: 87 additions & 14 deletions tests/testsuite/cfg.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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' }
"#,
)
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
"#]])
Expand Down

0 comments on commit 1c88b85

Please sign in to comment.