Bikesheds to be done when RfCing clippy

[Manishearth]

At some point I wish to RfC the clippy lints.

One part of this will be figuring out Allow/Warn defaults. It will also be bikeshedding various questionable choices clippy has made.

Feel free to list anything you find bikesheddable here:

• match_ref_pats on if let (is if let &foo = bar okay?) ; "if let &foo = bar" triggers match_ref_pats #532
• Glob enum imports: warn or allow? https://github.com/Manishearth/rust-clippy/pull/581
• Two-branch matches: warn or allow? https://github.com/Manishearth/rust-clippy/pull/579
• shadow_unrelated: warn or allow
• lints for specific libraries (e.g. Regex: Inform user about slow regex! macro #595): yay or nay?
• redundant closure: warn or allow? There is something to be said for the added clarity of knowing how many arguments there are.

[Manishearth]

See also: #541

[llogiq]

Another thing is shadow_unrelated – I consider those programmer lazyness, but I found so many matches in the wild that I haven't found the courage to make it warn by default.

[Manishearth]

😄 I'm 👍 for warning it now if you want, btw, but yeah, it's something that we should list here.

[nagisa]

Two-branch matches: warn or allow?

Obviously allow. That’s purely a programmer/style-preference thing.

Glob enum imports: warn or allow?

Also allow?

fn a() {
     use Enum::*;
     // do a lot with the variants.
}

is pretty common, though I’m not sure if this exact pattern is linted against.

[oli-obk]

@nagisa only glob imports at the module level are linted against. The question is whether to also not lint on pub use Enum::*; since that also seems to be a common pattern for old-style enums.

[oli-obk]

That’s purely a programmer/style-preference thing.

@nagisa I changed the lint to only trigger if the second arm is a block. In that case it's both more vertical space AND more indentation when using a match instead of an if let.

[Manishearth]

I changed the lint to only trigger if the second arm is a block

This is still preference/style based.

[leoyvens]

Allow cyclomatic_complexity, there is no universal answer to how complex is too complex. Allow explicit_iter_loop, &x is more concise but x.iter() is more explicit, not obvious which is more readable.

[mcarton]

When I made a PR to fix all Clippy warnings in Cargo, they asked me to remove my commit about explicit_iter_loop because they preferred x.iter() over &x.

[leoyvens]

toplevel_ref_arg should be allowed for let bindings, it's purely stylistic, let ref x = expr can be more readable than let x = &expr when expr is complex (for example a long match).

[Manishearth]

#1043 is bikesheddable

[llogiq]

Regarding #1043, I defend that !x is not only shorter, but also more idiomatic. I remember x == false from my time as university tutor, often used by students who failed to understand boolean logic. Also ! is more easily grepped than == false (and leaves no questions as to spacing/line breaks/..)

[Manishearth]

https://github.com/Manishearth/rust-clippy/issues/1078

[porglezomp]

Things like #1043 is why I prefer Python's not keyword to !...

[leoyvens]

Undeprecate string_to_string. I think it is unidiomatic to use to_string to clone a String. clone is clearer in intent. Also there are many lints related to clone, using to_string may hide bad practices.

[Manishearth]

I'm not sure if we should undeprecate, but a new lint sounds bikesheddable.

[llogiq]

@alexcrichton prefers .iter()/.iter_mut() to &/&mut,see futures-rs PR#55. I'm a bit torn on this issue (and I'm usually plenty opinionated). Showing newbies that they can use anything that implements IntoIterator in a for loop seems a good idea, is shorter and certainly rustic. On the other hand, the functions are more discoverable, and don't introduce sigils, to which some developers seem to have an aversion.

Perhaps the lint should not warn, but merely suggest that it is possible to iterate over &x instead of x.iter() (E.g. just issue a cx.sess().span_note_without_error(..)). I suspect this would make the lint appear friendlier.

[killercup]

Oh sorry, I didn't see this issue when I created #1192: I'd to make option_map_unwrap_or and option_map_unwrap_or_else allow by default.

[Manishearth]

Default allow/warn for option_map_unwrap_or (see #1192)?

cc @shepmaster

[Manishearth]

(Clippy was RFCd)