`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.
Don't forget that the lifetime on hir types is `'tcx`
This PR just tracks the `'tcx` lifetime to wherever the original objects actually have that lifetime. This code is needed for https://github.com/rust-lang/rust/pull/107606 (now #120131) so that `ast_ty_to_ty` can invoke `lit_to_const` on an argument passed to it. Currently the argument is `&hir::Ty<'_>`, but after this PR it is `&'tcx hir::Ty<'tcx>`.
no_effect_underscore_binding: _ prefixed variables can be used
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.
changelog: [`no_effect_underscore_binding`]: warn only if variable is unused
Fix#12166
Add . to end of lint lists in configuration + Fix typo in pub_underscore_fields_behavior
Fixes https://github.com/rust-lang/rust-clippy/pull/10283#issuecomment-1890600381
In the "/// Lint: " list on each configuration option, you have to end with a dot. If the lint list doesn't have a dot, the configuration won't have documentation.
This PR adds those missing dots in some of the configuration, thus also adding their documentation.
changelog: Fix bug where a lot of config documentation wasn't showing.
changelog: Fix typo in `pub_underscore_fields_behavior` (`PublicallyExported` -> `PubliclyExported`)
Find function path references early in the same lint pass
This removes a visitor that existed to collect paths to functions in a context where the exact signature is required in order to cancel the lint.
E.g. when there's a `let _: fn(&mut i32) = path_to_fn_ref_mut_i32;` statement somewhere in the crate, we shouldn't suggest removing the mutable reference in the function signature.
It was doing a whole pass through the crate at the end, which seems unnecessary.
It seems like we should be able to add entries to the map in the same lint pass.
The map is untouched all the way until `check_crate_post` (at which point it will be populated by the visitor and finally checked), so it doesn't seem like this changes behavior: it will only be fully populated by the time we reach `check_crate_post` no matter what.
I don't think this will have a significant perf impact but it did show up in a profile with 0.5% for a crate I was looking into and looked like a low hanging fruit.
changelog: none
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
Move async closure parameters into the resultant closure's future eagerly
Move async closure parameters into the closure's resultant future eagerly.
Before, we used to desugar `async |p1, p2, ..| { body }` as `|p1, p2, ..| { || async { body } }`. Now, we desugar the above like `|p1, p2, ..| { async move { let p1 = p1; let p2 = p2; ... body } }`. This mirrors the same desugaring that `async fn` does with its parameter types, and the compiler literally uses the same code via a shared helper function.
This removes the necessity for E0708, since now expressions like `async |x: i32| { x }` will not give you confusing borrow errors.
This does *not* fix the case where async closures have self-borrows. This will come with a general implementation of async closures, which is still in the works.
r? oli-obk
Fix [`multiple_crate_versions`] to correctly normalize package names to avoid missing the local one
Fixes#12145
changelog: [`multiple_crate_versions`]: correctly normalize package name
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`
Try to improve wording and fix dead link in description of arc_with_non_send_sync lint.
changelog: [`arc_with_non_send_sync`]: Improve wording and fix dead link.
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);
```
[`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
['arc_with_non_send_sync`] documentation edits
Arc's documentation uses the term "thread"; aligning to that terminology. Fix casing of "Rc".
changelog: None
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`
One consequence is that errors returned by
`maybe_new_parser_from_source_str` now must be consumed, so a bunch of
places that previously ignored those errors now cancel them. (Most of
them explicitly dropped the errors before. I guess that was to indicate
"we are explicitly ignoring these", though I'm not 100% sure.)
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
rustc_span: Optimize syntax context comparisons
Including comparisons with root context.
- `eq_ctxt` doesn't require retrieving full `SpanData`, or taking the span interner lock twice.
- Checking `SyntaxContext` for "rootness" is cheaper than extracting a full outer `ExpnData` for it and checking *it* for rootness.
The internal lint for `eq_ctxt` is also tweaked to detect `a.ctxt() != b.ctxt()` in addition to `a.ctxt() == b.ctxt()`.
Fix scoping for let chains in match guards
If let guards were previously represented as a different type of guard in HIR and THIR. This meant that let chains in match guards were not handled correctly because they were treated exactly like normal guards.
- Remove `hir::Guard` and `thir::Guard`.
- Make the scoping different between normal guards and if let guards also check for let chains.
closes#118593
Don't emit `struct_field_names` lint if all fields are booleans and don't start with the type's name
Fixes#11936.
I only checked that all fields are booleans and not the prefix (nor the suffix) because when I started to list accepted prefixes (like "is", "has", "should", "could", etc), the list was starting to get a bit too long and I thought it was not really worth for such a small change.
r? `@llogiq`
changelog: Don't emit `struct_field_names` lint if all fields are booleans and don't start with the type's name
Don't lint `let_unit_value` when `()` is explicit
since these are explicitly written (and not the result of a function call or anything else), they should be allowed, as they are both useful in some cases described in #9048Fixes#9048
changelog: [`let_unit_value`]: Don't lint when `()` is explicit
Polish `missing_enforced_import_renames` documentation
* Fixes a typo in the name of the lint (`enforce-import-renames` instead of `enforced-import-renames`).
* Copyedit “Why” paragraph.
* Make the example configuration use a multi-line list, since it is not particularly expected that a real project will have *exactly one* rename to enforce (and the old formatting had unbalanced whitespace).
changelog: none
Don't look for safety comments in doc tests
Fixes#12048.
What happened in the linked issue is that the lint checks for lines that start with `//` and have `SAFETY:` somewhere in it above the function item.
This works for regular comments, but when the `//` is the start of a doc comment (e.g. `/// // SAFETY: ...`) and it's part of a doc test (i.e. within \`\`\`), we probably shouldn't lint that, since the user most likely meant to refer to a different node than the one currently being checked. For example in the linked issue, the safety comment refers to `unsafe { *five_pointer }`, but the lint believes it's part of the function item.
We also can't really easily test whether the `// SAFETY:` comment within a doc comment is necessary or not, since I think that would require creating a new compiler session to re-parse the contents of the doc comment. We already do this for one of the doc markdown lints, to look for a main function in doc tests, but I don't know how to feel about doing that in more places, so probably best to just ignore them?
changelog: [`unnecessary_safety_comment`]: don't look for safety comments in doc tests
Add .as_ref() to suggestion to remove .to_string()
The case of `.to_owned().split(…)` is treated specially in the `unnecessary_to_owned` lint. Test cases check that it works both for slices and for strings, but they missed a corner case: `x.to_string().split(…)` when `x` implements `AsRef<str>` but not `Deref<Target = str>`. In this case, it is wrong to suggest to remove `.to_string()` without adding `.as_ref()` instead.
Fix#12068
changelog: [`unnecessary_to_owned`]: suggest replacing `.to_string()` by `.as_ref()`
new lint: `option_as_ref_cloned`
Closes#12009
Adds a new lint that looks for `.as_ref().cloned()` on `Option`s. That's the same as just `.clone()`-ing the option directly.
changelog: new lint: [`option_as_ref_cloned`]
Extend `unconditional_recursion` lint to check for `Default` trait implementation
In case the `Default` trait is implemented manually and is calling a static method (let's call it `a`) and then `a` is using `Self::default()`, it makes an infinite call recursion difficult to see without debugging. This extension checks that there is no such recursion possible.
r? `@llogiq`
changelog: Extend `unconditional_recursion` lint to check for `Default` trait implementation
Lint nested binary operations and handle field projections in `eager_transmute`
This PR makes the lint a bit stronger. Previously it would only lint `(x < 4).then_some(transmute(x))` (that is, a single binary op in the condition). With this change, it understands:
- multiple, nested binary ops: `(x < 4 && x > 1).then_some(...)`
- local references with projections: `(x.field < 4 && x.field > 1).then_some(transmute(x.field))`
changelog: [`eager_transmute`]: lint nested binary operations and look through field/array accesses
r? llogiq (since you reviewed my initial PR #11981, I figured you have the most context here, sorry if you are too busy with other PRs, feel free to reassign to someone else then)
Fix false positive `unconditional_recursion`
Fixes#12052.
Only checking if both variables are `local` was not enough, we also need to confirm they have the same type as `Self`.
changelog: Fix false positive for `unconditional_recursion` lint
Fixes: #12050 - `identity_op` correctly suggests a deference for coerced references
When `identity_op` identifies a `no_op`, provides a suggestion, it also checks the type of the type of the variable. If the variable is a reference that's been coerced into a value, e.g.
```
let x = &0i32;
let _ = x + 0;
```
the suggestion will now use a derefence. This is done by identifying whether the variable is a reference to an integral value, and then whether it gets dereferenced.
changelog: false positive: [`identity_op`]: corrected suggestion for reference coerced to value.
fixes: #12050
Adds a new lint to suggest using `const` on `thread_local!`
initializers that can be evaluated at compile time.
Impl details:
The lint relies on the expansion of `thread_local!`. For non
const-labelled initializers, `thread_local!` produces a function
called `__init` that lazily initializes the value. We check the function
and decide whether the body can be const. The body of the function is
exactly the initializer. If so, we lint the body.
changelog: new lint [`thread_local_initializer_can_be_made_const`]
Extend UNCONDITIONAL_RECURSION to check for ToString implementations
Follow-up of https://github.com/rust-lang/rust-clippy/pull/11938.
r? `@llogiq`
changelog: Extend `UNCONDITIONAL_RECURSION` to check for `ToString` implementations
New Lint: empty_enum_variants_with_brackets
This PR:
- adds a new early pass lint that checks for enum variants with no fields that were defined using brackets. **Category: Restriction**
- adds relevant UI tests for the new lint.
Closes#12007
```
changelog: New lint: [`empty_enum_variants_with_brackets`]
```
don't lint [`default_numeric_fallback`] on return and local assigned macro calls with type stated
fixes: #11535
changelog: don't lint [`default_numeric_fallback`] on return and local assigned macro calls with type stated
feat: add `manual_is_variant_and` lint
changelog: add a new lint [`manual_is_variant_and`].
- Replace `option.map(f).unwrap_or_default()` and `result.map(f).unwrap_or_default()` with `option.is_some_and(f)` and `result.is_ok_and(f)` where `f` is a function or closure that returns `bool`.
- MSRV is set to 1.70.0 for this lint; when `is_some_and` and `is_ok_and` was stabilised
---
For example, for the following code:
```rust
let opt = Some(0);
opt.map(|x| x > 1).unwrap_or_default();
```
It suggests to instead write:
```rust
let opt = Some(0);
opt.is_some_and(|x| x > 1)
```
make [`mutex_atomic`] more type aware
fixes: #9872
---
changelog: [`mutex_atomic`] now suggests more specific atomic types and skips mutex i128 and u128
When `identity_op` identifies a `no_op`, provides a suggestion, it also
checks the type of the type of the variable. If the variable is
a reference that's been coerced into a value, e.g.
```
let x = &0i32;
let _ = x + 0;
```
the suggestion will now use a derefence. This is done by identifying
whether the variable is a reference to an integral value, and then
whether it gets dereferenced.
changelog: false positive: [`identity_op`]: corrected suggestion for
reference coerced to value.
fixes: #12050
feature: add new lint `pub_underscore_fields`
fixes: #10282
This PR introduces a new lint `pub_underscore_fields` that lints when a user has marked a field of a struct as public, but also prefixed it with an underscore (`_`). This is something users should avoid because the two ideas are contradictory. Prefixing a field with an `_` is inferred as the field being unused, but making a field public infers that it will be used.
- \[x] Followed [lint naming conventions][lint_naming]
- I believe I followed the naming conventions, more than happy to update the naming if I did not :)
- \[x] Added passing UI tests (including committed `.stderr` file)
- \[x] `cargo test` passes locally
- \[x] Executed `cargo dev update_lints`
- \[x] Added lint documentation
- \[x] Run `cargo dev fmt`
---
changelog: new lint: [`pub_underscore_fields`]
[#10283](https://github.com/rust-lang/rust-clippy/pull/10283)
<!-- changelog_checked -->
- add a new late pass lint, with config options
- add ui tests for both variations of config option
- update CHANGELOG.md
github feedback
bump version to 1.77 and run cargo collect-metadata
Change `,` to `;` in `conf.rs`
* Fixes a typo in the name of the lint (`enforce-import-renames`
instead of `enforced-import-renames`).
* Copyedit “Why” paragraph.
* Make the example configuration use a multi-line list, since it is not
particularly expected that a real project will have *exactly one*
rename to enforce (and the old formatting had unbalanced whitespace).
new lint: `eager_transmute`
A small but still hopefully useful lint that looks for patterns such as `(x < 5).then_some(transmute(x))`.
This is almost certainly wrong because it evaluates the transmute eagerly and can lead to surprises such as the check being completely removed and always evaluating to `Some` no matter what `x` is (it is UB after all when the integer is not a valid bitpattern for the transmuted-to type). [Example](https://godbolt.org/z/xoY34fPzh).
The user most likely meant to use `then` instead.
I can't remember where I saw this but this is inspired by a real bug that happened in practice.
This could probably be a correctness lint?
changelog: new lint: [`eager_int_transmute`]
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.
New lints `iter_filter_is_some` and `iter_filter_is_ok`
Adds a pair of lints that check for cases of an iterator over `Result` and `Option` followed by `filter` without being followed by `map` as that is covered already by a different, specialized lint.
Fixes#11843
PS, I also made some minor documentations fixes in a case where a double tick (`) was included.
---
changelog: New Lint: [`iter_filter_is_some`]
[#12004](https://github.com/rust-lang/rust/pull/12004)
changelog: New Lint: [`iter_filter_is_ok`]
[#12004](https://github.com/rust-lang/rust/pull/12004)
Make closures carry their own ClosureKind
Right now, we use the "`movability`" field of `hir::Closure` to distinguish a closure and a coroutine. This is paired together with the `CoroutineKind`, which is located not in the `hir::Closure`, but the `hir::Body`. This is strange and redundant.
This PR introduces `ClosureKind` with two variants -- `Closure` and `Coroutine`, which is put into `hir::Closure`. The `CoroutineKind` is thus removed from `hir::Body`, and `Option<Movability>` no longer needs to be a stand-in for "is this a closure or a coroutine".
r? eholk
Do not consider `async { (impl IntoFuture).await }` as redundant
changelog: [`redundant_async_block`]: do not trigger on `IntoFuture` instances
Fix#11959
Move `uninhabited_references` to `nursery`
I think this lint has too many false positives and should be put in pedantic. See #11984 and #11985 for context.
The lint is already in beta and is causing trouble for us, so I would also like this PR to be backported to beta as well.
changelog: Moved [`uninhabited_references`] to `nursery` (Now allow-by-default)
[#11997](https://github.com/rust-lang/rust-clippy/pull/11997)
(Check if this has been backported)
Fixes#11984.
[`question_mark`]: also trigger on `return` statements
This fixes the false negative mentioned in #11993: the lint only used to check for `return` expressions, and not a statement containing a `return` expression (doesn't close the issue tho since there's still a useful suggestion that we could make, which is to suggest `.ok_or()?`/`.ok_or_else()?` for `else { return Err(..) }`)
changelog: [`question_mark`]: also trigger on `return` statements
Clairify `ast::PatKind::Struct` presese of `..` by using an enum instead of a bool
The bool is mainly used for when a `..` is present, but it is also set on recovery to avoid errors. The doc comment not describes both of these cases.
See cee794ee98/compiler/rustc_parse/src/parser/pat.rs (L890-L897) for the only place this is constructed.
r? ``@compiler-errors``
fix typo in infinite loop lint
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: This fixes a small typo introduced in https://github.com/rust-lang/rust-clippy/pull/11829
Extend `UNNECESSARY_TO_OWNED` to handle `split`
Fixes https://github.com/rust-lang/rust-clippy/issues/9965.
When you have `to_string().split('a')` or equivalent, it'll suggest to remove the `to_owned`/`to_string` part.
r? `@flip1995`
changelog: Extend `UNNECESSARY_TO_OWNED` to handle `split`
Check whether out of bound when access a known length array with a constant index
fixes [Issue#11762](https://github.com/rust-lang/rust-clippy/issues/11762)
Issue#11762 points that `Array references with known length are not flagged when indexed out of bounds`.
To fix this problem, it is needed to add check for `Expr::Index`. We expand this issue include reference and direct accessing a array.
When we access a array with a constant index `off`, and already know the length `size`, if `off >= size`, these code will throw an error, instead rustc's lint checking them or runtime panic happening.
changelog: [`out_of_bound_indexing`]: Add check for illegal accessing known length array with a constant index
Add support for `for await` loops
This adds support for `for await` loops. This includes parsing, desugaring in AST->HIR lowering, and adding some support functions to the library.
Given a loop like:
```rust
for await i in iter {
...
}
```
this is desugared to something like:
```rust
let mut iter = iter.into_async_iter();
while let Some(i) = loop {
match core::pin::Pin::new(&mut iter).poll_next(cx) {
Poll::Ready(i) => break i,
Poll::Pending => yield,
}
} {
...
}
```
This PR also adds a basic `IntoAsyncIterator` trait. This is partly for symmetry with the way `Iterator` and `IntoIterator` work. The other reason is that for async iterators it's helpful to have a place apart from the data structure being iterated over to store state. `IntoAsyncIterator` gives us a good place to do this.
I've gated this feature behind `async_for_loop` and opened #118898 as the feature tracking issue.
r? `@compiler-errors`
Adds a pair of lints that check for cases of an iterator over `Result`
and `Option` followed by `filter` without being followed by `map` as
that is covered already by a different, specialized lint.
changelog: New Lint: [`iter_filter_is_some`]
changelog: New Lint: [`iter_filter_is_ok`]
New Lint: `result_filter_map` / Mirror of `option_filter_map`
Added the `Result` mirror of `option_filter_map`.
changelog: New Lint: [`result_filter_map`]
I had to move around some code because the function def was too long 🙃.
I have also added some pattern checks on `option_filter_map`
Add new `unconditional_recursion` lint
Currently, rustc `unconditional_recursion` doesn't detect cases like:
```rust
enum Foo {
A,
B,
}
impl PartialEq for Foo {
fn eq(&self, other: &Self) -> bool {
self == other
}
}
```
This is because the lint is currently implemented only for one level, and in the above code, `self == other` will then call `impl PartialEq for &T`, escaping from the detection. The fix for it seems to be a bit tricky (I started investigating potential solution to add one extra level of recursion [here](https://github.com/rust-lang/rust/compare/master...GuillaumeGomez:rust:trait-impl-recursion?expand=1) but completely broken at the moment).
I expect that this situation will remain for a while. In the meantime, I think it's acceptable to check it directly into clippy for the time being as a lot of easy cases like this one can be easily checked (next I plan to extend it to cover other traits like `ToString`).
changelog: Add new `unconditional_recursion` lint
Added the `Result` mirror of `option_filter_map` to catch
```
.into_iter().filter(Result::is_ok).map(Result::unwrap)
```
changelog: New Lint: [`result_filter_map`]
Co-authored-by: Alex Macleod <alex@macleod.io>
Uplift `TypeAndMut` and `ClosureKind` to `rustc_type_ir`
Uplifts `TypeAndMut` and `ClosureKind`
I know I said I was just going to get rid of `TypeAndMut` (https://github.com/rust-lang/types-team/issues/124) but I think this is much simpler, lol
r? `@jackh726` or `@lcnr`
Fix binder handling in `unnecessary_to_owned`
fixes#11952
The use of `rebind` instead of `EarlyBinder::bind` isn't technically needed, but it is the semantically correct operation.
changelog: None
[`doc_markdown`] Recognize words followed by empty parentheses `()` for quoting
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: [`doc_markdown`] Recognize words followed by empty parentheses for quoting, e.g. `func()`.
---
Developers often write function/method names with trailing `()`, but `doc_markdown` lint did not consider that.
Old clippy suggestion was not very good:
```patch
-/// There is no try (do() or do_not()).
+/// There is no try (do() or `do_not`()).
```
New behavior recognizes function names such as `do()` even they contain no `_`/`::`; and backticks are suggested outside of the `()`:
```patch
-/// There is no try (do() or do_not()).
+/// There is no try (`do()` or `do_not()`).
```
Useless vec false positive
changelog: [`useless_vec`]: fix false positive in macros.
fixes#11861
We delay the emission of `useless_vec` lints to the check_crate_post stage, which allows us to effectively undo lints if we find that a `vec![]` expression is being used multiple times after macro expansion.
new lint to detect infinite loop
closes: #11438
changelog: add new lint to detect infinite loop
~*I'll change the lint name*~. Should I name it `infinite_loop` or `infinite_loops` is fine? Ahhhh, English is hard...
Renamings:
- find -> opt_hir_node
- get -> hir_node
- find_by_def_id -> opt_hir_node_by_def_id
- get_by_def_id -> hir_node_by_def_id
Fix rebase changes using removed methods
Use `tcx.hir_node_by_def_id()` whenever possible in compiler
Fix clippy errors
Fix compiler
Apply suggestions from code review
Co-authored-by: Vadim Petrochenkov <vadim.petrochenkov@gmail.com>
Add FIXME for `tcx.hir()` returned type about its removal
Simplify with with `tcx.hir_node_by_def_id`
Add lint against ambiguous wide pointer comparisons
This PR is the resolution of https://github.com/rust-lang/rust/issues/106447 decided in https://github.com/rust-lang/rust/issues/117717 by T-lang.
## `ambiguous_wide_pointer_comparisons`
*warn-by-default*
The `ambiguous_wide_pointer_comparisons` lint checks comparison of `*const/*mut ?Sized` as the operands.
### Example
```rust
let ab = (A, B);
let a = &ab.0 as *const dyn T;
let b = &ab.1 as *const dyn T;
let _ = a == b;
```
### Explanation
The comparison includes metadata which may not be expected.
-------
This PR also drops `clippy::vtable_address_comparisons` which is superseded by this one.
~~One thing: is the current naming right? `invalid` seems a bit too much.~~
Fixes https://github.com/rust-lang/rust/issues/117717
uninhabited_reference: new lint
Close#11851
The lint is implemented on function parameters and return types, as this is the place where the risk of exchanging references to uninhabited types is the highest. Other constructs, such as in a local variable,
would require the use of `unsafe` and will clearly be done on purpose.
changelog: [`uninhabited_reference`]: new lint
This is an extension of the previous commit. It means the output of
something like this:
```
stringify!(let a: Vec<u32> = vec![];)
```
goes from this:
```
let a: Vec<u32> = vec![] ;
```
With this PR, it now produces this string:
```
let a: Vec<u32> = vec![];
```
detects redundant imports that can be eliminated.
for #117772 :
In order to facilitate review and modification, split the checking code and
removing redundant imports code into two PR.
Introduce support for `async gen` blocks
I'm delighted to demonstrate that `async gen` block are not very difficult to support. They're simply coroutines that yield `Poll<Option<T>>` and return `()`.
**This PR is WIP and in draft mode for now** -- I'm mostly putting it up to show folks that it's possible. This PR needs a lang-team experiment associated with it or possible an RFC, since I don't think it falls under the jurisdiction of the `gen` RFC that was recently authored by oli (https://github.com/rust-lang/rfcs/pull/3513, https://github.com/rust-lang/rust/issues/117078).
### Technical note on the pre-generator-transform yield type:
The reason that the underlying coroutines yield `Poll<Option<T>>` and not `Poll<T>` (which would make more sense, IMO, for the pre-transformed coroutine), is because the `TransformVisitor` that is used to turn coroutines into built-in state machine functions would have to destructure and reconstruct the latter into the former, which requires at least inserting a new basic block (for a `switchInt` terminator, to match on the `Poll` discriminant).
This does mean that the desugaring (at the `rustc_ast_lowering` level) of `async gen` blocks is a bit more involved. However, since we already need to intercept both `.await` and `yield` operators, I don't consider it much of a technical burden.
r? `@ghost`
never_patterns: Parse match arms with no body
Never patterns are meant to signal unreachable cases, and thus don't take bodies:
```rust
let ptr: *const Option<!> = ...;
match *ptr {
None => { foo(); }
Some(!),
}
```
This PR makes rustc accept the above, and enforces that an arm has a body xor is a never pattern. This affects parsing of match arms even with the feature off, so this is delicate. (Plus this is my first non-trivial change to the parser).
~~The last commit is optional; it introduces a bit of churn to allow the new suggestions to be machine-applicable. There may be a better solution? I'm not sure.~~ EDIT: I removed that commit
r? `@compiler-errors`
Add a function to check whether binary oprands are nontrivial
fixes [#issue11885](https://github.com/rust-lang/rust-clippy/issues/11885)
It's hard to check whether operator is overrided through context of lint.
So, assume non-trivial structure like tuple, array or sturt, using a overrided binary operator in this lint, which might cause a side effict.
This is not detected before.
Althrough this might weaken the ability of this lint, it may more useful than before. Maybe this lint will cause an error, but now, it not. And assuming side effect of non-trivial structure with operator is not a bad thing, right?
changelog: Fix: [`no_effect`] check if binary operands are nontrivial
fix(ptr_as_ptr): handle `std::ptr::null{_mut}`
close rust-lang#11066
close rust-lang#11665
close rust-lang#11911
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: [`ptr_as_ptr`]: handle `std::ptr::null` and `std::ptr::null_mut`
needless_borrows_for_generic_args: Handle when field operand impl Drop
Before this fix, the lint had a false positive, namely when a reference was taken to a field when the field operand implements a custom Drop. The compiler will refuse to partially move a type that implements Drop, because that would put the type in a weird state.
## False Positive Example (Fixed)
```rs
struct CustomDrop(String);
impl Drop for CustomDrop {
fn drop(&mut self) {}
}
fn check_str<P: AsRef<str>>(_to: P) {}
fn test() {
let owner = CustomDrop(String::default());
check_str(&owner.0); // Don't lint. `owner` can't be partially moved because it impl Drop
}
```
changelog: [`needless_borrows_for_generic_args`]: Handle when field operand impl Drop
docs(explicit_write): add missing backtick to complete code snippet
close#11918
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: [`explicit_write`]: add missing backtick to document to complete code snippet
Update regex-syntax to support new word boundry assertions
From the regex v1.10.0 release notes [1]:
This is a new minor release of regex that adds support for start
and end word boundary assertions. [...]
The new word boundary assertions are:
• \< or \b{start}: a Unicode start-of-word boundary (\W|\A
on the left, \w on the right).
• \> or \b{end}: a Unicode end-of-word boundary (\w on the
left, \W|\z on the right)).
• \b{start-half}: half of a Unicode start-of-word boundary
(\W|\A on the left).
• \b{end-half}: half of a Unicode end-of-word boundary
(\W|\z on the right).
[1]: https://github.com/rust-lang/regex/blob/master/CHANGELOG.md#1100-2023-10-09
changelog: [`regex`]: add support for start and end word boundary assertions ("\<", "\b{start}", etc.) introduced in regex v0.10
Simpfy code of `is_operator_overrided`, directly use `is_method_call` to
check
if operator is overrided, at least one oprand of binary-expr must be ADT-type
So no need to check type of lhs and rhs
Check whether operator is overrided with a `struct` operand.
The struct here refers to `struct`, `enum`, `union`.
Add and fix test for `no_effect` lint.
From the regex v1.10.0 release notes [1]:
This is a new minor release of regex that adds support for start
and end word boundary assertions. [...]
The new word boundary assertions are:
• \< or \b{start}: a Unicode start-of-word boundary (\W|\A
on the left, \w on the right).
• \> or \b{end}: a Unicode end-of-word boundary (\w on the
left, \W|\z on the right)).
• \b{start-half}: half of a Unicode start-of-word boundary
(\W|\A on the left).
• \b{end-half}: half of a Unicode end-of-word boundary
(\W|\z on the right).
[1]: https://github.com/rust-lang/regex/blob/master/CHANGELOG.md#1100-2023-10-09
expending lint [`blocks_in_if_conditions`] to check match expr as well
closes: #11814
changelog: rename lint `blocks_in_if_conditions` to [`blocks_in_conditions`] and expand it to check blocks in match scrutinees
[`missing_asserts_for_indexing`]: accept length equality checks
Fixes#11835
The lint now allows indexing with indices 0 and 1 when an `assert!(x.len() == 2);` is found.
(Also fixed a typo in the doc example)
changelog: [`missing_asserts_for_indexing`]: accept len equality checks as a valid assertion
Before this fix, the lint had a false positive, namely when a reference
was taken to a field when the field operand implements a custom Drop.
The compiler will refuse to partially move a type that implements Drop,
because that would put the operand in a weird state. See added
regression test.
`option_if_let_else`: do not trigger on expressions returning `()`
Fix#11893
Trigerring on expressions returning `()` uses the arguments of the `map_or_else()` rewrite only for their side effects. This does lead to code which is harder to read than the original.
changelog: [`option_if_let_else`]: do not trigger on unit expressions
add lint against unit tests in doctests
During RustLab, Alice Ryhl brought to my attention that the Andoid team stumbled over the fact that if one attempts to write a unit test within a doctest, it will be summarily ignored. So this lint should help people wondering why their tests won't run.
---
changelog: New lint: [`test_attr_in_doctest`]
[#11872](https://github.com/rust-lang/rust-clippy/pull/11872)
Fix#11893
Trigerring on expressions returning `()` uses the arguments of the
`map_or_else()` rewrite only for their side effects. This does lead
to code which is harder to read than the original.
[`redundant_guards`]: catch `is_empty`, `starts_with` and `ends_with` on slices and `str`s
Fixes#11807
Few things worth mentioning:
- Taking `snippet`s is now done at callsite, instead of passing a span and doing it in `emit_redundant_guards`. This is because we now need custom suggestion strings in certain places, like `""` for `str::is_empty`.
- This now uses `snippet` instead of `snippet_with_applicability`. I don't think this really makes any difference for `MaybeIncorrect`, though?
- This could also lint byte strings, as they're of type `&[u8; N]`, but that can be ugly so I decided to leave it out for now
changelog: [`redundant_guards`]: catch `str::is_empty`, `slice::is_empty`, `slice::starts_with` and `slice::ends_with`
Add `never_patterns` feature gate
This PR adds the feature gate and most basic parsing for the experimental `never_patterns` feature. See the tracking issue (https://github.com/rust-lang/rust/issues/118155) for details on the experiment.
`@scottmcm` has agreed to be my lang-team liaison for this experiment.
[`redundant_closure_call`]: avoid duplicated `async` keyword when triggering on closure that returns `async` block
close#11357
----
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: [`redundant_closure_call`]: avoid duplicated `async` keyword when triggering on closure that returns `async` block
- Rename them both `as_str`, which is the typical name for a function
that returns a `&str`. (`to_string` is appropriate for functions
returning `String` or maybe `Cow<'a, str>`.)
- Change `UnOp::as_str` from an associated function (weird!) to a
method.
- Avoid needless `self` dereferences.
Don't suggest `a.mul_add(b, c)` if parameters are not float
clippy::suboptimal_flops used to not check if the second parameter to f32/f64.mul_add() was float. Since the method is only defined to take `Self` as parameters, the suggestion was wrong.
Fixes#11831
changelog: [`suboptimal_float`]: Don't suggest `a.mul_add(b, c)` if parameters are not f32/f64
[`ptr_arg`]: recognize methods that also exist on slices
Fixes#11816
Not a new lint, just a very small improvement to the existing `ptr_arg` lint which would have caught the linked issue.
The problem was that the lint checks if a `Vec`-specific method was called, that is, if the receiver is `Vec<_>`.
This is the case for `len` and `is_empty`, however these methods also exist on slices so we can still lint there.
This logic exists in a different lint, so we can just reuse that here.
Interestingly, there was even a comment up top that explained what it should have been doing, but the logic for it just wasn't there?
changelog: [`ptr_arg`]: recognize methods that also exist on slices
<sub>Also, this is my 100th PR to clippy 🎉 </sub>
`manual_try_fold`: check that `fold` is really `Iterator::fold`
Fix#11876
changelog: [`manual_try_fold`]: suggest using `try_fold` only for `Iterator::fold` uses
Move `implied_bounds_in_impls` back to complexity
This lint was originally in the complexity category when I PR'd it. It was then moved to nursery by me due to a number of issues (a false positive, an invalid suggestion and an ICE), but that was probably an overreaction and all of the issues were fixed quickly after.
This is a useful lint imo and there hasn't been any issues with it in a few months, so I say we should give it another try and move it back to complexity.
I did a lintcheck run on the top 400 crates and all of them are legitimate, with 18 warnings. Most of them are from anstyle having a `impl Display + Copy + Clone` return type, or the bitvec crate with a return type like `impl Iterator + DoubleEndedIterator`.
changelog: Move [`implied_bounds_in_impls`] to `complexity` (Now warn-by-default)
[#11867](https://github.com/rust-lang/rust-clippy/pull/11867)
Create new lint `option_map_or_err_ok`
Fixes#10045.
For the following code:
```rust
let opt = Some(1);
opt.map_or(Err("error"), Ok);
```
It suggests to instead write:
```rust
let opt = Some(1);
opt.ok_or("error");
```
r? `@flip1995`
changelog: Create new lint `option_map_or_err_ok`
suggest alternatives to iterate an array of ranges
works towards #7125
changelog: [`single_element_loop`]: suggest better syntax when iterating over an array of a single range
`@thinkerdreamer` and myself worked on this issue during a workshop by `@llogiq` at the RustLab 2023 conference. It is our first contribution to clippy.
When iterating over an array of only one element, _which is a range_, our change suggests to replace the array with the contained range itself. Additionally, a hint is printed stating that the user probably intended to iterate over the range and not the array. If the single element in the array is not a range, the previous suggestion in the form of `let {pat_snip} = {prefix}{arg_snip};{block_str}`is used.
This change lints the array with the single range directly, so any prefixes or suffixes are covered as well.
Nit re `matches!` formatting
I think formatting `matches!` with `if` guards is [still unsupported](https://github.com/rust-lang/rustfmt/issues/5547), which is probably why this was missed.
changelog: none
[`deprecated_semver`]: Allow `#[deprecated(since = "TBD")]`
"TBD" is allowed by rustdoc, saying that it will be deprecated in a future version. rustc will also not actually warn on it.
I found this while checking the rust-lang/rust with clippy.
changelog: [`deprecated_semver`]: allow using `since = "TBD"`
[`missing_asserts_for_indexing`]: work with bodies instead of blocks separately
Fixes#11856
Before this change, this lint would check blocks independently of each other, which means that it misses `assert!()`s from parent blocks.
```rs
// check_block
assert!(x.len() > 1);
{
// check_block
// no assert here
let _ = x[0] + x[1];
}
```
This PR changes it to work with bodies rather than individual blocks. That means that a function will be checked in one go and we can remember if an `assert!` occurred anywhere.
Eventually it would be nice to have a more control flow-aware analysis, possibly by rewriting it as a MIR lint, but that's more complicated and I wanted this fixed first.
changelog: [`missing_asserts_for_indexing`]: accept `assert!`s from parent blocks
Fix iter_kv_map false positive into_keys and into_values suggestion
fixes: #11752
changelog: [`iter_kv_map`]: fix false positive: Don't suggest `into_keys()` and `into_values()` if the MSRV is to low
[`needless_return_with_question_mark`]: don't lint if never type is used for coercion
Fixes#11616
When we have something like
```rs
let _x: String = {
return Err(())?;
};
```
we shouldn't suggest removing the `return` because the `!`-ness of `return` is used to coerce the enclosing block to some other type. That will lead to a typeck error without a diverging expression like `return`.
changelog: [`needless_return_with_question_mark`]: don't lint if `return`s never typed-ness is used for coercion
Split `doc.rs` up into a subdirectory
So, first, sorry for the bad diff. 😅
In #11798, `@flip1995` suggested splitting `doc.rs` up, much like how we have the `methods/`, `matches/`, `types/` subdirectories.
I agree with this, the file is getting bigger as we add more and more doc lints that it makes sense to do this refactoring.
This is purely an internal change that moves things around a bit.
(**EDIT:** depending on the outcome of https://github.com/rust-lang/rust-clippy/pull/11801#issuecomment-1816715615 , this may change the lint group name from `doc_markdoc` to `doc`).
I tried to not change any of the actual logic of the lints and as such some things weren't as easy to move to a separate file. So we still have some `span_lint*` calls in the `doc/mod.rs` file, which I think is fine. This is also the case in `methods/mod.rs`.
Also worth mentioning that the lints missing_errors_doc, missing_panics_doc, missing_safety_doc and unnecessary_safety_doc have a lot of the same logic so it didn't make much sense for each of these to be in their own file. Instead I just put them all in `missing_headers.rs`
I also added a bit of documentation to the involved `check_{attrs,doc}` methods.
changelog: none
Improve maybe misused cfg
Follow-up of the improvements that were suggested to me in https://github.com/rust-lang/rust-clippy/pull/11821:
* I unified the output to use the same terms.
* I updated the code to prevent creating a new symbol.
r? `@blyxyas`
changelog: [`maybe_misued_cfg`]: Output and code improvements
Verify Borrow<T> semantics for types that implement Hash, Borrow<str> and Borrow<[u8]>.
Fixes#11710
The essence of the issue is that types that implement Borrow<T> provide a facet or a representation of the underlying type. Under these semantics `hash(a) == hash(a.borrow())`.
This is a problem when a type implements `Borrow<str>`, `Borrow<[u8]>` and Hash, it is expected that the hash of all three types is identical. The problem is that the hash of [u8] is not the same as that of a String, even when the byte reference ([u8]) is derived from `.as_bytes()`
- [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`
---
- [x] Explanation of the issue in the code
- [x] Tests reproducing the issue
- [x] Lint rule and emission
---
changelog: New lint: [`impl_hash_borrow_with_str_and_bytes`]
[#11781](https://github.com/rust-lang/rust-clippy/pull/11781)
Implements a lint to prevent implementation of Hash, Borrow<str> and
Borrow<[u8]> as it breaks Borrow<T> "semantics". According to the book,
types that implement Borrow<A> and Borrow<B> must ensure equality of
borrow results under Eq,Ord and Hash.
> In particular Eq, Ord and Hash must be equivalent for borrowed and
owned values: x.borrow() == y.borrow() should give the same result as x == y.
In the same way, hash(x) == hash(x as Borrow<[u8]>) != hash(x as Borrow<str>).
changelog: newlint [`impl_hash_with_borrow_str_and_bytes`]
Fix typos in recent lint documentation.
Fixes typos and markup errors, and also makes the examples more realistic by hiding the `;`s so as not to visibly be discarding the computed value. Affected lints:
* `redundant_as_str`
* `unnecessary_map_on_constructor`
changelog: none
clippy::suboptimal_flops used to not check if the second parameter to f32/f64.mul_add() was float. Since the method is
only defined to take `Self` as paremters, the suggestion was wrong.
Fixes#11831
teach `eager_or_lazy` about panicky arithmetic operations
Fixes#9422Fixes#9814Fixes#11793
It's a bit sad that we have to do this because arithmetic operations seemed to me like the prime example where a closure would not be necessary, but this has "side effects" (changes behavior when going from lazy to eager) as some of these panic on overflow/underflow if compiled with `-Coverflow-checks` (which is the default in debug mode).
Given the number of backlinks in the mentioned issues, this seems to be a FP that is worth fixing, probably.
changelog: [`unnecessary_lazy_evaluations`]: don't lint if closure has panicky arithmetic operations
Extend `maybe_misused_cfg` lint over `cfg(test)`
Fixes#11240.
One thought I had is that we could use the levenshtein distance (of 1) to ensure this is indeed `test` that was targeted. But maybe it's overkill, not sure.
changelog: [`maybe_misused_cfg`]: Extend lint over `cfg(test)`
r? `@blyxyas`
Changelog for Rust 1.74 🎃
Roses are red,
Halloween is over,
Have you considered,
Buying a Mars rover?
---
### The cats of this release:
<img height=500 src="https://github.com/rust-lang/rust-clippy/assets/17087237/095bd32e-b5e3-41db-8f0f-bdef7ca1a6d0" alt="The cats of this Clippy release" />
<sub>The cat for the next release can be nominated in the comments</sub>
---
changelog: none
Change `if_same_then_else` to be a `style` lint
CC #3770
From https://github.com/rust-lang/rust-clippy/issues/3770#issuecomment-687565594 (`@flip1995):`
> Oh I thought I replied to this: I definitely see now that having this
> as a correctness lint might be the wrong categorization. What we might
> want to do is to just allow this lint, if there are comments in the
> arm bodies. But a good first step would be to downgrade this lint to
> style or complexity. I would vote for style since merging two arms is
> not always less complex.
changelog: [`if_same_then_else`]: Change to be a `style` lint
[`impl_trait_in_params`]: avoid ICE when function with `impl Trait` type has no parameters
Fixes#11803
If I'm reading the old code correctly, it was taking the span of the first parameter (without checking that it exists, which caused the ICE) and uses that to figure out where the generic parameter to insert should go (cc `@blyxyas` you wrote the lint, is that correct?).
This seemed equivalent to just `generics.span`, which doesn't require calculating the spans like that and simplifies it a fair bit
changelog: don't ICE when function has no parameters but generics have an `impl Trait` type
CC #3770
From https://github.com/rust-lang/rust-clippy/issues/3770#issuecomment-687565594 (@flip1995):
> Oh I thought I replied to this: I definitely see now that having this
> as a correctness lint might be the wrong categorization. What we might
> want to do is to just allow this lint, if there are comments in the
> arm bodies. But a good first step would be to downgrade this lint to
> style or complexity. I would vote for style since merging two arms is
> not always less complex.
Implement new lint `iter_over_hash_type`
Implements and fixes https://github.com/rust-lang/rust-clippy/issues/11788
This PR adds a new *restriction* lint `iter_over_hash_type` which prevents `Hash`-types (that is, `HashSet` and `HashMap`) from being used as the iterator in `for` loops.
The justification for this is because in `Hash`-based types, the ordering of items is not guaranteed and may vary between executions of the same program on the same hardware. In addition, it reduces readability due to the unclear iteration order.
The implementation of this lint also ensures the following:
- Calls to `HashMap::keys`, `HashMap::values`, and `HashSet::iter` are also denied when used in `for` loops,
- When this expression is used in procedural macros, it is not linted/denied.
changelog: add new `iter_over_hash_type` lint to prevent unordered iterations through hashed data structures
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
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
[`mod_module_files`] Don't emit lint for mod.rs in tests
fixes: #11775
current state: indiscriminately emits the lint for mod files in tests.
The following
```
tests/
common/
mod.rs
test.rs
```
is a common pattern for code shared across the tests and is suggested in the rust book. The change adds an additional check to verify that the mod file is not in tests.
changelog: Fix [`mod_module_files`]: false positive for mod files in tests folder
fixes: #11775
current state: indiscriminately emits the lint for mod files in tests.
The following
tests/
common/
mod.rs
test.rs
is a common pattern for code shared across the tests and is suggested in
the rust book. The change adds an additional check to verify that the
mod file is not in tests.
changelog: Fix [`mod_module_files`]: false positive for mod files in
tests folder
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
Most notably, this commit changes the `pub use crate::*;` in that file
to `use crate::*;`. This requires a lot of `use` items in other crates
to be adjusted, because everything defined within `rustc_span::*` was
also available via `rustc_span::source_map::*`, which is bizarre.
The commit also removes `SourceMap::span_to_relative_line_string`, which
is unused.
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
Use multiple pushes in `vec_init_then_push` example
Makes the perf argument clearer, since a single push doesn't have unnecessary allocations compared to `vec![x]`
changelog: none
Remove internal feature from clippy_utils
It's only used to gate a few `const`s, removing the feature gate means it doesn't have to be recompiled when moving between a normal and `-F internal` build/test/etc
changelog: none
ignore lower-camel-case words in `doc_markdown`
This fixes#11568 by ignoring camelCase words starting with a lower case letter.
r? `@blyxyas`
---
changelog: none
move `read_zero_byte_vec` to nursery
I think the concerns in #9274 are valid, and we should move this to nursery while we're reworking this.
changelog: [`read_zero_byte_vec`] moved to nursery
Expand docs on clippy::todo
https://doc.rust-lang.org/nightly/core/macro.todo.html describes that `todo!()` is intended for explicitly unfinished code. Explain this, and mention `unimplemented!()` as an alternative.
Whilst we're here, improve the punctuation on the other lints.
changelog: [`todo`]: expand docs
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)
https://doc.rust-lang.org/nightly/core/macro.todo.html
describes that `todo!()` is intended for explicitly unfinished
code. Explain this, and mention `unimplemented!()` as an alternative.
Whilst we're here, improve the punctuation on the other lints.
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
Remove the `internal_warn` lint category
`LINT_AUTHOR` and `DUMP_HIR` can be removed since their corresponding lint passes are only used for side effects
The metadata collector lint can be made an `internal` lint since `clippy::internal` is set to deny when running the collector
Also renames some internal lints/passes
changelog: none
Set doc-tests to `no_run`
This excludes `should_panic` tests, those are still run to ensure they panic. Most of our other doc snippets don't gain much from being run though so this frees up a nice bit of CI time
It also fixes the occasional issue such as `foo.txt`s being created f942470ca7/clippy_lints/src/permissions_set_readonly_false.rs (L19)
changelog: none
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`]
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
Implement rustc part of RFC 3127 trim-paths
This PR implements (or at least tries to) [RFC 3127 trim-paths](https://github.com/rust-lang/rust/issues/111540), the rustc part. That is `-Zremap-path-scope` with all of it's components/scopes.
`@rustbot` label: +F-trim-paths
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
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
There are several that are unused and can be removed.
And there are some calls to `to_string`, which can be expressed more
nicely as a `foo_to_string` call, and then `to_string` need not be
`pub`. (This requires adding `pat_to_string`).
`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
The word "active" is currently used in two different and confusing ways:
- `ACTIVE_FEATURES` actually means "available unstable features"
- `Features::active_features` actually means "features declared in the
crate's code", which can include feature within `ACTIVE_FEATURES` but
also others.
(This is also distinct from "enabled" features which includes declared
features but also some edition-specific features automatically enabled
depending on the edition in use.)
This commit changes the `Features::active_features` to
`Features::declared_features` which actually matches its meaning.
Likewise, `Features::active` becomes `Features::declared`.
