feat: Add a basic linting system #13621

Muscraft · 2024-03-22T02:50:41Z

This PR adds a basic linting system, the first step towards User control over cargo warnings. To demonstrate the system, a lint for #12826 is added. It supports controlling cargo lints via the [lints.cargo] table. Lints can be controlled either by a group or individually.

This is meant to lay down some fundamental infrastructure for the whole linting system and is not meant to be fully featured. Over time, features will be added that will help us reach a much more solid state.

rustbot · 2024-03-22T02:50:46Z

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

weihanglo

🎉 🎉 🎉

src/cargo/util/lints.rs

Cargo.toml

src/cargo/core/workspace.rs

src/cargo/util/lints.rs

tests/testsuite/features.rs

src/cargo/util/lints.rs

tests/testsuite/lints.rs

src/cargo/util/toml/mod.rs

tests/testsuite/lints.rs

weihanglo · 2024-03-22T14:27:18Z

src/cargo/util/lints.rs

+}
+
+impl Lint {
+    pub fn level(&self, lints: &TomlToolLints, edition: Edition) -> LintLevel {


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.

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

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

bors · 2024-03-22T18:15:01Z

☔ The latest upstream changes (presumably #13627) made this pull request unmergeable. Please resolve the merge conflicts.

weihanglo · 2024-03-22T21:55:00Z

src/cargo/util/lints.rs

+    });
+    // Add implicit features for optional dependencies if they weren't
+    // explicitly listed anywhere.
+    let explicitly_listed = user_defined_features.map_or(HashSet::new(), |f| {


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.

src/cargo/util/lints.rs

weihanglo · 2024-03-22T22:50:55Z

As this is for linting system, the only blocker is an unstable flag for [lints.cargo]. Let me know if you are willing to work on it in this PR or later.

weihanglo

Thanks! Now looks pretty great and we're ready to merge this 👍🏾

weihanglo · 2024-03-23T18:38:31Z

This public interface of the linting system ([lints.cargo] table) is behind a new unstable flag -Z cargo-lints, with only a warning if not on nightly (non-blocking flag). Since the Cargo team is aware of the linting system Scott has been working on, I think we could merge it as-is without an FCP. Let me know if something goes wrong then.

@bors r+

bors · 2024-03-23T18:38:32Z

📌 Commit 307c7f8 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2024-03-23T18:39:39Z

⌛ Testing commit 307c7f8 with merge 61855e7...

bors · 2024-03-23T19:26:28Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 61855e7 to master...

epage · 2024-03-25T18:27:44Z

src/cargo/util/lints.rs

+use std::path::Path;
+use toml_edit::ImDocument;
+
+fn get_span(document: &ImDocument<String>, path: &[&str], get_value: bool) -> Option<Range<usize>> {


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

epage · 2024-03-25T18:28:51Z

src/cargo/util/lints.rs

+use std::path::Path;
+use toml_edit::ImDocument;
+
+fn get_span(document: &ImDocument<String>, path: &[&str], get_value: bool) -> Option<Range<usize>> {


These needs documentation, especially around how arrays are handled

epage · 2024-03-25T18:29:10Z

src/cargo/util/lints.rs

+
+/// 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 {


ditto, this belongs at the end of the file

epage · 2024-03-25T18:29:37Z

src/cargo/util/lints.rs

+
+/// 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 {


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

Or should we try to switch to camino?

epage · 2024-03-25T18:30:22Z

src/cargo/util/lints.rs

+        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
+            }
+        }
+    }


Do we document anywhere the status of the warnings group?

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.

epage · 2024-03-25T18:31:14Z

src/cargo/util/lints.rs

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


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

epage · 2024-03-25T18:32:37Z

src/cargo/util/lints.rs

+        {
+            continue;
+        }
+        if lint_level == LintLevel::Forbid || lint_level == LintLevel::Deny {


Should we have an is_error function?

epage · 2024-03-25T18:33:32Z

src/cargo/util/lints.rs

+        }
+        let level = lint_level.to_diagnostic_level();
+        let manifest_path = rel_cwd_manifest_path(path, gctx);
+        let message = level.title("unused optional dependency").snippet(


This message doesn't make sense pre-2024

epage · 2024-03-25T18:34:08Z

tests/testsuite/lints/implicit_features/edition_2021/mod.rs

@@ -0,0 +1,34 @@
+use cargo_test_support::prelude::*;


None of our other UI tests nest this deeply

epage · 2024-03-25T18:35:56Z

src/cargo/util/lints.rs

+    name: "rust-2024-compatibility",
+    default_level: LintLevel::Allow,
+    desc: "warn about compatibility with Rust 2024",
+    edition_lint_opts: Some((Edition::Edition2024, LintLevel::Deny)),


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.

epage · 2024-03-25T18:37:08Z

src/cargo/util/lints.rs

+}
+
+impl Lint {
+    pub fn level(&self, lints: &TomlToolLints, edition: Edition) -> LintLevel {


We should verify this handles Forbid correctly.

epage · 2024-03-25T18:38:24Z

src/cargo/util/lints.rs

+        }
+        let level = lint_level.to_diagnostic_level();
+        let manifest_path = rel_cwd_manifest_path(path, gctx);
+        let message = level.title("unused optional dependency").snippet(


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

refactor: Make lint names snake_case When working on #13621, I somehow missed that lint names should be `snake_case` according to the [`rustc-dev-guide`](https://rustc-dev-guide.rust-lang.org/diagnostics.html#lint-naming) as well as [`RFC #344`](https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints). This PR renames: - `implicit-features` => `implicit_featires` - `rust-2024-compatibility` => `rust_2024_compatibility`. <hr> Note: We should probably have some tooling to enforce this, but I was unsure if it belonged to this PR or another one. One solution would be to use a macro to create the `const LINT_NAME: Lint = {...}`, where `LINT_NAME` would be the `ident` as well as the `name: &'static str` and then have a method on `Lint` to make it lowercase as needed. This is what `rustc` does, and it could work well here. It would ensure snake case as `const` names need to be [`SCREAMING_SNAKE_CASE`](https://rust-lang.github.io/rfcs/0430-finalizing-naming-conventions.html#general-naming-conventions), or a warning is shown.

epage · 2024-03-25T18:51:35Z

src/cargo/util/lints.rs

+        }
+        let level = lint_level.to_diagnostic_level();
+        let manifest_path = rel_cwd_manifest_path(path, gctx);
+        let message = level.title("unused optional dependency").snippet(


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

epage · 2024-03-25T19:51:59Z

src/cargo/core/workspace.rs

+            let path = path.join("Cargo.toml");
+            if let MaybePackage::Package(pkg) = maybe_pkg {
+                self.emit_lints(pkg, &path)?
+            }


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

Update cargo 13 commits in d438c80c45c24be676ef5867edc79d0a14910efe..a510712d05c6c98f987af24dd73cdfafee8922e6 2024-03-19 16:11:22 +0000 to 2024-03-25 03:45:54 +0000 - Remove unnecessary test (rust-lang/cargo#13637) - Use `gitoxide` for `list_files_git` (rust-lang/cargo#13592) - fix: Warn on -Zlints (rust-lang/cargo#13632) - feat: Add a basic linting system (rust-lang/cargo#13621) - docs: remove untrue TODO for `native_dirs` (rust-lang/cargo#13631) - refactor(testsuite): Rename lints to lints_table (rust-lang/cargo#13627) - Fix debuginfo strip when using `--target` (rust-lang/cargo#13618) - refactor(toml): Push diagnostic complexity on annotate-snippets (rust-lang/cargo#13619) - Fix publish script due to crates.io CDN change (rust-lang/cargo#13614) - fix(alias): dont panic when resolving an empty alias (rust-lang/cargo#13613) - Update annotate snippets (rust-lang/cargo#13609) - refactor(vendor): tiny not important refactors (rust-lang/cargo#13610) - feat: Report some dependency changes on any command (rust-lang/cargo#13561) r? ghost

epage · 2024-03-25T21:21:42Z

src/cargo/util/toml/mod.rs

+        if tool == "cargo" && !gctx.cli_unstable().cargo_lints {
+            warn_for_cargo_lint_feature(gctx, warnings);
+        }


Do we need to strip lints.cargo before we publish?

Update cargo 13 commits in d438c80c45c24be676ef5867edc79d0a14910efe..a510712d05c6c98f987af24dd73cdfafee8922e6 2024-03-19 16:11:22 +0000 to 2024-03-25 03:45:54 +0000 - Remove unnecessary test (rust-lang/cargo#13637) - Use `gitoxide` for `list_files_git` (rust-lang/cargo#13592) - fix: Warn on -Zlints (rust-lang/cargo#13632) - feat: Add a basic linting system (rust-lang/cargo#13621) - docs: remove untrue TODO for `native_dirs` (rust-lang/cargo#13631) - refactor(testsuite): Rename lints to lints_table (rust-lang/cargo#13627) - Fix debuginfo strip when using `--target` (rust-lang/cargo#13618) - refactor(toml): Push diagnostic complexity on annotate-snippets (rust-lang/cargo#13619) - Fix publish script due to crates.io CDN change (rust-lang/cargo#13614) - fix(alias): dont panic when resolving an empty alias (rust-lang/cargo#13613) - Update annotate snippets (rust-lang/cargo#13609) - refactor(vendor): tiny not important refactors (rust-lang/cargo#13610) - feat: Report some dependency changes on any command (rust-lang/cargo#13561) r? ghost

ehuss · 2024-03-26T19:19:29Z

Would it be possible to update unstable.md with documentation about this feature?

Also, is #12235 the main tracking issue for what needs to be done next? Is there an outline of what steps need to be taken?

Unused dependencies cleanup The implementation of #12826 was used as a demonstration for #12235, in #13621. The demonstration implementation was far from ideal and was lacking a few features. This PR makes the implementation "feature complete", and fixes known bugs with the initial implementation.

test(cargo-lints): Add a test to ensure cap-lints works When implementing the linting system, [it was noted that there was no test to ensure this it is to cap-lints](#13621 (comment)), this PR adds that missing test.

Refactor cargo lint tests In #13621, it was brought up that [the lints tests are nested more deeply than other UI tests](#13621 (comment)). This got me wondering if there was a better way to structure all the lint tests. What I came up with was: - Lints should not have UI tests, only parts of the diagnostic system, i.e., how warnings, errors, notes, etc., look - This is because UI tests should focus on parts of the system that make up each lint's output - We can always add UI tests for each lint if desired - All tests related to the linting system should live in `tests/testsuite/lints/` - Tests related to `[lints.cargo]` should stay in `lints_table.rs` as it is for the whole lints table - Each lint will get a file in `lints/` for all of its tests - This makes `lints/mod.rs` smaller and targeted only at tests for the linting system itself - It makes it much easier to know where to place a test

rustbot assigned weihanglo Mar 22, 2024

rustbot added A-manifest Area: Cargo.toml issues A-workspaces Area: workspaces S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 22, 2024

weihanglo reviewed Mar 22, 2024

View reviewed changes

Muscraft force-pushed the linting-system branch from 998524f to df44151 Compare March 22, 2024 19:32

weihanglo mentioned this pull request Mar 22, 2024

Next step for deprecations in Cargo #13629

Open

13 tasks

weihanglo reviewed Mar 22, 2024

View reviewed changes

Muscraft added 2 commits March 23, 2024 10:24

feat: Add -Zcargo-lints

abf0953

feat: Add a basic linting system

307c7f8

Muscraft force-pushed the linting-system branch from df44151 to 307c7f8 Compare March 23, 2024 16:31

rustbot added the A-unstable Area: nightly unstable support label Mar 23, 2024

weihanglo approved these changes Mar 23, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 23, 2024

bors merged commit 61855e7 into rust-lang:master Mar 23, 2024
23 checks passed

bors mentioned this pull request Mar 23, 2024

Stabilize -Zcheck-cfg as always enabled #13571

Merged

Muscraft deleted the linting-system branch March 23, 2024 19:30

bors mentioned this pull request Mar 23, 2024

fix: Warn on -Zlints #13632

Merged

Muscraft mentioned this pull request Mar 24, 2024

fix: Make lint names snake_case #13635

Merged

weihanglo mentioned this pull request Mar 25, 2024

Update cargo rust-lang/rust#123040

Merged

epage reviewed Mar 25, 2024

View reviewed changes

epage mentioned this pull request Mar 25, 2024

User control over cargo warnings #12235

Open

6 tasks

epage reviewed Mar 25, 2024

View reviewed changes

rustbot added this to the 1.79.0 milestone Mar 25, 2024

ehuss mentioned this pull request Mar 26, 2024

Tracking Issue for remove-implicit-feature RFC 3491 #12826

Open

weihanglo mentioned this pull request Mar 29, 2024

Make .cargo/config deprecation warnings silent #13667

Closed

weihanglo mentioned this pull request Apr 8, 2024

Add warning for unused workspace dependency #13723

Open

Muscraft mentioned this pull request Apr 18, 2024

Unused dependencies cleanup #13778

Merged

Muscraft mentioned this pull request Apr 29, 2024

test(cargo-lints): Add a test to ensure cap-lints works #13829

Merged

Muscraft mentioned this pull request May 8, 2024

Refactor cargo lint tests #13880

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add a basic linting system #13621

feat: Add a basic linting system #13621

Muscraft commented Mar 22, 2024

rustbot commented Mar 22, 2024

weihanglo left a comment

weihanglo Mar 22, 2024

Muscraft Mar 22, 2024

epage Mar 25, 2024

bors commented Mar 22, 2024

weihanglo Mar 22, 2024

weihanglo commented Mar 22, 2024

weihanglo left a comment

weihanglo commented Mar 23, 2024 •

edited

Loading

bors commented Mar 23, 2024

bors commented Mar 23, 2024

bors commented Mar 23, 2024

epage Mar 25, 2024

epage Mar 25, 2024

epage Mar 25, 2024

epage Mar 25, 2024

epage Mar 25, 2024

epage Mar 25, 2024

epage Mar 25, 2024

epage Mar 25, 2024

epage Mar 25, 2024

epage Mar 25, 2024

epage Mar 25, 2024

epage Mar 25, 2024

epage Mar 25, 2024

epage Mar 25, 2024

epage Mar 25, 2024

epage Mar 25, 2024

ehuss commented Mar 26, 2024

feat: Add a basic linting system #13621

feat: Add a basic linting system #13621

Conversation

Muscraft commented Mar 22, 2024

rustbot commented Mar 22, 2024

weihanglo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Mar 22, 2024

Choose a reason for hiding this comment

weihanglo commented Mar 22, 2024

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented Mar 23, 2024 • edited Loading

bors commented Mar 23, 2024

bors commented Mar 23, 2024

bors commented Mar 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss commented Mar 26, 2024

weihanglo commented Mar 23, 2024 •

edited

Loading