Back from burnout
This reverts commit 5ea7044d72. I needed some time free from reviewing to focus on the Project Goal and myself.
Now I'm much better, and we can continue reviewing!
I hope that I can approve this myself 😅
changelog: none
Don't warn on proc macro generated code in `needless_return`
Fixes#13458Fixes#13457Fixes#13467Fixes#13479Fixes#13481Fixes#13526Fixes#13486
The fix is unfortunately a little more convoluted than just simply adding a `is_from_proc_macro`. That check *does* fix the issue, however it also introduces a bunch of false negatives in the tests, specifically when the returned expression is in a different syntax context, e.g. `return format!(..)`.
The proc macro check builds up a start and end pattern based on the HIR nodes and compares it to a snippet of the span, however that would currently fail for `return format!(..)` because we would have the patterns `("return", <something inside of the format macro>)`, which doesn't compare equal. So we now return an empty string pattern for when it's in a different syntax context.
"Hide whitespace" helps a bit for reviewing the proc macro detection change
changelog: none
Check for needless raw strings in `format_args!()` template as well
changelog: [`needless_raw_strings`, `needless_raw_string_hashes`]: check `format_args!()` template as well
Fix#13503
Show interior mutability chain in `mutable_key_type`
Fixes#10619
Just ran into this myself and I definitely agree it's not very nice to have to manually go through all the types involved to figure out why this happens and to evaluate if this is really a problem (knowing if the field of a struct is something that a hash impl relies on), so this changes the lint to emit notes for each step involved.
changelog: none
Style: do not defensively use `saturating_sub()`
Using `saturating_sub()` here in code which cannot fail brings a false sense of security. If for any reason a logic error was introduced and caused `self.loop_depth` to reach 0 before being decremented, using `saturating_sub(1)` would silently mask the programming error instead of panicking loudly as it should (at least in dev profile).
changelog: none
Using `saturating_sub()` here in code which cannot fail brings a false
sense of security. If for any reason a logic error was introduced and
caused `self.loop_depth` to reach 0 before being decremented, using
`saturating_sub(1)` would silently mask the programming error instead of
panicking loudly as it should (at least in dev profile).
Reduce default 'large array' threshold
As-is this threshold is `512kb`, but as #9449 points out this is way too high for most people to consider sensible (why would you want to copy `256kb` of data around on the stack or duplicate it via `const`) and didn't get any discussion when originally added. This PR reduces it the threshold to `1kb`, which is higher than the issue says ("a few cpu words") but helps out for actual codebases.
While reducing this, I found that `large_stack_arrays` was triggering for statically promoted arrays in constants/statics, so I also fixed that up as seen in the difference to [array_size_threshold.stderr](https://github.com/rust-lang/rust-clippy/compare/master...GnomedDev:rust-clippy:reduce-large-threshold?expand=1#diff-4c2a2a855d9ff7777f1d385be0c1bede2a3fc8aaab94837cde27a35235233fc7).
Closes#9449.
changelog: [`large_stack_arrays`]: No longer triggers in `static`/`const` context
changelog: [`large_const_arrays`]: Changed the default of [`array-size-threshold`] from `512kb` to `16kb`
Implement lint for regex::Regex compilation inside a loop
Closes#598.
Seems like a pretty simple one, I'm not sure if I sorted out all the lint plumbing correctly because I was adding it to the existing regex pass, but seems to work. The name is a bit jank and I'm super open to suggestions for changing it.
changelog: [`regex_creation_in_loops`]: Added lint for Regex compilation inside loops.
Compare trait references in `trait_duplication_in_bounds` correctly
Fixes#13476Fixes#11067Fixes#9915Fixes#9626
Currently, the `trait_duplication_in_bounds` lints has a helper type for a trait reference that can be used for comparison and hashing, represented as `{trait: Res, generic_args: Vec<Res>}`. However, there are a lot of issues with this. For one, a `Res` can't represent e.g. references, slices, or lots of other types, as well as const generics and associated type equality. In those cases, the lint simply ignores them and has no way of checking if they're actually the same.
So, instead of using `Res` for this, use `SpanlessEq` and `SpanlessHash` for comparisons with the trait path for checking if there are duplicates.
However, using `SpanlessEq` as is alone lead to a false negative in the test. `std::clone::Clone` + `foo::Clone` wasn't recognized as a duplicate, because it has different segments. So this also adds a new "mode" to SpanlessEq which compares by final resolution. (I've been wondering if this can't just be the default but it's quite a large scale change as it affects a lot of lints and I haven't yet looked at all uses of it to see if there are lints that really do care about having exactly the same path segments).
Maybe an alternative would be to turn the hir types/consts into middle types/consts and compare them instead but I'm not sure there's really a good way to do that
changelog: none
Simplify negative `Option::{is_some_and,is_none_or}`
Closes#13436.
Improved based on the existing lint `nonminimal_bool`, since there is already handling of similar methods `Option::{is_some,is_none}` and `Result::{is_ok,is_err}`, and there is a lot of reusable code.
When `is_some_and` or `is_none_or` have a negation, we invert it into another method by removing the Not sign and inverting the expression in the closure.
For the case where the closure block has statements, currently no simplification is implemented. (Should we do it?)
```rust
// Currently will not simplify this
_ = !opt.is_some_and(|x| {
let complex_block = 100;
x == complex_block
});
```
changelog: [`nonminimal_bool`]: Simplify negative `Option::{is_some_and,is_none_or}`
Fix `mut_mutex_lock` when reference not ultimately mutable
When there is are multiple references where one of the references isn't mutable then this results in a false-positive for `mut_mutex_lock` as it only checks the mutability of the first reference level.
Fix this by using `peel_mid_ty_refs_is_mutable` which correctly determines whether the reference is ultimately mutable and thus whether `Mutex::get_lock()` can actually be used.
Fixes#9854
changelog: [`mut_mutex_lock`]: No longer lints if the mutex is behind multiple references and one of those references isn't mutable
When there is are multiple references where one of the references
isn't mutable then this results in a false-positive for
`mut_mutex_lock` as it only checks the mutability of the first
reference level.
Fix this by using `peel_mid_ty_refs_is_mutable` which correctly
determines whether the reference is ultimately mutable and thus
whether `Mutex::get_lock()` can actually be used.
Fixes#9854
`zombie_processes`: consider `wait()` calls in nested bodies
Fixes#13459
Small oversight. We weren't considering uses of the local in closures.
changelog: none
Extend `needless_lifetimes` to suggest eliding `impl` lifetimes
Example:
```
error: the following explicit lifetimes could be elided: 'a
--> tests/ui/needless_lifetimes.rs:332:10
|
LL | impl<'a> Foo for Baz<'a> {}
| ^^ ^^
|
help: elide the lifetimes
|
LL - impl<'a> Foo for Baz<'a> {}
LL + impl Foo for Baz<'_> {}
```
The main change is in how `impl` lifetime uses are tracked. Previously, a hashmap was created, and lifetimes were removed from the hashmap as their uses were discovered. However, the uses are needed to generate elision suggestions. So, now, uses are added to the hashmap as they are discovered.
The PR is currently organized as six commits, which I think are self-explanatory:
- Extend `needless_lifetimes` to suggest eliding `impl` lifetimes
- Reorder functions _[not strictly necessary, but IMHO, the code is better structured as a result]_
- Fix lifetime tests
- Fix non-lifetime tests
- Fix `clippy_lints` and `clippy_utils`
- Fix typo in `needless_lifetimes` test
r? `@Alexendoo` (I think you are `needless_lifetimes`' primary author? Sorry if I have this wrong.)
---
changelog: Extend `needless_lifetimes` to suggest eliding `impl` lifetimes
Convert `&Option<T>` to `Option<&T>`
Run `ref_option` (#13336) on the Clippy's own code, quiet a few hits. Per mentioned video, this may actually improve performance as well. Switch lint to `pedantic`
----
changelog: [`ref_option`]: upgrade lint to `pedantic`
Suggest `Option<&T>` instead of `&Option<T>`
closes#13054
```rust
// bad code
fn foo(a: &Option<T>) {}
fn bar(&self) -> &Option<T> {}
// Use instead
fn foo(a: Option<&T>) {}
fn bar(&self) -> Option<&T> {}
```
Handles argument types and return types in functions, methods, and closures with explicit types. Honors `avoid_breaking_exported_api` parameter.
See this great [YouTube video](https://www.youtube.com/watch?v=6c7pZYP_iIE) with the in-depth explanation.
### Open Questions
These are not blocking, and could be done in separate PRs if needed.
* [ ] Should `&Option<Box<T>>` be suggested as `Option<&T>` -- without the box? Handled by [clippy::borrowed_box](https://rust-lang.github.io/rust-clippy/master/index.html#/borrowed_box)
* [ ] Should `&Option<String>` be suggested as `Option<&str>` -- using de-refed type?
### Possible Future Improvements
These cases might also be good to handle, probably in a separate PR.
```rust
fn lambdas() {
let x = |a: &Option<String>| {};
let x = |a: &Option<String>| -> &Option<String> { todo!() };
}
fn mut_ref_to_ref(a: &mut &Option<u8>) {}
```
changelog: [`ref_option`]: Suggest `Option<&T>` instead of `&Option<T>`
Remove method call receiver special casing in `unused_async` lint
Fixes the false positive mentioned in https://github.com/rust-lang/rust-clippy/issues/13466#issuecomment-2380025514.
The false negative in the OP would be nice to fix too, but I'd rather do that in a separate PR because it's much more involved
Before this change, the `unused_async` lint would check if the async fn is also used anywhere and avoid linting if so. The exception is if the async function is immediately called, because the returned future handling can be easily removed (and also if we don't have some exceptions then the lint wouldn't trigger anywhere) *or* if it's a method call receiver.
I'm not exactly sure why I implemented that special casing for method call receivers in #11200, but it doesn't make much sense in hindsight imo. Especially given that method calls are essentially equivalent to function calls with the receiver as the first argument, which was the primary motivation for not linting in the first place (async fn passed to another function, like `axum::get(handler)` where handler has to be an async fn).
changelog: none
