rust
/
cargo
mirror of https://github.com/rust-lang/cargo
0
0
Fork 0
Code Issues Releases Wiki Activity

Auto merge of #13852 - Muscraft:revert-seperating-lints, r=epage 

fix(lints): Prevent inheritance from bring exposed for published packages

#13843 demonstrated a regression caused by #13801, where we started to keep `[lints]` and `[workspace.lints]` separate, and not truly resolve `[lints]`. This was a nice thing to have and made it easier to tell when a lint came from a workspace. The downside of doing so is the lints table would not get resolved when vendoring or publishing.

To fix this issue, I reverted the change for keeping `[lints]` and `[workspace.lints]` separate and modified how cargo's linting system figures out where a lint is coming from. Due to this change, we no longer specify that a lint was set by `[workspace.lints]`, only `[lints]`. It is true that a lint level is set by `[lints]` always, as it would've had to specify the lint outright or specify that it was inheriting it, seeing that, I do not think this is a regression in diagnostic quality. I still manage to keep the ability to render a lint's location in the workspace's manifest when running ` analyze_cargo_lints_table`, which I am pleased about.
This commit is contained in:
bors 2024-05-03 00:49:36 +00:00
parent 97181c67e1 6c2334613c
commit cf7b3c4cf3
4 changed files with 122 additions and 213 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

61
src/cargo/core/workspace.rs
View File

@ -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"

238
src/cargo/util/lints.rs
View File

@ -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<String>,
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();
.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]` "
));
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"),
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<String>,
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,
)
.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();
} 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),
)
.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),
),
)
}
level
.title(&title)
.snippet(
Snippet::source(ws_contents)
.origin(&ws_path)
.annotation(Level::Error.span(lint_span))
.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(());
}

34
src/cargo/util/toml/mod.rs
View File

@ -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

2
tests/testsuite/lints_table.rs
View File

@ -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();