Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Roadmap] Configuration file for lints #6625

Closed
4 tasks
Tracked by #22
flip1995 opened this issue Jan 22, 2021 · 21 comments
Closed
4 tasks
Tracked by #22

[Roadmap] Configuration file for lints #6625

flip1995 opened this issue Jan 22, 2021 · 21 comments
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages C-tracking-issue Category: Tracking Issue P-medium Priority: Medium

Comments

@flip1995
Copy link
Member

This is something that comes up every now and then: a reusable configuration
file, where lint levels can be defined. Discussions about this often lead to
nothing specific or to "we need an RFC for this". And this is exactly what needs
to be done. Get together with the cargo team and write an RFC and implement such
a configuration file somehow and somewhere.

Steps to completion:

  • Draft a RFC
  • Coordinate with the cargo team and publish the RFC
  • Bikeshedding
  • Implement the configuration (file) in cargo
@flip1995 flip1995 added C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages C-tracking-issue Category: Tracking Issue P-medium Priority: Medium labels Jan 22, 2021
@matthiaskrgr
Copy link
Member

I wondered if it is somehow possible to declare a (proc?) macro in an extern crate that expands to a bunch of allow/deny directives and can be imported into crates where you want the warnings.
Unfortunately rustc does not seem to like the idea :( https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=7b020af857b7622d0cc9e3422f52c020

@CPerezz
Copy link

CPerezz commented Feb 2, 2021

I've been a couple of days taking a look to this and I was also thinking about post something in the IRLO and arrive to an RFC which we could start to work towards.

Do you need some help on Drafting the RFC and implementing the stuff? I would be happy to help 😃

@flip1995
Copy link
Member Author

flip1995 commented Feb 2, 2021

Help on drafting the RFC would be nice. I don't think another forum post will help much. Every discussion on this topic let to the same conclusion: this needs an RFC.

@CPerezz
Copy link

CPerezz commented Feb 2, 2021

Should we setup a hackmd or something similar? Or What do you have in mind?

@flip1995
Copy link
Member Author

flip1995 commented Feb 3, 2021

Yeah, I think a hackmd would be nice. I started one here and filled out summary and motivation. You have to sign in to hackmd.io in order to edit it. You can use your GH account for that. (I just want some low barrier of entrance to protect us from spammers).


One thing that is important is, that it is clear, that the draft should not set the syntax of the configuration in stone. Also it is an open question if it should be an extra file or included in Cargo.toml (or a hybrid). Both of this should be discussed in the RFC.

@CPerezz
Copy link

CPerezz commented Feb 3, 2021

Absolutely! Will start later on today then! 😃 Thanks @flip1995 !

@CPerezz
Copy link

CPerezz commented Feb 3, 2021

Added:

Will complete the sections by the end of the weekend. Feel free to ask or comment anything! 😃

@flip1995
Copy link
Member Author

flip1995 commented Feb 3, 2021

Thanks! I may put some time into this the next few days as well.

@repi
Copy link

repi commented Mar 10, 2021

This is one of our top Clippy feature requests from us at Embark, we have a standard set of an increasing amount of Clippy (and a few Rust) lints that we enable by default across 50+ crates in 10+ repos. See EmbarkStudios/rust-ecosystem#59 for the set of lints and our process.

This is currently done through a code snippet that add to every lib.rs & main.rs and search'n'replace on changes, which works and adds a lot of value. But having this proper configuration file later on will make the workflow way easier and also easier to review changes without having to see 40 crates enabling a lint in the same PR.

Here is how the our "lint configuration" looks like right now: https://github.com/EmbarkStudios/rust-ecosystem/blob/main/lints.rs.

@CPerezz
Copy link

CPerezz commented Mar 10, 2021

Hey that's helpful. Thanks for reporting it. I've been really busy the last month but I plan to get back to this and ship at least the RFC so that the discussions can start to take place. And this example will be a good one to have.

@danieleades
Copy link

arti is doing something very similar to Embark, using a python script to find and replace compiler flags. There's got to be a better way...

@jplatte
Copy link
Contributor

jplatte commented Dec 28, 2021

There has been a better way for a while, see EmbarkStudios/rust-ecosystem#22 (comment) (and following).

@danieleades
Copy link

There has been a better way for a while, see EmbarkStudios/rust-ecosystem#22 (comment) (and following).

That's a start, but I'm not sure it's a complete solution, at least not in arti's case. Would also need to be possible to selectively overwrite the compiler flags for specific crates (and the command-line flags will overwrite inline flags, right?). Is that supported?

@jplatte
Copy link
Contributor

jplatte commented Jan 3, 2022

I'm not sure. When you add a .cargo/config.toml in a sub-directory, flags from that will be combined with those from higher-up .cargo/config.tomls. I don't know in what order and whether a --allow lint cancels out an earlier --warn lint or --deny lint, but I recommend you just try it.

Worst case you would have to employ hacks like a .cargo/extra-config.toml that you symlink to crate/.cargo/config.toml for all crates in your workspace except one where you don't want a particular lint to be enabled.

@repi
Copy link

repi commented Apr 24, 2022

Would also need to be possible to selectively overwrite the compiler flags for specific crates (and the command-line flags will overwrite inline flags, right?). Is that supported?

Yes this works, we enable the lints for the full repository and all crates by default in .cargo/config (with this section), and then for specific crates one can still do top level or mod specific #![allow(clippy::xx)] that will override the command-line lints.

So it is the closest we've found to a solution before having a proper one with this issue.

There are two major issues though with this solution:

  • All the lints in .config/cargo.toml will also be applied to patched in path dependencies when they are compiled, so if you have an external crate that you have forked and want to patch in to use that locally in the [patch.crates-io] section, it will also be compiled with this config. This is usually a temporary local workflow so then we locally disable the .config/cargo.toml file. This problem does not exist with forked / patched in Git dependencies.
  • Any changes to .config/cargo.toml will rebuild the entire project

So still having proper support for specifying Clippy & Rust & Rustdoc lints in a single file in an official manner in a single file for the repository is still one of our top requests. But glad we do have this workaround for now at least.

@tgross35
Copy link
Contributor

tgross35 commented Jul 15, 2023

The main issue linked here was addressed in the manifest-lint? https://github.com/rust-lang/rfcs/blob/master/text/3389-manifest-lint.md

Perhaps the way forward is to use the exact configuration in the lints.clippy table specified in that RFC and allow it in clippy.toml? I think that should be compatible with the current clippy.toml, at least to the extent that we could detect and deprecate old behavior as needed, rather than deprecating the file (which I saw discussed but really hope we don't do - the name fits and is already in pretty wide use).

(side note, I think clippy should gain a --config-file argument to point to a specfic clippy.toml whenever this happens)

@Centri3
Copy link
Member

Centri3 commented Jul 16, 2023

(side note, I think clippy should gain a --config-file argument to point to a specfic clippy.toml whenever this happens)

Does the CLIPPY_CONF_DIR environment variable work? Not quite the most convenient though, --config-file would be nice.

@tgross35
Copy link
Contributor

I had no clue that existed, thanks for the hint! I don’t see it mentioned anywhere but here https://doc.rust-lang.org/nightly/clippy/development/adding_lints.html, I suppose I can send a docs PR if it’s intended to be better known

@Centri3
Copy link
Member

Centri3 commented Jul 17, 2023

It's here as well but yeah there should be a short section for it, rather than quickly mentioning it

@emilk
Copy link

emilk commented Aug 8, 2023

Perhaps some inspiration can be taken from cargo-cranky, whose whole existence is only needed because the issue at hand is not yet resolved.

@flip1995
Copy link
Member Author

flip1995 commented Aug 9, 2023

This is currently properly implemented in cargo: rust-lang/cargo#12115

So I think this can be closed

@flip1995 flip1995 closed this as completed Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages C-tracking-issue Category: Tracking Issue P-medium Priority: Medium
Projects
None yet
Development

No branches or pull requests

9 participants