Skip to content

Commit

Permalink
feat: Add a basic linting system
Browse files Browse the repository at this point in the history
  • Loading branch information
Muscraft committed Mar 22, 2024
1 parent 7ff7e34 commit df44151
Show file tree
Hide file tree
Showing 25 changed files with 793 additions and 6 deletions.
4 changes: 2 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ tempfile = "3.10.1"
thiserror = "1.0.57"
time = { version = "0.3", features = ["parsing", "formatting", "serde"] }
toml = "0.8.10"
toml_edit = { version = "0.22.7", features = ["serde"] }
toml_edit = { version = "0.22.9", features = ["serde"] }
tracing = "0.1.40" # be compatible with rustc_log: https://github.com/rust-lang/rust/blob/e51e98dde6a/compiler/rustc_log/Cargo.toml#L9
tracing-chrome = "0.7.1"
tracing-subscriber = { version = "0.3.18", features = ["env-filter"] }
Expand Down
31 changes: 30 additions & 1 deletion src/cargo/core/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,12 @@ 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_implicit_features;
use crate::util::toml::{read_manifest, InheritableFields};
use crate::util::{context::ConfigRelativePath, Filesystem, GlobalContext, IntoUrl};
use cargo_util::paths;
use cargo_util::paths::normalize_path;
use cargo_util_schemas::manifest;
use cargo_util_schemas::manifest::RustVersion;
use cargo_util_schemas::manifest::{TomlDependency, TomlProfiles};
use pathdiff::diff_paths;
Expand Down Expand Up @@ -1095,11 +1097,14 @@ impl<'gctx> Workspace<'gctx> {

pub fn emit_warnings(&self) -> CargoResult<()> {
for (path, maybe_pkg) in &self.packages.packages {
let path = path.join("Cargo.toml");
if let MaybePackage::Package(pkg) = maybe_pkg {
self.emit_lints(pkg, &path)?
}
let warnings = match maybe_pkg {
MaybePackage::Package(pkg) => pkg.manifest().warnings().warnings(),
MaybePackage::Virtual(vm) => vm.warnings().warnings(),
};
let path = path.join("Cargo.toml");
for warning in warnings {
if warning.is_critical {
let err = anyhow::format_err!("{}", warning.message);
Expand All @@ -1121,6 +1126,30 @@ impl<'gctx> Workspace<'gctx> {
Ok(())
}

pub fn emit_lints(&self, pkg: &Package, path: &Path) -> CargoResult<()> {
let mut error_count = 0;
let lints = pkg
.manifest()
.resolved_toml()
.lints
.clone()
.map(|lints| lints.lints)
.unwrap_or(manifest::TomlLints::default())
.get("cargo")
.cloned()
.unwrap_or(manifest::TomlToolLints::default());

check_implicit_features(pkg, &path, &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"
))
.into())
} else {
Ok(())
}
}

pub fn set_target_dir(&mut self, target_dir: Filesystem) {
self.target_dir = Some(target_dir);
}
Expand Down
226 changes: 226 additions & 0 deletions src/cargo/util/lints.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,226 @@
use crate::core::FeatureValue::Dep;
use crate::core::{Edition, FeatureValue, Package};
use crate::util::interning::InternedString;
use crate::{CargoResult, GlobalContext};
use annotate_snippets::{Level, Renderer, Snippet};
use cargo_util_schemas::manifest::{TomlLintLevel, TomlToolLints};
use pathdiff::diff_paths;
use std::collections::HashSet;
use std::ops::Range;
use std::path::Path;
use toml_edit::ImDocument;

fn get_span(document: &ImDocument<String>, path: &[&str], get_value: bool) -> Option<Range<usize>> {
let mut table = document.as_item().as_table_like().unwrap();
let mut iter = path.into_iter().peekable();
while let Some(key) = iter.next() {
let (key, item) = table.get_key_value(key).unwrap();
if iter.peek().is_none() {
return if get_value {
item.span()
} else {
let leaf_decor = key.dotted_decor();
let leaf_prefix_span = leaf_decor.prefix().and_then(|p| p.span());
let leaf_suffix_span = leaf_decor.suffix().and_then(|s| s.span());
if let (Some(leaf_prefix_span), Some(leaf_suffix_span)) =
(leaf_prefix_span, leaf_suffix_span)
{
Some(leaf_prefix_span.start..leaf_suffix_span.end)
} else {
key.span()
}
};
}
if item.is_table_like() {
table = item.as_table_like().unwrap();
}
if item.is_array() && iter.peek().is_some() {
let array = item.as_array().unwrap();
let next = iter.next().unwrap();
return array.iter().find_map(|item| {
if next == &item.to_string() {
item.span()
} else {
None
}
});
}
}
None
}

/// Gets the relative path to a manifest from the current working directory, or
/// the absolute path of the manifest if a relative path cannot be constructed
fn rel_cwd_manifest_path(path: &Path, gctx: &GlobalContext) -> String {
diff_paths(path, gctx.cwd())
.unwrap_or_else(|| path.to_path_buf())
.display()
.to_string()
}

#[derive(Copy, Clone, Debug)]
pub struct LintGroup {
pub name: &'static str,
pub default_level: LintLevel,
pub desc: &'static str,
pub edition_lint_opts: Option<(Edition, LintLevel)>,
}

const RUST_2024_COMPATIBILITY: LintGroup = LintGroup {
name: "rust-2024-compatibility",
default_level: LintLevel::Allow,
desc: "warn about compatibility with Rust 2024",
edition_lint_opts: Some((Edition::Edition2024, LintLevel::Deny)),
};

#[derive(Copy, Clone, Debug)]
pub struct Lint {
pub name: &'static str,
pub desc: &'static str,
pub groups: &'static [LintGroup],
pub default_level: LintLevel,
pub edition_lint_opts: Option<(Edition, LintLevel)>,
}

impl Lint {
pub fn level(&self, lints: &TomlToolLints, edition: Edition) -> LintLevel {
let level = 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)| (l.priority(), std::cmp::Reverse(*n)));

match level {
Some((_, toml_lint)) => toml_lint.level().into(),
None => {
if let Some((lint_edition, lint_level)) = self.edition_lint_opts {
if edition >= lint_edition {
return lint_level;
}
}
self.default_level
}
}
}
}

