Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix recent clippy errors on master #941

Merged
merged 3 commits into from
Aug 6, 2021
Merged

Fix recent clippy errors on master #941

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a9b91fc
Fix needless borrows in codebase
thanethomson Aug 3, 2021
fb187e9
Ignore needless collect warning (we do actually seem to need it)
thanethomson Aug 3, 2021
a606c1d
Remove trailing semicolon in macro to fix docs compiling
thanethomson Aug 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions light-client/src/components/verifier.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,8 @@ impl Verifier for ProdVerifier {
&*self.voting_power_calculator,
&*self.commit_validator,
&*self.hasher,
&trusted,
&untrusted,
trusted,
untrusted,
options,
now,
)
Expand Down
2 changes: 1 addition & 1 deletion light-client/src/evidence.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ mod prod {
#[pre(self.peer_map.contains_key(&peer))]
fn rpc_client_for(&self, peer: PeerId) -> Result<rpc::HttpClient, IoError> {
let peer_addr = self.peer_map.get(&peer).unwrap().to_owned();
Ok(rpc::HttpClient::new(peer_addr).map_err(IoError::from)?)
rpc::HttpClient::new(peer_addr).map_err(IoError::from)
}
}
}
2 changes: 1 addition & 1 deletion light-client/src/macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
#[macro_export]
macro_rules! bail {
($kind:expr) => {
return Err($kind.into());
return Err($kind.into())
};
}

Expand Down
4 changes: 2 additions & 2 deletions light-client/src/peer_list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ impl<T> PeerList<T> {
/// - The given peer id must not be the primary peer id.
/// - The given peer must be in the witness list
#[pre(faulty_witness != self.primary && self.witnesses.contains(&faulty_witness))]
#[post(Self::invariant(&self))]
#[post(Self::invariant(self))]
pub fn replace_faulty_witness(&mut self, faulty_witness: PeerId) -> Option<PeerId> {
let mut result = None;

Expand All @@ -137,7 +137,7 @@ impl<T> PeerList<T> {
///
/// ## Errors
/// - If there are no witness left, returns `ErrorKind::NoWitnessLeft`.
#[post(ret.is_ok() ==> Self::invariant(&self))]
#[post(ret.is_ok() ==> Self::invariant(self))]
pub fn replace_faulty_primary(
&mut self,
primary_error: Option<Error>,
Expand Down
6 changes: 3 additions & 3 deletions light-client/src/predicates.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,10 +240,10 @@ pub fn verify(
vp.is_header_from_past(&untrusted.signed_header.header, options.clock_drift, now)?;

// Ensure the header validator hashes match the given validators
vp.validator_sets_match(&untrusted, &*hasher)?;
vp.validator_sets_match(untrusted, &*hasher)?;

// Ensure the header next validator hashes match the given next validators
vp.next_validators_match(&untrusted, &*hasher)?;
vp.next_validators_match(untrusted, &*hasher)?;

// Ensure the header matches the commit
vp.header_matches_commit(&untrusted.signed_header, hasher)?;
Expand All @@ -266,7 +266,7 @@ pub fn verify(
if untrusted.height() == trusted_next_height {
// If the untrusted block is the very next block after the trusted block,
// check that their (next) validator sets hashes match.
vp.valid_next_validator_set(&untrusted, trusted)?;
vp.valid_next_validator_set(untrusted, trusted)?;
} else {
// Otherwise, ensure that the untrusted block has a greater height than
// the trusted block.
Expand Down
1 change: 1 addition & 0 deletions light-client/src/store/memory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ impl LightStore for MemoryStore {
.map(|(_, e)| e.light_block.clone())
}

#[allow(clippy::needless_collect)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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())
    }

Copy link
Member

@romac romac Aug 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fn all(&self, status: Status) -> Box<dyn Iterator<Item = LightBlock>> {
let light_blocks: Vec<_> = self
.store
Expand Down
2 changes: 1 addition & 1 deletion light-client/src/supervisor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ impl Supervisor {
.collect();

self.fork_detector
.detect_forks(verified_block, &trusted_block, witnesses)
.detect_forks(verified_block, trusted_block, witnesses)
}

/// Run the supervisor event loop in the same thread.
Expand Down
2 changes: 1 addition & 1 deletion tendermint/src/abci/tag.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ mod test {
#[test]
fn tag_serde() {
let json = r#"{"key": "cGFja2V0X3RpbWVvdXRfaGVpZ2h0", "value": "MC00ODQw"}"#;
let tag: Tag = serde_json::from_str(&json).unwrap();
let tag: Tag = serde_json::from_str(json).unwrap();
assert_eq!("packet_timeout_height", tag.key.0);
assert_eq!("0-4840", tag.value.0);
}
Expand Down
2 changes: 1 addition & 1 deletion tendermint/src/chain/id.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ impl Id {

/// Get the chain ID as a raw bytes.
pub fn as_bytes(&self) -> &[u8] {
&self.0.as_str().as_bytes()
self.0.as_str().as_bytes()
}
}

Expand Down
6 changes: 3 additions & 3 deletions testgen/src/tester.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ impl Tester {
T: 'static + DeserializeOwned + UnwindSafe,
F: Fn(T) + UnwindSafe + RefUnwindSafe + 'static,
{
let test_fn = move |_path: &str, input: &str| match parse_as::<T>(&input) {
let test_fn = move |_path: &str, input: &str| match parse_as::<T>(input) {
Ok(test_case) => Tester::capture_test(|| {
test(test_case);
}),
Expand All @@ -288,7 +288,7 @@ impl Tester {
{
let test_env = self.env().unwrap();
let output_env = self.output_env().unwrap();
let test_fn = move |path: &str, input: &str| match parse_as::<T>(&input) {
let test_fn = move |path: &str, input: &str| match parse_as::<T>(input) {
Ok(test_case) => Tester::capture_test(|| {
// It is OK to unwrap() here: in case of unwrapping failure, the test will fail.
let dir = TempDir::new().unwrap();
Expand All @@ -311,7 +311,7 @@ impl Tester {
T: 'static + DeserializeOwned,
F: Fn(T) -> Vec<(String, String)> + 'static,
{
let batch_fn = move |_path: &str, input: &str| match parse_as::<T>(&input) {
let batch_fn = move |_path: &str, input: &str| match parse_as::<T>(input) {
Ok(test_batch) => Some(batch(test_batch)),
Err(_) => None,
};
Expand Down
2 changes: 1 addition & 1 deletion testgen/src/validator_set.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ impl Generator<validator::Set> for ValidatorSet {
}

fn generate(&self) -> Result<validator::Set, SimpleError> {
let vals = generate_validators(&self.validators.as_ref().unwrap())?;
let vals = generate_validators(self.validators.as_ref().unwrap())?;
Ok(validator::Set::without_proposer(vals))
}
}
Expand Down