Support defining enabled and disabled lints in a configuration file

[Rantanen]

This issue started its life in clippy (rust-lang/rust-clippy#1313). Once it was realized this should apply to built-in lints as well, it migrated to rust (rust-lang/rust#45832). There @kennytm suggested that it should be Cargo's responsibility to pass the -D/-W/-A flags to rustc.

Motivation

Currently project-wide lint configuration needs to be included in the crate sources. This is okay when the project consists of a single crate, but gets more cumbersome when the project is a workspace with dozen crates.

Being able to define the lints in an external file that will be used when building the crates would have several benefits:

• Sharing one configuration file for all the crates in a workspace.
• A canonical place to check for lint configuration when contributing to a new project.
• An easy way to examine the version history for lint changes.

Proposal

• A configuration lints.toml (or [lints] in Cargo.toml?) file that specifies the lints.
• The lints can be specified on the workspace level and for individual packages. Anything on the package level will override the workspace setup on per lint basis.
• The configuration file is cfg/feature-aware and can specify different lints depending on features. Specifying clippy-lints will result in clippy complaining about unknown lints if clippy isn't used.

Also... unlike what @oli-obk suggested in the rust issue, personally I'd prefer the following format:

[lints]
allow_unused = "allow"
non_snake_case = "allow"

While this is more verbose than allow = [ "allow_unused", "non_snake_case" ], I think the diff benefits and the ability for the user to group logically similar lints together even if some of them are deny and some allow outweighs the increased verbosity.

Also if Cargo/rustc ever support lint configurations, this would be more future proof:

cyclomatic_complexity = { state = "allow", threshold = 30 }

Example configuration

[lints]
allow_unused = "deny"
non_snake_case = "allow"

[lints.'cfg(feature = "clippy")']
cyclomatic_compexity = "deny"

The feature syntax is lifted from Cargo's dependency syntax - at least that was the intention. Personally I'm not a fan of it, but I guess that's a limitation of toml. Don't really have a better proposal for it, given I don't really know what's available int he toml syntax. :<

[Mark-Simulacrum]

I am strongly in favor. We should also probably specify that unknown lints should be a warning, not an error, for the purposes of backwards compatibility.

[Rantanen]

Forgot to mention: I'm also interested in contributing code for the implementation, if there is consensus on what should happen. Also do Cargo changes, such as this, go through the RFC process?

[Mark-Simulacrum]

This seems potentially worthy of an RFC, but I could see it going either way. There doesn't seem to be much design space. @alexcrichton can probably say more.

[alexcrichton]

This seems plausible to me! I think it'd be fine to add this to Cargo.toml on an unstable basis (behind a nightly feature) and we can decide to stabilize at a later date.

[lukaslueg]

Do we have a chance for rustc to report where the linter settings are coming from? Given the increasing amount of knobs and switches, it would be very nice to have rustc report that allow_unused was denied and that deny was specified by the workspace's root Cargo.toml or the commandline.

As rustc knows nothing about these, we could introduce a new switch to rustc that specifies the source of the following linter settings (e.g. --LS "/home/myproject/foobar/Cargo.toml" -D ... --LS "/home/myproject/Cargo.toml" -D ...).

Is this a thing?

[oli-obk]

I'm not sure if these should be in the Cargo.toml, as we many want to be able to specify them globally for a group of projects.

[oli-obk]

Is this a thing?

rustc already reports whether something was

• defined by command line
• defined by default settings
• defined by code

so we just add another variant for config file there.

[phansch]

I'm really interested in this, too.

Also if Cargo/rustc ever support lint configurations, this would be more future proof:
cyclomatic_complexity = { state = "allow", threshold = 30 }

It could be cyclomatic_complexity = { state = "allow" } from the start, then we would have the option to add further options in a second step.

[phansch]

This Rollup PR from rust demonstrates how rust itself could benefit from a shared lint config, too: rust-lang/rust#52268 (note all the 'deny bare trait object' PRs; and there's a few more)

[detrumi]

I'd like to start working on this, unless @phansch or @Rantanen started on this already.

Seems like the lints.toml name is the one I've seen used most, so let's go with that.
As for the syntax, allow_unused = "allow" seems reasonable and more ergonomic than requiring the cyclomatic_complexity = { state = "allow" } syntax.

I'm not sure if these should be in the Cargo.toml, as we many want to be able to specify them globally for a group of projects.

A Cargo.toml file can already be used for a workspace, unless you meant a group of workspaces?

[Mark-Simulacrum]

I'd prefer that we centralize this into Cargo.toml rather than adding an additional file, but that can likely be decided in further detail at a later point in time.

[oli-obk]

The argument against Cargo.toml is that you can have a single lints.toml for multiple projects. Or even hierarchical lints.tomls

[phansch]

@detrumi I didn't start any work on this, and won't have that much time either. From my side, feel free to start 👍

[Mark-Simulacrum]

@oli-obk Right, which is what workspaces are for IMO. I'd prefer to avoid "one more" file in every Rust directory.

[oli-obk]

That would mean that companies need one enormous workspace for all the crates in their company if they want to have a consistent lint setup. This is the very use case that started the discussion about lint configuration files. A company-wide uniform linting experience. Maybe specializeable for crates, but they can already do that via #![allow(foo)] in their lib.rs. If we leave the door open for importing lint configurations in a dependency-style manner, doing it in Cargo.toml seems fine though.

[Luthaf]

Why not use .cargo/config here ? This seems like the obvious candidate for a file shared across repositories/workspaces/crates. Plus, one can commit a .cargo/config file per repository in OSS scenarios.

[detrumi]

Alright, placing it in Cargo.toml seems like a better place to start then.

That would mean that companies need one enormous workspace for all the crates in their company if they want to have a consistent lint setup.

Hierarchical Cargo.toml files might be a solution to the enormous workspaces, but that's something that can be added later.

[Rantanen]

I'd like to start working on this, unless @phansch or @Rantanen started on this already.

Feel free to! I have been waiting for some kind of a decision on how to proceed, so haven't done anything for this myself.

[sanmai-NL]

@oli-obk, @Mark-Simulacrum: My company has one enormous workspace indeed. And we are highly in need of DRY linting/warnings/etc. configuration with local exceptions (preferably down to the statement-level). So if industrial use cases/productivity are the motivation, perhaps we can survey a bit to come up with requirements in a more deliberate process?

[oli-obk]

down to the statement-level

that part is no problem, you can configure lints in source code just fine.

And if you have one enormous workspace, then the Cargo.toml solution will work just fine for you I think.

[sanmai-NL]

Suppose we settle on configuring this in Cargo.toml, then at a later stage we can design a more general feature, namely supporting Cargo.toml override files. Such override files could e.g. patch sections such as the lints and warnings configuration. We can then circumvent the discussion about the need for a separate configuration file. The name of the section is more important, since compiler warnings and clippy lints may be merged in some form later.

[marrub--]

Is this dead currently? For projects with a hierarchical structure and lots of lints this is a fairly vital feature. Hoping to see this gain activity soon!

[ehuss]

@marrub-- A more detailed proposal was posted at https://internals.rust-lang.org/t/proposal-cargo-lint-configuration/9135.

[joshtriplett]

I very much don't want to see this go into a separate configuration file. I've worked with C and similar for decades, where warning options live in the build system rather than in source code, and that leads to quite a bit of skew where source code doesn't have the information needed to build.

I definitely think this information should not be in .cargo/config or anywhere that needs separate setup, and it shouldn't be in a workspace's Cargo.toml if that information doesn't get published in a .crate file. I could live with Cargo.toml, though I'd prefer sticking with code (and having a dependency crate that encompasses a pile of options for an include-like mechanism).

[dbeckwith]

The main thing I'm hoping to avoid is the repetition of lints in every crate in a workspace. I don't much care about it being in a config file vs. source code as long as there's some way to inherit/include the list of lints from some source.

[oli-obk]

Maybe we could come up with a scheme where you can import the lint configuration from another crate? So making it a language feature where you can define a crate that does nothing but set lint flags, and then you have another crate depend on it and import these flags into its own scope.

[sanmai-NL]

@joshtriplett: I do not understand your argument. The proposal is not to remove static analysis configuration from Rust source files, but to also support setting this configuration in an external file.

You mention being able to build from source alone, rather than source complemented with packaging metadata. Presently, Rust source code in the open source ecosystem of crates.io often cannot be built from source alone. The configuration in Cargo.toml is rather crucial. I define building from source alone as just using rustc and then input and appropriate output files, across many command lines.

[dbeckwith]

@sanmai-NL I think part of his argument was that some of those configuration files don't end up being published with the crate, especially if you're in a workspace. So having the lints live in a top-level Cargo.toml or something means that it wouldn't be included in individual crate sources, and information required to build the source properly would be missing.
I don't know how often people try to build crates from the source published on crates.io though; I would always go to the original repository to make sure I'm getting the full source code.

[sanmai-NL]

@dbeckwith: But even if the configuration isn't being included in the source distribution via crates.io, builds won't fail because of it. Clippy does not break builds when run as standalone tool. In the past there was a compiler plugin, but I do not see documentation about it in the current README.md. Now suppose there is some possibility that Clippy is used within rustc's compilation pipeline and that the configuration to it is the only way to unbreak this pipeline. Only then, for under those conditions, Clippy could be designed to not accept out-of-source configuration. That would solve the problem.

The use case for this feature request is of course within a context of Clippy being run separately, during a build pipeline (Continuous X).

Also, clippy.toml already exists as a configuration mechanism. So @joshtriplett's concern is not specific to the proposal here.

[dbeckwith]

@sanmai-NL What about #deny rules? If there are any of those, then building with vs. without clippy would provide different results in regards to whether the code will build successfully or not, which is important for repeatability.
I agree though, maybe this point is not relevant to this feature request, and your point about clippy.toml is a good one.

[sanmai-NL]

Suppose the scenario @joshtriplett tries to get at happens: the external config hasn't been distributed together with the source code.

• If there are Clippy deny rules in the code, these will be enforced as before.
• If there are Clippy deny rules in the original external configuration, these will not impede the build, since the external configuration is absent now.

So what @dbeckwith describes isn't a valid failure scenario.

The only valid failure scenario I can think of is the following. There are allow rules that except default deny rules. The allow rules are (only) present in external configuration, which itself is absent.

There are, however, no default deny rules, according to https://rust-lang.github.io/rust-clippy/master/index.html. Only within the clippy::correctness lint group, which isn't enabled by default. So this scenario is currently impossible.

Now let's concede the following conditions: the maintainership of some software solution in Rust has enabled the clippy::correctness lint group, and then made exceptions to that in external configuration. You want to build this solution to a release artifact using the Rust compiler only. Let's also concede that a Clippy compiler plugin is configured in the source code to be run unconditionally. Let's furthermore concede that a release build does not cap lints. Only then, perhaps, we have a valid failure scenario.

I find this wildly far-fetched and at so many steps the maintainers and consumers would be going against best practices, that I find it hard to see @joshtriplett reservation as warranted.

Not to mention, by making one or more simple design decisions we can defuse the scenario. As I wrote in my previous comment: Clippy could be designed to not accept out-of-source configuration under some conditions, for example when there is Clippy configuration within the source code already.

[joshtriplett]

On March 21, 2019 6:02:09 AM PDT, Sander Maijers ***@***.***> wrote: @dbeckwith: But even if the configuration isn't being included in the source distribution via crates.io, builds won't fail because of it. Clippy does not break builds when run as standalone tool. In the past there was a compiler plugin, but I do not see documentation about it in the current [README.md](https://github.com/rust-lang/rust-clippy/blob/c7d444508655eab132f0d24d81f9cdc08b848056/README.md). Now suppose there is some possibility that Clippy is used within `rustc`'s compilation pipeline and that the configuration to it is the only way to unbreak this pipeline. Only then, for under those conditions, Clippy could be designed to not accept out-of-source configuration. That would solve the problem. The use case for this feature request is of course within a context of Clippy being run separately, during a build pipeline (Continuous X). Also, [`clippy.toml` already exists](https://github.com/rust-lang/rust-clippy/tree/c7d444508655eab132f0d24d81f9cdc08b848056#configuration) as a configuration mechanism. So @joshtriplett's concern is not specific to the proposal here.

Note that this isn't just about clippy, it's also about compiler lints.

[joshtriplett]

To clarify something: I can live with lint configuration in the Cargo.toml shipped with a crate (though based on experience with many C packages I feel that will still cause problems). However, lint configuration in a file not shipped in the .crate file will cause problems for distributions. That includes shipping lint configuration in some other .toml file that Cargo doesn't ship, or shipping lint configuration in a configuration file of a workspace, or shipping lint configuration in .cargo/config.