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.
There was a problem hiding this comment.
I'm happy to write an RFC for that. I'd like to use roughly the same design because I think it's conceptually quite clean and extensible. When I saw that issue I was worried that an RFC with a similar design would be disliked because it would be motivated by more than just #13157, but maybe I can do this in a way that doesn't
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.
I disagree with this characterization: this RFC specifies an inheritance mechanism in detail.
(I double checked: your comment was written after I pushed the reference level text)
There was a problem hiding this comment.
When I saw that issue I was worried that an RFC with a similar design would be disliked because it would be motivated by more than just #13157, but maybe I can do this in a way that doesn't
There is justification enough for an overriding lint inheritance RFC to exist. I see no problem with listing this RFC as a future possibility and talking about it as part of the Rationale and Alternatives.
I disagree with this characterization: this RFC specifies an inheritance mechanism in detail.
While the Reference section has been expanded it, I find it ambiguous on some points and it doesn't cover every feature of the lints table. There is also covering it in Rationale and exploring Alternatives.
|
|
||
| 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.
|
|
||
| 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)]
| 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
| 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.
| 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.
There was a problem hiding this comment.
I feel like this very neatly resolves a lot of use cases for toggling lints at once, including long-standing Clippy issues. I don't find the complexity to be that much: I think the inheritance model is a little complex but only from an implementation standpoint, and something like that is necessary for workspace inheritance anyway (which I think people want to fix, and I shall perhaps write a separate RFC for)
There was a problem hiding this comment.
I agree this isn't too big a deal for implementation complexity. We are very mindful of that as workspace inheritance has pushed the implementation complexity quite and bit. There are common asks that go beyond that (e.g. not needing to specify path in dependencies) but this is an incremental addition on workspace inheritance complexity.
My concerns roughly boil down to the mixture of
- We are adding a third form of inheritance (similar but not quite the same as
[profile]) - We are mixing two different forms of inheritance
- We have or are considering features that strongly overlap with this (
build.warnings, inheritance groups) - We are spending this schema complexity on something that is a couple levels removed from the user getting their job done, rather than on the problems more directly in the users way (build-target issues, features, dependency resolution)
- Just in terms of space occupied, I'm already pretty bothered that
[lints]takes up 69 out of 131 lines of a no-op package of mine (reference)
- Just in terms of space occupied, I'm already pretty bothered that
| 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).
There was a problem hiding this comment.
That's an interesting and pretty promising option, and I think it could work. It wouldn't solve the contextualness of lints which is what I try to highlight at the beginning, and I think that's pretty important. Which lints one cares about is very contextual and this RFC tries to get at the heart of that by giving ways to group and name them.
| 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. |
|
In case it is relevant for this conversation, I've posted rust-lang/cargo#16796 for stabilizing |
Manishearth
force-pushed
the
custom-lint-profiles
branch
from
26e8f11 to
032d9ae
Compare
3 participants
View all comments
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