From 2df02f07d80b420754944d2c53fbcca30861836a Mon Sep 17 00:00:00 2001 From: Scott Schafer Date: Thu, 25 Apr 2024 15:50:26 -0600 Subject: [PATCH 1/3] fix(lints): Feature-gate the im_a_teapot lint --- src/cargo/core/features.rs | 7 ++--- src/cargo/util/lints.rs | 48 ++++++++++++++++++++++++++-------- tests/testsuite/lints_table.rs | 30 +++++++++++++++++++++ 3 files changed, 71 insertions(+), 14 deletions(-) diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index 7013cc2a458..a185dde0b00 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -343,7 +343,7 @@ impl FromStr for Edition { } } -#[derive(PartialEq)] +#[derive(Debug, PartialEq)] enum Status { Stable, Unstable, @@ -387,11 +387,11 @@ macro_rules! features { $( $(#[$attr])* #[doc = concat!("\n\n\nSee .")] - pub fn $feature() -> &'static Feature { + pub const fn $feature() -> &'static Feature { fn get(features: &Features) -> bool { stab!($stab) == Status::Stable || features.$feature } - static FEAT: Feature = Feature { + const FEAT: Feature = Feature { name: stringify!($feature), stability: stab!($stab), version: $version, @@ -512,6 +512,7 @@ features! { } /// Status and metadata for a single unstable feature. +#[derive(Debug)] pub struct Feature { /// Feature name. This is valid Rust identifier so no dash only underscore. name: &'static str, diff --git a/src/cargo/util/lints.rs b/src/cargo/util/lints.rs index 839d29b0180..0356c2754f5 100644 --- a/src/cargo/util/lints.rs +++ b/src/cargo/util/lints.rs @@ -1,6 +1,6 @@ use crate::core::dependency::DepKind; use crate::core::FeatureValue::Dep; -use crate::core::{Edition, FeatureValue, Package}; +use crate::core::{Edition, Feature, FeatureValue, Features, Package}; use crate::util::interning::InternedString; use crate::{CargoResult, GlobalContext}; use annotate_snippets::{Level, Snippet}; @@ -13,10 +13,10 @@ use std::path::Path; use toml_edit::ImDocument; fn get_span(document: &ImDocument, path: &[&str], get_value: bool) -> Option> { - let mut table = document.as_item().as_table_like().unwrap(); + let mut table = document.as_item().as_table_like()?; let mut iter = path.into_iter().peekable(); while let Some(key) = iter.next() { - let (key, item) = table.get_key_value(key).unwrap(); + let (key, item) = table.get_key_value(key)?; if iter.peek().is_none() { return if get_value { item.span() @@ -82,6 +82,7 @@ pub struct Lint { pub groups: &'static [LintGroup], pub default_level: LintLevel, pub edition_lint_opts: Option<(Edition, LintLevel)>, + pub feature_gate: Option<&'static Feature>, } impl Lint { @@ -90,7 +91,17 @@ impl Lint { pkg_lints: &TomlToolLints, ws_lints: Option<&TomlToolLints>, edition: Edition, + unstable_features: &Features, ) -> (LintLevel, LintLevelReason) { + // We should return `Allow` if a lint is behind a feature, but it is + // not enabled, that way the lint does not run. + if self + .feature_gate + .is_some_and(|f| !unstable_features.is_enabled(f)) + { + return (LintLevel::Allow, LintLevelReason::Default); + } + self.groups .iter() .map(|g| { @@ -164,7 +175,7 @@ impl From for LintLevel { } } -#[derive(Copy, Clone, Debug)] +#[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum LintLevelReason { Default, Edition(Edition), @@ -228,6 +239,7 @@ const IM_A_TEAPOT: Lint = Lint { groups: &[TEST_DUMMY_UNSTABLE], default_level: LintLevel::Allow, edition_lint_opts: None, + feature_gate: Some(Feature::test_dummy_unstable()), }; pub fn check_im_a_teapot( @@ -239,7 +251,13 @@ pub fn check_im_a_teapot( gctx: &GlobalContext, ) -> CargoResult<()> { let manifest = pkg.manifest(); - let (lint_level, reason) = IM_A_TEAPOT.level(pkg_lints, ws_lints, manifest.edition()); + let (lint_level, reason) = IM_A_TEAPOT.level( + pkg_lints, + ws_lints, + manifest.edition(), + manifest.unstable_features(), + ); + if lint_level == LintLevel::Allow { return Ok(()); } @@ -295,6 +313,7 @@ const IMPLICIT_FEATURES: Lint = Lint { groups: &[], default_level: LintLevel::Allow, edition_lint_opts: None, + feature_gate: None, }; pub fn check_implicit_features( @@ -305,19 +324,20 @@ pub fn check_implicit_features( error_count: &mut usize, gctx: &GlobalContext, ) -> CargoResult<()> { - let edition = pkg.manifest().edition(); + let manifest = pkg.manifest(); + let edition = manifest.edition(); // In Edition 2024+, instead of creating optional features, the dependencies are unused. // See `UNUSED_OPTIONAL_DEPENDENCY` if edition >= Edition::Edition2024 { return Ok(()); } - let (lint_level, reason) = IMPLICIT_FEATURES.level(pkg_lints, ws_lints, edition); + let (lint_level, reason) = + IMPLICIT_FEATURES.level(pkg_lints, ws_lints, edition, manifest.unstable_features()); if lint_level == LintLevel::Allow { return Ok(()); } - let manifest = pkg.manifest(); let activated_opt_deps = manifest .resolved_toml() .features() @@ -373,6 +393,7 @@ const UNUSED_OPTIONAL_DEPENDENCY: Lint = Lint { groups: &[], default_level: LintLevel::Warn, edition_lint_opts: None, + feature_gate: None, }; pub fn unused_dependencies( @@ -383,18 +404,23 @@ pub fn unused_dependencies( error_count: &mut usize, gctx: &GlobalContext, ) -> CargoResult<()> { - let edition = pkg.manifest().edition(); + let manifest = pkg.manifest(); + let edition = manifest.edition(); // Unused optional dependencies can only exist on edition 2024+ if edition < Edition::Edition2024 { return Ok(()); } - let (lint_level, reason) = UNUSED_OPTIONAL_DEPENDENCY.level(pkg_lints, ws_lints, edition); + let (lint_level, reason) = UNUSED_OPTIONAL_DEPENDENCY.level( + pkg_lints, + ws_lints, + edition, + manifest.unstable_features(), + ); if lint_level == LintLevel::Allow { return Ok(()); } let mut emitted_source = None; - let manifest = pkg.manifest(); let original_toml = manifest.original_toml(); // Unused dependencies were stripped from the manifest, leaving only the used ones let used_dependencies = manifest diff --git a/tests/testsuite/lints_table.rs b/tests/testsuite/lints_table.rs index 6b756903130..c0ca87e41aa 100644 --- a/tests/testsuite/lints_table.rs +++ b/tests/testsuite/lints_table.rs @@ -1078,3 +1078,33 @@ implicit-features = "warn" ) .run(); } + +#[cargo_test] +fn check_feature_gated() { + let p = project() + .file( + "Cargo.toml", + r#" +[package] +name = "foo" +version = "0.0.1" +edition = "2015" +authors = [] + +[lints.cargo] +im-a-teapot = "warn" + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("check -Zcargo-lints") + .masquerade_as_nightly_cargo(&["cargo-lints"]) + .with_stderr( + "\ +[CHECKING] foo v0.0.1 ([CWD]) +[FINISHED] [..] +", + ) + .run(); +} From f00752729166220ed396d9fdcf0d0b1213cbca1c Mon Sep 17 00:00:00 2001 From: Scott Schafer Date: Fri, 26 Apr 2024 15:17:18 -0600 Subject: [PATCH 2/3] fix(lints): Mark the im_a_teapot lint as unstable --- src/cargo/core/features.rs | 4 + src/cargo/core/workspace.rs | 24 ++++- src/cargo/util/lints.rs | 171 ++++++++++++++++++++++++++++++++- tests/testsuite/lints_table.rs | 65 ++++++++++++- 4 files changed, 260 insertions(+), 4 deletions(-) diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index a185dde0b00..cdcb76956a9 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -406,6 +406,10 @@ macro_rules! features { fn is_enabled(&self, features: &Features) -> bool { (self.get)(features) } + + pub(crate) fn name(&self) -> &str { + self.name + } } impl Features { diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 7fb0b1ca5c0..bc49b74d687 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -24,7 +24,9 @@ use crate::sources::{PathSource, CRATES_IO_INDEX, CRATES_IO_REGISTRY}; use crate::util::edit_distance; use crate::util::errors::{CargoResult, ManifestError}; use crate::util::interning::InternedString; -use crate::util::lints::{check_im_a_teapot, check_implicit_features, unused_dependencies}; +use crate::util::lints::{ + analyze_cargo_lints_table, check_im_a_teapot, check_implicit_features, unused_dependencies, +}; use crate::util::toml::{read_manifest, InheritableFields}; use crate::util::{ context::CargoResolverConfig, context::CargoResolverPrecedence, context::ConfigRelativePath, @@ -1227,6 +1229,26 @@ impl<'gctx> Workspace<'gctx> { .is_some_and(|l| l.workspace) .then(|| ws_cargo_lints); + let ws_contents = match self.root_maybe() { + MaybePackage::Package(pkg) => pkg.manifest().contents(), + MaybePackage::Virtual(v) => v.contents(), + }; + + let ws_document = match self.root_maybe() { + MaybePackage::Package(pkg) => pkg.manifest().document(), + MaybePackage::Virtual(v) => v.document(), + }; + + analyze_cargo_lints_table( + pkg, + &path, + &normalized_lints, + ws_cargo_lints, + ws_contents, + ws_document, + self.root_manifest(), + self.gctx, + )?; check_im_a_teapot( pkg, &path, diff --git a/src/cargo/util/lints.rs b/src/cargo/util/lints.rs index 0356c2754f5..ee97a6d651e 100644 --- a/src/cargo/util/lints.rs +++ b/src/cargo/util/lints.rs @@ -1,6 +1,6 @@ use crate::core::dependency::DepKind; use crate::core::FeatureValue::Dep; -use crate::core::{Edition, Feature, FeatureValue, Features, Package}; +use crate::core::{Edition, Feature, FeatureValue, Features, Manifest, Package}; use crate::util::interning::InternedString; use crate::{CargoResult, GlobalContext}; use annotate_snippets::{Level, Snippet}; @@ -12,6 +12,164 @@ use std::ops::Range; use std::path::Path; use toml_edit::ImDocument; +const LINTS: &[Lint] = &[IM_A_TEAPOT, IMPLICIT_FEATURES, UNUSED_OPTIONAL_DEPENDENCY]; + +pub fn analyze_cargo_lints_table( + pkg: &Package, + path: &Path, + pkg_lints: &TomlToolLints, + ws_lints: Option<&TomlToolLints>, + ws_contents: &str, + ws_document: &ImDocument, + ws_path: &Path, + gctx: &GlobalContext, +) -> CargoResult<()> { + let mut error_count = 0; + let manifest = pkg.manifest(); + let manifest_path = rel_cwd_manifest_path(path, gctx); + let ws_path = rel_cwd_manifest_path(ws_path, gctx); + + for lint_name in pkg_lints + .keys() + .chain(ws_lints.map(|l| l.keys()).unwrap_or_default()) + { + if let Some(lint) = LINTS.iter().find(|l| l.name == lint_name) { + let (_, reason, _) = level_priority( + lint.name, + lint.default_level, + lint.edition_lint_opts, + pkg_lints, + ws_lints, + manifest.edition(), + ); + + // Only run analysis on user-specified lints + if !reason.is_user_specified() { + continue; + } + + // Only run this on lints that are gated by a feature + if let Some(feature_gate) = lint.feature_gate { + verify_feature_enabled( + lint.name, + feature_gate, + reason, + manifest, + &manifest_path, + ws_contents, + ws_document, + &ws_path, + &mut error_count, + gctx, + )?; + } + } + } + if error_count > 0 { + Err(anyhow::anyhow!( + "encountered {error_count} errors(s) while verifying lints", + )) + } else { + Ok(()) + } +} + +fn verify_feature_enabled( + lint_name: &str, + feature_gate: &Feature, + reason: LintLevelReason, + manifest: &Manifest, + manifest_path: &str, + ws_contents: &str, + ws_document: &ImDocument, + ws_path: &str, + error_count: &mut usize, + gctx: &GlobalContext, +) -> CargoResult<()> { + if !manifest.unstable_features().is_enabled(feature_gate) { + let dash_name = lint_name.replace("_", "-"); + let dash_feature_name = feature_gate.name().replace("_", "-"); + let title = format!("use of unstable lint `{}`", dash_name); + let label = format!( + "this is behind `{}`, which is not enabled", + dash_feature_name + ); + let second_title = format!("`cargo::{}` was inherited", dash_name); + let help = format!( + "consider adding `cargo-features = [\"{}\"]` to the top of the manifest", + dash_feature_name + ); + let message = match reason { + LintLevelReason::Package => { + let span = get_span( + manifest.document(), + &["lints", "cargo", dash_name.as_str()], + false, + ) + .or(get_span( + manifest.document(), + &["lints", "cargo", lint_name], + false, + )) + .unwrap(); + + Level::Error + .title(&title) + .snippet( + Snippet::source(manifest.contents()) + .origin(&manifest_path) + .annotation(Level::Error.span(span).label(&label)) + .fold(true), + ) + .footer(Level::Help.title(&help)) + } + LintLevelReason::Workspace => { + let lint_span = get_span( + ws_document, + &["workspace", "lints", "cargo", dash_name.as_str()], + false, + ) + .or(get_span( + ws_document, + &["workspace", "lints", "cargo", lint_name], + false, + )) + .unwrap(); + let inherit_span_key = + get_span(manifest.document(), &["lints", "workspace"], false).unwrap(); + let inherit_span_value = + get_span(manifest.document(), &["lints", "workspace"], true).unwrap(); + + Level::Error + .title(&title) + .snippet( + Snippet::source(ws_contents) + .origin(&ws_path) + .annotation(Level::Error.span(lint_span).label(&label)) + .fold(true), + ) + .footer( + Level::Note.title(&second_title).snippet( + Snippet::source(manifest.contents()) + .origin(&manifest_path) + .annotation( + Level::Note + .span(inherit_span_key.start..inherit_span_value.end), + ) + .fold(true), + ), + ) + .footer(Level::Help.title(&help)) + } + _ => unreachable!("LintLevelReason should be one that is user specified"), + }; + + *error_count += 1; + gctx.shell().print_message(message)?; + } + Ok(()) +} + fn get_span(document: &ImDocument, path: &[&str], get_value: bool) -> Option> { let mut table = document.as_item().as_table_like()?; let mut iter = path.into_iter().peekable(); @@ -194,6 +352,17 @@ impl Display for LintLevelReason { } } +impl LintLevelReason { + fn is_user_specified(&self) -> bool { + match self { + LintLevelReason::Default => false, + LintLevelReason::Edition(_) => false, + LintLevelReason::Package => true, + LintLevelReason::Workspace => true, + } + } +} + fn level_priority( name: &str, default_level: LintLevel, diff --git a/tests/testsuite/lints_table.rs b/tests/testsuite/lints_table.rs index c0ca87e41aa..3ed6606bf3b 100644 --- a/tests/testsuite/lints_table.rs +++ b/tests/testsuite/lints_table.rs @@ -1100,10 +1100,71 @@ im-a-teapot = "warn" p.cargo("check -Zcargo-lints") .masquerade_as_nightly_cargo(&["cargo-lints"]) + .with_status(101) .with_stderr( "\ -[CHECKING] foo v0.0.1 ([CWD]) -[FINISHED] [..] +error: use of unstable lint `im-a-teapot` + --> Cargo.toml:9:1 + | +9 | im-a-teapot = \"warn\" + | ^^^^^^^^^^^ this is behind `test-dummy-unstable`, which is not enabled + | + = help: consider adding `cargo-features = [\"test-dummy-unstable\"]` to the top of the manifest +error: encountered 1 errors(s) while verifying lints +", + ) + .run(); +} + +#[cargo_test] +fn check_feature_gated_workspace() { + let p = project() + .file( + "Cargo.toml", + r#" +[workspace] +members = ["foo"] + +[workspace.lints.cargo] +im-a-teapot = { level = "warn", priority = 10 } +test-dummy-unstable = { level = "forbid", priority = -1 } + "#, + ) + .file( + "foo/Cargo.toml", + r#" +[package] +name = "foo" +version = "0.0.1" +edition = "2015" +authors = [] + +[lints] +workspace = true + "#, + ) + .file("foo/src/lib.rs", "") + .build(); + + p.cargo("check -Zcargo-lints") + .masquerade_as_nightly_cargo(&["cargo-lints"]) + .with_status(101) + .with_stderr( + "\ +error: use of unstable lint `im-a-teapot` + --> Cargo.toml:6:1 + | +6 | im-a-teapot = { level = \"warn\", priority = 10 } + | ^^^^^^^^^^^ this is behind `test-dummy-unstable`, which is not enabled + | +note: `cargo::im-a-teapot` was inherited + --> foo/Cargo.toml:9:1 + | +9 | workspace = true + | ---------------- + | + = help: consider adding `cargo-features = [\"test-dummy-unstable\"]` to the top of the manifest +error: encountered 1 errors(s) while verifying lints ", ) .run(); From 712946c518938520784a349f2b69839aaa214714 Mon Sep 17 00:00:00 2001 From: Scott Schafer Date: Fri, 26 Apr 2024 15:45:17 -0600 Subject: [PATCH 3/3] fix(lints): Mark the test_dummy_unstable lint group as unstable --- src/cargo/util/lints.rs | 44 +++++++++++++++++++++++++++++----- tests/testsuite/lints_table.rs | 15 +++++++++++- 2 files changed, 52 insertions(+), 7 deletions(-) diff --git a/src/cargo/util/lints.rs b/src/cargo/util/lints.rs index ee97a6d651e..5f09249342a 100644 --- a/src/cargo/util/lints.rs +++ b/src/cargo/util/lints.rs @@ -12,6 +12,7 @@ use std::ops::Range; use std::path::Path; use toml_edit::ImDocument; +const LINT_GROUPS: &[LintGroup] = &[TEST_DUMMY_UNSTABLE]; const LINTS: &[Lint] = &[IM_A_TEAPOT, IMPLICIT_FEATURES, UNUSED_OPTIONAL_DEPENDENCY]; pub fn analyze_cargo_lints_table( @@ -33,11 +34,13 @@ pub fn analyze_cargo_lints_table( .keys() .chain(ws_lints.map(|l| l.keys()).unwrap_or_default()) { - if let Some(lint) = LINTS.iter().find(|l| l.name == lint_name) { + if let Some((name, default_level, edition_lint_opts, feature_gate)) = + find_lint_or_group(lint_name) + { let (_, reason, _) = level_priority( - lint.name, - lint.default_level, - lint.edition_lint_opts, + name, + *default_level, + *edition_lint_opts, pkg_lints, ws_lints, manifest.edition(), @@ -49,9 +52,9 @@ pub fn analyze_cargo_lints_table( } // Only run this on lints that are gated by a feature - if let Some(feature_gate) = lint.feature_gate { + if let Some(feature_gate) = feature_gate { verify_feature_enabled( - lint.name, + name, feature_gate, reason, manifest, @@ -74,6 +77,33 @@ pub fn analyze_cargo_lints_table( } } +fn find_lint_or_group<'a>( + name: &str, +) -> Option<( + &'static str, + &LintLevel, + &Option<(Edition, LintLevel)>, + &Option<&'static Feature>, +)> { + if let Some(lint) = LINTS.iter().find(|l| l.name == name) { + Some(( + lint.name, + &lint.default_level, + &lint.edition_lint_opts, + &lint.feature_gate, + )) + } else if let Some(group) = LINT_GROUPS.iter().find(|g| g.name == name) { + Some(( + group.name, + &group.default_level, + &group.edition_lint_opts, + &group.feature_gate, + )) + } else { + None + } +} + fn verify_feature_enabled( lint_name: &str, feature_gate: &Feature, @@ -224,6 +254,7 @@ pub struct LintGroup { pub default_level: LintLevel, pub desc: &'static str, pub edition_lint_opts: Option<(Edition, LintLevel)>, + pub feature_gate: Option<&'static Feature>, } const TEST_DUMMY_UNSTABLE: LintGroup = LintGroup { @@ -231,6 +262,7 @@ const TEST_DUMMY_UNSTABLE: LintGroup = LintGroup { desc: "test_dummy_unstable is meant to only be used in tests", default_level: LintLevel::Allow, edition_lint_opts: None, + feature_gate: Some(Feature::test_dummy_unstable()), }; #[derive(Copy, Clone, Debug)] diff --git a/tests/testsuite/lints_table.rs b/tests/testsuite/lints_table.rs index 3ed6606bf3b..6faa7094424 100644 --- a/tests/testsuite/lints_table.rs +++ b/tests/testsuite/lints_table.rs @@ -1164,7 +1164,20 @@ note: `cargo::im-a-teapot` was inherited | ---------------- | = help: consider adding `cargo-features = [\"test-dummy-unstable\"]` to the top of the manifest -error: encountered 1 errors(s) while verifying lints +error: use of unstable lint `test-dummy-unstable` + --> Cargo.toml:7:1 + | +7 | test-dummy-unstable = { level = \"forbid\", priority = -1 } + | ^^^^^^^^^^^^^^^^^^^ this is behind `test-dummy-unstable`, which is not enabled + | +note: `cargo::test-dummy-unstable` was inherited + --> foo/Cargo.toml:9:1 + | +9 | workspace = true + | ---------------- + | + = help: consider adding `cargo-features = [\"test-dummy-unstable\"]` to the top of the manifest +error: encountered 2 errors(s) while verifying lints ", ) .run();