Fixed FP in `unused_io_amount` for Ok(lit), unrachable! and unwrap de…
…sugar
Fixes fp caused by linting on Ok(_) for all cases outside binding.
We introduce the following rules for match exprs.
- `panic!` and `unreachable!` are treated as consumed.
- `Ok( )` patterns outside `DotDot` and `Wild` are treated as consuming.
changelog: FP [`unused_io_amount`] when matching Ok(literal) or unreachable
fixes#12208
r? `@blyxyas`
We introduce the following rules for match exprs.
- `panic!` and `unreachable!` are treated as consumption.
- guard expressions in any arm imply consumption.
For match exprs:
- Lint only if exacrtly 2 non-consuming arms exist
- Lint only if one arm is an `Ok(_)` and the other is `Err(_)`
Added additional requirement that for a block return expression
that is a match, the source must be `Normal`.
changelog: FP [`unused_io_amount`] when matching Ok(literal)
`Diagnostic::keys`, which is used for hashing and equating diagnostics,
has a surprising behaviour: it ignores children, but only for lints.
This was added in #88493 to fix some duplicated diagnostics, but it
doesn't seem necessary any more.
This commit removes the special case and only four tests have changed
output, with additional errors. And those additional errors aren't
exact duplicates, they're just similar. For example, in
src/tools/clippy/tests/ui/same_name_method.rs we currently have this
error:
```
error: method's name is the same as an existing method in a trait
--> $DIR/same_name_method.rs:75:13
|
LL | fn foo() {}
| ^^^^^^^^^^^
|
note: existing `foo` defined here
--> $DIR/same_name_method.rs:79:9
|
LL | impl T1 for S {}
| ^^^^^^^^^^^^^^^^
```
and with this change we also get this error:
```
error: method's name is the same as an existing method in a trait
--> $DIR/same_name_method.rs:75:13
|
LL | fn foo() {}
| ^^^^^^^^^^^
|
note: existing `foo` defined here
--> $DIR/same_name_method.rs:81:9
|
LL | impl T2 for S {}
| ^^^^^^^^^^^^^^^^
```
I think printing this second argument is reasonable, possibly even
preferable to hiding it. And the other cases are similar.
Add regression ui test for #2371Fixes#2371.
#2371 seems to already be handled correctly in the lint. This PR adds a ui regression test so we can close it.
r? `@blyxyas`
changelog: Add regression ui test for #2371
[fix] [`redundant_closure_for_method_calls`] Suggest relative paths for local modules
Fixes#10854.
Currently, `redundant_closure_for_method_calls` suggest incorrect paths when a method defined on a struct within inline mod is referenced (see the description in the aforementioned issue for an example; also see [this playground link](https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=f7d3c5b2663c9bd3ab7abdb0bd38ee43) for the current-version output for the test cases added in this PR). It will now try to construct a relative path path to the module and suggest it instead.
changelog: [`redundant_closure_for_method_calls`] Fix incorrect path suggestions for types within local modules
FP: `needless_return_with_question_mark` with implicit Error Conversion
Return with a question mark was triggered in situations where the `?` desuraging was performing error conversion via `Into`/`From`.
The desugared `?` produces a match over an expression with type `std::ops::ControlFlow<B,C>` with `B:Result<Infallible, E:Error>` and `C:Result<_, E':Error>`, and the arms perform the conversion. The patch adds another check in the lint that checks that `E == E'`. If `E == E'`, then the `?` is indeed unnecessary.
changelog: False Positive: [`needless_return_with_question_mark`] when implicit Error Conversion occurs.
fixes: #11982
fix: incorrect suggestions generated by `manual_retain` lint
fixes#10393, fixes#11457, fixes#12081#10393: In the current implementation of `manual_retain`, if the argument to the closure is matched using tuple, they are all treated as the result of a call to `map.into_iter().filter(<f>)`. However, such tuple pattern matching can also occur in many different containers that stores tuples internally. The correct approach is to apply different lint policies depending on whether the receiver of `into_iter` is a map or not.
#11457 and #12081: In the current implementation of `manual_retain`, if the argument to the closure is `Binding`, the closure will be used directly in the `retain` method, which will result in incorrect suggestion because the first argument to the `retain` closure may be of a different type. In addition, if the argument to the closure is `Ref + Binding`, the lint will simply remove the `Ref` part and use the `Binding` part as the argument to the new closure, which will lead to bad suggestion for the same reason. The correct approach is to detect each of these cases and apply lint suggestions conservatively.
changelog: [`manual_retain`] refactor and add check for various patterns
Fix/Issue11932: assert* in multi-condition after unrolling will cause lint `nonminimal_bool` emit warning
fixes [Issue#11932](https://github.com/rust-lang/rust-clippy/issues/11932)
After `assert`, `assert_eq`, `assert_ne`, etc, assert family marcos unrolling in multi-condition expressions, lint `nonminimal_bool` will recognize whole expression as a entirety, analyze each simple condition expr of them, and check whether can simplify them.
But `assert` itself is a entirety to programmers, we don't need to lint on `assert`. This commit add check whether lint snippet contains `assert` when try to warning to an expression.
changelog: [`nonminimal_bool`] add check for condition expression
[`never_loop`]: recognize desugared `try` blocks
Fixes#12205
The old code assumed that only blocks with an explicit label can be jumped to (using `break`). This is mostly correct except for `try` desugaring, where the `?` operator is rewritten to a `break` to that block, even without a label on the block. `Block::targeted_by_break` is a little more accurate than just checking if a block has a label in that regard, so we should just use that instead
changelog: [`never_loop`]: avoid linting when `?` is used inside of a try block
Fixed FP in `redundant_closure_call` when closures are passed to macros
There are cases where the closure call is needed in some macros, this in particular occurs when the closure has parameters. To handle this case, we allow the lint when there are no parameters in the closure, or the closure is outside a macro invocation.
fixes: #11274#1553
changelog: FP: [`redundant_closure_call`] when closures with parameters are passed in macros.
Warn if an item coming from more recent version than MSRV is used
Part of https://github.com/rust-lang/rust-clippy/issues/6324.
~~Currently, the lint is not working for the simple reason that the `stable` attribute is not kept in dependencies. I'll send a PR to rustc to see if they'd be okay with keeping it.~~
EDIT: There was actually a `lookup_stability` function providing this information, so all good now!
cc `@epage`
changelog: create new [`incompatible_msrv`] lint
remove StructuralEq trait
The documentation given for the trait is outdated: *all* function pointers implement `PartialEq` and `Eq` these days. So the `StructuralEq` trait doesn't really seem to have any reason to exist any more.
One side-effect of this PR is that we allow matching on some consts that do not implement `Eq`. However, we already allowed matching on floats and consts containing floats, so this is not new, it is just allowed in more cases now. IMO it makes no sense at all to allow float matching but also sometimes require an `Eq` instance. If we want to require `Eq` we should adjust https://github.com/rust-lang/rust/pull/115893 to check for `Eq`, and rule out float matching for good.
Fixes https://github.com/rust-lang/rust/issues/115881
respect `#[allow]` attributes in `single_call_fn` lint
Fixes#12182
If we delay linting to `check_crate_post`, we need to use `span_lint_hir_and_then`, since otherwise it would only respect those lint level attributes at the crate root.
<sub>... maybe we can have an internal lint for this somehow?</sub>
changelog: respect `#[allow]` attributes in `single_call_fn` lint
Don't emit `derive_partial_eq_without_eq` lint if the type has the `non_exhaustive` attribute
Part of https://github.com/rust-lang/rust-clippy/issues/9063.
If a type has a field/variant with the `#[non_exhaustive]` attribute or the type itself has it, then do no emit the `derive_partial_eq_without_eq` lint.
changelog: Don't emit `derive_partial_eq_without_eq` lint if the type has the `non_exhaustive` attribute
`unused_io_amount` captures `Ok(_)`s
Partial rewrite of `unused_io_amount` to lint over `Ok(_)` and `Ok(..)`.
Moved the check to `check_block` to simplify context checking for expressions and allow us to check only some expressions.
For match (expr, arms) we emit a lint for io ops used on `expr` when an arm is `Ok(_)|Ok(..)`. Also considers the cases when there are guards in the arms and `if let Ok(_) = ...` cases.
For `Ok(_)` and `Ok(..)` it emits a note indicating where the value is ignored.
changelog: False Negatives [`unused_io_amount`]: Extended `unused_io_amount` to catch `Ok(_)`s in `If let` and match exprs.
Closes#11713
r? `@giraffate`
Partial rewrite of `unused_io_account` to lint over Ok(_).
Moved the check to `check_block` to simplify context checking for
expressions and allow us to check only some expressions.
For match (expr, arms) we emit a lint for io ops used on `expr` when an
arm is `Ok(_)`. Also considers the cases when there are guards in the
arms. It also captures `if let Ok(_) = ...` cases.
For `Ok(_)` it emits a note indicating where the value is ignored.
changelog: False Negatives [`unused_io_amount`]: Extended
`unused_io_amount` to catch `Ok(_)`s in `If let` and match exprs.
Prefixing a variable with a `_` does not mean that it will not be used.
If such a variable is used later, do not warn about the fact that its
initialization does not have a side effect as this is fine.
Fix error warning span for issue12045
fixes [Issue#12045](https://github.com/rust-lang/rust-clippy/issues/12045)
In issue#12045, unexpected warning span occurs on attribute `#[derive(typed_builder::TypedBuilder)]`, actually the warning should underline `_lifetime`.
In the source code we can find that the original intend is to warning on `ident.span`, but in this case, `stmt.span` is unequal with `ident.span`. So, fix the nit here is fine.
Besides, `ident.span` have an accurate range than `stmt.span`.
changelog: [`no_effect_underscore_binding`]: correct warning span
fix FP on [`semicolon_if_nothing_returned`]
fixes: #12123
---
changelog: fix FP on [`semicolon_if_nothing_returned`] which suggesting adding semicolon after attr macro
Correctly handle type relative in trait_duplication_in_bounds lint
Fixes#9961.
The generic bounds were not correctly checked and left out `QPath::TypeRelative`, making different bounds look the same and generating invalid errors (and fix).
r? `@blyxyas`
changelog: [`trait_duplication_in_bounds`]: Correctly handle type relative.
`read_zero_byte_vec` refactor for better heuristics
Fixes#9274
Previously, the implementation of `read_zero_byte_vec` only checks for the next statement after the vec init. This fails when there is a block with statements that are expanded and walked by the old visitor.
This PR refactors so that:
1. It checks if there is a `resize` on the vec
2. It works on blocks properly
e.g. This should properly lint now:
```
let mut v = Vec::new();
{
f.read(&mut v)?;
//~^ ERROR: reading zero byte data to `Vec`
}
```
changelog: [`read_zero_byte_vec`] Refactored for better heuristics
Add suspicious_open_options lint.
changelog: [`suspicious_open_options`]: Checks for the suspicious use of std::fs::OpenOptions::create() without an explicit OpenOptions::truncate().
create() alone will either create a new file or open an existing file. If the file already exists, it will be overwritten when written to, but the file will not be truncated by default. If less data is written to the file than it already contains, the remainder of the file will remain unchanged, and the end of the file will contain old data.
In most cases, one should either use `create_new` to ensure the file is created from scratch, or ensure `truncate` is called so that the truncation behaviour is explicit. `truncate(true)` will ensure the file is entirely overwritten with new data, whereas `truncate(false)` will explicitely keep the default behavior.
```rust
use std::fs::OpenOptions;
OpenOptions::new().create(true).truncate(true);
```
- [x] Followed [lint naming conventions][lint_naming]
- [x] Added passing UI tests (including committed `.stderr` file)
- [x] `cargo test` passes locally
- [x] Executed `cargo dev update_lints`
- [x] Added lint documentation
- [x] Run `cargo dev fmt`
Correctly suggest std or core path depending if this is a `no_std` crate
A few lints emit suggestions using `std` paths whether or not this is a `no_std` crate, which is an issue when running `rustfix` afterwards. So in case this is an item that is defined in both `std` and `core`, we need to check if the crate is `no_std` to emit the right path.
r? `@llogiq`
changelog: Correctly suggest std or core path depending if this is a `no_std` crate
- New ineffective_open_options had to be fixed.
- Now not raising an issue on missing `truncate` when `append(true)`
makes the intent clear.
- Try implementing more advanced tests for non-chained operations. Fail
Checks for the suspicious use of OpenOptions::create()
without an explicit OpenOptions::truncate().
create() alone will either create a new file or open an
existing file. If the file already exists, it will be
overwritten when written to, but the file will not be
truncated by default. If less data is written to the file
than it already contains, the remainder of the file will
remain unchanged, and the end of the file will contain old
data.
In most cases, one should either use `create_new` to ensure
the file is created from scratch, or ensure `truncate` is
called so that the truncation behaviour is explicit.
`truncate(true)` will ensure the file is entirely overwritten
with new data, whereas `truncate(false)` will explicitely
keep the default behavior.
```rust
use std::fs::OpenOptions;
OpenOptions::new().create(true).truncate(true);
```
The OpenTelemetry project's name is all one word (see https://opentelemetry.io),
so currently triggers a false positive in the `doc_markdown` lint.
The project is increasing rapidly in popularity, so it seems like a worthy
contender for inclusion in the default `doc_valid_idents` configuration.
I've also moved the existing "OpenDNS" entry earlier in the list, to restore
the alphabetical ordering of that "Open*" row.
The docs changes were generated using `cargo collect-metadata`.
changelog: [`doc_markdown`]: Add `OpenTelemetry` to the default configuration as an allowed identifier
[`useless_asref`]: check that the clone receiver is the parameter
Fixes#12135
There was no check for the receiver of the `clone` call in the map closure. This makes sure that it's a path to the parameter.
changelog: [`useless_asref`]: check that the clone receiver is the closure parameter
Make `HirEqInterExpr::eq_block` take comments into account while checking if two blocks are equal
This PR:
- now makes `HirEqInterExpr::eq_block` take comments into account. Identical code with varying comments will no longer be considered equal.
- makes necessary adjustments to UI tests.
Closes#12044
**Lintcheck Changes**
- `match_same_arms` 53 => 52
- `if_same_then_else` 3 => 0
changelog: [`if_same_then_else`]: Blocks with different comments will no longer trigger this lint.
changelog: [`match_same_arms`]: Arms with different comments will no longer trigger this lint.
```
There are cases where the closure call is needed in some macros, this in
particular occurs when the closure has parameters. To handle this case,
we allow the lint when there are no parameters in the closure, or the
closure is outside a macro invocation.
fixes: #11274, #1553
changelog: FP: [`redundant_closure_call`] when closures with parameters
are passed in macros.
Fix false positive in `PartialEq` check in `unconditional_recursion` lint
Fixes https://github.com/rust-lang/rust-clippy/issues/12133.
We needed to check for the type of the previous element <del>in case it's a field</del>.
EDIT: After some extra thoughts, no need to check if it's a field, just if it's the same type as `Self`.
r? `@llogiq`
changelog: Fix false positive in `PartialEq` check in `unconditional_recursion` lint
Fix suggestion for `map_clone` lint on types implementing `Copy`
Follow-up of https://github.com/rust-lang/rust-clippy/pull/12104.
It was missing this check to suggest the correct method.
r? `@llogiq`
changelog: Fix suggestion for `map_clone` lint on types implementing `Copy`
fix/issue#11243: allow 3-digit-grouped binary in non_octal_unix_permissions
fixes [Issue#11243](https://github.com/rust-lang/rust-clippy/issues/11243)
Issue#11243 suggest lint `non_octal_unix_permissions` should not report binary format literal unix permissions as an error, and we think binary format is a good way to understand these permissions.
To solve this problem, we need to add check for binary literal, which is written in function `check_binary_unix_permissions` , only `binary, 3 groups and each group length equals to 3` is a legal format.
changelog: [`non_octal_unix_permissions`]: Add check for binary format literal unix permissions like 0b111_111_111
Fixed ICE introduced in #12004
Issue: in https://github.com/rust-lang/rust-clippy/pull/12004, we emit a lint for `filter(Option::is_some)`. If the
parent expression is a `.map` we don't emit that lint as there exists a
more specialized lint for that.
The ICE introduced in https://github.com/rust-lang/rust-clippy/pull/12004 is a consequence of the assumption that a
parent expression after a filter would be a method call with the filter
call being the receiver. However, it is entirely possible to have a
closure of the form
```
|| { vec![Some(1), None].into_iter().filter(Option::is_some) }
```
The previous implementation looked at the parent expression; namely the
closure, and tried to check the parameters by indexing [0] on an empty
list.
This commit is an overhaul of the lint with significantly more FP tests
and checks.
Impl details:
1. We verify that the filter method we are in is a proper trait method
to avoid FPs.
2. We check that the parent expression is not a map by checking whether
it exists; if is a trait method; and then a method call.
3. We check that we don't have comments in the span.
4. We verify that we are in an Iterator of Option and Result.
5. We check the contents of the filter.
1. For closures we peel it. If it is not a single expression, we don't
lint. We then try again by checking the peeled expression.
2. For paths, we do a typecheck to avoid FPs for types that impl
functions with the same names.
3. For calls, we verify the type, via the path, and that the param of
the closure is the single argument to the call.
4. For method calls we verify that the receiver is the parameter of
the closure. Since we handle single, non-block exprs, the
parameter can't be shadowed, so no FP.
This commit also adds additional FP tests.
Fixes: #12058
Adding `@xFrednet` as you've the most context for this as you reviewed it last time.
`@rustbot` r? `@xFrednet`
---
changelog: none
(Will be backported and therefore don't effect stable)
Issue: in #12004, we emit a lint for `filter(Option::is_some)`. If the
parent expression is a `.map` we don't emit that lint as there exists a
more specialized lint for that.
The ICE introduced in #12004 is a consequence of the assumption that a
parent expression after a filter would be a method call with the filter
call being the receiver. However, it is entirely possible to have a
closure of the form
```
|| { vec![Some(1), None].into_iter().filter(Option::is_some) }
```
The previous implementation looked at the parent expression; namely the
closure, and tried to check the parameters by indexing [0] on an empty
list.
This commit is an overhaul of the lint with significantly more FP tests
and checks.
Impl details:
1. We verify that the filter method we are in is a proper trait method
to avoid FPs.
2. We check that the parent expression is not a map by checking whether
it exists; if is a trait method; and then a method call.
3. We check that we don't have comments in the span.
4. We verify that we are in an Iterator of Option and Result.
5. We check the contents of the filter.
1. For closures we peel it. If it is not a single expression, we don't
lint.
2. For paths, we do a typecheck to avoid FPs for types that impl
functions with the same names.
3. For calls, we verify the type, via the path, and that the param of
the closure is the single argument to the call.
4. For method calls we verify that the receiver is the parameter of
the closure. Since we handle single, non-block exprs, the
parameter can't be shadowed, so no FP.
This commit also adds additional FP tests.
Handle "calls" inside the closure as well in `map_clone` lint
Follow-up of https://github.com/rust-lang/rust-clippy/pull/12104.
I just realized that I didn't handle the case where the `clone` method was made as a call and not a method call.
r? `@llogiq`
changelog: Handle "calls" inside the closure as well in `map_clone` lint
improve [`cast_sign_loss`], to skip warning on always positive expressions
fixes: #11642
changelog: improve [`cast_sign_loss`] to skip warning on always positive expressions
Turns out this is change became quite big, and I still can't cover all the cases, like method calls such as `POSITIVE_NUM.mul(POSITIVE_NUM)`, or `NEGATIVE_NUM.div(NEGATIVE_NUM)`... but well, if I do, I'm scared that this will goes forever, so I stopped, unless it needs to be done, lol.
Do not suggest `[T; n]` instead of `vec![T; n]` if `T` is not `Copy`
changelog: [`useless_vec`]: do not suggest replacing `&vec![T; N]` by `&[T; N]` if `T` is not `Copy`
Fix#11958
Extend `map_clone` lint to also work on non-explicit closures
I found it weird that this case was not handled by the current line so I added it. The only thing is that I don't see an obvious way to infer the current type to determine if it's copyable or not, so for now I always suggest `cloned` and I added a FIXME.
r? `@llogiq`
changelog: Extend `map_clone` lint to also work on non-explicit closures
don't change eagerness for struct literal syntax with significant drop
Fixes the bug reported by `@ju1ius` in https://github.com/rust-lang/rust-clippy/issues/9427#issuecomment-1878428001.
`eager_or_lazy` already understands to suppress eagerness changes when the expression type has a significant drop impl, but only for initialization of tuple structs or unit structs. This changes it to also avoid changing it for `Self { .. }` and `TypeWithDrop { .. }`
changelog: [`unnecessary_lazy_eval`]: don't suggest changing eagerness for struct literal syntax when type has a significant drop impl
