Open Manishearth opened 9 years ago
I think our macro checking should be stronger, perhaps with some specific checks to totally ignore derive or even all plugins
Although I don't like it, I think it's fair enough to let people use unwrap
by default.
I'd really like to see how the spans for derive-macros look. Will have to investigate.
This seems to be a rustc bug btw
Our in_external_macro
is fine, but the spans for certain serde impls are broken in Servo. I have a MWE which I'll file a bug with
Even a lint that is allow by default can have some value – if people care enough to use it, e.g. per-project.
Until that gets fixed, keeping the let lint on allow for servo
Btw, I'd love for a way to configure allows/warns dynamically
What do you mean by "dynamically"?
I'm running the "servo" branch of Clippy on servo, btw. Not all lints changed to Allow
should be changed to Allow
in Clippy itself, in some cases it's because Servo is special snowflake-y (like float_cmp -- we have a lot of legit reasons to compare floats), or just because Servo is an old codebase and some lints are noisy (eg the for loop iter ones), but should be turned back to warn when servo fixes them.
Oh, sorry, that was a drive-by comment without context.
Basically, it would be nice if once could specify the matrix of allow/warn/deny when plugin_registrar
is run. In Servo I just run Clippy's plugin_registrar
from our existing plugins module.
Even if there was a way to do this as a Cargo feature it would be fine. The idea is that the set of allow/warn/denys can be changed for an entire codebase, instead of per-lib.
https://github.com/Manishearth/rust-clippy/compare/servo
Feel free to pick up fixes from here as they appear and hoist them to master
I have an open cargo issue here (actually huon filed it then, after I had asked how to do it on StackOverflow).
First batch of Servo improvements. This is a small chunk of overall linted stuff.
https://github.com/servo/servo/pull/7224/files
Full list of warnings (with some filtered out): https://gist.github.com/Manishearth/f127d1353586b8ebb271
Awesome!
Magnificient! I was a bit surprised to see so many needless returns, then again there are probably a lot of C++ coders on the servo team who by necessity switch languages ever so often.
I recently notice that I'm missing parentheses in if expressions when writing Java...
A lot of the needless returns come from the way the specs are written. And for each needless return, there are tons of bare returns, so I think it's just a matter of Servo being a large codebase.
@Manishearth Now that we fixed the float_cmp
-zero false positive and augmented identity_op
, can you post a new warning list? I don't want to clone servo just yet, but I'd be interested in how our profile changes.
Btw. should we change inline_always
to Allow
? I submit that for a newbie, Warn
is correct, but servo isn't exactly newbie code.
Also should we shorten the code snippet in single_match
with ellipsis if it gets larger than e.g. 5 lines of code?
Btw.
lint | occurrences |
---|---|
200 | needless_lifetimes |
62 | single_match |
45 | needless_return |
40 | float_cmp |
23 | toplevel_ref_arg |
21 | linkedlist |
12 | redundant_closure |
8 | let_and_return |
4 | needless_range_loop |
4 | mut_mut |
2 | string_to_string |
2 | collapsible_if |
2 | box_vec |
1 | needless_bool |
426 | TOTAL |
Wouldn't #![allow(inline_always)]
be the right way to say "yes we are not newbies" in Servo?
Unless we have another "newb-only" lint that I have missed, yes.
Btw. I was a bit shocked to find a needless_bool
. This is such a classic newbie mistake that whoever wrote it must've had terrifyingly high levels of either dopamine or adenosine (or some related neurotransmitter). :smile:
In Servo I plan to run it off a branch where the defaults are changed. (Too annoying to pepper allow
across all the crates)
needless_bool
isn't that much a newbie mistake in this case -- the code is heavily commented there so it's mostly okay.
Current run of clippy: https://gist.github.com/Manishearth/4fa159d16375a521871b
I think this is a false positive:
/home/manishearth/Mozilla/servo/target/debug/build/style-b5f07438e1401083/out/properties.rs:9726:13: 9731:14 warning: the loop variable `i` is only used to index `lengths`. Consider using `for item in &lengths` or similar iterators, #[warn(needless_range_loop)] on by default
/home/manishearth/Mozilla/servo/target/debug/build/style-b5f07438e1401083/out/properties.rs:9726 for i in 0..2 {
/home/manishearth/Mozilla/servo/target/debug/build/style-b5f07438e1401083/out/properties.rs:9727 match specified::Length::parse_non_negative(input) {
/home/manishearth/Mozilla/servo/target/debug/build/style-b5f07438e1401083/out/properties.rs:9728 Err(()) => break,
/home/manishearth/Mozilla/servo/target/debug/build/style-b5f07438e1401083/out/properties.rs:9729 Ok(length) => lengths[i] = Some(length),
/home/manishearth/Mozilla/servo/target/debug/build/style-b5f07438e1401083/out/properties.rs:9730 }
/home/manishearth/Mozilla/servo/target/debug/build/style-b5f07438e1401083/out/properties.rs:9731 }
The index isn't used to get, but to put.
The following shows that we may want to tweak our threshold:
/home/manishearth/Mozilla/servo/components/gfx/paint_context.rs:437:44: 437:102 warning: very complex type used. Consider factoring parts into `type` definitions, #[warn(type_complexity)] on by default
/home/manishearth/Mozilla/servo/components/gfx/paint_context.rs:437 -> (Option<(Point2D<f32>, f32)>, Option<(Point2D<f32>, f32)>) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/manishearth/Mozilla/servo/components/gfx/paint_context.rs:437:44: 437:102 help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#type_complexity
That doesn't look to scary, though a type Point1To3D = Option<(Point2D<f32>, f32)>
would simplify the declaration to (Point1To3D, Point1To3D)
.
The shadow lint could probably use a note of the shadowed declaration. I've opened #288 to track this.
Counting lint occurrences again:
occurrences | lint |
---|---|
462 | wrong_self_convention |
179 | needless_lifetimes |
136 | match_ref_pats |
46 | shadow_unrelated |
39 | needless_return |
26 | single_match |
22 | linkedlist |
10 | let_and_return |
7 | should_implement_trait |
4 | mut_mut |
3 | collapsible_if |
3 | needless_range_loop |
3 | redundant_closure |
3 | while_let_loop |
2 | box_vec |
2 | needless_bool |
2 | string_to_string |
2 | type_complexity |
wrong_self_convention
is mostly false positives, that's Servo's naming scheme for those and it's within the autogenerated types (hence the overabundance of lints)
Perhaps span_lint
should have an optional feature (via cargo features) to ignore files matching target/*/build/script-*/out
I think this is a false positive:
Can't that be handled via a mutable iteration?
Anyway, if you want to handle it, replace the regular visitor with ExprUseVisitor
and only lint on rvalues that index with i
.
I'm not sure what the mem_categorization::categorization
of i
in values[i] = foo
is, actually, we should double check. Seems like a cat_interior
, but I'm not sure if it applies to values
or i
.
Bunch of fixes landed (fixing the style
crate, with some pending fixes here).
gist updated. I'll probably continue to tack on fixes as time passes, but feel free to try it out yourself and make a PR (making a PR to my clippyfix branch will also work and keep efforts in one place). Ignore lints in generated files, for now.
(I'll update as I move forward)
in_external_macro
, otherwise it lints on all derived thingies. We should find other lints that are prone to lint onderive
too.option_unwrap_unused
has too many false positives (places where it is known to exist, for example), and in general unwrap seems pervasive throughout Servo and other Rust codebases. I vote we move this to Allow.PRs: https://github.com/servo/servo/pull/7224, https://github.com/servo/servo/pull/7536, https://github.com/servo/servo/pull/7519