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.

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

*: Activate clippy::style lint group #2620

Merged
merged 8 commits into from
May 3, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
2 changes: 1 addition & 1 deletion .cargo/config.toml
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
[alias]
# Temporary solution to have clippy config in a single place until https://github.com/rust-lang/rust-clippy/blob/master/doc/roadmap-2021.md#lintstoml-configuration is shipped.
custom-clippy = "clippy -- -A clippy::type_complexity -A clippy::pedantic -A clippy::style -D warnings"
custom-clippy = "clippy -- -A clippy::type_complexity -A clippy::pedantic -D warnings"
9 changes: 6 additions & 3 deletions core/src/transport/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,9 +150,12 @@ impl Future for DialFuture {
.take()
.expect("Future should not be polled again once complete");
let dial_port = self.dial_port;
match self.sender.start_send((channel_to_send, dial_port)) {
Err(_) => return Poll::Ready(Err(MemoryTransportError::Unreachable)),
Ok(()) => {}
if self
.sender
.start_send((channel_to_send, dial_port))
.is_err()
{
return Poll::Ready(Err(MemoryTransportError::Unreachable));
}

Poll::Ready(Ok(self
Expand Down
7 changes: 2 additions & 5 deletions misc/metrics/src/gossipsub.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,8 @@ impl Metrics {

impl super::Recorder<libp2p_gossipsub::GossipsubEvent> for super::Metrics {
fn record(&self, event: &libp2p_gossipsub::GossipsubEvent) {
match event {
libp2p_gossipsub::GossipsubEvent::Message { .. } => {
self.gossipsub.messages.inc();
}
_ => {}
if let libp2p_gossipsub::GossipsubEvent::Message { .. } = event {
self.gossipsub.messages.inc();
}
}
}
22 changes: 11 additions & 11 deletions protocols/gossipsub/src/behaviour.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1226,19 +1226,19 @@ where
}

for id in ids {
if !self.duplicate_cache.contains(&id) && !self.pending_iwant_msgs.contains(&id) {
if self
if !self.duplicate_cache.contains(&id)
&& !self.pending_iwant_msgs.contains(&id)
&& self
.peer_score
.as_ref()
.map(|(_, _, _, promises)| !promises.contains(&id))
.unwrap_or(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is quite the monster boolean condition now. Perhaps we should extract 3 boolean variables that we can then chain together to make this more readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For readability, it would make sense, but it would kill the possibility of short-circuiting, which may result in a performance drop.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess functions would allow for readability and short-circuiting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, a lambda would do the trick! For the first two conditions I don't think it would help much, i.e. !self.duplicate_cache.contains(&id) would most likely become duplicate_cache.contains(&id). Is it fine for you?

The last one is definitely a monster though and I'd be happy to turn it into a short lambda. Any suggestions on a meaningful name for it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably even better would be to do:

impl Gossipsub {
	fn want_message(&self, id: ???) -> bool {
		if self.duplicate_cache.contains(&id) {
			return false;
		}

		if self.pending_iwant_msgs.contains(&id) {
			return false;
		}

		self.peer_score
     		  .as_ref()
            .map(|(_, _, _, promises)| !promises.contains(&id))
            .unwrap_or(true)
	}
}

Although I think clippy might complain about this one being possible to write as a && chain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I went ahead with similar change. I was a bit confused with a contains method being mutable for no visible reason so I made them immutable. Maybe pedantic clippy would have caught that before.

{
// have not seen this message and are not currently requesting it
if iwant_ids.insert(id) {
// Register the IWANT metric
if let Some(metrics) = self.metrics.as_mut() {
metrics.register_iwant(&topic);
}
{
// have not seen this message and are not currently requesting it
if iwant_ids.insert(id) {
// Register the IWANT metric
if let Some(metrics) = self.metrics.as_mut() {
metrics.register_iwant(&topic);
}
}
}
Expand Down Expand Up @@ -1353,7 +1353,7 @@ where
} else if let Some(m) = self.metrics.as_mut() {
// Sending of messages succeeded, register them on the internal metrics.
for topic in topics.iter() {
m.msg_sent(&topic, msg_bytes);
m.msg_sent(topic, msg_bytes);
}
}
}
Expand Down Expand Up @@ -2136,7 +2136,7 @@ where
for peer_id in self.connected_peers.keys() {
scores
.entry(peer_id)
.or_insert_with(|| peer_score.metric_score(&peer_id, self.metrics.as_mut()));
.or_insert_with(|| peer_score.metric_score(peer_id, self.metrics.as_mut()));
}
}

Expand Down
2 changes: 1 addition & 1 deletion protocols/gossipsub/src/mcache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ impl MessageCache {
message.validated = true;
// Clear the known peers list (after a message is validated, it is forwarded and we no
// longer need to store the originating peers).
let originating_peers = std::mem::replace(known_peers, HashSet::new());
let originating_peers = std::mem::take(known_peers);
(&*message, originating_peers)
})
}
Expand Down
4 changes: 1 addition & 3 deletions protocols/gossipsub/src/metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -517,9 +517,7 @@ impl Metrics {
let metric = self
.peers_per_protocol
.get_or_create(&ProtocolLabel { protocol: kind });
if metric.get() == 0 {
return;
} else {
if metric.get() != 0 {
// decrement the counter
metric.set(metric.get() - 1);
}
Expand Down
2 changes: 1 addition & 1 deletion protocols/gossipsub/src/peer_score.rs
Original file line number Diff line number Diff line change
Expand Up @@ -860,7 +860,7 @@ impl PeerScore {
//should always be true
let window_time = validated_time
.checked_add(topic_params.mesh_message_deliveries_window)
.unwrap_or_else(|| *now);
.unwrap_or(*now);
if now > &window_time {
falls_in_mesh_deliver_window = false;
}
Expand Down
24 changes: 11 additions & 13 deletions protocols/identify/src/identify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,16 +196,14 @@ impl Identify {
I: IntoIterator<Item = PeerId>,
{
for p in peers {
if self.pending_push.insert(p) {
if !self.connected.contains_key(&p) {
let handler = self.new_handler();
self.events.push_back(NetworkBehaviourAction::Dial {
opts: DialOpts::peer_id(p)
.condition(dial_opts::PeerCondition::Disconnected)
.build(),
handler,
});
}
if self.pending_push.insert(p) && !self.connected.contains_key(&p) {
let handler = self.new_handler();
self.events.push_back(NetworkBehaviourAction::Dial {
opts: DialOpts::peer_id(p)
.condition(dial_opts::PeerCondition::Disconnected)
.build(),
handler,
});
}
}
}
Expand Down Expand Up @@ -240,7 +238,7 @@ impl NetworkBehaviour for Identify {
if let Some(entry) = self.discovered_peers.get_mut(peer_id) {
for addr in failed_addresses
.into_iter()
.flat_map(|addresses| addresses.into_iter())
.flat_map(|addresses| addresses.iter())
{
entry.remove(addr);
}
Expand Down Expand Up @@ -451,7 +449,7 @@ impl NetworkBehaviour for Identify {
self.discovered_peers
.get(peer)
.cloned()
.map(|addr| Vec::from_iter(addr))
.map(Vec::from_iter)
.unwrap_or_default()
}
}
Expand Down Expand Up @@ -510,7 +508,7 @@ fn multiaddr_matches_peer_id(addr: &Multiaddr, peer_id: &PeerId) -> bool {
if let Some(Protocol::P2p(multi_addr_peer_id)) = last_component {
return multi_addr_peer_id == *peer_id.as_ref();
}
return true;
true
}

#[cfg(test)]
Expand Down
5 changes: 4 additions & 1 deletion protocols/kad/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@

- Derive `Error` for `GetRecordError` (see [PR 2614]).

[pr 2614]: https://github.com/libp2p/rust-libp2p/pull/2614
- Renamed `PeriodicJob::is_ready` to `PeriodicJob::check_ready`. See [PR 2620].
LesnyRumcajs marked this conversation as resolved.
Show resolved Hide resolved

[PR 2614]: https://github.com/libp2p/rust-libp2p/pull/2614
[PR 2620]: https://github.com/libp2p/rust-libp2p/pull/2620

# 0.36.0

Expand Down
1 change: 1 addition & 0 deletions protocols/kad/src/addresses.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ impl Addresses {
///
/// An address should only be removed if is determined to be invalid or
/// otherwise unreachable.
#[allow(clippy::result_unit_err)]
pub fn remove(&mut self, addr: &Multiaddr) -> Result<(), ()> {
if self.addrs.len() == 1 {
return Err(());
Expand Down
2 changes: 1 addition & 1 deletion protocols/kad/src/behaviour.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1825,7 +1825,7 @@ where
errors: Option<&Vec<Multiaddr>>,
other_established: usize,
) {
for addr in errors.map(|a| a.into_iter()).into_iter().flatten() {
for addr in errors.map(|a| a.iter()).into_iter().flatten() {
self.address_failed(*peer_id, addr);
}

Expand Down
10 changes: 5 additions & 5 deletions protocols/kad/src/jobs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ impl<T> PeriodicJob<T> {

/// Returns `true` if the job is currently not running but ready
/// to be run, `false` otherwise.
fn is_ready(&mut self, cx: &mut Context<'_>, now: Instant) -> bool {
fn check_ready(&mut self, cx: &mut Context<'_>, now: Instant) -> bool {
if let PeriodicJobState::Waiting(delay, deadline) = &mut self.state {
if now >= *deadline || !Future::poll(Pin::new(delay), cx).is_pending() {
return true;
Expand Down Expand Up @@ -195,7 +195,7 @@ impl PutRecordJob {
where
for<'a> T: RecordStore<'a>,
{
if self.inner.is_ready(cx, now) {
if self.inner.check_ready(cx, now) {
let publish = self.next_publish.map_or(false, |t_pub| now >= t_pub);
let records = store
.records()
Expand Down Expand Up @@ -239,7 +239,7 @@ impl PutRecordJob {
let deadline = now + self.inner.interval;
let delay = Delay::new(self.inner.interval);
self.inner.state = PeriodicJobState::Waiting(delay, deadline);
assert!(!self.inner.is_ready(cx, now));
assert!(!self.inner.check_ready(cx, now));
}

Poll::Pending
Expand Down Expand Up @@ -296,7 +296,7 @@ impl AddProviderJob {
where
for<'a> T: RecordStore<'a>,
{
if self.inner.is_ready(cx, now) {
if self.inner.check_ready(cx, now) {
let records = store
.provided()
.map(|r| r.into_owned())
Expand All @@ -317,7 +317,7 @@ impl AddProviderJob {
let deadline = now + self.inner.interval;
let delay = Delay::new(self.inner.interval);
self.inner.state = PeriodicJobState::Waiting(delay, deadline);
assert!(!self.inner.is_ready(cx, now));
assert!(!self.inner.check_ready(cx, now));
}

Poll::Pending
Expand Down
10 changes: 5 additions & 5 deletions protocols/kad/src/kbucket/bucket.rs
Original file line number Diff line number Diff line change
Expand Up @@ -365,11 +365,11 @@ where
// Adjust `first_connected_pos` accordingly.
match status {
NodeStatus::Connected => {
if self.first_connected_pos.map_or(false, |p| p == pos.0) {
if pos.0 == self.nodes.len() {
// It was the last connected node.
self.first_connected_pos = None
}
if self.first_connected_pos.map_or(false, |p| p == pos.0)
&& pos.0 == self.nodes.len()
{
// It was the last connected node.
self.first_connected_pos = None
}
}
NodeStatus::Disconnected => {
Expand Down
6 changes: 3 additions & 3 deletions protocols/kad/src/kbucket/key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,9 @@ impl<T> Key<T> {
}
}

impl<T> Into<KeyBytes> for Key<T> {
fn into(self) -> KeyBytes {
self.bytes
impl<T> From<Key<T>> for KeyBytes {
fn from(key: Key<T>) -> KeyBytes {
key.bytes
}
}

Expand Down
4 changes: 2 additions & 2 deletions protocols/mdns/src/behaviour.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ impl NetworkBehaviour for Mdns {

fn inject_new_listen_addr(&mut self, _id: ListenerId, _addr: &Multiaddr) {
log::trace!("waking interface state because listening address changed");
for (_, iface) in &mut self.iface_states {
for iface in self.iface_states.values_mut() {
iface.fire_timer();
}
}
Expand Down Expand Up @@ -178,7 +178,7 @@ impl NetworkBehaviour for Mdns {
}
// Emit discovered event.
let mut discovered = SmallVec::<[(PeerId, Multiaddr); 4]>::new();
for (_, iface_state) in &mut self.iface_states {
for iface_state in self.iface_states.values_mut() {
while let Some((peer, addr, expiration)) = iface_state.poll(cx, params) {
if let Some((_, _, cur_expires)) = self
.discovered_nodes
Expand Down
6 changes: 6 additions & 0 deletions protocols/ping/src/handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,12 @@ impl Config {
}
}

impl Default for Config {
fn default() -> Self {
Self::new()
}
}

/// The successful result of processing an inbound or outbound ping.
#[derive(Debug)]
pub enum Success {
Expand Down
2 changes: 1 addition & 1 deletion protocols/relay/src/v2/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ impl NetworkBehaviour for Client {
),
};

return Poll::Ready(action);
Poll::Ready(action)
}
}

Expand Down
8 changes: 4 additions & 4 deletions protocols/relay/src/v2/client/transport.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ impl Transport for ClientTransport {
// traversal. One would coordinate such traversal via a previously
// established relayed connection, but never using a relayed connection
// itself.
return Err(TransportError::MultiaddrNotSupported(addr));
Err(TransportError::MultiaddrNotSupported(addr))
}

fn address_translation(&self, _server: &Multiaddr, _observed: &Multiaddr) -> Option<Multiaddr> {
Expand Down Expand Up @@ -244,20 +244,20 @@ fn parse_relayed_multiaddr(
if before_circuit {
before_circuit = false;
} else {
Err(RelayError::MultipleCircuitRelayProtocolsUnsupported)?;
return Err(RelayError::MultipleCircuitRelayProtocolsUnsupported.into());
}
}
Protocol::P2p(hash) => {
let peer_id = PeerId::from_multihash(hash).map_err(|_| RelayError::InvalidHash)?;

if before_circuit {
if relayed_multiaddr.relay_peer_id.is_some() {
Err(RelayError::MalformedMultiaddr)?;
return Err(RelayError::MalformedMultiaddr.into());
}
relayed_multiaddr.relay_peer_id = Some(peer_id)
} else {
if relayed_multiaddr.dst_peer_id.is_some() {
Err(RelayError::MalformedMultiaddr)?;
return Err(RelayError::MalformedMultiaddr.into());
}
relayed_multiaddr.dst_peer_id = Some(peer_id)
}
Expand Down
4 changes: 3 additions & 1 deletion protocols/relay/src/v2/protocol/inbound_hop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,9 @@ impl upgrade::InboundUpgrade<NegotiatedSubstream> for Upgrade {
.map_err(|_| FatalUpgradeError::ParsePeerId)?;
Req::Connect(CircuitReq { dst, substream })
}
hop_message::Type::Status => Err(FatalUpgradeError::UnexpectedTypeStatus)?,
hop_message::Type::Status => {
return Err(FatalUpgradeError::UnexpectedTypeStatus.into())
}
};

Ok(req)
Expand Down
2 changes: 1 addition & 1 deletion protocols/relay/src/v2/protocol/inbound_stop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ impl upgrade::InboundUpgrade<NegotiatedSubstream> for Upgrade {
limit: limit.map(Into::into),
})
}
stop_message::Type::Status => Err(FatalUpgradeError::UnexpectedTypeStatus)?,
stop_message::Type::Status => Err(FatalUpgradeError::UnexpectedTypeStatus.into()),
}
}
.boxed()
Expand Down
Loading