Make clippy happier #57
Conversation
as it is no different than the default one.
clippy::or_fun_call for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#or_fun_call
clippy::op_ref
| @@ -58,7 +58,7 @@ impl SyntaxText { | |||
|
|
|||
| pub fn slice<R: private::SyntaxTextRange>(&self, range: R) -> SyntaxText { | |||
| let start = range.start().unwrap_or_default(); | |||
| let end = range.end().unwrap_or(self.len()); | |||
| let end = range.end().unwrap_or_else(|| self.len()); | |||
| @@ -96,6 +96,7 @@ impl SyntaxText { | |||
|
|
|||
| pub fn for_each_chunk<F: FnMut(&str)>(&self, mut f: F) { | |||
| enum Void {} | |||
| #[allow(clippy::unit_arg)] | |||
There was a problem hiding this comment.
I am pretty 👎 on silencing clippy lints in code. I think this is a point where it clearly becomes a hindrance rather than a helper.
There was a problem hiding this comment.
I don't know if I take a fully opposite position, but I do see value in locally allowing lints.
For one, it's explicitly saying "yes, this may be weird, but it's intended."
Secondly, and more importantly to me, is that a linter is most useful when it's silent. Warnings are a lot more useful when you only have a few and plan to address them all. Not suppressing false positives is how you get warning fatigue and ignoring new lints.
Anecdotal case in point is that the 10 or so clippy warnings this PR fixes were already enough to make me change my check workflow when working with rowan from clippy to check. With multiple warnings, some of which I have to ignore, it makes it harder to find the ones that are an improvement to fix.
In another formulation, imho, the lack of silencing the clippy lint harms clippy's usefulness more than some #[allow]s get in the way of reading code. (In fact, I think they have a (minor) positive that approximately pays for the vertical space they occupy.)
The TL;DR is that clippy has false positives, as a consequence of its design. The options are either to allow some lints sometimes or allow the false positives to overwhelm any useful information the linter provides.
That said, I'd be fine yeeting the two problematic commits and either globally allowing those lints or just stopping trying to use clippy on rowan, depending on your preference.
There was a problem hiding this comment.
This is going to get a tad too philosophical, so bear with me.... (but the TL;DR is that I don't find clippy useful in this context).
I feel that the general "use linters" advice is too religiously carried over to Rust. In languages like TypeScript, for example, you basically cannot exist without linter, as even the simplest things, like "did I just drop this promise on the floor, really?" are not checked by the compiler. Basically, linterns tend to paper over compiler deficiencies.
In Rust, however, everything really important is already handled by compiler. For a user, who is already familiar with Rust idioms, majority of clippy suggestions are just a wash. Like in this PR:
- removing
intos is great (and should really be moved into the compiler) - removing
fold's is great (and probably should be moved into the compiler as well) - the nth -> next change is negative, in my opinion. I've specifically used nth repeatedly, as it seems clearer for the example. It's not a big deal, it's just arbitrary change.
- removal of explicit lifetime: it's, of course, not bad but it doesn't make the code better. Like, the next time I see it, i'll fix it. If I don't see it, than ok, I haven't spend my time on change.
unwrap_or_elseis, again, slightly negative change, but the differences in utility is tiny in comparisong to the fact that this is a change.- the change in tests is pretty negative. It adds two new and (what's worse) two new names.
The overall theme here is that clippy makes me change my code, without making it better. It creates noise, and I think that noise has a pretty high cost. Similarly, explicit allows create noise. They don't show "something's fishy is going on" (b/c these are caught by compiler), they disproportionally highlight code trivialities.
What I can get behind is conservative clippy profile, consisting of a lints which are candidates for inclusion in the compiler. IIRC, there's no such clippy profile yet. What we do in rust-analyzer is that we define, globally, the list of disabled lints:
But we still don't run clippy on CI, as I find that would be mostly waste of time.
Where clippy shines is when teaching new users Rust idioms, but this use case is different.
I guess the bottom line is that I am 👎 on running clippy on CI, and, for this reason in particular, I don't want to see anything just to appease clippy in the code.
There was a problem hiding this comment.
Chiming in from Clippy: The Clippy philosophy is that sprinkling of allows is encouraged, for the reason @CAD97 already stated: "The following code is intentional."
I think disabling clippy::style and clippy::complexity lints already gets pretty close to what you want from Clippy. The clippy::correctness group exists for lints, that catch faulty code and I would recommend to everyone to at least use that lint group in every project (But I'm obviously biased).
| @@ -111,7 +111,7 @@ impl<I: Iterator<Item = (SyntaxKind, SmolStr)>> Parser<I> { | |||
| } | |||
|
|
|||
| fn print(indent: usize, element: SyntaxElement) { | |||
| let kind: SyntaxKind = element.kind().into(); | |||
| let kind: SyntaxKind = element.kind(); | |||
There was a problem hiding this comment.
| let kind: SyntaxKind = element.kind(); | |
| let kind = element.kind(); |
10440: Fix Clippy warnings and replace some `if let`s with `match` r=Veykril a=arzg I decided to try fixing a bunch of Clippy warnings. I am aware of this project’s opinion of Clippy (I have read both [rust-lang/clippy#5537](rust-lang/rust-clippy#5537) and [rust-analyzer/rowan#57 (comment)](rust-analyzer/rowan#57 (comment))), so I totally understand if part of or the entirety of this PR is rejected. In particular, I can see how the semicolons and `if let` vs `match` commits provide comparatively little benefit when compared to the ensuing churn. I tried to separate each kind of change into its own commit to make it easier to discard certain changes. I also only applied Clippy suggestions where I thought they provided a definite improvement to the code (apart from semicolons, which is IMO more of a formatting/consistency question than a linting question). In the end I accumulated a list of 28 Clippy lints I ignored entirely. Sidenote: I should really have asked about this on Zulip before going through all 1,555 `if let`s in the codebase to decide which ones definitely look better as `match` :P Co-authored-by: Aramis Razzaghipour <[email protected]>
Each commit addresses a single clippy lint so each change can easily be reviewed separately.