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
Now `declare_interior_mutable_const` and `borrow_interior_mutable_const` respect the `ignore-interior-mutability` configuration entry
Fix#10537
changelog: Now `declare_interior_mutable_const` and `borrow_interior_mutable_const` respect the `ignore-interior-mutability` configuration entry
[`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
Deserialize `Msrv` directly in `Conf`
Gives the error a span pointing to the invalid config value
Also puts `Conf` itself in the `OnceLock` rather than just the `Msrv` for [the `register_late_mod_pass` work](https://github.com/rust-lang/rust/pull/116731) since it will be used from two different callbacks
changelog: none
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
changelog: Now `declare_interior_mutable_const` and `borrow_interior_mutable_const` respect the `ignore-interior-mutability` configuration entry
Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
[`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
prevent ice when threshold is 0 and enum has no variants
changelog: [`enum_variant_names`]: prevent ice when threshold is 0 and enum has no variants
r? `@y21`
Fixes the same ice issue raised during review of https://github.com/rust-lang/rust-clippy/pull/11496
Add missing tests for configuration options
I noticed that a lot of lints didn't have test(s) for their configuration. This leads to issues like #11481 where the lint just does nothing with it.
This PR adds tests for *almost*[^1] all of the lints with a configuration that didn't have a test in ui-toml.
The tests that I wrote here are usually two cases: one for where it's right above or under the limit set by the config where it shouldn't lint and another one for right above where it should.
changelog: none
[^1]: allow-one-hash-in-raw-strings is ignored by needless_raw_string_hashes
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
Fix `i686-unknown-linux-gnu` CI job
When testing https://github.com/oli-obk/ui_test/pull/161 I gave `--ignored` a try, I was surprised to see many of the 32bit tests passing even though I'm on a 64bit target
Turns out the `.stderr`s were incorrect, and our `i686-unknown-linux-gnu` job has been running `x86_64-unknown-linux-gnu` so it didn't get picked up
changelog: none
fix fp when [`undocumented_unsafe_blocks`] not able to detect comment on globally defined const/static variables
fixes: #11246
changelog: fix detection on global variables for [`undocumented_unsafe_blocks`]
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.
Bump ui_test
This makes `ui_test` parse `--bless` and allows a follow up change to use `Mode::Error` (instead of `Mode::Yolo`) with `RustfixMode::Everything`
changelog: none
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.
Added new lint: `reserve_after_initialization`
Closes https://github.com/rust-lang/rust-clippy/issues/11330.
A new lint that informs the user about a more concise way to create a vector with a known capacity.
Example:
```rust
let mut v: Vec<usize> = vec![];
v.reserve(10);
```
Produces the following help:
```rust
|
2 | / let mut v: Vec<usize> = vec![];
3 | | v.reserve(10);
| |__________________^ help: consider using `Vec::with_capacity(space_hint)`: `let v: Vec<usize> = Vec::with_capacity(10);`
|
```
And can be rewritten as:
```rust
let v: Vec<usize> = Vec::with_capacity(10);
```
changelog: new lint [`reserve_after_initialization`]
Fix tuple_array_conversions lint on nightly
```
changelog: ICE: [`tuple_array_conversions`]: Don't expect array length to always be usize
```
tl;dr: changed [`Const::eval_target_usize`](https://github.com/rust-lang/rust/blob/master/compiler/rustc_middle/src/ty/consts.rs#L359) to [`Consts::try_eval_target_usize`](https://github.com/rust-lang/rust/blob/master/compiler/rustc_middle/src/ty/consts.rs#L327) to get rid of ICE.
I have encountered a problem with clippy: it caught ICE when working with a codebase that uses a lot of nightly features.
Here's a (stripped) ICE info:
```
error: internal compiler error: /rustc/5c6a7e71cd66705c31c9af94077901a220f0870c/compiler/rustc_middle/src/ty/consts.rs:361:32: expected usize, got Const { ty: usize, kind: N/#1 }
thread 'rustc' panicked at /rustc/5c6a7e71cd66705c31c9af94077901a220f0870c/compiler/rustc_errors/src/lib.rs:1635:9:
Box<dyn Any>
stack backtrace:
...
16: 0x110b9c590 - rustc_middle[449edf845976488d]::util:🐛:bug_fmt
17: 0x102f76ae0 - clippy_lints[71754038dd04c2d2]::tuple_array_conversions::all_bindings_are_for_conv
...
```
I don't really know what's going on low-level-wise, but seems like this lin assumed that the length of the array can always be treated as `usize`, and *I assume* this doesn't play well with `feat(generic_const_exprs)`.
I wasn't able to build a minimal reproducible example, but locally this fix does resolve the issue.
key idea:
for `f` in `.map(f)` and `.for_each(f)`:
1. `f` must be a closure with one parameter
2. don't lint if mutable paramter in clsure `f`: `|mut x| ...`
3. don't lint if parameter is moved
[new_without_default]: include `where` clause in suggestions, make applicable
changelog: [`new_without_default`]: include `where` clause in suggestions
Correctly handle async blocks for NEEDLESS_PASS_BY_REF_MUT
Fixes https://github.com/rust-lang/rust-clippy/issues/11299.
The problem was that the `async block`s are popping a closure which we didn't go into, making it miss the mutable access to the variables.
cc `@Centri3`
changelog: none
[`useless_conversion`]: only lint on paths to fn items and fix FP in macro
Fixes#11065 (which is actually two issues: an ICE and a false positive)
It now makes sure that the function call path points to a function-like item (and not e.g. a `const` like in the linked issue), so that calling `TyCtxt::fn_sig` later in the lint does not ICE (fixes https://github.com/rust-lang/rust-clippy/issues/11065#issuecomment-1616836099).
It *also* makes sure that the expression is not part of a macro call (fixes https://github.com/rust-lang/rust-clippy/issues/11065#issuecomment-1616919639). ~~I'm not sure if there's a better way to check this other than to walk the parent expr chain and see if any of them are expansions.~~ (edit: it doesn't do this anymore)
changelog: [`useless_conversion`]: fix ICE when call receiver is a non-fn item
changelog: [`useless_conversion`]: don't lint if argument is a macro argument (fixes a FP)
r? `@llogiq` (reviewed #10814, which introduced these issues)
Correctly handle async blocks for NEEDLESS_PASS_BY_REF_MUT
Fixes https://github.com/rust-lang/rust-clippy/issues/11299.
The problem was that the `async block`s are popping a closure which we didn't go into, making it miss the mutable access to the variables.
cc `@Centri3`
changelog: none
[`useless_conversion`]: only lint on paths to fn items and fix FP in macro
Fixes#11065 (which is actually two issues: an ICE and a false positive)
It now makes sure that the function call path points to a function-like item (and not e.g. a `const` like in the linked issue), so that calling `TyCtxt::fn_sig` later in the lint does not ICE (fixes https://github.com/rust-lang/rust-clippy/issues/11065#issuecomment-1616836099).
It *also* makes sure that the expression is not part of a macro call (fixes https://github.com/rust-lang/rust-clippy/issues/11065#issuecomment-1616919639). ~~I'm not sure if there's a better way to check this other than to walk the parent expr chain and see if any of them are expansions.~~ (edit: it doesn't do this anymore)
changelog: [`useless_conversion`]: fix ICE when call receiver is a non-fn item
changelog: [`useless_conversion`]: don't lint if argument is a macro argument (fixes a FP)
r? `@llogiq` (reviewed #10814, which introduced these issues)
Use ui_test's Windows path backslash heuristic
changelog: none
Instead of unconditionally replacing `\` with `/` we now use [`Match::PathBackslash`](https://docs.rs/ui_test/latest/ui_test/enum.Match.html#variant.PathBackslash) to only replace backslashes in paths that look like windows paths
`ui-toml` and `ui-cargo` tests still use the old way because they produce verbatim paths on windows in some tests (`\\?\C:\foo\...`) which was finnicky to get the replacement order correct with
Also removes the `ui_test` -> `compiletest` alias and `VarGuard`
Fix SPEEDTEST instructions and output
* `--nocapture` hasn't been needed anymore since forever (even before `ui_test`)
* the result was dividing by 1000 instead of the number of test runs, giving bogus (but still useful for the purpose) timing results.
changelog: fix SPEEDTEST instructions and output
redundant_locals: fix FPs on mutated shadows
Fixes#11290.
When a mutable binding is shadowed by
a mutable binding of the same name in a different scope, mutations in that scope have different meaning.
This PR fixes spurious `redundant_locals` emissions on such locals.
cc `@Centri3,` `@flip1995`
changelog: [`redundant_locals`]: fix false positives on mutated shadows
Rustup
r? `@ghost`
cc `@max-niederman` With the latest sync, I'm getting a lot of FP in the `redundant_locals` lint you recently added. Any ideas where this could come from?
changelog: none
When a mutable binding is shadowed by
a mutable binding of the same name in a different scope,
mutations in that scope have different meaning.
This commit fixes spurious `redundant_locals` emissions
on such locals.
[`redundant_guards`]: don't lint on float literals
Fixes#11304
changelog: [`redundant_guards`]: don't lint on float literals
r? `@Centri3` i figured you are probably a good reviewer for this since you implemented the lint ^^
redundant_type_annotations: only pass certain def kinds to type_of
Fixes#11190Fixesrust-lang/rust#113516
Also adds an `is_lint_allowed` check to skip the lint when it's not needed
changelog: none
Add `internal_features` lint
Implements https://github.com/rust-lang/compiler-team/issues/596
Also requires some more test blessing for codegen tests etc
`@jyn514` had the idea of just `allow`ing the lint by default in the test suite. I'm not sure whether this is a good idea, but it's definitely one worth considering. Additional input encouraged.
Fix `suspicious_xor_used_as_pow.rs` performance
The original `suspicious_xor_used_as_pow` lint had poor performance, so I fixed that + a little refactor so that module is readable.
**107 millis. -> 106 millis.** Using `SPEEDTEST` on Rust's VMs
fix#11060
changelog: [`suspicious_xor_used_as_pow`]: Improve performance by 0.934%
It lints against features that are inteded to be internal to the
compiler and standard library. Implements MCP #596.
We allow `internal_features` in the standard library and compiler as those
use many features and this _is_ the standard library from the "internal to the compiler and
standard library" after all.
Marking some features as internal wasn't exactly the most scientific approach, I just marked some
mostly obvious features. While there is a categorization in the macro,
it's not very well upheld (should probably be fixed in another PR).
We always pass `-Ainternal_features` in the testsuite
About 400 UI tests and several other tests use internal features.
Instead of throwing the attribute on each one, just always allow them.
There's nothing wrong with testing internal features^^
New lint `ignored_unit_patterns`
This idea comes from #11238. I've put the lint in `pedantic` as it might trigger numerous positives (three in Clippy itself).
changelog: [`ignored_unit_patterns`]: new lint
Suppress `question_mark` warning if `question_mark_used` is not allowed
Closes#11283
changelog: [`question_mark`]: Don't lint if `question_mark_used` is not allowed
Rename and allow `cast_ref_to_mut` lint
This PR is a small subset of https://github.com/rust-lang/rust/pull/112431, that is the renaming of the lint (`cast_ref_to_mut` -> `invalid_reference_casting`).
BUT also temporarily change the default level of the lint from deny-by-default to allow-by-default until https://github.com/rust-lang/rust/pull/112431 is merged.
r? `@Nilstrieb`
new lint: [`readonly_write_lock`]
Closes#8555
A new lint that catches `RwLock::write` calls to acquire a write lock only to read from it and not actually do any writes (mutations).
changelog: new lint: [`readonly_write_lock`]
Now `option_env_unwrap` warns even if a variable isn't set at compiletime
Fixes#10742
changelog: Fix false negative where `option_env_unwrap` wouldn't warn if the env variable isn't set at compile-time.
Currently, Clippy, Miri, Rustfmt, and rustc all use an environment variable to
indicate that output should be blessed, but they use different variable names.
In order to improve consistency, this patch applies the following changes:
- Emit `RUSTC_BLESS` within `prepare_cargo_test` so it is always
available
- Change usage of `MIRI_BLESS` in the Miri subtree to use `RUSTC_BLESS`
- Change usage of `BLESS` in the Clippy subtree to `RUSTC_BLESS`
- Change usage of `BLESS` in the Rustfmt subtree to `RUSTC_BLESS`
- Adjust the blessable test in `rustc_errors` to use this same
convention
- Update documentation where applicable
Any tools that uses `RUSTC_BLESS` should check that it is set to any value
other than `"0"`.
Fix integration tests #2
fix integration tests.
It turned out that the following tests fail to build at all:
chalk, combine, stdarch and hyper.
This is often a problem of passing `--all-targets --all-features`, in case of combine though, outdated deps were to blame.
I have opened tickets against combine and rustfmt
https://github.com/rust-lang/rustfmt/issues/5859https://github.com/Marwes/combine/issues/357
should we just remove the other failing repos? :/
changelog: fix integration tests on ci
[`slow_vector_initialization`]: catch `Vec::new()` followed by `.resize(len, 0)`
Closes#10938
changelog: [`slow_vector_initialization`]: catch `Vec::new()` followed by `.resize(len, 0)`
Remove Gha status emitter in compile-test
Disables the github specific output for now since it can be a bit confusing - https://github.com/oli-obk/ui_test/issues/109, in particular the truncation/repetition
r? `@flip1995`
changelog: none
New lint [`needless_return_with_try`]
Closes#10902
Rather than having a config option, this will just suggest removing the "return"; if `try_err` is used as well, then it'll be added again but without the `?`.
changelog: New lint [`needless_return_with_try`]
ptr_arg should ignore extern functions
Fixes: #11181
changelog: [`ptr_arg`]: ignore extern functions that are not
I am not sure whether we should ignore other Rust calling conventions like `rust-intrinsic`, `rust-call` or `rust-cold`.
