Integration with clippy

[ishitatsuyuki]

Clippy is quite useful for sure, and I'm looking forward to having it integrated.

[nrc]

I wonder whether this should be done on the RLS end or in the IDE plugin? The interesting bit of this is it would be nice to apply the suggested changes, but I expect that should be similar to what we will do for compiler suggestions and won't need tight RLS integration.

[KalitaAlexey]

@ishitatsuyuki,
Look at https://github.com/KalitaAlexey/vscode-rust
I have a special command for running Clippy - "Cargo: Clippy".

[Hoverbear]

I propose this can be closed. clippy is a separate project and this is more suitable to an IDE plugin.

[nrc]

clippy is a separate project and this is more suitable to an IDE plugin

Perhaps, I'm keen to track this to ensure we do get a good experience in the long run. In particular, if we are able to apply Clippy's suggestions then I want to be sure that works smoothly with the RLS.

[tomprince]

Using vscode-rust, the rls integration provides feedback as you type, but as far as I can tell, running clippy directly only provides feedback after saving. And additionally (at least currently) it appears that the clippy output doesn't get the nice interactive benefits of the RLS formatted output.

It seems like it would make sense to provide access to the clippy diagnostics through the same format as the built-in compiler diagnostics, so each IDE doesn't need to add special support for clippy.

[ishitatsuyuki]

I was able to get clippy working as a command-line argument plugin, by setting RUSTFLAGS: see clippy README.

Concerns:

• Packaging. Currently there's no way to install any dylib other than rustc's.
• Long warning messages. At least overflows (clipped 😜 ) in vscode-rust.

[nrc]

Packaging. Currently there's no way to install any dylib other than rustc's.

Long term, Clippy will be distributed with Rustup (like the RLS) so I think this won't be a a problem, but it will be a bit of a long time coming. We might need something to keep us going in the mean-time

Long warning messages. At least overflows (clipped 😜 ) in vscode-rust.

Even if these are cut off in the panel, they should be displayed in full when you hover over the squiggle, correct?

[ishitatsuyuki]

Packaging. Currently there's no way to install any dylib other than rustc's.

Long term, Clippy will be distributed with Rustup (like the RLS) so I think this won't be a a problem, but it will be a bit of a long time coming. We might need something to keep us going in the mean-time

No. While the cargo-clippy executable is magically statically linked, the plugin isn't. I don't see any short term solution to this.

Long warning messages. At least overflows (clipped 😜 ) in vscode-rust.

Even if these are cut off in the panel, they should be displayed in full when you hover over the squiggle, correct?

No, the hover message is even clipped.

[nrc]

No, the hover message is even clipped.

I wonder why Clippy messages are clipped but compiler ones are not? They should be presented through the same mechanisms

[nrc]

If anyone wants to try to tackle this for the impl period, lets discuss how we might do it.

[ishitatsuyuki]

Basically it's just the matter of how we distribute clippy, and IIRC it's going to be distributed as dylib as a part of becoming submodule of Rust.

[nrc]

Yeah, it will be distributed by Rustup like the RLS.

[oli-obk]

I don't know much about rls, but isn't it essentially just calling rustc manually (via Command::new) at some point, right? We have a drop-in replacement for rustc: clippy-driver. So if I understand this correctly, we can just set RUSTC=clippy-driver and call it a day.

/me goes experimenting

[oli-obk]

Nevermind, my knowledge is severly outdated. Last time I looked at the rls source was in April

So... since clippy is trying to eliminate the plugin-method, the best way would for rls's rustc driver to duplicate the clippy's lint injection:

https://github.com/rust-lang-nursery/rust-clippy/blob/master/src/driver.rs#L78-L111

and depend on clippy_lints. Unfortunately that would mean that clippy not compiling causes rls to not compile. That can be worked around by making clippy a cargo feature of rls. In the rustc test suite for distributing rls, the feature could then be disabled if clippy fails to build.

[oli-obk]

@nrc what do you think about trying this scheme out by making rustfmt an optional feature?

[jkeiser]

Note: I did manage to get it working in my project by specifying it as a default feature. I modified Cargo.toml and lib.rs and messages started showing up!

Sometimes I have to restart VS Code because it starts taking an awfully long time to update, and some nightlies show nothing at all; but mostly I've had good luck :)

[oli-obk]

Yea, the plugin method works for now, but there is a push towards removing the plugin interface from the compiler and use custom drivers everwhere, so we need to figure out the next step now

[nrc]

#698 added clippy integration as an optional feature. Some outstanding questions:

