Use UnordMap and UnordSet for id collections (DefIdMap, LocalDefIdMap, etc)
This PR changes the `rustc_data_structures::define_id_collections!` macro to use `UnordMap` and `UnordSet` instead of `FxHashMap` and `FxHashSet`. This should account for a large portion of hash-maps being used in places where they can cause trouble.
The changes required are moderate but non-zero:
- In some places the collections are extracted into sorted vecs.
- There are a few instances where for-loops have been changed to extends.
~~Let's see what the performance impact is. With a bit more refactoring, we might be able to get rid of some of the additional sorting -- but the change set is already big enough. Unless there's a performance impact, I'd like to do further changes in subsequent PRs.~~
Performance does not seem to be negatively affected ([perf-run here](https://github.com/rust-lang/rust/pull/106977#issuecomment-1396776699)).
Part of [MCP 533](https://github.com/rust-lang/compiler-team/issues/533).
r? `@ghost`
Add `multiple_unsafe_ops_per_block` lint
Adds a lint, which restricts an `unsafe` block to only one unsafe operation.
Closes#10064
---
changelog: New lint: [`multiple_unsafe_ops_per_block`]
[#10206](https://github.com/rust-lang/rust-clippy/pull/10206)
<!-- changelog_checked -->
Fix suggestion in `transmutes_expressible_as_ptr_casts` when the source type is a borrow.
fixes#9894
changelog: `transmutes_expressible_as_ptr_casts`: Fix suggestion when the source type is a borrow.
[needless_return]: Remove all semicolons on suggestion
Closes#10182
Multiple semicolons currently breaks autofix for `needless_return` suggestions. Any semicolons left after removing return means that the return type will always be `()`, and thus fail to compile.
This PR allows `needless_return` to remove multiple semicolons.
The change won't cover the case where there is multiple line yet.
i.e.
```rust
fn needless_return() -> bool {
return true;
;;
}
```
---
changelog: Sugg: [`needless_return`]: Now removes all semicolons on the same line
[#10187](https://github.com/rust-lang/rust-clippy/pull/10187)
<!-- changelog_checked -->
`cast_possible_truncation` Suggest TryFrom when truncation possible
This fixes the last issues from https://github.com/rust-lang/rust-clippy/pull/9664 as the author seems to be inactive. The PR author was sadly kept during the rebase, due to the conflict resolution.
IDK if it's worth it do to a full review, I only added the last commit, everything else remained the same, besides a rebase.
---
changelog: Sugg: [`cast_possible_truncation`]: Now suggests using `try_from` or allowing the lint
[#10038](https://github.com/rust-lang/rust-clippy/pull/10038)
<!-- changelog_checked -->
closes: https://github.com/rust-lang/rust-clippy/issues/9231
Allow implementing `Hash` with derived `PartialEq` (`derive_hash_xor_eq`
This is a common pattern and is totally allowed by the `Hash` trait.
Fixes#2627
changelog: Move: Renamed `derive_hash_xor_eq` to [`derived_hash_with_manual_eq`]
[#10184](https://github.com/rust-lang/rust-clippy/pull/10184)
changelog: Enhancement: [`derived_hash_with_manual_eq`]: Now allows `#[derive(PartialEq)]` with custom `Hash` implementations
[#10184](https://github.com/rust-lang/rust-clippy/pull/10184)
<!-- changelog_checked -->
trim paths in `suspicious_to_owned`
This continues my path trimming spree. I'm not going to add yet another changelog entry, we should have one "trim paths in some applicable lints" entry instead.
---
changelog: none
unused_self: Don't trigger if the method body contains todo!()
If the author is using todo!(), presumably they intend to use self at some point later, so we don't have a good basis to recommend factoring out to an associated function.
Fixes#10117.
---
changelog: Enhancement: [`unused_self`]: No longer lints, if the method body contains a `todo!()` call
[#10166](https://github.com/rust-lang/rust-clippy/pull/10166)
<!-- changelog_checked -->
[#10167] Clarify that the lint only works if x eq. y in a `for` loop.
Reading the documentation for the lint, one could expect that the lint works in all cases that `X == Y`. This is false.
While the lint was updated, the documentation wasn't.
More information about the `N..N` problem in #5689 and #5628
---
Fixes#10167
changelog: [`reversed_empty_ranges`]: Update and clarify documentation
Reading the documentation for the lint, one could expect that the lint works in all cases that `X == Y`. This is false.
While the lint was updated, the documentation wasn't.
More information about the `N..N` problem in #5689 and #5628
trim paths in `box_default`
This might help with #10089, though I have not tested that yet. In any event, it keeps the suggestion short and to the point.
---
changelog: Trim paths in [`box_default`] suggestion
trim paths in `default_trait_access`/`clone_on_copy` suggestions
This should help making the suggestions more palatable. Similar to #10153.
---
changelog: trim paths in [`default_trait_access`]/[`clone_on_copy`] suggestions
If the author is using todo!(), presumably they intend to use self at
some point later, so we don't have a good basis to recommend factoring
out to an associated function.
Fixes#10117.
changelog: Don't trigger [`unused_self`] if the method body contains a `todo!()` call
[`drop_ref`]: don't lint idiomatic in match arm
fixes#10122
As established in issue #9482, it is idiomatic to use a single `drop()` expression in a match arm to achieve a side-effect of a function while discarding its output. This should also apply to cases where the function returns a reference.
The change to the lint's code was less than 1 line, because all the heavy lifting was done in PR #9491.
---
changelog: FP: [`drop_ref`]: No longer lints idiomatic expression in `match` arms
[#10142](https://github.com/rust-lang/rust-clippy/pull/10142)
<!-- changelog_checked -->
Make the iter_kv_map lint handle ref/mut annotations.
For the degenerate (`map(|(k, _)| k)`/`map(|(_, v)| v)`) cases a mut annotation is superfluous and a ref annotation won't compile, so no additional handling is required. For cases where the `map` call must be preserved ref/mut annotations should also be presereved so that the map body continues to work as expected.
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: [`iter_kv_map`]: handle ref/mut annotations
For the degenerate (`map(|(k, _)| k)`/`map(|(_, v)| v)`) cases a mut annotation is superfluous and a ref annotation won't compile, so no additional handling is required. For cases where the `map` call must be preserved ref/mut annotations should also be presereved so that the map body continues to work as expected.
don't lint field_reassign when field in closure
fixes#10136
This change makes the ContainsName struct visit all interior expressions, which means that ContainsName will return true even if `name` is used in a closure within `expr`.
---
changelog: FP: [`field_reassign_with_default`]: No longer lints cases, where values are initializes from closures capturing struct values
[#10143](https://github.com/rust-lang/rust-clippy/pull/10143)
<!-- changelog_checked -->
chore: fix identation of `if_chain` in `filter_map`
This is a really small fix.
If someone could take a look at it, I would appreciate it🙏
---
changelog: none
<!-- changelog_checked -->
This commit makes the ContainsName struct visit all interior
expressions, which means that ContainsName will return true
even if `name` is used in a closure within `expr`.
Move `mutex_atomic` to `restriction`
By #4295, the general consensus seems to be that `mutex_atomic` is not a useful lint in most cases. If anything, it could be useful as a restriction on code that for whatever reason can't use atomics. Keeping it in `clippy::nursery` is harmful to people attempting to use clippy for soundness.
---
changelog: Moved [`mutex_atomic`] to `restriction`
[#10115](https://github.com/rust-lang/rust-clippy/pull/10115)
<!-- chnagelog_checked -->
Rollup of 9 pull requests
Successful merges:
- #104531 (Provide a better error and a suggestion for `Fn` traits with lifetime params)
- #105899 (`./x doc library --open` opens `std`)
- #106190 (Account for multiple multiline spans with empty padding)
- #106202 (Trim more paths in obligation types)
- #106234 (rustdoc: simplify settings, help, and copy button CSS by not reusing)
- #106236 (docs/test: add docs and a UI test for `E0514` and `E0519`)
- #106259 (Update Clippy)
- #106260 (Fix index out of bounds issues in rustdoc)
- #106263 (Formatter should not try to format non-Rust files)
Failed merges:
r? `@ghost`
`@rustbot` modify labels: rollup
fix incorrect suggestion in `suboptimal_flops`
fixes#10003
There was an error when trying to negate an expression like `x - 1.0`. We used to format it as `-x - 1.0` whereas a proper negation would be `-(x - 1.0)`.
Therefore, we add parentheses around the expression when it is `ExprKind::Binary`.
We also add parentheses around multiply and divide expressions, even though this is not strictly necessary.
changelog: [`suboptimal_flops`]: fix incorrect suggestion caused by an incorrect negation of floating point expressions.
There was an error when trying to negate an expression
like `x - 1.0`. We used to format it as `-x - 1.0` whereas
a proper negation would be `-(x - 1.0)`.
Therefore, we add parentheses around the expression when it is a
Binary ExprKind.
We also add parentheses around multiply and divide expressions,
even though this is not strictly necessary.
This patch not only improves visibility, but also fixes a potential bug.
When a lint description ends with code block, the string will have three
backquotes at the end.
Since the current implementation prints the default value immediately
after that, the markdown renderer is unable to properly close the code
block.
Add size_of_ref lint
This addresses #9995, which is likely raising a valid point about `std::mem::size_of_val()`: It's [very easy to use double-references as the argument](https://github.com/apache/arrow-datafusion/pull/4371#discussion_r1032385224), which the function will happily accept and give back the size of _the reference_, not the size of the value _behind_ the reference. In the worst case, if the value matches the programmer's expectation, this seems to work, while in fact, everything will go horribly wrong e.g. on a different platform.
The size of a `&T` is independent of what `T` is, and people might want to use `std::mem::size_of_val()` to actually get the size of _any_ reference (e.g. via `&&()`). I would rather suggest that this is always bad behavior, though ([instead](https://doc.rust-lang.org/reference/type-layout.html#pointers-and-references-layout), [and](https://doc.rust-lang.org/stable/std/primitive.usize.html#associatedconstant.BITS)). I, therefore, put this lint into `correctness`.
Since the problem is usually easily fixed by removing extra `&`, I went light on suggesting code.
---
changelog: New lint: [`size_of_ref`]
[#10098](https://github.com/rust-lang/rust-clippy/pull/10098)
<!-- changelog_checked -->
Improve `possible_borrower`
This PR makes several improvements to `clippy_uitls::mir::possible_borrower`. These changes benefit both `needless_borrow` and `redundant clone`.
1. **Use the compiler's `MaybeStorageLive` analysis**
I could spot not functional differences between the one in the compiler and the one in Clippy's repository. So, I removed the latter in favor of the the former.
2. **Make `PossibleBorrower` a dataflow analysis instead of a visitor**
The main benefit of this change is that allows `possible_borrower` to take advantage of statements' relative locations, which is easier to do in an analysis than in a visitor.
This is easier to illustrate with an example, so consider this one:
```rust
fn foo(cx: &LateContext<'_>, lint: &'static Lint) {
cx.struct_span_lint(lint, rustc_span::Span::default(), "", |diag| diag.note(&String::new()));
// ^
}
```
We would like to flag the `&` pointed to by the `^` for removal. `foo`'s MIR begins like this:
```rust
fn span_lint::foo::{closure#0}(_1: [closure@$DIR/needless_borrow.rs:396:68: 396:74], _2: &mut rustc_errors::diagnostic_builder::DiagnosticBuilder<'_, ()>) -> &mut rustc_errors::diagnostic_builder::DiagnosticBuilder<'_, ()> {
debug diag => _2; // in scope 0 at $DIR/needless_borrow.rs:396:69: 396:73
let mut _0: &mut rustc_errors::diagnostic_builder::DiagnosticBuilder<'_, ()>; // return place in scope 0 at $DIR/needless_borrow.rs:396:75: 396:75
let mut _3: &mut rustc_errors::diagnostic_builder::DiagnosticBuilder<'_, ()>; // in scope 0 at $DIR/needless_borrow.rs:396:75: 396:100
let mut _4: &mut rustc_errors::diagnostic_builder::DiagnosticBuilder<'_, ()>; // in scope 0 at $DIR/needless_borrow.rs:396:75: 396:100
let mut _5: &std::string::String; // in scope 0 at $DIR/needless_borrow.rs:396:85: 396:99
let _6: std::string::String; // in scope 0 at $DIR/needless_borrow.rs:396:86: 396:99
bb0: {
StorageLive(_3); // scope 0 at $DIR/needless_borrow.rs:396:75: 396:100
StorageLive(_4); // scope 0 at $DIR/needless_borrow.rs:396:75: 396:100
_4 = &mut (*_2); // scope 0 at $DIR/needless_borrow.rs:396:75: 396:100
StorageLive(_5); // scope 0 at $DIR/needless_borrow.rs:396:85: 396:99
StorageLive(_6); // scope 0 at $DIR/needless_borrow.rs:396:86: 396:99
_6 = std::string::String::new() -> bb1; // scope 0 at $DIR/needless_borrow.rs:396:86: 396:99
// mir::Constant
// + span: $DIR/needless_borrow.rs:396:86: 396:97
// + literal: Const { ty: fn() -> std::string::String {std::string::String::new}, val: Value(<ZST>) }
}
bb1: {
_5 = &_6; // scope 0 at $DIR/needless_borrow.rs:396:85: 396:99
_3 = rustc_errors::diagnostic_builder::DiagnosticBuilder::<'_, ()>::note::<&std::string::String>(move _4, move _5) -> [return: bb2, unwind: bb4]; // scope 0 at $DIR/needless_borrow.rs:396:75: 396:100
// mir::Constant
// + span: $DIR/needless_borrow.rs:396:80: 396:84
// + literal: Const { ty: for<'a> fn(&'a mut rustc_errors::diagnostic_builder::DiagnosticBuilder<'_, ()>, &std::string::String) -> &'a mut rustc_errors::diagnostic_builder::DiagnosticBuilder<'_, ()> {rustc_errors::diagnostic_builder::DiagnosticBuilder::<'_, ()>::note::<&std::string::String>}, val: Value(<ZST>) }
}
```
The call to `diag.note` appears in `bb1` on the line beginning with `_3 =`. The `String` is owned by `_6`. So, in the call to `diag.note`, we would like to know whether there are any references to `_6` besides `_5`.
The old, visitor approach did not consider the relative locations of statements. So all borrows were treated the same, *even if they occurred after the location of interest*.
For example, before the `_3 = ...` call, the possible borrowers of `_6` would be just `_5`. But after the call, the possible borrowers would include `_2`, `_3`, and `_4`.
So, in a sense, the call from which we are try to remove the needless borrow is trying to prevent us from removing the needless borrow(!).
With an analysis, things do not get so muddled. We can determine the set of possible borrowers at any specific location, e.g., using a `ResultsCursor`.
3. **Change `only_borrowers` to `at_most_borrowers`**
`possible_borrowers` exposed a function `only_borrowers` that determined whether the borrowers of some local were *exactly* some set `S`. But, from what I can tell, this was overkill. For the lints that currently use `possible_borrower` (`needless_borrow` and `redundant_clone`), all we really want to know is whether there are borrowers *other than* those in `S`. (Put another way, we only care about the subset relation in one direction.) The new function `at_most_borrowers` takes this more tailored approach.
4. **Compute relations "on the fly" rather than using `transitive_relation`**
The visitor would compute and store the transitive closure of the possible borrower relation for an entire MIR body.
But with an analysis, there is effectively a different possible borrower relation at each location in the body. Computing and storing a transitive closure at each location would not be practical.
So the new approach is to compute the transitive closure on the fly, as needed. But the new approach might actually be more efficient, as I now explain.
In all current uses of `at_most_borrowers` (previously `only_borrowers`), the size of the set of borrowers `S` is at most 2. So you need only check at most three borrowers to determine whether the subset relation holds. That is, once you have found a third borrower, you can stop, since you know the relation cannot hold.
Note that `transitive_relation` is still used by `clippy_uitls::mir::possible_origin` (a kind of "subroutine" of `possible_borrower`).
cc: `@Jarcho`
---
changelog: [`needless_borrow`], [`redundant_clone`]: Now track references better and detect more cases
[#9701](https://github.com/rust-lang/rust-clippy/pull/9701)
<!-- changelog_checked -->
Avoid `match_wildcard_for_single_variants` on guarded wild matches
fix#9993
changelog: FP: [`match_wildcard_for_single_variants`]: No longer lints on wildcards with a guard
[#10056](https://github.com/rust-lang/rust-clippy/pull/10056)
<!-- changelog_checked -->
r? `@Jarcho`
Null fn lints
Adds lints to check for code, that assumes nullable `fn()`.
### Lint examples:
`transmute_null_to_fn`:
```rust
error: transmuting a known null pointer into a function pointer
--> $DIR/transmute_null_to_fn.rs:9:23
|
LL | let _: fn() = std::mem::transmute(std::ptr::null::<()>());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this transmute results in undefined behavior
|
= help: try wrapping your function pointer type in `Option<T>` instead, and using `None` as a null pointer value
```
`fn_null_check`:
```rust
error: function pointer assumed to be nullable, even though it isn't
--> $DIR/fn_null_check.rs:13:8
|
LL | if (fn_ptr as *mut ()).is_null() {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: try wrapping your function pointer type in `Option<T>` instead, and using `is_none` to check for null pointer value
```
Closes#1644
---
changelog: Improvement: [`transmuting_null`]: Now detects `const` pointers to all types
[#10099](https://github.com/rust-lang/rust-clippy/pull/10099)
changelog: New lint: [`transmute_null_to_fn`]
[#10099](https://github.com/rust-lang/rust-clippy/pull/10099)
changelog: New lint: [`fn_null_check`]
[#10099](https://github.com/rust-lang/rust-clippy/pull/10099)
<!-- changelog_checked (This is just a flag for me, please don't add it manually) -->
Identify more cases of useless `into_iter()` calls
changelog: Sugg: [`useless_conversion`]: Now suggests removing calls to `into_iter()` on an expression implementing `Iterator`
[#10020](https://github.com/rust-lang/rust-clippy/pull/10020)
<!-- changelog_checked -->
If the type of the result of a call to `IntoIterator::into_iter()`
and the type of the receiver are the same, then the receiver
implements `Iterator` and `into_iter()` is the identity function.
The call to `into_iter()` may be removed in all but two cases:
- If the receiver implements `Copy`, `into_iter()` will produce
a copy of the receiver and cannot be removed. For example,
`x.into_iter().next()` will not advance `x` while `x.next()` will.
- If the receiver is an immutable local variable and the call to
`into_iter()` appears in a larger expression, removing the call to
`into_iter()` might cause mutability issues. For example, if `x`
is an immutable local variable, `x.into_iter().next()` will
compile while `x.next()` will not as `next()` receives
`&mut self`.
Rustup
r? `@ghost`
I'm on the train and my internet is too bad to download the necessary toolchain, so I have to use CI to find sync fallout.
changelog: none
<!-- changelog_checked -->
fix: not suggest seek_to_start_instead_of_rewind when expr is used
changelog: [`seek_to_start_instead_of_rewind`]: No longer lints, if the return of `seek` is used.
[#10096](https://github.com/rust-lang/rust-clippy/pull/10096)
<!-- changelog_checked -->
Fixes#10065
There used to be a logical bug where IncrementVisitor would
completely stop checking an expression/block after seeing a continue
statement. This led to issue #10058 where a variable incremented
(or otherwise modified) after any continue statement would still be
considered incremented only once.
The solution is to continue scanning the expression after seeing a
`continue` statement, but increment self.depth so that the Visitor
thinks that the rest of the loop is within a conditional.
Changelog 1.66
It's really nice to see a changelog with so many suggestion fixes and improvements. Not much else to say. This should be merged with the coming release on 2022-12-15. For the reviewer, please review it and approve it if it looks good. The merge should wait until the release :)
---
changelog: none
<!-- changelog_checked -->
improve `manual_is_ascii_check ` check
Sorry, not familiar the api, i can only check the method name of expression `<expr-1>.contains(<expr-2>)` after read clippy book and hints from #9933 . i dont know how to check
1. if <expr-1> is a specific range
2. <expr-2> is a character
r? `@xFrednet` could you please provide some more hints? 😝️
---
changelog: Enhancement: [`manual_is_ascii_check`]: Now detects ranges with `.contains()` calls
[#10053](https://github.com/rust-lang/rust-clippy/pull/10053)
<!-- changelog_checked -->
Remove `token::Lit` from `ast::MetaItemLit`.
Currently `ast::MetaItemLit` represents the literal kind twice. This PR removes that redundancy. Best reviewed one commit at a time.
r? `@petrochenkov`
