Should rust-analyzer run clippy by default?

tools and infrastructure
1

To get compiler errors, rust-analyzer runs cargo check. We could run cargo clippy instead, the question is, should we?

At the moment we don't do that, but I the primary reason for that is historical (I just don't use clippy myself).

I am not sure how do decide here, so I figured that I'd just through the question here to get more feedback :slight_smile:

To clarify, switching from check to clippy is simple, one needs to set "rust-analyzer.checkOnSave.command": "clippy" in the config file of the editor, this is specifically a question of defaults.

3 Likes
2

I personally don't think clippy should be run by default. It should be easy to enable (and already is), but not the default. Running clippy by default is tantamount to just merging clippy intro rustc/rust-analyzer, IMO.

There's a reason clippy is separate from the core tooling. I don't think clippy should be run by default for this same reason.

5 Likes
3

I don’t have a strong opinion, in particular since it is reasonably easy to opt in, so just a thought: clippy also performs an analysis, so based on naming it could be included by default.

4

Of course not. Clippy is very opinionated - when I use it, I write multiple lines just to turn off the lints I don't like. If you will use Clippy without any configs - it will be very annoying.

1 Like
5

As a counterpoint, whenever I use clippy (which is to say on all of my non trivial repos) I have multiple lines to turn on lints that I think it should have enabled by default, and never turn lints off project wide.

5 Likes
6

This may be too subtle, but could rust-analyzer use the presence of a clippy.toml config file to figure out if it should run clippy by default? I know that this file is optional, and many people use clippy using the default settings (thus they do not have a clippy.toml). But the presence of this file might be a strong indicator that the developer is prepared to handle warnings issued by clippy and may want to see them in their IDE.

13 Likes
7

I really liked the rls approach here. If there were #![warn(clippy::all)] or similar in the crate it enables clippy, otherwise it just checks.

5 Likes
8

Personally I would prefer if it uses clippy by default since most/all default lints lead to better code in my experience. Detecting the use of clippy annotations and only using clippy if it detects a tool annotation or the configuration file also seems reasonable.

I think there are two further options not mentioned so far:

  1. The IDE extension/rust-analyzer could ask the user once if they prefer to enable extended linting/clippy. Such a prompt also occurs for other extensions, for example the python Vscode extension asks for a choice between jedi, Microsoft languagesserver and pylance.
  2. If the full set of clippy lints is too noisy, how about running only a subset of them by default? The obvious candidate here are the correctness lints which are deny by default.
1 Like
9

I think I roughly agree that the best possible behavior would be for the extension to prompt on first startup to chose the global setting between

  • just r-a, no use of cargo
  • always cargo check
  • always cargo clippy
  • if clippy.toml is present { cargo clippy } else { cargo check }

(And as a default before the user makes a choice, either just don't use cargo, or sniff clippy.toml.)

4 Likes
10

I didn't know that there was an option to use clippy. Enabling that option is easy enough once someone knows it exists. Perhaps a happy medium would be to leave the defaults as they are and make the option more visible/discoverable to the user?

11

I think it should be incredibly easy ("check a box" easy) to turn on, but it should be off by default. Clippy is where lints go that aren't suitable to go in rustc directly (or aren't yet, but may be in the future). Enabling clippy by default would give a different impression of the language, and not necessarily the impression we want to give.

9 Likes
12

Maybe run clippy if there are no other warnings in the project? I'm worried about hundreds of style lints like "you should format your numbers with thousands separators" drowning out real warnings.

13

I'm with @josh here. I just went looking for the option to turn it on and couldn't find it in the Extensions settings in VS Code. It should definitely be there -- but not enabled by default.

Also I've seen quite a few issues where cargo clippy on the command-line (usually at the workspace level) doesn't show me warnings but then cargo clippy --workspace --all-targets -- -D warnings (which is what I use in CI) fails anyway. In other words, I'd hope the RA integration would set --workspace --all-targets by default or allow me to change the defaults.

1 Like
14

If the warnings of clippy would be uncontroversial, then they would be in rustc.

Therefore I don't see a reason why the default experience with rustc and rust-analyzer should be any different.

2 Likes
15

I don't see how that is so. This is talking about what the default should be for Rust IDE's that are using Rust Analyzer. Should they default to using "Clippy Lints"? Prior Art would suggest YES. Java and C#, for example, both enable the equivalent by default in all the normal IDE's for these languages.

1 Like
16

There's also another factor involved in IDE errors that changes the trade-off some: lints in the IDE aren't (typically) just presented in a big list. It's a lot easier to cut to "important" lints when they're specially spread out, as opposed to the output on the command line.

"Ideal" integrated behavior would be for all of the warnings (even the off-by-default ones!) to be reported to the IDE, which then you use the IDE's configuration to handle warning severity. (Complicating the matter is that clippy's warning model probably doesn't line up 1-1 with your IDEs warning model.)

17

What if clippy is not installed?

I don't use clippy simply because I cannot install from normal Debian repositories, unlike the rest of Rust.

18

Then, to be fair, you probably aren't using r-a either, because it's also not available in Debian upstream either.

I'm pretty sure that r-a assumes by default what you're using rustup and any other setup is still mostly on your own.

1 Like
19

Well, the downloaded rust-analyzer does work fine with my system toolchain on Fedora. AFAICS the only place it tries to actually invoke rustup is if rust-src is not found in the sysroot.

20

I don't even know how to configure rust-analyzer, I wish it is the default.