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

Expect lints.workspace = true for crates in a workspace with a [workspace.lints] section #11933

Open
JarredAllen opened this issue Dec 6, 2023 · 4 comments
Labels
A-lint Area: New lints

Comments

@JarredAllen
Copy link
Contributor

JarredAllen commented Dec 6, 2023

What it does

If your workspace has a [lints] table specifying lints, then this lint checks that the lints.workspace = true value is set in the Cargo.toml of all packages in the workspace, to ensure that those lints are actually applied.

We also might want to check for the other workspace-inheritable things, but those should be separate lints since people might want to only enforce this for some of them, and I don't personally use any of them, so I'm less qualified to talk about that.

Advantage

Specifying lint levels once in the package workspace is nice, but it's easy to forget to add the corresponding field to the Cargo.toml for each package member, which can result in extra lints specified in the package workspace silently not being applied. Having this lint will allow me to be confident that those lints are picked up in all of my packages.

See my post on URLO asking about how to check for this for the problem it solves.

Drawbacks

Some people might deliberately not inherit the workspace table for all lints, enough that I feel like allow-by-default is probably the play (idk if this belongs in cargo or pedantic, but one of those feels right).

Example

If the workspace Cargo.toml has:

...
[workspace.lints.clippy]
# Unimportant what's here, just that something is set
# Also should apply if it's not clippy lints but some other linter
pedantic = "warn"
...

And the package Cargo.toml is missing a

[lints]
workspace = true

Then we can add it for them.

@JarredAllen JarredAllen added the A-lint Area: New lints label Dec 6, 2023
@torokati44
Copy link

Oh my gosh, I thought/hoped that this is implicitly on without having to opt in for every workspace member crate. 😱

@emilk
Copy link

emilk commented Apr 25, 2024

I 100% want this lint, but with one small modification

If your workspace has a [lints] table specifying lints, then this lint checks that the lints.workspace = true value is set in the Cargo.toml of all packages in the workspace

I think the lint should enforce that all crates in the workspace has lint.workspace set to something., That is: it is fine to explicitly opt-out using lints.workspace = false, but NOT ok to omit lint.workspace completely

@Kriskras99
Copy link

@emilk that would be great, but currently setting lints.workspace to false is not allowed.

@epage
Copy link

epage commented Apr 29, 2024

So one one about this, you do not inherit from [lints] but [workspace.lints].

One way to reframe an opt-out to not require workspace = false is to check if the [lints] table, even if empty, is present. That still requires an explicit action.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

No branches or pull requests

5 participants