• how should we turn this feature on? At present you have to compile the RLS yourself to get support. I suppose we want to have it on in the distro version whenever clippy is working and off when it is broken. That will take some work in the build system. If it is not available we'll need to message that to users some how.
• we should add a config option for running with Clippy - even if there is support available, not everyone wants to use it
• Clippy lints show up just like warnings at the moment (I think) I wonder if we want to distinguish them somehow? And/or have the ability to ignore them in the UI rather than permanently in the source.

[oli-obk]

we should add a config option for running with Clippy - even if there is support available, not everyone wants to use it

We already have a solution for that. It's that you need to add #![warn(clippy)] to your crate in order to have it activated. We can probably also do this automatically if a clippy.toml is present

Clippy lints show up just like warnings at the moment (I think) I wonder if we want to distinguish them somehow? And/or have the ability to ignore them in the UI rather than permanently in the source.

I would treat them just like other warnings, because the users already opted in.

[alexheretic]

It's really cool to see clippy lints being added to rls!

how should we turn this feature on? ...

Yes having this available whenever it can compile is good. But it would be great if we had some method to tell a nightly had rls+clippy or rls-sans-clippy. This way clients could inform or skip releases missing desired functionality.

we should add a config option for running with Clippy

Yes perhaps it'd be nice to also enable this by rls client config as having to slap #![cfg_attr(feature = "cargo-clippy", warn(clippy))] on each project may not be desired. This will allow me to add a "clippy" button on the client that turns on/off the functionality for all projects.

Clippy lints show up just like warnings at the moment (I think) I wonder if we want to distinguish them somehow? And/or have the ability to ignore them in the UI rather than permanently in the source.

It would be good to have source: clippy rather than rustc which is our current hard-coded source. It seems doable to detect that a diagnostic is from clippy (`.contains("clippy") would probably work).

[oli-obk]

This way clients could inform or skip releases missing desired functionality.

that will be done by also depending on clippy. if clippy builds, it'll be there as component and part of rls. If not, it'll not be in rls and not available as a component

This will allow me to add a "clippy" button on the client that turns on/off the functionality for all projects.

<3 That would be a great button for warnings, too (which already exist as a setting)

It would be good to have source: clippy rather than rustc which is our current hard-coded source. It seems doable to detect that a diagnostic is from clippy (`.contains("clippy") would probably work).

That's a general issue. lints aren't namespaced, I think we'll have to add namespacing before stabilizing clippy

[alexheretic]

that will be done by also depending on clippy. if clippy builds, it'll be there as component and part of rls. If not, it'll not be in rls and not available as a component

That should work thanks.

That's a general issue. lints aren't namespaced, I think we'll have to add namespacing before stabilizing clippy

So you're planning to change all the lints to have clippy_ in front of them or something? In any case in the mean time since our diagnostics include notes, we can at least check to see if the rendered message includes "clippy", or the clippy help url. This should catch everything pretty well until a namespace or similar is introduced.

[alexheretic]

I'm guessing this is blocked by getting clippy into the nightly releases. Is there anywhere we can track that progress, or is there an informal eta?

[oli-obk]

Not blocked. Just waiting on a PR: rust-lang/rust#48097

[alexheretic]

@oli-obk Do you know how we would add a config option to Rls that enabled clippy warning in the project + all other workspace projects without having to modify the project source?

[oli-obk]

Seems to me like that would be no different from any other rls config. So just copy what's there for other configurations and conditionally enable the -Aclippy

[alexheretic]

Isn't that what we already have? Currently we get -Aclippy when clippy feature is on, but this only actually supplies lints if the project opts in with #![warn(clippy)] & all workspace members also opt-in in the same way.

So what I'm asking is: Is it possible to enable workspace wide clippy lints without having to add #![warn(clippy)] to all projects? Because if it is I'd like to add an Rls config to do just that.

[oli-obk]

That's what I meant. Rls has configuration values supplied by the IDE. We should add one that does not do -Aclippy. Irrelevant of whether the clippy feature is active

[alexheretic]

Clippy has been included with rls for a few days now and seems to work well 🎉 Thanks to everyone that helped put this together!

I do have a couple of issues:

• Client-side I'd like a way to figure out if clippy is enabled before downloading the release. Clippy is not currently mentioned in https://static.rust-lang.org/dist/channel-rust-nightly.toml. Is there somewhere else I can infer this from, or is there an issue I can follow?

• I'd like the following 3 rls config options for clippy

  • Off Clippy is never run
  • Opt-in Clippy is run for crates with #![warn(clippy)] (current behaviour & default)
  • On/Always On Clippy is run for all workspace crates

  Implementing the first two seems trivial, however I'm still not sure how to force on Clippy for all workspace projects.

