lints: Add extra clippy lints used at Traverse #61

MarijnS95 · 2021-03-08T14:49:19Z

Thanks for providing a good clippy base in #59!

These are the extra lints that we have enabled over at Traverse. They were mostly picked at random a while back by looking at default-allowed lints and is likely incomplete. In addition we have these pending, hoping they become useful one day after fixing all violations:

clippy::clone_on_ref_ptr
clippy::cognitive_complexity
clippy::pattern_type_mismatch
clippy::unimplemented
clippy::use_self

Hope these are helpful to others as they are to others. Some might already be considered, or are disabled due to false-positives elsehwere and should be filtered out, let me know.

repi

Thanks for the suggestions! That was fast from me creating the template :)

Cool to hear / see which additional lints y'all use also!

clippy::clone_on_ref_ptr

I like this one, and been testing with it a bit, unfortunately our code would require a lot of fixups to use it so haven't fully figured out if it would be worth it. Would be a bit afraid also when refactoring code from Rc to Arc to have to change everywhere it is cloned also. Though in practice have almost never changed anything between Rc and Arc so probably not a major challenge :)

Did you find it feels and looks good to have the Arc::clone functions stand out compared to common .clone()?

clippy::cognitive_complexity

Believe this was turned on for a while by default in clippy or maybe we had it on some projects as have seen some very big functions we have with that lint explicitly allowed. Also do feels like a generally good idea lint but can be messy for some codebases that do have a lot of well complex functions that is hard to refactor. Think this one isn't that popular ("who is Rust to complain about how many parameters my function has?" :) ).

We should give it a go and see what it triggers in our codebases

clippy::pattern_type_mismatch

This one I haven't tried with yet, have you had it find any interesting/good problems?

clippy::unimplemented

Yes this @emilk just today brought up in #60 also and sound like a good idea to enable!

clippy::use_self

We've used this and liked it, but it has a blocking bug that made us disable it and remove it from out standard set. Discussed and tracked in #53 and rust-lang/rust-clippy#4140.

Once that is solved really want to enable it again!

MarijnS95 · 2021-03-08T22:04:26Z

@repi Thanks! Immediately jumped the gun when seeing Embarks lints, knowing that I have been hunting for some of them manually during review when I can let Clippy do the heavy lifting!
Some day I just hope to be able to turn on each and every linter warning, (automatically) temporarily disable everything that actually triggers, and get a nice sorted overview of the number of hits per linting rule. That should make it a lot easier to find and fix potential one-off mishaps (bugs!) while keeping larger (usually readability) refactors for a later date (in our codebase needless_borrow and use_self are prime examples, but not exclusively!).
It is unfortunate to see many sensible and properly working lints default-allow as they would have definitely helped us commit clean and consistent code early on - especially those without known problems - but I guess they'll be flipped to a warn state after more testing and widespread use?

Note that the lints quoted there are prospective linter warnings in our codebase; we want to enable them at some point but have yet to invest actual time fixing their warnings, or decide whether they are worth it at all. They are commented out and as such perhaps irrelevant to have pasted here :)

clippy::clone_on_ref_ptr

I like this one, and been testing with it a bit, unfortunately our code would require a lot of fixups to use it so haven't fully figured out if it would be worth it. Would be a bit afraid also when refactoring code from Rc to Arc to have to change everywhere it is cloned also. Though in practice have almost never changed anything between Rc and Arc so probably not a major challenge :)

Did you find it feels and looks good to have the Arc::clone functions stand out compared to common .clone()?

Same problem here, we have a boatload of Arcs in our codebase and replacing them all would take significant effort. It seems good to distinguish refcount increments from actual data duplication though I would have preferred to have this as method (ie. my_rc.add_ref() or something like that). .clone() is anyway a sensitive subject with .to_string() and .to_owned() around 😬.

clippy::cognitive_complexity

Believe this was turned on for a while by default in clippy or maybe we had it on some projects as have seen some very big functions we have with that lint explicitly allowed. Also do feels like a generally good idea lint but can be messy for some codebases that do have a lot of well complex functions that is hard to refactor.

This points out just one function on our side, one that is indeed far too large with too many responsibilities :).

Think this one isn't that popular ("who is Rust to complain about how many parameters my function has?" :) ).

That's a separate lint, too_many_arguments, right?

clippy::pattern_type_mismatch

This one I haven't tried with yet, have you had it find any interesting/good problems?

This one is a no-go for me. I have enjoyed patterns like if let Some(...) = &foo.bar but clippy insists to use if let Some(ref ...) = foo.bar instead; and worse for match blocks with many enums being unpacked. Iirc that isn't actually the desired way to write matches, right?

clippy::unimplemented

Yes this @emilk just today brought up in #60 also and sound like a good idea to enable!

Tough one for us as well since we're quite heavy on the prototype side, and that would hamper our trunk-based development a bit (get code reviewed and merged as quick as possible, inserting unimplemented!() stubs in yet-unused bits is fine as long as they are not actively invoked). Not sure why we have it listed in the first place 😬

clippy::use_self

We've used this and liked it, but it has a blocking bug that made us disable it and remove it from out standard set. Discussed and tracked in #53 and rust-lang/rust-clippy#4140.

Once that is solved really want to enable it again!

We'll probably enable this and do a pre-pass to fix all the violations (of which there somehow slipped in a lot... 👀), in hopes of not being bitten by one of those false-positives: we don't have much if any complicated trait + type code yet.

Final note: we're only enabling these lints for our core library so far, all applications using it are excluded. Really looking forward to a shareable (or workspace-global-enabled) config file, we're not ready to copypaste this pretty massive list of extra clippy lints to each and every (small or large) project just yet - and keep it synchronized 😞

MarijnS95 · 2021-03-08T22:09:32Z

Had another look through the Clippy allow-list and while there's definitely a lot of interesting stuff that still needs my attention, that would take at least two full days to sift through. These two stood out though, and have been pushed into this PR:

clippy::trivially_copy_pass_by_ref has been haunting me for a while, especially in &self on methods on integer-like enums. There it is much clearer to pass the value by value instead of by reference. Did need some more & matches and * derefs in places though.
Unfortunately there appears to be some mismatch between 32- and 64-bit target architectures, enable with caution.
clippy::map_err_ignore: We had the .map_err(|_| anyhow::anyhow!("some text")) pattern in a few places, loosing all error information while trying to attach more context. Fortunately anyhow has a function doing just that: Context::context and Context::with_context, turning this into .context("some text") and .with_context(|| format!("foo {}", foo)) respectively.
EDIT: Unfortunately it does not catch Err(_) => Err(...) 😞

repi · 2021-03-08T22:18:02Z

Oh think I read your PR description wrong, that the ones you listed there were the ones you wanted to add, but it was the opposite :)

repi · 2021-03-08T22:25:22Z

lints.rs

@@ -13,36 +13,45 @@
    clippy::exit,
    clippy::explicit_into_iter_loop,
    clippy::filter_map_next,
+    clippy::float_cmp_const,


have tested float_cmp_const before, but didn't catch anything in our codebase, and when we don't find a lint catching anything we typically don't enable it. at least not right away.

but do think it can be good lint in general, and would be interesting to test on the rest of our code and see, likely can be added

I don't think we had any triggering this just yet but we want to prevent it from showing up in the future. Chances that a single explicit floating point constant is expected are low.

repi · 2021-03-08T22:26:25Z

lints.rs

    clippy::fn_params_excessive_bools,
+    clippy::get_unwrap,


get_unwrap is also one of those that we have tested but didn't catch any issues in our code, so been lower priority for us. but could be enabled

I don't think it caught anything on our end, but it may have prevented contributors from submitting PRs with this when clippy was checked prior.

Tested it on our codebase again and I'm a bit torn, actually prefer the explicit unwrap for temporary code so one sees that one needs to clean up the unwraps with proper error handling, instead of the more hidden indexing.

We had a case like this for example:

