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

feat: Add a basic linting system #13621

Merged
merged 2 commits into from
Mar 23, 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
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"] }
weihanglo marked this conversation as resolved.
Show resolved Hide resolved
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
2 changes: 2 additions & 0 deletions src/cargo/core/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -750,6 +750,7 @@ unstable_cli_options!(
#[serde(deserialize_with = "deserialize_build_std")]
build_std: Option<Vec<String>> = ("Enable Cargo to compile the standard library itself as part of a crate graph compilation"),
build_std_features: Option<Vec<String>> = ("Configure features enabled for the standard library itself when building the standard library"),
cargo_lints: bool = ("Enable the `[lints.cargo]` table"),
check_cfg: bool = ("Enable compile-time checking of `cfg` names/values/features"),
codegen_backend: bool = ("Enable the `codegen-backend` option in profiles in .cargo/config.toml file"),
config_include: bool = ("Enable the `include` key in config files"),
Expand Down Expand Up @@ -1117,6 +1118,7 @@ impl CliUnstable {
self.build_std = Some(crate::core::compiler::standard_lib::parse_unstable_flag(v))
}
"build-std-features" => self.build_std_features = Some(parse_features(v)),
"cargo-lints" => self.cargo_lints = parse_empty(k, v)?,
"check-cfg" => {
self.check_cfg = parse_empty(k, v)?;
}
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)?
}
Comment on lines +1100 to +1103
Copy link
Contributor

Choose a reason for hiding this comment

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

We need a test to ensure this is subject to cap-lints

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)?;
weihanglo marked this conversation as resolved.
Show resolved Hide resolved
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
weihanglo marked this conversation as resolved.
Show resolved Hide resolved
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>> {
weihanglo marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: please put the focus of the file (what comes first) on the API and not the implementation details

Copy link
Contributor

Choose a reason for hiding this comment

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

These needs documentation, especially around how arrays are handled

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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto, this belongs at the end of the file

Copy link
Contributor

Choose a reason for hiding this comment

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

String in this is suspect. Should this have display in the name?

Or should we try to switch to camino?

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)),
Copy link
Contributor

Choose a reason for hiding this comment

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

We're only making this Deny for 2024? That means people can override this to allow it. If they do, we should make sure that the dependency truly is unused and doesn't create a feature.

};

#[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 {
Copy link
Member

Choose a reason for hiding this comment

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

One thing worth thinking hard is that how to avoid future build failure if people set something like -D cargo::all and we introduce new cargo lints.

A possible way is having every new lint capped max to warning in the current edition, and relax in the next.

Copy link
Member Author

Choose a reason for hiding this comment

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

One way to potentially do this is to have each lint have a version field similar to Feature, that is the version they are stabilized in. From there, we add a "dynamic" group cargo:new-lints that all lints stabilized in that version are apart of. We then add documentation saying if a user doesn't want to break CI with -D cargo::all, they should have the command be:

cargo <cmd> -- -W cargo:new-lints -D cargo:all

I don't think this is perfect but it would work

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think rustc or clippy maintain compatibility with all

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 verify this handles Forbid correctly.

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
}
}
}
Comment on lines +87 to +106
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we document anywhere the status of the warnings group?

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: even if we don't support the group in [lints], we'd need to be able to support it in the future with #8424.

}

#[derive(Copy, Clone, Debug, PartialEq)]
pub enum LintLevel {
Muscraft marked this conversation as resolved.
Show resolved Hide resolved
Allow,
Warn,
Deny,
Forbid,
}

impl LintLevel {
pub fn to_diagnostic_level(self) -> Level {
match self {
LintLevel::Allow => Level::Note,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is Allow translating to Note. We shouldn't be showing Allow

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 {
weihanglo marked this conversation as resolved.
Show resolved Hide resolved
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(
Copy link
Contributor

Choose a reason for hiding this comment

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

imo its a bit messy to shove lint implementations in here just because they are lints. This body should live where its called.

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| {
Copy link
Member

Choose a reason for hiding this comment

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

We might want some more test cases to exercising implicit features lint.

  • has dep:foo
  • has foo = [] feature
  • build.dependencies
  • target platform dependencies

Since this PR aims for the linting system. Let's leave it later.

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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have an is_error function?

*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(
Copy link
Contributor

Choose a reason for hiding this comment

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

This message doesn't make sense pre-2024

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have a transitional note that implicit features are no longer supported?

Copy link
Contributor

Choose a reason for hiding this comment

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

rustc/clippy lints display the lint name (and where the level was set) on first instance of it being reported

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
Loading