`apply_member_constraints`: fix placeholder check
Checking whether the member region is *an existential region from a higher universe* is just wrong and I am pretty sure we've added that check by accident as the naming was just horribly confusing before rust-lang/rust#140466.
I've encountered this issue separately while working on rust-lang/rust#139587, but feel like it's probably easier to separately FCP this change. This allows the following code to compile
```rust
trait Proj<'a> {
type Assoc;
}
impl<'a, 'b, F: FnOnce() -> &'b ()> Proj<'a> for F {
type Assoc = ();
}
fn is_proj<F: for<'a> Proj<'a>>(f: F) {}
fn define<'a>() -> impl Sized + use<'a> {
// This adds a use of `opaque::<'a>` with hidden type `&'unconstrained_b ()`.
// 'unconstrained_b is an inference variable from a higher universe as it gets
// created inside of the binder of `F: for<'a> Proj<'a>`. This previously
// caused us to not apply member constraints. We now do, constraining
// it to `'a`.
is_proj(define::<'a>);
&()
}
fn main() {}
```
This should not be breaking change, even in theory. Applying member constraints is incomplete in rare circumstances which means that applying them in more cases can cause spurious errors, cc rust-lang/rust#140569/rust-lang/rust#142073. However, as we always skipped these member regions in `apply_member_constraints` the skipped region is guaranteed to cause an error in `check_member_constraints` later on.
When encountering a move error involving a closure because the captured value isn't `Copy`, and the obligation comes from a bound on a type parameter that requires `Fn` or `FnMut`, we point at it and explain that an `FnOnce` wouldn't cause the move error.
```
error[E0507]: cannot move out of `foo`, a captured variable in an `Fn` closure
--> f111.rs:15:25
|
14 | fn do_stuff(foo: Option<Foo>) {
| --- ----------- move occurs because `foo` has type `Option<Foo>`, which does not implement the `Copy` trait
| |
| captured outer variable
15 | require_fn_trait(|| async {
| -- ^^^^^ `foo` is moved here
| |
| captured by this `Fn` closure
16 | if foo.map_or(false, |f| f.foo()) {
| --- variable moved due to use in coroutine
|
help: `Fn` and `FnMut` closures require captured values to be able to be consumed multiple times, but an `FnOnce` consume them only once
--> f111.rs:12:53
|
12 | fn require_fn_trait<F: Future<Output = ()>>(_: impl Fn() -> F) {}
| ^^^^^^^^^
help: consider cloning the value if the performance cost is acceptable
|
16 | if foo.clone().map_or(false, |f| f.foo()) {
| ++++++++
```
`suggest_borrow_generic_arg`: use the correct generic args
The suggestion now gets calls' generic arguments from the callee's type to handle cases where the callee isn't an identifier expression. Fixesrust-lang/rust#145164.
Simplify polonius location-sensitive analysis
This PR reworks the location-sensitive analysis into what we think is a worthwhile subset of the datalog analysis. A sort of polonius alpha analysis that handles NLL problem case 3 and more, but is still using the faster "reachability as an approximation of liveness", as well as the same loans-in-scope computation as NLLs -- and thus doesn't handle full flow-sensitivity like the datalog implementation.
In the last few months, we've identified this subset as being actionable:
- we believe we can make a stabilizable version of this analysis
- it is an improvement over the status quo
- it can also be modeled in a-mir-formality, or some other formalism, for assurances about soundness, and I believe ````````@nikomatsakis```````` is interested in looking into this during H2.
- and we've identified the areas of work we wish to explore later to gradually expand the supported cases: the differences between reachability and liveness, support of kills, and considerations of time-traveling, for example.
The approach in this PR is to try less to have the graph only represent live paths, by checking whether we reach a live region during traversal and recording the loan as live there, instead of equating traversal with liveness like today because it has subtleties with the typeck edges in statements (that could forward loans to the successor point without ensuring their liveness). We can then also simplify these typeck stmt edges. And we also can simplify traversal by removing looking at kills, because that's enough to handle a bunch of NLL problem 3 cases -- and we can gradually support them more and more in traversal in the future, to reduce the approximation of liveness.
There's still some in-progress pieces of work w/r/t opaque types that I'm expecting [lcnr's opaque types rework](https://github.com/rust-lang/rust/pull/139587), and [amanda's SCCs rework](https://github.com/rust-lang/rust/pull/130227) to handle. That didn't seem to show up in tests until I rebased today (and shows lack of test coverage once again) when https://github.com/rust-lang/rust/pull/142255 introduced a couple of test failures with the new captures rules from edition 2024. It's not unexpected since we know more work is needed with member constraints (and we're not even using SCCs in this prototype yet)
I'll look into these anyways, both for future work, and checking how these other 2 PRs would change things.
---
I'm not sure the following means a lot until we have some formalism in-place, but:
- I've changed the polonius compare-mode to use this analysis: the tests pass with it, except 2 cases with minor diagnostics differences, and the 2 edition 2024 opaque types one I mentioned above and need to investigate
- things that are expected to work still do work: it bootstraps, can run our rustc-perf benchmarks (and the results are not even that bad), and a crater run didn't find any regressions (forgetting that crater currently fails to test around a quarter of all crates 👼)
- I've added tests with improvements, like the NLL problem case 3 and others, as well as some that behave the same as NLLs today and are thus worse than the datalog implementation
r? ````````@jackh726````````
(no rush I know you're deep in phd work and "implmentating" the new trait solver for r-a :p <3)
This also fixesrust-lang/rust#135646, a diagnostics ICE from the previous implementation.
defer opaque type errors, generally greatly reduce tainting
fixes the test for rust-lang/rust#135528, does not actually fix that issue properly.
This is useful as it causes the migration to rust-lang/rust#139587 to be a lot easier.
Add annotations to the graphviz region graph on region origins
This adds
- `(ex<'e>)` for regions whose origin is existential, with name if one exists,
- `(for<'p>)` for regions whose origin is a placeholder, with name if one exists
For any region whose name we don't know, use `'_`.
This has helped _my_ debugging and it doesn't create too bad clutter, I feel.
The change ~~is~~was ridiculously small, but I turned it into a separate PR so we can bikeshed the format.
The following snippet:
```rust
struct Co<'a>(&'a ());
struct Contra<'a>(fn(&'a ()));
// `exists<'e> forall<'p> 'p: 'e` -> ERROR
fn p_outlives_e(
x: for<'e> fn(for<'p> fn(fn(fn(Contra<'e>, Co<'p>)))),
) -> fn(fn(fn(for<'unify> fn(Contra<'unify>, Co<'unify>)))) {
x
```
Gives this graph:
Remove incomplete handling of kills during traversal for loan liveness
to get to a simpler and actionable prototype.
This handles the cases, on sufficiently simple examples, that were
deferred from NLLs (NLL problem case 3, lending iterators), and is still
a good step to put in people's hands without needing to wait for another
full implementation. This is a practical cut in scope, but it also
shows where are the areas of improvement, that we will explore in the
future.
Enforce tail call type is related to body return type in borrowck
Like all call terminators, tail call terminators instantiate the binder of the callee signature with region variables and equate the arg operand types with that signature's args to ensure that the call is valid.
However, unlike normal call terminators, we were forgetting to also relate the return type of the call terminator to anything. In the case of tail call terminators, the correct thing is to relate it to the return type of the caller function (or in other words, the return local `_0`).
This meant that if the caller's return type had some lifetime constraint, then that constraint wouldn't flow through the signature and affect the args.
This is what's happening in the example test I committed:
```rust
fn link(x: &str) -> &'static str {
become passthrough(x);
}
fn passthrough<T>(t: T) -> T { t }
fn main() {
let x = String::from("hello, world");
let s = link(&x);
drop(x);
println!("{s}");
}
```
Specifically, the type `x` is `'?0 str`, where `'?0` is some *universal* arg. The type of `passthrough` is `fn(&'?1 str) -> &'?1 str`. Equating the args sets `'?0 = '?1`. However, we need to also equate the return type `&'?1 str` to `&'static str` so that we eventually require that `'?0 = 'static`, which is a borrowck error!
-----
Look at the first commit for the functional change, and the second commit is just a refactor because we don't need to pass `Option<BasicBlock>` to `check_call_dest`, but just whether or not the terminator is expected to be diverging (i.e. if the return type is `!`).
Fixesrust-lang/rust#144916
This adds
- (ex) for regions whose origin is existential,
- (p) for regoins whose origin is a placeholder, and
- (p for <name>) if the originating placeholder is named.
This has helped _my_ debugging and it doesn't create too bad clutter, I feel.
The change is ridiculously small, but I turned it into a separate PR so we can bikeshed the format.
Currently they are mostly named `cx`, which is a terrible name for a
type that impls `Printer`/`PrettyPrinter`, and is easy to confuse with
other types like `TyCtxt`. This commit changes them to `p`. A couple of
existing `p` variables had to be renamed to make way.
Improve formatting of doc code blocks
We don't currently apply automatic formatting to doc comment code blocks. As a
result, it has built up various idiosyncracies, which make such automatic
formatting difficult. Some of those idiosyncracies also make things harder for
human readers or other tools.
This PR makes a few improvements to doc code formatting, in the hopes of making
future automatic formatting easier, as well as in many cases providing net
readability improvements.
I would suggest reading each commit separately, as each commit contains one
class of changes.
Remove the witness type from coroutine *args* (without actually removing the type)
This does as much of rust-lang/rust#144157 as we can without having to break rust-lang/rust#143545 and/or introduce some better way of handling higher ranked assumptions.
Namely, it:
* Stalls coroutines based off of the *coroutine* type rather than the witness type.
* Reworks the dtorck constraint hack to not rely on the witness type.
* Removes the witness type from the args of the coroutine, eagerly creating the type for nested obligations when needed (auto/clone impls).
I'll experiment with actually removing the witness type in a follow-up.
r? lcnr
This commit changes it to store a `Region` instead of a `RegionVid` for the `Var` cases:
- We avoid having to call `Region::new_var` to re-create `Region`s from
`RegionVid`s in a few places, avoiding the interning process, giving a
small perf win. (At the cost of the type allowing some invalid
combinations of values.)
- All the cases now store two `Region`s, so the commit also separates
the `ConstraintKind` (a new type) from the `sub` and `sup` arguments
in `Constraint`.
Currently there is `Ty` and `BoundTy`, and `Region` and `BoundRegion`,
and `Const` and... `BoundVar`. An annoying inconsistency.
This commit repurposes the existing `BoundConst`, which was barely used,
so it's the partner to `Const`. Unlike `BoundTy`/`BoundRegion` it lacks
a `kind` field but it's still nice to have because it makes the const
code more similar to the ty/region code everywhere.
The commit also removes `impl From<BoundVar> for BoundTy`, which has a
single use and doesn't seem worth it.
These changes fix the "FIXME: We really should have a separate
`BoundConst` for consts".
Make sure to account for the right item universal regions in borrowck
Fixes https://github.com/rust-lang/rust/issues/144608.
The ICE comes from a mismatch between the liberated late bound regions (i.e. "`ReLateParam`"s) that come from promoting closure outlives, and the regions we have in our region vid mapping from `UniversalRegions`.
When building `UniversalRegions`, we end up using the liberated regions from the binder of the closure's signature:
c8bb4e8a12/compiler/rustc_borrowck/src/universal_regions.rs (L521)
Notably, this signature may be anonymized if the closure signature being deduced comes from an external constraints:
c8bb4e8a12/compiler/rustc_hir_typeck/src/closure.rs (L759-L762)
This is true in the test file I committed, where the signature is influenced by the `impl FnMut(&mut ())` RPIT.
However, when promoting a type outlives constraint we end up creating a late bound lifetime mapping that disagrees with those liberated late bound regions we constructed in `UniversalRegions`:
c8bb4e8a12/compiler/rustc_borrowck/src/universal_regions.rs (L299)
Specifically, in `for_each_late_bound_region_in_item` (which is called by `for_each_late_bound_region_in_recursive_scope`), we were using `tcx.late_bound_vars` which uses the late bound regions *from the HIR*. This query both undercounts the late bound regions (e.g. those that end up being deduced from bounds), and also doesn't account for the fact that we anonymize them in the signature as mentioned above.
c8bb4e8a12/compiler/rustc_borrowck/src/universal_regions.rs (L977)
This PR fixes that function to use the *correct signature*, which properly considers the bound vars that come from deducing the signature of the closure, and which comes from the closure's args from the `type_of` query.
These examples feature Rust code that's presented primarily to
illustrate how its compilation would be handled, and these examples are
formatted to highlight those aspects in ways that rustfmt wouldn't
preserve. Turn formatting off in those examples.
(Doc code isn't formatted yet, but this will make it easier to enable
doc code formatting in the future.)
When encountering a moved value of a type that isn't `Clone` because of unmet obligations, but where all the unmet predicates reference crate-local types, mention them and suggest cloning, as we do in other cases already:
```
error[E0507]: cannot move out of `foo`, a captured variable in an `Fn` closure
--> f111.rs:14:25
|
13 | fn do_stuff(foo: Option<Foo>) {
| --- captured outer variable
14 | require_fn_trait(|| async {
| -- ^^^^^ `foo` is moved here
| |
| captured by this `Fn` closure
15 | if foo.map_or(false, |f| f.foo()) {
| ---
| |
| variable moved due to use in coroutine
| move occurs because `foo` has type `Option<Foo>`, which does not implement the `Copy` trait
|
note: if `Foo` implemented `Clone`, you could clone the value
--> f111.rs:4:1
|
4 | struct Foo;
| ^^^^^^^^^^ consider implementing `Clone` for this type
...
15 | if foo.map_or(false, |f| f.foo()) {
| --- you could clone this value
```
Tweak output for non-`Clone` values moved into closures
When we encounter a non-`Clone` value being moved into a closure, try to find the corresponding type of the binding being moved, if it is a `let`-binding or a function parameter. If any of those cases, we point at them with the note explaining that the type is not `Copy`, instead of giving that label to the place where it is captured. When it is a `let`-binding with no explicit type, we point at the initializer (if it fits in a single line).
```
error[E0507]: cannot move out of `foo`, a captured variable in an `Fn` closure
--> f111.rs:14:25
|
13 | fn do_stuff(foo: Option<Foo>) {
| --- ----------- move occurs because `foo` has type `Option<Foo>`, which does not implement the `Copy` trait
| |
| captured outer variable
14 | require_fn_trait(|| async {
| -- ^^^^^ `foo` is moved here
| |
| captured by this `Fn` closure
15 | if foo.map_or(false, |f| f.foo()) {
| --- variable moved due to use in coroutine
```
instead of
```
error[E0507]: cannot move out of `foo`, a captured variable in an `Fn` closure
--> f111.rs:14:25
|
13 | fn do_stuff(foo: Option<Foo>) {
| --- captured outer variable
14 | require_fn_trait(|| async {
| -- ^^^^^ `foo` is moved here
| |
| captured by this `Fn` closure
15 | if foo.map_or(false, |f| f.foo()) {
| ---
| |
| variable moved due to use in coroutine
| move occurs because `foo` has type `Option<Foo>`, which does not implement the `Copy` trait
```
```
error[E0507]: cannot move out of `f`, a captured variable in an `FnMut` closure
--> $DIR/borrowck-call-is-borrow-issue-12224.rs:57:13
|
LL | let mut f = move |g: Box<dyn FnMut(isize)>, b: isize| {
| ----- captured outer variable
...
LL | f(Box::new(|a| {
| --- captured by this `FnMut` closure
LL |
LL | foo(f);
| ^ move occurs because `f` has type `{closure@$DIR/borrowck-call-is-borrow-issue-12224.rs:52:17: 52:58}`, which does not implement the `Copy` trait
```
instead of
```
error[E0507]: cannot move out of `f`, a captured variable in an `FnMut` closure
--> $DIR/borrowck-call-is-borrow-issue-12224.rs:57:13
|
LL | let mut f = move |g: Box<dyn FnMut(isize)>, b: isize| {
| _________-----___-
| | |
| | captured outer variable
LL | | let _ = s.len();
LL | | };
| |_____- move occurs because `f` has type `{closure@$DIR/borrowck-call-is-borrow-issue-12224.rs:52:17: 52:58}`, which does not implement the `Copy` trait
LL | f(Box::new(|a| {
| --- captured by this `FnMut` closure
LL |
LL | foo(f);
| ^ `f` is moved here
```
Account not only for `fn` parameters when moving non-`Copy` values into closure, but also for let bindings.
```
error[E0507]: cannot move out of `bar`, a captured variable in an `FnMut` closure
--> $DIR/borrowck-move-by-capture.rs:9:29
|
LL | let bar: Box<_> = Box::new(3);
| --- ------ move occurs because `bar` has type `Box<isize>`, which does not implement the `Copy` trait
| |
| captured outer variable
LL | let _g = to_fn_mut(|| {
| -- captured by this `FnMut` closure
LL | let _h = to_fn_once(move || -> isize { *bar });
| ^^^^^^^^^^^^^^^^ ---- variable moved due to use in closure
| |
| `bar` is moved here
|
help: consider cloning the value before moving it into the closure
|
LL ~ let value = bar.clone();
LL ~ let _h = to_fn_once(move || -> isize { value });
|
```
```
error[E0507]: cannot move out of `y`, a captured variable in an `Fn` closure
--> $DIR/unboxed-closures-move-upvar-from-non-once-ref-closure.rs:12:9
|
LL | let y = vec![format!("World")];
| - ---------------------- move occurs because `y` has type `Vec<String>`, which does not implement the `Copy` trait
| |
| captured outer variable
LL | call(|| {
| -- captured by this `Fn` closure
LL | y.into_iter();
| ^ ----------- `y` moved due to this method call
| |
| `y` is moved here
|
note: `into_iter` takes ownership of the receiver `self`, which moves `y`
--> $SRC_DIR/core/src/iter/traits/collect.rs:LL:COL
help: you can `clone` the value and consume it, but this might not be your desired behavior
|
LL | <Vec<String> as Clone>::clone(&y).into_iter();
| +++++++++++++++++++++++++++++++ +
help: consider cloning the value if the performance cost is acceptable
|
LL | y.clone().into_iter();
| ++++++++
```
When encountering a non-`Copy` value that is moved into a closure which is coming directly from a fn parameter, point at the parameter's type when mentioning it is not `Copy`.
Before:
```
error[E0507]: cannot move out of `foo`, a captured variable in an `Fn` closure
--> f111.rs:14:25
|
13 | fn do_stuff(foo: Option<Foo>) {
| --- captured outer variable
14 | require_fn_trait(|| async {
| -- ^^^^^ `foo` is moved here
| |
| captured by this `Fn` closure
15 | if foo.map_or(false, |f| f.foo()) {
| ---
| |
| variable moved due to use in coroutine
| move occurs because `foo` has type `Option<Foo>`, which does not implement the `Copy` trait
```
After:
```
error[E0507]: cannot move out of `foo`, a captured variable in an `Fn` closure
--> f111.rs:14:25
|
13 | fn do_stuff(foo: Option<Foo>) {
| --- ----------- move occurs because `foo` has type `Option<Foo>`, which does not implement the `Copy` trait
| |
| captured outer variable
14 | require_fn_trait(|| async {
| -- ^^^^^ `foo` is moved here
| |
| captured by this `Fn` closure
15 | if foo.map_or(false, |f| f.foo()) {
| --- variable moved due to use in coroutine
```
