Findings from running Clippy on Servo

[Manishearth]

(I'll update as I move forward)

• float_cmp/single_match needs in_external_macro, otherwise it lints on all derived thingies. We should find other lints that are prone to lint on derive 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.
• let_unit_value also has a problem with derives, even though it uses in_macro. Will investigate

PRs: servo/servo#7224, servo/servo#7536, servo/servo#7519

[Manishearth]

I think our macro checking should be stronger, perhaps with some specific checks to totally ignore derive or even all plugins

[birkenfeld]

Although I don't like it, I think it's fair enough to let people use unwrap by default.

[llogiq]

I'd really like to see how the spans for derive-macros look. Will have to investigate.

[Manishearth]

This seems to be a rustc bug btw

[Manishearth]

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

[llogiq]

Even a lint that is allow by default can have some value – if people care enough to use it, e.g. per-project.

[Manishearth]

rust-lang/rust#27817

[Manishearth]

Until that gets fixed, keeping the let lint on allow for servo

[Manishearth]

Btw, I'd love for a way to configure allows/warns dynamically

[birkenfeld]

What do you mean by "dynamically"?

[Manishearth]

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.

[Manishearth]

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.

[Manishearth]

https://github.com/Manishearth/rust-clippy/compare/servo

Feel free to pick up fixes from here as they appear and hoist them to master

[llogiq]

I have an open cargo issue here (actually huon filed it then, after I had asked how to do it on StackOverflow).

[Manishearth]

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

[birkenfeld]

Awesome!

[llogiq]

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...

[Manishearth]

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.

[llogiq]

@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

[birkenfeld]

Wouldn't #![allow(inline_always)] be the right way to say "yes we are not newbies" in Servo?

[llogiq]

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). 😄

[Manishearth]

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.

[Manishearth]

Current run of clippy: https://gist.github.com/Manishearth/4fa159d16375a521871b

[llogiq]

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

[Manishearth]

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

[Manishearth]

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.

[Manishearth]

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.