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. |
|
|
||
|
|
||
|
|
||
| # Design |
There was a problem hiding this comment.
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.
I would find it helpful to have these as separate sections, particularly
- The reference being a succinct coverage of the syntax and semantics for quick referencing back to and easier discussion
- I generally recommend the reference have text that is a draft of what will go in the Cargo reference docs
| 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.
| ``` | ||
| $ cargo build --lints ci | ||
| ``` |
There was a problem hiding this comment.
what commands would this be supported on?
|
|
||
| # Prior art | ||
|
|
||
| The [custom named Cargo profiles](https://rust-lang.github.io/rfcs/2678-named-custom-cargo-profiles.html) RFC is probably the main prior art here. I think this feature works well, including profile inheritance. |
There was a problem hiding this comment.
If profiles are prior art for this, please compare them, e.g. one only operates on the workspace level while this is on the package level. This has problems with defining CLI semantics and maintenance.
|
|
||
| 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?
|
|
||
| # Unresolved questions | ||
|
|
||
| I've marked most unresolved questions as "open question" in the text of this RFC. I'd rather not duplicate them here, but I can do so if people like. |
There was a problem hiding this comment.
One problem with this approach is it makes it hard to identify if something is an unresolved question.
I lean towards writing the RFC in your recommended way and moving all questions to here.
There was a problem hiding this comment.
One benefit to inline questions is it makes it clearer what needs resolving before stabilization. That is something we could improve with the RFC process.
| This proposal adds the ability to define custom lint profiles: | ||
|
|
||
| ```toml | ||
| [lints.profiles.ci] |
There was a problem hiding this comment.
profiles should be profile. Yes, there are multiple profiles underneath it but the expected way to write a profile is to use a standard header in TOML which means the header pertains to only one profile.
This is similar to dependencies vs profile
| This proposal adds the ability to define custom lint profiles: | ||
|
|
||
| ```toml | ||
| [lints.profiles.ci] |
There was a problem hiding this comment.
We have enough problems with reusing target between "build target" and "target platform". We should have a different name than profile, especially one that can be correlated with the CLI better.
| 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?
|
|
||
| I think the main drawback here is that this is Cargo.toml-focused, so it requires people to buy in to doing lints via `[lints]` and not any other method if they wish to have the benefits. It also | ||
|
|
||
| # Rationale and alternatives |
There was a problem hiding this comment.
Wouldn't build.warnings also be an alternative to compare?
There was a problem hiding this comment.
Good call.
Breadcrumb for later: https://doc.rust-lang.org/beta/cargo/reference/unstable.html#buildwarnings
Will do
| - In code, by means of `[lints]` in Cargo.toml. | ||
| - This **does not** support use with `cfg` | ||
| - This **does** allow fine grained control over code sections |
There was a problem hiding this comment.
What is fine grained about this?
There was a problem hiding this comment.
This might just be a typo.
| - This **does** allow fine grained control over individual lints | ||
| - This **cannot** be easily tweaked at runtime without having to edit code | ||
| - This **can** be easily shared between crates (via workspaces) | ||
| - In code, by means of `[profiles.foo.rustflags]` and `-Afoobar` |
There was a problem hiding this comment.
Should we call out this is unstable (and maybe never will be stabilizaed)?
| ## 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?
|
|
||
|
|
||
|
|
||
| ## 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?
|
|
||
| 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.
Do you mean the entire crate graph?
|
|
||
| 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?
| ### 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)]
| ### "Teaching" lints | ||
|
|
||
| A proposal that comes up semi-regularly is for there to be lints that teach you more about your code. These may not even point out _mistakes_, but rather teach you things about code you are writing. While lint profiles doesn't solve this problem entirely, being able to toggle sets of lints for the purposes of "I am learning Rust and want some more helpful nudges" helps solve part of the problem for designs here. |
There was a problem hiding this comment.
Wouldn't this be overwhelming?
There was a problem hiding this comment.
Depends on how it's done. When people discuss this avoiding that problem is a part of that discussion. The goal of this RFC is not to design teaching lints, I'm just noting that group toggles help some of the solution space there.
There was a problem hiding this comment.
I bring this up because if teaching lints are a motivating case, can we feel confident that we are providing the right solution for it?
| $ cargo build --lints ci | ||
| ``` | ||
|
|
||
| Open question: what should the flag be called? Should it also be available as an environment variable? `--lints` is short but perhaps too short, `--lint-profiles` also seems nice. |
There was a problem hiding this comment.
Another one: "Is this justified as a dedicated CLI flag?"
Something that came up with --artifact-dir and a previous generation of the Cargo team is CLI bloat (rust-lang/cargo#6100). I've seen first hand the problems of a large "API" surface in my maintenance of clap. By adding a feature, we make discoverability of all features harder, making it so people use all of the features less, meaning we get less value out of all of our features, new and existing.
|
|
||
| Open question: what should the flag be called? Should it also be available as an environment variable? `--lints` is short but perhaps too short, `--lint-profiles` also seems nice. | ||
|
|
||
| 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. |
There was a problem hiding this comment.
If we're calling [lints.*] the default profile, we should probably reserve the profile name default. We can defer any other decisions about its use.
|
|
||
| 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.
| 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.
How do inherits and workspace interact?
| 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?
| 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)
|
|
||
| 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
| 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.
| 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.
| (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.
| 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.
| 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.
|
|
||
| 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?
| ```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 |
| ## 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.
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