Don't lint `assertions_on_constants` on any const assertions
close#12816close#12847
cc #12817
----
changelog: Fix false positives in consts for `assertions_on_constants` and `unnecessary_operation`.
Tighten `fn_decl_span` for async blocks
Tightens the span of `async {}` blocks in diagnostics, and subsequently async closures and async fns, by actually setting the `fn_decl_span` correctly. This is kinda a follow-up on #125078, but it fixes the problem in a more general way.
I think the diagnostics are significantly improved, since we no longer have a bunch of overlapping spans. I'll point out one caveat where I think the diagnostic may get a bit more confusing, but where I don't think it matters.
r? ````@estebank```` or ````@oli-obk```` or someone else on wg-diag or compiler i dont really care lol
resolve `clippy::invalid_paths` on `bool::then`
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: none
This removes the ICE codepaths for `f16` and `f128` in Clippy.
`rustc_apfloat` is used as a dependency for the parsing of these types,
since their `FromStr` implementation will not be available in the
standard library for a while.
Add more types to `is_from_proc_macro`
I've been running through going through all the lint implementations to clean them up. I'll be separating out the changes into small PRs to make reviewing easier.
changelog: none
* Delay the parsing of the use node
* Mark when the `SyntaxContext` changes rather than return `None`
* Return a default value if the HIR tree is broken rather than `None`
Avoid emitting `assigning_clones` when cloned data borrows from the place to clone into
Fixes#12444Fixes#12460Fixes#12749Fixes#12757Fixes#12929
I think the documentation for the function should describe what- and how this is fixing the issues well.
It avoids emitting a warning when the data being cloned borrows from the place to clone into, which is information that we can get from `PossibleBorrowerMap`. Unfortunately, it is a tiny bit tedious to match on the MIR like that and I'm not sure if this is possibly relying a bit too much on the exact MIR lowering for assignments.
Things left to do:
- [x] Handle place projections (or verify that they work as expected)
- [x] Handle non-`Drop` types
changelog: [`assigning_clones`]: avoid warning when the suggestion would lead to a borrow-check error
Let `qualify_min_const_fn` deal with drop terminators
Fixes#12677
The `method_accepts_droppable` check that was there seemed overly conservative.
> Returns true if any of the method parameters is a type that implements `Drop`.
> The method can't be made const then, because `drop` can't be const-evaluated.
Accepting parameters that implement `Drop` should still be fine as long as the parameter isn't actually dropped, as is the case in the linked issue where the droppable is moved into the return place. This more accurate analysis ("is there a `drop` terminator") is already done by `qualify_min_const_fn` [here](f5e250180c/clippy_utils/src/qualify_min_const_fn.rs (L298)), so I don't think this additional check is really necessary?
Fixing the other, second case in the linked issue was only slightly more involved, since `Vec::new()` is a function call that has the ability to panic, so there must be a `drop()` terminator for cleanup, however we should be able to freely ignore that. [Const checking ignores cleanup blocks](https://github.com/rust-lang/rust/blob/master/compiler/rustc_const_eval/src/transform/check_consts/check.rs#L382-L388), so we should, too?
r? `@Jarcho`
----
changelog: [`missing_const_for_fn`]: continue linting on fns with parameters implementing `Drop` if they're not actually dropped
Make `for_each_expr` visit closures by default, rename the old version `for_each_expr_without_closures`
A lot of the time `for_each_expr` is picked when closures should be visited so I think it makes sense for this to be the default with the alternative available for when you don't need to visit them.
The first commit renames `for_each_expr` to `for_each_expr_without_closures` and `for_each_expr_with_closures` to `for_each_expr`
The second commit switches a few uses that I caught over to include closures to fix a few bugs
changelog: none
Handle const effects inherited from parent correctly in `type_certainty`
This fixes a (debug) ICE in `type_certainty` that happened in the [k256 crate]. (I'm sure you can also specifically construct an edge test case that will run into type_certainty false positives visible outside of debug builds from this bug)
<details>
<summary>Minimal ICE repro</summary>
```rs
use std::ops::Add;
Add::add(1_i32, 1).add(i32::MIN);
```
</details>
The subtraction here overflowed:
436675b477/clippy_utils/src/ty/type_certainty/mod.rs (L209)
... when we have something like `Add::add` where `add` fn has 0 generic params but the `host_effect_index` is `Some(2)` (inherited from the parent generics, the const trait `Add`), and we end up executing `0 - 1`.
(Even if the own generics weren't empty and we didn't overflow, this would still be wrong because it would assume that a trait method with 1 generic parameter didn't have any generics).
So, *only* exclude the "host" generic parameter if it's actually bound by the own generics
changelog: none
[k256 crate]: https://github.com/RustCrypto/elliptic-curves/tree/master/k256
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
[perf] Delay the construction of early lint diag structs
Attacks some of the perf regressions from https://github.com/rust-lang/rust/pull/124417#issuecomment-2123700666.
See individual commits for details. The first three commits are not strictly necessary.
However, the 2nd one (06bc4fc671, *Remove `LintDiagnostic::msg`*) makes the main change way nicer to implement.
It's also pretty sweet on its own if I may say so myself.
* instead simply set the primary message inside the lint decorator functions
* it used to be this way before [#]101986 which introduced `msg` to prevent
good path delayed bugs (which no longer exist) from firing under certain
circumstances when lints were suppressed / silenced
* this is no longer necessary for various reasons I presume
* it shaves off complexity and makes further changes easier to implement
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.
make sure the msrv for `const_raw_ptr_deref` is met when linting [`missing_const_for_fn`]
fixes: #8864
---
changelog: make sure the msrv for `const_ptr_deref` is met when linting [`missing_const_for_fn`]
`InferCtxt::next_{ty,const}_var*` all take an origin, but the
`param_def_id` is almost always `None`. This commit changes them to just
take a `Span` and build the origin within the method, and adds new
methods for the rare cases where `param_def_id` might not be `None`.
This avoids a lot of tedious origin building.
Specifically:
- next_ty_var{,_id_in_universe,_in_universe}: now take `Span` instead of
`TypeVariableOrigin`
- next_ty_var_with_origin: added
- next_const_var{,_in_universe}: takes Span instead of ConstVariableOrigin
- next_const_var_with_origin: added
- next_region_var, next_region_var_in_universe: these are unchanged,
still take RegionVariableOrigin
The API inconsistency (ty/const vs region) seems worth it for the
large conciseness improvements.
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.
Some hir cleanups
It seemed odd to not put `AnonConst` in the arena, compared with the other types that we did put into an arena. This way we can also give it a `Span` without growing a lot of other HIR data structures because of the extra field.
r? compiler
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`
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
Fix `is_test_module_or_function`
The rustdoc comment for `is_test_module_or_function` states: 2795a60189/clippy_utils/src/lib.rs (L2561-L2566)
Given `item`, the function calls `is_in_test_function` with `item.hir_id()`. However, `is_in_test_function` considers only `item`'s parents, not `item` itself. This PR fixes the problem.
The `test_with_disallowed_name` test fails without the fix, but passes once applied.
changelog: none
Implement syntax for `impl Trait` to specify its captures explicitly (`feature(precise_capturing)`)
Implements `impl use<'a, 'b, T, U> Sized` syntax that allows users to explicitly list the captured parameters for an opaque, rather than inferring it from the opaque's bounds (or capturing *all* lifetimes under 2024-edition capture rules). This allows us to exclude some implicit captures, so this syntax may be used as a migration strategy for changes due to #117587.
We represent this list of captured params as `PreciseCapturingArg` in AST and HIR, resolving them between `rustc_resolve` and `resolve_bound_vars`. Later on, we validate that the opaques only capture the parameters in this list.
We artificially limit the feature to *require* mentioning all type and const parameters, since we don't currently have support for non-lifetime bivariant generics. This can be relaxed in the future.
We also may need to limit this to require naming *all* lifetime parameters for RPITIT, since GATs have no variance. I have to investigate this. This can also be relaxed in the future.
r? `@oli-obk`
Tracking issue:
- https://github.com/rust-lang/rust/issues/123432
type certainty: clear `DefId` when an expression's type changes to non-adt
Fixes#12585
The root cause of the ICE in the linked issue was in the expression `one.x`, in the array literal.
The type of `one` is the `One` struct: an adt with a DefId, so its certainty is `Certain(def_id_of_one)`. However, the field access `.x` can then change the type (to `i32` here) and that should update that `DefId` accordingly. It does do that correctly when `one.x` would be another adt with a DefId:
97ba291d5a/clippy_utils/src/ty/type_certainty/mod.rs (L90-L91)
but when it *isn't* an adt and there is no def id (which is the case in the linked issue: `one.x` is an i32), it keeps the `DefId` of `One`, even though that's the wrong type (which would then lead to a contradiction later when joining `Certainty`s):
97ba291d5a/clippy_utils/src/ty/type_certainty/mod.rs (L92-L93)
In particular, in the linked issue, `from_array([one.x, two.x])` would try to join the `Certainty` of the two array elements, which *should* have been `[Certain(None), Certain(None)]`, because `i32`s have no `DefId`, but instead it was `[Certain(One), Certain(Two)]`, because the DefId wasn't cleared from when it was visiting `one` and `two`. This is the "contradiction" that could be seen in the ICE message
... so this changes it to clear the `DefId` when it isn't an adt.
cc `@smoelius` you implemented this initially in #11135, does this change make sense to you?
changelog: none
rename ptr::from_exposed_addr -> ptr::with_exposed_provenance
As discussed on [Zulip](https://rust-lang.zulipchat.com/#narrow/stream/136281-t-opsem/topic/To.20expose.20or.20not.20to.20expose/near/427757066).
The old name, `from_exposed_addr`, makes little sense as it's not the address that is exposed, it's the provenance. (`ptr.expose_addr()` stays unchanged as we haven't found a better option yet. The intended interpretation is "expose the provenance and return the address".)
The new name nicely matches `ptr::without_provenance`.
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)
Remove `unwrap` from `match_trait_method`
Unused_IO_amount relies on `match_trait_method` in order to match trait methods that exist in Tokio traits as the corresponding symbols don't exist.
With this commit we remove the unwrap that caused #12366.
Note: author (`@m-rph)` and `@GuillaumeGomez` couldn't replicate #12366.
changelog:none
r? `@blyxyas`
Unused_IO_amount relies on `match_trait_method` in order to match
trait methods that exist in Tokio traits as the corresponding symbols don't exist.
With this commit we remove the unwrap that may have caused 12366.
Note: author (@m-rph) and @GuillaumeGomez couldn't replicate 12366.
refactor check_{lang,library}_ub: use a single intrinsic
This enacts the plan I laid out [here](https://github.com/rust-lang/rust/pull/122282#issuecomment-1996917998): use a single intrinsic, called `ub_checks` (in aniticpation of https://github.com/rust-lang/compiler-team/issues/725), that just exposes the value of `debug_assertions` (consistently implemented in both codegen and the interpreter). Put the language vs library UB logic into the library.
This makes it easier to do something like https://github.com/rust-lang/rust/pull/122282 in the future: that just slightly alters the semantics of `ub_checks` (making it more approximating when crates built with different flags are mixed), but it no longer affects whether these checks can happen in Miri or compile-time.
The first commit just moves things around; I don't think these macros and functions belong into `intrinsics.rs` as they are not intrinsics.
r? `@saethlin`
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```
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
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.
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)`
