Skip to content

Commit

Permalink
Auto merge of #13913 - Urgau:check-cfg-lints-sub-config, r=epage
Browse files Browse the repository at this point in the history
Add special `check-cfg` lint config for the `unexpected_cfgs` lint

### What does this PR try to resolve?

This PR adds a special `check-cfg` lint config for the `unexpected_cfgs` lint, as it was decided by T-cargo (in today's meeting).

The goal of this lint config is to provide a simple and cost-less alternative to the build-script `cargo::rustc-check-cfg` instruction.

```toml
[lints.rust]
unexpected_cfgs = { level = "warn", check-cfg = ['cfg(foo, values("bar"))'] }
```

### How should we test and review this PR?

I recommand reviewing commit by commit; and looking at all the new tests added in `check_cfg.rs`, I tried making them as exhaustive as I could, many of them are very similar to their non-config counterpart.

### Additional information

I didn't add *(actually removed from the 1st version of this PR)* the possibility to omit the `level` field if `check-cfg` is specified, #13913 (comment).

Regarding the implementation, I tried making it is as straight forward as possible, nothing over-engineered or complex.

r? `@epage` (or `@weihanglo` maybe)
  • Loading branch information
bors committed May 17, 2024
2 parents 2b88044 + 5e9ac4b commit 0de7f2e
Show file tree
Hide file tree
Showing 7 changed files with 386 additions and 37 deletions.
9 changes: 9 additions & 0 deletions crates/cargo-util-schemas/src/manifest/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1503,6 +1503,13 @@ impl TomlLint {
Self::Config(config) => config.priority,
}
}

pub fn config(&self) -> Option<&toml::Table> {
match self {
Self::Level(_) => None,
Self::Config(config) => Some(&config.config),
}
}
}

#[derive(Serialize, Deserialize, Debug, Clone)]
Expand All @@ -1511,6 +1518,8 @@ pub struct TomlLintConfig {
pub level: TomlLintLevel,
#[serde(default)]
pub priority: i8,
#[serde(flatten)]
pub config: toml::Table,
}

