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
Fix FP in `threadlocal!` when falling back to `os_local`
**Issue**: The lint always triggers for some targets.
**Why**: The lint assumes an `__init` function is not present for const initializers.
This does not hold for all targets. Some targets always have an initializer function.
**Fix**: If an initializer function exists, we check that it is not a `const` fn.
Lint overview before the patch:
1. Outside `threadlocal!`, then exit early.
2. In `threadlocal!` and `__init` does not exist => is const already, exit early.
3. In `threadlocal!` and `__init` exists and `__init` body can be `const` => we lint.
Lint overview after the patch:
1. Outside `threadlocal!`, then exit early.
2. In `threadlocal!` and `__init` does not exist => is const already, exit early.
3. In `threadlocal!` and `__init` exists and is `const fn` => is const already, exit early.
4. In `threadlocal!` and `__init` exists and is not `const fn` and `__init` body can be `const` => we lint.
The patch adds step 3.
changelog: Fixed fp in [`thread_local_initializer_can_be_made_const`] where fallback implementation of `threadlocal!` was always linted.
## Verifying the changes
For each of these platforms [ mac, x86_64-windows-gnu, x86_64-windows-msvc ]:
1. switched to master, ran bless. Found a failure for windows-gnu.
2. switched to patch, ran bless. Success for all three platforms.
## How platform affects the lint:
The compiler performs a [conditional compilation depending on platform features](https://doc.rust-lang.org/src/std/sys/common/thread_local/mod.rs.html). The `os_local` fallback includes a call to `__init`, without step 3, we lint in all cases.
```rust
cfg_if::cfg_if! {
if #[cfg(any(all(target_family = "wasm", not(target_feature = "atomics")), target_os = "uefi"))] {
#[doc(hidden)]
mod static_local;
#[doc(hidden)]
pub use static_local::{Key, thread_local_inner};
} else if #[cfg(target_thread_local)] {
#[doc(hidden)]
mod fast_local;
#[doc(hidden)]
pub use fast_local::{Key, thread_local_inner};
} else {
#[doc(hidden)]
mod os_local;
#[doc(hidden)]
pub use os_local::{Key, thread_local_inner};
}
}
```
r? `@llogiq`
`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>
Bump macOS CI version to 13
I looked into the random failures we've been getting on macOS CI, not entirely sure what is going on but it seems to be an issue out of our department. A [plain driver](314425001d/src/driver.rs) being ran on the test files [without `ui_test` or any special flags](314425001d/src/driver.rs) hit the same issue
It didn't occur on `macos-13` the few times I tried it though so let's upgrade to that. [The current `macos-latest` refers to `macos-12`](https://github.com/actions/runner-images?tab=readme-ov-file#available-images), later this year it will refer to `macos-14` which runs on `aarch64` so specifying the version for our x64 macOS tests will also save a future headache
r? `@flip1995`
changelog: none
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
Alphabetize configuration options and lints in Clippy documentation
changelog: alphabetize configuration options and affected lints listed in section 3.1. of the Clippy Documentation
r? blyxyas
[`redundant_closure_call`]: Don't lint if closure origins from a macro
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
----
changelog: [`redundant_closure_call`]: If `x!()` returns a closure, don't suggest replacing `x!()()` with `x!()`
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
Fix `nonminimal_bool` lint regression
Fixes#12371.
Fixes#12369.
cc `@RalfJung`
The problem was an invalid condition. Shame on me...
changelog: Fix `nonminimal_bool` lint regression
Improve `is_lint_level` code
Since https://github.com/rust-lang/rust/pull/121230 was merged, we can now rely on `Level` directly instead of keeping the list of symbols to check in clippy.
changelog: Improve `is_lint_level` code
[`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
[`unnecessary_cast`]: Avoid breaking precedence
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
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: [`unnecessary_cast`]: Avoid breaking precedence with unary operators (`(*x as T).foo()` -- before: `*x.foo()` -- now: `{*x}.foo()`)
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
[`box_default`]: Preserve required path segments
When encountering code such as:
```rs
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
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: [`box_default`]: Preserve required path segments
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
style: sync GitHub Corner colors
fixes#12349.
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: sync site GitHub Corner colors
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
Merge `single_call_fn` post-crate visitor into lint pass
The `single_call_fn` lint worked by first collecting a list of function definitions in the lint pass, then populating the list of uses for each function in a second visitor after the crate is checked.
Doing another pass through the crate shouldn't be needed, and we should be able to do it in the same lint pass, by looking for path references to functions only and then processing them post-crate.
Other changes:
- `FxHashMap` -> `FxIndexMap` so that we emit warnings in a consistent order, as we see them (making the diff a bit confusing to look at, because warnings were moved around)
- no longer storing a `Vec<Span>` per function: an enum representing "seen once" or "seen more than once" should be enough (only the first element is used later)
- "used here" help is now a note
I also noticed that it lints on trait methods with a default implementation, but not on regular trait methods without a body (because that's what `check_fn` does). I'm not sure if that's useful though, maybe we shouldn't lint trait methods at all? It's not like you can avoid it sometimes (but then again it's a restriction lint). Either way, I left the behavior where it was before so that there are no functional changes made in this PR and it's purely a refactor. I can change it though
changelog: none
