Skip to content

Commit

Permalink
feat(cargo-lints): Add a lint analysis step
Browse files Browse the repository at this point in the history
  • Loading branch information
Muscraft committed Apr 30, 2024
1 parent 433cf68 commit 6abc8a2
Show file tree
Hide file tree
Showing 4 changed files with 252 additions and 13 deletions.
4 changes: 4 additions & 0 deletions src/cargo/core/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,10 @@ macro_rules! features {
fn is_enabled(&self, features: &Features) -> bool {
(self.get)(features)
}

pub(crate) fn name(&self) -> &str {
self.name
}
}

impl Features {
Expand Down
24 changes: 23 additions & 1 deletion src/cargo/core/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ use crate::sources::{PathSource, CRATES_IO_INDEX, CRATES_IO_REGISTRY};
use crate::util::edit_distance;
use crate::util::errors::{CargoResult, ManifestError};
use crate::util::interning::InternedString;
use crate::util::lints::{check_im_a_teapot, check_implicit_features, unused_dependencies};
use crate::util::lints::{
analyze_cargo_lints_table, check_im_a_teapot, check_implicit_features, unused_dependencies,
};
use crate::util::toml::{read_manifest, InheritableFields};
use crate::util::{
context::CargoResolverConfig, context::CargoResolverPrecedence, context::ConfigRelativePath,
Expand Down Expand Up @@ -1227,6 +1229,26 @@ impl<'gctx> Workspace<'gctx> {
.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(),
};

let ws_document = match self.root_maybe() {
MaybePackage::Package(pkg) => pkg.manifest().document(),
MaybePackage::Virtual(v) => v.document(),
};

analyze_cargo_lints_table(
pkg,
&path,
&normalized_lints,
ws_cargo_lints,
ws_contents,
ws_document,
self.root_manifest(),
self.gctx,
)?;
check_im_a_teapot(
pkg,
&path,
Expand Down
172 changes: 162 additions & 10 deletions src/cargo/util/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,157 @@ use std::ops::Range;
use std::path::Path;
use toml_edit::ImDocument;

const LINTS: &[Lint] = &[IM_A_TEAPOT, IMPLICIT_FEATURES, UNUSED_OPTIONAL_DEPENDENCY];

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,
gctx: &GlobalContext,
) -> CargoResult<()> {
let mut error_count = 0;
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
.keys()
.chain(ws_lints.map(|l| l.keys()).unwrap_or_default())
{
if let Some(lint) = LINTS.iter().find(|l| l.name == lint_name) {
let (_, reason) = lint.level(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) = lint.feature_gate {
verify_feature_enabled(
lint.name,
feature_gate,
reason,
manifest,
&manifest_path,
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",
))
} else {
Ok(())
}
}

fn verify_feature_enabled(
lint_name: &str,
feature_gate: &Feature,
reason: LintLevelReason,
manifest: &Manifest,
manifest_path: &str,
ws_contents: &str,
ws_document: &ImDocument<String>,
ws_path: &str,
error_count: &mut usize,
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 label = format!(
"this is behind `{}`, which is not enabled",
dash_feature_name
);
let second_title = format!("`cargo::{}` was inherited", dash_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();

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

*error_count += 1;
gctx.shell().print_message(message)?;
}
Ok(())
}

fn get_span(document: &ImDocument<String>, path: &[&str], get_value: bool) -> Option<Range<usize>> {
let mut table = document.as_item().as_table_like()?;
let mut iter = path.into_iter().peekable();
Expand Down Expand Up @@ -122,12 +273,6 @@ impl Lint {
.map(|(_, (l, r, _))| (l, r))
.unwrap()
}

/// Returns true if the lint is feature gated and the feature is not enabled
fn is_feature_gated(&self, manifest: &Manifest) -> bool {
self.feature_gate
.map_or(false, |f| !manifest.unstable_features().is_enabled(f))
}
}

#[derive(Copy, Clone, Debug, PartialEq)]
Expand Down Expand Up @@ -190,6 +335,17 @@ impl Display for LintLevelReason {
}
}

impl LintLevelReason {
fn is_user_specified(&self) -> bool {
match self {
LintLevelReason::Default => false,
LintLevelReason::Edition(_) => false,
LintLevelReason::Package => true,
LintLevelReason::Workspace => true,
}
}
}

fn level_priority(
name: &str,
default_level: LintLevel,
Expand Down Expand Up @@ -249,10 +405,6 @@ pub fn check_im_a_teapot(
let manifest = pkg.manifest();
let (lint_level, reason) = IM_A_TEAPOT.level(pkg_lints, ws_lints, manifest.edition());

if IM_A_TEAPOT.is_feature_gated(manifest) {
return Ok(());
}

if lint_level == LintLevel::Allow {
return Ok(());
}
Expand Down
65 changes: 63 additions & 2 deletions tests/testsuite/lints_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1100,10 +1100,71 @@ im-a-teapot = "warn"

p.cargo("check -Zcargo-lints")
.masquerade_as_nightly_cargo(&["cargo-lints"])
.with_status(101)
.with_stderr(
"\
[CHECKING] foo v0.0.1 ([CWD])
[FINISHED] [..]
error: use of unstable lint `im-a-teapot`
--> Cargo.toml:9:1
|
9 | im-a-teapot = \"warn\"
| ^^^^^^^^^^^ this is behind `test-dummy-unstable`, which is not enabled
|
= help: consider adding `cargo-features = [\"test-dummy-unstable\"]` to the top of the manifest
error: encountered 1 errors(s) while verifying lints
",
)
.run();
}

#[cargo_test]
fn check_feature_gated_workspace() {
let p = project()
.file(
"Cargo.toml",
r#"
[workspace]
members = ["foo"]
[workspace.lints.cargo]
im-a-teapot = { level = "warn", priority = 10 }
test-dummy-unstable = { level = "forbid", priority = -1 }
"#,
)
.file(
"foo/Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
edition = "2015"
authors = []
[lints]
workspace = true
"#,
)
.file("foo/src/lib.rs", "")
.build();

p.cargo("check -Zcargo-lints")
.masquerade_as_nightly_cargo(&["cargo-lints"])
.with_status(101)
.with_stderr(
"\
error: use of unstable lint `im-a-teapot`
--> Cargo.toml:6:1
|
6 | im-a-teapot = { level = \"warn\", priority = 10 }
| ^^^^^^^^^^^ this is behind `test-dummy-unstable`, which is not enabled
|
note: `cargo::im-a-teapot` was inherited
--> foo/Cargo.toml:9:1
|
9 | workspace = true
| ----------------
|
= help: consider adding `cargo-features = [\"test-dummy-unstable\"]` to the top of the manifest
error: encountered 1 errors(s) while verifying lints
",
)
.run();
Expand Down

0 comments on commit 6abc8a2

Please sign in to comment.