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
disallow calls to `LintContext::struct_span_lint` and `TyCtxt::struct_span_lint_hir`
`LintContext::struct_span_lint` and `TyCtxt::struct_span_lint_hir` don't show the link to the clippy documentation, see: #11805
In #11810, the last few calls to those methods were replaced with `span_lint_*`. It seems like we should just disallow them altogether so that no new code tries to use them.
The existing `disallowed_methods` lint makes this easy.
changelog: none
[`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
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`.
`unwrap_or_else_default` -> `unwrap_or_default` and improve resulting lint
Resolves#10080 (though it doesn't implement exactly what's described there)
This PR does the following:
1. Merges `unwrap_or_else_default.rs`'s code into `or_fun_call.rs`
2. Extracts the code to handle `unwrap_or(/* default value */)` and similar, and moves it into `unwrap_or_else_default`
3. Implements the missing functionality from #9342, e.g.,, to handle `or_insert_with(Default::default)`
4. Renames `unwrap_or_else_default` to `unwrap_or_default` (since the "new" lint handles both `unwrap_or` and `unwrap_or_else`, it seemed sensible to use the shortened name)
This PR is currently two commits. The first implements 1-3, the second implements 4.
A word about 2: the `or_fun_call` lint currently produces warnings like the following:
```
error: use of `unwrap_or` followed by a call to `new`
--> $DIR/or_fun_call.rs:56:14
|
LL | with_new.unwrap_or(Vec::new());
| ^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()`
```
To me, such warnings look like they should come from `unwrap_or_else_default`, not `or_fun_call`, especially since `or_fun_call` is [in the nursery](https://github.com/rust-lang/rust-clippy/pull/9829).
---
changelog: Move: Renamed `unwrap_or_else_default` to [`unwrap_or_default`]
[#10120](https://github.com/rust-lang/rust-clippy/pull/10120)
changelog: Enhancement: [`unwrap_or_default`]: Now handles more functions, like `or_insert_with`
[#10120](https://github.com/rust-lang/rust-clippy/pull/10120)
<!-- changelog_checked-->
check that the types are equal in `SpanlessEq::eq_expr`
Fixes#11213
changelog: [`if_same_then_else`]: don't lint for integer literals of different types
Fix async functions handling for `needless_pass_by_ref_mut` lint
Fixes https://github.com/rust-lang/rust-clippy/issues/11179.
The problem with async is that "internals" are actually inside a closure from the `ExprUseVisitor` point of view, meaning we need to actually run the check on the closures' body as well.
changelog: none
r? `@llogiq`
Make `comparison_to_empty` work on `if let`/`let` chains
This adds `LetChain` to `clippy_utils::higher`, other lints may benefit from such a change as well :D
changelog: Enhancement: [`comparison_to_empty`]: Now lints on `if let`
[`unused_async`]: don't lint if paths reference async fn without immediate call
Fixes#9695Fixes#9359
Clippy shouldn't lint unused `async` if there are paths referencing them if that path isn't the receiver of a function call, because that means that the function might be passed to some other function:
```rs
async fn f() {} // No await statements, so unused at this point
fn requires_fn_future<F: Future<Output = ()>>(_: fn() -> F) {}
requires_fn_future(f); // `f`'s asyncness is actually not unused.
```
(This isn't limited to just passing the function as a parameter to another function, it could also first be stored in a variable and later passed to another function as an argument)
This requires delaying the linting until post-crate and collecting path references to local async functions along the way.
changelog: [`unused_async`]: don't lint if paths reference async fn that require asyncness
fix dogfood lints in `redundant_local`
keep `redundant_local` from running in proc macros
rewrite `redundant_local` as late pass
make redundant_local's `find_binding` more readable
pluralize `redundant_locals` name
add test for `redundant_locals` in macros
test `redundant_locals` in proc macros
use more destructuring in `redundant_locals`
fix: format redundant_locals.rs
ignore needless_pass_by_mut_ref in redundant_locals test
Allow `Self::cmp(self, other)` as a correct impl
Fixes#11178
Also no longer checks if the method name is *just* cmp, but the path. That was an oversight on my part ^^
r? `@xFrednet`
(and `@blyxyas` too!)
changelog: [`incorrect_partial_ord_impl_on_ord_type`]: Now allows non-method calls to `cmp` like `Self::cmp(self, other)`
fix: false positive for `option_env!` in `ifs_same_cond`
Clippy had a false positive for with `ifs_same_cond` when two if-let expressions have an `option_env!` macro. The fix is similar to the `env!` macro fix.
The following example had a clippy error:
```rust
if let Some(env1) = option_env!("ENV1") {
// ...
} else if let Some(env2) = option_env!("ENV2") {
// ...
}
```
See https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=01b85c61b56ddd900117fb247af04824
changelog: [`ifs_same_cond`]: fix false positive when using `option_env!` in if-let expressions.
[`unnecessary_literal_unwrap`]: Fix ICE on None.unwrap_or_default()
Fixes#11099Fixes#11064
I'm running into #11099 (cc `@y21)` on my Rust codebase. Clippy ICEs on this code when evaluating the `unnecessary_literal_unwrap` lint:
```rust
fn main() {
let val1: u8 = None.unwrap_or_default();
}
```
This fixes that ICE and adds an message specifically for that case:
```
error: used `unwrap_or_default()` on `None` value
--> $DIR/unnecessary_literal_unwrap.rs:26:5
|
LL | None::<String>.unwrap_or_default();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the `None` and `unwrap_or_default()`: `String::default()`
```
This PR also fixes the same ICE with `None.unwrap_or_else` (by giving the generic error message for the lint in that case).
changelog: Fix ICE in `unnecessary_literal_unwrap` on `None.unwrap_or_default()`
Clippy had a false positive for with `ifs_same_cond` when two
if-let expressions have an `option_env!` macro. The fix is similar to the
`env!` macro fix.
The following example had a clippy error:
```rust
if let Some(env1) = option_env!("ENV1") {
// ...
} else if let Some(env2) = option_env!("ENV2") {
// ...
}
```
See https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=01b85c61b56ddd900117fb247af04824
changelog: Fix [`ifs_same_cond`] false positive when using `option_env!` in if-let expressions.
[`manual_filter_map`]: lint on `matches` and pattern matching
Fixes#8010
Previously this lint only worked specifically for a very limited set of methods on the filter call (`.filter(|opt| opt.is_some())` and `.filter(|res| res.is_ok())`). This PR extends it to also recognize `matches!` in the `filter` and pattern matching with `if let` or `match` in the `map`.
Example:
```rs
enum Enum {
A(i32),
B,
}
let _ = [Enum::A(123), Enum::B].into_iter()
.filter(|x| matches!(x, Enum::A(_)))
.map(|x| if let Enum::A(s) = x { s } else { unreachable!() });
```
Now suggests:
```diff
- .filter(|x| matches!(x, Enum::A(_))).map(if let Enum::A(s) = x { s } else { unreachable!() })
+ .filter_map(|x| match x { Enum::A(s) => Some(s), _ => None })
```
Adding this required a somewhat large change in code because it originally seemed to be specifically written with only method calls in the filter in mind, and `matches!` has different behavior in the map, so this new setup should make it possible to support more "generic" cases that need different handling for the filter and map calls.
changelog: [`manual_filter_map`]: lint on `matches` and pattern matching (and some internal refactoring)
Fix `unwrap_or_else_default` false positive
This PR fixes a false positive in the handling of `unwrap_or_else` with a default value when the value is needed for type inference.
An easy example to exhibit the false positive is the following:
```rust
let option = None;
option.unwrap_or_else(Vec::new).push(1);
```
The following code would not compile, because the fact that the value is a `Vec` has been lost:
```rust
let option = None;
option.unwrap_or_default().push(1);
```
The fix is to:
- implement a heuristic to tell whether an expression's type can be determined purely from its subexpressions, and the arguments and locals they use;
- apply the heuristic to `unwrap_or_else`'s receiver.
The heuristic returns false when applied to `option` in the above example, but it returns true when applied to `option` in either of the following examples:
```rust
let option: Option<Vec<u64>> = None;
option.unwrap_or_else(Vec::new).push(1);
```
```rust
let option = None::<Vec<u64>>;
option.unwrap_or_else(Vec::new).push(1);
```
(Aside: https://github.com/rust-lang/rust-clippy/pull/10120 unfairly contained multiple changes in one PR. I am trying to break that PR up into smaller pieces.)
---
changelog: FP: [`unwrap_or_else_default`]: No longer lints if the default value is needed for type inference
Remove `#![allow(unused)]` and `--crate-name` from `cargo dev new_lint` generated tests
changelog: none
Also removes some unused flags from `ui-cargo` tests because the entrypoint is now the `Cargo.toml`, not the `.rs` files
Rewrite [`tuple_array_conversions`]
Fixes#11100Fixes#11144Fixes#11124#11082 still needs discussion and #11085 likely can't be fixed.
changelog: [`tuple_array_conversions`]: Move to `pedantic`
changelog: [`tuple_array_conversions`]: Don't lint if mutability of references changes
changelog: [`tuple_array_conversions`]: Don't lint if bindings don't come from the exact same pattern
changelog: [`tuple_array_conversions`]: Don't lint if bindings are used for more than just the conversion
Add `imports_granularity = "Module"` to rustfmt.toml
This lets rustfmt split/merge imports, `Module` seems to be the most common style in clippy
https://rust-lang.github.io/rustfmt/?version=v1.6.0&search=#imports_granularity
changelog: none
Almost all the updates other than the config file change are from `cargo dev fmt` or blessed tests, the exceptions being
- `tests/ui/single_component_path_imports.rs`
- `tests/ui/single_component_path_imports_nested_first.rs`
- `tests/ui/single_component_path_imports_self_after.rs`
- `tests/ui/single_component_path_imports_self_before.rs`
- `tests/ui/unsafe_removed_from_name.rs` (added a test with merged imports as a drive by)
- `tests/ui/wildcard_imports.rs`
- `tests/ui/wildcard_imports_2021.rs`
[`arithmetic_side_effect`]: allow different types on the right hand side for `Wrapping<T>`
Fixes#11145
This lint has a list of allowed types, one of which is `Wrapping<T>`, but it was only actually allowed if the type on the right hand side was also `Wrapping<T>`, which meant that, for example, `Wrapping<u32> += u32` would still lint. It now allows binary ops involving `Wrapping<T>` regardless of the type on the rhs.
These impls have only existed since Rust 1.60.0, so that is probably why the lint was previously not handling this correctly
changelog: [`arithmetic_side_effect`]: allow different types on the right hand side for `Wrapping<T>` (e.g. `Wrapping<T> += T`)
[`unnecessary_literal_unwrap`]: also lint `unwrap_(err_)unchecked`
Closes#11093
changelog: [`unnecessary_literal_unwrap`]: also lint `unwrap_unchecked` and `unwrap_err_unchecked`
Use depinfo to discover UI test dependencies
changelog: none
context: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Building.20test.20dependencies
This restores [the old `EXTERN_FLAGS` method](4cf5bdc60c/tests/compile-test.rs (L67-L75)) of passing `--extern` flags for building UI tests with minor changes
- Unused deps were removed
- It's now a `Vec` of args instead of a command string
- It uses a `BTreeMap` so the extern flags are in alphabetical order and deterministic
I don't know if the `HOST_LIBS` part is still required, but I figured it best to leave it in for now. If the change is accepted we can take a look if it's needed in `rust-lang/rust` after the next sync
This isn't as pleasant as having a `Cargo.toml`, though there is something satisfying about knowing the dependencies are already built and not needing to invoke `cargo`
r? `@flip1995`
This commit fixes#11025 by removing checks for `todo!`,
`unimplemented!` and `unreachable!`.
Signed-off-by: Panagiotis Foliadis <pfoliadis@hotmail.com>
Uplift `clippy::fn_null_check` lint
This PR aims at uplifting the `clippy::fn_null_check` lint into rustc.
## `incorrect_fn_null_checks`
(warn-by-default)
The `incorrect_fn_null_checks` lint checks for expression that checks if a function pointer is null.
### Example
```rust
let fn_ptr: fn() = /* somehow obtained nullable function pointer */
if (fn_ptr as *const ()).is_null() { /* ... */ }
```
### Explanation
Function pointers are assumed to be non-null, checking for their nullity is incorrect.
-----
Mostly followed the instructions for uplifting a clippy lint described here: https://github.com/rust-lang/rust/pull/99696#pullrequestreview-1134072751
`@rustbot` label: +I-lang-nominated
r? compiler
[`unnecessary_literal_unwrap`]: don't lint if binding initializer comes from expansion
Fixes https://github.com/rust-lang/rust-clippy/discussions/11109
changelog: [`unnecessary_literal_unwrap`]: don't lint if binding initializer comes from expansion
"try this" -> "try"
Current help messages contain a mix of "try", "try this", and one "try this instead". In the spirit of #10631, this PR adopts the first, as it is the most concise.
It also updates the `lint_message_conventions` test to catch cases of "try this".
(Aside: #10120 unfairly contained multiple changes in one PR. I am trying to break that PR up into smaller pieces.)
changelog: Make help messages more concise ("try this" -> "try").
Add `needless_pass_by_ref_mut` lint
changelog: [`needless_pass_by_ref_mut`]: This PR add a new lint `needless_pass_by_ref_mut` which emits a warning in case a `&mut` function argument isn't used mutably. It doesn't warn on trait and trait impls functions.
Fixes#8863.
cargo dev fmt
cargo test passes
cargo test passes
refactor a lil
Update bool_comparison.stderr
heavily refactor + bump `clippy::version`
refactor
refactor
check bounds to increase accuracy, and add todos
new lint: `read_line_without_trim`
This adds a new lint that checks for calls to `Stdin::read_line` with a reference to a string that is then attempted to parse into an integer type without first trimming it, which is always going to fail at runtime.
This is something that I've seen happen a lot to beginners, because it's easy to run into when following the example of chapter 2 in the book where it shows how to program a guessing game.
It would be nice if we could point beginners to clippy and tell them "let's see what clippy has to say" and have clippy explain to them why it fails 👀
I think this lint can later be "generalized" to work not just for `Stdin` but also any `BufRead` (which seems to be where the guarantee about the trailing newline comes from) and also, matching/comparing it to a string slice that doesn't end in a newline character (e.g. `input == "foo"` is always going to fail)
changelog: new lint: [`read_line_without_trim`]
[`useless_vec`]: add more tests and don't lint inside of macros
Closes#11084.
I realized that the fix I added in #11081 itself also causes an error in a suggestion when inside of a macro. Example:
```rs
macro_rules! x {
() => {
for _ in vec![1, 2] {}
}
}
x!();
```
Here it would suggest replacing `vec![1, 2]` with `[x!()]`, because that's what the source callsite is (reminder: it does this to get the correct span of `x!()` for code like `for _ in vec![x!()]`), but that's wrong when *inside* macros, so I decided to make it not lint if the whole loop construct is inside a macro to avoid this issue.
changelog: [`useless_vec`]: add more tests and don't lint inside of macros
r? `@Alexendoo` since these were your tests, I figured it makes most sense to assign you
Don't lint manual_let_else in cases where ? would work
Don't lint `manual_let_else` where the question mark operator `?` would be sufficient, that is, mostly in cases like:
```Rust
let v = if let Some(v) = ex { v } else { return None };
```
Also, this PR emits the `question_mark` lint for `let...else` patterns that could be written with `?` (also, only `return None` like cases).
```
changelog: [`manual_let_else`]: don't lint in cases where question_mark already lints
changelog: [`question_mark`]: lint for `let Some(...) = ex else { return None };`
```
Fixes #8755
[`useless_vec`]: use the source span for initializer
Fixes#11075.
changelog: [`useless_vec`]: use the source span for the initializer expression when inside of a macro
[`arc_with_non_send_sync`]: don't lint if type has nested type parameters
Fixes#11076
changelog: [`arc_with_non_send_sync`]: don't lint if type has nested type parameters
r? `@Manishearth`
new lint: `type_id_on_box`
Closes#7687.
A new lint that detects calling `.type_id()` on `Box<dyn Any>` (and not on the underlying `dyn Any`), which can make up for some pretty confusing bugs!
changelog: new lint: [`type_id_on_box`]
Add `SPEEDTEST`
In the `master` branch, we currently don't have any way to test the performance of a single lint in changes.
This PR adds `SPEEDTEST`, the environment variable which lets you do a speed test on a lint / category of tests with various configuration options.
Maybe we should merge this with `lintcheck` 🤔
See the book page for more information.
changelog:none
`let_and_return`: lint 'static lifetimes, don't lint borrows in closures
Fixes#11056
Now also ignores functions returning `'static` lifetimes, since I noticed the `stdin.lock()` example was still being linted but doesn't need to be since https://github.com/rust-lang/rust/pull/93965
changelog: none
