Change `if_same_then_else` to be a `style` lint
CC #3770
From https://github.com/rust-lang/rust-clippy/issues/3770#issuecomment-687565594 (`@flip1995):`
> Oh I thought I replied to this: I definitely see now that having this
> as a correctness lint might be the wrong categorization. What we might
> want to do is to just allow this lint, if there are comments in the
> arm bodies. But a good first step would be to downgrade this lint to
> style or complexity. I would vote for style since merging two arms is
> not always less complex.
changelog: [`if_same_then_else`]: Change to be a `style` lint
[`impl_trait_in_params`]: avoid ICE when function with `impl Trait` type has no parameters
Fixes#11803
If I'm reading the old code correctly, it was taking the span of the first parameter (without checking that it exists, which caused the ICE) and uses that to figure out where the generic parameter to insert should go (cc `@blyxyas` you wrote the lint, is that correct?).
This seemed equivalent to just `generics.span`, which doesn't require calculating the spans like that and simplifies it a fair bit
changelog: don't ICE when function has no parameters but generics have an `impl Trait` type
CC #3770
From https://github.com/rust-lang/rust-clippy/issues/3770#issuecomment-687565594 (@flip1995):
> Oh I thought I replied to this: I definitely see now that having this
> as a correctness lint might be the wrong categorization. What we might
> want to do is to just allow this lint, if there are comments in the
> arm bodies. But a good first step would be to downgrade this lint to
> style or complexity. I would vote for style since merging two arms is
> not always less complex.
Implement new lint `iter_over_hash_type`
Implements and fixes https://github.com/rust-lang/rust-clippy/issues/11788
This PR adds a new *restriction* lint `iter_over_hash_type` which prevents `Hash`-types (that is, `HashSet` and `HashMap`) from being used as the iterator in `for` loops.
The justification for this is because in `Hash`-based types, the ordering of items is not guaranteed and may vary between executions of the same program on the same hardware. In addition, it reduces readability due to the unclear iteration order.
The implementation of this lint also ensures the following:
- Calls to `HashMap::keys`, `HashMap::values`, and `HashSet::iter` are also denied when used in `for` loops,
- When this expression is used in procedural macros, it is not linted/denied.
changelog: add new `iter_over_hash_type` lint to prevent unordered iterations through hashed data structures
Don't check for late-bound vars, check for escaping bound vars
Fixes an assertion that didn't make sense. Many valid and well-formed types *have* late-bound vars (e.g. `for<'a> fn(&'a ())`), they just must not have *escaping* late-bound vars in order to be normalized correctly.
Addresses rust-lang/rust-clippy#11230, cc `@jyn514` and `@matthiaskrgr`
changelog: don't check for late-bound vars, check for escaping bound vars. Addresses rust-lang/rust-clippy#11230
Fixes to `manual_let_else`'s divergence check
A few changes to the divergence check in `manual_let_else` and moves it the implementation to `clippy_utils` since it's generally useful:
* Handle internal `break` and `continue` expressions.
e.g. The first loop is divergent, but the second is not.
```rust
{
loop {
break 'outer;
};
}
{
loop {
break;
};
}
```
* Match rust's definition of divergence which is defined via the type system.
e.g. The following is not considered divergent by rustc as the inner block has a result type of `()`:
```rust
{
'a: {
panic!();
break 'a;
};
}
```
* Handle when adding a single semicolon would make the expression divergent.
e.g. The following would be a divergent if a semicolon were added after the `if` expression:
```rust
{ if panic!() { 0 } else { 1 } }
```
changelog: None
Lint `needless_borrow` and `explicit_auto_deref` on most union field accesses
Changes both lints to follow rustc's rules around auto-deref through `ManuallyDrop` union fields rather than just bailing on union fields.
changelog: [`needless_borrow`] & [`explicit_auto_deref`]: Lint on most union field accesses
[`map_identity`]: respect match ergonomics
Fixes#11764
Note: the original tests before this were slightly wrong themselves already and had to be changed. They were calling `map` on an iterator of `&(i32, i32)`s, so this PR would stop linting there, but they were meant to test something else unrelated to binding modes, so I just changed them to remove the references so that it iterates over owned values and they all bind by value. This way they continue to test what they checked for before: the identity function for tuple patterns.
changelog: [`map_identity`]: respect match ergonomics
Disable `vec_box` when using different allocators
Fixes#7114
This PR disables the `vec_box` lint when the `Box` and `Vec` use different allocators (but not when they use the same - custom - allocator).
For example - `Vec<Box<i32, DummyAllocator>>` will disable the lint, and `Vec<Box<i32, DummyAllocator>, DummyAllocator>` will not disable the lint.
In addition, the applicability of this lint has been changed to `Unspecified` due to the automatic fixes potentially breaking code such as the following:
```rs
fn foo() -> Vec<Box<i32>> { // -> Vec<i32>
vec![Box::new(1)]
}
```
It should be noted that the `if_chain->let-chains` fix has also been applied to this lint, so the diff does contain many changes.
changelog: disable `vec_box` lint when using nonstandard allocators
Fix `dbg_macro` semi span calculation
`span_including_semi` was using a `BytePos` to index into a file's source which happened to work because the root file of the test started at `BytePos` 0, it didn't work for other files
changelog: none
new lint: `unnecessary_fallible_conversions`
Closes#11577
A new lint that looks for calls such as `i64::try_from(1i32)` and suggests `i64::from(1i32)`. See lint description (and linked issue) for more details for why.
There's a tiny bit of overlap with the `useless_conversion` lint, in that the other one warns `T::try_from(T)` (i.e., fallibly converting to the same type), so this lint ignores cases like `i32::try_from(1i32)` to avoid emitting two warnings for the same expression.
Also, funnily enough, with this one exception, this lint would warn on exactly every case in the `useless_conversion_try` ui test that `useless_conversion` didn't cover (but never two warnings at the same time), which is neat. I did add an `#![allow]` though since we don't want interleaved warnings from multiple lints in the same uitest.
changelog: new lint: `unnecessary_fallible_conversions`
fix enum_variant_names depending lint depending on order
changelog: [`enum_variant_names`]: fix single word variants preventing lint of later variant pre/postfixed with the enum name
fixes#11494
Single word variants prevented checking the `check_enum_start` and `check_enum_end` for being run on later variants
Fix missing parenthesis in suboptimal floating point help
This fixes#11559 by adding a branch in the `Neg` implementation for `Sugg` that adds parentheses to keep precedence in order, then using that in the suggestion. I also removed some needless `.to_string()`s while I was at it.
---
changelog: none
[`iter_without_into_iter`]: fix papercuts in suggestion and restrict linting to exported types
See #11692 for more context.
tldr: the lint `iter_without_into_iter` has suggestions that don't compile, which imo isn't that problematic because it does have the appropriate `Applicability` that tells external tools that it shouldn't be auto-applied.
However there were some obvious "errors" in the suggestion that really should've been included in my initial PR adding the lint, which is fixed by this PR:
- `IntoIterator::into_iter` needs a `self` argument.
- `IntoIterator::Iter` associated type doesn't exist. This should've just been `Item`.
This still doesn't make it machine applicable, and the remaining things are imho quite non-trivial to implement, as I've explained in https://github.com/rust-lang/rust-clippy/issues/11692#issuecomment-1773886111.
I personally think it's fine to leave it there and let the user change the remaining errors when copy-pasting the suggestion (e.g. errors caused by lifetimes that were permitted in fn return-position but are not in associated types).
This is how many of our other lint suggestions already work.
Also, we now restrict linting to only exported types. This required moving basically all of the tests around since they were previously in the `main` function. Same for `into_iter_without_iter`. The git diff is a bit useless here...
changelog: [`iter_without_into_iter`]: fix papercuts in suggestion and restrict linting to exported types
(cc `@lopopolo,` figured I should mention you since you created the issue)
Add `waker_clone_and_wake` lint to check needless `Waker` clones
Check for patterns of `waker.clone().wake()` and replace them with `waker.wake_by_ref()`.
An alternative name could be `waker_clone_then_wake`
changelog: [ `waker_clone_wake`]: new lint
Validate `feature` and `since` values inside `#[stable(…)]`
Previously the string passed to `#[unstable(feature = "...")]` would be validated as an identifier, but not `#[stable(feature = "...")]`. In the standard library there were `stable` attributes containing the empty string, and kebab-case string, neither of which should be allowed.
Pre-existing validation of `unstable`:
```rust
// src/lib.rs
#![allow(internal_features)]
#![feature(staged_api)]
#![unstable(feature = "kebab-case", issue = "none")]
#[unstable(feature = "kebab-case", issue = "none")]
pub struct Struct;
```
```console
error[E0546]: 'feature' is not an identifier
--> src/lib.rs:5:1
|
5 | #![unstable(feature = "kebab-case", issue = "none")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```
For an `unstable` attribute, the need for an identifier is obvious because the downstream code needs to write a `#![feature(...)]` attribute containing that identifier. `#![feature(kebab-case)]` is not valid syntax and `#![feature(kebab_case)]` would not work if that is not the name of the feature.
Having a valid identifier even in `stable` is less essential but still useful because it allows for informative diagnostic about the stabilization of a feature. Compare:
```rust
// src/lib.rs
#![allow(internal_features)]
#![feature(staged_api)]
#![stable(feature = "kebab-case", since = "1.0.0")]
#[stable(feature = "kebab-case", since = "1.0.0")]
pub struct Struct;
```
```rust
// src/main.rs
#![feature(kebab_case)]
use repro::Struct;
fn main() {}
```
```console
error[E0635]: unknown feature `kebab_case`
--> src/main.rs:3:12
|
3 | #![feature(kebab_case)]
| ^^^^^^^^^^
```
vs the situation if we correctly use `feature = "snake_case"` and `#![feature(snake_case)]`, as enforced by this PR:
```console
warning: the feature `snake_case` has been stable since 1.0.0 and no longer requires an attribute to enable
--> src/main.rs:3:12
|
3 | #![feature(snake_case)]
| ^^^^^^^^^^
|
= note: `#[warn(stable_features)]` on by default
```
report `unused_import` for empty reexports even it is pub
Fixes#116032
An easy fix. r? `@petrochenkov`
(Discovered this issue while reviewing #115993.)
suggest passing function instead of calling it in closure for [`option_if_let_else`]
fixes: #11429
changelog: suggest passing function instead of calling it in closure for [`option_if_let_else`]
Avoid a `track_errors` by bubbling up most errors from `check_well_formed`
I believe `track_errors` is mostly papering over issues that a sufficiently convoluted query graph can hit. I made this change, while the actual change I want to do is to stop bailing out early on errors, and instead use this new `ErrorGuaranteed` to invoke `check_well_formed` for individual items before doing all the `typeck` logic on them.
This works towards resolving https://github.com/rust-lang/rust/issues/97477 and various other ICEs, as well as allowing us to use parallel rustc more (which is currently rather limited/bottlenecked due to the very sequential nature in which we do `rustc_hir_analysis::check_crate`)
cc `@SparrowLii` `@Zoxc` for the new `try_par_for_each_in` function
Skip if_not_else lint for '!= 0'-style checks
Currently, clippy makes unhelpful suggestions such as this:
```
warning: unnecessary `!=` operation
--> src/vm.rs:598:36
|
598 | *destination = if source & 0x8000 != 0 { 0xFFFF } else { 0 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: change to `==` and swap the blocks of the `if`/`else`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#if_not_else
= note: `-W clippy::if-not-else` implied by `-W clippy::pedantic`
```
Bit tests often take on the form `if foo & 0x1234 != 0 { … } else { … }`, and the `!= 0` part reads as "has any bits set". Therefore, this code already has the "correct" order, and shouldn't be changed.
This PR disables the lint for these cases, and in fact all cases where the condition is "foo is non-zero".
I did my homework:
- \[X] Followed [lint naming conventions][lint_naming] → Not applicable, this PR fixes an existing lint
- \[X] Added passing UI tests (including committed `.stderr` file) → Yes, `tests/ui/if_not_else_bittest.rs`
- \[X] `cargo test` passes locally
- \[X] Executed `cargo dev update_lints`
- \[X] Added lint documentation → Not applicable, this PR fixes an existing lint
- \[X] Run `cargo dev fmt`
changelog: Fix [`if_not_else`] false positive when something like `bitflags != 0` is used
[`map_identity`]: allow closure with type annotations
Fixes#9122
`.map(|a: u32| a)` can help type inference, so we should probably allow this and not warn about "unnecessary map of the identity function"
changelog: [`map_identity`]: allow closure with type annotations
add lint for struct field names
changelog: [`struct_field_names`]: lint structs with the same pre/postfix in all fields or with fields that are pre/postfixed with the name of the struct.
fixes#2555
I've followed general structure and naming from the code in [enum_variants](b788addfcc/clippy_lints/src/enum_variants.rs) lint, which implements the same logic for enum variants.
side effect for `enum_variants`:
use .first() instead of .get(0) in enum_variants lint
move to_camel_case to str_util module
move module, enum and struct name repetitions check to a single file `item_name_repetitions`
rename enum_variants threshold config option
Add regression test for #11610 about mutable usage of argument in async function for the `needless_pass_by_ref_mut` lint
Fixes https://github.com/rust-lang/rust-clippy/issues/11610.
This was already fixed. I simply added a regression test.
changelog: Add regression test for #11610 about mutable usage of argument in async function for the `needless_pass_by_ref_mut` lint
Make `multiple_unsafe_ops_per_block` ignore await desugaring
The await desugaring contains two calls (`Poll::new_unchecked` and `get_context`) inside a single unsafe block. That violates the lint.
fixes#11312
changelog: [`multiple_unsafe_ops_per_block`]: fix false positives in `.await`
[`unnecessary_lazy_eval`]: reduce applicability if closure has return type annotation
Fixes#11672
We already check if closure parameters don't have type annotations and reduce the applicability to `MaybeIncorrect` if they do, since those help type inference and removing them breaks code. We didn't do this for return type annotations however. This PR adds it. This doesn't change it to produce a fix that will compile, but it will prevent rustfix from auto-applying it.
(In general I'm not sure if we can suggest a fix that will compile. In this specific example, it might be possible to suggest `&[] as &[u8]`, but as-casts won't always work, e.g. `Default::default() as &[u8]` is a compile error, so just reducing applicability should be a safe fix in any case for now)
changelog: [`unnecessary_lazy_eval`]: reduce applicability to `MaybeIncorrect` if closure has return type annotation
[`get_first`]: lint on non-primitive slices
Fixes#11594
I left the issue open for a couple days before making the PR to see if anyone has something to say, but it looks like there aren't any objections to removing this check that prevented linting on non-primitive slices, so here's the PR now.
There's a couple of instances in clippy itself where we now emit the lint. The actual relevant change is in the first commit and fixing the `.get(0)` instances in clippy itself is in the 2nd commit.
changelog: [`get_first`]: lint on non-primitive slices
Fix/11134
Fix#11134
Hir of `qpath` will be `TypeRelative(Ty { kind: Path(LangItem...` when a closure contains macro (e.g. https://github.com/rust-lang/rust-clippy/issues/11651) and #11134, it causes panic.
This PR avoids panicking and emitting incomplete path string when `qpath` contains `LangItem`.
changelog: none
`impl_trait_in_params` now supports impls and traits
Before this PR, the lint `impl_trait_in_params`. This PR gives the lint support for functions in impls and traits. (Also, some pretty heavy refactor)
fixes#11548
changelog:[`impl_trait_in_params`] now supports `impl` blocks and functions in traits
Improve `redundant_locals` help message
Fixes#11625
AFAIK, `span_lint_and_help` points the beginning of spans when we pass multiple spans to the second argument, so This PR I also modified its help span and its message.
lint result of the given example in the issue will be:
```console
error: redundant redefinition of a binding `apple`
--> src/main.rs:5:5
|
5 | let apple = apple;
| ^^^^^^^^^^^^^^^^^^
|
help: `apple` is initially defined here
--> src/main.rs:4:9
|
4 | let apple = 42;
| ^^^^^
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_locals
```
I hope that this change might help reduce user confusion, but I'd appreciate alternative suggestions:)
changelog: [`redundant_locals`]: Now points at the rebinding of the variable
Fix `items_after_test_module` for non root modules, add applicable suggestion
Fixes#11050Fixes#11153
changelog: [`items_after_test_module`]: Now suggests a machine-applicable suggestion.
changelog: [`items:after_test_module`]: Also lints for non root modules
std_instead_of_core: avoid lint inside of proc-macro
- fixes https://github.com/rust-lang/rust-clippy/issues/10198
note: The lint for the reported `thiserror::Error` has been suppressed by [Don't lint unstable moves in std_instead_of_core](https://github.com/rust-lang/rust-clippy/pull/9545/files#diff-2cb8a24429cf9d9898de901450d640115503a10454d692dddc6a073a299fbb7eR29) because `thiserror::Error` internally implements `std::error::Error for (derived struct)`.
changelog: [`std_intead_of_core`]: avoid linting inside proc-macro
I confirmed this change fixes the problem:
<details>
<summary>test result without the change</summary>
```console
error: used import from `std` instead of `core`
--> tests/ui/std_instead_of_core.rs:65:14
|
LL | #[derive(ImplStructWithStdDisplay)]
| ^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: this error originates in the derive macro `ImplStructWithStdDisplay` (in Nightly builds, run with -Z macro-backtrace for more info)
```
</details>
Move `needless_pass_by_ref_mut`: `suspicious` -> `nursery`
[Related to [this Zulip thread](https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/needless_pass_by_ref_mut.20isn't.20ready.20for.20stable)]
`needless_pass_by_ref_mut` has been released with some important bugs (notably having a lot of reported false positives and an ICE). So it may not be really ready for being in stable until these problems are solved. This PR changes the lint's category from `suspicious` to `nursery`, just that.
changelog: none
Partially outline code inside the panic! macro
This outlines code inside the panic! macro in some cases. This is split out from https://github.com/rust-lang/rust/pull/115562 to exclude changes to rustc.
There are cases where users create a unit variant for the purposes
of tracking the number of variants for an nonexhaustive enum.
We should check if an enum is explicitly marked as nonexhaustive
before reporting `manual_non_exhaustive` in these cases. Fixes#11583
new lint: `into_iter_without_iter`
Closes#9736 (part 2)
This implements the other lint that my earlier PR missed: given an `IntoIterator for &Type` impl, check that there exists an inherent `fn iter(&self)` method.
changelog: new lint: `into_iter_without_iter`
r? `@Jarcho` since you reviewed #11527 I figured it makes sense for you to review this as well?
[`manual_let_else`]: only omit block if span is from same ctxt
Fixes#11579.
The lint already had logic for omitting a block in `else` if a block is already present, however this didn't handle the case where the block is from a different expansion/syntax context. E.g.
```rs
macro_rules! panic_in_block {
() => { { panic!() } }
}
let _ = match Some(1) {
Some(v) => v,
_ => panic_in_block!()
};
```
It would see this in its expanded form as `_ => { panic!() }` and think it doesn't have to include a block in its suggestion because it is already there, however that's not true if it's from a different expansion like in this case.
changelog: [`manual_let_else`]: only omit block in suggestion if the block is from the same expansion
fixed fp caused by moving &mut reference inside of a closure
changelog: [`needless_pass_by_ref mut`]: fixes false positive caused by not covering mutable references passed to a closure inside of a fuction
fixes#11545
Remove most usage of `hir_ty_to_ty`
Removes the usages where there's a suitable query or the type was already available elsewhere. The remaining cases would all require more involved changes
changelog: none
r? `@Jarcho`
fix FP with needless_raw_string_hashes
changelog: Fix [`needless_raw_string_hashes`]: Continue the lint checking of raw string when `needless_raw_strings` is allowed.
fix#11420
[`redundant_guards`]: lint if the pattern is on the left side
A tiny improvement to the `redundant_guards` lint. There's no associated issue for this, just noticed it while going through the code.
Right now it warns on `Some(x) if x == 2` and suggests `Some(2)`, but it didn't do that for `Some(x) if 2 == x` (i.e. when the local is on the right side and the pattern on the left side).
changelog: [`redundant_guards`]: also lint if the pattern is on the left side
Change defaults of `accept-comment-above-statement` and `accept-comment-above-attributes`
This patch sets the two configuration options for `undocumented_unsafe_blocks` to `true` by default: these are `accept-comment-above-statement` and `accept-comment-above-attributes`. Having these values `false` by default prevents what many users would consider clean code, e.g. placing the `// SAFETY:` comment above a single-line functino call, rather than directly next to the argument.
This was originally discussed in https://github.com/rust-lang/rust-clippy/issues/11162
changelog: [`undocumented_unsafe_blocks`]: set
`accept-comment-above-statement` and `accept-comment-above-attributes` to `true` by default.
Fix mutaby used async function argument in closure for `needless_pass_by_ref_mut`
Fixes https://github.com/rust-lang/rust-clippy/issues/11380.
The problem was that it needed to go through closures as well in async functions to correctly find the mutable usage of async function arguments.
changelog: Correctly handle mutable usage of async function arguments in closures.
r? `@Centri3`
This patch sets the two configuration options for
`undocumented_unsafe_blocks` to `true` by default: these are
`accept-comment-above-statement` and `accept-comment-above-attributes`.
Having these values `false` by default prevents what many users would
consider clean code, e.g. placing the `// SAFETY:` comment above a
single-line functino call, rather than directly next to the argument.
changelog: [`undocumented_unsafe_blocks`]: set
`accept-comment-above-statement` and `accept-comment-above-attributes`
to `true` by default.
Add redundant_as_str lint
This lint checks for `as_str` on a `String` immediately followed by `as_bytes` or `is_empty` as those methods are available on `String` too. This could possibly also be extended to `&[u8]` in the future.
changelog: New lint [`redundant_as_str`] #11526
move required_consts check to general post-mono-check function
This factors some code that is common between the interpreter and the codegen backends into shared helper functions. Also as a side-effect the interpreter now uses the same `eval` functions as everyone else to get the evaluated MIR constants.
Also this is in preparation for another post-mono check that will be needed for (the current hackfix for) https://github.com/rust-lang/rust/issues/115709: ensuring that all locals are dynamically sized.
I didn't expect this to change diagnostics, but it's just cycle errors that change.
r? `@oli-obk`
This lint checks for `as_str` on a `String` immediately followed by `as_bytes` or `is_empty` as those methods are available on `String` too. This could possibly also be extended to `&[u8]` in the future.
Split `needless_borrow` into two lints
Splits off the case where the borrow is used as a generic argument to a function. I think the two cases are different enough to warrant a separate lint.
The tests for the new lint have been reordered to group related parts together. Two warning have been dropped, one looked like it was testing the generic argument form, but it ends up triggering the auto-deref variant. The second was just a redundant test that didn't do anything interesting.
An issue with cycle detection is also included. The old version was checking if a cycle was reachable from a block when it should have been checking if the block is part or a cycle.
As a side note, I'm liking the style of just jamming all the tests into separate scopes in main.
changelog: Split off `needless_borrows_for_generic_args` from `needless_borrow`
[`filter_map_bool_then`]: include multiple derefs from adjustments
In #11506 this lint was improved to suggest one deref if the bool is behind references (fixed the FP #11503), however it might need multiple dereferences if the bool is behind multiple layers of references or custom derefs. E.g. `&&&bool` needs `***b`.
changelog: [`filter_map_bool_then`]: suggest as many dereferences as there are needed to get to the bool
add extra `byref` checking for the guard's local
changelog: [`redundant_guards`]: Now checks if the variable is bound using `ref` before linting.
The lint should not be emitted, when the local variable is bind by-ref in the pattern.
fixes#11465
[`useless_conversion`]: don't lint if type parameter has unsatisfiable bounds for `.into_iter()` receiver
Fixes#11300.
Before this PR, clippy assumed that if it sees a `f(x.into_iter())` call and the type at that argument position is generic over any `IntoIterator`, then the `.into_iter()` call must be useless because `x` already implements `IntoIterator`, *however* this assumption is not right if the generic parameter has more than just the `IntoIterator` bound (because other traits can be implemented for the IntoIterator target type but not the IntoIterator implementor, as can be seen in the linked issue: `<[i32; 3] as IntoIterator>::IntoIter` satisfies `ExactSizeIterator`, but `[i32; 3]` does not).
So, this PR makes it check that the type parameter only has a single `IntoIterator` bound. It *might* be possible to check if the type of `x` in `f(x.into_iter())` satisfies all the bounds on the generic type parameter as defined on the function (which would allow removing the `.into_iter()` call even with multiple bounds), however I'm not sure how to do that, and the current fix should always work.
**Edit:** This PR has been changed to check if any of the bounds don't hold for the type of the `.into_iter()` receiver, so we can still lint in some cases.
changelog: [`useless_conversion`]: don't lint `.into_iter()` if type parameter has multiple bounds
fix filter_map_bool_then with a bool reference
changelog: [`filter_map_bool_then`]: Fix the incorrect autofix when the `bool` in question is a reference.
fix#11503
[`extra_unused_type_parameters`]: Fix edge case FP for parameters in where bounds
Generic parameters can end up being used on the left side of where-bounds if they are not directly bound but instead appear nested in some concrete generic type. Therefore, we should walk the left side of where bounds, but only if the bounded type is *not* a generic param, in which case we still need to ignore the bound.
Fixes#11302
changelog: [`extra_unused_type_parameters`]: Fix edge case false positive for parameters in where bounds
[`len_without_is_empty`]: follow type alias to find inherent `is_empty` method
Fixes#11165
When we see an `impl B` and `B` is a type alias to some type `A`, then we need to follow the type alias to look for an `is_empty` method on the aliased type `A`. Before this PR, it'd get the inherent impls of `B`, which there aren't any and so it would warn that there isn't an `is_empty` method even if there was one.
Passing the type alias `DefId` to `TyCtxt::type_of` gives us the aliased `DefId` (or simply return the type itself if it wasn't a type alias) so we can just use that
changelog: [`len_without_is_empty`]: follow type alias to find inherent `is_empty` method
[`implied_bounds_in_impls`]: include (previously omitted) associated types in suggestion
Fixes#11435
It now includes associated types from the implied bound that were omitted in the second bound. Example:
```rs
fn f() -> impl Iterator<Item = u8> + ExactSizeIterator> {..}
```
Suggestion before this change:
```diff
- pub fn my_iter() -> impl Iterator<Item = u32> + ExactSizeIterator {
+ pub fn my_iter() -> impl ExactSizeIterator {
```
It didn't include `<Item = u32>` on `ExactSizeIterator`. Now, with this change, it does.
```diff
- pub fn my_iter() -> impl Iterator<Item = u32> + ExactSizeIterator {
+ pub fn my_iter() -> impl ExactSizeIterator<Item = u32> {
```
We also now extend the span to include not just possible `+` ahead of it, but also behind it (an example for this is in the linked issue as well).
**Note:** The overall diff is a bit noisy, because building up the suggestion involves quite a bit more logic now and I decided to extract that into its own function. For that reason, I split this PR up into two commits. The first commit contains the actual "logic" changes. Second commit just moves code around.
changelog: [`implied_bounds_in_impls`]: include (previously omitted) associated types in suggestion
changelog: [`implied_bounds_in_impls`]: include the `+` behind bound if it's the last bound
Rename incorrect_impls to non_canonical_impls, move them to warn by default
The wording/category of these feel too strong to me, I would expect most of the time it's linting the implementations aren't going to be *incorrect*, just unnecessary
changelog: rename `incorrect_clone_impl_on_copy_type` to [`non_canonical_clone_impl`]
changelog: rename `incorrect_partial_ord_impl_on_ord_type` to [`non_canonical_partial_ord_impl`]
changelog: Move [`non_canonical_clone_impl`], [`non_canonical_partial_ord_impl`] to suspicious
Preserve literals and range kinds in `manual_range_patterns`
Fixes#11461
Also enables linting when there are 3 or fewer alternatives if one of them is already a range pattern
changelog: none
[`slow_vector_initialization`]: use the source span of vec![] macro and fix another FP
Fixes#11408
<details>
<summary>Also fixes a FP when the vec initializer comes from a macro other than `vec![]`</summary>
```rs
macro_rules! x {
() => { vec![] }
}
fn f() {
let mut v = x!();
v.resize(10, 0);
}
```
This shouldn't warn. The `x!` macro might be doing other things, so just replacing `x!()` with `vec![0; 10]` is not always an option.
</details>
I added some test cases for macro expansions, however I don't think there's a way to write a test for that specific warning that appeared in the linked issue. As far as I understand, that happens when the rust-src rustup component isn't installed (so the stdlib source is unavailable) and the span points to the `vec![]` *expansion*, instead of the `vec![]` that the user wrote.
changelog: [`slow_vector_initialization`]: use the source span of `vec![]` macro
changelog: [`slow_vector_initialization`]: only warn on `vec![]` expansions and allow other macros
skip `todo!()` in `never_loop`
As promised in #11450, here is an implementation which skips occurrences of the `todo!()` macro.
changelog: [`never_loop`]: skip loops containing `todo!()`
Don't pass extra generic arguments in `needless_borrow`
fixes#10253
Also switches to using `implements_trait` which does ICE when clippy's debug assertions are enabled.
changelog: None
[`implied_bounds_in_impls`]: don't ICE on default generic parameter and move to nursery
Fixes#11422
This fixes two ICEs ([1](https://github.com/rust-lang/rust-clippy/issues/11422#issue-1872351763), [2](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=2901e6febb479d3bd2a74f8a5b8a9305)), and moves it to nursery for now, because this lint needs some improvements in its suggestion (see #11435, for one such example).
changelog: Moved [`implied_bounds_in_impls`] to nursery (Now allow-by-default)
[#11437](https://github.com/rust-lang/rust-clippy/pull/11437)
changelog: [`implied_bounds_in_impls`]: don't ICE on default generic parameter in supertrait clause
r? `@xFrednet` (since you reviewed my PR that added this lint, I figured it might make sense to have you review this as well since you have seen this code before. If you don't want to review this, sorry! Feel free to reroll then)
--------
As for the ICE, it's pretty complicated and very confusing imo, so I'm going to try to explain the idea here (partly for myself, too, because I've confused myself several times writing- and fixing this):
<details>
<summary>Expand</summary>
The general idea behind the lint is that, if we have this function:
```rs
fn f() -> impl PartialEq<i32> + PartialOrd<i32> { 0 }
```
We want to lint the `PartialEq` bound because it's unnecessary. That exact bound is already specified in `PartialOrd<i32>`'s supertrait clause:
```rs
trait PartialOrd<Rhs>: PartialEq<Rhs> {}
// PartialOrd<i32>: PartialEq<i32>
```
The way it does this is in two steps:
- Go through all of the bounds in the `impl Trait` return type and collect each of the trait's supertrait bounds into a vec. We also store the generic arguments for later.
- `PartialEq` has no supertraits, nothing to add.
- `PartialOrd` is defined as `trait PartialOrd: PartialEq`, so add `PartialEq` to the list, as well as the generic argument(s) `<i32>`
Once we are done, we have these entries in the vec: `[(PartialEq, [i32])]`
- Go through all the bounds again, and looking for those bounds that have their trait `DefId` in the implied bounds vec.
- `PartialEq` is in that vec. However, that is not enough, because the trait is generic. If the user wrote `impl PartialEq<String> + PartialOrd<i32>`, then `PartialOrd` clearly doesn't imply `PartialEq`. Which means, we also need to check that the generic parameters match. This is why we also collected the generic arguments in `PartialOrd<i32>`. This process of checking generic arguments is pretty complicated and is also where the two ICEs happened.
The way it checks that the generic arguments match is by comparing the generic parameters in the super trait clause:
```rs
trait PartialOrd<Rhs>: PartialEq<Rhs> {}
// ^^^^^^^^^^^^^^
```
...this needs to match...
```rs
fn f() -> impl PartialEq<i32> + ...
// ^^^^^^^^^^^^^^
```
In the compiler, the `Rhs` generic parameter is its own type and we cannot just compare it to `i32`. We need to "substitute" it.
Internally, `Rhs` is represented as `Rhs#1` (the number next to # represents the type parameter index. They start at 0, but 0 is "reserved" for the implicit `Self` generic parameter).
How do we go from `Rhs#1` to `i32`? Well, we know that all the generic parameters had to be substituted in the `impl ... + PartialOrd<i32>` type. So we subtract 1 from the type parameter index, giving us 0 (`Self` is not specified in that list of arguments). We use that as the index into the generic argument list `<i32>`. That's `i32`. Now we know that the supertrait clause looks like `: PartialEq<i32>`.
Then, we can compare that to what the user actually wrote on the bound that we think is being implied: `impl PartialEq<i32> + ...`.
Now to the actual bug: this whole logic doesn't take into account *default* generic parameters. Actually, `PartialOrd` is defined like this:
```rs
trait PartialOrd<Rhs = Self>: PartialEq<Rhs> {}
```
If we now have a function like this:
```rs
fn f() -> impl PartialOrd + PartialEq {}
```
that logic breaks apart... We look at the supertrait predicate `: PartialEq<Rhs>` (`Rhs` is `Rhs#1`), then take the first argument in the generic argument list `PartialEq<..>` to resolve the `Rhs`, but at this point we crash because there *is no* generic argument.
The index 0 is out of bounds. If this happens (and we even get to linting here, which could only happen if it passes typeck), it must mean that that generic parameter has a default type that is not required to be specified.
This PR changes the logic such that if we have a type parameter index that is out of bounds, it looks at the definition of the trait and check that there exists a default type that we can use instead.
So, we see `<Rhs = Self>`, and use `Self` for substitution, and end up with this predicate: `: PartialEq<Self>`. No crash this time.
</details>
Also stabilizes saturating_int_assign_impl, gh-92354.
And also make pub fns const where the underlying saturating_*
fns became const in the meantime since the Saturating type was
created.
Fix span when linting `explicit_auto_deref` immediately after `needless_borrow`
fixes#11366
changelog: `explicit_auto_deref`: Fix span when linting immediately after `needless_borrow`
Add config flag for reborrows in explicit_iter_loop
This PR adds a config flag for enforcing explicit into iter lint for reborrowed values. The config flag, `enforce_iter_loop_reborrow`, can be added to clippy.toml files to enable the linting behaviour. By default the reborrow lint is disabled.
fixes: #11074
changelog: [`explicit_iter_loop`]: add config flag `enforce_iter_loop_reborrow` to disable reborrow linting by default
new lint: `iter_out_of_bounds`
Closes#11345
The original idea in the linked issue seemed to be just about arrays afaict, but I extended this to catch some other iterator sources such as `iter::once` or `iter::empty`.
I'm not entirely sure if this name makes a lot of sense now that it's not just about arrays anymore (specifically, not sure if you can call `.take(1)` on an `iter::Empty` to be "out of bounds"?).
changelog: [`iter_out_of_bounds`]: new lint
[`unnecessary_unwrap`]: lint on `.as_ref().unwrap()`
Closes#11371
This turned out to be a little more code than I originally thought, because the lint also makes sure to not lint if the user tries to mutate the option:
```rs
if option.is_some() {
option = None;
option.unwrap(); // don't lint here
}
```
... which means that even if we taught this lint to recognize `.as_mut()`, it would *still* not lint because that would count as a mutation. So we need to allow `.as_mut()` calls but reject other kinds of mutations.
Unfortunately it doesn't look like this is possible with `is_potentially_mutated` (seeing what kind of mutation happened).
This replaces it with a custom little visitor that does basically what it did before, but also allows `.as_mut()`.
changelog: [`unnecessary_unwrap`]: lint on `.as_ref().unwrap()`
skip float_cmp check if lhs is a custom type
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: [`float_cmp`]: allow float eq comparison when lhs is a custom type that implements PartialEq<f32/f64>
If the lhs of a comparison is not float, it means there is a user implemented PartialEq, and the caller is invoking that custom version of `==`, instead of the default floating point equal comparison.
People may wrap f32 with a struct (say `MyF32`) and implement its PartialEq that will do the `is_close()` check, so that `MyF32` can be compared with either f32 or `MyF32`.
[`if_then_some_else_none`]: look into local initializers for early returns
Fixes#11394
As the PR title says, problem was that it only looked for early returns in semi statements. Local variables don't count as such, so it didn't count `let _v = x?;` (or even just `let _ = return;`) as a possible early return and didn't realize that it can't lint then.
Imo the `stmts_contains_early_return` function that was used before is redundant. `contains_return` could already do that if we just made the parameter a bit more generic, just like `for_each_expr`, which can already accept `&[Stmt]`
changelog: [`if_then_some_else_none`]: look into local initializers for early returns
This commit adds a config flag for enforcing explicit into iter lint
for reborrowed values. The config flag, enforce_iter_loop_reborrow, can be
added to clippy.toml files to enable the linting behaviour. By default
the lint is not enabled.
fix the uitest `enum_clike_unportable_variant`
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: none
fix "derivable_impls: attributes are ignored"
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: [`derivable_impls`]: allow the lint when the trait-impl methods has any attribute.
