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.

Sign up for GitHub

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

Add a [lints] table to Cargo.toml #3389

Merged
merged 64 commits into from
May 9, 2023
Merged

Add a [lints] table to Cargo.toml #3389

Changes from 1 commit
Commits
Show all changes
64 commits
Select commit Hold shift + click to select a range
b80c2ff
chore: Start manifest-lint RFC
epage Feb 14, 2023
28f51c3
feat: Start manifest-lint RFC
epage Feb 14, 2023
6ac518e
fix: Remove stray paren
epage Feb 14, 2023
aa8695b
fix: Pluralize the table
epage Feb 14, 2023
680fe11
fix: Make lints table top-level
epage Feb 14, 2023
054de51
fix: Call out workspace lint name reservation
epage Feb 14, 2023
3796056
fix: Link to user-defined attribute RFC
epage Feb 14, 2023
cfbd231
feat: Add a couple more brainstorming ideas
epage Feb 15, 2023
c57a510
fix: Use correct --forbid syntax
epage Feb 15, 2023
42010e8
fix: Apply ehuss' feedback
epage Feb 15, 2023
b79b792
fix: Add RFC number
epage Feb 15, 2023
4f4107f
fix: Link out to rubocop
epage Feb 15, 2023
bf6d9fc
fix: Typos
epage Feb 15, 2023
d136690
fix: Include cargo-cranky as prior art
epage Feb 15, 2023
75198c2
fix: Typo
epage Feb 15, 2023
4a267a6
fix: Be more explicit on workspace inheritance
epage Feb 15, 2023
c726cf3
feat: Add external file possibility
epage Feb 15, 2023
508e92f
feat: Add cargo lints as a future possibility
epage Feb 15, 2023
1c7ef77
fix: Note that cargo-metadata support is needed for configurable lints
epage Feb 15, 2023
4bfc545
fix: Update to lints.tool.lint
epage Feb 15, 2023
f8071a3
feat: Add open question about rustfmt
epage Feb 15, 2023
1e43928
fix: Discuss all supported lint tools
epage Feb 17, 2023
3cd6125
fix: Update for latest conversation
epage Feb 21, 2023
b02771a
fix: Typo
epage Feb 21, 2023
5a70d45
feat: Take a stab at lint precedence
epage Feb 21, 2023
08f7190
fix: Make priority signed, giving a clear center value
epage Feb 22, 2023
7d81bf1
fix: Add another reason against 'rules'
epage Feb 22, 2023
31c9587
fix: Some TOML formatting
epage Feb 22, 2023
cf9a148
fix: Clarify we are overriding lint groups
epage Feb 23, 2023
1adde1b
fix: Typo
epage Feb 23, 2023
78083ed
fix: Document future idea for lint-level source
epage Feb 23, 2023
72cdd44
fix: Typos
epage Feb 23, 2023
7073266
fix: Spelling and language
epage Feb 27, 2023
86b0b64
fix: remindme priority with multiple lint sources
epage Feb 27, 2023
a270d28
fix: Be explicit that lints does not affect dependencies
epage Feb 27, 2023
78ab70d
fix: Typo
epage Feb 27, 2023
cf93e59
fix: Expand on lint source future
epage Feb 28, 2023
e92a52b
fix: Isolate array precedence
epage Mar 7, 2023
ef224ec
fix: Document 'auto-priority' alternative
epage Mar 7, 2023
ef4a490
fix: Add missing namespacing of rust lints
epage Mar 18, 2023
86932bd
fix: Call out rust/rustc category confusion
epage Mar 24, 2023
5afa0cf
fix: Be more explicit in how the lints table is loaded
epage Mar 24, 2023
a54d985
fix: Clarified this isn't limited to rustc/clippy
epage Mar 24, 2023
de44058
fix: Explicitly call out why rust table exists'
epage Mar 24, 2023
1557767
fix: Clarify I meant lint levels, not general lint configuration
epage Mar 24, 2023
e1230cd
fix: Clarify clippy.toml isn't going away yet
epage Mar 24, 2023
2f5f873
fix: Add clarification that an example is only an example
epage Mar 24, 2023
3543967
fix: Be explicit that lint configuration is a future possibility
epage Mar 24, 2023
45766c4
fix: Update now that we have confirmation on ruff's design choice
epage Mar 29, 2023
fbf6f48
fix: Expand on why not `::` but separate tables
epage Mar 30, 2023
935593f
fix: Expand more on why not level=lint
epage Mar 30, 2023
c3f932c
fix: Remove confusion over :: and tool-config
epage Mar 31, 2023
e73e6b9
fix: Add auto-sort as a future possibility
epage Apr 10, 2023
d32801b
fix: Add high-level guidance
epage Apr 10, 2023
330782a
refactor: Break up Rationale / Alts into smaller sections
epage Apr 10, 2023
9cbc977
fix: Rewrite :: section
epage Apr 10, 2023
2f1b799
fix: Typos
epage Apr 10, 2023
660bcdb
fix: Expand the schema section
epage Apr 10, 2023
28f14ef
fix: Expand on precedence options
epage Apr 10, 2023
51f4984
fix: Move some discussion to stablization
epage Apr 18, 2023
2b10a5e
fix: Be more precise when talking about disjoint groups
epage Apr 19, 2023
4cb8421
fix: Clarify dependency situation
epage Apr 27, 2023
a47520f
fix: Call out reducing rebuilds for filtering lints
epage Apr 27, 2023
7aab0bd
Add tracking issue.
ehuss May 9, 2023
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
Prev Previous commit
Next Next commit
fix: Spelling and language
  • Loading branch information
