Don't lint literal `None` from expansion
This addresses https://github.com/rust-lang/rust-clippy/pull/9288#issuecomment-1229398524: If the literal `None` is from expansion, we never lint. This is correct because e.g. replacing the call to `option_env!` with whatever that macro expanded to at the time of linting is certainly wrong.
changelog: Don't lint [`partialeq_to_none`] for macro-expansions
Ignore `match_like_matches_macro` when there is comment
Closes#9164
changelog: [`match_like_matches_macro`] is ignored when there is some comment inside the match block.
Also add `span_contains_comment` util to check if given span contains comments.
Implemented `suspicious_to_owned` lint to check if `to_owned` is called on a `Cow`
changelog: Add lint ``[`suspicious_to_owned`]``
-----------------
Hi,
posting this unsolicited PR as I've been burned by this issue :)
Being unsolicited, feel free to reject it or reassign a different lint level etc.
This lint checks whether `to_owned` is called on `Cow<'_, _>`. This is done because `to_owned` is very similarly named to `into_owned`, but the effect of calling those two methods is completely different (one makes the `Cow::Borrowed` into a `Cow::Owned`, the other just clones the `Cow`). If the cow is then passed to code for which the type is not checked (e.g. generics, closures, etc.) it might slip through and if the cow data is coming from an unsafe context there is the potential for accidentally cause undefined behavior.
Even if not falling into this painful case, there's really no reason to call `to_owned` on a `Cow` other than confusing people reading the code: either `into_owned` or `clone` should be called.
Note that this overlaps perfectly with `implicit_clone` as a warning, but `implicit_clone` is classified pedantic (while the consequences for `Cow` might be of a wider blast radius than just pedantry); given the overlap, I set-up the lint so that if `suspicious_to_owned` triggers `implicit_clone` will not trigger. I'm not 100% sure this is done in the correct way (I tried to copy what other lints were doing) so please provide feedback on it if it isn't.
### Checklist
- \[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`
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
This is done because `to_owned` is very similarly named to `into_owned`, but the
effect of calling those two methods is completely different. This creates
confusion (stemming from the ambiguity of the 'owned' term in the context of
`Cow`s) and might not be what the writer intended.
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
