Provide more context on `Fn` closure modifying binding
When modifying a binding from inside of an `Fn` closure, point at the binding definition and suggest using an `std::sync` type that would allow the code to compile.
```
error[E0594]: cannot assign to `counter`, as it is a captured variable in a `Fn` closure
--> f703.rs:6:9
|
4 | let mut counter = 0;
| ----------- `counter` declared here, outside the closure
5 | let x: Box<dyn Fn()> = Box::new(|| {
| -- in this closure
6 | counter += 1;
| ^^^^^^^^^^^^ cannot assign
|
= help: consider using `std::sync::atomic::AtomicI32` instead, which allows for multiple threads to access and modify the value
```
This provides more context on where the binding being modified was declared, and more importantly, guides newcomers towards `std::sync` when encountering cases like these.
When the requirement comes from an argument of a local function, we already tell the user that they might want to change from `Fn` to `FnMut`:
```
error[E0594]: cannot assign to `x`, as it is a captured variable in a `Fn` closure
--> $DIR/borrow-immutable-upvar-mutation.rs:21:27
|
LL | fn to_fn<A: std::marker::Tuple, F: Fn<A>>(f: F) -> F {
| - change this to accept `FnMut` instead of `Fn`
...
LL | let mut x = 0;
| ----- `x` declared here, outside the closure
LL | let _f = to_fn(|| x = 42);
| ----- -- ^^^^^^ cannot assign
| | |
| | in this closure
| expects `Fn` instead of `FnMut`
|
= help: consider using `std::sync::atomic::AtomicI32` instead, which allows for multiple threads to access and modify the value
```
We might want to avoid the `help` in this case.
_Inspired by a [part of Niko's keynote at RustNation UK 2024](https://youtu.be/04gTQmLETFI?si=dgJL2OJRtuShkxdD&t=600)._
When modifying a binding from inside of an `Fn` closure, point at the binding definition and suggest using an `std::sync` type that would allow the code to compile.
```
error[E0594]: cannot assign to `counter`, as it is a captured variable in a `Fn` closure
--> f703.rs:6:9
|
4 | let mut counter = 0;
| ----------- `counter` declared here, outside the closure
5 | let x: Box<dyn Fn()> = Box::new(|| {
| -- in this closure
6 | counter += 1;
| ^^^^^^^^^^^^ cannot assign
```
Suggest making binding `mut` on `&mut` reborrow
When a binding needs to be mutably reborrowed multiple times, suggesting removing `&mut` will lead to follow up errors. Instead suggest both making the binding mutable and removing the reborrow.
```
error[E0596]: cannot borrow `outer` as mutable, as it is not declared as mutable
--> f14.rs:2:12
|
2 | match (&mut outer, 23) {
| ^^^^^^^^^^ cannot borrow as mutable
|
note: the binding is already a mutable borrow
--> f14.rs:1:16
|
1 | fn test(outer: &mut Option<i32>) {
| ^^^^^^^^^^^^^^^^
help: consider making the binding mutable if you need to reborrow multiple times
|
1 | fn test(mut outer: &mut Option<i32>) {
| +++
help: if there is only one mutable reborrow, remove the `&mut`
|
2 - match (&mut outer, 23) {
2 + match (outer, 23) {
|
```
Address rust-lang/rust#81059.
Implement pin-project in pattern matching for `&pin mut|const T`
This PR implements part of rust-lang/rust#130494. It supports pin-project in pattern matching for `&pin mut|const T`.
~Pin-projection by field access (i.e. `&pin mut|const place.field`) is not fully supported yet since pinned-borrow is not ready (rust-lang/rust#135731).~
CC ``````@traviscross``````
When a binding needs to be mutably reborrowed multiple times, suggesting removing `&mut` will lead to follow up errors. Instead suggest both making the binding mutable and removing the reborrow.
```
error[E0596]: cannot borrow `outer` as mutable, as it is not declared as mutable
--> f14.rs:2:12
|
2 | match (&mut outer, 23) {
| ^^^^^^^^^^ cannot borrow as mutable
|
note: the binding is already a mutable borrow
--> f14.rs:1:16
|
1 | fn test(outer: &mut Option<i32>) {
| ^^^^^^^^^^^^^^^^
help: consider making the binding mutable if you need to reborrow multiple times
|
1 | fn test(mut outer: &mut Option<i32>) {
| +++
help: if there is only one mutable reborrow, remove the `&mut`
|
2 - match (&mut outer, 23) {
2 + match (outer, 23) {
|
```
Use `mut` less in dataflow analysis
`&mut Analysis` is used a lot:
- In results visitors, even though only one visitor needs mutability.
- In all the `apply_*` methods, even though only one visitor needs mutability.
I've lost track of the number of times I've thought "why are these `mut` again?" and had to look through the code to remind myself. It's really unexpected, and most `Analysis` instances are immutable, because the `state` values are what get mutated.
This commit introduces `RefCell` in one analysis and one results visitor. This then lets another existing `RefCell` be removed, and a ton of `&mut Analysis` arguments become `&Analysis`. And then `Analysis` and `Results` can be recombined.
r? `@cjgillot`
`Results` used to contain an `Analysis`, but it was removed in #140234.
That change made sense because the analysis was mutable but the entry
states were immutable and it was good to separate them so the mutability
of the different pieces was clear.
Now that analyses are immutable there is no need for the separation,
lots of analysis+results pairs can be combined, and the names are going
back to what they were before:
- `Results` -> `EntryStates`
- `AnalysisAndResults` -> `Results`
The `state: A::Domain` value is the primary things that's modified when
performing an analysis. The `Analysis` impl is immutable in every case
but one (`MaybeRequiredStorage`) and it now uses interior mutability.
As well as changing many `&mut A` arguments to `&A`, this also:
- lets `CowMut` be replaced with the simpler `SimpleCow` in `cursor.rs`;
- removes the need for the `RefCell` in `Formatter`;
- removes the need for `MaybeBorrowedLocals` to impl `Clone`, because
it's a unit type and it's now clear that its constructor can be used
directly instead of being put into a local variable and cloned.
Limit impl_trait_header query to only trait impls
Changes `impl_trait_header` to panic on inherent impls intstead of returning None. A few downstream functions are split into option and non-option returning functions. This gets rid of a lot of unwraps where we know we have a trait impl, while there are still some cases where the Option is helpful.
Summary of changes to tcx methods:
* `impl_is_of_trait` (new)
* `impl_trait_header` -> `impl_trait_header`/`impl_opt_trait_header`
* `impl_trait_ref` -> `impl_trait_ref`/`impl_opt_trait_ref`
* `trait_id_of_impl` -> `impl_trait_id`/`impl_opt_trait_id`
Pre-compute MIR CFG caches for borrowck and other analyses
I was puzzled that https://github.com/rust-lang/rust/pull/142390 introduces additional computations of CFG traversals: borrowck computes them, right?
It turns out that borrowck clones the MIR body, so doesn't share its cache with other analyses.
This PR:
- forces the computation of all caches in `mir_promoted` query;
- modifies region renumbering to avoid dropping that cache.
Polonius liveness has to contain boring locals, and we ignore them in
diagnostics to match NLL diagnostics, since they doesn't contain boring locals.
We ignored these when explaining why a loan contained a point due to
a use of a live var, but not when it contained a point due to a drop of
a live var.
Perform unused assignment and unused variables lints on MIR.
Rebase of https://github.com/rust-lang/rust/pull/101500
Fixes https://github.com/rust-lang/rust/issues/51003.
The first commit moves detection of uninhabited types from the current liveness pass to MIR building.
In order to keep the same level of diagnostics, I had to instrument MIR a little more:
- keep for which original local a guard local is created;
- store in the `VarBindingForm` the list of introducer places and whether this was a shorthand pattern.
I am not very proud of the handling of self-assignments. The proposed scheme is in two parts: first detect probable self-assignments, by pattern matching on MIR, and second treat them specially during dataflow analysis. I welcome ideas.
Please review carefully the changes in tests. There are many small changes to behaviour, and I'm not sure all of them are desirable.
Validate CopyForDeref and DerefTemps better and remove them from runtime MIR
(split from my WIP rust-lang/rust#145344)
This PR:
- Removes `Rvalue::CopyForDeref` and `LocalInfo::DerefTemp` from runtime MIR
- Using a new mir pass `EraseDerefTemps`
- `CopyForDeref(x)` is turned into `Use(Copy(x))`
- `DerefTemp` is turned into `Boring`
- Not sure if this part is actually necessary, it made more sense in rust-lang/rust#145344 with `DerefTemp` storing actual data that I wanted to keep from having to be kept in sync with the rest of the body in runtime MIR
- Checks in validation that `CopyForDeref` and `DerefTemp` are only used together
- Removes special handling for `CopyForDeref` from many places
- Removes `CopyForDeref` from `custom_mir` reverting rust-lang/rust#111587
- In runtime MIR simple copies can be used instead
- In post cleanup analysis MIR it was already wrong to use due to the lack of support for creating `DerefTemp` locals
- Possibly this should be its own PR?
- Adds an argument to `deref_finder` to avoid creating new `DerefTemp`s and `CopyForDeref` in runtime MIR.
- Ideally we would just avoid making intermediate derefs instead of fixing it at the end of a pass / during shim building
- Removes some usages of `deref_finder` that I found out don't actually do anything
r? oli-obk
Turn ProjectionElem::Subtype into CastKind::Subtype
I noticed that drop elaboration can't, in general, handle `ProjectionElem::SubType`. It creates a disjoint move path that overlaps with other move paths. (`Subslice` does too, and I'm working on a different PR to make that special case less fragile.) If its skipped and treated as the same move path as its parent then `MovePath.place` has multiple possible projections. (It would probably make sense to remove all `Subtype` projections for the canonical place but it doesn't make sense to have this special case for a problem that doesn't actually occur in real MIR.)
The only reason this doesn't break is that `Subtype` is always the sole projection of the local its applied to. For the same reason, it works fine as a `CastKind` so I figured that makes more sense than documenting and validating this hidden invariant.
cc rust-lang/rust#112651, rust-lang/rust#133258
r? Icnr (bc you've been the main person dealing with `Subtype` it looks like)
Rename various "concrete opaque type" things to say "hidden type"
r? lcnr
I've found "concrete opaque type" terminology to be somewhat confusing as in conversation and when explaining opaque type stuff to people I always just talk about things in terms of hidden types. Also the hidden types of opaques are very much not *concrete* in the same sense that a type without any generic parameters is concrete which is an unfortunate overlap in terminology.
I've tried to update comments to also stop referring to things as concrete opaque types but this is mostly best effort as it difficult to find all such cases amongst the massive amounts of uses of "concrete" or "hidden" across the whole compiler.
```
error[E0716]: temporary value dropped while borrowed
--> $DIR/multiple-sources-for-outlives-requirement.rs:5:38
|
LL | fn foo<'b>() {
| -- lifetime `'b` defined here
LL | outlives_indir::<'_, 'b, _>(&mut 1u32);
| ---------------------------------^^^^-- temporary value is freed at the end of this statement
| | |
| | creates a temporary value which is freed while still in use
| argument requires that borrow lasts for `'b`
|
note: requirements that the value outlives `'b` introduced here
--> $DIR/multiple-sources-for-outlives-requirement.rs:1:23
|
LL | fn outlives_indir<'a: 'b, 'b, T: 'a>(_x: T) {}
| ^^ ^^
```
```
error[E0597]: `c` does not live long enough
--> $DIR/without-precise-captures-we-are-powerless.rs:19:20
|
LL | fn simple<'a>(x: &'a i32) {
| -- lifetime `'a` defined here
...
LL | let c = async move || { println!("{}", *x); };
| - binding `c` declared here
LL | outlives::<'a>(c());
| ---------------^---
| | |
| | borrowed value does not live long enough
| argument requires that `c` is borrowed for `'a`
LL | outlives::<'a>(call_once(c));
LL | }
| - `c` dropped here while still borrowed
|
note: requirement that `c` is borrowed for `'a` introduced here
--> $DIR/without-precise-captures-we-are-powerless.rs:7:33
|
LL | fn outlives<'a>(_: impl Sized + 'a) {}
| ^^
```
When encountering a `ConstraintCategory::Predicate` in a funtion call, point at the `Span` for that `Predicate` to explain where the lifetime obligation originates from.
fix 2 borrowck issues
fixes https://github.com/rust-lang/rust/issues/146467 cc ``@amandasystems``
our understanding here is as follows: region constraints from computing implied bounds gets `ConstraintCategory::Internal`. If there's a higher-ranked subtyping errors while computing implied bounds we then ended up with only `ConstraintCategory::Internal` and `ConstraintCategory::OutlivesUnnameablePlaceholder(_)` constraints.
The path was something like
- `'placeholderU2: 'placeholderU1` (`Internal`)
- `'placeholderU1: 'static` (`OutlivesUnnameablePlaceholder('placeholderU2)`)
It's generally somewhat subtle here as ideally relating placeholders doesn't introduce `'static` constraints. Relating the placeholders themselves will always error regardless, cc https://github.com/rust-lang/rust/pull/142623.
---
separately fixes https://github.com/rust-lang/rust/pull/145925#issuecomment-3303733357 by updating the location for deferred closure requirements inside of promoteds. I am not updating their category as doing so is 1) effort and 2) imo actually undesirable 🤔 see the comments in `TypeChecker::check_promoted` cc ``@lqd``
r? lqd
Clean up universe evaluation during type test evaluation
The logic was, as the removed comments suggest, hackish and meant to implement previous logic that was factored out. The new logic does exactly what the comments say, and is much less surprising.
I'm afraid we may want
r? `@lcnr`
for this one too.
I am sorry, but at least it should be easier to review.
The logic was, as the removed comments suggest, hackish
and meant to implement previous logic that was factored out.
The new logic does exactly what the comments say, and is much
less surprising.
