Rewrite config parsing for better span support #13084

Jarcho · 2024-07-11T03:29:15Z

changelog: Improved spans on config errors.
rust-lang/rust-clippy#13084

xFrednet

I really like these changes! One tiny nit, then you can r=me :D

clippy_config/src/conf.rs

xFrednet · 2024-07-13T23:32:58Z

Oh one more thing, do we maybe want to add a lint or warning, if the ".." is not in the last index? I feel like it could be confusing otherwise 🤔

Jarcho · 2024-07-13T23:45:06Z

I'm personally on the side that it should be first since this is extending an existing list. An ellipsis comes first when writing so the thing that's acting like one should also come first.

As for linting when it appears in the middle it might be worth it since it's otherwise silently ignored now.

xFrednet · 2024-07-14T09:02:15Z

I'm personally on the side that it should be first since this is extending an existing list. An ellipsis comes first when writing so the thing that's acting like one should also come first.

I think for a single value, the other thing looks a bit better as in

x = ["custom", ".."]

But with multiple values and potentially multiple lines, it's definitely better to have it in the front. So, that should be our default suggestion.

It's probably easier to make this a warning, and not a lint, since we can then just emit it while reading the config file. It'd be good if this warning/lint could be part of this PR, but we could make it a followup PR, if you prefer.

bors · 2024-07-17T19:04:54Z

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

Jarcho · 2024-07-18T07:15:28Z

Added a change to the metadata collector. Should this be changed back to printing [] for the default value of an empty array? msrv and trivial_copy_size_limit already didn't list a default value.

For the warning that requires changing how values are deserialized in order to get the span correct. I'll look into it.

xFrednet · 2024-07-18T07:22:27Z

Hmm, I like having the default value for everything for consistency. For booleans etc we also display the default, even if it's the default of those values. So, I'd be in favor of keeping it.

I like that the original order of the values in the default configuration is now retained.

Jarcho · 2024-07-27T20:20:36Z

Latest change is basically a full rewrite using toml_edit instead or serde and toml. This lets us deserialize values with spans and get better error reporting. A bunch of misc. changes are included as well at the moment.

I'll be splitting this up later into multiple commits for easier reading, but you can get a feel for the end result with this. The diff is going to be totally useless right now.

Misc changes to `clippy_config` Contains part of #13084 Changes include: * Sort config list and each configs lint list. * Add default text for the two configs that were missing it. * Switch the lint list in the configs to an attribute. * Make `dev fmt` sort the config list. r? `@xFrednet` changelog: none

xFrednet · 2024-07-30T09:21:19Z

Could you rebase to include the changes from the last PR?

book/src/lint_configuration.md

clippy_config/src/conf.rs

xFrednet

Tiny NIT, the rest looks good to me. We can merge this, once the commit has a proper name and the nit is fixed.

This PR was initially created to restrict the ".." value to the beginning or end. My understanding is that currently all positions are accepted again, right? Adding this restriction and a respective warning can be done in a followup, this PR is already quite large.

xFrednet · 2024-08-01T08:35:49Z

clippy_config/src/de.rs

