New stack macro implementation triggers clippy::toplevel_ref_arg warning

[dsemi]

This line triggers the clippy::toplevel_ref_arg warning for users of stack!.

Example warning:

warning: `ref` on an entire `let` pattern is discouraged, take a reference with `&` instead
  --> src/main.rs:35:15
   |
35 |       let rhs = stack![ bp.cross(&bv) - ap.cross(&av);
   |  _______________^
36 | |                       cp.cross(&cv) - ap.cross(&av) ];
   | |_____________________________________________________^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#toplevel_ref_arg
   = note: `#[warn(clippy::toplevel_ref_arg)]` on by default
   = note: this warning originates in the macro `stack` (in Nightly builds, run with -Z macro-backtrace for more info)

[Andlon]

Uhm, does clippy normally give warnings from code generated by macros in external crates? That seems... excessive? Did you enable any particular options?

[dsemi]

It seems so: rust-lang/rust-clippy#702. I don't understand why that decision was made.

[Andlon]

This comment suggests that it's due to our use of spans in the stack! macro to improve error messages. We should fix this by just plastering #[allow(clippy::all)] in the macro-generated code.

I'm not sure off the top of my head if we can put these attributes on code blocks though (not a function/impl block).

[danieleades]

We should fix this by just plastering #[allow(clippy::all)] in the macro-generated code.

I would absolutely not do that. That will suppress all future clippy lints, some of which might be useful.

i think the (reasonable) options are:

1. address the lint
2. suppress specifically this lint in the smallest possible scope
3. suppress all lints, but have CI in this repo that would catch new clippy warnings in similar code

I'm not even sure how option 3. would work to be honest.

[Andlon]

@danieleades: appreciate your input. However, in my 8 years of working with Rust, I've never seen clippy actually point out a real problem or bug, but I've seen plenty of energy and time spent by maintainers and contributors fixing clippy complaints. I don't consider it likely that clippy will uncover an actual bug in the future inside this generated code. I personally find it a better trade-off to just ignore clippy entirely within the generated code, so we don't land back into the same situation if/when clippy gets a new lint that the generated code may trigger.

[danieleades]

I've never seen clippy actually point out a real problem or bug

I absolutely have. Though generally related to (ab)use of static items, synchronisation primitives, etc. There's nothing like that in this code so I definitely take your point

I've seen plenty of energy and time spent by maintainers and contributors fixing clippy complaints

yep.

I had a look at the code for stack! and it's not doing anything wild, so while suppressing all lints is an awfully big hammer, it's possibly justifiable here