Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add where lint was set #13801

Merged
merged 4 commits into from
Apr 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 39 additions & 3 deletions src/cargo/core/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1180,6 +1180,21 @@ impl<'gctx> Workspace<'gctx> {
}

pub fn emit_lints(&self, pkg: &Package, path: &Path) -> 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()
.map(|(k, v)| (k.replace('-', "_"), v))
.collect();

let mut error_count = 0;
let toml_lints = pkg
.manifest()
Expand All @@ -1197,9 +1212,30 @@ impl<'gctx> Workspace<'gctx> {
.map(|(name, lint)| (name.replace('-', "_"), lint))
.collect();

check_im_a_teapot(pkg, &path, &normalized_lints, &mut error_count, self.gctx)?;
check_implicit_features(pkg, &path, &normalized_lints, &mut error_count, self.gctx)?;
unused_dependencies(pkg, &path, &normalized_lints, &mut error_count, self.gctx)?;
check_im_a_teapot(
pkg,
&path,
&normalized_lints,
&ws_cargo_lints,
&mut error_count,
self.gctx,
)?;
check_implicit_features(
pkg,
&path,
&normalized_lints,
&ws_cargo_lints,
&mut error_count,
self.gctx,
)?;
unused_dependencies(
pkg,
&path,
&normalized_lints,
&ws_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"
Expand Down
145 changes: 105 additions & 40 deletions src/cargo/util/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,40 +85,41 @@ pub struct Lint {
}

impl Lint {
pub fn level(&self, lints: &TomlToolLints, edition: Edition) -> LintLevel {
let edition_level = self
.edition_lint_opts
.filter(|(e, _)| edition >= *e)
.map(|(_, l)| l);

if self.default_level == LintLevel::Forbid || edition_level == Some(LintLevel::Forbid) {
return LintLevel::Forbid;
}

let level = self
.groups
pub fn level(
&self,
pkg_lints: &TomlToolLints,
ws_lints: &TomlToolLints,
edition: Edition,
) -> (LintLevel, LintLevelReason) {
self.groups
.iter()
.map(|g| g.name)
.chain(std::iter::once(self.name))
.filter_map(|n| lints.get(n).map(|l| (n, l)))
.max_by_key(|(n, l)| {
.map(|g| {
(
l.level() == TomlLintLevel::Forbid,
l.priority(),
std::cmp::Reverse(*n),
g.name,
level_priority(
g.name,
g.default_level,
g.edition_lint_opts,
pkg_lints,
ws_lints,
edition,
),
)
});

match level {
Some((_, toml_lint)) => toml_lint.level().into(),
None => {
if let Some(level) = edition_level {
level
} else {
self.default_level
}
}
}
})
.chain(std::iter::once((
self.name,
level_priority(
self.name,
self.default_level,
self.edition_lint_opts,
pkg_lints,
ws_lints,
edition,
),
)))
.max_by_key(|(n, (l, _, p))| (l == &LintLevel::Forbid, *p, std::cmp::Reverse(*n)))
.map(|(_, (l, r, _))| (l, r))
.unwrap()
}
}

Expand Down Expand Up @@ -163,6 +164,64 @@ impl From<TomlLintLevel> for LintLevel {
}
}

#[derive(Copy, Clone, Debug)]
pub enum LintLevelReason {
Default,
Edition(Edition),
Package,
Workspace,
}

impl Display for LintLevelReason {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
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]`"),
}
}
}

fn level_priority(
name: &str,
default_level: LintLevel,
edition_lint_opts: Option<(Edition, LintLevel)>,
pkg_lints: &TomlToolLints,
ws_lints: &TomlToolLints,
edition: Edition,
) -> (LintLevel, LintLevelReason, i8) {
let (unspecified_level, reason) = if let Some(level) = edition_lint_opts
.filter(|(e, _)| edition >= *e)
.map(|(_, l)| l)
{
(level, LintLevelReason::Edition(edition))
} else {
(default_level, LintLevelReason::Default)
};

// Don't allow the group to be overridden if the level is `Forbid`
if unspecified_level == LintLevel::Forbid {
return (unspecified_level, reason, 0);
}

if let Some(defined_level) = pkg_lints.get(name) {
(
defined_level.level().into(),
LintLevelReason::Package,
defined_level.priority(),
)
} else if let Some(defined_level) = ws_lints.get(name) {
(
defined_level.level().into(),
LintLevelReason::Workspace,
defined_level.priority(),
)
} else {
(unspecified_level, reason, 0)
}
}

const IM_A_TEAPOT: Lint = Lint {
name: "im_a_teapot",
desc: "`im_a_teapot` is specified",
Expand All @@ -174,12 +233,13 @@ const IM_A_TEAPOT: Lint = Lint {
pub fn check_im_a_teapot(
pkg: &Package,
path: &Path,
lints: &TomlToolLints,
pkg_lints: &TomlToolLints,
ws_lints: &TomlToolLints,
error_count: &mut usize,
gctx: &GlobalContext,
) -> CargoResult<()> {
let manifest = pkg.manifest();
let lint_level = IM_A_TEAPOT.level(lints, manifest.edition());
let (lint_level, reason) = IM_A_TEAPOT.level(pkg_lints, ws_lints, manifest.edition());
if lint_level == LintLevel::Allow {
return Ok(());
}
Expand All @@ -194,7 +254,10 @@ pub fn check_im_a_teapot(
}
let level = lint_level.to_diagnostic_level();
let manifest_path = rel_cwd_manifest_path(path, gctx);
let emitted_reason = format!("`cargo::{}` is set to `{lint_level}`", IM_A_TEAPOT.name);
let emitted_reason = format!(
"`cargo::{}` is set to `{lint_level}` {reason}",
IM_A_TEAPOT.name
);

let key_span = get_span(manifest.document(), &["package", "im-a-teapot"], false).unwrap();
let value_span = get_span(manifest.document(), &["package", "im-a-teapot"], true).unwrap();
Expand Down Expand Up @@ -242,7 +305,8 @@ const IMPLICIT_FEATURES: Lint = Lint {
pub fn check_implicit_features(
pkg: &Package,
path: &Path,
lints: &TomlToolLints,
pkg_lints: &TomlToolLints,
ws_lints: &TomlToolLints,
error_count: &mut usize,
gctx: &GlobalContext,
) -> CargoResult<()> {
Expand All @@ -253,7 +317,7 @@ pub fn check_implicit_features(
return Ok(());
}

let lint_level = IMPLICIT_FEATURES.level(lints, edition);
let (lint_level, reason) = IMPLICIT_FEATURES.level(pkg_lints, ws_lints, edition);
if lint_level == LintLevel::Allow {
return Ok(());
}
Expand Down Expand Up @@ -298,7 +362,7 @@ pub fn check_implicit_features(
);
if emitted_source.is_none() {
emitted_source = Some(format!(
"`cargo::{}` is set to `{lint_level}`",
"`cargo::{}` is set to `{lint_level}` {reason}",
IMPLICIT_FEATURES.name
));
message = message.footer(Level::Note.title(emitted_source.as_ref().unwrap()));
Expand All @@ -325,7 +389,8 @@ const UNUSED_OPTIONAL_DEPENDENCY: Lint = Lint {
pub fn unused_dependencies(
pkg: &Package,
path: &Path,
lints: &TomlToolLints,
pkg_lints: &TomlToolLints,
ws_lints: &TomlToolLints,
error_count: &mut usize,
gctx: &GlobalContext,
) -> CargoResult<()> {
Expand All @@ -335,7 +400,7 @@ pub fn unused_dependencies(
return Ok(());
}

let lint_level = UNUSED_OPTIONAL_DEPENDENCY.level(lints, edition);
let (lint_level, reason) = UNUSED_OPTIONAL_DEPENDENCY.level(pkg_lints, ws_lints, edition);
if lint_level == LintLevel::Allow {
return Ok(());
}
Expand Down Expand Up @@ -401,7 +466,7 @@ pub fn unused_dependencies(
);
if emitted_source.is_none() {
emitted_source = Some(format!(
"`cargo::{}` is set to `{lint_level}`",
"`cargo::{}` is set to `{lint_level}` {reason}",
UNUSED_OPTIONAL_DEPENDENCY.name
));
message =
Expand Down
36 changes: 14 additions & 22 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -411,15 +411,7 @@ fn resolve_toml(
}
resolved_toml.target = (!resolved_target.is_empty()).then_some(resolved_target);

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.lints = original_toml.lints.clone();

let resolved_badges = original_toml
.badges
Expand Down Expand Up @@ -803,7 +795,7 @@ impl InheritableFields {
}

/// Gets the field `workspace.lint`.
fn lints(&self) -> CargoResult<manifest::TomlLints> {
pub fn lints(&self) -> CargoResult<manifest::TomlLints> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo this shouldn't be in InheritableFields anymore because we are no longer treating it like normal inheritance.

Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is still "Inheritable", it is just not inherited in the normal way. I could go either way on where it should be

let Some(val) = &self.lints else {
bail!("`workspace.lints` was not defined");
};
Expand Down Expand Up @@ -1284,18 +1276,18 @@ fn to_real_manifest(
}
}

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

let metadata = ManifestMetadata {
description: resolved_package
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading