Write a Clippy lint banning sum and product

[michaelsproul]

Description

As identified in #1098, the integer_arithmetic lint doesn't cover the sum and product functions on iterators. It would be good to lint against these so that we don't accidentally reintroduce them after #1620 is merged.

Steps to resolve

We could accomplish this one of two ways:

Specialised Lint (easier)

Write a clippy lint that just checks for sum and product. Nothing generic about it. It could be similar to implementations of lints like unwrap_used, that just looks for these methods, confirms their presence by looking at the types, and raises an error.

See: https://github.com/rust-lang/rust-clippy/blob/master/clippy_lints/src/methods/mod.rs

Something better would be...

Generic Lint (harder)

I don't know how hard this would be, but if we're feeling ambitious we could write a lint to ban arbitrary functions... The invocation syntax could look something like this:

#![deny(clippy::function_calls(
    std::Iterator::sum,
    std::Iterator::product
))]

The implementation would likely be quite gnarly. It could draw inspiration from the method lints linked above, but would have to do the extra work of parsing those string paths from the lint attribute, converting those into types, and checking those types against the context, etc.

[ilknarf]

Hi, can I take a crack at this?

[michaelsproul]

@ilknarf Sure! I was planning to get to it myself eventually because I suspected it would be quite involved, but I'm also happy to mentor you through the process.

Have you worked on a clippy lint before? If not, I'd recommend forking the rust-clippy repo, and following their setup guide here: https://github.com/rust-lang/rust-clippy/blob/master/CONTRIBUTING.md

[ilknarf]

Thanks! Will do just that! Would we need to submit an issue in rust-lang/rust-clippy as well?

[michaelsproul]

Yeah, we could do (I'm happy to do it Monday). I'd be interested to hear if the Clippy devs think the generic lint is feasible

[ilknarf]

OK, I can look more into how to create a lint in the meantime, then.

[ilknarf]

On conducting some initial research, it seems like directly implementing it is pretty straight forward as a `LateLintPass => check Trait => check if method called.

My current idea for approaching the generic implementation would be to use Clippy's toml config for arbitrary functions (they support Vec on the config file for traits/method names), then using the built in AST and context utilities to do the checking.

For parsing, do you think regex matching for . vs :: would be good enough for differentiating between traits and structs, or would it be better to maybe use a map, or to just check for both altogether?

[ilknarf]

I went ahead and created rust-lang/rust-clippy#6073 but I can go ahead and try to update integer_arithmetic if the proposed generic lint doesn't seem super flexible.

[ilknarf]

rust-lang/rust-clippy#6081 Submitted PR here.

[michaelsproul]

@ilknarf, thank you for being amazingly productive and taking the initiative here! I am super impressed and very grateful 😊

[michaelsproul]

Another candidate for banning, the integer abs method: https://doc.rust-lang.org/std/primitive.i64.html#method.abs

We use it in fork choice at the moment