@epage
epage committed Feb 27, 2023
commit 70732661ab14212bf163de1b5f1795fac976c4f8
8 changes: 4 additions & 4 deletions text/3389-manifest-lint.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,9 @@ unsafe = { level = "forbid", priority = 0 }
- `warn`
- `allow`

`priority` is a signed value that controls which lints or lint groups override other lint groups:
`priority` is a signed integer that controls which lints or lint groups override other lint groups:
Copy link
Member

Choose a reason for hiding this comment

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

I don't think a priority attributes really solves the issue here. This puts a lot of burden on the user. Users will have to know how lint levels are resolved in rustc and why this is even necessary.

Figuring this part out is the biggest blocker of this RFC IMO.

I think a solution here would be to handle lints in the order:

  1. lint groups first
  2. lint levels in the order: allow, warn, deny, forbid. This also applies to lint groups.

Taking the example below:

  • -Aclippy::all -Wclippy::doc_markdown
  • -D future-incompatible -A semicolon_in_expressions_from_macros

This would look like (using my preferred syntax):

[lints]
allow = [ "semicolon_in_expressions_from_macros" ]
deny = [ "future_incompatible" ]

[lints.clippy]
allow = [ "all" ]
warn = [ "doc_markdown" ]

Which would then become

-Aclippy::all -D future-incompatible -A semicolon_in_expressions_from_macros -Wclippy::doc_markdown

I'm not 100% sure if this ordering will work out though. Maybe there are still edge cases with overlapping groups 🤔

Now, cargo doesn't know the difference between a group and a lint. However, rustc does know the difference and can order them according to this specification. But this would probably mean, that new CLI flags in rustc have to be introduced to pass lints that were defined in the Cargo.toml. The question is, if we want this.

I generally think, that lint level configuration should be as easy as possible without any surprises. Lint levels shouldn't be a complicated construct you have to learn about to do it right. Things like the priority field over complicates this IMO.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I suggested this in #3389 (comment) as well

Copy link
Member

Choose a reason for hiding this comment

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

I think the "in the order they are specified" model is useful for when lint levels are being specified in multiple places. But I think it might be fine to do it just for CLI flags.