+macro_rules! deserialize_table {
+    ($dcx:ident, $table:ident, $($name:ident($name_str:literal): $ty:ty,)+) => {


NIT: I think the usage if this macro is not self-explanatory. I would suggest adding a doc comment and renaming it to deserialize_table_row since it only processes one row.

Maybe:

Suggested change

macro_rules! deserialize_table {

($dcx:ident, $table:ident, $($name:ident($name_str:literal): $ty:ty,)+) => {

/// Can be used to deserialize a table row. Each column has a local

/// of type `Option<T>` that is filled, when the value is found.

///

/// Example usage:

/// ```

/// deserialize_table_row!(dcx, table,

/// path("path"): String,

/// reason("reason"): String,

/// );

/// ```

///

/// Example input:

/// ```toml

/// disallowed-methods = [

/// { path = "std::vec::Vec::leak", reason = "no leaking memory" },

/// # path = Some("std::vec::Vec::leak")

/// # reason = Some("no leaking memory")

///

/// { path = "std::time::Instant::now" },

/// # path = Some("std::vec::Vec::leak")

/// # reason = None

/// ]

/// ```

macro_rules! deserialize_table_row {

($dcx:ident, $table:ident, $($name:ident($name_str:literal): $ty:ty,)+) => {

What's this about rows? It can deserialize any toml table, both inline and regular. The TableLike trait is implemented for both.

Huh, I guess it depends on what you describe as a "table". I thought the whole disallowed-methods is a table, like this

path reason

"std::vec::Vec::leak" "std::vec::Vec::leak"

"std::time::Instant::now"

But I'm guessing now that you called { path = "std::vec::Vec::leak", reason = "no leaking memory" } a table?

Ah, that's what you mean. The name is referring to a toml table.

Okay, then we can keep it like this. Could you rename the commit and fix the last error? Then we can merge this :D

xFrednet · 2024-08-02T08:35:38Z

Have you seen my question from the last review:

This PR was initially created to restrict the ".." value to the beginning or end. My understanding is that currently all positions are accepted again right? Adding this restriction and a respective warning can be done in a followup, this PR is already quite large.

bors · 2024-08-06T15:13:11Z

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

xFrednet · 2025-03-16T14:44:21Z

I'm unassigning myself from this PR. @Jarcho if you want to continue this work, please request a new reviewer with r? clippy

Jarcho · 2025-04-12T20:55:37Z

r? clippy

Jarcho · 2025-04-12T21:26:45Z

disallow_span_lint is removed because the path to clippy.toml isn't normalized (it's in a parent directory of the test). It's a pretty low value test that only checks that clippy has it's own config file set correctly so away it goes.

The part has been split from this PR.

Failing run

Jarcho · 2025-04-21T01:55:03Z

@rustbot ready

* More precise spans in diagnostics * Allow multiple diagnostics for each config value * Ability to capture spans in the parsed config * Easier support of `..` in lists * Remove `clippy_utils` dependancy from `clippy_config` * Multi-column support for long suggestion lists

rustbot · 2025-05-04T13:53:31Z

☔ The latest upstream changes (possibly ea13461) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot assigned xFrednet Jul 11, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 11, 2024

xFrednet approved these changes Jul 13, 2024

View reviewed changes

clippy_config/src/conf.rs Outdated Show resolved Hide resolved

Jarcho mentioned this pull request Jul 17, 2024

Create lint passes using Conf #13088

Merged

Jarcho force-pushed the conf_refactor branch from beda539 to f53fce5 Compare July 17, 2024 22:31

Jarcho mentioned this pull request Jul 28, 2024

Misc changes to clippy_config #13173

Merged

Jarcho force-pushed the conf_refactor branch from e71254c to 9979380 Compare July 30, 2024 12:26

xFrednet reviewed Jul 31, 2024

View reviewed changes

book/src/lint_configuration.md Outdated Show resolved Hide resolved

clippy_config/src/conf.rs Outdated Show resolved Hide resolved

xFrednet approved these changes Aug 1, 2024

View reviewed changes

xFrednet mentioned this pull request Aug 12, 2024

Replace the metadata collector with tests #13221

Merged

xFrednet removed their assignment Mar 16, 2025

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 31, 2025

Jarcho force-pushed the conf_refactor branch 3 times, most recently from 7cf2738 to 219d1ec Compare April 12, 2025 20:54

rustbot assigned dswij Apr 12, 2025

Jarcho changed the title ~~Make extendable configs easier to add~~ Rewrite config parsing for better span support Apr 12, 2025

Jarcho force-pushed the conf_refactor branch 2 times, most recently from 0ec2bd5 to 22cfaaa Compare April 12, 2025 21:25

Jarcho force-pushed the conf_refactor branch from 22cfaaa to f09a875 Compare April 12, 2025 21:44

This comment has been minimized.

Sign in to view

Jarcho force-pushed the conf_refactor branch 4 times, most recently from 141033c to 2bf8a6b Compare April 21, 2025 01:54

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Apr 21, 2025

This comment has been minimized.

Sign in to view

Jarcho mentioned this pull request Apr 21, 2025

Add a note for disallowed items referring to the config file #14663

Open

Jarcho force-pushed the conf_refactor branch from 2bf8a6b to 76ddb0a Compare April 21, 2025 15:34

		macro_rules! deserialize_table {
		($dcx:ident, $table:ident, $($name:ident($name_str:literal): $ty:ty,)+) => {

-macro_rules! deserialize_table {
-    ($dcx:ident, $table:ident, $($name:ident($name_str:literal): $ty:ty,)+) => {
+/// Can be used to deserialize a table row. Each column has a local
+/// of type `Option<T>` that is filled, when the value is found.
+///
+/// Example usage:
+/// ```
+/// deserialize_table_row!(dcx, table,
+///     path("path"): String,
+///     reason("reason"): String,
+/// );
+/// ```
+///
+/// Example input:
+/// ```toml
+/// disallowed-methods = [
+///     { path = "std::vec::Vec::leak", reason = "no leaking memory" },
+///     # path   = Some("std::vec::Vec::leak")
+///     # reason = Some("no leaking memory")
+///
+///     { path = "std::time::Instant::now" },
+///     # path   = Some("std::vec::Vec::leak")
+///     # reason = None
+/// ]
+/// ```
+macro_rules! deserialize_table_row {
+    ($dcx:ident, $table:ident, $($name:ident($name_str:literal): $ty:ty,)+) => {

path	reason
`"std::vec::Vec::leak"`	`"std::vec::Vec::leak"`
`"std::time::Instant::now"`

Rewrite config parsing for better span support #13084

Are you sure you want to change the base?

Rewrite config parsing for better span support #13084

Uh oh!

Conversation

Jarcho commented Jul 11, 2024 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xFrednet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xFrednet commented Jul 13, 2024

Uh oh!

Jarcho commented Jul 13, 2024

Uh oh!

xFrednet commented Jul 14, 2024

Uh oh!

bors commented Jul 17, 2024

Uh oh!

Jarcho commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xFrednet commented Jul 18, 2024

Uh oh!

Jarcho commented Jul 27, 2024

Uh oh!

xFrednet commented Jul 30, 2024

Uh oh!

Uh oh!

Uh oh!

xFrednet left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xFrednet Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

Jarcho Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

xFrednet Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

Jarcho Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

xFrednet Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

xFrednet commented Aug 2, 2024

Uh oh!

bors commented Aug 6, 2024

Uh oh!

xFrednet commented Mar 16, 2025

Uh oh!

This comment has been minimized.

Jarcho commented Apr 12, 2025

Uh oh!

Jarcho commented Apr 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

Jarcho commented Apr 21, 2025

Uh oh!

This comment has been minimized.

rustbot commented May 4, 2025

Uh oh!

Uh oh!

Jarcho commented Jul 11, 2024 •

edited by rustbot

Loading

Jarcho commented Jul 18, 2024 •

edited

Loading

xFrednet left a comment •

edited

Loading

Jarcho commented Apr 12, 2025 •

edited

Loading