#[derive(Serialize, Deserialize, Debug, Copy, Clone, Eq, PartialEq)]
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/compiler/build_runner/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> {
let mut args = compiler::extern_args(&self, unit, &mut unstable_opts)?;
args.extend(compiler::lto_args(&self, unit));
args.extend(compiler::features_args(unit));
args.extend(compiler::check_cfg_args(&self, unit));
args.extend(compiler::check_cfg_args(&self, unit)?);

let script_meta = self.find_build_script_metadata(unit);
if let Some(meta) = script_meta {
Expand Down
39 changes: 32 additions & 7 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ use std::io::{BufRead, Write};
use std::path::{Path, PathBuf};
use std::sync::Arc;

use anyhow::{Context as _, Error};
use anyhow::{bail, Context as _, Error};
use lazycell::LazyCell;
use tracing::{debug, trace};

Expand Down Expand Up @@ -732,7 +732,7 @@ fn prepare_rustdoc(build_runner: &BuildRunner<'_, '_>, unit: &Unit) -> CargoResu
let doc_dir = build_runner.files().out_dir(unit);
rustdoc.arg("-o").arg(&doc_dir);
rustdoc.args(&features_args(unit));
rustdoc.args(&check_cfg_args(build_runner, unit));
rustdoc.args(&check_cfg_args(build_runner, unit)?);

add_error_format_and_color(build_runner, &mut rustdoc);
add_allow_features(build_runner, &mut rustdoc);
Expand Down Expand Up @@ -1125,7 +1125,7 @@ fn build_base_args(
}

cmd.args(&features_args(unit));
cmd.args(&check_cfg_args(build_runner, unit));
cmd.args(&check_cfg_args(build_runner, unit)?);

let meta = build_runner.files().metadata(unit);
cmd.arg("-C").arg(&format!("metadata={}", meta));
Expand Down Expand Up @@ -1310,7 +1310,7 @@ fn trim_paths_args(
}

/// Generates the `--check-cfg` arguments for the `unit`.
fn check_cfg_args(build_runner: &BuildRunner<'_, '_>, unit: &Unit) -> Vec<OsString> {
fn check_cfg_args(build_runner: &BuildRunner<'_, '_>, unit: &Unit) -> CargoResult<Vec<OsString>> {
if build_runner
.bcx
.target_data
Expand Down Expand Up @@ -1353,14 +1353,39 @@ fn check_cfg_args(build_runner: &BuildRunner<'_, '_>, unit: &Unit) -> Vec<OsStri
// Cargo and docs.rs than rustc and docs.rs. In particular, all users of docs.rs use
// Cargo, but not all users of rustc (like Rust-for-Linux) use docs.rs.

vec![
let mut args = vec![
OsString::from("--check-cfg"),
OsString::from("cfg(docsrs)"),
OsString::from("--check-cfg"),
arg_feature,
]
];

// Also include the custom arguments specified in `[lints.rust.unexpected_cfgs.check_cfg]`
if let Ok(Some(lints)) = unit.pkg.manifest().resolved_toml().resolved_lints() {
if let Some(rust_lints) = lints.get("rust") {
if let Some(unexpected_cfgs) = rust_lints.get("unexpected_cfgs") {
if let Some(config) = unexpected_cfgs.config() {
if let Some(check_cfg) = config.get("check-cfg") {
if let Ok(check_cfgs) =
toml::Value::try_into::<Vec<String>>(check_cfg.clone())
{
for check_cfg in check_cfgs {
args.push(OsString::from("--check-cfg"));
args.push(OsString::from(check_cfg));
}
// error about `check-cfg` not being a list-of-string
} else {
bail!("`lints.rust.unexpected_cfgs.check-cfg` must be a list of string");
}
}
}
}
}
}

Ok(args)
} else {
Vec::new()
Ok(Vec::new())
}
}

Expand Down
15 changes: 14 additions & 1 deletion src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2265,7 +2265,7 @@ supported tools: {}",
if tool == "cargo" && !gctx.cli_unstable().cargo_lints {
warn_for_cargo_lint_feature(gctx, warnings);
}
for name in lints.keys() {
for (name, config) in lints {
if let Some((prefix, suffix)) = name.split_once("::") {
if tool == prefix {
anyhow::bail!(
Expand All @@ -2278,6 +2278,19 @@ supported tools: {}",
} else {
anyhow::bail!("`lints.{tool}.{name}` is not a valid lint name")
}
} else if let Some(config) = config.config() {
for config_name in config.keys() {
// manually report unused manifest key warning since we collect all the "extra"
// keys and values inside the config table
//
// except for `rust.unexpected_cfgs.check-cfg` which is used by rustc/rustdoc
if !(tool == "rust" && name == "unexpected_cfgs" && config_name == "check-cfg")
{
let message =
format!("unused manifest key: `lints.{tool}.{name}.{config_name}`");
warnings.push(message);
}
}
}
}
}
Expand Down
26 changes: 0 additions & 26 deletions src/doc/src/reference/build-scripts.md
Original file line number Diff line number Diff line change
Expand Up @@ -292,32 +292,6 @@ if foo_bar_condition {
}
```

##### Example of local-only `build.rs`/build script

Build scripts can impose costs on downstream users, and crate authors who wish to avoid
this can use "local-only" build scripts, which are active locally but not packaged in the
distributed package. Completly removing the cost from downstream users.

The way to achieved this, is by excluding the `build.rs` in the `Cargo.toml` with the
`exclude` key:

```diff
[package]
name = "foo"
version = "0.1.0"
+ exclude = ["build.rs"]
```

*`build.rs`:*
```rust
fn main() {
// Warning: build.rs is not published to crates.io.

println!("cargo::rerun-if-changed=build.rs");
println!("cargo::rustc-check-cfg=cfg(foo)");
}
```

For a more complete example see in the [build script examples][build-script-examples] page
the [conditional compilation][conditional-compilation-example] example.

Expand Down
Loading

0 comments on commit 0de7f2e

Please sign in to comment.