Fix grammer for the Safety documentation check
The original message ("unsafe function's docs miss `# Safety` section") reads quite awkwardly. I've changed it to "unsafe function's docs are missing a `# Safety` section" to have it read better.
```
changelog: [`missing_headers`]: Tweak the grammar in the lint message
```
[`overly_complex_bool_expr`]: Fix trigger wrongly on never type
fixes#12689
---
changelog: fix [`overly_complex_bool_expr`] triggers wrongly on never type
Dedup nonminimal_bool_methods diags
Relates to #12379
Fix `nonminimal_bool` lint so that it doesn't check the same span multiple times.
`NotSimplificationVisitor` was called for each expression from `NonminimalBoolVisitor` whereas `NotSimplificationVisitor` also recursively checked all expressions.
---
changelog: [`nonminimal_bool`]: Fix duplicate diagnostics
The original message ("unsafe function's docs miss `# Safety` section")
reads quite awkwardly. I've changed it to "unsafe function's docs are missing
a `# Safety` section" to have it read better.
Signed-off-by: Paul R. Tagliamonte <paultag@gmail.com>
Currently we have an awkward mix of fallible and infallible functions:
```
new_parser_from_source_str
maybe_new_parser_from_source_str
new_parser_from_file
(maybe_new_parser_from_file) // missing
(new_parser_from_source_file) // missing
maybe_new_parser_from_source_file
source_str_to_stream
maybe_source_file_to_stream
```
We could add the two missing functions, but instead this commit removes
of all the infallible ones and renames the fallible ones leaving us with
these which are all fallible:
```
new_parser_from_source_str
new_parser_from_file
new_parser_from_source_file
source_str_to_stream
source_file_to_stream
```
This requires making `unwrap_or_emit_fatal` public so callers of
formerly infallible functions can still work.
This does make some of the call sites slightly more verbose, but I think
it's worth it for the simpler API. Also, there are two `catch_unwind`
calls and one `catch_fatal_errors` call in this diff that become
removable thanks this change. (I will do that in a follow-up PR.)
Modify str_to_string to be machine-applicable
Fixes https://github.com/rust-lang/rust-clippy/issues/12768
I'm not sure if there is any potential for edge cases with this - since it only ever acts on `&str` types I can't think of any, and especially since the methods do the same thing anyway.
changelog: allow `str_to_string` lint to be automatically applied
Uplift `{Closure,Coroutine,CoroutineClosure}Args` and friends to `rustc_type_ir`
Part of converting the new solver's `structural_traits.rs` to be interner-agnostic.
I decided against aliasing `ClosureArgs<TyCtxt<'tcx>>` to `ClosureArgs<'tcx>` because it seemed so rare. I could do so if desired, though.
r? lcnr
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