#[derive(Copy, Clone, Debug, PartialEq)]
pub enum LintLevel {
Allow,
Warn,
Deny,
Forbid,
}

impl LintLevel {
pub fn to_diagnostic_level(self) -> Level {
match self {
LintLevel::Allow => Level::Note,
LintLevel::Warn => Level::Warning,
LintLevel::Deny => Level::Error,
LintLevel::Forbid => Level::Error,
}
}
}

impl From<TomlLintLevel> for LintLevel {
fn from(toml_lint_level: TomlLintLevel) -> LintLevel {
match toml_lint_level {
TomlLintLevel::Allow => LintLevel::Allow,
TomlLintLevel::Warn => LintLevel::Warn,
TomlLintLevel::Deny => LintLevel::Deny,
TomlLintLevel::Forbid => LintLevel::Forbid,
}
}
}

/// By default, cargo will treat any optional dependency as a [feature]. As of
/// cargo 1.60, these can be disabled by declaring a feature that activates the
/// optional dependency as `dep:<name>` (see [RFC #3143]).
///
/// In the 2024 edition, `cargo` will stop exposing optional dependencies as
/// features implicitly, requiring users to add `foo = ["dep:foo"]` if they
/// still want it exposed.
///
/// For more information, see [RFC #3491]
///
/// [feature]: https://doc.rust-lang.org/cargo/reference/features.html
/// [RFC #3143]: https://rust-lang.github.io/rfcs/3143-cargo-weak-namespaced-features.html
/// [RFC #3491]: https://rust-lang.github.io/rfcs/3491-remove-implicit-features.html
const IMPLICIT_FEATURES: Lint = Lint {
name: "implicit-features",
desc: "warn about the use of unstable features",
groups: &[RUST_2024_COMPATIBILITY],
default_level: LintLevel::Allow,
edition_lint_opts: Some((Edition::Edition2024, LintLevel::Deny)),
};

