Fix recent clippy errors on master

[thanethomson]

Some new clippy errors have appeared recently on master (most likely due to upgrading to Rust 1.54). This PR aims to fix them.

See https://github.com/informalsystems/tendermint-rs/runs/3242281271

• Referenced an issue explaining the need for the change
• Updated all relevant documentation in docs
• Updated all code comments where relevant
• Wrote tests
• Added entry in .changelog/

[codecov-commenter]

Codecov Report

Merging #941 (a606c1d) into master (09c8454) will increase coverage by 0.1%.
The diff coverage is 92.3%.

@@           Coverage Diff            @@
##           master    #941     +/-   ##
========================================
+ Coverage    70.5%   70.7%   +0.1%     
========================================
  Files         205     205             
  Lines       16470   16433     -37     
========================================
- Hits        11626   11622      -4     
+ Misses       4844    4811     -33     

Impacted Files	Coverage Δ
light-client/src/evidence.rs	50.0% <ø> (ø)
light-client/src/macros.rs	10.3% <ø> (ø)
light-client/src/store/memory.rs	66.6% <ø> (ø)
tendermint/src/chain/id.rs	73.8% <0.0%> (ø)
light-client/src/components/verifier.rs	100.0% <100.0%> (ø)
light-client/src/peer_list.rs	90.3% <100.0%> (ø)
light-client/src/predicates.rs	100.0% <100.0%> (ø)
light-client/src/supervisor.rs	90.6% <100.0%> (ø)
tendermint/src/abci/tag.rs	74.0% <100.0%> (ø)
testgen/src/tester.rs	72.2% <100.0%> (ø)
... and 8 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09c8454...a606c1d. Read the comment docs.

[romac]

Quick question, pre-approving already otherwise

[romac]

Why can't we just return the boxes iterator directly here? I assume you've tried that already, so just curious what's the issue here.

[thanethomson]

Yeah it's a good question. I refactored the code to the following:

    fn all(&self, status: Status) -> Box<dyn Iterator<Item = LightBlock>> {
        let light_blocks = self
            .store
            .iter()
            .filter(|(_, e)| e.status == status)
            .map(|(_, e)| e.light_block.clone());

        Box::new(light_blocks)
    }

Is that what you mean?

If so, this is the error I get from the compiler:

error[E0759]: `self` has an anonymous lifetime `'_` but it needs to satisfy a `'static` lifetime requirement
  --> light-client/src/store/memory.rs:87:14
   |
84 |       fn all(&self, status: Status) -> Box<dyn Iterator<Item = LightBlock>> {
   |              ----- this data with an anonymous lifetime `'_`...
85 |           let light_blocks = self
   |  ____________________________-
86 | |             .store
   | |__________________- ...is captured here...
87 |               .iter()
   |                ^^^^
...
91 |           Box::new(light_blocks)
   |           ---------------------- ...and is required to live as long as `'static` here
   |
help: to declare that the trait object captures data from argument `self`, you can add an explicit `'_` lifetime bound
   |
84 |     fn all(&self, status: Status) -> Box<dyn Iterator<Item = LightBlock> + '_> {
   |                                                                          ^^^^

error: aborting due to previous error

I'm not totally comfortable just yet with the way that these lifetime bounds work (due to lack of experience with using them). Does declaring that the trait object captures data from argument 'self' make sense to you? What does this mean, practically?

[thanethomson]

And if I apply the suggestion, we'll need to modify the trait signature:

error: `impl` item signature doesn't match `trait` item signature
  --> light-client/src/store/memory.rs:84:5
   |
84 |     fn all(&self, status: Status) -> Box<dyn Iterator<Item = LightBlock> + '_> {
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ found `fn(&MemoryStore, types::Status) -> Box<dyn Iterator<Item = types::LightBlock>>`
   | 
  ::: light-client/src/store.rs:48:5
   |
48 |     fn all(&self, status: Status) -> Box<dyn Iterator<Item = LightBlock>>;
   |     ---------------------------------------------------------------------- expected `fn(&MemoryStore, types::Status) -> Box<(dyn Iterator<Item = types::LightBlock> + 'static)>`
   |
   = note: expected `fn(&MemoryStore, types::Status) -> Box<(dyn Iterator<Item = types::LightBlock> + 'static)>`
              found `fn(&MemoryStore, types::Status) -> Box<dyn Iterator<Item = types::LightBlock>>`
   = help: the lifetime requirements from the `impl` do not correspond to the requirements in the `trait`
   = help: verify the lifetime relationships in the `trait` and `impl` between the `self` argument, the other inputs and its output

[tony-iqlusion]

@thanethomson heh, that's a really "fun" syntax it's nearly impossible to discover without assistance from rustc...

That said, why not write it with impl Trait instead of Box + dyn?

    fn all(&self, status: Status) -> impl Iterator<Item = LightBlock> + '_ {
       self
            .store
            .iter()
            .filter(|(_, e)| e.status == status)
            .map(|(_, e)| e.light_block.clone())
    }

[romac]

@tony-iqlusion Because all is a trait method and impl Trait return types in trait methods are not supported (yet?)

[romac]

Sounds like closures borrow the variables they close over by default, even if they are Copy. Moving it into the closure will just copy it into it and should just work(tm)

[romac]

I wonder if we should just return a Vec here rather than an iterator. This code is very suboptimal already (and would deserve a rewrite) so I don't think we mind the extra allocations.

[thanethomson]

I wonder if we should just return a Vec here rather than an iterator.

That probably makes sense, given that we're cloning light blocks anyways. So should we perhaps look at changing the method signature to:

    /// Get an iterator of all light blocks with the given status.
    fn all(&self, status: Status) -> Vec<LightBlock>;

[thanethomson]

For now though, perhaps we should just leave the #[allow(clippy::needless_collect)]? We can fix this in a follow-up PR, as it touches various aspects of the codebase and is a breaking API change.

[thanethomson]

I've opened #943 to follow up on this.