Disable `indexing_slicing` for custom Index impls
Fixes https://github.com/rust-lang/rust-clippy/issues/11525
Disables `indexing_slicing` for custom Index impls, specifically any implementations that also do not have a `get` method anywhere along the deref chain (so, for example, it still lints on Vec, which has its `get` method as part of the deref chain).
Thanks `@y21` for pointing me in the right direction with a couple of handy util functions for deref chain and inherent methods, saved a headache there!
changelog: FP: Disable `indexing_slicing` for custom Index impls
Rename HIR `TypeBinding` to `AssocItemConstraint` and related cleanup
Rename `hir::TypeBinding` and `ast::AssocConstraint` to `AssocItemConstraint` and update all items and locals using the old terminology.
Motivation: The terminology *type binding* is extremely outdated. "Type bindings" not only include constraints on associated *types* but also on associated *constants* (feature `associated_const_equality`) and on RPITITs of associated *functions* (feature `return_type_notation`). Hence the word *item* in the new name. Furthermore, the word *binding* commonly refers to a mapping from a binder/identifier to a "value" for some definition of "value". Its use in "type binding" made sense when equality constraints (e.g., `AssocTy = Ty`) were the only kind of associated item constraint. Nowadays however, we also have *associated type bounds* (e.g., `AssocTy: Bound`) for which the term *binding* doesn't make sense.
---
Old terminology (HIR, rustdoc):
```
`TypeBinding`: (associated) type binding
├── `Constraint`: associated type bound
└── `Equality`: (associated) equality constraint (?)
├── `Ty`: (associated) type binding
└── `Const`: associated const equality (constraint)
```
Old terminology (AST, abbrev.):
```
`AssocConstraint`
├── `Bound`
└── `Equality`
├── `Ty`
└── `Const`
```
New terminology (AST, HIR, rustdoc):
```
`AssocItemConstraint`: associated item constraint
├── `Bound`: associated type bound
└── `Equality`: associated item equality constraint OR associated item binding (for short)
├── `Ty`: associated type equality constraint OR associated type binding (for short)
└── `Const`: associated const equality constraint OR associated const binding (for short)
```
r? compiler-errors
fix: let non_canonical_impls skip proc marco
Fixed#12788
Although the issue only mentions `NON_CANONICAL_CLONE_IMPL`, this fix will also affect `NON_CANONICAL_PARTIAL_ORD_IMPL` because I saw
> Because of these unforeseeable or unstable behaviors, macro expansion should often not be regarded as a part of the stable API.
on Clippy Documentation and these two lints are similar, so I think it might be good, not sure if it's right or not.
---
changelog: `NON_CANONICAL_CLONE_IMPL`, `NON_CANONICAL_PARTIAL_ORD_IMPL` will skip proc marco now
Make `body_owned_by` return the `Body` instead of just the `BodyId`
fixes#125677
Almost all `body_owned_by` callers immediately called `body`, too, so just return `Body` directly.
This makes the inline-const query feeding more robust, as all calls to `body_owned_by` will now yield a body for inline consts, too.
I have not yet figured out a good way to make `tcx.hir().body()` return an inline-const body, but that can be done as a follow-up
don't inhibit random field reordering on repr(packed(1))
`inhibit_struct_field_reordering_opt` being false means we exclude this type from random field shuffling. However, `packed(1)` types can still be shuffled! The logic was added in https://github.com/rust-lang/rust/pull/48528 since it's pointless to reorder fields in packed(1) types (there's no padding that could be saved) -- but that shouldn't inhibit `-Zrandomize-layout` (which did not exist at the time).
We could add an optimization elsewhere to not bother sorting the fields for `repr(packed)` types, but I don't think that's worth the effort.
This *does* change the behavior in that we may now reorder fields of `packed(1)` structs (e.g. if there are niches, we'll try to move them to the start/end, according to `NicheBias`). We were always allowed to do that but so far we didn't. Quoting the [reference](https://doc.rust-lang.org/reference/type-layout.html):
> On their own, align and packed do not provide guarantees about the order of fields in the layout of a struct or the layout of an enum variant, although they may be combined with representations (such as C) which do provide such guarantees.
[`many_single_char_names`]: Deduplicate diagnostics
Relates to #12379
Fix `many_single_char_names` lint so that it doesn't emit diagnostics when the current level of the scope doesn't contain any single character name.
```rust
let (a, b, c, d): (i32, i32, i32, i32);
match 1 {
1 => (),
e => {},
}
```
produced the exact same MANY_SINGLE_CHAR_NAMES diagnostic at each of the Arm `e => {}` and the Block `{}`.
---
changelog: [`many_single_char_names`]: Fix duplicate diagnostics
Fix `unnecessary_to_owned` interaction with macro expansion
fixes#12821
In the case of an unnecessary `.iter().cloned()`, the lint `unnecessary_to_owned` might suggest to remove the `&` from references without checking if such references are inside a macro expansion. This can lead to unexpected behavior or even broken code if the lint suggestion is applied blindly. See issue #12821 for an example.
This PR checks if such references are inside macro expansions and skips this part of the lint suggestion in these cases.
changelog: [`unnecessary_to_owned`]: Don't suggest to remove `&` inside macro expansion
`significant_drop_in_scrutinee`: Trigger lint only if lifetime allows early significant drop
I want to argue that the following code snippet should not trigger `significant_drop_in_scrutinee` (https://github.com/rust-lang/rust-clippy/issues/8987). The iterator holds a reference to the locked data, so it is expected that the mutex guard must be alive until the entire loop is finished.
```rust
use std::sync::Mutex;
fn main() {
let mutex_vec = Mutex::new(vec![1, 2, 3]);
for number in mutex_vec.lock().unwrap().iter() {
dbg!(number);
}
}
```
However, the lint should be triggered when we clone the vector. In this case, the iterator does not hold any reference to the locked data.
```diff
- for number in mutex_vec.lock().unwrap().iter() {
+ for number in mutex_vec.lock().unwrap().clone().iter() {
```
Unfortunately, it seems that regions on the types of local variables are mostly erased (`ReErased`) in the late lint pass. So it is hard to tell if the final expression has a lifetime relevant to the value with a significant drop.
In this PR, I try to make a best-effort guess based on the function signatures. To avoid false positives, no lint is issued if the result is uncertain. I'm not sure if this is acceptable or not, so any comments are welcome.
Fixes https://github.com/rust-lang/rust-clippy/issues/8987
changelog: [`significant_drop_in_scrutinee`]: Trigger lint only if lifetime allows early significant drop.
r? `@flip1995`
fulfill expectations in `check_partial_eq_without_eq`
This is a followup to #12804, fixing a similar issue for `derive_partial_eq_without_eq` by using `span_lint_hir_and_then` instead of `span_lint_and_sugg`.
Additionally tests for both `#[allow(clippy::derive_partial_eq_without_eq)]` and `#[expect(clippy::derive_partial_eq_without_eq)]` are added.
changelog:[`derive_partial_eq_without_eq`]: fulfill expectations
The `restriction` group contains many lints which are not about
necessarily “bad” things, but style choices — perhaps even style choices
which contradict conventional Rust style — or are otherwise very
situational. This results in silly wording like “Why is this bad?
It isn't, but ...”, which I’ve seen confuse a newcomer at least once.
To improve this situation, this commit replaces the “Why is this bad?”
section heading with “Why restrict this?”, for most, but not all,
restriction lints. I left alone the ones whose placement in the
restriction group is more incidental.
In order to make this make sense, I had to remove the “It isn't, but”
texts from the contents of the sections. Sometimes further changes
were needed, or there were obvious fixes to make, and I went ahead
and made those changes without attempting to split them into another
commit, even though many of them are not strictly necessary for the
“Why restrict this?” project.
* Remove incorrect claim that “wrappers around it are the conventional
way to define an uninhabited type”.
* Discuss why one would use `!`, a newtype struct, or keep the enum.
* Add links to relevant documentation.
fulfill expectations in `check_unsafe_derive_deserialize`
The utility function `clippy_utils::fulfill_or_allowed` is not used because using it would require to move the check for allowed after the check iterating over all inherent impls of the type, doing possibly unnecessary work.
Instead, `is_lint_allowed` is called as before, but additionally, once certain that the lint should be emitted, `span_lint_hir_and_then` is called instead of `span_lint_and_help` to also fulfill expectations.
Note: as this is my first contribution, please feel free to nitpick or request changes. I am happy to adjust the implementation.
fixes: #12802
changelog: fulfill expectations in [`unsafe_derive_deserialize`]
Add new lint `while_float`
This PR adds a nursery lint that checks for while loops comparing floating point values.
changelog:
```
changelog: [`while_float`]: Checks for while loops comparing floating point values.
```
Fixes#758
chore: Remove repeated words (extension of #124924)
When I saw #124924 I thought "Hey, I'm sure that there are far more than just two typos of this nature in the codebase". So here's some more typo-fixing.
Some found with regex, some found with a spellchecker. Every single one manually reviewed by me (along with hundreds of false negatives by the tools)
This avoids event spans that would otherwise cause crashes, since an
End's span covers the range of the tag (which will be earlier than the
line break within the tag).
Rename Unsafe to Safety
Alternative to #124455, which is to just have one Safety enum to use everywhere, this opens the posibility of adding `ast::Safety::Safe` that's useful for unsafe extern blocks.
This leaves us today with:
```rust
enum ast::Safety {
Unsafe(Span),
Default,
// Safe (going to be added for unsafe extern blocks)
}
enum hir::Safety {
Unsafe,
Safe,
}
```
We would convert from `ast::Safety::Default` into the right Safety level according the context.
Add configuration option for ignoring `panic!()` in tests
```
changelog: [`panic`]: Now can be disabled in tests with the `allow-panic-in-tests` option
```
I often find myself using `panic!(…)` in tests a lot, where I often do something like:
```rust
match enam {
Enam::A => …,
Enam::B => …,
_ => panic!("This should not happen at all."),
}
```
I think this patch should go nicely with already existing `allow-unwrap-in-tests` and `allow-expect-in-tests`.
The utility function `clippy_utils::fulfill_or_allowed` is not used because
using it would require to move the check for allowed after the check
iterating over all inherent impls of the type, doing possibly
unnecessary work.
Instead, `is_lint_allowed` is called as before, but additionally, once
certain that the lint should be emitted, `span_lint_hir_and_then` is called
instead of `span_lint_and_help` to also fulfill expectations.
fixes: #12802
changelog: fulfill expectations in `check_unsafe_derive_deserialize`
less aggressive needless_borrows_for_generic_args
Current implementation looks for significant drops, that can change the behavior, but that's not enough - value might not have a `Drop` itself but one of its children might have it.
A good example is passing a reference to `PathBuf` to `std::fs::File::open`. There's no benefits to pass `PathBuf` by value, but since `clippy` can't see `Drop` on `Vec` several layers down it complains forcing pass by value and making it impossible to use the same name later.
New implementation only looks at copy values or values created in place so existing variable will never be moved but things that take a string reference created and value is created inplace `&"".to_owned()` will make it to suggest to use `"".to_owned()` still.
Fixes https://github.com/rust-lang/rust-clippy/issues/12454
changelog: [`needless_borrows_for_generic_args`]: avoid moving variables
`assigning_clones`: move to `pedantic` so it is allow by default
In a nutshell, the `assigning_clones` lint suggests to make your code less readable for a small performance gain. See #12778 for more motivation.
fixes#12778
changelog: [`assigning_clones`]: move to the `pedantic` group
improve [`match_same_arms`] messages, enable rustfix test
closes: #9251
don't worry about the commit size, most of them are generated
---
changelog: improve [`match_same_arms`] lint messages
`significant_drop_in_scrutinee`: Fix false positives due to false drops of place expressions
Place expressions do not really create temporaries, so they will not create significant drops. For example, the following code snippet is quite good (#8963):
```rust
fn main() {
let x = std::sync::Mutex::new(vec![1, 2, 3]);
let x_guard = x.lock().unwrap();
match x_guard[0] {
1 => println!("1!"),
x => println!("{x}"),
}
drop(x_guard); // Some "usage"
}
```
Also, the previous logic thinks that references like `&MutexGuard<_>`/`Ref<'_, MutexGuard<'_, _>>` have significant drops, which is simply not true, so it is fixed together in this PR.
Fixes https://github.com/rust-lang/rust-clippy/issues/8963
Fixes https://github.com/rust-lang/rust-clippy/issues/9072
changelog: [`significant_drop_in_scrutinee`]: Fix false positives due to false drops of place expressions.
r? `@blyxyas`
add new lint that disallow renaming parameters in trait functions
fixes: #11443fixes: #486
changelog: add new lint [`renamed_function_params`]
Note that the lint name is not final, because I have a bad reputation in naming things, and I don't trust myself.
Lint direct priority conflicts in `[workspace.lints]`
Partially addresses #12729
This still doesn't do any workspace resolution stuff, so it will not catch any virtual workspaces or conflicts from inherited definitions. But while we're parsing the `Cargo.toml` we might as well check the workspace definitions if we find them
changelog: none
Handle `rustc_on_unimplemented` in duplicated_attributes
```rust
#[rustc_on_unimplemented(
on(
_Self = "&str",
label = "`a"
),
on(
_Self = "alloc::string::String",
label = "a"
),
)]
```
The lint treats this as a repetition because `rustc_on_unimplemented:🔛:label` appears twice, but that's ok.
Fixes#12619
changelog: [`duplicated_attributes`]: fix handling of `rustc_on_unimplemented`
Add new lint `doc_lazy_continuation`
changelog: [`doc_lazy_continuation`]: add lint that warns on so-called "lazy paragraph continuations"
This is a follow-up for https://github.com/rust-lang/rust/pull/121659, since most cases of unintended block quotes are lazy continuations. The lint is designed to be more generally useful than that, though, because it will also catch unintended list items and unintended block quotes that didn't coincidentally hit a pulldown-cmark bug.
The second commit is the result of running `cargo dev dogfood --fix`, and manually fixing anything that seems wrong. NOTE: this lint's suggestions should never change the parser's interpretation of the markdown, but in many cases, it seems that doc comments in clippy were written without regard for this feature of Markdown (which, I suppose, is why this lint should exist).
Ignore `_to_string` lints in code `from_expansion`
Includes the `string_to_string` and `str_to_string` lints.
changelog: [`str_to_string`]: Ignore code from expansion
changelog: [`string_to_string`]: Ignore code from expansion
fix: use hir_with_context to produce correct snippets for assigning_clones
The `assigning_clones` lint is producing wrong output when the assignment is a macro call.
Since Applicability level `Unspecified` will never be changed inside `hir_with_applicability`, so it is safe here to replace `hir_with_applicability` with `hir_with_context` to generate snippets of the macro call instead of the expansion.
fixes#12776
changelog: [`assigning_clones`]: use `hir_with_context` to produce correct snippets
fix false positive in Issue/12098 because lack of consideration of mutable caller
fixes [Issue#12098](https://github.com/rust-lang/rust-clippy/issues/12098)
In issue#12098, the former code doesn't consider the caller for clone is mutable, and suggests to delete clone function.
In this change, we first get the inner caller requests for clone,
and if it's immutable, the following code will suggest deleting clone.
If it's mutable, the loop will check whether a borrow check violation exists,
if exists, the lint should not execute, and the function will directly return;
otherwise, the following code will handle this.
changelog: [`clippy::unnecessary_to_owned`]: fix false positive
Simplify `use crate::rustc_foo::bar` occurrences.
They can just be written as `use rustc_foo::bar`, which is far more standard. (I didn't even know that a `crate::` prefix was valid.)
r? ``@eholk``
Remove braces when fixing a nested use tree into a single item
[Back in 2019](https://github.com/rust-lang/rust/pull/56645) I added rustfix support for the `unused_imports` lint, to automatically remove them when running `cargo fix`. For the most part this worked great, but when removing all but one childs of a nested use tree it turned `use foo::{Unused, Used}` into `use foo::{Used}`. This is slightly annoying, because it then requires you to run `rustfmt` to get `use foo::Used`.
This PR automatically removes braces and the surrouding whitespace when all but one child of a nested use tree are unused. To get it done I had to add the span of the nested use tree to the AST, and refactor a bit the code I wrote back then.
A thing I noticed is, there doesn't seem to be any `//@ run-rustfix` test for fixing the `unused_imports` lint. I created a test in `tests/suggestions` (is that the right directory?) that for now tests just what I added in the PR. I can followup in a separate PR to add more tests for fixing `unused_lints`.
This PR is best reviewed commit-by-commit.
This is a follow-up for https://github.com/rust-lang/rust/pull/121659,
since most cases of unintended block quotes are lazy continuations.
The lint is designed to be more generally useful than that, though,
because it will also catch unintended list items and unintended
block quotes that didn't coincidentally hit a pulldown-cmark bug.
Allow more attributes in `clippy::useless_attribute`
Fixes#12753Fixes#4467Fixes#11595Fixes#10878
changelog: [`useless_attribute`]: Attributes allowed on `use` items now include `ambiguous_glob_exports`, `hidden_glob_reexports`, `dead_code`, `unused_braces`, and `clippy::disallowed_types`.
Current implementation looks for significant drops, that can change the
behavior, but that's not enough - value might not have a Drop itself but
one of its children might have it.
A good example is passing a reference to `PathBuf` to `std::fs::File::open`.
There's no benefits to pass `PathBuf` by value, but since clippy can't
see `Drop` on `Vec` several layers down it complains forcing pass by
value and making it impossible to use the same name later.
New implementation only looks at copy values or values created inplace
so existing variable will never be moved but things that take a string
reference created and value is created inplace `&"".to_owned()` will
make it to suggest to use `"".to_owned()` still.
Fixes https://github.com/rust-lang/rust-clippy/issues/12454
Fix `FormatArgs` storage when `-Zthreads` > 1
Fixes#11886
The initial way I thought of was a little gross so I never opened a PR for it, I thought of a nicer way today that no longer involves any `thread_local`s or `static`s
`rustc_data_strucutres::sync::{Lrc, OnceLock}` implement `DynSend` + `DynSync` so we can pass them to the lint passes that need the storage
changelog: none
r? `@flip1995`
Suggest collapsing nested or patterns if the MSRV allows it
Nested `or` patterns have been stable since 1.53, so we should be able to suggest `Some(1 | 2)` if the MSRV isn't set below that.
This change adds an msrv check and also moves it to `matches/mod.rs`, because it's also needed by `redundant_guards`.
changelog: [`collapsible_match`]: suggest collapsing nested or patterns if the MSRV allows it
Changelog for Clippy 1.78 🪄
Roses and Violets have colors,
Red and Blue are the two,
I'm getting to the end of my masters,
what a cool goal to pursue
---
### The cat of this release is: *Shadow* submitted by `@benwh1:`
<img height=500 src="https://github.com/rust-lang/rust-clippy/assets/32911992/c56af314-4644-482a-a08e-f32f4c7d7b22" alt="The cats of this Clippy release" />
Cats for the next release can be nominated in the comments :D
---
changelog: none
fix suggestion error for [`manual_is_ascii_check`] with missing type
fixes: #11324fixes: #11776
changelog: improve [`manual_is_ascii_check`] to suggest labeling type in closure, fix FP with type generics, and improve linting on ref expressions.
suppress `readonly_write_lock` for underscore-prefixed bindings
Fixes#12733
Unsure if there's a better way to prevent this kind of false positive but this is the one that made most sense to me.
In my experience, prefixing bindings with an underscore is the usual way to name variables that aren't used and that exist purely for executing drop code at the end of the scope.
-------
changelog: suppress [`readonly_write_lock`] for underscore-prefixed bindings
clippy::single_match(_else) may be machine applicable
```
changelog: [`single_match`]: make the lint machine-applicable
changelog: [`single_match_else`]: make the lint machine-applicable
```
---
The lint doesn't use placeholders. I've tried it on my codebases, and all instances of it applied without problems.
check if closure as method arg has read access in [`collection_is_never_read`]
fixes: #11783
---
changelog: fix [`collection_is_never_read`] misfires when use `retain` for iteration
configurably allow `useless_vec` in tests
This adds a `àllow-useless-vec-in-test` configuration which, when set to `true` will allow the `useless_vec` lint in `#[test]` functions and code within `#[cfg(test)]`. It also moves a `is_in_test` helper to `clippy_utils`.
---
changelog: configurably allow [`useless_vec`] in test code
This adds a `àllow-useless-vec-in-test` configuration which, when set
to `true` will allow the `useless_vec` lint in `#[test]` functions and
code within `#[cfg(test)]`. It also moves a `is_in_test` helper to
`clippy_utils`.
fix [`large_stack_arrays`] linting in `vec` macro
fixes: #12586
this PR also adds a wrapper function `matching_root_macro_call` to `clippy_utils::macros`, considering how often that same pattern appears in the codebase.
(I'm always very indecisive towards naming, so, if anyone have better idea of how that function should be named, feel free to suggest it)
---
changelog: fix [`large_stack_arrays`] linting in `vec` macro; add `matching_root_macro_call` to clippy_utils
[`non_canonical_partial_ord_impl`]: Fix emitting warnings which conflict with `needless_return`
fixes#12683
---
changelog: fix [`non_canonical_partial_ord_impl`] emitting warnings which conflict with `needless_return`
reduce `single_char_pattern` to only lint on ascii chars
This should mostly fix the `single_char_pattern` lint, because with a single byte, the optimizer will usually see through the char-to-string-expansion and single loop iteration. This fixes#11675 and #8111.
Update: As per the meeting on November 28th, 2023, we voted to also downgrade the lint to pedantic.
---
changelog: downgrade [`single_char_pattern`] to `pedantic`
Rework interior mutability detection
Replaces the existing interior mutability detection, the two main changes being
- It now follows references/pointers e.g. `struct S(&Cell)`
- `mutable_key_type` ignores pointers as it did before
- The `ignore_interior_mutability` config now applies to types containing the ignored type, e.g. `http::HeaderName`
Fixes https://github.com/rust-lang/rust-clippy/issues/7752
Fixes https://github.com/rust-lang/rust-clippy/issues/9776
Fixes https://github.com/rust-lang/rust-clippy/issues/9801
changelog: [`mutable_key_type`], [`declare_interior_mutable_const`]: now considers types that have references to interior mutable types as interior mutable
This commit introduces a check to ensure that the lint won't trigger when the initializer is
unreachable, such as:
```
thread_local! {
static STATE: Cell<usize> = panic!();
}
```
This is achieved by looking at the unpeeled initializer expression and ensuring that the parent
macro is not `panic!()`, `todo!()`, `unreachable!()`, `unimplemented!()`.
fixes#12637
changelog: [`threadlocal_initializer_can_be_made_const`] will no longer trigger on `unreachable` macros.
Emit the `needless_pass_by_ref_mut` lint on `self` arguments as well
Fixes https://github.com/rust-lang/rust-clippy/issues/12589.
Fixes https://github.com/rust-lang/rust-clippy/issues/9591.
The first commit fixes a bug I uncovered while working on this: sometimes, the mutable borrow "event" happens before the alias one, which makes some argument detected as not used mutably even if they are. The fix was simply to fill the map with the aliases afterwards.
The second commit removes the restriction to not run `self` argument for the `needless_pass_by_ref_mut` lint.
changelog: emit the `needless_pass_by_ref_mut` lint on `self` arguments as well
r? `@Manishearth`
Currently `SourceMap` is constructed slightly later than
`SessionGlobals`, and inserted. This commit changes things so they are
done at the same time.
Benefits:
- `SessionGlobals::source_map` changes from
`Lock<Option<Lrc<SourceMap>>>` to `Option<Lrc<SourceMap>>`. It's still
optional, but mutability isn't required because it's initialized at
construction.
- `set_source_map` is removed, simplifying `run_compiler`, which is
good because that's a critical function and it's nice to make it
simpler.
This requires moving things around a bit, so the necessary inputs are
available when `SessionGlobals` is created, in particular the `loader`
and `hash_kind`, which are no longer computed by `build_session`. These
inputs are captured by the new `SourceMapInputs` type, which is threaded
through various places.
Fix pretty HIR for anon consts in diagnostics
This removes the `NoAnn` printer which skips over nested bodies altogether, which is confusing, and requires users of `{ty|qpath|pat}_to_string` to pass in `&tcx` which now impleemnts `hir_pretty::PpAnn`.
There's one case where this "regresses" by actually printing out the body of the anon const -- we could suppress that, but I don't expect people to actually get anon consts like that unless they're fuzzing, tbh.
r? estebank
Cleanup: Rename `ModSep` to `PathSep`
`::` is usually referred to as the *path separator* (citation needed).
The existing name `ModSep` for *module separator* is a bit misleading since it in fact separates the segments of arbitrary path segments, not only ones resolving to modules. Let me just give a shout-out to associated items (`T::Assoc`, `<Ty as Trait>::function`) and enum variants (`Option::None`).
Motivation: Reduce friction for new contributors, prevent potential confusion.
cc `@petrochenkov`
r? nnethercote or compiler
fix: incorrect suggestions when `.then` and `.then_some` is used
fixes#11910
In the current implementation of `search_is_some`, if a `.is_none` call is followed by a `.then` or `.then_some` call, the generated `!` will incorrectly negate the values returned by the `then` and `.then_some` calls. To fix this, we need to add parentheses to the generated suggestions when appropriate.
changelog: [`search_is_some`]: add parenthesis to suggestions when appropriate
[`module_name_repetition`] Recognize common prepositions
Fixes#12544
changelog: [`module_name_repetition`]: don't report an item name if it consists only of a prefix from `allowed-prefixes` list and a module name (e.g. `AsFoo` in module `foo`). Prefixes allowed by default: [`to`, `from`, `into`, `as`, `try_into`, `try_from`]
Use `check_attributes` in doc lints
Ensures we catch all the places that doc comments could occur, found one that we were currently missing - docs on `extern` items
changelog: none
Fixes#12544.
- don't report an item name if it consists only of a prefix from `allowed-prefixes` list and a module name (e.g. `AsFoo` in module `foo`).
- configured by `allowed-prefixes` config entry
- prefixes allowed by default: [`to`, `from`, `into`, `as`, `try_into`, `try_from`]
- update docs
Correct parentheses for [`needless_borrow`] suggestion
This fixes#12268
Clippy no longer adds unnecessary parentheses in suggestions when the expression is part of a tuple.
---
changelog: Fix [`needless_borrow`] unnecessary parentheses in suggestion.
Implement minimal, internal-only pattern types in the type system
rebase of https://github.com/rust-lang/rust/pull/107606
You can create pattern types with `std::pat::pattern_type!(ty is pat)`. The feature is incomplete and will panic on you if you use any pattern other than integral range patterns. The only way to create or deconstruct a pattern type is via `transmute`.
This PR's implementation differs from the MCP's text. Specifically
> This means you could implement different traits for different pattern types with the same base type. Thus, we just forbid implementing any traits for pattern types.
is violated in this PR. The reason is that we do need impls after all in order to make them usable as fields. constants of type `std::time::Nanoseconds` struct are used in patterns, so the type must be structural-eq, which it only can be if you derive several traits on it. It doesn't need to be structural-eq recursively, so we can just manually implement the relevant traits on the pattern type and use the pattern type as a private field.
Waiting on:
* [x] move all unrelated commits into their own PRs.
* [x] fix niche computation (see 2db07f94f44f078daffe5823680d07d4fded883f)
* [x] add lots more tests
* [x] T-types MCP https://github.com/rust-lang/types-team/issues/126 to finish
* [x] some commit cleanup
* [x] full self-review
* [x] remove 61bd325da19a918cc3e02bbbdce97281a389c648, it's not necessary anymore I think.
* [ ] ~~make sure we never accidentally leak pattern types to user code (add stability checks or feature gate checks and appopriate tests)~~ we don't even do this for the new float primitives
* [x] get approval that [the scope expansion to trait impls](https://rust-lang.zulipchat.com/#narrow/stream/326866-t-types.2Fnominated/topic/Pattern.20types.20types-team.23126/near/427670099) is ok
r? `@BoxyUwU`
Stop exporting `TypeckRootCtxt` and `FnCtxt`.
While they have many convenient APIs, it is better to expose dedicated functions for them
noticed in #122213
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
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.
Fix typo in comment
Thank you for making Clippy better!
We're collecting our changelog from pull request descriptions.
If your PR only includes internal changes, you can just write
`changelog: none`. Otherwise, please write a short comment
explaining your change.
It's also helpful for us that the lint name is put within backticks (`` ` ` ``),
and then encapsulated by square brackets (`[]`), for example:
```
changelog: [`lint_name`]: your change
```
If your PR fixes an issue, you can add `fixes #issue_number` into this
PR description. This way the issue will be automatically closed when
your PR is merged.
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]
- \[ ] Added passing UI tests (including committed `.stderr` file)
- \[ ] `cargo test` passes locally
- \[ ] Executed `cargo dev update_lints`
- \[ ] Added lint documentation
- \[x] Run `cargo dev fmt`
[lint_naming]: https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints
Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.
Delete this line and everything above before opening your PR.
---
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: None
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)
allow [`manual_unwrap_or_default`] in const function
closes: #12568
---
changelog: allow [`manual_unwrap_or_default`] in const function
This is a small fix, I was originally decided to fix it along with `#12568` but there are some problems needs to be addressed (which is why my branch is called `issue12569` 😆 ), so I decide to open a separated PR to fix them one at a time.
Rename `Inherited` -> `TypeckRootCtxt`
`Inherited` is a confusing name. Rename it to `TypeckRootCtxt`.
I don't think this needs a type MCP or anything since it's not nearly as pervasive as `FnCtxt` , for example.
r? `@lcnr` `@oli-obk`
It turns out there is a bit of a circular dependency - I cannot add
anything to `core` because Clippy fails, and I can't actually add
correct Clippy implementations without new implementations from `core`.
Change some of the Clippy stubs from `unimplemented!` to success values
and leave a FIXME in their place to mitigate this.
Fixes <https://github.com/rust-lang/rust/issues/122587>
Change applicability of `assigning_clones` to `Unspecified`
Before we deal with https://github.com/rust-lang/rust-clippy/pull/12473 and the borrow checker errors, I think that it would be better to downgrade this lint, since it can break code.
changelog: Change the applicability of `assigning_clones` to `Unspecified`
r? `@blyxyas`
fix: `suspicious_else_formatting` false positive when else is included …
This PR addresses an issue where invalid suggestions are generated for `if-else` formatting if comments contain the keyword `else`.
The root of the problem is identified [here](95c62ffae9/clippy_lints/src/formatting.rs (L217)). Specifically, when a comment contains the word `else`, the lint mistakenly interprets it as part of an `if-else` clause. This misinterpretation leads to an incorrect splitting of the snippet, resulting in erroneous suggestions.
fixes: #12497
changelog: [`suspicious_else_formatting`]: Fixes invalid suggestions when comments include word else
Rename `hir::Local` into `hir::LetStmt`
Follow-up of #122776.
As discussed on [zulip](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Improve.20naming.20of.20.60ExprKind.3A.3ALet.60.3F).
I made this change into a separate PR because I'm less sure about this change as is. For example, we have `visit_local` and `LocalSource` items. Is it fine to keep these two as is (I supposed it is but I prefer to ask) or not? Having `Node::Local(LetStmt)` makes things more explicit but is it going too far?
r? ```@oli-obk```
`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
Rollup of 8 pull requests
Successful merges:
- #114009 (compiler: allow transmute of ZST arrays with generics)
- #122195 (Note that the caller chooses a type for type param)
- #122651 (Suggest `_` for missing generic arguments in turbofish)
- #122784 (Add `tag_for_variant` query)
- #122839 (Split out `PredicatePolarity` from `ImplPolarity`)
- #122873 (Merge my contributor emails into one using mailmap)
- #122885 (Adjust better spastorino membership to triagebot's adhoc_groups)
- #122888 (add a couple more tests)
r? `@ghost`
`@rustbot` modify labels: rollup
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
Mention `size_hint()` effect in `flat_map_option` lint documentation.
The previous documentation for `flat_map_option` mentioned only readability benefits, but there is also at least one performance benefit: the `size_hint()` upper bound is preserved, whereas `flat_map().size_hint()` is always `(0, None)`.
Program demonstrating this difference:
```rust
fn main() {
let evens = |i| if i % 2 == 0 { Some(i) } else { None };
dbg!(
[1, 2, 3].iter().flat_map(evens).size_hint(),
[1, 2, 3].iter().filter_map(evens).size_hint(),
);
}
```
changelog: [`flat_map_option`]: Mention the benefit to `size_hint()`.
Experimental feature postfix match
This has a basic experimental implementation for the RFC postfix match (rust-lang/rfcs#3295, #121618). [Liaison is](https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Postfix.20Match.20Liaison/near/423301844) ```@scottmcm``` with the lang team's [experimental feature gate process](https://github.com/rust-lang/lang-team/blob/master/src/how_to/experiment.md).
This feature has had an RFC for a while, and there has been discussion on it for a while. It would probably be valuable to see it out in the field rather than continue discussing it. This feature also allows to see how popular postfix expressions like this are for the postfix macros RFC, as those will take more time to implement.
It is entirely implemented in the parser, so it should be relatively easy to remove if needed.
This PR is split in to 5 commits to ease review.
1. The implementation of the feature & gating.
2. Add a MatchKind field, fix uses, fix pretty.
3. Basic rustfmt impl, as rustfmt crashes upon seeing this syntax without a fix.
4. Add new MatchSource to HIR for Clippy & other HIR consumers
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.
Split an item bounds and an item's super predicates
This is the moral equivalent of #107614, but instead for predicates this applies to **item bounds**. This PR splits out the item bounds (i.e. *all* predicates that are assumed to hold for the alias) from the item *super predicates*, which are the subset of item bounds which share the same self type as the alias.
## Why?
Much like #107614, there are places in the compiler where we *only* care about super-predicates, and considering predicates that possibly don't have anything to do with the alias is problematic. This includes things like closure signature inference (which is at its core searching for `Self: Fn(..)` style bounds), but also lints like `#[must_use]`, error reporting for aliases, computing type outlives predicates.
Even in cases where considering all of the `item_bounds` doesn't lead to bugs, unnecessarily considering irrelevant bounds does lead to a regression (#121121) due to doing extra work in the solver.
## Example 1 - Trait Aliases
This is best explored via an example:
```
type TAIT<T> = impl TraitAlias<T>;
trait TraitAlias<T> = A + B where T: C;
```
The item bounds list for `Tait<T>` will include:
* `Tait<T>: A`
* `Tait<T>: B`
* `T: C`
While `item_super_predicates` query will include just the first two predicates.
Side-note: You may wonder why `T: C` is included in the item bounds for `TAIT`? This is because when we elaborate `TraitAlias<T>`, we will also elaborate all the predicates on the trait.
## Example 2 - Associated Type Bounds
```
type TAIT<T> = impl Iterator<Item: A>;
```
The `item_bounds` list for `TAIT<T>` will include:
* `Tait<T>: Iterator`
* `<Tait<T> as Iterator>::Item: A`
But the `item_super_predicates` will just include the first bound, since that's the only bound that is relevant to the *alias* itself.
## So what
This leads to some diagnostics duplication just like #107614, but none of it will be user-facing. We only see it in the UI test suite because we explicitly disable diagnostic deduplication.
Regarding naming, I went with `super_predicates` kind of arbitrarily; this can easily be changed, but I'd consider better names as long as we don't block this PR in perpetuity.
Changelog for Clippy 1.77 🏫
Roses are violets,
Red is blue,
Let's create a world,
Perfect for me and you
---
### The cat of this release is: *Luigi*
<img width=500 src="https://github.com/rust-lang/rust-clippy/assets/17087237/ea13d05c-e5ba-4189-9e16-49bf1b43c468" alt="The cats of this Clippy release" />
The cat for the next release can be voted on: [here](https://forms.gle/57gbrNvXtCUmrHYh6)
The cat for the next next release can be nominated in the comments and will be voted in the next changelog PR (Submission deadline is 2024-03-30 23:59CET)
---
changelog: none
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
Make `assigning_clones` MSRV check more precise
Continuation of #12511
`clone_into` is the only suggestion subject to the 1.63 MSRV requirement, and the lint should still emit other suggestions regardless of the MSRV.
changelog: [assigning_clones]: only apply MSRV check to `clone_into` suggestions.
`assigning_clones` should respect MSRV
Fixes: #12502
This PR fixes the `assigning_clones` lint suggesting to use `clone_from` or `clone_into` on incompatible MSRVs.
`assigning_clones` will suggest using either `clone_from` or `clone_into`, both of which were stabilized in 1.63. If the current MSRV is below 1.63, the lint should not trigger.
changelog: [`assigning_clones`]: don't lint when the MSRV is below 1.63.
Use hir::Node helper methods instead of repeating the same impl multiple times
I wanted to do something entirely different and stumbled upon a bunch of cleanups
[`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
hir: Remove `opt_local_def_id_to_hir_id` and `opt_hir_node_by_def_id`
Also replace a few `hir_node()` calls with `hir_node_by_def_id()`.
Follow up to https://github.com/rust-lang/rust/pull/120943.
[`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
chore: fix some typos
Thank you for making Clippy better!
We're collecting our changelog from pull request descriptions.
If your PR only includes internal changes, you can just write
`changelog: none`. Otherwise, please write a short comment
explaining your change.
It's also helpful for us that the lint name is put within backticks (`` ` ` ``),
and then encapsulated by square brackets (`[]`), for example:
```
changelog: [`lint_name`]: your change
```
If your PR fixes an issue, you can add `fixes #issue_number` into this
PR description. This way the issue will be automatically closed when
your PR is merged.
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]
- \[ ] Added passing UI tests (including committed `.stderr` file)
- \[ ] `cargo test` passes locally
- \[ ] Executed `cargo dev update_lints`
- \[ ] Added lint documentation
- \[x] Run `cargo dev fmt`
[lint_naming]: https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints
Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.
Delete this line and everything above before opening your PR.
---
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog:
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
[`single_match`]: Fix duplicate diagnostics
Relates to #12379
edit two test file
`tests/ui/single_match_else.rs`
`tests/ui/single_match.rs`
those two test file point to the same lint
---
changelog: [`single_match`] Fix duplicate diagnostics
[`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
Add asm goto support to `asm!`
Tracking issue: #119364
This PR implements asm-goto support, using the syntax described in "future possibilities" section of [RFC2873](https://rust-lang.github.io/rfcs/2873-inline-asm.html#asm-goto).
Currently I have only implemented the `label` part, not the `fallthrough` part (i.e. fallthrough is implicit). This doesn't reduce the expressive though, since you can use label-break to get arbitrary control flow or simply set a value and rely on jump threading optimisation to get the desired control flow. I can add that later if deemed necessary.
r? ``@Amanieu``
cc ``@ojeda``
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
Existing names for values of this type are `sess`, `parse_sess`,
`parse_session`, and `ps`. `sess` is particularly annoying because
that's also used for `Session` values, which are often co-located, and
it can be difficult to know which type a value named `sess` refers to.
(That annoyance is the main motivation for this change.) `psess` is nice
and short, which is good for a name used this much.
The commit also renames some `parse_sess_created` values as
`psess_created`.
Add missing header for `manual_is_variant_and`
Noticed this while generating our lint completions failed in rust-analyzer (separate PR from https://github.com/rust-lang/rust-clippy/pull/12415 as I made these via the github interface quickly)
changelog: none
[`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
Add stubs in IR and ABI for `f16` and `f128`
This is the very first step toward the changes in https://github.com/rust-lang/rust/pull/114607 and the [`f16` and `f128` RFC](https://rust-lang.github.io/rfcs/3453-f16-and-f128.html). It adds the types to `rustc_type_ir::FloatTy` and `rustc_abi::Primitive`, and just propagates those out as `unimplemented!` stubs where necessary.
These types do not parse yet so there is no feature gate, and it should be okay to use `unimplemented!`.
The next steps will probably be AST support with parsing and the feature gate.
r? `@compiler-errors`
cc `@Nilstrieb` suggested breaking the PR up in https://github.com/rust-lang/rust/pull/120645#issuecomment-1925900572
`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>
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
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
Fix `nonminimal_bool` lint regression
Fixes#12371.
Fixes#12369.
cc `@RalfJung`
The problem was an invalid condition. Shame on me...
changelog: Fix `nonminimal_bool` lint regression
[`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?
Empty docs
Fixes https://github.com/rust-lang/rust-clippy/issues/9931
changelog: [`empty_doc`]: Detects documentation that is empty.
changelog: Doc comment lints now trigger for struct field and enum variant documentation
When encountering code such as:
```
Box::new(outer::Inner::default())
```
clippy would suggest replacing with `Box::<Inner>::default()`, dropping
the `outer::` segment. This behavior is incorrect and that commit fixes
it.
What it does is it checks the contents of the `Box::new` and, if it is
of the form `A::B::default`, does a text replacement, inserting `A::B`
in the `Box`'s quickfix generic list.
If the source does not match that pattern (including `Vec::from(..)`
or other `T::new()` calls), we then fallback to the original code.
Fixes#11927
Look for `implied_bounds_in_impls` in more positions
With this, we lint `impl Trait` implied bounds in more positions:
- Type alias impl trait
- Associated type position impl trait
- Argument position impl trait
- these are not opaque types, but instead are desugared to `where` clauses, so we need extra logic for finding them (`check_generics`), however the rest of the logic is the same
Before this, we'd only lint RPIT `impl Trait`s.
"Hide whitespaces" and reviewing commits individually might make this easier
changelog: [`implied_bounds_in_impls`]: start linting implied bounds in APIT, ATPIT, TAIT
Merge `single_call_fn` post-crate visitor into lint pass
The `single_call_fn` lint worked by first collecting a list of function definitions in the lint pass, then populating the list of uses for each function in a second visitor after the crate is checked.
Doing another pass through the crate shouldn't be needed, and we should be able to do it in the same lint pass, by looking for path references to functions only and then processing them post-crate.
Other changes:
- `FxHashMap` -> `FxIndexMap` so that we emit warnings in a consistent order, as we see them (making the diff a bit confusing to look at, because warnings were moved around)
- no longer storing a `Vec<Span>` per function: an enum representing "seen once" or "seen more than once" should be enough (only the first element is used later)
- "used here" help is now a note
I also noticed that it lints on trait methods with a default implementation, but not on regular trait methods without a body (because that's what `check_fn` does). I'm not sure if that's useful though, maybe we shouldn't lint trait methods at all? It's not like you can avoid it sometimes (but then again it's a restriction lint). Either way, I left the behavior where it was before so that there are no functional changes made in this PR and it's purely a refactor. I can change it though
changelog: none
