Fix FP for `suspicious_arithmetic_impl` from `suspicious_trait_impl` …
As discussed in #3215, the `suspicious_trait_impl` lint causes too many false positives, as it is complex to find out if binary operations are suspicious or not.
This PR restricts the number of binary operations to at most one, otherwise we don't lint.
This can be seen as very conservative, but at least FP can be reduced to bare minimum.
Fixes: #3215
changelog: limit the `suspicious_arithmetic_impl` lint to one binop, to avoid many FPs
Ignore not really redundant clones of ManuallyDrop
"Redundant" clones of `ManuallyDrop` are sometimes used for the side effect of
invoking the clone, without running the drop implementation of the inner type.
In other words, they aren't really redundant. For example, futures-rs crate:
```rust
#[allow(clippy::redundant_clone)] // The clone here isn't actually redundant.
unsafe fn increase_refcount<T: ArcWake>(data: *const ()) {
// Retain Arc, but don't touch refcount by wrapping in ManuallyDrop
let arc = mem::ManuallyDrop::new(Arc::<T>::from_raw(data as *const T));
// Now increase refcount, but don't drop new refcount either
let _arc_clone: mem::ManuallyDrop<_> = arc.clone();
}
```
changelog: Ignore redundant clone lint for ManuallyDrop.
"Redundant" clones of `ManuallyDrop` are sometimes used for the side effect of
invoking the clone, without running the drop implementation of the inner type.
In other words, they aren't really redundant. For example, futures-rs crate:
```rust
#[allow(clippy::redundant_clone)] // The clone here isn't actually redundant.
unsafe fn increase_refcount<T: ArcWake>(data: *const ()) {
// Retain Arc, but don't touch refcount by wrapping in ManuallyDrop
let arc = mem::ManuallyDrop::new(Arc::<T>::from_raw(data as *const T));
// Now increase refcount, but don't drop new refcount either
let _arc_clone: mem::ManuallyDrop<_> = arc.clone();
}
```
Ignore redundant clone lint for ManuallyDrop.
Panic multiple args
changelog: Fixes bug with `panic` lint reported in #5767. I also did the same changes to the lints for `todo`, `unimplemented` and `unreachable`, so those lints should now also detect calls to those macros with a message.
improve advice in iter_nth_zero
fixes#5783
*Please keep the line below*
changelog: For iter_nth_zero, the "use .next()" replacement advice is on the last line of the code snippet, where it is vulnerable to truncation. Display that advice at the beginning instead.
This lint catches cases where the last statement of a closure expecting
an instance of Ord has a trailing semi-colon. It compiles since the
closure ends up return () which also implements Ord but causes
unexpected results in cases such as sort_by_key.
Fixes#5080
reprise: rebase, update and address all concerns
Rename collapsable_if fix suggestion to "collapse nested if block"
The name "try" is confusing when shown as quick fix by rust-analyzer
changelog: Rename `collapsable_if` fix suggestion to "collapse nested if block"
The "use .next()" replacement advice is on the last line of the code snippet,
where it is vulnerable to truncation. Display that advice at the beginning
instead.
closes#5783
Move range_minus_one to pedantic
This moves the range_minus_one lint to the pedantic category, so there
will not be any warnings emitted by default. This should work around
problems where the suggestion is impossible to resolve due to the range
consumer only accepting a specific range implementation, rather than the
`RangeBounds` trait (see #3307).
While it is possible to work around this by extracting the boundary into
a variable, I don't think clippy should encourage people to disable or
work around lints, but instead the lints should be fixable. So hopefully
this will help until a proper implementation checks what the range is
used for.
*Please keep the line below*
changelog: move [`range_minus_one`] to pedantic
Some accuracy lints for floating point operations
This will add some lints for accuracy on floating point operations suggested by @clarfon in #2040 (fixes#2040).
These are the remaining lints:
- [x] x.powi(2) => x * x
- [x] x.logN() / y.logN() => x.logbase(y)
- [x] x.logbase(E) => x.log()
- [x] x.logbase(10) => x.log10()
- [x] x.logbase(2) => x.log2().
- [x] x * PI / 180 => x.to_radians()
- [x] x * 180 / PI => x.to_degrees()
- [x] (x + 1).log() => x.log_1p()
- [x] sqrt(x * x + y * y) => x.hypot(y)
changelog: Included some accuracy lints for floating point operations
Stabilize `transmute` in constants and statics but not const fn
cc #53605 (leaving issue open so we can add `transmute` to `const fn` later)
Previous attempt: #64011
r? @RalfJung
cc @rust-lang/wg-const-eval
new lint: match_like_matches_macro
Suggests using the `matches!` macro from `std` where appropriate.
`redundant_pattern_matching` has been moved into the `matches` pass to allow suppressing the suggestion where `is_some` and friends are a better replacement.
changelog: new lint: `match_like_matches_macro`
single_match_else - single expr/stmt else block corner case
One approach to fix#3489.
See discussion in the issue.
changelog: single_match_else - single expr/stmt else block corner case fix
Added restriction lint: pattern-type-mismatch
changelog: Added a new restriction lint `pattern-type-mismatch`. This lint is especially helpful for beginners learning about the magic behind pattern matching. (This explanation might be worth to include in the next changelog.)
Fix the versions of packages in the multiple_crate_versions ui test by
checking in the Cargo.lock for the test package. `ansi_term 0.11`
depends on `winapi ^0.3.4`. This means means that the expected stderr for
this test would have to be updated whenever `winapi 0.3` is updated
otherwise.
cmp_owned: handle when PartialEq is not implemented symmetrically
changelog: Handle asymmetrical implementations of PartialEq in [`cmp_owned`].
Fixes#4874
clone_on_copy - add machine applicability
Fix#4826.
Change the applicability of the lint clone_on_copy. Split a test file and run rustfix on the clone_on_copy part.
changelog: clone_on_copy - add machine applicability
#5626: lint iterator.map(|x| x)
changelog: adds a new lint for iterator.map(|x| x) (see https://github.com/rust-lang/rust-clippy/issues/5626)
The code also lints for result.map(|x| x) and option.map(|x| x). Also, I'm not sure if I'm checking for type adjustments correctly and I can't think of an example where .map(|x| x) would apply type adjustments.
New lint: suggest `ptr::read` instead of `mem::replace(..., uninitialized())`
resolves: #5575
changelog: improvements to `MEM_REPLACE_WITH_UNINIT`:
- add a new test case in `tests/ui/repl_uninit.rs` to cover the case of replacing with `mem::MaybeUninit::uninit().assume_init()`.
- modify the existing `MEM_REPLACE_WITH_UNINIT` when replacing with `mem::uninitialized` to suggest using `ptr::read` instead.
- lint with `MEM_REPLACE_WITH_UNINIT` when replacing with `mem::MaybeUninit::uninit().assume_init()`
None of the tools seem to need syn 0.15.35, so we can just build syn
1.0.
This was causing an issue with clippy's `compile-test` program: since
multiple versions of `syn` would exist in the build directory, we would
non-deterministically pick one based on filesystem iteration order. If
the pre-1.0 version of `syn` was picked, a strange build error would
occur (see
https://github.com/rust-lang/rust/pull/73594#issuecomment-647671463)
To prevent this kind of issue from happening again, we now panic if we
find multiple versions of a crate in the build directly, instead of
silently picking the first version we find.
let_and_return: avoid "does not live long enough" errors
EDIT: Add #3324 to the list of fixes
<details>
<summary>Description of old impl</summary>
<br>
Avoid suggesting turning the RHS expression of the last statement into the block tail expression if a temporary borrows from a local that would be destroyed before.
This is my first incursion into MIR so there's probably room for improvement!
</details>
Avoid linting if the return type of some method or function called in the last statement has a lifetime parameter.
changelog: Fix false positive in [`let_and_return`]
Fixes#3792Fixes#3324
Add regression test for `string_lit_as_bytes` issue
Closes#5619
Before the fix in https://github.com/rust-lang/rust/pull/72637, `string_lit_as_bytes` was incorrectly triggering on the `env!` macro. With the fix merged, this test makes sure that the lint is not triggering anymore.
changelog: none
New lint: iter_next_slice
Hello, this is a work-in-progress PR for issue: https://github.com/rust-lang/rust-clippy/issues/5572
I have implemented lint to replace `iter().next()` for `slice[index..]` and `array` with `get(index)` and `get(0)` respectively. However since I made a lot of changes, I would like to request some feedback before continuing so that I could fix mistakes.
Thank you!
---
changelog: implement `iter_next_slice` lint and test, and modify `needless_continues`, `for_loop_over_options_result` UI tests since they have `iter().next()`
Add regression test for endless loop / update `pulldown_cmark`
Closes#4917
This was fixed in pulldown_cmark 0.7.1, specifically raphlinus/pulldown-cmark#438
changelog: none
cargo-ui tests: check that <dir>/src exists before processing test
I forgot that I had fixed this in a PR I closed some days ago (#5643).
Before this change, cargo UI tests could fail when switching between branches if the previous branch had a test that the current branch does not have. The directory is not removed when switching because an ignored `Cargo.lock` file exists, and the code was trying to reach `$DIR/src` unconditionally.
This change will just skip a directory that has no `src` subdirectory.
changelog: none
len_zero: skip ranges if feature `range_is_empty` is not enabled
If the feature is not enabled, calling `is_empty()` on a range is ambiguous. Moreover, the two possible resolutions are unstable methods, one inherent to the range and the other being part of the `ExactSizeIterator` trait.
Since `len_zero` only checks for existing `is_empty()` inherent methods, we only take into account the `range_is_empty` feature.
Related: https://github.com/rust-lang/rust/issues/48111#issuecomment-445132965
changelog: len_zero: avoid linting ranges without #![feature(range_is_empty)]
Fixes: #3807
Extend useless conversion
This PR extends `useless_conversion` lint with `TryFrom` and `TryInto`
fixes: #5344
changelog: Extend `useless_conversion` with `TryFrom` and `TryInto`
Make empty_line_after_outer_attr an early lint
Fixes#5567
Unfortunately I couldn't find a way to reproduce the issue without syn/quote. Considering that most real-world macros use syn and/or quote, I think it's okay to pull them in anyway.
changelog: Fix false positive in [`empty_line_after_outer_attr`]
reversed_empty_ranges: add suggestion for &slice[N..N]
As discussed in the issue thread, the user accepted this solution. Let me know if this is what we want, or if changing the way we lint the N..N case is prefered.
changelog: reversed_empty_ranges: add suggestion for &slice[N..N]
Closes#5628
ptr_arg: honor `allow` attribute on arguments
The `intravisit::Visitor` impl for `LateContextAndPass` only takes into account the attributes of a function parameter inside the `check_param` method. `ptr_arg` starts its heuristics at `check_item` / `check_impl_item` / `check_trait_item`, so the `allow` is not taken into account automatically.
changelog: ptr_arg: honor `allow` attribute on arguments
Fixes#5644
option_option test case #4298
Adds regression test case for #4298.
The bug seems still present although rust Playground said otherwise.
changelog: none
new_without_default: do not suggest deriving
---
changelog: do not suggest deriving `Default` in `new_without_default`
This commit changes the behavior of the `new_without_default` lint to not suggest deriving `Default`. This suggestion is misleading if the `new` implementation does something different than what a derived `Default` implementation would do, because then the two methods would not be equivalent.
Instead, the `can_derive_default` check is removed, and we always suggest implementing `Default` in terms of `new()`.
New lint: `match_wildcard_for_single_variants`
changelog: Added a new lint match_wildcard_for_single_variants to warn on enum matches where a wildcard is used to match a single variant
Closes#5556
Rename lint `identity_conversion` to `useless_conversion`
Lint name `identity_conversion` was misleading, so this PR renames it to `useless_conversion`.
As decision has not really came up in the issue comments, this PR will probably need discussion.
fixes#3106
changelog: Rename lint `identity_conversion` to `useless_conversion`
Merge some lints together
This PR merges following lints:
- `block_in_if_condition_expr` and `block_in_if_condition_stmt` → `blocks_in_if_conditions`
- `option_map_unwrap_or`, `option_map_unwrap_or_else` and `result_map_unwrap_or_else` → `map_unwrap`
- `option_unwrap_used` and `result_unwrap_used` → `unwrap_used`
- `option_expect_used` and `result_expect_used` → `expect_used`
- `wrong_pub_self_convention` into `wrong_self_convention`
- `for_loop_over_option` and `for_loop_over_result` → `for_loops_over_fallibles`
Lints that have already been merged since the issue was created:
- [x] `new_without_default` and `new_without_default_derive` → `new_without_default`
Need more discussion:
- `string_add` and `string_add_assign`: do we agree to merge them or not? Is there something more to do? → **not merge finally**
- `identity_op` and `modulo_one` → `useless_arithmetic`: seems outdated, since `modulo_arithmetic` has been created.
fixes#1078
changelog: Merging some lints together:
- `block_in_if_condition_expr` and `block_in_if_condition_stmt` → `blocks_in_if_conditions`
- `option_map_unwrap_or`, `option_map_unwrap_or_else` and `result_map_unwrap_or_else` → `map_unwrap_or`
- `option_unwrap_used` and `result_unwrap_used` → `unwrap_used`
- `option_expect_used` and `result_expect_used` → `expect_used`
- `for_loop_over_option` and `for_loop_over_result` → `for_loops_over_fallibles`
identity_op: allow `1 << 0`
I went for accepting `1 << 0` verbatim instead of something more general as it seems to be what everyone in the issue thread needed.
changelog: identity_op: allow `1 << 0` as it's a common pattern in bit manipulation code.
Fixes#3430
Reversed empty ranges
This lint checks range expressions with inverted limits which result in empty ranges. This includes also the ranges used to index slices.
The lint reverse_range_loop was covering iteration of reversed ranges in a for loop, which is a subset of what this new lint covers, so it has been removed. I'm not sure if that's the best choice. It would be doable to check in the new lint that we are not in the arguments of a for loop; I went for removing it because the logic was too similar to keep them separated.
changelog: Added reversed_empty_ranges lint that checks for ranges where the limits have been inverted, resulting in empty ranges. Removed reverse_range_loop which was covering a subset of the new lint.
Closes#4192Closes#96
Fix match on vec items: match on vec[..]
- Added new tests
- Fixed false positive when matching on full range, which will never panic
Closes#5551
changelog: fix match_on_vec_items when matching full range
Fix `unnecessary_unwrap` lint when checks are done in parameters
Fixes a false positive in `unnecessary_unwrap` lint when checks are done in macro parameters.
FIxes#5174
changelog: Fixes a false positive in `unnecessary_unwrap` lint when checks are done in macro parameters.
Fix FP on while-let-on-iterator
- fix `is_refutable` for slice patterns
- fix `is_refutable` for bindings
- add some TODO-s for cases, which can not be fixed easily
fixes#3780
changelog: fix FP on while-let-on-iterator for arrays and bindings
Implement the manual_non_exhaustive lint
Some implementation notes:
* Not providing automatic fixups because additional changes may be needed in other parts of the code, e.g. when constructing a struct.
* Even though the attribute is valid on enum variants, it's not possible to use the manual implementation of the pattern because the visibility is always public, so the lint ignores enum variants.
* Unit structs are also ignored, it's not possible to implement the pattern manually without fields.
* The attribute is not accepted in unions, so those are ignored too.
* Even though the original issue did not mention it, tuple structs are also linted because it's possible to apply the pattern manually.
changelog: Added the manual non-exhaustive implementation lint
Closes#2017
Fix the bugs of `manual_memcpy`, simplify the suggestion and refactor it
While I’m working on the long procrastinated work to expand `manual_memcpy`(#1670), I found a few minor bugs and probably unidiomatic or old coding style. There is a brief explanation of changes to the behaviour this PR will make below. And, I have a questoin: do I need to add tests for the first and second fixed bugs? I thought it might be too rare cases to include the tests for those. I added for the last one though.
* Bug fix
* It negates resulted offsets (`src/dst_offset`) when `offset` is subtraction by 0. This PR will remove any subtraction by 0 as a part of minification.
```rust
for i in 0..5 {
dst[i - 0] = src[i];
}
```
```diff
warning: it looks like you're manually copying between slices
--> src/main.rs:2:14
|
LL | for i in 0..5 {
- | ^^^^ help: try replacing the loop by: `dst[..-5].clone_from_slice(&src[..5])`
+ | ^^^^ help: try replacing the loop by: `dst[..5].clone_from_slice(&src[..5])`
|
```
* It prints `RangeTo` or `RangeFull` when both of `end` and `offset` are 0, which have different meaning. This PR will print 0. I could reject the cases `end` is 0, but I thought I won’t catch other cases `reverse_range_loop` will trigger, and it’s over to catch every such cases.
```rust
for i in 0..0 {
dst[i] = src[i];
}
```
```diff
warning: it looks like you're manually copying between slices
--> src/main.rs:2:14
|
LL | for i in 0..0 {
- | ^^^^ help: try replacing the loop by: `dst.clone_from_slice(&src[..])`
+ | ^^^^ help: try replacing the loop by: `dst[..0].clone_from_slice(&src[..0])`
|
```
* it prints four dots when `end` is `None`. This PR will ignore any `for` loops without `end` because a `for` loop that takes `RangeFrom` as its argument and contains indexing without the statements or the expressions that end loops such as `break` will definitely panic, and `manual_memcpy` should ignore the loops with such control flow.
```rust
fn manual_copy(src: &[u32], dst: &mut [u32]) {
for i in 0.. {
dst[i] = src[i];
}
}
```
```diff
-warning: it looks like you're manually copying between slices
- --> src/main.rs:2:14
- |
-LL | for i in 0.. {
- | ^^^ help: try replacing the loop by: `dst[....].clone_from_slice(&src[....])`
- |
```
* Simplification of the suggestion
* It prints 0 when `start` or `end` and `offset` are same (from #3323). This PR will use `RangeTo`
changelog: fixed the bugs of `manual_memcpy` and also simplify the suggestion.
New lint `match_vec_item`
Added new lint to warn a match on index item which can panic. It's always better to use `get(..)` instead.
Closes#5500
changelog: New lint `match_on_vec_items`
- Show just one error message with multiple suggestions in case of
using multiple times an OS in target family position
- Only suggest #[cfg(unix)] when the OS is in the Unix family
- Test all the operating systems
Don't trigger while_let_on_iterator when the iterator is recreated every iteration
r? @phansch
Fixes#1654
changelog: Fix false positive in [`while_let_on_iterator`]
Downgrade match_bool to pedantic
I don't quite buy the justification in https://rust-lang.github.io/rust-clippy/. The justification is:
> It makes the code less readable.
In the Rust codebases I've worked in, I have found people were comfortable using `match bool` (selectively) to make code more readable. For example, initializing struct fields is a place where the indentation of `match` can work better than the indentation of `if`:
```rust
let _ = Struct {
v: {
...
},
w: match doing_w {
true => ...,
false => ...,
},
x: Nested {
c: ...,
b: ...,
a: ...,
},
y: if doing_y {
...
} else { // :(
...
},
z: ...,
};
```
Or sometimes people prefer something a bit less pithy than `if` when the meaning of the bool doesn't read off clearly from the condition:
```rust
if set.insert(...) {
... // ???
} else {
...
}
match set.insert(...) {
// set.insert returns false if already present
false => ...,
true => ...,
}
```
Or `match` can be a better fit when the bool is playing the role more of a value than a branch condition:
```rust
impl ErrorCodes {
pub fn from(b: bool) -> Self {
match b {
true => ErrorCodes::Yes,
false => ErrorCodes::No,
}
}
}
```
And then there's plain old it's-1-line-shorter, which means we get 25% more content on a screen when stacking a sequence of conditions:
```rust
let old_noun = match old_binding.is_import() {
true => "import",
false => "definition",
};
let new_participle = match new_binding.is_import() {
true => "imported",
false => "defined",
};
```
Bottom line is I think this lint fits the bill better as a pedantic lint; I don't think linting on this by default is justified.
changelog: Remove match_bool from default set of enabled lints
Fixes#4226
This introduces the lint await_holding_lock. For async functions, we iterate
over all types in generator_interior_types and look for types named MutexGuard,
RwLockReadGuard, or RwLockWriteGuard. If we find one then we emit a lint.
If let else mutex
changelog: Adds lint to catch incorrect use of `Mutex::lock` in `if let` expressions with lock calls in any of the blocks.
closes: #5219
Fix issue #2907.
Update the "borrow box" lint to avoid recommending the following
conversion:
```
// Old
pub fn f(&mut Box<T>) {...}
// New
pub fn f(&mut T) {...}
```
Given a mutable reference to a box, functions may want to change
"which" object the Box is pointing at.
This change avoids recommending removing the "Box" parameter
for mutable references.
changelog: Don't trigger [`borrow_box`] lint on `&mut Box` references
Update the "borrow box" lint to avoid recommending the following
conversion:
```
// Old
pub fn f(&mut Box<T>) {...}
// New
pub fn f(&mut T) {...}
```
Given a mutable reference to a box, functions may want to change
"which" object the Box is pointing at.
This change avoids recommending removing the "Box" parameter
for mutable references.
add lint futures_not_send
changelog: add lint futures_not_send
fixes#5379
~Remark: one thing that can (should?) still be improved is to directly include the error message from the `Send` check so that the programmer stays in the flow. Currently, getting the actual error message requires a restructuring of the code to make the `Send` constraint explicit.~
It now shows all unmet constraints for allowing the Future to be Send.
Do not lint in macros for match lints
Don't lint in macros for match lints, more precisely in `check_pat` and `check_local` where it was not the case.
changelog: none
fixes: #5362
large_enum_variant: Report sizes of variants
This reports the sizes of the largest and second-largest variants.
Closes#5459
changelog: `large_enum_variant`: Report the sizes of the largest and second-largest variants.
Disallow bit-shifting in integer_arithmetic
Make the `integer_arithmetic` lint detect all the operations that are defined as being capable of overflow in the [Rust Reference](https://doc.rust-lang.org/reference/expressions/operator-expr.html#overflow), by also linting for bit-shifting operations (`<<`, `>>`).
changelog: Disallow bit-shifting in `integer_arithmetic`
Add lint on large non scalar const
This PR adds the new lint `non_scalar_const` that aims to warn against `const` declaration of large arrays. For performance, because of inlining, large arrays should be preferably declared as `static`.
Note: i made this one to warn on all const arrays, whether they are in a body function or not. I don't know if this is really necessary, i could just reduce this lint to variables out of function scope.
Fixes: #400
changelog: add new lint for large non-scalar types declared as const
