rust-lang / rust-analyzer

A Rust compiler front-end for IDEs
https://rust-analyzer.github.io/
Apache License 2.0
14.28k stars 1.61k forks source link

Default lints should correspond to rustc #16628

Open dhardy opened 8 months ago

dhardy commented 8 months ago

Summary

I would expect that rust-analyzer, with default settings, would report the same lints (warnings, errors, etc.) as rustc

Details

Currently this is not the case; for example I also see messages about these:

The default enabled lints also do not correspond to Clippy's default lints (there is a note in the manual about enabling Clippy by default).

Motivation

I would (naively) expect that a project which is clean (reports no lints) under cargo check is also clean under rust-analyzer, and vice-versa. This makes it easier for developers using different tools to collaborate on a project.

Further motivation: the rust-analyzer project should probably not be responsible for choosing which lints are enabled by default (especially when both rustc and clippy already do this).

dhardy commented 8 months ago

Possibly related:

flodiebold commented 8 months ago

These lints are all "WeakWarning" level, which VSCode shows in a very inconspicuous way, just some dots to give a hint you can do something there; the inactive-code is also marked as unused, which makes it be shown still differently, just darkening the code a bit: 2024-02-22-110848_379x393_scrot In general I agree that for warnings/errors we emit that emulate corresponding rustc/clippy ones we should honor their lint settings, and we aren't quite there yet. These hints are a different category though.

dhardy commented 8 months ago

The specification doesn't list a "WeakWarning" level. I think non-standard extensions should be either disabled by default or gated to editors known to support them?

Or is the LSP standard too dated to be useful now (but surely it should also correspond to what VSCode supports since they're both MS products)?

(This should probably be forked to a new issue.)


That very inconspicuous hint sounds fine for inactive-code. Given that this is not a critique of the code but merely a notification of the current configuration, it does make sense that this is part of rust-analyzer and not rustc or Clippy.

The remove-unnecessary-else lint though, I still don't think should be here: it's a pedantic style lint, and thus falls into Clippy's remit.

flodiebold commented 8 months ago

Sorry, we apparently call "WeakWarning" what's called "Hint" on the LSP level. It's not a non-standard extension.

flodiebold commented 8 months ago

I don't know. I think these hints are more intended as "there's a useful thing to do here", not necessarily that there's something wrong. And you might not be using Clippy at all, so I don't think we should only go by what's enabled for Clippy. OTOH we should probably honor if a similar lint is explicitly disabled for Clippy.

dhardy commented 8 months ago

"there's a useful thing to do here"

This is very opinionated (the "useful" part). Your example above (where the else block is empty) is suspicious, and Clippy reports needless_else. cargo check doesn't report this, presumably because it ignores comments and there could be a comment there (which is enough to suppress the Clippy warning). My motivating example had code in the else block, at which point removal of the else is a stylistic change: image

More generally,

There is a counter-argument that certain things should be hinted by rustc if there were a minimally-conspicious way of doing so, and thus rust-analyzer can pick up these missing hints. If that is what you are going for, then these hints should be minimally opinionated.

flodiebold commented 8 months ago

Why would we want to "minimize the difference in experience between those who use [rust-analyzer] and those who don't"? You can always turn off any of these diagnostics using the rust-analyzer.diagnostics.disabled setting, so you can configure the experience to your personal liking.

dhardy commented 8 months ago

Sorry, I mean to minimize the difference in reported lints, so that usage of rust-analyzer is an individual decision, not a project-level decision.