`useless_asref`: do not lint `.as_ref().map(Arc::clone)`
This applies to `Arc`, `Rc`, and their weak variants. Using `.clone()` would be less idiomatic.
This follows the discussion in <https://github.com/rust-lang/rust-clippy/issues/12528#issuecomment-2014444305>.
changelog: [`useless_asref`]: do not lint `.as_ref().map(Arc::clone)` and similar
don't lint [`mixed_attributes_style`] when mixing docs and other attrs
fixes: #12435fixes: #12436fixes: #12530
---
changelog: don't lint [`mixed_attributes_style`] when mixing different kind of attrs; and move it to late pass;
don't lint [`mixed_attributes_style`] when mixing docs and other attrs
add test files for issue #12436
move [`mixed_attributes_style`] to `LateLintPass` to enable global `allow`
stop [`mixed_attributes_style`] from linting on different attributes
add `@compile-flags` to [`mixed_attributes_style`]'s test;
turns out not linting in test mod is not a FN.
Apply suggestions from code review
Co-authored-by: Timo <30553356+y21@users.noreply.github.com>
move [`mixed_attributes_style`] to late pass and stop it from linting on different kind of attributes
Note that the caller chooses a type for type param
```
error[E0308]: mismatched types
--> $DIR/return-impl-trait.rs:23:5
|
LL | fn other_bounds<T>() -> T
| - -
| | |
| | expected `T` because of return type
| | help: consider using an impl return type: `impl Trait`
| expected this type parameter
...
LL | ()
| ^^ expected type parameter `T`, found `()`
|
= note: expected type parameter `T`
found unit type `()`
= note: the caller chooses the type of T which can be different from ()
```
Tried to see if "expected this type parameter" can be replaced, but that goes all the way to `rustc_infer` so seems not worth the effort and can affect other diagnostics.
Revives #112088 and #104755.
Do not warn on .map(_::clone) for Arc, Rc, and their weak variants
Those constructions are idiomatic, and using `Arc::clone(x)` and `Rc::clone(x)` is often the recommended way of cloning a `Arc` or a `Rc`.
Fix#12528
changelog: [`map_clone`]: do not warn on `.map(_::clone)` for `Arc`, `Rc`, and their `Weak` variants
Fix infinite loop in `cast_sign_loss` when peeling unwrap method calls
Fixes#12506
The lint wants to peel method calls but didn't actually reassign the expression, leading to an infinite loop.
----
changelog: Fix infinite loop in [`cast_sign_loss`] when having two chained `.unwrap()` calls
Several (doc) comments were super outdated or didn't provide enough context.
Some doc comments shoved everything in a single paragraph without respecting
the fact that the first paragraph should be a single sentence because rustdoc
treats these as item descriptions / synopses on module pages.
Disable `cast_lossless` when casting to u128 from any (u)int type
Fixes https://github.com/rust-lang/rust-clippy/issues/12492
Disables `cast_lossless` when casting to u128 from any int or uint type. The lint states that when casting to any int type, there can potentially be lossy behaviour if the source type ever exceeds the size of the destination type in the future, which is impossible with a destination of u128.
It's possible this is a bit of a niche edge case which is better addressed by just disabling the lint in code, but I personally couldn't think of any good reason to still lint in this specific case - maybe except if the source is a bool, for readability reasons :).
changelog: FP: `cast_lossless`: disable lint when casting to u128 from any (u)int type
[`map_entry`]: call the visitor on the local's `else` block
Fixes#12489
The lint already has all the logic it needs for figuring out if it can or can't suggest a closure if it sees control flow expressions like `break` or `continue`, but it was ignoring the local's else block, which meant that it didn't see the `return None;` in a `let..else`.
changelog: [`map_entry`]: suggest `if let` instead of a closure when `return` expressions exist in the else block of a `let..else`
new restriction lint: `integer_division_remainder_used`
Fixes https://github.com/rust-lang/rust-clippy/issues/12391
Introduces a restriction lint which disallows the use of `/` and `%` operators on any `Int` or `Uint` types (i.e., any that use the default `Div` or `Rem` trait implementations). Custom implementations of these traits are ignored.
----
changelog: Add new restriction lint [`integer_division_remainder_used`]
[`option_option`]: Fix duplicate diagnostics
Relates to #12379
This `option_option` lint change skips checks against `ty`s inside `field_def`s defined by external macro to prevent duplicate diagnostics to the same span `ty` by multiple `Struct` definitions.
---
changelog: [`option_option`]: Fix duplicate diagnostics
move `readonly_write_lock` to perf
[There haven't been any issues](https://github.com/rust-lang/rust-clippy/issues?q=is%3Aissue+readonly_write_lock) since its creation and it's a pretty useful lint I think, so I'd say it's worth giving it a try?
Did a lintcheck run on 300 crates with no results, but I guess `RwLock` is usually not something that's used much in libraries.
changelog: move [`readonly_write_lock`] to perf (now warn-by-default)
fix [`dbg_macro`] FN when dbg is inside some complex macros
fixes: #12131
It appears that [`root_macro_call_first_node`] only detects `println!` in the following example:
```rust
println!("{:?}", dbg!(s));
```
---
changelog: fix [`dbg_macro`] FN when `dbg` is inside some complex macros
(re-opening b'cuz bors doesn't like my previous one)
fix span calculation for non-ascii in `needless_return`
Fixes#12491
Probably fixes#12328 as well, but that one has no reproducer, so 🤷
The bug was that the lint used `rfind()` for finding the byte index of the start of the previous non-whitespace character:
```
// abc\n return;
^
```
... then subtracting one to get the byte index of the actual whitespace (the `\n` here).
(Subtracting instead of adding because it treats this as the length from the `return` token to the `\n`)
That's correct for ascii, like here, and will get us to the `\n`, however for non ascii, the `c` could be multiple bytes wide, which would put us in the middle of a codepoint if we simply subtract 1 and is what caused the ICE.
There's probably a lot of ways we could fix this.
This PR changes it to iterate backwards using bytes instead of characters, so that when `rposition()` finally finds a non-whitespace byte, we *know* that we've skipped exactly 1 byte. This was *probably*(?) what the code was intending to do
changelog: Fix ICE in [`needless_return`] when previous line end in a non-ascii character
[`unused_enumerate_index`]: trigger on method calls
The lint used to check for patterns looking like:
```rs
for (_, x) in some_iter.enumerate() {
// Index is ignored
}
```
This commit further checks for chained method calls constructs where we
can detect that the index is unused. Currently, this checks only for the
following patterns:
```rs
some_iter.enumerate().map_function(|(_, x)| ..)
let x = some_iter.enumerate();
x.map_function(|(_, x)| ..)
```
where `map_function` is one of `all`, `any`, `filter_map`, `find_map`,
`flat_map`, `for_each` or `map`.
Fixes#12411.
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: [`unused_enumerate_index`]: add detection for method chains such as `iter.enumerate().map(|(_, x)| x)`
Prior to this change, it might be that the lint would remove an explicit
type that was necessary for the type system to keep track of types.
This should be the last change that prevented this lint to be machine
applicable.
Don't emit `doc_markdown` lint for missing backticks if it's inside a quote
Fixes#10262.
changelog: Don't emit `doc_markdown` lint for missing backticks if it's inside a quote
[`use_self`]: Make it aware of lifetimes
Have the lint trigger even if `Self` has generic lifetime parameters.
```rs
impl<'a> Foo<'a> {
type Item = Foo<'a>; // Can be replaced with Self
fn new() -> Self {
Foo { // No lifetime, but they are inferred to be that of Self
// Can be replaced as well
...
}
}
// Don't replace `Foo<'b>`, the lifetime is different!
fn eq<'b>(self, other: Foo<'b>) -> bool {
..
}
```
Fixes#12381
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: [`use_self`]: Have the lint trigger even if `Self` has generic lifetime parameters
The lint used to check for patterns looking like:
```rs
for (_, x) in some_iter.enumerate() {
// Index is ignored
}
```
This commit further checks for chained method calls constructs where we
can detect that the index is unused. Currently, this checks only for the
following patterns:
```rs
some_iter.enumerate().map_function(|(_, x)| ..)
let x = some_iter.enumerate();
x.map_function(|(_, x)| ..)
```
where `map_function` is one of `all`, `any`, `filter_map`, `find_map`,
`flat_map`, `for_each` or `map`.
Fixes#12411.
lint when calling the blanket `Into` impl from a `From` impl
Closes#11150
```
warning: function cannot return without recursing
--> x.rs:9:9
|
9 | / fn from(value: f32) -> Self {
10 | | value.into()
11 | | }
| |_________^
|
note: recursive call site
--> x.rs:10:13
|
10 | value.into()
| ^^^^^^^^^^^^
```
I'm also thinking that we can probably generalize this lint to #11032 at some point (instead of hardcoding a bunch of impls), like how rustc's `unconditional_recursion` works, at least up to one indirect call, but this still seems useful for now :)
I've also noticed that we use `fn_def_id` in a bunch of lints and then try to get the node args of the call in a separate step, so I made a helper function that does both in one. I intend to refactor a bunch of uses of `fn_def_id` to use this later
I can add more test cases, but this is already using much of the same logic that exists for the other impls that this lint looks for (e.g. making sure that there are no conditional returns).
changelog: [`unconditional_recursion`]: emit a warning inside of `From::from` when unconditionally calling the blanket `.into()` impl
Move `iter_nth` to `style`, add machine applicable suggestion
There's no `O(n)` involved with `.iter().nth()` on the linted types since the iterator implementations provide `nth` and/or `advance_by` that operate in `O(1)`
For slice iterators the codegen is equivalent, `VecDeque`'s iterator seems to codegen differently but that doesn't seem significant enough to keep it as a perf lint
changelog: [`iter_nth`] Move to `style`
r? `@flip1995`
[`manual_retain`]: Fix duplicate diagnostics
Relates to: #12379
The first lint guard executed in `LateLintPass::check_expr` was testing if the parent was of type `ExprKind::Assign`. This meant the lint emitted on both sides of the assignment operator when `check_expr` is called on either `Expr`. The guard in the fix only lints once when the `Expr` is of kind `Assign`.
changelog: Fix duplicate lint diagnostic emission from [`manual_retain`]
Add new `duplicated_attributes` lint
It's a lint idea that `@llogiq` gave me while reviewing another PR.
There are some limitations, in particular for the "output". Initially I wanted to make it possible for directly lint against the whole attribute if its parts were all duplicated, but then I realized that the output would be chaotic if the duplicates were coming from different attributes, so I preferred to go to the simplest way and simply emit a warning for each entry. Not the best, but makes the implementation much easier.
Another limitation is that `cfg_attr` would be a bit more tricky to implement because we need to check if two `cfg` sets are exactly the same. I added a FIXME and will likely come back to it later.
And finally, I updated the `cargo dev update_lints` command because the generated `tests/ui/rename.rs` file was emitting the `duplicated_attributes` lint, so I allowed this lint inside it to prevent it from working.
changelog: Add new `duplicated_attributes` lint
[`mut_mut`]: Fix duplicate diags
Relates to #12379
The `mut_mut` lint produced two diagnostics for each `mut mut` pattern in `ty` inside `block`s because `MutVisitor::visit_ty` was called from `MutMut::check_ty` and `MutMut::check_block` independently. This PR fixes the issue.
---
changelog: [`mut_mut`]: Fix duplicate diagnostics
New lint `const_is_empty`
This lint detects calls to `.is_empty()` on an entity initialized from a string literal and flag them as suspicious. To avoid triggering on macros called from generated code, it checks that the `.is_empty()` receiver, the call itself and the initialization come from the same context.
Fixes#12307
changelog: [`const_is_empty`]: new lint
fix [`missing_docs_in_private_items`] on some proc macros
fixes: #12197
---
changelog: [`missing_docs_in_private_items`] support manually search for docs as fallback method
Have the lint trigger even if `Self` has generic lifetime parameters.
```rs
impl<'a> Foo<'a> {
type Item = Foo<'a>; // Can be replaced with Self
fn new() -> Self {
Foo { // No lifetime, but they are inferred to be that of Self
// Can be replaced as well
...
}
}
// Don't replace `Foo<'b>`, the lifetime is different!
fn eq<'b>(self, other: Foo<'b>) -> bool {
..
}
```
Fixes#12381
Remove double expr lint
Related to #12379.
Previously the code manually checked nested binop exprs in unary exprs, but those were caught anyway by `check_expr`. Removed that code path, the path is used in the tests.
---
changelog: [`nonminimal_bool`] Remove duplicate output on nested Binops in Unary exprs.
Add `assigning_clones` lint
This PR is a "revival" of https://github.com/rust-lang/rust-clippy/pull/10613 (with `@kpreid's` permission).
I tried to resolve most of the unresolved things from the mentioned PR:
1) The lint now checks properly if we indeed call the functions `std::clone::Clone::clone` or `std::borrow::ToOwned::to_owned`.
2) It now supports both method and function (UFCS) calls.
3) A heuristic has been added to decide if the lint should apply. It will only apply if the type on which the method is called has a custom implementation of `clone_from/clone_into`. Notably, it will not trigger for types that use `#[derive(Clone)]`.
4) `Deref` handling has been (hopefully) a bit improved, but I'm not sure if it's ideal yet.
I also added a bunch of additional tests.
There are a few things that could be improved, but shouldn't be blockers:
1) When the right-hand side is a function call, it is transformed into e.g. `::std::clone::Clone::clone(...)`. It would be nice to either auto-import the `Clone` trait or use the original path and modify it (e.g. `clone::Clone::clone` -> `clone::Clone::clone_from`). I don't know how to modify the `QPath` to do that though.
2) The lint currently does not trigger when the left-hand side is a local variable without an initializer. This is overly conservative, since it could trigger when the variable has no initializer, but it has been already initialized at the moment of the function call, e.g.
```rust
let mut a;
...
a = Foo;
...
a = b.clone(); // Here the lint should trigger, but currently doesn't
```
These cases probably won't be super common, but it would be nice to make the lint more precise. I'm not sure how to do that though, I'd need access to some dataflow analytics or something like that.
changelog: new lint [`assigning_clones`]
[`misrefactored_assign_op`]: Fix duplicate diagnostics
Relate to #12379
The following diagnostics appear twice
```
--> tests/ui/assign_ops2.rs:26:5
|
LL | a *= a * a;
| ^^^^^^^^^^
|
help: did you mean `a = a * a` or `a = a * a * a`? Consider replacing it with
```
because `a` (lhs) appears in both left operand and right operand in the right hand side.
This PR fixes the issue so that if a diagnostic is created for an operand, the check of the other operand will be skipped. It's fine because the result is always the same in the affected operators.
changelog: [`misrefactored_assign_op`]: Fix duplicate diagnostics
Don't emit "missing backticks" lint if the element is wrapped in `<code>` HTML tags
Fixes#9473.
changelog: Don't emit "missing backticks" lint if the element is wrapped in `<code>` HTML tags
[`identity_op`]: Fix duplicate diagnostics
Relates to #12379
In the `identity_op` lint, the following diagnostic was emitted two times
```
--> tests/ui/identity_op.rs:156:5
|
LL | 1 * 1;
| ^^^^^ help: consider reducing it to: `1`
|
```
because both of the left operand and the right operand are the identity element of the multiplication.
This PR fixes the issue so that if a diagnostic is created for an operand, the check of the other operand will be skipped. It's fine because the result is always the same in the affected operators.
---
changelog: [`identity_op`]: Fix duplicate diagnostics
Check for try blocks in `question_mark` more consistently
Fixes#12337
I split this PR up into two commits since this moves a method out of an `impl`, which makes for a pretty bad diff (the `&self` parameter is now unused, and there isn't a reason for that function to be part of the `impl` now).
The first commit is the actual relevant change and the 2nd commit just moves stuff (github's "hide whitespace" makes the diff easier to look at)
------------
Now for the actual issue:
`?` within `try {}` blocks desugars to a `break` to the block, rather than a `return`, so that changes behavior in those cases.
The lint has multiple patterns to look for and in *some* of them it already does correctly check whether we're in a try block, but this isn't done for all of its patterns.
We could add another `self.inside_try_block()` check to the function that looks for `let-else-return`, but I chose to actually just move those checks out and instead have them in `LintPass::check_{stmt,expr}`. This has the advantage that we can't (easily) accidentally forget to add that check in new patterns that might be added in the future.
(There's also a bit of a subtle interaction between two lints, where `question_mark`'s LintPass calls into `manual_let_else`, so I added a check to make sure we don't avoid linting for something that doesn't have anything to do with `?`)
changelog: [`question_mark`]: avoid linting on try blocks in more cases
fix [`derive_partial_eq_without_eq`] FP on trait projection
fixes: #9413#9319
---
changelog: fix [`derive_partial_eq_without_eq`] FP on trait projection
Well, this is awkward, it works but I don't understand why, why `clippy_utils::ty::implements_trait` couldn't detects the existance of `Eq` trait, even thought it's obviously present in the derive attribute.
Pointers cannot be converted to integers at compile time
Fix#12402
changelog: [`transmutes_expressible_as_ptr_casts`]: do not suggest invalid const casts
Dedup std_instead_of_core by using first segment span for uniqueness
Relates to #12379.
Instead of checking that the paths have an identical span, it checks that the relevant `std` part of the path segment's span is identical. Added a multiline test, because my first implementation was worse and failed that, then I realized that you could grab the span off the first_segment `Ident`.
I did find another bug that isn't addressed by this, and that exists on master as well.
The path:
```Rust
use std::{io::Write, fmt::Display};
```
Will get fixed into:
```Rust
use core::{io::Write, fmt::Display};
```
Which doesn't compile since `io::Write` isn't in `core`, if any of those paths are present in `core` it'll do the replace and cause a miscompilation. Do you think I should file a separate bug for that? Since `rustfmt` default splits those up it isn't that big of a deal.
Rustfmt:
```Rust
// Pre
use std::{io::Write, fmt::Display};
// Post
use std::fmt::Display;
use std::io::Write;
```
---
changelog: [`std_instead_of_core`]: Fix duplicated output on multiple imports
fix: `manual_memcpy` wrong indexing for multi dimensional arrays
fixes: #9334
This PR fixes an invalid suggestion for multi-dimensional arrays.
For example,
```rust
let src = vec![vec![0; 5]; 5];
let mut dst = vec![0; 5];
for i in 0..5 {
dst[i] = src[i][i];
}
```
For the above code, Clippy suggests `dst.copy_from_slice(&src[i]);`, but it is not compilable because `i` is only used to loop the array.
I adjusted it so that Clippy `manual_memcpy` works properly for multi-dimensional arrays.
changelog: [`manual_memcpy`]: Fixes invalid indexing suggestions for multi-dimensional arrays
`os_local` impl of `thread_local` — regardless of whether it is const and
unlike other implementations — includes an `fn __init(): EXPR`.
Existing implementation of the lint checked for the presence of said
function and whether the expr can be made const. Because for `os_local`
we always have an `__init()`, it triggers for const implementations.
The solution is to check whether the `__init()` function is already const.
If it is `const`, there is nothing to do. Otherwise, we verify that we can
make it const.
Co-authored-by: Alejandra González <blyxyas@gmail.com>
Count stashed errors again
Stashed diagnostics are such a pain. Their "might be emitted, might not" semantics messes with lots of things.
#120828 and #121206 made some big changes to how they work, improving some things, but still leaving some problems, as seen by the issues caused by #121206. This PR aims to fix all of them by restricting them in a way that eliminates the "might be emitted, might not" semantics while still allowing 98% of their benefit. Details in the individual commit logs.
r? `@oli-obk`
Add new `mixed_attributes_style` lint
Add a new lint to detect cases where both inner and outer attributes are used on a same item.
r? `@llogiq`
----
changelog: Add new [`mixed_attributes_style`] lint
Stashed errors used to be counted as errors, but could then be
cancelled, leading to `ErrorGuaranteed` soundness holes. #120828 changed
that, closing the soundness hole. But it introduced other difficulties
because you sometimes have to account for pending stashed errors when
making decisions about whether errors have occured/will occur and it's
easy to overlook these.
This commit aims for a middle ground.
- Stashed errors (not warnings) are counted immediately as emitted
errors, avoiding the possibility of forgetting to consider them.
- The ability to cancel (or downgrade) stashed errors is eliminated, by
disallowing the use of `steal_diagnostic` with errors, and introducing
the more restrictive methods `try_steal_{modify,replace}_and_emit_err`
that can be used instead.
Other things:
- `DiagnosticBuilder::stash` and `DiagCtxt::stash_diagnostic` now both
return `Option<ErrorGuaranteed>`, which enables the removal of two
`delayed_bug` calls and one `Ty::new_error_with_message` call. This is
possible because we store error guarantees in
`DiagCtxt::stashed_diagnostics`.
- Storing the guarantees also saves us having to maintain a counter.
- Calls to the `stashed_err_count` method are no longer necessary
alongside calls to `has_errors`, which is a nice simplification, and
eliminates two more `span_delayed_bug` calls and one FIXME comment.
- Tests are added for three of the four fixed PRs mentioned below.
- `issue-121108.rs`'s output improved slightly, omitting a non-useful
error message.
Fixes#121451.
Fixes#121477.
Fixes#121504.
Fixes#121508.
The following code used to trigger the lint:
```rs
macro_rules! make_closure {
() => {
(|| {})
};
}
make_closure!()();
```
The lint would suggest to replace `make_closure!()()` with
`make_closure!()`, which changes the code and removes the call to the
closure from the macro. This commit fixes that.
Fixes#12358
Show duplicate diagnostics in UI tests by default
Duplicated diagnostics can indicate where redundant work is being done, this PR doesn't fix any of that but does indicate in which tests they're occurring for future investigation or to catch issues in future lints
changelog: none
[`map_entry`]: Check insert expression for map use
The lint makes sure that the map is not used (borrowed) before the call to `insert`. Since the lint creates a mutable borrow on the map with the `Entry`, it wouldn't be possible to replace such code with `Entry`. However, expressions up to the `insert` call are checked, but not expressions for the arguments of the `insert` call itself. This commit fixes that.
Fixes#11935
----
changelog: [`map_entry`]: Fix false positive when borrowing the map in the `insert` call
If the whole cast expression is a unary expression (`(*x as T)`) or an
addressof expression (`(&x as T)`), then not surrounding the suggestion
into a block risks us changing the precedence of operators if the cast
expression is followed by an operation with higher precedence than the
unary operator (`(*x as T).foo()` would become `*x.foo()`, which changes
what the `*` applies on).
The same is true if the expression encompassing the cast expression is a
unary expression or an addressof expression.
The lint supports the latter case, but missed the former one. This PR
fixes that.
Fixes#11968
The lint makes sure that the map is not used (borrowed) before the call
to `insert`. Since the lint creates a mutable borrow on the map with the
`Entry`, it wouldn't be possible to replace such code with `Entry`.
However, expressions up to the `insert` call are checked, but not
expressions for the arguments of the `insert` call itself. This commit
fixes that.
Fixes#11935
Fix sign-handling bugs and false negatives in `cast_sign_loss`
**Note: anyone should feel free to move this PR forward, I might not see notifications from reviewers.**
changelog: [`cast_sign_loss`]: Fix sign-handling bugs and false negatives
This PR fixes some arithmetic bugs and false negatives in PR #11883 (and maybe earlier PRs).
Cc `@J-ZhengLi`
I haven't updated the tests yet. I was hoping for some initial feedback before adding tests to cover the cases listed below.
Here are the issues I've attempted to fix:
#### `abs()` can return a negative value in release builds
Example:
```rust
i32::MIN.abs()
```
https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=022d200f9ef6ee72f629c0c9c1af11b8
Docs: https://doc.rust-lang.org/std/primitive.i32.html#method.abs
Other overflows that produce negative values could cause false negatives (and underflows could produce false positives), but they're harder to detect.
#### Values with uncertain signs can be positive or negative
Any number of values with uncertain signs cause the whole expression to have an uncertain sign, because an uncertain sign can be positive or negative.
Example (from UI tests):
```rust
fn main() {
foo(a: i32, b: i32, c: i32) -> u32 {
(a * b * c * c) as u32
//~^ ERROR: casting `i32` to `u32` may lose the sign of the value
}
println!("{}", foo(1, -1, 1));
}
```
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=165d2e2676ee8343b1b9fe60db32aadd
#### Handle `expect()` the same way as `unwrap()`
Since we're ignoring `unwrap()` we might as well do the same with `expect()`.
This doesn't seem to have tests but I'm happy to add some like `Some(existing_test).unwrap() as u32`.
#### A negative base to an odd exponent is guaranteed to be negative
An integer `pow()`'s sign is only uncertain when its operants are uncertain. (Ignoring overflow.)
Example:
```rust
((-2_i32).pow(3) * -2) as u32
```
This offsets some of the false positives created by one or more uncertain signs producing an uncertain sign. (Rather than just an odd number of uncertain signs.)
#### Both sides of a multiply or divide should be peeled recursively
I'm not sure why the lhs was peeled recursively, and the rhs was left intact. But the sign of any sequence of multiplies and divides is determined by the signs of its operands. (Ignoring overflow.)
I'm not sure what to use as an example here, because most expressions I want to use are const-evaluable.
But if `p()` is [a non-const function that returns a positive value](https://doc.rust-lang.org/std/primitive.i32.html#method.isqrt), and if the lint handles unary negation, these should all lint:
```rust
fn peel_all(x: i32) {
(-p(x) * -p(x) * -p(x)) as u32;
((-p(x) * -p(x)) * -p(x)) as u32;
(-p(x) * (-p(x) * -p(x))) as u32;
}
```
#### The right hand side of a Rem doesn't change the sign
Unlike Mul and Div,
> Given remainder = dividend % divisor, the remainder will have the same sign as the dividend.
https://doc.rust-lang.org/reference/expressions/operator-expr.html#arithmetic-and-logical-binary-operators
I'm not sure what to use as an example here, because most expressions I want to use are const-evaluable.
But if `p()` is [a non-const function that returns a positive value](https://doc.rust-lang.org/std/primitive.i32.html#method.isqrt), and if the lint handles unary negation, only the first six expressions should lint.
The expressions that start with a constant should lint (or not lint) regardless of whether the lint supports `p()` or unary negation, because only the dividend's sign matters.
Example:
```rust
fn rem_lhs(x: i32) {
(-p(x) % -1) as u32;
(-p(x) % 1) as u32;
(-1 % -p(x)) as u32;
(-1 % p(x)) as u32;
(-1 % -x) as u32;
(-1 % x) as u32;
// These shouldn't lint:
(p(x) % -1) as u32;
(p(x) % 1) as u32;
(1 % -p(x)) as u32;
(1 % p(x)) as u32;
(1 % -x) as u32;
(1 % x) as u32;
}
```
#### There's no need to bail on other expressions
When peeling, any other operators or expressions can be left intact and sent to the constant evaluator.
If these expressions can be evaluated, this offsets some of the false positives created by one or more uncertain signs producing an uncertain sign. If not, they end up marked as having uncertain sign.
[`read_line_without_trim`]: detect string literal comparison and `.ends_with()` calls
This lint now also realizes that a comparison like `s == "foo"` and calls such as `s.ends_with("foo")` will fail if `s` was initialized by a call to `Stdin::read_line` (because of the trailing newline).
changelog: [`read_line_without_trim`]: detect string literal comparison and `.ends_with()` calls
r? `@giraffate` assigning you because you reviewed #10970 that added this lint, so this is kinda a followup PR ^^
fix suggestion error in [`useless_vec`]
fixes: #12101
---
changelog: fix suggestion error in [`useless_vec`]
r+ `@matthiaskrgr` since they opened the issue?
Empty docs
Fixes https://github.com/rust-lang/rust-clippy/issues/9931
changelog: [`empty_doc`]: Detects documentation that is empty.
changelog: Doc comment lints now trigger for struct field and enum variant documentation
When encountering code such as:
```
Box::new(outer::Inner::default())
```
clippy would suggest replacing with `Box::<Inner>::default()`, dropping
the `outer::` segment. This behavior is incorrect and that commit fixes
it.
What it does is it checks the contents of the `Box::new` and, if it is
of the form `A::B::default`, does a text replacement, inserting `A::B`
in the `Box`'s quickfix generic list.
If the source does not match that pattern (including `Vec::from(..)`
or other `T::new()` calls), we then fallback to the original code.
Fixes#11927
Look for `implied_bounds_in_impls` in more positions
With this, we lint `impl Trait` implied bounds in more positions:
- Type alias impl trait
- Associated type position impl trait
- Argument position impl trait
- these are not opaque types, but instead are desugared to `where` clauses, so we need extra logic for finding them (`check_generics`), however the rest of the logic is the same
Before this, we'd only lint RPIT `impl Trait`s.
"Hide whitespaces" and reviewing commits individually might make this easier
changelog: [`implied_bounds_in_impls`]: start linting implied bounds in APIT, ATPIT, TAIT
FIX(12243): redundant_guards
Fixed#12243
changelog: Fix[`redundant_guards`]
I have made a correction so that no warning does appear when y.is_empty() is used within a constant function as follows.
```rust
pub const fn const_fn(x: &str) {
match x {
// Shouldn't lint.
y if y.is_empty() => {},
_ => {},
}
}
```
A warning is now suppressed when "<str_va> if <str_var>.is_empty" is used in a constant function.
FIX: instead of clippy_util::in_const
FIX: Merged `redundant_guards_const_fn.rs` into `redundant_guards.rs`.
Extend `unnecessary_to_owned` to handle `Borrow` trait in map types
Fixes https://github.com/rust-lang/rust-clippy/issues/8088.
Alternative to #12315.
r? `@y21`
changelog: Extend `unnecessary_to_owned` to handle `Borrow` trait in map types
----
UPDATE: add async block into test.
FIX: no_effect
Fixed asynchronous function parameter names with underscores so that warnings are not displayed when underscores are added to parameter names
ADD: test case
Always evaluate free constants and statics, even if previous errors occurred
work towards https://github.com/rust-lang/rust/issues/79738
We will need to evaluate static items before the `definitions.freeze()` below, as we will start creating new `DefId`s (for nested allocations) within the `eval_static_initializer` query.
But even without that motivation, this is a good change. Hard errors should always be reported and not silenced if other errors happened earlier.
Ensure ASM syntax detect `global_asm!` and `asm!` only on x86 architectures
The ASM syntax lint is only relevant on x86 architectures, so this PR ensures it doesn't trigger on other architectures. This PR also makes the lints check `global_asm!` items as well as `asm!` expressions.
changelog: Check `global_asm!` items in the ASM syntax lints, and fix false positives on non-x86 architectures.
When encountering a verbose/multipart suggestion that has changes
that are only caused by different capitalization of ASCII letters that have
little differenciation, expand the message to highlight that fact (like we
already do for inline suggestions).
The logic to do this was already present, but implemented incorrectly.
[`incompatible_msrv`]: allow expressions that come from desugaring
Fixes#12273
changelog: [`incompatible_msrv`]: don't lint on the `IntoFuture::into_future` call desugared by `.await`
[`implied_bounds_in_impls`]: avoid linting on overlapping associated tys
Fixes#11880
Before this change, we were simply ignoring associated types (except for suggestion purposes), because of an incorrect assumption (see the comment that I also removed).
For something like
```rs
trait X { type T; }
trait Y: X { type T; }
// Can't constrain `X::T` through `Y`
fn f() -> impl X<T = i32> + Y<T = u32> { ... }
```
We now avoid linting if the implied bound (`X<T = i32>`) "names" associated types that also exists in the implying trait (`trait Y`). Here that would be the case.
But if we only wrote `impl X + Y<T = u32>` then that's ok because `X::T` was never constrained in the first place.
I haven't really thought about how this interacts with GATs, but I think it's fine. Fine as in, it might create false negatives, but hopefully no false positives.
(The diff is slightly annoying because of formatting things. Really the only thing that changed in the if chain is extracting the `implied_by_def_id` which is needed for getting associated types from the trait, and of course actually checking for overlap)
cc `@Jarcho` ? idk if you want to review this or not. I assume you looked into this code a bit to find this bug.
changelog: [`implied_bounds_in_impls`]: avoid linting when associated type from supertrait can't be constrained through the implying trait bound
[`mem_replace_with_default`] No longer triggers on unused expression
changelog:[`mem_replace_with_default`]: No longer triggers on unused expression
Change [`mem_replace_with_default`] to not trigger on unused expression because the lint from `#[must_use]` handle this case better.
fixes: #5586
fix: ICE when array index exceeds usize
fixes#12253
This PR fixes ICE in `indexing_slicing` as it panics when the index of the array exceeds `usize`.
changelog: none
stop linting [`blocks_in_conditions`] on `match` with weird attr macro case
should fixes: #12016
---
changelog: [`blocks_in_conditions`] - fix FP on `match` with weird attr macro
This might not be the best solution, as the root cause (i think?) is the `span` of block was incorrectly given by the compiler?
I'm open to better solutions
A lot of cases of the "noise" cases of `similar_names` come from two
idents with a different first letter, which is easy enough to
differentiate visually but causes this lint to be raised.
Do not raise the lint in these cases, as long as the first character
does not have a lookalike.
Link: https://github.com/rust-lang/rust-clippy/issues/10926
Fix issue #12034: add autofixes for unnecessary_fallible_conversions
fixes#12034
Currently, the `unnecessary_fallible_conversions` lint was capable of autofixing expressions like `0i32.try_into().unwrap()`. However, it couldn't autofix expressions in the form of `i64::try_from(0i32).unwrap()` or `<i64 as TryFrom<i32>>::try_from(0).unwrap()`.
This pull request extends the functionality to correctly autofix these latter forms as well.
changelog: [`unnecessary_fallible_conversions`]: Add autofixes for more forms
[`unconditional_recursion`]: compare by `Ty`s instead of `DefId`s
Fixes#12154Fixes#12181 (this was later edited in, so the rest of the description refers to the first linked issue)
Before this change, the lint would work with `DefId`s and use those to compare types. This PR changes it to compare types directly. It fixes the linked issue, but also other false positives I found in a lintcheck run. For example, one of the issues is that some types don't have `DefId`s (primitives, references, etc., leading to possible FNs), and the helper function used to extract a `DefId` didn't handle type parameters.
Another issue was that the lint would use `.peel_refs()` in a few places where that could lead to false positives (one such FP was in the `http` crate). See the doc comment on one of the added functions and also the test case for what I mean.
The code in the linked issue was linted because the receiver type is `T` (a `ty::Param`), which was not handled in `get_ty_def_id` and returned `None`, so this wouldn't actually *get* to comparing `self_arg != ty_id` here, and skip the early-return:
70573af31e/clippy_lints/src/unconditional_recursion.rs (L171-L178)
This alone could be fixed by doing something like `&& get_ty_def_id(ty).map_or(true, |ty_id)| self_arg != ty_id)`, but we don't really need to work with `DefId`s in the first place, I don't think.
changelog: [`unconditional_recursion`]: avoid linting when the other comparison type is a type parameter
Fix false positive in `redundant_type_annotations` lint
This PR changes the `redundant_type_annotations` lint to allow slice type annotations (i.e., `&[u8]`) for byte string literals. It will still consider _array_ type annotations (i.e., `&[u8; 4]`) as redundant. The reasoning behind this is that the type of byte string literals is by default a reference to an array, but, by using a type annotation, you can force it to be a slice. For example:
```rust
let a: &[u8; 4] = b"test";
let b: &[u8] = b"test";
```
Now, the type annotation for `a` will still be linted (as it is still redundant), but the type annotation for `b` will not.
Fixes#12212.
changelog: [`redundant_type_annotations`]: Fix false positive with byte string literals
Return `Some` from `walk_to_expr_usage` more
fixes#11786
supersedes #11097
The code removed in the first commit would have needed changes due to the second commit. Since it's useless it just gets removed instead.
changelog: `needless_borrow`: Fix linting in tuple and array expressions.
[`redundant_locals`]: take by-value closure captures into account
Fixes#12225
The same problem in the linked issue can happen to regular closures too, and conveniently async blocks are closures in the HIR so fixing closures will fix async blocks as well.
changelog: [`redundant_locals`]: avoid linting when redefined variable is captured by-value
make matching on NaN a hard error, and remove the rest of illegal_floating_point_literal_pattern
These arms would never be hit anyway, so the pattern makes little sense. We have had a future-compat lint against float matches in general for a *long* time, so I hope we can get away with immediately making this a hard error.
This is part of implementing https://github.com/rust-lang/rfcs/pull/3535.
Closes https://github.com/rust-lang/rust/issues/41620 by removing the lint.
https://github.com/rust-lang/reference/pull/1456 updates the reference to match.
Add new lint: `ref_as_ptr`
Fixes#10130
Added new lint `ref_as_ptr` that checks for conversions from references to pointers and suggests using `std::ptr::from_{ref, mut}` instead.
The name is different than suggested in the issue (`as_ptr_cast`) since there were some other lints with similar names (`ptr_as_ptr`, `borrow_as_ptr`) and I wanted to follow the convention.
Note that this lint conflicts with the `borrow_as_ptr` lint in the sense that it recommends changing `&foo as *const _` to `std::ptr::from_ref(&foo)` instead of `std::ptr::addr_of!(foo)`. Personally, I think the former is more readable and, in contrast to `addr_of` macro, can be also applied to temporaries (cf. #9884).
---
changelog: New lint: [`ref_as_ptr`]
[#12087](https://github.com/rust-lang/rust-clippy/pull/12087)
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
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`]
```
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
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.
Make some non-diagnostic-affecting `QPath::LangItem` into regular `QPath`s
The rest of 'em affect diagnostics, so leave them alone... for now.
cc #115178
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)
Do not consider `async { (impl IntoFuture).await }` as redundant
changelog: [`redundant_async_block`]: do not trigger on `IntoFuture` instances
Fix#11959
[`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
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
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`
don't visit nested bodies in `is_const_evaluatable`
Fixes#11939
This ICE happened in `if_let_some_else_none`, but the root problem is in one of the utils that it uses.
It is (was) possible for `is_const_evalutable` to visit nested bodies which would lead to it trying to get the type of one of the expressions with the wrong typeck table, which won't have the type stored.
Notably, for the expression `Bytes::from_static(&[0; 256 * 1024]);` in the linked issue, the array length is an anonymous const in which type checking happens on its own, so we can't use the typeck table of the enclosing function in there.
Visiting nested bodies is also not needed for checking whether an expression can be const, so I think it's safe to ignore just ignore them altogether.
changelog: Fix ICE when checking for constness in nested bodies
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>
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...
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
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
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
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
Stabilize C string literals
RFC: https://rust-lang.github.io/rfcs/3348-c-str-literal.html
Tracking issue: https://github.com/rust-lang/rust/issues/105723
Documentation PR (reference manual): https://github.com/rust-lang/reference/pull/1423
# Stabilization report
Stabilizes C string and raw C string literals (`c"..."` and `cr#"..."#`), which are expressions of type [`&CStr`](https://doc.rust-lang.org/stable/core/ffi/struct.CStr.html). Both new literals require Rust edition 2021 or later.
```rust
const HELLO: &core::ffi::CStr = c"Hello, world!";
```
C strings may contain any byte other than `NUL` (`b'\x00'`), and their in-memory representation is guaranteed to end with `NUL`.
## Implementation
Originally implemented by PR https://github.com/rust-lang/rust/pull/108801, which was reverted due to unintentional changes to lexer behavior in Rust editions < 2021.
The current implementation landed in PR https://github.com/rust-lang/rust/pull/113476, which restricts C string literals to Rust edition >= 2021.
## Resolutions to open questions from the RFC
* Adding C character literals (`c'.'`) of type `c_char` is not part of this feature.
* Support for `c"..."` literals does not prevent `c'.'` literals from being added in the future.
* C string literals should not be blocked on making `&CStr` a thin pointer.
* It's possible to declare constant expressions of type `&'static CStr` in stable Rust (as of v1.59), so C string literals are not adding additional coupling on the internal representation of `CStr`.
* The unstable `concat_bytes!` macro should not accept `c"..."` literals.
* C strings have two equally valid `&[u8]` representations (with or without terminal `NUL`), so allowing them to be used in `concat_bytes!` would be ambiguous.
* Adding a type to represent C strings containing valid UTF-8 is not part of this feature.
* Support for a hypothetical `&Utf8CStr` may be explored in the future, should such a type be added to Rust.
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`
[`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
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
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.
[`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
Add tests for issues #10285, #10286, #10289, #10287Fixes#10285.
Fixes#10286.
Fixes#10289.
Fixes#10287.
This PR simply adds tests for the listed issues as they're already implemented so we can close them.
r? `@blyxyas`
changelog:none
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`]
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`
This was made possible by the removal of plugin support, which
simplified lint store creation.
This simplifies the places in rustc and rustdoc that call
`describe_lints`, which are early on. The lint store is now built before
those places, so they don't have to create their own lint store for
temporary use, they can just use the main one.
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
Don't check for late-bound vars, check for escaping bound vars
Fixes an assertion that didn't make sense. Many valid and well-formed types *have* late-bound vars (e.g. `for<'a> fn(&'a ())`), they just must not have *escaping* late-bound vars in order to be normalized correctly.
Addresses rust-lang/rust-clippy#11230, cc `@jyn514` and `@matthiaskrgr`
changelog: don't check for late-bound vars, check for escaping bound vars. Addresses rust-lang/rust-clippy#11230
Fixes to `manual_let_else`'s divergence check
A few changes to the divergence check in `manual_let_else` and moves it the implementation to `clippy_utils` since it's generally useful:
* Handle internal `break` and `continue` expressions.
e.g. The first loop is divergent, but the second is not.
```rust
{
loop {
break 'outer;
};
}
{
loop {
break;
};
}
```
* Match rust's definition of divergence which is defined via the type system.
e.g. The following is not considered divergent by rustc as the inner block has a result type of `()`:
```rust
{
'a: {
panic!();
break 'a;
};
}
```
* Handle when adding a single semicolon would make the expression divergent.
e.g. The following would be a divergent if a semicolon were added after the `if` expression:
```rust
{ if panic!() { 0 } else { 1 } }
```
changelog: None
Lint `needless_borrow` and `explicit_auto_deref` on most union field accesses
Changes both lints to follow rustc's rules around auto-deref through `ManuallyDrop` union fields rather than just bailing on union fields.
changelog: [`needless_borrow`] & [`explicit_auto_deref`]: Lint on most union field accesses
