From 6c2334613c31e0dac1d42f9bb22f545ba42f6c9f Mon Sep 17 00:00:00 2001 From: Scott Schafer Date: Thu, 2 May 2024 15:37:53 -0600 Subject: [PATCH] fix(lints): Prevent inheritance from bring exposed for published packages --- src/cargo/core/workspace.rs | 61 +-------- src/cargo/util/lints.rs | 242 +++++++++++++-------------------- src/cargo/util/toml/mod.rs | 34 +++-- tests/testsuite/lints_table.rs | 2 +- 4 files changed, 124 insertions(+), 215 deletions(-) diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index e7bbb6cab88..015bee2341f 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -1149,25 +1149,11 @@ impl<'gctx> Workspace<'gctx> { } pub fn emit_warnings(&self) -> CargoResult<()> { - let ws_lints = self - .root_maybe() - .workspace_config() - .inheritable() - .and_then(|i| i.lints().ok()) - .unwrap_or_default(); - - let ws_cargo_lints = ws_lints - .get("cargo") - .cloned() - .unwrap_or_default() - .into_iter() - .collect(); - for (path, maybe_pkg) in &self.packages.packages { let path = path.join("Cargo.toml"); if let MaybePackage::Package(pkg) = maybe_pkg { if self.gctx.cli_unstable().cargo_lints { - self.emit_lints(pkg, &path, &ws_cargo_lints)? + self.emit_lints(pkg, &path)? } } let warnings = match maybe_pkg { @@ -1195,12 +1181,7 @@ impl<'gctx> Workspace<'gctx> { Ok(()) } - pub fn emit_lints( - &self, - pkg: &Package, - path: &Path, - ws_cargo_lints: &manifest::TomlToolLints, - ) -> CargoResult<()> { + pub fn emit_lints(&self, pkg: &Package, path: &Path) -> CargoResult<()> { let mut error_count = 0; let toml_lints = pkg .manifest() @@ -1214,16 +1195,6 @@ impl<'gctx> Workspace<'gctx> { .cloned() .unwrap_or(manifest::TomlToolLints::default()); - // We should only be using workspace lints if the `[lints]` table is - // present in the manifest, and `workspace` is set to `true` - let ws_cargo_lints = pkg - .manifest() - .resolved_toml() - .lints - .as_ref() - .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(), @@ -1238,36 +1209,14 @@ impl<'gctx> Workspace<'gctx> { pkg, &path, &cargo_lints, - ws_cargo_lints, ws_contents, ws_document, self.root_manifest(), self.gctx, )?; - check_im_a_teapot( - pkg, - &path, - &cargo_lints, - ws_cargo_lints, - &mut error_count, - self.gctx, - )?; - check_implicit_features( - pkg, - &path, - &cargo_lints, - ws_cargo_lints, - &mut error_count, - self.gctx, - )?; - unused_dependencies( - pkg, - &path, - &cargo_lints, - ws_cargo_lints, - &mut error_count, - self.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)?; if error_count > 0 { Err(crate::util::errors::AlreadyPrintedError::new(anyhow!( "encountered {error_count} errors(s) while running lints" diff --git a/src/cargo/util/lints.rs b/src/cargo/util/lints.rs index 1d47872f440..bd08846a51d 100644 --- a/src/cargo/util/lints.rs +++ b/src/cargo/util/lints.rs @@ -19,7 +19,6 @@ 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, @@ -30,20 +29,11 @@ pub fn analyze_cargo_lints_table( let manifest_path = rel_cwd_manifest_path(path, gctx); let ws_path = rel_cwd_manifest_path(ws_path, gctx); let mut unknown_lints = Vec::new(); - for (lint_name, specified_in) in pkg_lints - .keys() - .map(|name| (name, SpecifiedIn::Package)) - .chain( - ws_lints - .map(|l| l.keys()) - .unwrap_or_default() - .map(|name| (name, SpecifiedIn::Workspace)), - ) - { + for lint_name in pkg_lints.keys().map(|name| name) { let Some((name, default_level, edition_lint_opts, feature_gate)) = find_lint_or_group(lint_name) else { - unknown_lints.push((lint_name, specified_in)); + unknown_lints.push(lint_name); continue; }; @@ -52,7 +42,6 @@ pub fn analyze_cargo_lints_table( *default_level, *edition_lint_opts, pkg_lints, - ws_lints, manifest.edition(), ); @@ -66,7 +55,6 @@ pub fn analyze_cargo_lints_table( verify_feature_enabled( name, feature_gate, - reason, manifest, &manifest_path, ws_contents, @@ -83,7 +71,6 @@ pub fn analyze_cargo_lints_table( manifest, &manifest_path, pkg_lints, - ws_lints, ws_contents, ws_document, &ws_path, @@ -130,7 +117,6 @@ fn find_lint_or_group<'a>( fn verify_feature_enabled( lint_name: &str, feature_gate: &Feature, - reason: LintLevelReason, manifest: &Manifest, manifest_path: &str, ws_contents: &str, @@ -151,55 +137,55 @@ fn verify_feature_enabled( "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", 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", lint_name], - false, + + let message = if let Some(span) = + get_span(manifest.document(), &["lints", "cargo", lint_name], false) + { + Level::Error + .title(&title) + .snippet( + Snippet::source(manifest.contents()) + .origin(&manifest_path) + .annotation(Level::Error.span(span).label(&label)) + .fold(true), ) - .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"), + .footer(Level::Help.title(&help)) + } else { + let lint_span = get_span( + ws_document, + &["workspace", "lints", "cargo", lint_name], + false, + ) + .expect(&format!( + "could not find `cargo::{lint_name}` in `[lints]`, or `[workspace.lints]` " + )); + + let inherited_note = if let (Some(inherit_span_key), Some(inherit_span_value)) = ( + get_span(manifest.document(), &["lints", "workspace"], false), + get_span(manifest.document(), &["lints", "workspace"], true), + ) { + 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), + ) + } else { + Level::Note.title(&second_title) + }; + + Level::Error + .title(&title) + .snippet( + Snippet::source(ws_contents) + .origin(&ws_path) + .annotation(Level::Error.span(lint_span).label(&label)) + .fold(true), + ) + .footer(inherited_note) + .footer(Level::Help.title(&help)) }; *error_count += 1; @@ -287,7 +273,6 @@ impl Lint { pub fn level( &self, pkg_lints: &TomlToolLints, - ws_lints: Option<&TomlToolLints>, edition: Edition, unstable_features: &Features, ) -> (LintLevel, LintLevelReason) { @@ -310,7 +295,6 @@ impl Lint { g.default_level, g.edition_lint_opts, pkg_lints, - ws_lints, edition, ), ) @@ -322,7 +306,6 @@ impl Lint { self.default_level, self.edition_lint_opts, pkg_lints, - ws_lints, edition, ), ))) @@ -378,7 +361,6 @@ pub enum LintLevelReason { Default, Edition(Edition), Package, - Workspace, } impl Display for LintLevelReason { @@ -387,7 +369,6 @@ impl Display for LintLevelReason { LintLevelReason::Default => write!(f, "by default"), LintLevelReason::Edition(edition) => write!(f, "in edition {}", edition), LintLevelReason::Package => write!(f, "in `[lints]`"), - LintLevelReason::Workspace => write!(f, "in `[workspace.lints]`"), } } } @@ -398,22 +379,15 @@ impl LintLevelReason { LintLevelReason::Default => false, LintLevelReason::Edition(_) => false, LintLevelReason::Package => true, - LintLevelReason::Workspace => true, } } } -enum SpecifiedIn { - Package, - Workspace, -} - fn level_priority( name: &str, default_level: LintLevel, edition_lint_opts: Option<(Edition, LintLevel)>, pkg_lints: &TomlToolLints, - ws_lints: Option<&TomlToolLints>, edition: Edition, ) -> (LintLevel, LintLevelReason, i8) { let (unspecified_level, reason) = if let Some(level) = edition_lint_opts @@ -436,12 +410,6 @@ fn level_priority( LintLevelReason::Package, defined_level.priority(), ) - } else if let Some(defined_level) = ws_lints.and_then(|l| l.get(name)) { - ( - defined_level.level().into(), - LintLevelReason::Workspace, - defined_level.priority(), - ) } else { (unspecified_level, reason, 0) } @@ -460,17 +428,12 @@ pub fn check_im_a_teapot( pkg: &Package, path: &Path, pkg_lints: &TomlToolLints, - ws_lints: Option<&TomlToolLints>, error_count: &mut usize, gctx: &GlobalContext, ) -> CargoResult<()> { let manifest = pkg.manifest(); - let (lint_level, reason) = IM_A_TEAPOT.level( - pkg_lints, - ws_lints, - manifest.edition(), - manifest.unstable_features(), - ); + let (lint_level, reason) = + IM_A_TEAPOT.level(pkg_lints, manifest.edition(), manifest.unstable_features()); if lint_level == LintLevel::Allow { return Ok(()); @@ -534,7 +497,6 @@ pub fn check_implicit_features( pkg: &Package, path: &Path, pkg_lints: &TomlToolLints, - ws_lints: Option<&TomlToolLints>, error_count: &mut usize, gctx: &GlobalContext, ) -> CargoResult<()> { @@ -547,7 +509,7 @@ pub fn check_implicit_features( } let (lint_level, reason) = - IMPLICIT_FEATURES.level(pkg_lints, ws_lints, edition, manifest.unstable_features()); + IMPLICIT_FEATURES.level(pkg_lints, edition, manifest.unstable_features()); if lint_level == LintLevel::Allow { return Ok(()); } @@ -611,30 +573,25 @@ const UNKNOWN_LINTS: Lint = Lint { }; fn output_unknown_lints( - unknown_lints: Vec<(&String, SpecifiedIn)>, + unknown_lints: Vec<&String>, manifest: &Manifest, manifest_path: &str, pkg_lints: &TomlToolLints, - ws_lints: Option<&TomlToolLints>, ws_contents: &str, ws_document: &ImDocument, ws_path: &str, error_count: &mut usize, gctx: &GlobalContext, ) -> CargoResult<()> { - let (lint_level, reason) = UNKNOWN_LINTS.level( - pkg_lints, - ws_lints, - manifest.edition(), - manifest.unstable_features(), - ); + let (lint_level, reason) = + UNKNOWN_LINTS.level(pkg_lints, manifest.edition(), manifest.unstable_features()); if lint_level == LintLevel::Allow { return Ok(()); } let level = lint_level.to_diagnostic_level(); let mut emitted_source = None; - for (lint_name, specified_in) in unknown_lints { + for lint_name in unknown_lints { if lint_level == LintLevel::Forbid || lint_level == LintLevel::Deny { *error_count += 1; } @@ -651,50 +608,50 @@ fn output_unknown_lints( let help = matching.map(|(name, kind)| format!("there is a {kind} with a similar name: `{name}`")); - let mut message = match specified_in { - SpecifiedIn::Package => { - let span = - get_span(manifest.document(), &["lints", "cargo", lint_name], false).unwrap(); + let mut message = if let Some(span) = + get_span(manifest.document(), &["lints", "cargo", lint_name], false) + { + level.title(&title).snippet( + Snippet::source(manifest.contents()) + .origin(&manifest_path) + .annotation(Level::Error.span(span)) + .fold(true), + ) + } else { + let lint_span = get_span( + ws_document, + &["workspace", "lints", "cargo", lint_name], + false, + ) + .expect(&format!( + "could not find `cargo::{lint_name}` in `[lints]`, or `[workspace.lints]` " + )); - level.title(&title).snippet( + let inherited_note = if let (Some(inherit_span_key), Some(inherit_span_value)) = ( + get_span(manifest.document(), &["lints", "workspace"], false), + get_span(manifest.document(), &["lints", "workspace"], true), + ) { + Level::Note.title(&second_title).snippet( Snippet::source(manifest.contents()) .origin(&manifest_path) - .annotation(Level::Error.span(span)) + .annotation( + Level::Note.span(inherit_span_key.start..inherit_span_value.end), + ) .fold(true), ) - } - SpecifiedIn::Workspace => { - let lint_span = get_span( - ws_document, - &["workspace", "lints", "cargo", lint_name], - false, + } else { + Level::Note.title(&second_title) + }; + + level + .title(&title) + .snippet( + Snippet::source(ws_contents) + .origin(&ws_path) + .annotation(Level::Error.span(lint_span)) + .fold(true), ) - .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 - .title(&title) - .snippet( - Snippet::source(ws_contents) - .origin(&ws_path) - .annotation(Level::Error.span(lint_span)) - .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(inherited_note) }; if emitted_source.is_none() { @@ -728,7 +685,6 @@ pub fn unused_dependencies( pkg: &Package, path: &Path, pkg_lints: &TomlToolLints, - ws_lints: Option<&TomlToolLints>, error_count: &mut usize, gctx: &GlobalContext, ) -> CargoResult<()> { @@ -739,12 +695,8 @@ pub fn unused_dependencies( return Ok(()); } - let (lint_level, reason) = UNUSED_OPTIONAL_DEPENDENCY.level( - pkg_lints, - ws_lints, - edition, - manifest.unstable_features(), - ); + let (lint_level, reason) = + UNUSED_OPTIONAL_DEPENDENCY.level(pkg_lints, edition, manifest.unstable_features()); if lint_level == LintLevel::Allow { return Ok(()); } diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 82c154bf218..813868ac3ec 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -492,7 +492,15 @@ fn resolve_toml( } resolved_toml.target = (!resolved_target.is_empty()).then_some(resolved_target); - resolved_toml.lints = original_toml.lints.clone(); + let resolved_lints = original_toml + .lints + .clone() + .map(|value| lints_inherit_with(value, || inherit()?.lints())) + .transpose()?; + resolved_toml.lints = resolved_lints.map(|lints| manifest::InheritableLints { + workspace: false, + lints, + }); resolved_toml.badges = original_toml.badges.clone(); } else { @@ -1336,18 +1344,18 @@ fn to_real_manifest( } } - let resolved_lints = resolved_toml - .lints - .clone() - .map(|value| { - lints_inherit_with(value, || { - load_inheritable_fields(gctx, manifest_file, &workspace_config)?.lints() - }) - }) - .transpose()?; - - verify_lints(resolved_lints.as_ref(), gctx, warnings)?; - let rustflags = lints_to_rustflags(&resolved_lints.unwrap_or_default()); + verify_lints( + resolved_toml.resolved_lints().expect("previously resolved"), + gctx, + warnings, + )?; + let default = manifest::TomlLints::default(); + let rustflags = lints_to_rustflags( + resolved_toml + .resolved_lints() + .expect("previously resolved") + .unwrap_or(&default), + ); let metadata = ManifestMetadata { description: resolved_package diff --git a/tests/testsuite/lints_table.rs b/tests/testsuite/lints_table.rs index 2a14da99088..6ebdda1a35b 100644 --- a/tests/testsuite/lints_table.rs +++ b/tests/testsuite/lints_table.rs @@ -975,7 +975,7 @@ error: `im_a_teapot` is specified 13 | im-a-teapot = true | ^^^^^^^^^^^^^^^^^^ | - = note: `cargo::im_a_teapot` is set to `forbid` in `[workspace.lints]` + = note: `cargo::im_a_teapot` is set to `forbid` in `[lints]` ", ) .run();