Don't lint `needless_return` if `return` has attrs
Fixes#9361
The lint used to have a mechanic to allow `cfg`-attrs on naked `return`-statements. This was well-intentioned, yet we can have any kind of attribute, e.g. `allow`, `expect` or even custom `derive`. So the mechanic was simply removed. We now never lint on a naked `return`-statement that has attributes on it.
Turns out that the ui-test had a Catch22 in it: In `check_expect()` the `#[expect(clippy::needless_return)]` is an attribute on the `return` statement that can and will be rustfixed away without side effects. But any other attribute would also have been removed, which is what #9361 is about. The test proved the wrong thing. Removed the test, the body is tested elsewhere as well.
changelog: Ignore [`needless_return`] on `return`s with attrs
new lint
This fixes#6576
If you added a new lint, here's a checklist for things that will be
checked during review or continuous integration.
- \[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`
---
changelog: add [`multi_assignments`] lint
Replace `contains_ty(..)` with `Ty::contains(..)`
This removes some code we don't need and the method syntax is
also more readable IMO.
changelog: none
Rename `manual_empty_string_creation` and move to pedantic
Renames it to `manual_string_new` and moves it to the pedantic category
Pedantic because it's a fairly minor style change but could be very noisy
changelog: *doesn't need its own entry, but remember to s/manual_empty_string_creation/manual_string_new/ the changelog entry for #9295*
r? `@xFrednet` to get it in before the upcoming sync as this isn't a `cargo dev rename_lint` style rename
feat(fix): Do not lint if the target code is inside a loop
close#8753
we consider the following code.
```rust
fn main() {
let vec = vec![1];
let w: Vec<usize> = vec.iter().map(|i| i * i).collect(); // <- once.
for i in 0..2 {
let _ = w.contains(&i);
}
}
```
and the clippy will issue the following warning.
```rust
warning: avoid using `collect()` when not needed
--> src/main.rs:3:51
|
3 | let w: Vec<usize> = vec.iter().map(|i| i * i).collect();
| ^^^^^^^
...
6 | let _ = w.contains(&i);
| -------------- the iterator could be used here instead
|
= note: `#[warn(clippy::needless_collect)]` on by default
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_collect
help: check if the original Iterator contains an element instead of collecting then checking
|
3 ~
4 |
5 | for i in 0..2 {
6 ~ let _ = vec.iter().map(|i| i * i).any(|x| x == i);
```
Rewrite the code as indicated.
```rust
fn main() {
let vec = vec![1];
for i in 0..2 {
let _ = vec.iter().map(|i| i * i).any(|x| x == i); // <- execute `map` every loop.
}
}
```
this code is valid in the compiler, but, it is different from the code before the rewrite.
So, we should not lint, If `collect` is outside of a loop.
Thank you in advance.
---
changelog: Do not lint if the target code is inside a loop in `needless_collect`
check for if-some-or-ok-else-none-or-err
fixes: #8492
---
changelog: make [`option_if_let_else`] to check for match expression with both Option and Result; **TODO: Change lint name? Add new lint with similar functionality?**
Add test for #8855Fix#8855
Here is what I think is going on.
First, the expression `format!("{:>6} {:>6}", a, b.to_string())` expands to:
```rust
{
let res =
::alloc::fmt::format(::core::fmt::Arguments::new_v1_formatted(&["",
" "],
&[::core::fmt::ArgumentV1::new_display(&a),
::core::fmt::ArgumentV1::new_display(&b.to_string())],
&[::core::fmt::rt::v1::Argument {
position: 0usize,
format: ::core::fmt::rt::v1::FormatSpec {
fill: ' ',
align: ::core::fmt::rt::v1::Alignment::Right,
flags: 0u32,
precision: ::core::fmt::rt::v1::Count::Implied,
width: ::core::fmt::rt::v1::Count::Is(6usize),
},
},
::core::fmt::rt::v1::Argument {
position: 1usize,
format: ::core::fmt::rt::v1::FormatSpec {
fill: ' ',
align: ::core::fmt::rt::v1::Alignment::Right,
flags: 0u32,
precision: ::core::fmt::rt::v1::Count::Implied,
width: ::core::fmt::rt::v1::Count::Is(6usize),
},
}], unsafe { ::core::fmt::UnsafeArg::new() }));
res
}
```
When I dump the expressions that get past the call to `has_string_formatting` [here](b312ad7d0c/clippy_lints/src/format_args.rs (L83)), I see more than I would expect.
In particular, I see this subexpression of the above:
```
&[::core::fmt::ArgumentV1::new_display(&a),
::core::fmt::ArgumentV1::new_display(&b.to_string())],
```
This suggests to me that more expressions are getting past [this call](b312ad7d0c/clippy_lints/src/format_args.rs (L71)) to `FormatArgsExpn::parse` than should.
Those expressions are then visited, but no `::core::fmt::rt::v1::Argument`s are found and pushed [here](b312ad7d0c/clippy_utils/src/macros.rs (L407)).
As a result, the expressions appear unformatted, hence, the false positive.
My proposed fix is to restrict `FormatArgsExpn::parse` so that it only matches `Call` expressions.
cc: `@akanalytics`
changelog: none
Lint `collapsible_str_replace`
fixes#6651
```
changelog: [`collapsible_str_replace`]: create new lint `collapsible_str_replace`
```
If you added a new lint, here's a checklist for things that will be
checked during review or continuous integration.
- \[x] Followed [lint naming conventions][lint_naming]
- \[x] Added passing UI tests (including committed `.stderr` file)
- \[x] `cargo test` passes locally
- \[ ] Executed `cargo dev update_lints`
- \[x] Added lint documentation
- \[x] Run `cargo dev fmt`
Rework `only_used_in_recursion`
fixes#8782fixes#8629fixes#8560fixes#8556
This is a complete rewrite of the lint. This loses some capabilities of the old implementation. Namely the ability to track through tuple and slice patterns, as well as the ability to trace through assignments.
The two reported bugs are fixed with this. One was caused by using the name of the method rather than resolving to the `DefId` of the called method. The second was cause by using the existence of a cycle in the dependency graph to determine whether the parameter was used in recursion even though there were other ways to create a cycle in the graph.
Implementation wise this switches from using a visitor to walking up the tree from every use of each parameter until it has been determined the parameter is used for something other than recursion. This is likely to perform better as it avoids walking the entire function a second time, and it is unlikely to walk up the HIR tree very much. Some cases would perform worse though.
cc `@buttercrab`
changelog: Scale back `only_used_in_recursion` to fix false positives
changelog: Move `only_used_in_recursion` back to `complexity`
Refactor `FormatArgsExpn`
It now for each format argument `{..}` has:
- The `Expr` it points to, and how it does so (named/named inline/numbered/implicit)
- The parsed `FormatSpec` (format trait/fill/align/etc., the precision/width and any value they point to)
- Many spans
The caller no longer needs to pair up arguments to their value, or separately interpret the `specs` `Expr`s when it isn't `None`
The gist is that it combines the result of [`rustc_parse_format::Parser`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_parse_format/struct.Parser.html) with the macro expansion itself
This unfortunately makes the code a bit longer, however we need to use both as neither have all the information we're after. `rustc_parse_format` doesn't have the information to resolve named arguments to their values. The macro expansion doesn't contain whether the positions are implicit/numbered/named, or the spans for format arguments
Wanted by #9233 and #8518 to be able to port the changes from #9040
Also fixes#8643, previously the format args seem to have been paired up with the wrong values somehow
changelog: [`format_in_format_args`]: Fix false positive due to misattributed arguments
r? `@flip1995`
cc `@nyurik`