[oli-obk]

You can keep using #![cfg_attr(cargo-clippy, warn(clippy))] for now. I would suggest to not use any other method, as otherwise nightlies without clippy in rls or building your project without clippy won't work.

Client-side I'd like a way to figure out if clippy is enabled before downloading the release.

That's because there is no way other than looking at https://rust-lang-nursery.github.io/rust-toolstate/ and doing an educated guess

Not sure how to proceed from here atm. I'll bring it up at the all hands next week

[alexheretic]

You can keep using #![cfg_attr(cargo-clippy, warn(clippy))] for now. I would suggest to not use any other method, as otherwise nightlies without clippy in rls or building your project without clippy won't work.

Well I'm using

#![allow(unknown_lints)]
#![warn(clippy)]

but it's annoying to have to add it to all crates in a workspace, though it doesn't break anything. Running cargo clippy already manages to run clippy without this declaration, though I don't think it runs on all workspace members. Are you saying running clippy in On mode for all crates without #![warn(clippy)] in each crate is not possible?

Not sure how to proceed from here atm. I'll bring it up at the all hands next week

Thanks.

[oli-obk]

You can use cargo clippy --all to run on all crates in the workspace

Are you saying running clippy in On mode for all crates without #![warn(clippy)] in each crate is not possible?

Oh that's entirely possible and implementable already, essentially just make rls stop emitting -Aclippy if the mode is On. It's just not guaranteed that it'll run and you'll get an unknown lint warning if you are using allow(some_clippy_lint) without cfg_attr.

[alexheretic]

Oh that's entirely possible and implementable already, essentially just make rls stop emitting -Aclippy if the mode is On. It's just not guaranteed that it'll run and you'll get an unknown lint warning if you are using allow(some_clippy_lint) without cfg_attr.

Right! I kinda assumed the -Aclippy enabled clippy :| doh! So -Aclippy means check for opt in?

[oli-obk]

-Aclippy is equivalent to #![allow(clippy)], a later #[warn(clippy)] undoes this change. By default clippy warns, so rls needs to turn it off without On mode.

[alexheretic]

Ok thanks that has entered my thick skull now! :)

[alexheretic]

I think we're in a pretty good place now, with the clippy_preference configuration. However, one point still needs addressing

Client-side I'd like a way to figure out if clippy is enabled before downloading the release. Clippy is not currently mentioned in https://static.rust-lang.org/dist/channel-rust-nightly.toml

This is clear with today's nightly with rls b17d799 2018-04-05, the version of clippy in the toml does not compile. But rls clients have no way to tell that this release lacks clippy functionality.

[oli-obk]

The only way forward that I see there is to also start blocking nightlies on clippy, just like it has been done for rls.

[nrc]

I think we could modify Rustup to include information in the manifest about whether Clippy is there or not, and report that to the RLS client some how (we've talked about adding 'dry run' functionality to Rustup, for example). I think it would be a lot of work though.

We might consider blocking nightly on Clippy, but I think Clippy would need to be a lot more stable first.

[alexheretic]

Being able to discern it from the toml manifest would be enough for me, including the enabled features there perhaps? Obviously ensuring clippy is always enabled also works.

[nrc]

We will be starting to block nightlies on Clippy soon, I think, so I don't think it is worth doing anything else. I believe that is the only thing left to do here

[alexheretic]

Clippy is back in rls for nightly-2018-07-13, which is great. Is there any news on blocking rustc on clippy? Is there a tracker, or any rough eta?

[mati865]

@alexheretic it's already done: rust-lang/rust-central-station#61

I'm not sure if rustup correctly copies clippy binaries to ~/.cargo/bin. It wasn't when I checked one week ago...

[alexheretic]

Excellent news!

…

On Sun, 15 Jul 2018, 23:47 Mateusz Mikuła, ***@***.***> wrote: @alexheretic <https://github.com/alexheretic> it's already done: rust-lang/rust-central-station#61 <rust-lang/rust-central-station#61> I'm not sure if rustup correctly copies clippy binaries to ~/.cargo/bin. It wasn't when I checked one week ago... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#149 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACOT15z9ri7-hjI5qSWAMJNYwnbGdUYqks5uG8ZsgaJpZM4LoN_d> .

[alexheretic]

We can close this then I guess, rls is fully integrated with clippy and should remain so for future releases. Thank you to everyone that worked toward this, look forward to way more clippy issues being raised!