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

fix(lints): Remove ability to specify - in lint name #13837

Merged
merged 2 commits into from
May 1, 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
13 changes: 4 additions & 9 deletions src/cargo/core/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1161,7 +1161,6 @@ impl<'gctx> Workspace<'gctx> {
.cloned()
.unwrap_or_default()
.into_iter()
.map(|(k, v)| (k.replace('-', "_"), v))
.collect();

for (path, maybe_pkg) in &self.packages.packages {
Expand Down Expand Up @@ -1214,10 +1213,6 @@ impl<'gctx> Workspace<'gctx> {
.get("cargo")
.cloned()
.unwrap_or(manifest::TomlToolLints::default());
let normalized_lints = cargo_lints
.into_iter()
.map(|(name, lint)| (name.replace('-', "_"), lint))
.collect();

// We should only be using workspace lints if the `[lints]` table is
// present in the manifest, and `workspace` is set to `true`
Expand All @@ -1242,7 +1237,7 @@ impl<'gctx> Workspace<'gctx> {
analyze_cargo_lints_table(
pkg,
&path,
&normalized_lints,
&cargo_lints,
ws_cargo_lints,
ws_contents,
ws_document,
Expand All @@ -1252,23 +1247,23 @@ impl<'gctx> Workspace<'gctx> {
check_im_a_teapot(
pkg,
&path,
&normalized_lints,
&cargo_lints,
ws_cargo_lints,
&mut error_count,
self.gctx,
)?;
check_implicit_features(
pkg,
&path,
&normalized_lints,
&cargo_lints,
ws_cargo_lints,
&mut error_count,
self.gctx,
)?;
unused_dependencies(
pkg,
&path,
&normalized_lints,
&cargo_lints,
ws_cargo_lints,
&mut error_count,
self.gctx,
Expand Down
233 changes: 180 additions & 53 deletions src/cargo/util/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,45 +29,68 @@ pub fn analyze_cargo_lints_table(
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
let mut unknown_lints = Vec::new();
for (lint_name, specified_in) in pkg_lints
.keys()
.chain(ws_lints.map(|l| l.keys()).unwrap_or_default())
.map(|name| (name, SpecifiedIn::Package))
.chain(
ws_lints
.map(|l| l.keys())
.unwrap_or_default()
.map(|name| (name, SpecifiedIn::Workspace)),
)
{
if let Some((name, default_level, edition_lint_opts, feature_gate)) =
let Some((name, default_level, edition_lint_opts, feature_gate)) =
find_lint_or_group(lint_name)
{
let (_, reason, _) = level_priority(
name,
*default_level,
*edition_lint_opts,
pkg_lints,
ws_lints,
manifest.edition(),
);

// Only run analysis on user-specified lints
if !reason.is_user_specified() {
continue;
}
else {
unknown_lints.push((lint_name, specified_in));
continue;
};

// Only run this on lints that are gated by a feature
if let Some(feature_gate) = feature_gate {
verify_feature_enabled(
name,
feature_gate,
reason,
manifest,
&manifest_path,
ws_contents,
ws_document,
&ws_path,
&mut error_count,
gctx,
)?;
}
let (_, reason, _) = level_priority(
name,
*default_level,
*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) = feature_gate {
verify_feature_enabled(
name,
feature_gate,
reason,
manifest,
&manifest_path,
ws_contents,
ws_document,
&ws_path,
&mut error_count,
gctx,
)?;
}
}

output_unknown_lints(
unknown_lints,
manifest,
&manifest_path,
pkg_lints,
ws_lints,
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",
Expand Down Expand Up @@ -117,31 +140,21 @@ fn verify_feature_enabled(
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 title = format!("use of unstable lint `{}`", lint_name);
Copy link
Member

Choose a reason for hiding this comment

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

nit: implicit arg capture (should we force that in clippy?)

Suggested change
let title = format!("use of unstable lint `{}`", lint_name);
let title = format!("use of unstable lint `{lint_name}`");

Copy link
Contributor

Choose a reason for hiding this comment

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

If its something we bring up in code review but could be automated, then yes

with the caveat on how often we don't want it which I believe is pretty rare for this one.

let label = format!(
"this is behind `{}`, which is not enabled",
dash_feature_name
);
let second_title = format!("`cargo::{}` was inherited", dash_name);
let second_title = format!("`cargo::{}` was inherited", lint_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();
let span =
get_span(manifest.document(), &["lints", "cargo", lint_name], false).unwrap();

Level::Error
.title(&title)
Expand All @@ -155,15 +168,10 @@ fn verify_feature_enabled(
}
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();
Expand Down Expand Up @@ -395,6 +403,11 @@ impl LintLevelReason {
}
}

enum SpecifiedIn {
Package,
Workspace,
}

fn level_priority(
name: &str,
default_level: LintLevel,
Expand Down Expand Up @@ -588,6 +601,120 @@ pub fn check_implicit_features(
Ok(())
}

const UNKNOWN_LINTS: Lint = Lint {
Copy link
Member

Choose a reason for hiding this comment

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

You know. I'll request for more doc comments like what IMPLICIT_FEATURES has :)

name: "unknown_lints",
desc: "unknown lint",
groups: &[],
default_level: LintLevel::Warn,
edition_lint_opts: None,
feature_gate: None,
};

fn output_unknown_lints(
unknown_lints: Vec<(&String, SpecifiedIn)>,
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(),
);
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 {
if lint_level == LintLevel::Forbid || lint_level == LintLevel::Deny {
*error_count += 1;
}
let title = format!("{}: `{lint_name}`", UNKNOWN_LINTS.desc);
let second_title = format!("`cargo::{}` was inherited", lint_name);
let underscore_lint_name = lint_name.replace("-", "_");
let matching = if let Some(lint) = LINTS.iter().find(|l| l.name == underscore_lint_name) {
Some((lint.name, "lint"))
} else if let Some(group) = LINT_GROUPS.iter().find(|g| g.name == underscore_lint_name) {
Some((group.name, "group"))
} else {
None
Copy link
Member

Choose a reason for hiding this comment

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

Besides dash-replaced, should we also leverage util::edit_distance::closest to find one more possible suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should. I saw that as a future improvement. The minimum we needed in this case was to help with dropping support for -.

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

level.title(&title).snippet(
Snippet::source(manifest.contents())
.origin(&manifest_path)
.annotation(Level::Error.span(span))
.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();

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),
),
)
}
};

if emitted_source.is_none() {
emitted_source = Some(format!(
"`cargo::{}` is set to `{lint_level}` {reason}",
UNKNOWN_LINTS.name
));
message = message.footer(Level::Note.title(emitted_source.as_ref().unwrap()));
}

if let Some(help) = help.as_ref() {
message = message.footer(Level::Help.title(help));
}

gctx.shell().print_message(message)?;
}

Ok(())
}

const UNUSED_OPTIONAL_DEPENDENCY: Lint = Lint {
name: "unused_optional_dependency",
desc: "unused optional dependency",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ baz = { version = "0.1.0", optional = true }
target-dep = { version = "0.1.0", optional = true }

[lints.cargo]
implicit-features = "warn"
implicit_features = "warn"
"#,
)
.file("src/lib.rs", "")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ baz = { version = "0.1.0", optional = true }
baz = ["dep:baz"]

[lints.cargo]
unused-optional-dependency = "allow"
unused_optional_dependency = "allow"
"#,
)
.file("src/lib.rs", "")
Expand Down
1 change: 1 addition & 0 deletions tests/testsuite/lints/mod.rs
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
mod implicit_features;
mod unknown_lints;
mod unused_optional_dependencies;
33 changes: 33 additions & 0 deletions tests/testsuite/lints/unknown_lints/default/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
use cargo_test_support::prelude::*;
use cargo_test_support::str;
use cargo_test_support::{file, project};

#[cargo_test]
fn case() {
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
edition = "2015"
authors = []

[lints.cargo]
this-lint-does-not-exist = "warn"
"#,
)
.file("src/lib.rs", "")
.build();

snapbox::cmd::Command::cargo_ui()
.masquerade_as_nightly_cargo(&["cargo-lints"])
.current_dir(p.root())
.arg("check")
.arg("-Zcargo-lints")
.assert()
.success()
.stdout_matches(str![""])
.stderr_matches(file!["stderr.term.svg"]);
}
Loading