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 1 commit
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
208 changes: 175 additions & 33 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 @@ -380,6 +403,11 @@ impl LintLevelReason {
}
}

enum SpecifiedIn {
Package,
Workspace,
}

fn level_priority(
name: &str,
default_level: LintLevel,
Expand Down Expand Up @@ -573,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
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"]);
}
47 changes: 47 additions & 0 deletions tests/testsuite/lints/unknown_lints/default/stderr.term.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
43 changes: 43 additions & 0 deletions tests/testsuite/lints/unknown_lints/inherited/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
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#"
[workspace]
members = ["foo"]

[workspace.lints.cargo]
this-lint-does-not-exist = "warn"
"#,
)
.file(
"foo/Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
edition = "2015"
authors = []

[lints]
workspace = true
"#,
)
.file("foo/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