[arithmetic_side_effects] Add more tests related to custom types
Add tests to ensure that custom types are triggered with any type of arithmetic operation as well as combinations with or without references.
---
changelog: none
<!-- changelog_checked -->
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 -->
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`.
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.
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.
Cleanup `rustc_tool_util` and add a convenient macro for `build.rs`
changelog: none
<!-- changelog_checked -->
If possible, I'd like to have a new release for this, maybe `v0.3.0` to use the changes in another project. Then we can also remove the `path = "./rustc_tools_util"` from `Cargo.toml`. I'd be happy to help with the release on crates.io if you'd like the help :)
r? `@matthiaskrgr`
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 -->
Add 1.58 MSRV for `collapsible_str_replace`
The `Pattern` impl for `[char; N]` was added in 1.58
changelog: Enhancement: [`collapsible_str_replace`]: Now takes MSRV into consideration. The minimal version is 1.58
[#10047](https://github.com/rust-lang/rust-clippy/pull/10047)
add `suppress_restriction_lint_in_const` config
According to #9808 , add a new lint `suppress_lint_in_const` to report even in const context. BTW, i am not good at naming either, if anyone have a better idea, i am happy to change it.
This PR is still in progress, so i keep it draft.
- \[x] Followed [lint naming conventions][lint_naming]
- \[x] Added passing UI tests (including committed `.stderr` file)
- \[x] `cargo test` passes locally
- \[x] Executed `cargo dev update_lints`
- \[x] Added lint documentation
- \[x] Run `cargo dev fmt`
changelog: Enhancement: [`indexing_slicing`]: add new config `suppress-restriction-lint-in-const` to enable restriction lints, even if the suggestion might not be applicable
r? `@xFrendet`
Fix 10021
This PR proposes a fix for #10021.
The problem is similar to the one that `@mikerite` described in #9505. The compiler is generating an empty substitution for a call, even though the type of `Self` seems to be needed for a predicate. In `@mikerite's` case, the call was to [`IntoFuture::into_future`](https://doc.rust-lang.org/std/future/trait.IntoFuture.html#tymethod.into_future). In this case, the call is to [`Try::branch`](https://doc.rust-lang.org/std/ops/trait.Try.html#tymethod.branch).
The proposed fix is to verify that the parameter whose type is changing has an index within the substitution. The strikes me as a reasonable approach, since if the check were to fail, the following code would be a no-op:
4c123a06ba/clippy_lints/src/methods/unnecessary_to_owned.rs (L420-L428)
Like `@mikerite's` original solution, this solution turns ICEs into false negatives.
changelog: fix `unnecessary_to_owned` false positive involving `Try::branch`
Don't lint `implicit_clone` when the type doesn't implement clone
fixes#10019
changelog: `implicit_clone`: Don't lint when the type doesn't implement clone
Fix#9958
This PR fixes#9958. In order to fix the issue, the lint will now peel reference operators and enclose the expression with parentheses when necessary.
changelog: [`comparison_to_empty`]: Peel deref operators in suggestions when necessary
Don't lint `from_over_into` for opaque types
fixes#9935
This is stalled until the next sync. The impl in question can't be written on the pinned nightly.
changelog: Don't lint `from_over_into` for opaque types
rustc_ast_lowering: Stop lowering imports into multiple items
Lower them into a single item with multiple resolutions instead.
This also allows to remove additional `NodId`s and `DefId`s related to those additional items.
Treat custom enum discriminant values as constants
fixes#9882
changelog: All lints: Don't lint in enum discriminant values when the suggestion won't work in a const context
