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

Apply clippy suggestions for rustc and core #89709

Merged
merged 3 commits into from
Oct 11, 2021
Merged
Show file tree
Hide file tree
Changes from all 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: 2 additions & 0 deletions compiler/rustc_apfloat/src/ieee.rs
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,7 @@ impl<S: Semantics> fmt::Display for IeeeFloat<S> {
let _: Loss = sig::shift_right(&mut sig, &mut exp, trailing_zeros as usize);

// Change the exponent from 2^e to 10^e.
#[allow(clippy::comparison_chain)]
if exp == 0 {
// Nothing to do.
} else if exp > 0 {
Expand Down Expand Up @@ -2526,6 +2527,7 @@ mod sig {
if *a_sign ^ b_sign {
let (reverse, loss);

#[allow(clippy::comparison_chain)]
if bits == 0 {
reverse = cmp(a_sig, b_sig) == Ordering::Less;
loss = Loss::ExactlyZero;
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_data_structures/src/base_n.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const BASE_64: &[u8; MAX_BASE as usize] =

#[inline]
pub fn push_str(mut n: u128, base: usize, output: &mut String) {
debug_assert!(base >= 2 && base <= MAX_BASE);
debug_assert!((2..=MAX_BASE).contains(&base));
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if we could use debug_assert_matches! (assuming it exists) for range checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

debug_assert_matches! is currently unstable

Copy link
Contributor

Choose a reason for hiding this comment

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

Compiler code can use unstable library features.

Copy link
Member

Choose a reason for hiding this comment

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

@clemenswasser If this PR is re-landed (after the #89905 revert), it would be good to fix this.

let mut s = [0u8; 128];
let mut index = 0;

Expand Down
10 changes: 2 additions & 8 deletions compiler/rustc_data_structures/src/graph/implementation/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,17 +206,11 @@ impl<N: Debug, E: Debug> Graph<N, E> {
AdjacentEdges { graph: self, direction, next: first_edge }
}

pub fn successor_nodes<'a>(
&'a self,
source: NodeIndex,
) -> impl Iterator<Item = NodeIndex> + 'a {
pub fn successor_nodes(&self, source: NodeIndex) -> impl Iterator<Item = NodeIndex> + '_ {
self.outgoing_edges(source).targets()
}

pub fn predecessor_nodes<'a>(
&'a self,
target: NodeIndex,
) -> impl Iterator<Item = NodeIndex> + 'a {
pub fn predecessor_nodes(&self, target: NodeIndex) -> impl Iterator<Item = NodeIndex> + '_ {
self.incoming_edges(target).sources()
}

Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_data_structures/src/graph/iterate/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ fn post_order_walk<G: DirectedGraph + WithSuccessors + WithNumNodes>(
let node = frame.node;
visited[node] = true;

while let Some(successor) = frame.iter.next() {
for successor in frame.iter.by_ref() {
if !visited[successor] {
stack.push(PostOrderFrame { node: successor, iter: graph.successors(successor) });
continue 'recurse;
Expand Down Expand Up @@ -112,7 +112,7 @@ where
/// This is equivalent to just invoke `next` repeatedly until
/// you get a `None` result.
pub fn complete_search(&mut self) {
while let Some(_) = self.next() {}
for _ in self {}
}

/// Returns true if node has been visited thus far.
Expand Down
12 changes: 6 additions & 6 deletions compiler/rustc_data_structures/src/obligation_forest/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ impl<O: ForestObligation> ObligationForest<O> {
.map(|(index, _node)| Error { error: error.clone(), backtrace: self.error_at(index) })
.collect();

self.compress(|_| assert!(false));
self.compress(|_| unreachable!());
errors
}

Expand Down Expand Up @@ -612,7 +612,7 @@ impl<O: ForestObligation> ObligationForest<O> {
fn compress(&mut self, mut outcome_cb: impl FnMut(&O)) {
let orig_nodes_len = self.nodes.len();
let mut node_rewrites: Vec<_> = std::mem::take(&mut self.reused_node_vec);
debug_assert!(node_rewrites.is_empty());
assert!(node_rewrites.is_empty());
node_rewrites.extend(0..orig_nodes_len);
let mut dead_nodes = 0;

Expand All @@ -623,13 +623,13 @@ impl<O: ForestObligation> ObligationForest<O> {
// self.nodes[0..index - dead_nodes] are the first remaining nodes
// self.nodes[index - dead_nodes..index] are all dead
// self.nodes[index..] are unchanged
for index in 0..orig_nodes_len {
for (index, node_rewrite) in node_rewrites.iter_mut().enumerate() {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible enumerate is less efficient than range iteration?

let node = &self.nodes[index];
match node.state.get() {
NodeState::Pending | NodeState::Waiting => {
if dead_nodes > 0 {
self.nodes.swap(index, index - dead_nodes);
node_rewrites[index] -= dead_nodes;
*node_rewrite -= dead_nodes;
}
}
NodeState::Done => {
Expand All @@ -646,7 +646,7 @@ impl<O: ForestObligation> ObligationForest<O> {
}
// Extract the success stories.
outcome_cb(&node.obligation);
node_rewrites[index] = orig_nodes_len;
*node_rewrite = orig_nodes_len;
dead_nodes += 1;
}
NodeState::Error => {
Expand All @@ -655,7 +655,7 @@ impl<O: ForestObligation> ObligationForest<O> {
// check against.
self.active_cache.remove(&node.obligation.as_cache_key());
self.insert_into_error_cache(index);
node_rewrites[index] = orig_nodes_len;
*node_rewrite = orig_nodes_len;
dead_nodes += 1;
}
NodeState::Success => unreachable!(),
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_data_structures/src/sorted_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -205,22 +205,22 @@ impl<K: Ord, V> SortedMap<K, V> {
R: RangeBounds<K>,
{
let start = match range.start_bound() {
Bound::Included(ref k) => match self.lookup_index_for(k) {
Bound::Included(k) => match self.lookup_index_for(k) {
Ok(index) | Err(index) => index,
},
Bound::Excluded(ref k) => match self.lookup_index_for(k) {
Bound::Excluded(k) => match self.lookup_index_for(k) {
Ok(index) => index + 1,
Err(index) => index,
},
Bound::Unbounded => 0,
};

let end = match range.end_bound() {
Bound::Included(ref k) => match self.lookup_index_for(k) {
Bound::Included(k) => match self.lookup_index_for(k) {
Ok(index) => index + 1,
Err(index) => index,
},
Bound::Excluded(ref k) => match self.lookup_index_for(k) {
Bound::Excluded(k) => match self.lookup_index_for(k) {
Ok(index) | Err(index) => index,
},
Bound::Unbounded => self.data.len(),
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_data_structures/src/sorted_map/index_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ impl<I: Idx, K: Ord, V> SortedIndexMultiMap<I, K, V> {
///
/// If there are multiple items that are equivalent to `key`, they will be yielded in
/// insertion order.
pub fn get_by_key(&'a self, key: K) -> impl 'a + Iterator<Item = &'a V> {
pub fn get_by_key(&self, key: K) -> impl Iterator<Item = &V> {
self.get_by_key_enumerated(key).map(|(_, v)| v)
}

Expand All @@ -84,7 +84,7 @@ impl<I: Idx, K: Ord, V> SortedIndexMultiMap<I, K, V> {
///
/// If there are multiple items that are equivalent to `key`, they will be yielded in
/// insertion order.
pub fn get_by_key_enumerated(&'a self, key: K) -> impl '_ + Iterator<Item = (I, &V)> {
pub fn get_by_key_enumerated(&self, key: K) -> impl Iterator<Item = (I, &V)> {
let lower_bound = self.idx_sorted_by_item_key.partition_point(|&i| self.items[i].0 < key);
self.idx_sorted_by_item_key[lower_bound..].iter().map_while(move |&i| {
let (k, v) = &self.items[i];
Expand Down
16 changes: 4 additions & 12 deletions compiler/rustc_data_structures/src/sso/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -257,11 +257,7 @@ impl<K: Eq + Hash, V> SsoHashMap<K, V> {
pub fn remove(&mut self, key: &K) -> Option<V> {
match self {
SsoHashMap::Array(array) => {
if let Some(index) = array.iter().position(|(k, _v)| k == key) {
Some(array.swap_remove(index).1)
} else {
None
}
array.iter().position(|(k, _v)| k == key).map(|index| array.swap_remove(index).1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I know other data structures have some instances of open coding map to reduce the amount of monomorphization bloat that needs to be compiled, so this is a potential perf culprit

Copy link
Member

Choose a reason for hiding this comment

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

I would worry less about bloat and more about that instance of Option::map having the closure inlined into it, but not being inlined itself (because of inlining swap_remove?).

}
SsoHashMap::Map(map) => map.remove(key),
}
Expand All @@ -272,11 +268,7 @@ impl<K: Eq + Hash, V> SsoHashMap<K, V> {
pub fn remove_entry(&mut self, key: &K) -> Option<(K, V)> {
match self {
SsoHashMap::Array(array) => {
if let Some(index) = array.iter().position(|(k, _v)| k == key) {
Some(array.swap_remove(index))
} else {
None
}
array.iter().position(|(k, _v)| k == key).map(|index| array.swap_remove(index))
}
SsoHashMap::Map(map) => map.remove_entry(key),
}
Expand Down Expand Up @@ -423,14 +415,14 @@ impl<K, V> IntoIterator for SsoHashMap<K, V> {

/// adapts Item of array reference iterator to Item of hashmap reference iterator.
#[inline(always)]
fn adapt_array_ref_it<K, V>(pair: &'a (K, V)) -> (&'a K, &'a V) {
fn adapt_array_ref_it<K, V>(pair: &(K, V)) -> (&K, &V) {
let (a, b) = pair;
(a, b)
}

/// adapts Item of array mut reference iterator to Item of hashmap mut reference iterator.
#[inline(always)]
fn adapt_array_mut_it<K, V>(pair: &'a mut (K, V)) -> (&'a K, &'a mut V) {
fn adapt_array_mut_it<K, V>(pair: &mut (K, V)) -> (&K, &mut V) {
let (a, b) = pair;
(a, b)
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_data_structures/src/sso/set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ impl<T> SsoHashSet<T> {
/// An iterator visiting all elements in arbitrary order.
/// The iterator element type is `&'a T`.
#[inline]
pub fn iter(&'a self) -> impl Iterator<Item = &'a T> {
pub fn iter(&self) -> impl Iterator<Item = &T> {
self.into_iter()
}

Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_data_structures/src/stable_hasher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,14 +229,14 @@ impl<CTX> HashStable<CTX> for ::std::num::NonZeroUsize {

impl<CTX> HashStable<CTX> for f32 {
fn hash_stable(&self, ctx: &mut CTX, hasher: &mut StableHasher) {
let val: u32 = unsafe { ::std::mem::transmute(*self) };
let val: u32 = self.to_bits();
val.hash_stable(ctx, hasher);
}
}

impl<CTX> HashStable<CTX> for f64 {
fn hash_stable(&self, ctx: &mut CTX, hasher: &mut StableHasher) {
let val: u64 = unsafe { ::std::mem::transmute(*self) };
let val: u64 = self.to_bits();
val.hash_stable(ctx, hasher);
}
}
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_data_structures/src/stack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const RED_ZONE: usize = 100 * 1024; // 100k

// Only the first stack that is pushed, grows exponentially (2^n * STACK_PER_RECURSION) from then
// on. This flag has performance relevant characteristics. Don't set it too high.
#[allow(clippy::identity_op)]
const STACK_PER_RECURSION: usize = 1 * 1024 * 1024; // 1MB

/// Grows the stack on demand to prevent stack overflow. Call this in strategic locations
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_data_structures/src/steal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ impl<T> Steal<T> {
#[track_caller]
pub fn borrow(&self) -> MappedReadGuard<'_, T> {
let borrow = self.value.borrow();
if let None = &*borrow {
if borrow.is_none() {
panic!("attempted to read from stolen value: {}", std::any::type_name::<T>());
}
ReadGuard::map(borrow, |opt| opt.as_ref().unwrap())
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_data_structures/src/tiny_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ impl<T: PartialEq> TinyList<T> {
#[inline]
pub fn contains(&self, data: &T) -> bool {
let mut elem = self.head.as_ref();
while let Some(ref e) = elem {
while let Some(e) = elem {
if &e.data == data {
return true;
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_data_structures/src/vec_linked_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ use rustc_index::vec::{Idx, IndexVec};

pub fn iter<Ls>(
first: Option<Ls::LinkIndex>,
links: &'a Ls,
) -> impl Iterator<Item = Ls::LinkIndex> + 'a
links: &Ls,
) -> impl Iterator<Item = Ls::LinkIndex> + '_
where
Ls: Links,
{
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_graphviz/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ impl<'a> LabelText<'a> {
pub fn to_dot_string(&self) -> String {
match *self {
LabelStr(ref s) => format!("\"{}\"", s.escape_default()),
EscStr(ref s) => format!("\"{}\"", LabelText::escape_str(&s)),
EscStr(ref s) => format!("\"{}\"", LabelText::escape_str(s)),
HtmlStr(ref s) => format!("<{}>", s),
}
}
Expand Down
7 changes: 3 additions & 4 deletions compiler/rustc_index/src/bit_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -990,9 +990,8 @@ impl<R: Idx, C: Idx> BitMatrix<R, C> {
pub fn insert_all_into_row(&mut self, row: R) {
assert!(row.index() < self.num_rows);
let (start, end) = self.range(row);
let words = &mut self.words[..];
for index in start..end {
words[index] = !0;
for word in self.words[start..end].iter_mut() {
Copy link
Member

Choose a reason for hiding this comment

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

I didn't get to this in time, but xs[range].iter_mut() should just be writen &mut xs[range] - surprised clippy doesn't have a lint for this.

*word = !0;
}
Comment on lines -993 to 995
Copy link
Member

Choose a reason for hiding this comment

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

This entire thing can just be self.words[start..end].fill(!0), just realized.

self.clear_excess_bits(row);
}
Expand Down Expand Up @@ -1144,7 +1143,7 @@ impl<R: Idx, C: Idx> SparseBitMatrix<R, C> {

/// Iterates through all the columns set to true in a given row of
/// the matrix.
pub fn iter<'a>(&'a self, row: R) -> impl Iterator<Item = C> + 'a {
pub fn iter(&self, row: R) -> impl Iterator<Item = C> + '_ {
self.row(row).into_iter().flat_map(|r| r.iter())
}

Expand Down
11 changes: 4 additions & 7 deletions compiler/rustc_index/src/vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -634,18 +634,15 @@ impl<I: Idx, T> IndexVec<I, T> {
}

#[inline]
pub fn drain<'a, R: RangeBounds<usize>>(
&'a mut self,
range: R,
) -> impl Iterator<Item = T> + 'a {
pub fn drain<R: RangeBounds<usize>>(&mut self, range: R) -> impl Iterator<Item = T> + '_ {
self.raw.drain(range)
}

#[inline]
pub fn drain_enumerated<'a, R: RangeBounds<usize>>(
&'a mut self,
pub fn drain_enumerated<R: RangeBounds<usize>>(
&mut self,
range: R,
) -> impl Iterator<Item = (I, T)> + 'a {
) -> impl Iterator<Item = (I, T)> + '_ {
self.raw.drain(range).enumerate().map(|(n, t)| (I::new(n), t))
}

Expand Down
11 changes: 5 additions & 6 deletions compiler/rustc_lexer/src/unescape.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,10 @@ pub enum EscapeError {
impl EscapeError {
/// Returns true for actual errors, as opposed to warnings.
pub fn is_fatal(&self) -> bool {
match self {
EscapeError::UnskippedWhitespaceWarning => false,
EscapeError::MultipleSkippedLinesWarning => false,
_ => true,
}
!matches!(
self,
EscapeError::UnskippedWhitespaceWarning | EscapeError::MultipleSkippedLinesWarning
)
}
}

Expand Down Expand Up @@ -330,7 +329,7 @@ where
callback(start..end, Err(EscapeError::MultipleSkippedLinesWarning));
}
let tail = &tail[first_non_space..];
if let Some(c) = tail.chars().nth(0) {
if let Some(c) = tail.chars().next() {
// For error reporting, we would like the span to contain the character that was not
// skipped. The +1 is necessary to account for the leading \ that started the escape.
let end = start + first_non_space + c.len_utf8() + 1;
Expand Down
8 changes: 3 additions & 5 deletions compiler/rustc_macros/src/hash_stable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,9 @@ fn parse_attributes(field: &syn::Field) -> Attributes {
}
if meta.path().is_ident("project") {
if let Meta::List(list) = meta {
if let Some(nested) = list.nested.iter().next() {
if let NestedMeta::Meta(meta) = nested {
attrs.project = meta.path().get_ident().cloned();
any_attr = true;
}
if let Some(NestedMeta::Meta(meta)) = list.nested.iter().next() {
attrs.project = meta.path().get_ident().cloned();
any_attr = true;
}
}
}
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_macros/src/session_diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -349,14 +349,14 @@ impl<'a> SessionDiagnosticDeriveBuilder<'a> {
) -> Result<proc_macro2::TokenStream, SessionDiagnosticDeriveError> {
let field_binding = &info.binding.binding;

let option_ty = option_inner_ty(&info.ty);
let option_ty = option_inner_ty(info.ty);

let generated_code = self.generate_non_option_field_code(
attr,
FieldInfo {
vis: info.vis,
binding: info.binding,
ty: option_ty.unwrap_or(&info.ty),
ty: option_ty.unwrap_or(info.ty),
span: info.span,
},
)?;
Expand Down Expand Up @@ -388,7 +388,7 @@ impl<'a> SessionDiagnosticDeriveBuilder<'a> {
let formatted_str = self.build_format(&s.value(), attr.span());
match name {
"message" => {
if type_matches_path(&info.ty, &["rustc_span", "Span"]) {
if type_matches_path(info.ty, &["rustc_span", "Span"]) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice, removing redundant & should maybe be more forcefully handled (i.e. denyed in rustc crates) - and if you open a PR with just that lint being accounted for, I will r+ it immediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a simple way I can deny the lints for all rustc crates, without copy pasting in all lib.rs?

Copy link
Member

Choose a reason for hiding this comment

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

In theory we could simply deny the lint via x.py clippy, but in reality it seems that --cap-lints warn prevents this from working 😆

diff --git a/src/bootstrap/check.rs b/src/bootstrap/check.rs
index 28e7f1fdca7..cf21a4dc857 100644
--- a/src/bootstrap/check.rs
+++ b/src/bootstrap/check.rs
@@ -44,6 +44,7 @@ fn strings<'a>(arr: &'a [&str]) -> impl Iterator<Item = String> + 'a {
         }
         args.extend(strings(&["--", "--cap-lints", "warn"]));
         args.extend(ignored_lints.iter().map(|lint| format!("-Aclippy::{}", lint)));
+        args.push("-Dclippy::needless_borrow".into());
         args
     } else {
         vec![]

quote! {
#diag.set_span(*#field_binding);
#diag.set_primary_message(#formatted_str);
Expand All @@ -401,7 +401,7 @@ impl<'a> SessionDiagnosticDeriveBuilder<'a> {
}
}
"label" => {
if type_matches_path(&info.ty, &["rustc_span", "Span"]) {
if type_matches_path(info.ty, &["rustc_span", "Span"]) {
quote! {
#diag.span_label(*#field_binding, #formatted_str);
}
Expand Down
Loading