Add consistency with phrases "meantime" and "mean time"
"mean time" is used in a few places while "meantime" is used everywhere else; this would make usage consistent throughout the codebase.
fix incorrect suggestion for `!(a as type >= b)`
fixes#12625
The expression `!(a as type >= b)` got simplified to `a as type < b`, but because of rust's parsing rules that `<` is interpreted as a start of generic arguments for `type`. This is fixed by recognizing this case and adding extra parens around the left-hand side of the comparison.
changelog: [`nonminimal_bool`]: fix incorrect suggestion for `!(a as type >= b)`
[`manual_unwrap_or_default`]: Check for Default trait implementation in initial condition when linting and use `IfLetOrMatch`
Fixes#12564
changelog: Fix [`manual_unwrap_or_default`] false positive when initial `match`/`if let` condition doesn't implement `Default` but the return type does.
Allow `cast` lints in macros
closes: #11738
Removed the `from_expansion` guard clause for cast lints, so that these warnings can be generated for internal macros.
changelog: allow `cast` lints in macros
type certainty: clear `DefId` when an expression's type changes to non-adt
Fixes#12585
The root cause of the ICE in the linked issue was in the expression `one.x`, in the array literal.
The type of `one` is the `One` struct: an adt with a DefId, so its certainty is `Certain(def_id_of_one)`. However, the field access `.x` can then change the type (to `i32` here) and that should update that `DefId` accordingly. It does do that correctly when `one.x` would be another adt with a DefId:
97ba291d5a/clippy_utils/src/ty/type_certainty/mod.rs (L90-L91)
but when it *isn't* an adt and there is no def id (which is the case in the linked issue: `one.x` is an i32), it keeps the `DefId` of `One`, even though that's the wrong type (which would then lead to a contradiction later when joining `Certainty`s):
97ba291d5a/clippy_utils/src/ty/type_certainty/mod.rs (L92-L93)
In particular, in the linked issue, `from_array([one.x, two.x])` would try to join the `Certainty` of the two array elements, which *should* have been `[Certain(None), Certain(None)]`, because `i32`s have no `DefId`, but instead it was `[Certain(One), Certain(Two)]`, because the DefId wasn't cleared from when it was visiting `one` and `two`. This is the "contradiction" that could be seen in the ICE message
... so this changes it to clear the `DefId` when it isn't an adt.
cc `@smoelius` you implemented this initially in #11135, does this change make sense to you?
changelog: none
Reword `arc_with_non_send_sync` note and help messages
Addresses https://github.com/rust-lang/rust-clippy/issues/12608#issuecomment-2029688054
Makes the note more concise and reframes the `Rc` suggestion around whether it crosses threads currently due to a manual `Send`/`Sync` impl or may do in the future
changelog: none
FIX(12334): manual_swap auto fix
Fixed: #12334
Initialization expressions are now generated as needed if the slice index is bound to a variable.
----
changelog: Fix [`manual_swap`]
Do not suggest `assigning_clones` in `Clone` impl
This PR modifies `assigning_clones` to detect situations where the `clone` call is inside a `Clone` impl, and avoids suggesting the lint in such situations.
r? `@blyxyas`
Fixes: https://github.com/rust-lang/rust-clippy/issues/12600
changelog: Do not invoke `assigning_clones` inside `Clone` impl
avoid an ICE in `ptr_as_ptr` when getting the def_id of a local
Fixes#12616
`Res::def_id` can panic, so avoid calling it in favor of `opt_def_id`, so we can gracefully handle resolutions that don't have a `DefId` (e.g. local variables) and get a false negative in the worst case, rather than an ICE
changelog: Fix ICE in [`ptr_as_ptr`] when the cast expression is a function call to a local variable
Initialization expressions are now generated when index is bound to a variable.
FIX: Check to see if variables are used after swap
FIX: rename StmtKind::Local to StmtKind::Let
Elide unit variables linted by `let_unit` and use `()` directly instead
Situation: `let_unit` lints when an expression binds a unit (`()`) to a variable. In some cases this binding may be passed down to another function. Currently, the lint removes the binding without considering usage.
fixes: #12594
changelog: Suggestion Fix [`let_unit`]. Clippy will remove unit bindings and replace all their instances in the body with `()`.
Situation: `let_unit` lints when an expression binds a unit (`()`)
to a variable. In some cases this binding may be passed down to
another function. Currently, the lint removes the binding without
considering usage.
Change: All usages of the elided variable are now replaced with `()`.
fixes: #12594
`Box::default()` had its `#[rustc_box]` attribute removed in 1.69 so is
no longer a perf related lint
The lint is moved to style but no longer produces suggestions containing
turbofishes, as they're often longer/more annoying to type
Allow `filter_map_identity` when the closure is typed
This extends the `filter_map_identity` lint to support typed closures.
For untyped closures, we know that the program compiles, and therefore we can safely suggest using flatten.
For typed closures, they may participate in type resolution. In this case we use `Applicability::MaybeIncorrect`.
Details:
https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Should.20.60filter_map_identity.60.20lint.20when.20closures.20are.20typed.3F
changelog: `filter_map_identity` will now suggest using flatten for typed closures.
r? `@y21` && `@Centri3`
[`type_id_on_box`]: lint on any `Box<dyn _>`
Closes#11349.
It now not only lints when calling `.type_id()` on the type `Box<dyn Any>`, but also on any `Box<dyn Trait>` where `Trait` is a subtrait of `Any`
changelog: FN: [`type_id_on_box`]: lint if `Any` is a sub trait
new lint `legacy_numeric_constants`
Rework of #10997
- uses diagnostic items
- does not lint imports of the float modules (`use std::f32`)
- does not lint usage of float constants that look like `f32::MIN`
I chose to make the float changes because the following pattern is actually pretty useful
```rust
use std::f32;
let omega = freq * 2 * f32::consts::PI;
```
and the float modules are not TBD-deprecated like the integer modules.
Closes#10995
---
changelog: New lint [`legacy_numeric_constants`]
[#12312](https://github.com/rust-lang/rust-clippy/pull/12312)
make sure checked type implements `Try` trait when linting [`question_mark`]
(indirectly) fixes: #12412 and fixes: #11983
---
changelog: make sure checked type implements `Try` trait when linting [`question_mark`]
restrict manual_clamp to const case, bring it out of nursery
Implements the plan that I described in https://github.com/rust-lang/rust-clippy/pull/9484#issuecomment-1374522054
This does two things primarily
1. Restrict `manual_clamp` such that it will only trigger if we are able to guarantee that `clamp` won't panic at runtime.
2. Bring `manual_clamp` out of nursery status and move it into the complexity group.
changelog: [`manual_clamp`]: Restrict this lint such that it only triggers if max and min are const, and max is greater than or equal to min. Then bring it out of the nursery group.
Instead of just saying “this function's stack frame is big”, report:
* the (presumed) size of the frame
* the size and type of the largest local contributing to that size
* the configurable limit that was exceeded (once)
`useless_asref`: do not lint `.as_ref().map(Arc::clone)`
This applies to `Arc`, `Rc`, and their weak variants. Using `.clone()` would be less idiomatic.
This follows the discussion in <https://github.com/rust-lang/rust-clippy/issues/12528#issuecomment-2014444305>.
changelog: [`useless_asref`]: do not lint `.as_ref().map(Arc::clone)` and similar
don't lint [`mixed_attributes_style`] when mixing docs and other attrs
fixes: #12435fixes: #12436fixes: #12530
---
changelog: don't lint [`mixed_attributes_style`] when mixing different kind of attrs; and move it to late pass;
don't lint [`mixed_attributes_style`] when mixing docs and other attrs
add test files for issue #12436
move [`mixed_attributes_style`] to `LateLintPass` to enable global `allow`
stop [`mixed_attributes_style`] from linting on different attributes
add `@compile-flags` to [`mixed_attributes_style`]'s test;
turns out not linting in test mod is not a FN.
Apply suggestions from code review
Co-authored-by: Timo <30553356+y21@users.noreply.github.com>
move [`mixed_attributes_style`] to late pass and stop it from linting on different kind of attributes
Note that the caller chooses a type for type param
```
error[E0308]: mismatched types
--> $DIR/return-impl-trait.rs:23:5
|
LL | fn other_bounds<T>() -> T
| - -
| | |
| | expected `T` because of return type
| | help: consider using an impl return type: `impl Trait`
| expected this type parameter
...
LL | ()
| ^^ expected type parameter `T`, found `()`
|
= note: expected type parameter `T`
found unit type `()`
= note: the caller chooses the type of T which can be different from ()
```
Tried to see if "expected this type parameter" can be replaced, but that goes all the way to `rustc_infer` so seems not worth the effort and can affect other diagnostics.
Revives #112088 and #104755.
Do not warn on .map(_::clone) for Arc, Rc, and their weak variants
Those constructions are idiomatic, and using `Arc::clone(x)` and `Rc::clone(x)` is often the recommended way of cloning a `Arc` or a `Rc`.
Fix#12528
changelog: [`map_clone`]: do not warn on `.map(_::clone)` for `Arc`, `Rc`, and their `Weak` variants
Fix infinite loop in `cast_sign_loss` when peeling unwrap method calls
Fixes#12506
The lint wants to peel method calls but didn't actually reassign the expression, leading to an infinite loop.
----
changelog: Fix infinite loop in [`cast_sign_loss`] when having two chained `.unwrap()` calls
Several (doc) comments were super outdated or didn't provide enough context.
Some doc comments shoved everything in a single paragraph without respecting
the fact that the first paragraph should be a single sentence because rustdoc
treats these as item descriptions / synopses on module pages.
Disable `cast_lossless` when casting to u128 from any (u)int type
Fixes https://github.com/rust-lang/rust-clippy/issues/12492
Disables `cast_lossless` when casting to u128 from any int or uint type. The lint states that when casting to any int type, there can potentially be lossy behaviour if the source type ever exceeds the size of the destination type in the future, which is impossible with a destination of u128.
It's possible this is a bit of a niche edge case which is better addressed by just disabling the lint in code, but I personally couldn't think of any good reason to still lint in this specific case - maybe except if the source is a bool, for readability reasons :).
changelog: FP: `cast_lossless`: disable lint when casting to u128 from any (u)int type
[`map_entry`]: call the visitor on the local's `else` block
Fixes#12489
The lint already has all the logic it needs for figuring out if it can or can't suggest a closure if it sees control flow expressions like `break` or `continue`, but it was ignoring the local's else block, which meant that it didn't see the `return None;` in a `let..else`.
changelog: [`map_entry`]: suggest `if let` instead of a closure when `return` expressions exist in the else block of a `let..else`
new restriction lint: `integer_division_remainder_used`
Fixes https://github.com/rust-lang/rust-clippy/issues/12391
Introduces a restriction lint which disallows the use of `/` and `%` operators on any `Int` or `Uint` types (i.e., any that use the default `Div` or `Rem` trait implementations). Custom implementations of these traits are ignored.
----
changelog: Add new restriction lint [`integer_division_remainder_used`]
[`option_option`]: Fix duplicate diagnostics
Relates to #12379
This `option_option` lint change skips checks against `ty`s inside `field_def`s defined by external macro to prevent duplicate diagnostics to the same span `ty` by multiple `Struct` definitions.
---
changelog: [`option_option`]: Fix duplicate diagnostics
move `readonly_write_lock` to perf
[There haven't been any issues](https://github.com/rust-lang/rust-clippy/issues?q=is%3Aissue+readonly_write_lock) since its creation and it's a pretty useful lint I think, so I'd say it's worth giving it a try?
Did a lintcheck run on 300 crates with no results, but I guess `RwLock` is usually not something that's used much in libraries.
changelog: move [`readonly_write_lock`] to perf (now warn-by-default)
fix [`dbg_macro`] FN when dbg is inside some complex macros
fixes: #12131
It appears that [`root_macro_call_first_node`] only detects `println!` in the following example:
```rust
println!("{:?}", dbg!(s));
```
---
changelog: fix [`dbg_macro`] FN when `dbg` is inside some complex macros
(re-opening b'cuz bors doesn't like my previous one)
fix span calculation for non-ascii in `needless_return`
Fixes#12491
Probably fixes#12328 as well, but that one has no reproducer, so 🤷
The bug was that the lint used `rfind()` for finding the byte index of the start of the previous non-whitespace character:
```
// abc\n return;
^
```
... then subtracting one to get the byte index of the actual whitespace (the `\n` here).
(Subtracting instead of adding because it treats this as the length from the `return` token to the `\n`)
That's correct for ascii, like here, and will get us to the `\n`, however for non ascii, the `c` could be multiple bytes wide, which would put us in the middle of a codepoint if we simply subtract 1 and is what caused the ICE.
There's probably a lot of ways we could fix this.
This PR changes it to iterate backwards using bytes instead of characters, so that when `rposition()` finally finds a non-whitespace byte, we *know* that we've skipped exactly 1 byte. This was *probably*(?) what the code was intending to do
changelog: Fix ICE in [`needless_return`] when previous line end in a non-ascii character
[`unused_enumerate_index`]: trigger on method calls
The lint used to check for patterns looking like:
```rs
for (_, x) in some_iter.enumerate() {
// Index is ignored
}
```
This commit further checks for chained method calls constructs where we
can detect that the index is unused. Currently, this checks only for the
following patterns:
```rs
some_iter.enumerate().map_function(|(_, x)| ..)
let x = some_iter.enumerate();
x.map_function(|(_, x)| ..)
```
where `map_function` is one of `all`, `any`, `filter_map`, `find_map`,
`flat_map`, `for_each` or `map`.
Fixes#12411.
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: [`unused_enumerate_index`]: add detection for method chains such as `iter.enumerate().map(|(_, x)| x)`
Prior to this change, it might be that the lint would remove an explicit
type that was necessary for the type system to keep track of types.
This should be the last change that prevented this lint to be machine
applicable.
Don't emit `doc_markdown` lint for missing backticks if it's inside a quote
Fixes#10262.
changelog: Don't emit `doc_markdown` lint for missing backticks if it's inside a quote
[`use_self`]: Make it aware of lifetimes
Have the lint trigger even if `Self` has generic lifetime parameters.
```rs
impl<'a> Foo<'a> {
type Item = Foo<'a>; // Can be replaced with Self
fn new() -> Self {
Foo { // No lifetime, but they are inferred to be that of Self
// Can be replaced as well
...
}
}
// Don't replace `Foo<'b>`, the lifetime is different!
fn eq<'b>(self, other: Foo<'b>) -> bool {
..
}
```
Fixes#12381
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: [`use_self`]: Have the lint trigger even if `Self` has generic lifetime parameters
The lint used to check for patterns looking like:
```rs
for (_, x) in some_iter.enumerate() {
// Index is ignored
}
```
This commit further checks for chained method calls constructs where we
can detect that the index is unused. Currently, this checks only for the
following patterns:
```rs
some_iter.enumerate().map_function(|(_, x)| ..)
let x = some_iter.enumerate();
x.map_function(|(_, x)| ..)
```
where `map_function` is one of `all`, `any`, `filter_map`, `find_map`,
`flat_map`, `for_each` or `map`.
Fixes#12411.
lint when calling the blanket `Into` impl from a `From` impl
Closes#11150
```
warning: function cannot return without recursing
--> x.rs:9:9
|
9 | / fn from(value: f32) -> Self {
10 | | value.into()
11 | | }
| |_________^
|
note: recursive call site
--> x.rs:10:13
|
10 | value.into()
| ^^^^^^^^^^^^
```
I'm also thinking that we can probably generalize this lint to #11032 at some point (instead of hardcoding a bunch of impls), like how rustc's `unconditional_recursion` works, at least up to one indirect call, but this still seems useful for now :)
I've also noticed that we use `fn_def_id` in a bunch of lints and then try to get the node args of the call in a separate step, so I made a helper function that does both in one. I intend to refactor a bunch of uses of `fn_def_id` to use this later
I can add more test cases, but this is already using much of the same logic that exists for the other impls that this lint looks for (e.g. making sure that there are no conditional returns).
changelog: [`unconditional_recursion`]: emit a warning inside of `From::from` when unconditionally calling the blanket `.into()` impl
Move `iter_nth` to `style`, add machine applicable suggestion
There's no `O(n)` involved with `.iter().nth()` on the linted types since the iterator implementations provide `nth` and/or `advance_by` that operate in `O(1)`
For slice iterators the codegen is equivalent, `VecDeque`'s iterator seems to codegen differently but that doesn't seem significant enough to keep it as a perf lint
changelog: [`iter_nth`] Move to `style`
r? `@flip1995`
[`manual_retain`]: Fix duplicate diagnostics
Relates to: #12379
The first lint guard executed in `LateLintPass::check_expr` was testing if the parent was of type `ExprKind::Assign`. This meant the lint emitted on both sides of the assignment operator when `check_expr` is called on either `Expr`. The guard in the fix only lints once when the `Expr` is of kind `Assign`.
changelog: Fix duplicate lint diagnostic emission from [`manual_retain`]
Add new `duplicated_attributes` lint
It's a lint idea that `@llogiq` gave me while reviewing another PR.
There are some limitations, in particular for the "output". Initially I wanted to make it possible for directly lint against the whole attribute if its parts were all duplicated, but then I realized that the output would be chaotic if the duplicates were coming from different attributes, so I preferred to go to the simplest way and simply emit a warning for each entry. Not the best, but makes the implementation much easier.
Another limitation is that `cfg_attr` would be a bit more tricky to implement because we need to check if two `cfg` sets are exactly the same. I added a FIXME and will likely come back to it later.
And finally, I updated the `cargo dev update_lints` command because the generated `tests/ui/rename.rs` file was emitting the `duplicated_attributes` lint, so I allowed this lint inside it to prevent it from working.
changelog: Add new `duplicated_attributes` lint
add documentation to the `span_lint_hir` functions
As far as I could tell, these weren't documented anywhere, and since this is sometimes needed over `span_lint` for `#[allow]` attrs to work, I thought I would add a little bit of documentation.
When I started with clippy development, I also had no idea what these functions were for.
changelog: none
[`mut_mut`]: Fix duplicate diags
Relates to #12379
The `mut_mut` lint produced two diagnostics for each `mut mut` pattern in `ty` inside `block`s because `MutVisitor::visit_ty` was called from `MutMut::check_ty` and `MutMut::check_block` independently. This PR fixes the issue.
---
changelog: [`mut_mut`]: Fix duplicate diagnostics
New lint `const_is_empty`
This lint detects calls to `.is_empty()` on an entity initialized from a string literal and flag them as suspicious. To avoid triggering on macros called from generated code, it checks that the `.is_empty()` receiver, the call itself and the initialization come from the same context.
Fixes#12307
changelog: [`const_is_empty`]: new lint
fix [`missing_docs_in_private_items`] on some proc macros
fixes: #12197
---
changelog: [`missing_docs_in_private_items`] support manually search for docs as fallback method
Have the lint trigger even if `Self` has generic lifetime parameters.
```rs
impl<'a> Foo<'a> {
type Item = Foo<'a>; // Can be replaced with Self
fn new() -> Self {
Foo { // No lifetime, but they are inferred to be that of Self
// Can be replaced as well
...
}
}
// Don't replace `Foo<'b>`, the lifetime is different!
fn eq<'b>(self, other: Foo<'b>) -> bool {
..
}
```
Fixes#12381
Remove double expr lint
Related to #12379.
Previously the code manually checked nested binop exprs in unary exprs, but those were caught anyway by `check_expr`. Removed that code path, the path is used in the tests.
---
changelog: [`nonminimal_bool`] Remove duplicate output on nested Binops in Unary exprs.
Add `assigning_clones` lint
This PR is a "revival" of https://github.com/rust-lang/rust-clippy/pull/10613 (with `@kpreid's` permission).
I tried to resolve most of the unresolved things from the mentioned PR:
1) The lint now checks properly if we indeed call the functions `std::clone::Clone::clone` or `std::borrow::ToOwned::to_owned`.
2) It now supports both method and function (UFCS) calls.
3) A heuristic has been added to decide if the lint should apply. It will only apply if the type on which the method is called has a custom implementation of `clone_from/clone_into`. Notably, it will not trigger for types that use `#[derive(Clone)]`.
4) `Deref` handling has been (hopefully) a bit improved, but I'm not sure if it's ideal yet.
I also added a bunch of additional tests.
There are a few things that could be improved, but shouldn't be blockers:
1) When the right-hand side is a function call, it is transformed into e.g. `::std::clone::Clone::clone(...)`. It would be nice to either auto-import the `Clone` trait or use the original path and modify it (e.g. `clone::Clone::clone` -> `clone::Clone::clone_from`). I don't know how to modify the `QPath` to do that though.
2) The lint currently does not trigger when the left-hand side is a local variable without an initializer. This is overly conservative, since it could trigger when the variable has no initializer, but it has been already initialized at the moment of the function call, e.g.
```rust
let mut a;
...
a = Foo;
...
a = b.clone(); // Here the lint should trigger, but currently doesn't
```
These cases probably won't be super common, but it would be nice to make the lint more precise. I'm not sure how to do that though, I'd need access to some dataflow analytics or something like that.
changelog: new lint [`assigning_clones`]
[`misrefactored_assign_op`]: Fix duplicate diagnostics
Relate to #12379
The following diagnostics appear twice
```
--> tests/ui/assign_ops2.rs:26:5
|
LL | a *= a * a;
| ^^^^^^^^^^
|
help: did you mean `a = a * a` or `a = a * a * a`? Consider replacing it with
```
because `a` (lhs) appears in both left operand and right operand in the right hand side.
This PR fixes the issue so that if a diagnostic is created for an operand, the check of the other operand will be skipped. It's fine because the result is always the same in the affected operators.
changelog: [`misrefactored_assign_op`]: Fix duplicate diagnostics
Don't emit "missing backticks" lint if the element is wrapped in `<code>` HTML tags
Fixes#9473.
changelog: Don't emit "missing backticks" lint if the element is wrapped in `<code>` HTML tags
[`identity_op`]: Fix duplicate diagnostics
Relates to #12379
In the `identity_op` lint, the following diagnostic was emitted two times
```
--> tests/ui/identity_op.rs:156:5
|
LL | 1 * 1;
| ^^^^^ help: consider reducing it to: `1`
|
```
because both of the left operand and the right operand are the identity element of the multiplication.
This PR fixes the issue so that if a diagnostic is created for an operand, the check of the other operand will be skipped. It's fine because the result is always the same in the affected operators.
---
changelog: [`identity_op`]: Fix duplicate diagnostics
Check for try blocks in `question_mark` more consistently
Fixes#12337
I split this PR up into two commits since this moves a method out of an `impl`, which makes for a pretty bad diff (the `&self` parameter is now unused, and there isn't a reason for that function to be part of the `impl` now).
The first commit is the actual relevant change and the 2nd commit just moves stuff (github's "hide whitespace" makes the diff easier to look at)
------------
Now for the actual issue:
`?` within `try {}` blocks desugars to a `break` to the block, rather than a `return`, so that changes behavior in those cases.
The lint has multiple patterns to look for and in *some* of them it already does correctly check whether we're in a try block, but this isn't done for all of its patterns.
We could add another `self.inside_try_block()` check to the function that looks for `let-else-return`, but I chose to actually just move those checks out and instead have them in `LintPass::check_{stmt,expr}`. This has the advantage that we can't (easily) accidentally forget to add that check in new patterns that might be added in the future.
(There's also a bit of a subtle interaction between two lints, where `question_mark`'s LintPass calls into `manual_let_else`, so I added a check to make sure we don't avoid linting for something that doesn't have anything to do with `?`)
changelog: [`question_mark`]: avoid linting on try blocks in more cases
fix [`derive_partial_eq_without_eq`] FP on trait projection
fixes: #9413#9319
---
changelog: fix [`derive_partial_eq_without_eq`] FP on trait projection
Well, this is awkward, it works but I don't understand why, why `clippy_utils::ty::implements_trait` couldn't detects the existance of `Eq` trait, even thought it's obviously present in the derive attribute.
Pointers cannot be converted to integers at compile time
Fix#12402
changelog: [`transmutes_expressible_as_ptr_casts`]: do not suggest invalid const casts
Dedup std_instead_of_core by using first segment span for uniqueness
Relates to #12379.
Instead of checking that the paths have an identical span, it checks that the relevant `std` part of the path segment's span is identical. Added a multiline test, because my first implementation was worse and failed that, then I realized that you could grab the span off the first_segment `Ident`.
I did find another bug that isn't addressed by this, and that exists on master as well.
The path:
```Rust
use std::{io::Write, fmt::Display};
```
Will get fixed into:
```Rust
use core::{io::Write, fmt::Display};
```
Which doesn't compile since `io::Write` isn't in `core`, if any of those paths are present in `core` it'll do the replace and cause a miscompilation. Do you think I should file a separate bug for that? Since `rustfmt` default splits those up it isn't that big of a deal.
Rustfmt:
```Rust
// Pre
use std::{io::Write, fmt::Display};
// Post
use std::fmt::Display;
use std::io::Write;
```
---
changelog: [`std_instead_of_core`]: Fix duplicated output on multiple imports
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
`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>
Count stashed errors again
Stashed diagnostics are such a pain. Their "might be emitted, might not" semantics messes with lots of things.
#120828 and #121206 made some big changes to how they work, improving some things, but still leaving some problems, as seen by the issues caused by #121206. This PR aims to fix all of them by restricting them in a way that eliminates the "might be emitted, might not" semantics while still allowing 98% of their benefit. Details in the individual commit logs.
r? `@oli-obk`
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
Stashed errors used to be counted as errors, but could then be
cancelled, leading to `ErrorGuaranteed` soundness holes. #120828 changed
that, closing the soundness hole. But it introduced other difficulties
because you sometimes have to account for pending stashed errors when
making decisions about whether errors have occured/will occur and it's
easy to overlook these.
This commit aims for a middle ground.
- Stashed errors (not warnings) are counted immediately as emitted
errors, avoiding the possibility of forgetting to consider them.
- The ability to cancel (or downgrade) stashed errors is eliminated, by
disallowing the use of `steal_diagnostic` with errors, and introducing
the more restrictive methods `try_steal_{modify,replace}_and_emit_err`
that can be used instead.
Other things:
- `DiagnosticBuilder::stash` and `DiagCtxt::stash_diagnostic` now both
return `Option<ErrorGuaranteed>`, which enables the removal of two
`delayed_bug` calls and one `Ty::new_error_with_message` call. This is
possible because we store error guarantees in
`DiagCtxt::stashed_diagnostics`.
- Storing the guarantees also saves us having to maintain a counter.
- Calls to the `stashed_err_count` method are no longer necessary
alongside calls to `has_errors`, which is a nice simplification, and
eliminates two more `span_delayed_bug` calls and one FIXME comment.
- Tests are added for three of the four fixed PRs mentioned below.
- `issue-121108.rs`'s output improved slightly, omitting a non-useful
error message.
Fixes#121451.
Fixes#121477.
Fixes#121504.
Fixes#121508.
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
[`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
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?