pub fn bounding_box(&self, node: NodeId) -> BoundingBox { let node = self.nodes.get(&node).unwrap();

Which was pretty clear that that unwrap should be replaced with some error handling and like Result to return, or do an explicit .expect("Invalid node ID") // node ID needs to have been validated before or something similar.

Though I also agree that it can be common beginner Rust code to do .get(x).unwrap() a lot. So not fully sure here

In hindsight, yeah that sounds like something we don't want to enable either. Rather we need a lint the other way around to never use unwrap but always expect (can be achieved by allowing expect_used and disallowing unwrap_used), and perhaps disallow direct indexing entirely (clippy::indexing_slicing). But then we have a lot of indexing/slicing cases where we "know" we are in bounds. Some cases could be replaced by chunks/windows though.

It doesn't seem like const_err is properly adhered to either; we're mapping a bunch of winit keys to imgui-rs' Io struct with a constant-sized key_map yet get a warning that every access could potentially be out of bounds 🤔

Ouch, and yeah for some extra sensitive code use the panic & indexing lints also

repi · 2021-03-08T22:28:41Z

lints.rs

    clippy::if_let_mutex,
    clippy::imprecise_flops,
    clippy::inefficient_to_string,
    clippy::let_unit_value,
    clippy::linkedlist,
    clippy::lossy_float_literal,
    clippy::macro_use_imports,
+    clippy::map_err_ignore,


map_err_ignore we've done extensive testing of and as of today switched over all of our code to use, for the same reason you mentioned also of enforcing not loosing error context, or having to drop error context explicitly with a named unused variable (|_err| instead of |_|). so this plan to add to our set!

repi · 2021-03-08T22:29:42Z

lints.rs

    clippy::map_flatten,
    clippy::map_unwrap_or,
    clippy::match_on_vec_items,
    clippy::match_wildcard_for_single_variants,
    clippy::mem_forget,
    clippy::mismatched_target_os,
+    clippy::mutex_integer,


mutex_integer lint has not triggered in our codebases, but it is a simple one and been on the our low-priority list to enable

Seems quite unlikely for this to end up in a codebase, and is usually caught during manual review anyway unless there are very specific requirements that cannot be modeled with a simple atomic.

repi · 2021-03-08T22:30:52Z

lints.rs

    clippy::needless_borrow,
    clippy::needless_continue,
    clippy::option_option,
+    clippy::print_stdout,


have tested with print_stdout and print_stderr lints but for some reason it wasn't detecting many of our println! and eprintln! calls which was surprising. need more investigating and testing before being comfortable / useful enabling it.

Did it seem to work alright for y'all?

Indeed it strangely doesn't seem to propagate into some modules unless manually placing #![warn] on top or #[warn] on the pub mod. I wonder how many lints we are loosing this way, of if just print_std* is affected...

Seems like it may be this bug: rust-lang/rust-clippy#5721

repi · 2021-03-08T22:31:28Z

lints.rs

    clippy::pub_enum_variant_names,
    clippy::ref_option_ref,
    clippy::rest_pat_in_fully_bound_structs,
+    clippy::string_add,


this we just fixed up our code for, which was very simple, will be part of v0.3 set most likely (together with string_add_assign: #59

repi · 2021-03-08T22:32:45Z

lints.rs

    clippy::string_to_string,
    clippy::suboptimal_flops,
    clippy::todo,
+    clippy::trivially_copy_pass_by_ref,


trivially_copy_pass_by_ref can catch some good small optimizations here and there, but as it want switches from references to values that can be a bit intrusive in places so not fully sure it is a good idea to generally enforce.

It is definitely kicking up some dust here and might be disruptive for smaller structs, but the win (readability-wise) for simple enums and integers is worth it IMO. It also helps us catch many Vec<> moves where a simple slice or even iterator could have been accepted.

It has also found a .clone() on a simple enum because that wasn't marked Copy at first 😬

repi · 2021-03-08T22:35:38Z

lints.rs

    clippy::unnested_or_patterns,
    clippy::unused_self,
+    clippy::useless_let_if_seq,


like the idea of useless_let_if_seq but when tested it before it only found like a single case in our code, so wasn't sure if it was really working correctly. has it been useful for y'all?

tested it again, and ran into a bad not compiling suggestion from the lint. so don't think it is mature enough to be enabled generally. rust-lang/rust-clippy#2918 (comment)

It might have found some mishaps in our code but yeah, given the false suggestions this is not safe to be enabled globally yet. That looks gross!

We'll keep it around though as we're not hitting any of those cases.

repi · 2021-03-08T22:36:20Z

lints.rs

    clippy::unnested_or_patterns,
    clippy::unused_self,
+    clippy::useless_let_if_seq,
+    clippy::useless_transmute,


useless_transmute lint I've tested in the past but caught nothing, but been on our low-prio list to enable

We are generally trying to limit our transmutes to safe_transmute so this appears to be unnecessary baggage for us as well. Other (especially FFI-heavy) crates might benefit though.

These are the extra lints that we have enabled over at [Traverse]. They were mostly picked at random a while back by looking at default-allowed lints and is likely incomplete. In addition we have these pending, hoping they become useful one day after fixing all violations: clippy::cognitive_complexity clippy::use_self [Traverse]: https://github.com/Traverse-Research/

MarijnS95 · 2021-03-10T13:26:17Z

Force-pushed this again to fix conflicts, the lints are in limbo now with some discussed over at #59 and some at #60. Once the remainder of these prospective lints that cannot be enabled globally yet are listed in one place (presumably #59) I'll prune this list again :)

repi · 2021-06-16T20:48:26Z

We've release v0.4 of the lint collection which now also includes most that are in here, the only ones I could see that are not included are the following:

clippy::needless_pass_by_value - it is interesting and finds some good stuff, but somewhat controversial style and changes in some case. but will continue evaluate it
clippy::print_stderr / clippy::print_stdout - doesn't fully work
clippy::trivially_copy_pass_by_ref - was a bit too intrusive for us in previous testing as changes function signatures

MarijnS95 requested a review from repi as a code owner March 8, 2021 14:49

repi reviewed Mar 8, 2021

View reviewed changes

MarijnS95 force-pushed the traverse-lints branch from 3bcc790 to 497b960 Compare March 8, 2021 22:04

repi reviewed Mar 8, 2021

View reviewed changes

MarijnS95 force-pushed the traverse-lints branch from 497b960 to 45f8c28 Compare March 9, 2021 09:49

MarijnS95 force-pushed the traverse-lints branch from 45f8c28 to 2c0bc07 Compare March 10, 2021 13:15

repi closed this Jun 16, 2021

MarijnS95 deleted the traverse-lints branch June 15, 2022 10:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lints: Add extra clippy lints used at Traverse #61

lints: Add extra clippy lints used at Traverse #61

MarijnS95 commented Mar 8, 2021

repi left a comment

MarijnS95 commented Mar 8, 2021

MarijnS95 commented Mar 8, 2021 •

edited

Loading

repi commented Mar 8, 2021

repi Mar 8, 2021

MarijnS95 Mar 9, 2021

repi Mar 8, 2021

MarijnS95 Mar 9, 2021

repi Mar 9, 2021

repi Mar 9, 2021 •

edited

Loading

MarijnS95 Mar 10, 2021

repi Mar 10, 2021

repi Mar 8, 2021

repi Mar 8, 2021

MarijnS95 Mar 9, 2021

repi Mar 8, 2021

MarijnS95 Mar 9, 2021

repi Mar 9, 2021

repi Mar 8, 2021

repi Mar 8, 2021

MarijnS95 Mar 9, 2021

MarijnS95 Mar 9, 2021

repi Mar 8, 2021

repi Mar 8, 2021

MarijnS95 Mar 9, 2021 •

edited

Loading

repi Mar 8, 2021

MarijnS95 Mar 9, 2021

MarijnS95 commented Mar 10, 2021

repi commented Jun 16, 2021

lints: Add extra clippy lints used at Traverse #61

lints: Add extra clippy lints used at Traverse #61

Conversation

MarijnS95 commented Mar 8, 2021

repi left a comment

Choose a reason for hiding this comment

MarijnS95 commented Mar 8, 2021

MarijnS95 commented Mar 8, 2021 • edited Loading

repi commented Mar 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

repi Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarijnS95 Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarijnS95 commented Mar 10, 2021

repi commented Jun 16, 2021

MarijnS95 commented Mar 8, 2021 •

edited

Loading

repi Mar 9, 2021 •

edited

Loading

MarijnS95 Mar 9, 2021 •

edited

Loading