From 4d09a6a7453c9bcc9d3226a26de7a4cf52433942 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 10 May 2023 16:16:15 +0200 Subject: [PATCH 01/19] feat(lints): `lints` feature skeleton --- src/cargo/core/features.rs | 3 +++ src/doc/src/reference/unstable.md | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index bb46ae91f55..04e2ca87741 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -483,6 +483,9 @@ features! { // Allow specifying rustflags directly in a profile (stable, workspace_inheritance, "1.64", "reference/unstable.html#workspace-inheritance"), + + // Allow specifying rustflags directly in a profile + (unstable, lints, "", "reference/unstable.html#lints"), } pub struct Feature { diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index bd610374747..88ff745e89f 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -93,6 +93,7 @@ Each new feature described below should explain how to use it. * [codegen-backend](#codegen-backend) --- Select the codegen backend used by rustc. * [per-package-target](#per-package-target) --- Sets the `--target` to use for each individual package. * [artifact dependencies](#artifact-dependencies) --- Allow build artifacts to be included into other build artifacts and build them for different targets. + * [`[lints]`](#lints) --- Configure lint levels for various linter tools * Information and metadata * [Build-plan](#build-plan) --- Emits JSON information on which commands will be run. * [unit-graph](#unit-graph) --- Emits JSON for Cargo's internal graph structure. @@ -1391,6 +1392,10 @@ Valid operations are the following: * When the unstable feature is on, fetching/cloning a git repository is always a shallow fetch. This roughly equals to `git fetch --depth 1` everywhere. * Even with the presence of `Cargo.lock` or specifying a commit `{ rev = "…" }`, gitoxide is still smart enough to shallow fetch without unshallowing the existing repository. +### `[lints]` + +* Tracking Issue: [#12115](https://github.com/rust-lang/cargo/issues/12115) + ## Stabilized and removed features ### Compile progress From 228f71f5f70d540bef2da16946d13a0e9227aca1 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 10 May 2023 16:18:34 +0200 Subject: [PATCH 02/19] docs(ref): Copy over guide-level explanation The implementation doesn't match this yet but this reflects what we'll be working towards. --- src/doc/src/reference/unstable.md | 96 ++++++++++++++++++++++++++++++- 1 file changed, 95 insertions(+), 1 deletion(-) diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index 88ff745e89f..08a84f3f62b 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -93,7 +93,7 @@ Each new feature described below should explain how to use it. * [codegen-backend](#codegen-backend) --- Select the codegen backend used by rustc. * [per-package-target](#per-package-target) --- Sets the `--target` to use for each individual package. * [artifact dependencies](#artifact-dependencies) --- Allow build artifacts to be included into other build artifacts and build them for different targets. - * [`[lints]`](#lints) --- Configure lint levels for various linter tools + * [`[lints]`](#lints) --- Configure lint levels for various linter tools. * Information and metadata * [Build-plan](#build-plan) --- Emits JSON information on which commands will be run. * [unit-graph](#unit-graph) --- Emits JSON for Cargo's internal graph structure. @@ -1396,6 +1396,100 @@ Valid operations are the following: * Tracking Issue: [#12115](https://github.com/rust-lang/cargo/issues/12115) +A new `lints` table would be added to configure lints: +```toml +cargo-features = ["lints"] + +[lints.rust] +unsafe = "forbid" +``` +and `cargo` would pass these along as flags to `rustc`, `clippy`, or other lint tools. + +This would work with +[RFC 2906 `workspace-deduplicate`](https://rust-lang.github.io/rfcs/2906-cargo-workspace-deduplicate.html): +```toml +cargo-features = ["lints"] + +[lints] +workspace = true + +[workspace.lints.rust] +unsafe = "forbid" +``` + +#### Documentation Updates + +##### The `lints` section + +*as a new ["Manifest Format" entry](./manifest.html#the-manifest-format)* + +Override the default level of lints from different tools by assigning them to a new level in a +table, for example: +```toml +[lints.rust] +unsafe = "forbid" +``` + +This is short-hand for: +```toml +[lints.rust] +unsafe = { level = "forbid", priority = 0 } +``` + +`level` corresponds to the lint levels in `rustc`: +- `forbid` +- `deny` +- `warn` +- `allow` + +`priority` is a signed integer that controls which lints or lint groups override other lint groups: +- lower (particularly negative) numbers have lower priority, being overridden + by higher numbers, and show up first on the command-line to tools like + `rustc` + +To know which table under `[lints]` a particular lint belongs under, it is the part before `::` in the lint +name. If there isn't a `::`, then the tool is `rust`. For example a warning +about `unsafe` would be `lints.rust.unsafe` but a lint about +`clippy::enum_glob_use` would be `lints.clippy.enum_glob_use`. + +For example: +```toml +[lints.rust] +unsafe = "forbid" + +[lints.clippy] +enum_glob_use = "deny" +``` + +##### The `lints` table + +*as a new [`[workspace]` entry](./workspaces.html#the-workspace-section)* + +The `workspace.lints` table is where you define lint configuration to be inherited by members of a workspace. + +Specifying a workspace lint configuration is similar to package lints. + +Example: + +```toml +# [PROJECT_DIR]/Cargo.toml +[workspace] +members = ["crates/*"] + +[workspace.lints.rust] +unsafe = "forbid" +``` + +```toml +# [PROJECT_DIR]/crates/bar/Cargo.toml +[package] +name = "bar" +version = "0.1.0" + +[lints] +workspace = true +``` + ## Stabilized and removed features ### Compile progress From 8de25292fd54755299bab2644e8da40f3e27b190 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 10 May 2023 23:11:12 +0200 Subject: [PATCH 03/19] feat(lints): Parse `[lints]` table on nightly --- src/cargo/util/toml/mod.rs | 89 ++++++++++++++++++++++++++++++++++++++ tests/testsuite/lints.rs | 69 +++++++++++++++++++++++++++++ tests/testsuite/main.rs | 1 + 3 files changed, 159 insertions(+) create mode 100644 tests/testsuite/lints.rs diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 3b2bba09cfc..07c8bbb18ae 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -355,6 +355,7 @@ pub struct TomlManifest { patch: Option>>, workspace: Option, badges: Option, + lints: Option, } #[derive(Deserialize, Serialize, Clone, Debug, Default)] @@ -1421,6 +1422,29 @@ impl<'de> de::Deserialize<'de> for MaybeWorkspaceBtreeMap { } } +type MaybeWorkspaceLints = MaybeWorkspace; + +impl<'de> de::Deserialize<'de> for MaybeWorkspaceLints { + fn deserialize(deserializer: D) -> Result + where + D: de::Deserializer<'de>, + { + let value = serde_value::Value::deserialize(deserializer)?; + + if let Ok(w) = TomlWorkspaceField::deserialize( + serde_value::ValueDeserializer::::new(value.clone()), + ) { + return if w.workspace() { + Ok(MaybeWorkspace::Workspace(w)) + } else { + Err(de::Error::custom("`workspace` cannot be false")) + }; + } + BTreeMap::deserialize(serde_value::ValueDeserializer::::new(value)) + .map(MaybeWorkspace::Defined) + } +} + #[derive(Deserialize, Serialize, Clone, Debug)] pub struct TomlWorkspaceField { #[serde(deserialize_with = "bool_no_false")] @@ -1507,6 +1531,7 @@ pub struct TomlWorkspace { // Properties that can be inherited by members. package: Option, dependencies: Option>, + lints: Option, // Note that this field must come last due to the way toml serialization // works which requires tables to be emitted after all values. @@ -1520,6 +1545,9 @@ pub struct InheritableFields { // and we don't want it present when serializing #[serde(skip)] dependencies: Option>, + #[serde(skip)] + lints: Option, + version: Option, authors: Option>, description: Option, @@ -1550,6 +1578,10 @@ impl InheritableFields { self.dependencies = deps; } + pub fn update_lints(&mut self, lints: Option) { + self.lints = lints; + } + pub fn update_ws_path(&mut self, ws_root: PathBuf) { self.ws_root = ws_root; } @@ -1561,6 +1593,12 @@ impl InheritableFields { ) } + pub fn lints(&self) -> CargoResult { + self.lints + .clone() + .map_or(Err(anyhow!("`workspace.lints` was not defined")), |d| Ok(d)) + } + pub fn get_dependency(&self, name: &str, package_root: &Path) -> CargoResult { self.dependencies.clone().map_or( Err(anyhow!("`workspace.dependencies` was not defined")), @@ -1878,6 +1916,7 @@ impl TomlManifest { workspace: None, badges: self.badges.clone(), cargo_features: self.cargo_features.clone(), + lints: self.lints.clone(), }); fn map_deps( @@ -2006,6 +2045,8 @@ impl TomlManifest { let mut inheritable = toml_config.package.clone().unwrap_or_default(); inheritable.update_ws_path(package_root.to_path_buf()); inheritable.update_deps(toml_config.dependencies.clone()); + verify_lints(toml_config.lints.as_ref(), &features)?; + inheritable.update_lints(toml_config.lints.clone()); if let Some(ws_deps) = &inheritable.dependencies { for (name, dep) in ws_deps { unused_dep_keys( @@ -2269,6 +2310,13 @@ impl TomlManifest { &inherit_cell, )?; + let lints = me + .lints + .clone() + .map(|mw| mw.resolve("lints", || inherit()?.lints())) + .transpose()?; + verify_lints(lints.as_ref(), &features)?; + let mut target: BTreeMap = BTreeMap::new(); for (name, platform) in me.target.iter().flatten() { cx.platform = { @@ -2566,6 +2614,7 @@ impl TomlManifest { .badges .as_ref() .map(|_| MaybeWorkspace::Defined(metadata.badges.clone())), + lints: lints.map(|lints| MaybeWorkspace::Defined(lints)), }; let mut manifest = Manifest::new( summary, @@ -2695,6 +2744,8 @@ impl TomlManifest { let mut inheritable = toml_config.package.clone().unwrap_or_default(); inheritable.update_ws_path(root.to_path_buf()); inheritable.update_deps(toml_config.dependencies.clone()); + verify_lints(toml_config.lints.as_ref(), &features)?; + inheritable.update_lints(toml_config.lints.clone()); let ws_root_config = WorkspaceRootConfig::new( root, &toml_config.members, @@ -2839,6 +2890,16 @@ impl TomlManifest { } } +fn verify_lints(lints: Option<&TomlLints>, features: &Features) -> CargoResult<()> { + if lints.is_none() { + return Ok(()); + }; + + features.require(Feature::lints())?; + + Ok(()) +} + fn unused_dep_keys( dep_name: &str, kind: &str, @@ -3396,3 +3457,31 @@ impl fmt::Debug for PathValue { self.0.fmt(f) } } + +pub type TomlLints = BTreeMap; + +pub type TomlToolLints = BTreeMap; + +#[derive(Serialize, Deserialize, Debug, Clone)] +#[serde(untagged)] +pub enum TomlLint { + Level(TomlLintLevel), + Config(TomlLintConfig), +} + +#[derive(Serialize, Deserialize, Debug, Clone)] +#[serde(rename_all = "kebab-case")] +pub struct TomlLintConfig { + level: TomlLintLevel, + #[serde(default)] + priority: i8, +} + +#[derive(Serialize, Deserialize, Debug, Copy, Clone)] +#[serde(rename_all = "kebab-case")] +pub enum TomlLintLevel { + Forbid, + Deny, + Warn, + Allow, +} diff --git a/tests/testsuite/lints.rs b/tests/testsuite/lints.rs new file mode 100644 index 00000000000..384d0958051 --- /dev/null +++ b/tests/testsuite/lints.rs @@ -0,0 +1,69 @@ +//! Tests for `[lints]` + +use cargo_test_support::project; + +#[cargo_test] +fn package_requires_option() { + let foo = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + authors = [] + + [lints.rust] + unsafe_code = "forbid" + "#, + ) + .file("src/lib.rs", "") + .build(); + + foo.cargo("check") + .with_status(101) + .with_stderr("\ +[..] + +Caused by: + feature `lints` is required + + The package requires the Cargo feature called `lints`, but that feature is not stabilized in this version of Cargo ([..]). + Consider trying a newer version of Cargo (this may require the nightly release). + See https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#lints for more information about the status of this feature. +") + .run(); +} + +#[cargo_test] +fn workspace_requires_option() { + let foo = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + authors = [] + + [workspace.lints.rust] + unsafe_code = "forbid" + "#, + ) + .file("src/lib.rs", "") + .build(); + + foo.cargo("check") + .with_status(101) + .with_stderr("\ +[..] + +Caused by: + feature `lints` is required + + The package requires the Cargo feature called `lints`, but that feature is not stabilized in this version of Cargo ([..]). + Consider trying a newer version of Cargo (this may require the nightly release). + See https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#lints for more information about the status of this feature. +") + .run(); +} diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index efc31f4933e..4308123ad2e 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -68,6 +68,7 @@ mod init; mod install; mod install_upgrade; mod jobserver; +mod lints; mod list_availables; mod local_registry; mod locate_project; From fad2ea5cfd33e790a4bda80d28aa56414b1532c1 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 12 May 2023 14:29:43 +0200 Subject: [PATCH 04/19] feat(lints): Error check the lints table --- src/cargo/util/toml/mod.rs | 27 +++++++- tests/testsuite/lints.rs | 136 +++++++++++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+), 3 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 07c8bbb18ae..2adcb8f812f 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -2891,12 +2891,33 @@ impl TomlManifest { } fn verify_lints(lints: Option<&TomlLints>, features: &Features) -> CargoResult<()> { - if lints.is_none() { - return Ok(()); - }; + let Some(lints) = lints else { return Ok(()); }; features.require(Feature::lints())?; + for (tool, lints) in lints { + let supported = ["rust", "clippy", "rustdoc"]; + if !supported.contains(&tool.as_str()) { + let supported = supported.join(", "); + anyhow::bail!("unsupported `{tool}` in `[lints]`, must be one of {supported}") + } + for name in lints.keys() { + if let Some((prefix, suffix)) = name.split_once("::") { + if tool == prefix { + anyhow::bail!( + "`lints.{tool}.{name}` is not valid lint name; try `lints.{prefix}.{suffix}`" + ) + } else if tool == "rust" && supported.contains(&prefix) { + anyhow::bail!( + "`lints.{tool}.{name}` is not valid lint name; try `lints.{prefix}.{suffix}`" + ) + } else { + anyhow::bail!("`lints.{tool}.{name}` is not a valid lint name") + } + } + } + } + Ok(()) } diff --git a/tests/testsuite/lints.rs b/tests/testsuite/lints.rs index 384d0958051..53a47f664c5 100644 --- a/tests/testsuite/lints.rs +++ b/tests/testsuite/lints.rs @@ -67,3 +67,139 @@ Caused by: ") .run(); } + +#[cargo_test] +fn fail_on_invalid_tool() { + let foo = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["lints"] + + [package] + name = "foo" + version = "0.0.1" + authors = [] + + [workspace.lints.super-awesome-linter] + unsafe_code = "forbid" + "#, + ) + .file("src/lib.rs", "") + .build(); + + foo.cargo("check") + .masquerade_as_nightly_cargo(&["lints"]) + .with_status(101) + .with_stderr( + "\ +[..] + +Caused by: + unsupported `super-awesome-linter` in `[lints]`, must be one of rust, clippy, rustdoc +", + ) + .run(); +} + +#[cargo_test] +fn fail_on_tool_injection() { + let foo = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["lints"] + + [package] + name = "foo" + version = "0.0.1" + authors = [] + + [workspace.lints.rust] + "clippy::cyclomatic_complexity" = "warn" + "#, + ) + .file("src/lib.rs", "") + .build(); + + foo.cargo("check") + .masquerade_as_nightly_cargo(&["lints"]) + .with_status(101) + .with_stderr( + "\ +[..] + +Caused by: + `lints.rust.clippy::cyclomatic_complexity` is not valid lint name; try `lints.clippy.cyclomatic_complexity` +", + ) + .run(); +} + +#[cargo_test] +fn fail_on_redundant_tool() { + let foo = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["lints"] + + [package] + name = "foo" + version = "0.0.1" + authors = [] + + [workspace.lints.rust] + "rust::unsafe_code" = "forbid" + "#, + ) + .file("src/lib.rs", "") + .build(); + + foo.cargo("check") + .masquerade_as_nightly_cargo(&["lints"]) + .with_status(101) + .with_stderr( + "\ +[..] + +Caused by: + `lints.rust.rust::unsafe_code` is not valid lint name; try `lints.rust.unsafe_code` +", + ) + .run(); +} + +#[cargo_test] +fn fail_on_conflicting_tool() { + let foo = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["lints"] + + [package] + name = "foo" + version = "0.0.1" + authors = [] + + [workspace.lints.rust] + "super-awesome-tool::unsafe_code" = "forbid" + "#, + ) + .file("src/lib.rs", "") + .build(); + + foo.cargo("check") + .masquerade_as_nightly_cargo(&["lints"]) + .with_status(101) + .with_stderr( + "\ +[..] + +Caused by: + `lints.rust.super-awesome-tool::unsafe_code` is not a valid lint name +", + ) + .run(); +} From 25c084a3447dad66705c77d11d840676bd666e15 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 15 May 2023 16:31:29 +0200 Subject: [PATCH 05/19] feat(lints): Expose TomlLints as Manifest::rustflags --- src/cargo/core/manifest.rs | 8 ++++++ src/cargo/util/toml/mod.rs | 56 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/src/cargo/core/manifest.rs b/src/cargo/core/manifest.rs index 9f77b1301fb..62b6501a422 100644 --- a/src/cargo/core/manifest.rs +++ b/src/cargo/core/manifest.rs @@ -63,6 +63,7 @@ pub struct Manifest { default_run: Option, metabuild: Option>, resolve_behavior: Option, + rustflags: Vec, } /// When parsing `Cargo.toml`, some warnings should silenced @@ -405,6 +406,7 @@ impl Manifest { original: Rc, metabuild: Option>, resolve_behavior: Option, + rustflags: Vec, ) -> Manifest { Manifest { summary, @@ -430,6 +432,7 @@ impl Manifest { default_run, metabuild, resolve_behavior, + rustflags, } } @@ -514,6 +517,11 @@ impl Manifest { self.resolve_behavior } + /// Package-wide RUSTFLAGS + pub fn rustflags(&self) -> &[String] { + self.rustflags.as_slice() + } + pub fn map_source(self, to_replace: SourceId, replace_with: SourceId) -> Manifest { Manifest { summary: self.summary.map_source(to_replace, replace_with), diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 2adcb8f812f..f251a53aacd 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -2316,6 +2316,34 @@ impl TomlManifest { .map(|mw| mw.resolve("lints", || inherit()?.lints())) .transpose()?; verify_lints(lints.as_ref(), &features)?; + let default = TomlLints::default(); + let mut rustflags = lints + .as_ref() + .unwrap_or(&default) + .iter() + .flat_map(|(tool, lints)| { + lints.iter().map(move |(name, config)| { + let flag = config.level().flag(); + let option = if tool == "rust" { + format!("{flag}={name}") + } else { + format!("{flag}={tool}::{name}") + }; + ( + config.priority(), + // Since the most common group will be `all`, put it last so people are more + // likely to notice that they need to use `priority`. + std::cmp::Reverse(name), + option, + ) + }) + }) + .collect::>(); + rustflags.sort(); + let rustflags = rustflags + .into_iter() + .map(|(_, _, option)| option) + .collect::>(); let mut target: BTreeMap = BTreeMap::new(); for (name, platform) in me.target.iter().flatten() { @@ -2639,6 +2667,7 @@ impl TomlManifest { Rc::new(resolved_toml), package.metabuild.clone().map(|sov| sov.0), resolve_behavior, + rustflags, ); if package.license_file.is_some() && package.license.is_some() { manifest.warnings_mut().add_warning( @@ -3490,6 +3519,22 @@ pub enum TomlLint { Config(TomlLintConfig), } +impl TomlLint { + fn level(&self) -> TomlLintLevel { + match self { + Self::Level(level) => *level, + Self::Config(config) => config.level, + } + } + + fn priority(&self) -> i8 { + match self { + Self::Level(_) => 0, + Self::Config(config) => config.priority, + } + } +} + #[derive(Serialize, Deserialize, Debug, Clone)] #[serde(rename_all = "kebab-case")] pub struct TomlLintConfig { @@ -3506,3 +3551,14 @@ pub enum TomlLintLevel { Warn, Allow, } + +impl TomlLintLevel { + fn flag(&self) -> &'static str { + match self { + Self::Forbid => "--forbid", + Self::Deny => "--deny", + Self::Warn => "--warn", + Self::Allow => "--allow", + } + } +} From 4279d0d56c161c91562c05c7ec85de575c97cdca Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 15 May 2023 11:45:47 -0500 Subject: [PATCH 06/19] feat(lints): Pass lints to tools --- src/cargo/core/compiler/fingerprint/mod.rs | 4 + src/cargo/core/compiler/mod.rs | 2 + tests/testsuite/lints.rs | 295 +++++++++++++++++++++ 3 files changed, 301 insertions(+) diff --git a/src/cargo/core/compiler/fingerprint/mod.rs b/src/cargo/core/compiler/fingerprint/mod.rs index 7401afebca4..a67c1eb3edc 100644 --- a/src/cargo/core/compiler/fingerprint/mod.rs +++ b/src/cargo/core/compiler/fingerprint/mod.rs @@ -75,6 +75,7 @@ //! [`Lto`] flags | ✓ | ✓ //! config settings[^5] | ✓ | //! is_std | | ✓ +//! `[lints]` table[^6] | ✓ | //! //! [^1]: Build script and bin dependencies are not included. //! @@ -86,6 +87,8 @@ //! [^5]: Config settings that are not otherwise captured anywhere else. //! Currently, this is only `doc.extern-map`. //! +//! [^6]: Via [`Manifest::rustflags`][crate::core::Manifest::rustflags] +//! //! When deciding what should go in the Metadata vs the Fingerprint, consider //! that some files (like dylibs) do not have a hash in their filename. Thus, //! if a value changes, only the fingerprint will detect the change (consider, @@ -1414,6 +1417,7 @@ fn calculate_normal(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult, unit: &Unit) -> CargoResult { add_error_format_and_color(cx, &mut rustdoc); add_allow_features(cx, &mut rustdoc); + rustdoc.args(unit.pkg.manifest().rustflags()); if let Some(args) = cx.bcx.extra_args_for(unit) { rustdoc.args(args); } @@ -1078,6 +1079,7 @@ fn build_base_args( cmd.arg("-C").arg(format!("debuginfo={}", debuginfo)); } + cmd.args(unit.pkg.manifest().rustflags()); if let Some(args) = cx.bcx.extra_args_for(unit) { cmd.args(args); } diff --git a/tests/testsuite/lints.rs b/tests/testsuite/lints.rs index 53a47f664c5..c8727d4fed2 100644 --- a/tests/testsuite/lints.rs +++ b/tests/testsuite/lints.rs @@ -1,6 +1,7 @@ //! Tests for `[lints]` use cargo_test_support::project; +use cargo_test_support::registry::Package; #[cargo_test] fn package_requires_option() { @@ -203,3 +204,297 @@ Caused by: ) .run(); } + +#[cargo_test] +fn package_lint_deny() { + let foo = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["lints"] + + [package] + name = "foo" + version = "0.0.1" + authors = [] + + [lints.rust] + "unsafe_code" = "deny" + "#, + ) + .file( + "src/lib.rs", + " +pub fn foo(num: i32) -> u32 { + unsafe { std::mem::transmute(num) } +} +", + ) + .build(); + + foo.cargo("check") + .masquerade_as_nightly_cargo(&["lints"]) + .with_status(101) + .with_stderr( + "\ +[..] +error: usage of an `unsafe` block +[..] +[..] +[..] +[..] +[..] +[..] +[..] +error: could not compile `foo` (lib) due to previous error +", + ) + .run(); +} + +#[cargo_test] +fn workspace_lint_deny() { + let foo = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["lints"] + + [package] + name = "foo" + version = "0.0.1" + authors = [] + + [lints] + workspace = true + + [workspace.lints.rust] + "unsafe_code" = "deny" + "#, + ) + .file( + "src/lib.rs", + " +pub fn foo(num: i32) -> u32 { + unsafe { std::mem::transmute(num) } +} +", + ) + .build(); + + foo.cargo("check") + .masquerade_as_nightly_cargo(&["lints"]) + .with_status(101) + .with_stderr( + "\ +[..] +error: usage of an `unsafe` block +[..] +[..] +[..] +[..] +[..] +[..] +[..] +error: could not compile `foo` (lib) due to previous error +", + ) + .run(); +} + +#[cargo_test] +fn attribute_has_precedence() { + let foo = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["lints"] + + [package] + name = "foo" + version = "0.0.1" + authors = [] + + [lints.rust] + "unsafe_code" = "deny" + "#, + ) + .file( + "src/lib.rs", + " +#![allow(unsafe_code)] + +pub fn foo(num: i32) -> u32 { + unsafe { std::mem::transmute(num) } +} +", + ) + .build(); + + foo.cargo("check") + .masquerade_as_nightly_cargo(&["lints"]) + .with_status(0) + .run(); +} + +#[cargo_test] +fn rustflags_has_precedence() { + let foo = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["lints"] + + [package] + name = "foo" + version = "0.0.1" + authors = [] + + [lints.rust] + "unsafe_code" = "deny" + "#, + ) + .file( + "src/lib.rs", + " +pub fn foo(num: i32) -> u32 { + unsafe { std::mem::transmute(num) } +} +", + ) + .build(); + + foo.cargo("check") + .arg("-v") + .env("RUSTFLAGS", "-Aunsafe_code") + .masquerade_as_nightly_cargo(&["lints"]) + .with_status(0) + .run(); +} + +#[cargo_test] +fn without_priority() { + Package::new("reg-dep", "1.0.0").publish(); + + let foo = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["lints"] + + [package] + name = "foo" + version = "0.0.1" + edition = "2018" + authors = [] + + [dependencies] + reg-dep = "1.0.0" + + [lints.rust] + "rust-2018-idioms" = "deny" + "unused-extern-crates" = "allow" + "#, + ) + .file( + "src/lib.rs", + " +extern crate reg_dep; + +pub fn foo() -> u32 { + 2 +} +", + ) + .build(); + + foo.cargo("check") + .masquerade_as_nightly_cargo(&["lints"]) + .with_status(101) + .with_stderr_contains( + "\ +error: unused extern crate +", + ) + .run(); +} + +#[cargo_test] +fn with_priority() { + Package::new("reg-dep", "1.0.0").publish(); + + let foo = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["lints"] + + [package] + name = "foo" + version = "0.0.1" + edition = "2018" + authors = [] + + [dependencies] + reg-dep = "1.0.0" + + [lints.rust] + "rust-2018-idioms" = { level = "deny", priority = -1 } + "unused-extern-crates" = "allow" + "#, + ) + .file( + "src/lib.rs", + " +extern crate reg_dep; + +pub fn foo() -> u32 { + 2 +} +", + ) + .build(); + + foo.cargo("check") + .masquerade_as_nightly_cargo(&["lints"]) + .with_status(0) + .run(); +} + +#[cargo_test] +fn rustdoc_lint() { + let foo = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["lints"] + + [package] + name = "foo" + version = "0.0.1" + authors = [] + + [lints.rustdoc] + broken_intra_doc_links = "deny" + "#, + ) + .file( + "src/lib.rs", + " +/// [`bar`] doesn't exist +pub fn foo() -> u32 { +} +", + ) + .build(); + + foo.cargo("doc") + .masquerade_as_nightly_cargo(&["lints"]) + .with_status(101) + .with_stderr_contains( + "\ +error: unresolved link to `bar` +", + ) + .run(); +} From a7555f976ed0a219dbb34f51433052606beecc1b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 15 May 2023 21:48:48 -0500 Subject: [PATCH 07/19] fix(lints): Only warn when seeing lints on stable This will make it easier for users to test this feature --- src/cargo/util/toml/mod.rs | 27 +++++++++++++++++---------- tests/testsuite/lints.rs | 29 ++++++++++++----------------- 2 files changed, 29 insertions(+), 27 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index f251a53aacd..36702c6359c 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -2045,8 +2045,8 @@ impl TomlManifest { let mut inheritable = toml_config.package.clone().unwrap_or_default(); inheritable.update_ws_path(package_root.to_path_buf()); inheritable.update_deps(toml_config.dependencies.clone()); - verify_lints(toml_config.lints.as_ref(), &features)?; - inheritable.update_lints(toml_config.lints.clone()); + let lints = verify_lints(toml_config.lints.clone(), &features, config)?; + inheritable.update_lints(lints); if let Some(ws_deps) = &inheritable.dependencies { for (name, dep) in ws_deps { unused_dep_keys( @@ -2315,7 +2315,7 @@ impl TomlManifest { .clone() .map(|mw| mw.resolve("lints", || inherit()?.lints())) .transpose()?; - verify_lints(lints.as_ref(), &features)?; + let lints = verify_lints(lints.clone(), &features, config)?; let default = TomlLints::default(); let mut rustflags = lints .as_ref() @@ -2773,8 +2773,8 @@ impl TomlManifest { let mut inheritable = toml_config.package.clone().unwrap_or_default(); inheritable.update_ws_path(root.to_path_buf()); inheritable.update_deps(toml_config.dependencies.clone()); - verify_lints(toml_config.lints.as_ref(), &features)?; - inheritable.update_lints(toml_config.lints.clone()); + let lints = verify_lints(toml_config.lints.clone(), &features, config)?; + inheritable.update_lints(lints); let ws_root_config = WorkspaceRootConfig::new( root, &toml_config.members, @@ -2919,12 +2919,19 @@ impl TomlManifest { } } -fn verify_lints(lints: Option<&TomlLints>, features: &Features) -> CargoResult<()> { - let Some(lints) = lints else { return Ok(()); }; +fn verify_lints( + lints: Option, + features: &Features, + config: &Config, +) -> CargoResult> { + let Some(lints) = lints else { return Ok(None); }; - features.require(Feature::lints())?; + if let Err(err) = features.require(Feature::lints()) { + let _ = config.shell().warn(err); + return Ok(None); + } - for (tool, lints) in lints { + for (tool, lints) in &lints { let supported = ["rust", "clippy", "rustdoc"]; if !supported.contains(&tool.as_str()) { let supported = supported.join(", "); @@ -2947,7 +2954,7 @@ fn verify_lints(lints: Option<&TomlLints>, features: &Features) -> CargoResult<( } } - Ok(()) + Ok(Some(lints)) } fn unused_dep_keys( diff --git a/tests/testsuite/lints.rs b/tests/testsuite/lints.rs index c8727d4fed2..c3f83edce2a 100644 --- a/tests/testsuite/lints.rs +++ b/tests/testsuite/lints.rs @@ -22,16 +22,15 @@ fn package_requires_option() { .build(); foo.cargo("check") - .with_status(101) .with_stderr("\ -[..] +warning: feature `lints` is required -Caused by: - feature `lints` is required +The package requires the Cargo feature called `lints`, but that feature is not stabilized in this version of Cargo ([..]). +Consider trying a newer version of Cargo (this may require the nightly release). +See https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#lints for more information about the status of this feature. - The package requires the Cargo feature called `lints`, but that feature is not stabilized in this version of Cargo ([..]). - Consider trying a newer version of Cargo (this may require the nightly release). - See https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#lints for more information about the status of this feature. +[CHECKING] [..] +[FINISHED] [..] ") .run(); } @@ -55,16 +54,15 @@ fn workspace_requires_option() { .build(); foo.cargo("check") - .with_status(101) .with_stderr("\ -[..] +warning: feature `lints` is required -Caused by: - feature `lints` is required +The package requires the Cargo feature called `lints`, but that feature is not stabilized in this version of Cargo ([..]). +Consider trying a newer version of Cargo (this may require the nightly release). +See https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#lints for more information about the status of this feature. - The package requires the Cargo feature called `lints`, but that feature is not stabilized in this version of Cargo ([..]). - Consider trying a newer version of Cargo (this may require the nightly release). - See https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#lints for more information about the status of this feature. +[CHECKING] [..] +[FINISHED] [..] ") .run(); } @@ -333,7 +331,6 @@ pub fn foo(num: i32) -> u32 { foo.cargo("check") .masquerade_as_nightly_cargo(&["lints"]) - .with_status(0) .run(); } @@ -368,7 +365,6 @@ pub fn foo(num: i32) -> u32 { .arg("-v") .env("RUSTFLAGS", "-Aunsafe_code") .masquerade_as_nightly_cargo(&["lints"]) - .with_status(0) .run(); } @@ -457,7 +453,6 @@ pub fn foo() -> u32 { foo.cargo("check") .masquerade_as_nightly_cargo(&["lints"]) - .with_status(0) .run(); } From 66038fad36f33383c56d5f8a15852198fbec8692 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 15 May 2023 21:55:36 -0500 Subject: [PATCH 08/19] test(lints): Show bug with lints on stable --- tests/testsuite/lints.rs | 64 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/tests/testsuite/lints.rs b/tests/testsuite/lints.rs index c3f83edce2a..e1bd8ca8c49 100644 --- a/tests/testsuite/lints.rs +++ b/tests/testsuite/lints.rs @@ -67,6 +67,70 @@ See https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#lints for mo .run(); } +#[cargo_test] +fn malformed_on_stable() { + let foo = project() + .file( + "Cargo.toml", + r#" + lints = 20 + [package] + name = "foo" + version = "0.0.1" + authors = [] + + "#, + ) + .file("src/lib.rs", "") + .build(); + + foo.cargo("check") + .with_status(101) + .with_stderr( + "\ +error: failed to parse manifest[..] + +Caused by: + invalid type: integer `20`, expected a map + in `lints` +", + ) + .run(); +} + +#[cargo_test] +fn malformed_on_nightly() { + let foo = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["lints"] + lints = 20 + [package] + name = "foo" + version = "0.0.1" + authors = [] + + "#, + ) + .file("src/lib.rs", "") + .build(); + + foo.cargo("check") + .masquerade_as_nightly_cargo(&["lints"]) + .with_status(101) + .with_stderr( + "\ +error: failed to parse manifest[..] + +Caused by: + invalid type: integer `20`, expected a map + in `lints` +", + ) + .run(); +} + #[cargo_test] fn fail_on_invalid_tool() { let foo = project() From 5072d9b789942c1a3edcebc3fa2be7b20e2a2484 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 17 May 2023 15:09:23 -0500 Subject: [PATCH 09/19] fix(lints): Ignore lints table on stable --- src/cargo/util/toml/mod.rs | 42 +++++++++++++++++++++++--------------- tests/testsuite/lints.rs | 13 ++++++------ 2 files changed, 32 insertions(+), 23 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 36702c6359c..83cf6416625 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -355,7 +355,7 @@ pub struct TomlManifest { patch: Option>>, workspace: Option, badges: Option, - lints: Option, + lints: Option, } #[derive(Deserialize, Serialize, Clone, Debug, Default)] @@ -1440,7 +1440,7 @@ impl<'de> de::Deserialize<'de> for MaybeWorkspaceLints { Err(de::Error::custom("`workspace` cannot be false")) }; } - BTreeMap::deserialize(serde_value::ValueDeserializer::::new(value)) + TomlLints::deserialize(serde_value::ValueDeserializer::::new(value)) .map(MaybeWorkspace::Defined) } } @@ -1531,7 +1531,7 @@ pub struct TomlWorkspace { // Properties that can be inherited by members. package: Option, dependencies: Option>, - lints: Option, + lints: Option, // Note that this field must come last due to the way toml serialization // works which requires tables to be emitted after all values. @@ -2045,7 +2045,8 @@ impl TomlManifest { let mut inheritable = toml_config.package.clone().unwrap_or_default(); inheritable.update_ws_path(package_root.to_path_buf()); inheritable.update_deps(toml_config.dependencies.clone()); - let lints = verify_lints(toml_config.lints.clone(), &features, config)?; + let lints = parse_unstable_lints(toml_config.lints.clone(), &features, config)?; + let lints = verify_lints(lints)?; inheritable.update_lints(lints); if let Some(ws_deps) = &inheritable.dependencies { for (name, dep) in ws_deps { @@ -2310,12 +2311,11 @@ impl TomlManifest { &inherit_cell, )?; - let lints = me - .lints - .clone() - .map(|mw| mw.resolve("lints", || inherit()?.lints())) - .transpose()?; - let lints = verify_lints(lints.clone(), &features, config)?; + let lints = + parse_unstable_lints::(me.lints.clone(), &features, config)? + .map(|mw| mw.resolve("lints", || inherit()?.lints())) + .transpose()?; + let lints = verify_lints(lints)?; let default = TomlLints::default(); let mut rustflags = lints .as_ref() @@ -2642,7 +2642,8 @@ impl TomlManifest { .badges .as_ref() .map(|_| MaybeWorkspace::Defined(metadata.badges.clone())), - lints: lints.map(|lints| MaybeWorkspace::Defined(lints)), + lints: lints + .map(|lints| toml::Value::try_from(MaybeWorkspaceLints::Defined(lints)).unwrap()), }; let mut manifest = Manifest::new( summary, @@ -2773,7 +2774,8 @@ impl TomlManifest { let mut inheritable = toml_config.package.clone().unwrap_or_default(); inheritable.update_ws_path(root.to_path_buf()); inheritable.update_deps(toml_config.dependencies.clone()); - let lints = verify_lints(toml_config.lints.clone(), &features, config)?; + let lints = parse_unstable_lints(toml_config.lints.clone(), &features, config)?; + let lints = verify_lints(lints)?; inheritable.update_lints(lints); let ws_root_config = WorkspaceRootConfig::new( root, @@ -2919,18 +2921,24 @@ impl TomlManifest { } } -fn verify_lints( - lints: Option, +fn parse_unstable_lints>( + lints: Option, features: &Features, config: &Config, -) -> CargoResult> { +) -> CargoResult> { let Some(lints) = lints else { return Ok(None); }; - if let Err(err) = features.require(Feature::lints()) { - let _ = config.shell().warn(err); + if let Err(unstable_err) = features.require(Feature::lints()) { + let _ = config.shell().warn(unstable_err); return Ok(None); } + lints.try_into().map(Some).map_err(|err| err.into()) +} + +fn verify_lints(lints: Option) -> CargoResult> { + let Some(lints) = lints else { return Ok(None); }; + for (tool, lints) in &lints { let supported = ["rust", "clippy", "rustdoc"]; if !supported.contains(&tool.as_str()) { diff --git a/tests/testsuite/lints.rs b/tests/testsuite/lints.rs index e1bd8ca8c49..ea0c51b2341 100644 --- a/tests/testsuite/lints.rs +++ b/tests/testsuite/lints.rs @@ -85,14 +85,16 @@ fn malformed_on_stable() { .build(); foo.cargo("check") - .with_status(101) .with_stderr( "\ -error: failed to parse manifest[..] +warning: feature `lints` is required -Caused by: - invalid type: integer `20`, expected a map - in `lints` +The package requires the Cargo feature called `lints`, but that feature is not stabilized in this version of Cargo ([..]). +Consider trying a newer version of Cargo (this may require the nightly release). +See https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#lints for more information about the status of this feature. + +[CHECKING] [..] +[FINISHED] [..] ", ) .run(); @@ -125,7 +127,6 @@ error: failed to parse manifest[..] Caused by: invalid type: integer `20`, expected a map - in `lints` ", ) .run(); From a8d7c8a58f52509f7f586f8d229aa0b5e833bbba Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 17 May 2023 15:30:58 -0500 Subject: [PATCH 10/19] fix(lints): Clean up warnings about lints feature --- src/cargo/util/toml/mod.rs | 33 ++++++++++++++++++++++++++++++-- tests/testsuite/lints.rs | 39 +++++++++++++++++++------------------- 2 files changed, 51 insertions(+), 21 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 83cf6416625..dff6b7fe0a0 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -2928,14 +2928,43 @@ fn parse_unstable_lints>( ) -> CargoResult> { let Some(lints) = lints else { return Ok(None); }; - if let Err(unstable_err) = features.require(Feature::lints()) { - let _ = config.shell().warn(unstable_err); + if !features.is_enabled(Feature::lints()) { + warn_for_feature("lints", config); return Ok(None); } lints.try_into().map(Some).map_err(|err| err.into()) } +fn warn_for_feature(name: &str, config: &Config) { + use std::fmt::Write as _; + + let mut message = String::new(); + + let _ = write!( + message, + "feature `{name}` is not supported on this version of Cargo and will be ignored" + ); + if config.nightly_features_allowed { + let _ = write!( + message, + " + +consider adding `cargo-features = [\"{name}\"]` to the manifest" + ); + } else { + let _ = write!( + message, + " + +this Cargo does not support nightly features, but if you +switch to nightly channel you can add +`cargo-features = [\"{name}\"]` to enable this feature", + ); + } + let _ = config.shell().warn(&message); +} + fn verify_lints(lints: Option) -> CargoResult> { let Some(lints) = lints else { return Ok(None); }; diff --git a/tests/testsuite/lints.rs b/tests/testsuite/lints.rs index ea0c51b2341..3db8ac0fca2 100644 --- a/tests/testsuite/lints.rs +++ b/tests/testsuite/lints.rs @@ -22,16 +22,17 @@ fn package_requires_option() { .build(); foo.cargo("check") - .with_stderr("\ -warning: feature `lints` is required - -The package requires the Cargo feature called `lints`, but that feature is not stabilized in this version of Cargo ([..]). -Consider trying a newer version of Cargo (this may require the nightly release). -See https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#lints for more information about the status of this feature. + .with_stderr( + "\ +warning: feature `lints` is not supported on this version of Cargo and will be ignored +this Cargo does not support nightly features, but if you +switch to nightly channel you can add +`cargo-features = [\"lints\"]` to enable this feature [CHECKING] [..] [FINISHED] [..] -") +", + ) .run(); } @@ -54,16 +55,17 @@ fn workspace_requires_option() { .build(); foo.cargo("check") - .with_stderr("\ -warning: feature `lints` is required - -The package requires the Cargo feature called `lints`, but that feature is not stabilized in this version of Cargo ([..]). -Consider trying a newer version of Cargo (this may require the nightly release). -See https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#lints for more information about the status of this feature. + .with_stderr( + "\ +warning: feature `lints` is not supported on this version of Cargo and will be ignored +this Cargo does not support nightly features, but if you +switch to nightly channel you can add +`cargo-features = [\"lints\"]` to enable this feature [CHECKING] [..] [FINISHED] [..] -") +", + ) .run(); } @@ -87,12 +89,11 @@ fn malformed_on_stable() { foo.cargo("check") .with_stderr( "\ -warning: feature `lints` is required - -The package requires the Cargo feature called `lints`, but that feature is not stabilized in this version of Cargo ([..]). -Consider trying a newer version of Cargo (this may require the nightly release). -See https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#lints for more information about the status of this feature. +warning: feature `lints` is not supported on this version of Cargo and will be ignored +this Cargo does not support nightly features, but if you +switch to nightly channel you can add +`cargo-features = [\"lints\"]` to enable this feature [CHECKING] [..] [FINISHED] [..] ", From 1dfa101cd12238f750205e89f05543df572b29b3 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 19 May 2023 13:47:08 -0500 Subject: [PATCH 11/19] fix(lints): Tie into manifest warnings This improves the messaging experience (sometimes showing manifest path) and hiding the warning when not relevant). --- src/cargo/util/toml/mod.rs | 33 ++++++++++++++++++++++++--------- tests/testsuite/lints.rs | 2 +- 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index dff6b7fe0a0..e67f3ca7897 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -2045,7 +2045,12 @@ impl TomlManifest { let mut inheritable = toml_config.package.clone().unwrap_or_default(); inheritable.update_ws_path(package_root.to_path_buf()); inheritable.update_deps(toml_config.dependencies.clone()); - let lints = parse_unstable_lints(toml_config.lints.clone(), &features, config)?; + let lints = parse_unstable_lints( + toml_config.lints.clone(), + &features, + config, + &mut warnings, + )?; let lints = verify_lints(lints)?; inheritable.update_lints(lints); if let Some(ws_deps) = &inheritable.dependencies { @@ -2311,10 +2316,14 @@ impl TomlManifest { &inherit_cell, )?; - let lints = - parse_unstable_lints::(me.lints.clone(), &features, config)? - .map(|mw| mw.resolve("lints", || inherit()?.lints())) - .transpose()?; + let lints = parse_unstable_lints::( + me.lints.clone(), + &features, + config, + cx.warnings, + )? + .map(|mw| mw.resolve("lints", || inherit()?.lints())) + .transpose()?; let lints = verify_lints(lints)?; let default = TomlLints::default(); let mut rustflags = lints @@ -2774,7 +2783,12 @@ impl TomlManifest { let mut inheritable = toml_config.package.clone().unwrap_or_default(); inheritable.update_ws_path(root.to_path_buf()); inheritable.update_deps(toml_config.dependencies.clone()); - let lints = parse_unstable_lints(toml_config.lints.clone(), &features, config)?; + let lints = parse_unstable_lints( + toml_config.lints.clone(), + &features, + config, + &mut warnings, + )?; let lints = verify_lints(lints)?; inheritable.update_lints(lints); let ws_root_config = WorkspaceRootConfig::new( @@ -2925,18 +2939,19 @@ fn parse_unstable_lints>( lints: Option, features: &Features, config: &Config, + warnings: &mut Vec, ) -> CargoResult> { let Some(lints) = lints else { return Ok(None); }; if !features.is_enabled(Feature::lints()) { - warn_for_feature("lints", config); + warn_for_feature("lints", config, warnings); return Ok(None); } lints.try_into().map(Some).map_err(|err| err.into()) } -fn warn_for_feature(name: &str, config: &Config) { +fn warn_for_feature(name: &str, config: &Config, warnings: &mut Vec) { use std::fmt::Write as _; let mut message = String::new(); @@ -2962,7 +2977,7 @@ switch to nightly channel you can add `cargo-features = [\"{name}\"]` to enable this feature", ); } - let _ = config.shell().warn(&message); + warnings.push(message); } fn verify_lints(lints: Option) -> CargoResult> { diff --git a/tests/testsuite/lints.rs b/tests/testsuite/lints.rs index 3db8ac0fca2..ac0df8f5501 100644 --- a/tests/testsuite/lints.rs +++ b/tests/testsuite/lints.rs @@ -57,7 +57,7 @@ fn workspace_requires_option() { foo.cargo("check") .with_stderr( "\ -warning: feature `lints` is not supported on this version of Cargo and will be ignored +warning: [CWD]/Cargo.toml: feature `lints` is not supported on this version of Cargo and will be ignored this Cargo does not support nightly features, but if you switch to nightly channel you can add From e82fe885b5346c47814f13dced89a19fd99c3d3a Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 19 May 2023 15:05:55 -0500 Subject: [PATCH 12/19] test(lints): Verify precedence with profile.rustflags --- tests/testsuite/lints.rs | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/tests/testsuite/lints.rs b/tests/testsuite/lints.rs index ac0df8f5501..fa658033702 100644 --- a/tests/testsuite/lints.rs +++ b/tests/testsuite/lints.rs @@ -434,6 +434,42 @@ pub fn foo(num: i32) -> u32 { .run(); } +#[cargo_test] +fn profile_rustflags_doesnt_have_precedence() { + let foo = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["lints", "profile-rustflags"] + + [package] + name = "foo" + version = "0.0.1" + + [lints.rust] + "unsafe_code" = "allow" + + [profile.dev] + rustflags = ["-D", "unsafe_code"] + "#, + ) + .file( + "src/lib.rs", + " +pub fn foo(num: i32) -> u32 { + unsafe { std::mem::transmute(num) } +} +", + ) + .build(); + + foo.cargo("check") + .arg("-v") + .masquerade_as_nightly_cargo(&["lints", "profile-rustflags"]) + .with_status(0) + .run(); +} + #[cargo_test] fn without_priority() { Package::new("reg-dep", "1.0.0").publish(); From 66a9116f034896939ec74c185154b9ea0ac13ae2 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 19 May 2023 15:11:42 -0500 Subject: [PATCH 13/19] fix(lints): Put lints after profile.rustflags This does involve a subtle change to `profile.rustflags` precedence but its nightly and most likely people won't notice it? The benefit is its in a location more like the rest of the rustflags. --- src/cargo/core/compiler/mod.rs | 7 +++---- tests/testsuite/lints.rs | 7 +++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index dc7a4ba336a..b1e7f79b3f2 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -1041,10 +1041,6 @@ fn build_base_args( cmd.arg("-C").arg(&format!("opt-level={}", opt_level)); } - if !rustflags.is_empty() { - cmd.args(&rustflags); - } - if *panic != PanicStrategy::Unwind { cmd.arg("-C").arg(format!("panic={}", panic)); } @@ -1080,6 +1076,9 @@ fn build_base_args( } cmd.args(unit.pkg.manifest().rustflags()); + if !rustflags.is_empty() { + cmd.args(&rustflags); + } if let Some(args) = cx.bcx.extra_args_for(unit) { cmd.args(args); } diff --git a/tests/testsuite/lints.rs b/tests/testsuite/lints.rs index fa658033702..da590ea03c0 100644 --- a/tests/testsuite/lints.rs +++ b/tests/testsuite/lints.rs @@ -435,7 +435,7 @@ pub fn foo(num: i32) -> u32 { } #[cargo_test] -fn profile_rustflags_doesnt_have_precedence() { +fn profile_rustflags_has_precedence() { let foo = project() .file( "Cargo.toml", @@ -447,10 +447,10 @@ fn profile_rustflags_doesnt_have_precedence() { version = "0.0.1" [lints.rust] - "unsafe_code" = "allow" + "unsafe_code" = "deny" [profile.dev] - rustflags = ["-D", "unsafe_code"] + rustflags = ["-A", "unsafe_code"] "#, ) .file( @@ -466,7 +466,6 @@ pub fn foo(num: i32) -> u32 { foo.cargo("check") .arg("-v") .masquerade_as_nightly_cargo(&["lints", "profile-rustflags"]) - .with_status(0) .run(); } From 94e1801177918196ba06105486cf091dc9ae83b2 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 22 May 2023 08:46:39 -0500 Subject: [PATCH 14/19] refactor(lints): Extract lints->rustflags function --- src/cargo/util/toml/mod.rs | 53 +++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index e67f3ca7897..3cabee8cea9 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -2326,33 +2326,7 @@ impl TomlManifest { .transpose()?; let lints = verify_lints(lints)?; let default = TomlLints::default(); - let mut rustflags = lints - .as_ref() - .unwrap_or(&default) - .iter() - .flat_map(|(tool, lints)| { - lints.iter().map(move |(name, config)| { - let flag = config.level().flag(); - let option = if tool == "rust" { - format!("{flag}={name}") - } else { - format!("{flag}={tool}::{name}") - }; - ( - config.priority(), - // Since the most common group will be `all`, put it last so people are more - // likely to notice that they need to use `priority`. - std::cmp::Reverse(name), - option, - ) - }) - }) - .collect::>(); - rustflags.sort(); - let rustflags = rustflags - .into_iter() - .map(|(_, _, option)| option) - .collect::>(); + let rustflags = lints_to_rustflags(lints.as_ref().unwrap_or(&default)); let mut target: BTreeMap = BTreeMap::new(); for (name, platform) in me.target.iter().flatten() { @@ -3009,6 +2983,31 @@ fn verify_lints(lints: Option) -> CargoResult> { Ok(Some(lints)) } +fn lints_to_rustflags(lints: &TomlLints) -> Vec { + let mut rustflags = lints + .iter() + .flat_map(|(tool, lints)| { + lints.iter().map(move |(name, config)| { + let flag = config.level().flag(); + let option = if tool == "rust" { + format!("{flag}={name}") + } else { + format!("{flag}={tool}::{name}") + }; + ( + config.priority(), + // Since the most common group will be `all`, put it last so people are more + // likely to notice that they need to use `priority`. + std::cmp::Reverse(name), + option, + ) + }) + }) + .collect::>(); + rustflags.sort(); + rustflags.into_iter().map(|(_, _, option)| option).collect() +} + fn unused_dep_keys( dep_name: &str, kind: &str, From 12bc06aaf2af75f26a6197c4070f41e2ef4ad847 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 22 May 2023 09:13:00 -0500 Subject: [PATCH 15/19] test(lints): Clarify why verbose is used --- tests/testsuite/lints.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/testsuite/lints.rs b/tests/testsuite/lints.rs index da590ea03c0..69e57a9c3f5 100644 --- a/tests/testsuite/lints.rs +++ b/tests/testsuite/lints.rs @@ -396,6 +396,7 @@ pub fn foo(num: i32) -> u32 { .build(); foo.cargo("check") + .arg("-v") // Show order of rustflags on failure .masquerade_as_nightly_cargo(&["lints"]) .run(); } @@ -428,7 +429,7 @@ pub fn foo(num: i32) -> u32 { .build(); foo.cargo("check") - .arg("-v") + .arg("-v") // Show order of rustflags on failure .env("RUSTFLAGS", "-Aunsafe_code") .masquerade_as_nightly_cargo(&["lints"]) .run(); @@ -464,7 +465,7 @@ pub fn foo(num: i32) -> u32 { .build(); foo.cargo("check") - .arg("-v") + .arg("-v") // Show order of rustflags on failure .masquerade_as_nightly_cargo(&["lints", "profile-rustflags"]) .run(); } From 3cba0c1b5202bbd7abde4efd6631b4d39c16ad65 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 22 May 2023 09:51:34 -0500 Subject: [PATCH 16/19] test(lints): Verify dependency behavior --- tests/testsuite/lints.rs | 46 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/tests/testsuite/lints.rs b/tests/testsuite/lints.rs index 69e57a9c3f5..a7775b053f0 100644 --- a/tests/testsuite/lints.rs +++ b/tests/testsuite/lints.rs @@ -69,6 +69,52 @@ switch to nightly channel you can add .run(); } +#[cargo_test] +fn dependency_warning_ignored() { + let foo = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + authors = [] + + [dependencies] + bar.path = "../bar" + "#, + ) + .file("src/lib.rs", "") + .build(); + + let _bar = project() + .at("bar") + .file( + "Cargo.toml", + r#" + [package] + name = "bar" + version = "0.0.1" + authors = [] + + [lints.rust] + unsafe_code = "forbid" + "#, + ) + .file("src/lib.rs", "") + .build(); + + foo.cargo("check") + .with_stderr( + "\ +[CHECKING] [..] +[CHECKING] [..] +[FINISHED] [..] +", + ) + .run(); +} + #[cargo_test] fn malformed_on_stable() { let foo = project() From be76a553589e41cb1a0e7d29e3a290d150cd5108 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 22 May 2023 10:06:57 -0500 Subject: [PATCH 17/19] test(lints): Verify precedence with build.rustflags --- tests/testsuite/lints.rs | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/testsuite/lints.rs b/tests/testsuite/lints.rs index a7775b053f0..d2fdf6def5f 100644 --- a/tests/testsuite/lints.rs +++ b/tests/testsuite/lints.rs @@ -516,6 +516,45 @@ pub fn foo(num: i32) -> u32 { .run(); } +#[cargo_test] +fn build_rustflags_has_precedence() { + let foo = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["lints", "profile-rustflags"] + + [package] + name = "foo" + version = "0.0.1" + + [lints.rust] + "unsafe_code" = "deny" + "#, + ) + .file( + ".cargo/config.toml", + r#" + [build] + rustflags = ["-A", "unsafe_code"] +"#, + ) + .file( + "src/lib.rs", + " +pub fn foo(num: i32) -> u32 { + unsafe { std::mem::transmute(num) } +} +", + ) + .build(); + + foo.cargo("check") + .arg("-v") // Show order of rustflags on failure + .masquerade_as_nightly_cargo(&["lints", "profile-rustflags"]) + .run(); +} + #[cargo_test] fn without_priority() { Package::new("reg-dep", "1.0.0").publish(); From e4b01367a16c5bbc20b565c570c1734d1615cf21 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 22 May 2023 10:14:35 -0500 Subject: [PATCH 18/19] test(lints): Simplify failure tests --- tests/testsuite/lints.rs | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-) diff --git a/tests/testsuite/lints.rs b/tests/testsuite/lints.rs index d2fdf6def5f..b2aa0eee7c7 100644 --- a/tests/testsuite/lints.rs +++ b/tests/testsuite/lints.rs @@ -345,18 +345,9 @@ pub fn foo(num: i32) -> u32 { foo.cargo("check") .masquerade_as_nightly_cargo(&["lints"]) .with_status(101) - .with_stderr( + .with_stderr_contains( "\ -[..] error: usage of an `unsafe` block -[..] -[..] -[..] -[..] -[..] -[..] -[..] -error: could not compile `foo` (lib) due to previous error ", ) .run(); @@ -395,18 +386,9 @@ pub fn foo(num: i32) -> u32 { foo.cargo("check") .masquerade_as_nightly_cargo(&["lints"]) .with_status(101) - .with_stderr( + .with_stderr_contains( "\ -[..] error: usage of an `unsafe` block -[..] -[..] -[..] -[..] -[..] -[..] -[..] -error: could not compile `foo` (lib) due to previous error ", ) .run(); From b435eda62f034fba182cfed4940f63ddec241c2d Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 22 May 2023 12:23:56 -0500 Subject: [PATCH 19/19] refactor(lints): Restrict new Manifest::rustflags to being lint_rustflags This will avoid people reusing it in the future and expanding its use without updating fingerprinting/metadata. --- src/cargo/core/compiler/fingerprint/mod.rs | 4 ++-- src/cargo/core/compiler/mod.rs | 4 ++-- src/cargo/core/manifest.rs | 12 ++++++------ 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/cargo/core/compiler/fingerprint/mod.rs b/src/cargo/core/compiler/fingerprint/mod.rs index a67c1eb3edc..a3523110bba 100644 --- a/src/cargo/core/compiler/fingerprint/mod.rs +++ b/src/cargo/core/compiler/fingerprint/mod.rs @@ -87,7 +87,7 @@ //! [^5]: Config settings that are not otherwise captured anywhere else. //! Currently, this is only `doc.extern-map`. //! -//! [^6]: Via [`Manifest::rustflags`][crate::core::Manifest::rustflags] +//! [^6]: Via [`Manifest::lint_rustflags`][crate::core::Manifest::lint_rustflags] //! //! When deciding what should go in the Metadata vs the Fingerprint, consider //! that some files (like dylibs) do not have a hash in their filename. Thus, @@ -1417,7 +1417,7 @@ fn calculate_normal(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult, unit: &Unit) -> CargoResult { add_error_format_and_color(cx, &mut rustdoc); add_allow_features(cx, &mut rustdoc); - rustdoc.args(unit.pkg.manifest().rustflags()); + rustdoc.args(unit.pkg.manifest().lint_rustflags()); if let Some(args) = cx.bcx.extra_args_for(unit) { rustdoc.args(args); } @@ -1075,7 +1075,7 @@ fn build_base_args( cmd.arg("-C").arg(format!("debuginfo={}", debuginfo)); } - cmd.args(unit.pkg.manifest().rustflags()); + cmd.args(unit.pkg.manifest().lint_rustflags()); if !rustflags.is_empty() { cmd.args(&rustflags); } diff --git a/src/cargo/core/manifest.rs b/src/cargo/core/manifest.rs index 62b6501a422..98498ead884 100644 --- a/src/cargo/core/manifest.rs +++ b/src/cargo/core/manifest.rs @@ -63,7 +63,7 @@ pub struct Manifest { default_run: Option, metabuild: Option>, resolve_behavior: Option, - rustflags: Vec, + lint_rustflags: Vec, } /// When parsing `Cargo.toml`, some warnings should silenced @@ -406,7 +406,7 @@ impl Manifest { original: Rc, metabuild: Option>, resolve_behavior: Option, - rustflags: Vec, + lint_rustflags: Vec, ) -> Manifest { Manifest { summary, @@ -432,7 +432,7 @@ impl Manifest { default_run, metabuild, resolve_behavior, - rustflags, + lint_rustflags, } } @@ -517,9 +517,9 @@ impl Manifest { self.resolve_behavior } - /// Package-wide RUSTFLAGS - pub fn rustflags(&self) -> &[String] { - self.rustflags.as_slice() + /// `RUSTFLAGS` from the `[lints]` table + pub fn lint_rustflags(&self) -> &[String] { + self.lint_rustflags.as_slice() } pub fn map_source(self, to_replace: SourceId, replace_with: SourceId) -> Manifest {