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.
```
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
Hide foreign `#[doc(hidden)]` paths in import suggestions
Stops the compiler from suggesting to import foreign `#[doc(hidden)]` paths.
```@rustbot``` label A-suggestion-diagnostics
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
Tweak suggestions for bare trait used as a type
```
error[E0782]: trait objects must include the `dyn` keyword
--> $DIR/not-on-bare-trait-2021.rs:11:11
|
LL | fn bar(x: Foo) -> Foo {
| ^^^
|
help: use a generic type parameter, constrained by the trait `Foo`
|
LL | fn bar<T: Foo>(x: T) -> Foo {
| ++++++++ ~
help: you can also use `impl Foo`, but users won't be able to specify the type paramer when calling the `fn`, having to rely exclusively on type inference
|
LL | fn bar(x: impl Foo) -> Foo {
| ++++
help: alternatively, use a trait object to accept any type that implements `Foo`, accessing its methods at runtime using dynamic dispatch
|
LL | fn bar(x: &dyn Foo) -> Foo {
| ++++
error[E0782]: trait objects must include the `dyn` keyword
--> $DIR/not-on-bare-trait-2021.rs:11:19
|
LL | fn bar(x: Foo) -> Foo {
| ^^^
|
help: use `impl Foo` to return an opaque type, as long as you return a single underlying type
|
LL | fn bar(x: Foo) -> impl Foo {
| ++++
help: alternatively, you can return an owned trait object
|
LL | fn bar(x: Foo) -> Box<dyn Foo> {
| +++++++ +
```
Fix#119525:
```
error[E0038]: the trait `Ord` cannot be made into an object
--> $DIR/bare-trait-dont-suggest-dyn.rs:3:33
|
LL | fn ord_prefer_dot(s: String) -> Ord {
| ^^^ `Ord` cannot be made into an object
|
note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
--> $SRC_DIR/core/src/cmp.rs:LL:COL
|
= note: the trait cannot be made into an object because it uses `Self` as a type parameter
::: $SRC_DIR/core/src/cmp.rs:LL:COL
|
= note: the trait cannot be made into an object because it uses `Self` as a type parameter
help: consider using an opaque type instead
|
LL | fn ord_prefer_dot(s: String) -> impl Ord {
| ++++
```
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
This commit:
- 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.
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`]
```
