[WIP] Configure lints in Cargo.toml #5728

detrumi · 2018-07-15T10:30:56Z

Implements #5034

This PR adds a [lints] section to Cargo.toml using the following format:

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

Progress

Add lints section to manifest
Pass -A/-W/-D/-F flags to rustc
Allow specifying lints in workspaces (workspace level lints take precedence over package level)
Support feature syntax: [lints.'cfg(feature = "clippy")']
Pass lints to rustdoc too, based on a feature flag
Unknown lints are a warning (currently flags are directly passed to rustc which results in an error)

rust-highfive · 2018-07-15T10:31:06Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

ehuss · 2018-07-15T16:42:07Z

@detrumi I saw your message on Discord, did you figure out the virtual manifest distinction? There are two manifest types, Manifest and VirtualManifest. You can look at profiles as an example of something that can be specified in both. The virtual one is usually accessed via the Workspace.

I'd caution about having packages override the workspace, since that's the opposite precedence of other workspace settings. I'm not sure how that should work.

Also, just a heads up, rustdoc lint settings will probably be stabilized soon (rust-lang/rust#52354), and will probably want to reuse these for rustdoc. (Which will also probably need cap-lint support, but I imagine that will land separately.) One tricky bit is that rustdoc is used in two places (one for doc, and one for tests).

This will likely need a feature flag (see features.rs).

bors · 2018-07-15T17:43:13Z

☔ The latest upstream changes (presumably #5726) made this pull request unmergeable. Please resolve the merge conflicts.

detrumi · 2018-07-16T19:45:59Z

@ehuss Thanks for pointing me to profiles, seeing how that's done clears it up a lot.

The package override direction was taken from the proposal text in #5034, but maybe I read that wrong or there was a mistake in that, since you're right that having workspaces override packages makes more sense.

I'll look into the rustdoc flags.

ehuss · 2018-07-16T23:54:42Z

I don't think you misread it, different people will have different expectations. I'm not sure which way is the right way. I'm just cautioning that it may not be clear how it should work, and that someone on the cargo team might want to clarify it.

alexcrichton · 2018-07-30T22:11:05Z

ping @detrumi, have you had a chance to handle some of the follow up comments here?

detrumi · 2018-07-31T18:21:42Z

@alexcrichton Yes, I'm slowly working my way through implementing it.

detrumi · 2018-08-07T18:49:50Z

Some questions:

How should -F (forbid) flags be handled? Workspace lints normally override package lints, but that seems kind of weird for forbidden lints. One option is to let -F flags in packages take priority over workspace lints too. However, this would mean that you can't override that lint in the workspace any more, which might be unpractical.
Edit: after thinking about it some more, workspace lints should only override packages if they're stricter, as allowing lints while the package treats them as errors would mean that you can make changes to the package that compile while in the workspace, but don't compile for the package on its own.
There was an idea to warn if you specify an unknown lint name. However, I don't know how to check against that, as we don't have a list of known lints. At the moment, passing an unknown lint results in rustc erroring out when Cargo calls it with the lint flags.
Looking at the Appveyor build of my last commit, an import in the test file I added failed to resolve:
use cargotest::support::{execs, project};
This same line is also used in other test files, and locally I can run the test fine. Am I doing something wrong here?

alexcrichton · 2018-08-07T20:45:06Z

Others may have a better idea how to handle the flags and such but for the test at least it's been tweaked I think since this started, you should be able to just drop the leading cargotest::

dwijnand · 2018-08-07T21:15:56Z

tests/testsuite/lints.rs

@@ -0,0 +1,177 @@
+use cargotest::support::{execs, project};


The cargotest::support module is now just support, as of #5762.

Thanks, didn't expect that not rebasing would cause such issues

Yeah it does because CI tests the merge of this PR branch with the (current) state of the target branch (where the merge has happened).

alexcrichton · 2018-08-20T16:02:46Z

src/cargo/util/toml/mod.rs

@@ -240,6 +241,9 @@ pub struct TomlManifest {
    patch: Option<BTreeMap<String, BTreeMap<String, TomlDependency>>>,
    workspace: Option<TomlWorkspace>,
    badges: Option<BTreeMap<String, BTreeMap<String, String>>>,
+    lints: Option<BTreeMap<String, String>>,
+    #[serde(rename = "lints2")]


I'm not entirely clear on what the lints2 name is doing here, can you add a comment to the naming choice here?

Ah, I haven't figured out yet how to deserialize both [lints] and [lints.'cfg(...)'] correctly, so I just temporarily named it something else (sorry!). Just using #[serde(rename = "lints")] here doesn't work, so it probably needs a custom Deserialize impl.

Hm ok, I'm not really sure we'd want to support cfg(..) for lint configurations though in the sense that it feels like it's a bit overkill for configuration that can otherwise be specific in the crate source anyway?

Right, let's simplify to just a [lints] section then.

alexcrichton · 2018-08-20T16:04:45Z

src/cargo/core/lints.rs

+    pub fn set_lint_flags(&self, unit_cfg: &[Cfg], features: &HashSet<String>, cmd: &mut ProcessBuilder) {
+        match self.cfg {
+            None => self.set_flags(cmd),
+            Some(CfgExpr::Value(Cfg::KeyPair(ref key, ref value)))


I think we've already got a number of #[cfg] matching functions throughout Cargo, could those be reused here?

I don't see one that could be used here, except maybe the CfgExpr::matches function that is used in the next line. The way I wrote it seems simpler than wrapping all feature strings inside Cfg::KeyPair, unless feature = foo keypairs can be nested inside other CfgExpr's?

I think we'll want to funnel everything through matches to ensure it's consistently appplied

tests/testsuite/lints.rs

azriel91 · 2018-10-15T00:32:39Z

Hiya @detrumi, are you still working on this?
It would be an awesome feature to have!

alexcrichton · 2018-11-06T20:56:34Z

This is unfortunately pretty old at this point so I'm going to close this (and we also have a soft feature freeze right now with Cargo). The Cargo team, however, will hopefully be able to allocate resources for this in early 2019!

phansch · 2018-12-13T07:14:49Z

@detrumi Would you mind if I pick up this PR in the next days? I would really like to see this implemented in 2019. (I would probably start with a fresh PR)

ehuss · 2018-12-13T18:34:11Z

@phansch We discussed lint configs at the Cargo meeting yesterday. We would like to see more discussion on the design before continuing. Would you be interested in helping to hash this out on IRLO? The following needs a closer look:

Need a clear description of the motivation and use cases.
Is there a more general solution? How does this relate to other tool configurations such as rustfmt.toml?
Is Cargo.toml really the best place? Need to clarify why to choose one approach over the others.
- If the use case is for large organizations to share common settings, then this assumes they have a monolithic workspace which may not be true for everyone.
- The counter argument for something like lints.toml is the proliferation of many config files for a Cargo project (rustfmt.toml, lints.toml, .cargo/config, etc.).
How does precedence work? Can packages override workspaces, or the other way around? Or maybe based on strictness (workspaces can make lints more restrictive, not less)?
How much of an issue is errors for unknown lints? My feeling is that it should be OK, but it does set a floor for the minimum supported rustc for something that is not really critical.
How important is it to report the location of the lint? Currently rustc will tell you it's in the command-line. Finding the correct location for the lint may be tricky in a large project. I personally think it would be good to have the intent to implement location reporting in rustc before stabilization.

We don't really feel this needs a full RFC process, but we would at least like to see the content/questions that arise during that process to be addressed.

phansch · 2018-12-13T19:35:47Z

@ehuss Makes sense. I'm going to try to write something up based on #5034 and incorporate all the different aspects this potentially touches. I will create a post on IRLO in the coming days 👍

detrumi · 2018-12-17T13:47:43Z

@detrumi Would you mind if I pick up this PR in the next days? I would really like to see this implemented in 2019. (I would probably start with a fresh PR)

Yes, feel free to work on this. Creating a post on IRLO seems like a good way to push this forward, as there are too many open questions. I'd be happy to help, but I don't have much experience pushing discussion forward and such.

rust-highfive assigned alexcrichton Jul 15, 2018

detrumi force-pushed the lints-in-manifest branch from 49d2614 to 68ae5b1 Compare July 16, 2018 19:25

dwijnand mentioned this pull request Jul 31, 2018

Resolve some Clippy warnings #5835

Merged

detrumi force-pushed the lints-in-manifest branch 2 times, most recently from 69fb5a4 to 4652dca Compare August 6, 2018 20:20

dwijnand reviewed Aug 7, 2018

View reviewed changes

detrumi force-pushed the lints-in-manifest branch 2 times, most recently from cd7dbf6 to 67ee176 Compare August 8, 2018 18:42

alexcrichton reviewed Aug 20, 2018

View reviewed changes

ehuss mentioned this pull request Sep 9, 2018

Improve documentation on cap-lints #5998

Closed

phansch mentioned this pull request Sep 11, 2018

It is impossible to understand from the readme file how to supress lints using clippy.toml rust-lang/rust-clippy#3164

Closed

detrumi added 6 commits September 21, 2018 09:31

Add lints section to manifest

d82c203

Warn for unknown lint states

c53c0cc

Add more unit tests

5ac592f

Allow lints in virtual workspace

286ec71

Add forbid lint kind

f4408a1

Leave lint priorities to rustc

54ffb7d

detrumi force-pushed the lints-in-manifest branch from 88e21f8 to 09abcb6 Compare September 21, 2018 08:05

Implement feature- and config-based lints

e6a64ea

detrumi force-pushed the lints-in-manifest branch from 09abcb6 to e6a64ea Compare September 21, 2018 08:31

alexcrichton closed this Nov 6, 2018

daxpedda mentioned this pull request Dec 1, 2018

Survey: 2019 wishlist rust-embedded/wg#256

Closed

yorickpeterse mentioned this pull request Dec 7, 2019

Be able to disable/enable Clippy lints globally EmbarkStudios/rust-ecosystem#22

Open

5 tasks

azriel91 mentioned this pull request Mar 12, 2020

Update to 2018 edition of Rust azriel91/autexousious#75

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Configure lints in Cargo.toml #5728

[WIP] Configure lints in Cargo.toml #5728

detrumi commented Jul 15, 2018 •

edited

Loading

rust-highfive commented Jul 15, 2018

ehuss commented Jul 15, 2018

bors commented Jul 15, 2018

detrumi commented Jul 16, 2018

ehuss commented Jul 16, 2018

alexcrichton commented Jul 30, 2018

detrumi commented Jul 31, 2018

detrumi commented Aug 7, 2018 •

edited

Loading

alexcrichton commented Aug 7, 2018

dwijnand Aug 7, 2018

detrumi Aug 7, 2018

dwijnand Aug 7, 2018

alexcrichton Aug 20, 2018

detrumi Aug 20, 2018 •

edited

Loading

alexcrichton Aug 23, 2018

detrumi Sep 21, 2018

alexcrichton Aug 20, 2018

detrumi Aug 20, 2018 •

edited

Loading

alexcrichton Aug 23, 2018

azriel91 commented Oct 15, 2018

alexcrichton commented Nov 6, 2018

phansch commented Dec 13, 2018 •

edited

Loading

ehuss commented Dec 13, 2018

phansch commented Dec 13, 2018

detrumi commented Dec 17, 2018 •

edited

Loading

[WIP] Configure lints in Cargo.toml #5728

[WIP] Configure lints in Cargo.toml #5728

Conversation

detrumi commented Jul 15, 2018 • edited Loading

Progress

rust-highfive commented Jul 15, 2018

ehuss commented Jul 15, 2018

bors commented Jul 15, 2018

detrumi commented Jul 16, 2018

ehuss commented Jul 16, 2018

alexcrichton commented Jul 30, 2018

detrumi commented Jul 31, 2018

detrumi commented Aug 7, 2018 • edited Loading

alexcrichton commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

detrumi Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

detrumi Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azriel91 commented Oct 15, 2018

alexcrichton commented Nov 6, 2018

phansch commented Dec 13, 2018 • edited Loading

ehuss commented Dec 13, 2018

phansch commented Dec 13, 2018

detrumi commented Dec 17, 2018 • edited Loading

detrumi commented Jul 15, 2018 •

edited

Loading

detrumi commented Aug 7, 2018 •

edited

Loading

detrumi Aug 20, 2018 •

edited

Loading

detrumi Aug 20, 2018 •

edited

Loading

phansch commented Dec 13, 2018 •

edited

Loading

detrumi commented Dec 17, 2018 •

edited

Loading