Conversation
Manishearth
added
the
T-cargo
|
This is a T-cargo RFC (@rust-lang/cargo), but T-clippy should probably also be involved (@rust-lang/clippy) |
Manishearth
force-pushed
the
custom-lint-profiles
branch
from
dcb3f82 to
6cf3896
Compare
|
For this RFC I found it easier to merge the guide level and reference level parts into a single "design" section. I'm happy to split it if that is easier for people to handle, I want to get further signals on this RFC before I go too deep down that road. |
| correctness = "deny" | ||
| ``` | ||
|
|
||
| Lint profiles can control lint groups and lints as usual. They can also "inherit" from an existing profile, which means that they copy over the settings from that profile, applying further lint levels on top. |
There was a problem hiding this comment.
lints.workspace = true only supports inheritance and not overriding. That is being discussed in rust-lang/cargo#13157. There are enough semantic issues to work out with just that topic that it likely needs to be a dedicated RFC done in parallel or as a precursor to this RFC.
There was a problem hiding this comment.
If we choose to fix the rust-lang/cargo#13157 issue in this RFC, then the following will work:
This item is not resolved.
This is still taking an RFC level design and glossing over it within this RFC, either implicitly embedding it through inherits or talking about "choosing" to fix it but not covering it in depth.
Workspace inheritance overriding needs to be designed first and then "inherits" can leverage that design. As I said, the scope is large enough to justify it and this help isolate topics so each RFC runs more smoothly and is less likely to miss details.
|
|
||
| Both of these solutions are likely to be simpler, though. | ||
|
|
||
| # Prior art |
There was a problem hiding this comment.
Do other ecosystems have the same problems as listed in the motivation? How do they solve these problems?
| Lint profiles can be controlled from the CLI: | ||
|
|
||
| ``` | ||
| $ cargo build --lints ci |
There was a problem hiding this comment.
Is --lints for all packages, local packages, or root/selected packages?
Depending on the workflow involved, a user may want to selected a specific package in their workspace to see a special set of lints only for that package without also seeing them for local dependencies.
There was a problem hiding this comment.
I guess this got answered before with
Lint profiles set via CLI flag are only relevant for the current workspace.
but we do rationale for this choice
|
|
||
| One major alternative is allowing `[lints]` to exist in `[profiles]` (having them inherit the default profile if so). This could work, but it doesn't allow some of the things this RFC does, like disabling lints in test mode, or inheriting with a warn -> deny transform. | ||
|
|
||
| [RFC 3730: Add a semantically non-blocking lint level][RFC 3730] attempts to address the problem of wanting lints to be blocking in some contexts and non blocking in others, essentially by extending `-Dwarnings` to be a bit more flexible, and adding an IDE-useful "nit" lint level that only applies to new code. I think this attempts to solve a bunch of the same problems as this, but it solves them more narrowly: it doesn't help non-IDE users as much, and doesn't solve things like the test mode problem. |
There was a problem hiding this comment.
Could we get a comparison at how well the different alternatives stack up against this for resolving the questions at hand?
| ## Summary | ||
| [summary]: #summary | ||
|
|
||
|
|
||
| This proposes the ability to add "lint profiles" to Cargo.toml to allow for wholesale toggling of lint levels in predefined ways. | ||
|
|
||
| In essence, it is a much more powerful version of the coarse lint modality currently offered by `-Dwarnings`. |
There was a problem hiding this comment.
Can you add a config and CLI usage example?
There was a problem hiding this comment.
I don't think I'm 100% clear on what you're asking for here.
There was a problem hiding this comment.
In the summary, include a Cargo.toml that uses this feature and a cargo check --lints <whatever> example command to draw a picture of what this RFC is about.
|
|
||
|
|
||
|
|
||
| ## The many ways of toggling lints |
There was a problem hiding this comment.
An important criteria to call out is what gets rebuilt when it changes
| - This **does** allow fine grained control over individual lints | ||
| - This **cannot** be easily tweaked at runtime | ||
| - This **can** be easily shared between crates (via workspaces) | ||
| - In the CLI, by means of `RUSTFLAGS=-Afoobar` and friends. |
There was a problem hiding this comment.
Technically, you left off the config version of RUSTFLAGS. Unsure how relevant that is.
There was a problem hiding this comment.
Yeah in practice I think everyone who sets it via rustflags is doing it as a poor hack and dislikes it, so I didn't include it. Can anyway.
|
|
||
|
|
||
|
|
||
| # Unresolved questions |
There was a problem hiding this comment.
Should this require an MSRV bump? How might us doing that or not impact things?
text/3926-custom-lint-profiles.md
Outdated
|
|
||
| At first glance, it appears that fine grained control is available at both "code editing time" and at runtime, however `-Afoobar` is not pleasant to use at all when you are configuring hundreds of lints. What is missing is a way to toggle groups of lints on and off at runtime, where these groups can be controlled by the developer at a fine grained level in source code somewhere. | ||
|
|
||
| Furthermore, `-Afoobar`, either via `[profiles]` or via `RUSTFLAGS` works poorly with Cargo: most solutions for doing this at runtime can trigger recompilation of the entire crate. `[lints]` was developed in part as a way to avoid this problem. |
There was a problem hiding this comment.
Avoiding recompilation isn't given as a motivation for [lints] but it is for build.warnings
| ### Deny in CI | ||
|
|
||
| The most common use case is wanting to have the codebase be lint-free but not hinder development while hacking on something, but have the lints gate landing on `main`. Workflows around this typically involve running CI with `-Dwarnings` (or the new `CARGO_BUILD_WARNINGS=deny`), with contributors often running `cargo check` / `cargo clippy` locally and ensuring they are warnings-clean before opening a PR. |
There was a problem hiding this comment.
Something missing here is that losing the original lint level is a usability issue. Especially newer users seeing an error will think they have to do what it says while if we are clear that something is a warning, that makes it more likely for them to feel empowered to decide whether to do what it says or silence it.
|
|
||
| In essence, lints can have different effects in different contexts. | ||
|
|
||
| ## Lint modalities and their use cases |
There was a problem hiding this comment.
This is a mix of implementation and use case. It is unclear where use cases like "ratcheting up lint levels gradually" / "adopting a new linter" should go.
| Something similar can happen for IDEs which have a relatively muted lint display (often a small :warning: icon near the offending line of code): it's somewhat fine to inundate the programmer with lints because they'll only see the ones affecting the files they are editing, not every file. This expands the scope of lints a user is exposed to without necessarily showing them more than a manageable number of lints. | ||
|
|
||
|
|
||
| ### Check at release time |
There was a problem hiding this comment.
From past discussions, this seems like a strong motivator for you for this design and one which I don't see the same level of importance of being a motivating use case. Could you help me understand why Cargo should consider this a motivating case for this RFC vs something we happen to improve vs something we regrettably improve vs would actively not want to improve?
There was a problem hiding this comment.
No, this is not a strong motivator for me. This is a weak motivator for me. I have mentioned it a lot because I have seen it come up often and it's an easy example of a non-obvious lint modality.
I can add more examples of lint modalities I have seen come up before. I don't think any individual one of these is super strongly motivating, put together they show a large gap.
The one I have seen crop up a lot is the "test vs non test" one, to the extent that Clippy has a pile of hodgepodge configs around this.
And of course the PR-integrated/IDE integrated linter one.
There was a problem hiding this comment.
Would it be accurate to say some of these modalities are motivating while some are side benefits? If so, could you distinguish that?
There was a problem hiding this comment.
I can try. I don't think that's quite how I view this section, it's more trying to give an idea of the general space of modalities. I'll think about it.
Some of this is: I've seen a lot of these crop up over time from my position as clippy maintainer, and it's hard to tell what's actually important, but it's easy to notice things cropping up again and again.
There was a problem hiding this comment.
By defining what is driving the design vs side benefits, we can make sure we are all on the same page about priorities before getting caught up in the details. It then can help us better evaluate designs. This was critical for the MSRV resolver RFC and I feel like it will be helpful here as we have very different perspectives.
text/3926-custom-lint-profiles.md
Outdated
| ### Only on non-test-code | ||
|
|
||
| Some lints protect production code from things like panics and bad API choices, things which aren't as much of a big deal (or even, counterproductive to prevent) for test code. It's common to do something like `#[cfg_attr(test, allow(...))]`, however this can't be combined with the Cargo `[lints]` table, making it less useful as a feature. |
There was a problem hiding this comment.
This is more generally the "crate charactertistic specific linting".
For example, I put the following in my src/lib.rs but only put it in some bins (depending on how "production quality" it is meant to be) and not at all in my examples, tests, and benches.
#![warn(clippy::print_stderr)]
#![warn(clippy::print_stdout)]
text/3926-custom-lint-profiles.md
Outdated
|
|
||
| Open question: When should it be a hard error to specify `--lints foo` for a nonexistant profile `foo`? | ||
|
|
||
| Open question: Would it hurt to *by default* inherit lint profiles from the workspace? A straightforward implementation would break current behavior of `[lints]` (which does not autoinherit, though perhaps it ought to?), but we could make this behavior kick in only when you specify `--lint someprofile` and `someprofile` is defined on the workspace but not the individual crate. |
There was a problem hiding this comment.
For myself, I feel like auto-inheritance is out of the scope of this RFC.
I know people have talked about auto-inheritance but not finding a relevant issue atm.
| Open question: Maybe we want to merge it with profiles anyway? Or perhaps provide a way to set a profile's default lint profile? I haven't seen a strong motivation for this yet. | ||
|
|
||
|
|
||
| # Drawbacks |
There was a problem hiding this comment.
The name we use for this eats into the namespace for linters. We do give "unused field" warnings for unknown linters, preserving our ability to eat into it.
There was a problem hiding this comment.
We are reserving:
inheritsprofiles
on top of the existing
workspace
| some-other-noisy-lint = "warn" | ||
| ``` | ||
|
|
||
| This creates a profile that inherits from the default profile, but with all warnings replaced with hard errors, and further tweaks to some other lints. |
There was a problem hiding this comment.
Can inherits only pull from the current package or also the workspace?
There was a problem hiding this comment.
Current package, you have to first inherit a workspace profile to then "copy" it.
There was a problem hiding this comment.
I'm not seeing this mentioned in the RFC.
| some-other-noisy-lint = "warn" | ||
| ``` | ||
|
|
||
| This creates a profile that inherits from the default profile, but with all warnings replaced with hard errors, and further tweaks to some other lints. |
There was a problem hiding this comment.
This would have an impact on the design of precursor a workspace inheritance overriding, see #3926 (comment)
text/3926-custom-lint-profiles.md
Outdated
|
|
||
| Open question: Pick one | ||
|
|
||
| ### Option 1: A magic `test` profile |
There was a problem hiding this comment.
There is prior art in Cargo wrt actual profiles and would be good to evaluate the design and see what lessons learned there are from that
text/3926-custom-lint-profiles.md
Outdated
| inherits = "default" | ||
| ``` | ||
|
|
||
| This profile is what is chosen when running `cargo test`/`cargo bench` or building `test` (including integration, unit, and doc tests) and `bench` targets during `cargo check --all-targets`. |
There was a problem hiding this comment.
So we are applying this magically both based on
- command chosen
- build target type
I am a little hesitant about that.
There was a problem hiding this comment.
This is just an enumeration of cfg(test) situations. We could instead hinge it on that.
There was a problem hiding this comment.
I feel like we would need to limit it to that.
text/3926-custom-lint-profiles.md
Outdated
| inherits = "default" | ||
| ``` | ||
|
|
||
| This profile is what is chosen when running `cargo test`/`cargo bench` or building `test` (including integration, unit, and doc tests) and `bench` targets during `cargo check --all-targets`. |
There was a problem hiding this comment.
By having this be the default for some commands, we would be forcing rebuilds of non-test workspace members which would likely frustrate users.
There was a problem hiding this comment.
I don't think that's true: this would only apply to the actual targets being built.
There was a problem hiding this comment.
If I run cargo run and then cargo test, the main lib will be needed in both cases. Today, cargo run will build it and then cargo test would only rebuild it if the presence of a dev-dependency caused the set of features for it or a dependency to cause it to change.
If cargo run and cargo test have different profiles, then the main lib will be built under both profiles. If the flags differ (if that is the level we do caching at), then cargo test will cause the lib to rebuild. If we cache at the lint profile name level, then cargo test will cause it to rebuild independent of the lints.
There was a problem hiding this comment.
No, cargo test rebuilds the main lib with --test, always. Test targets already get double-built.
There was a problem hiding this comment.
The lib gets built normally for integration tests and separately with --test to run the unit tests.
The "built normally for integration tests" version of the lib is already not fully true due to feature unification. It will be not true at all with this RFC as written.
text/3926-custom-lint-profiles.md
Outdated
| (The name "testprofile" is used here so it's unambiguous when "test" is used as a keyword) | ||
|
|
||
|
|
||
| Open question: Does configuring the default profile happen on `[lints]`, `[lints.profiles.default]`, or both? |
There was a problem hiding this comment.
Is this redundant with
Open question: Should it be possible to also access the default profile via [lints.profiles.default]? What's the behavior of specifying both? Probably doesn't matter too much.
text/3926-custom-lint-profiles.md
Outdated
| Open question: Does configuring the default profile happen on `[lints]`, `[lints.profiles.default]`, or both? | ||
|
|
||
|
|
||
| `context` can be `normal`, `all` (default), or `test`. Other contexts can be added if desired. |
There was a problem hiding this comment.
I feel like this is a step below using cfg and would likely need to be discussed.
text/3926-custom-lint-profiles.md
Outdated
| This does open up the question of multiple inheritance: if we want to inherit both a normal and a test context profile, we may need some form of multiple inheritance. There are three ways to do this that fit well with this design: | ||
|
|
||
| - `inherits` accepts an array: `inherits = ["default", {profile = "testprofile", context = "test"}]` | ||
| - `inherits` instead takes a profile name: `inherits.default = true`, `inherits.testprofile = {context = "test"}` |
There was a problem hiding this comment.
You can then only have one place you inherit it from and requires workarounds if you want to inherit in multiple contexts but not all.
text/3926-custom-lint-profiles.md
Outdated
|
|
||
| This is pretty straightforward, but a lot of the other configurability in this feature is lost on tests. With a special `test` profile, one can't, for example, have a distinction between test and non-test lints work for PR integrated CI linters. | ||
|
|
||
| ### Option 2: Test-only contexts |
There was a problem hiding this comment.
So this is about making a test profile a form of a mixin that you define how it interacts with the main profile?
text/3926-custom-lint-profiles.md
Outdated
| ```toml | ||
|
|
||
| [lints.clippy] | ||
| indexing-slicing = [{profile = "normal", level = "warn", profile = "test", level = "allow"}] |
There was a problem hiding this comment.
| indexing-slicing = [{profile = "normal", level = "warn", profile = "test", level = "allow"}] | |
| indexing-slicing = [{profile = "normal", level = "warn"}, {profile = "test", level = "allow"}] |
| Open question: Maybe we want to merge it with profiles anyway? Or perhaps provide a way to set a profile's default lint profile? I haven't seen a strong motivation for this yet. | ||
|
|
||
|
|
||
| # Drawbacks |
There was a problem hiding this comment.
For me, this is adding a lot of design / user complexity and the question is whether it pays off. At the moment, it does not feel like ti does.
| This works well with lint groups since you may then upgrade nits to warnings when you decide to try and fix these. PR-integrated linters can also choose to have different behavior with these if desired. | ||
|
|
||
|
|
||
| [RFC 3730](https://github.com/rust-lang/rfcs/pull/3730) No newline at end of file |
There was a problem hiding this comment.
| [RFC 3730](https://github.com/rust-lang/rfcs/pull/3730) | |
| [RFC 3730]: https://github.com/rust-lang/rfcs/pull/3730 |
text/3926-custom-lint-profiles.md
Outdated
| ## Custom lint groups | ||
|
|
||
|
|
||
| A thing this feature does *not* let one do is toggle multiple lints at once in code sections, a feature that is useful to have. A *potential* extension of this feature would be to allow profiles to be defined as lint groups so that one can write `#[allo w(customprofile)]`. There's a lot of subtletlies of such a design, including: |
There was a problem hiding this comment.
| A thing this feature does *not* let one do is toggle multiple lints at once in code sections, a feature that is useful to have. A *potential* extension of this feature would be to allow profiles to be defined as lint groups so that one can write `#[allo w(customprofile)]`. There's a lot of subtletlies of such a design, including: | |
| A thing this feature does *not* let one do is toggle multiple lints at once in code sections, a feature that is useful to have. A *potential* extension of this feature would be to allow profiles to be defined as lint groups so that one can write `#[allow(customprofile)]`. There's a lot of subtletlies of such a design, including: |
| Open question: Maybe we want to merge it with profiles anyway? Or perhaps provide a way to set a profile's default lint profile? I haven't seen a strong motivation for this yet. | ||
|
|
||
|
|
||
| # Drawbacks |
There was a problem hiding this comment.
Not seeing an issue for this but there has been some talk about workspaces having named sets that you can inherit from. For example, the Cargo workspace has 2 different MSRVs. Right now, one gets inherited and the other is manually specified. We could instead have a msrv3 and msrv0 groups and do package.rust-version.workspace = "msrv3" (or maybe its at the package level we say what we inherit from).
This offers some of what this RFC offers, making this a potential alternative. I marked this under Drawbacks because I could also see us only wanting to go in one of these directions (if at all).
| In the past people wished for tooling that produces lints that potentially tell new users about subtleties in their code, subtleties that are not really *problems* to be fixed, but interesting things to be noted. These would be opted in to by individual users and as they get used to a concept, disabled globally one by one. Lint profiles allow one to better handle toggles like this, but it is not in an of itself a major step in this direction. | ||
|
|
||
|
|
||
| ## Further lint levels |
There was a problem hiding this comment.
I had the impression you didn't want to add a new lint level but it is listed here. Could you help me understand where you stand on the two different RFCs and the path forward?
There was a problem hiding this comment.
I was opposed to the new lint level as a solution to the problems stated in that RFC. The problems stated in that RFC are similar to problems I and others face, but are not solved by it.
As I mentioned here, I think there are two problems in this space: lint modalities, and the rigidity of warning UX. I think improving warning UX is a separate problem worth working on with that as a primary motivation; so a "nit" lint level that has different IDE and local behavior is still potentially useful, if designed with that motivation in mind.
|
I separated out the guide/reference sections, which led to a much more precise definition of inheritance. I also redid the testing choices since I think I have cleaner designs now. I have not addressed every comment, but have tried to address the major ones. |
2 participants
This proposes adding a way of defining multiple "lint profiles" to
Cargo.toml, allowing for runtime selection of different lint behaviors.To some extent, it is an alternative to #3730: it attempts to solve many of the same problems, but also attempts to solve other problems.
Rendered