pub fn check_implicit_features(
pkg: &Package,
path: &Path,
lints: &TomlToolLints,
error_count: &mut usize,
gctx: &GlobalContext,
) -> CargoResult<()> {
let lint_level = IMPLICIT_FEATURES.level(lints, pkg.manifest().edition());
if lint_level == LintLevel::Allow {
return Ok(());
}

let manifest = pkg.manifest();
let user_defined_features = manifest.resolved_toml().features();
let features = user_defined_features.map_or(HashSet::new(), |f| {
f.keys().map(|k| InternedString::new(&k)).collect()
});
// Add implicit features for optional dependencies if they weren't
// explicitly listed anywhere.
let explicitly_listed = user_defined_features.map_or(HashSet::new(), |f| {
f.values()
.flatten()
.filter_map(|v| match FeatureValue::new(v.into()) {
Dep { dep_name } => Some(dep_name),
_ => None,
})
.collect()
});

for dep in manifest.dependencies() {
let dep_name_in_toml = dep.name_in_toml();
if !dep.is_optional()
|| features.contains(&dep_name_in_toml)
|| explicitly_listed.contains(&dep_name_in_toml)
{
continue;
}
if lint_level == LintLevel::Forbid || lint_level == LintLevel::Deny {
*error_count += 1;
}
let level = lint_level.to_diagnostic_level();
let manifest_path = rel_cwd_manifest_path(path, gctx);
let message = level.title("unused optional dependency").snippet(
Snippet::source(manifest.contents())
.origin(&manifest_path)
.annotation(
level.span(
get_span(
manifest.document(),
&["dependencies", &dep_name_in_toml],
false,
)
.unwrap(),
),
)
.fold(true),
);
let renderer = Renderer::styled().term_width(
gctx.shell()
.err_width()
.diagnostic_terminal_width()
.unwrap_or(annotate_snippets::renderer::DEFAULT_TERM_WIDTH),
);
writeln!(gctx.shell().err(), "{}", renderer.render(message))?;
}
Ok(())
}
1 change: 1 addition & 0 deletions src/cargo/util/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ pub mod into_url;
mod into_url_with_base;
mod io;
pub mod job;
pub mod lints;
mod lockserver;
pub mod machine_message;
pub mod network;
Expand Down
3 changes: 2 additions & 1 deletion src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1454,7 +1454,7 @@ fn verify_lints(lints: Option<&manifest::TomlLints>) -> CargoResult<()> {
};

for (tool, lints) in lints {
let supported = ["rust", "clippy", "rustdoc"];
let supported = ["rust", "clippy", "rustdoc", "cargo"];
if !supported.contains(&tool.as_str()) {
let supported = supported.join(", ");
anyhow::bail!("unsupported `{tool}` in `[lints]`, must be one of {supported}")
Expand Down Expand Up @@ -1482,6 +1482,7 @@ fn verify_lints(lints: Option<&manifest::TomlLints>) -> CargoResult<()> {
fn lints_to_rustflags(lints: &manifest::TomlLints) -> Vec<String> {
let mut rustflags = lints
.iter()
.filter(|(tool, _)| *tool != "cargo")
.flat_map(|(tool, lints)| {
lints.iter().map(move |(name, config)| {
let flag = match config.level() {
Expand Down
34 changes: 34 additions & 0 deletions tests/testsuite/lints/implicit_features/edition_2021/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
use cargo_test_support::prelude::*;
use cargo_test_support::project;
use cargo_test_support::registry::Package;
use cargo_test_support::str;

#[cargo_test]
fn case() {
Package::new("bar", "0.1.0").publish();
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.1.0"
edition = "2021"
[dependencies]
bar = { version = "0.1.0", optional = true }
"#,
)
.file("src/lib.rs", "")
.build();

snapbox::cmd::Command::cargo_ui()
.masquerade_as_nightly_cargo(&["always_nightly"])
.current_dir(p.root())
.arg("check")
.arg("--quiet")
.assert()
.success()
.stdout_matches(str![""])
.stderr_matches(str![""]);
}
Loading

0 comments on commit df44151

Please sign in to comment.