Strawman proposal: rustc gains a sort-lint-cli-flags arg that Cargo passes in, which perhaps becomes the default mode in the next edition.

Copy link
Member

Choose a reason for hiding this comment

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

Does the warnings meta-group complicate this at all? (AFAICT with some quick testing of -Dwarnings behavior it ignores flag order).

Copy link
Member

Choose a reason for hiding this comment

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

It's not actually a lint group; it turns all warnings into errors, including non-lint warnings

Copy link
Contributor

@jplatte jplatte Apr 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is using order for priority an option¹? As in, it's just an object with name to level mapping, but the order inside the TOML determines what overrides.

That is how many CLIs work: if you pass --no-foo --foo, foo is on, for --foo, --no-foo it's off.

¹ easy to maintain in deserialization w/ IndexMap

Copy link
Member

Choose a reason for hiding this comment

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

  • Could clippy change its mind about groups being disjoint?

Highly unlikely, but not impossible. Also other tools (rustc) already do this or will do this.

  1. Agreed

  2. I'm a bit confused: If we can detect conflicts in priority groups and warn about them, shouldn't it then be also possible to resolve those conflicts? Is resolving this always possible automagically? I think having priority in the MVP will give us time to figure those things out

  3. Agreed

  4. I hate everything about this 😅 : This is just not useable. It's all the bad parts from the lint = "level" and the level = [lint] syntaxes combined IMO.

  5. is maybe an option, but if we punt this to the user, I think priority is the better approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is using order for priority an option¹? As in, it's just an object with name to level mapping, but the order inside the TOML determines what overrides.

That is how many CLIs work: if you pass --no-foo --foo, foo is on, for --foo, --no-foo it's off.

¹ easy to maintain in deserialization w/ IndexMap

By making the reference to IndexMap, I assume you are meaning the order within a table? The cargo team ruled that out because (1) order for keys is not defined in the TOML spec and we don't want to rely on an extension of the TOML spec and (2) this doesn't communicate semantics to the user/tool and they could easily do things incorrectly, e.g. sorting the table.

Option 4 tries to instead explore ordering within TOML syntax and semantics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2 I'm a bit confused: If we can detect conflicts in priority groups and warn about them, shouldn't it then be also possible to resolve those conflicts?

We could except for when groups intersect. Framed differently, its the auto-sort solution but punts to the user instead of using a heuristic, with an MVP that always punts to the user.

Something I forgot to include in discussing is going past the MVP is the cargo/linter contract and acknowledging the parallel work for registering custom lints. I've not followed that too closely. Depending on how likely that is to be completed and what options are possibilities, each of these proposals could have a limiting factor on the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the RFC to reflect Option 2 (priority with warnings/auto-sort in the future).

- lower (particularly negative) numbers have lower priority, being overridden
by higher numbers, and shows up first on the command-line to tools like
by higher numbers, and show up first on the command-line to tools like
`rustc`

To know which table under `[lints]` a particular lint belongs under, it is the part before `::` in the lint
Expand Down Expand Up @@ -170,7 +170,7 @@ A concern brought up in
was that this will pass lints unconditionally to the underlying tool, leading
to "undefined lint" warnings when used on earlier versions, requiring that
warning to also be suppressed, reducing its value. However, in the "Future
possibility's", we mention direct support for tying lints to rust versions.
possibilities" section, we mention direct support for tying lints to rust versions.

This does not allow sharing lints across workspaces.

Expand Down Expand Up @@ -236,7 +236,7 @@ inherit with `workspace = true`, we could have `[workspace.lints.<preset>]`
which defines presets and the user could do `lints.<preset> = true`. The user
could then name them as they wish to avoid collision with rustc lints.

## Lint Predence
## Lint Precedence

The priority field is meant to allow mimicking
- `-Aclippy::all -Wclippy::doc_markdown`
Expand Down