Improved wording of or_fun_call lint
The current wording (e.g. ``use of `ok_or` followed by a function call``) is potentially confusing (at least it confused me) by suggesting that the function that follows the (in this case) `ok_or` is the problem and not the function that is an argument to it.
The code in my program that triggered the confusing message is the following:
```rust
let file_id = buf
.lines()
.next()
.ok_or((
InternalError::ProblemReadingFromInbox,
anyhow!("No first line in inbox response ({file:?}): {buf:?}"),
))
.html_context(stream, lang)?;
```
I thought that `html_context` was the problem and that I should do something along the following lines:
```rust
let file_id = buf
.lines()
.next()
.ok_or_else(
(
InternalError::ProblemReadingFromInbox,
anyhow!("No first line in inbox response ({file:?}): {buf:?}"),
),
html_context(stream, lang),
)?
```
This is of course wrong. My confusion was only cleared up through the help message indicating what I should try instead.
If someone has a better idea of a replacement wording (currently e.g. ``` function call inside of `ok_or` ```), I'm all ears.
changelog: none
Turn declare_clippy_lint into a declarative macro
Ease of development, and hopefully compile times (the dependencies are still there because of ui-test). The procedural macro was doing just some very basic processing (like assigning a lint level to each category), so it didn't have a reason to stay IMO
changelog: None
Fix large_stack_arrays triggering when nesting const items
Fixes#13529.
r? `@flip1995`
changelog: [`large_stack_arrays`]: No longer triggers in static/const context when using nested items
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}`
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
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
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>`
