Don't create impl candidates when obligation contains errors
Fixes#72839
In PR #72621, trait selection was modified to no longer bail out early
when an error type was encountered. This allowed us treat `ty::Error` as
`Sized`, causing us to avoid emitting a spurious "not sized" error after
a type error had already occured.
However, this means that we may now try to match an impl candidate
against the error type. Since the error type will unify with almost
anything, this can cause us to infinitely recurse (eventually triggering
an overflow) when trying to verify certain `where` clauses.
This commit causes us to skip generating any impl candidates when an
error type is involved.
Incomplete features can also be unsound
Some incomplete features do not just ICE, they are also currently unsound (e.g. https://github.com/rust-lang/rust/pull/72029, and also `specialization` -- which is not yet marked incomplete but [should be](https://github.com/rust-lang/rust/pull/71420)). This makes the message reflect that.
While at it I also added a link to the tracking issue, which hopefully should explain what is incomplete/unsound about the feature.
Rework the std::iter::Step trait
Previous attempts: #43127#62886#68807
Tracking issue: #42168
This PR reworks the `Step` trait to be phrased in terms of the *successor* and *predecessor* operations. With this, `Step` hopefully has a consistent identity that can have a path towards stabilization. The proposed trait:
```rust
/// Objects that have a notion of *successor* and *predecessor* operations.
///
/// The *successor* operation moves towards values that compare greater.
/// The *predecessor* operation moves towards values that compare lesser.
///
/// # Safety
///
/// This trait is `unsafe` because its implementation must be correct for
/// the safety of `unsafe trait TrustedLen` implementations, and the results
/// of using this trait can otherwise be trusted by `unsafe` code to be correct
/// and fulful the listed obligations.
pub unsafe trait Step: Clone + PartialOrd + Sized {
/// Returns the number of *successor* steps required to get from `start` to `end`.
///
/// Returns `None` if the number of steps would overflow `usize`
/// (or is infinite, or if `end` would never be reached).
///
/// # Invariants
///
/// For any `a`, `b`, and `n`:
///
/// * `steps_between(&a, &b) == Some(n)` if and only if `Step::forward(&a, n) == Some(b)`
/// * `steps_between(&a, &b) == Some(n)` if and only if `Step::backward(&a, n) == Some(a)`
/// * `steps_between(&a, &b) == Some(n)` only if `a <= b`
/// * Corollary: `steps_between(&a, &b) == Some(0)` if and only if `a == b`
/// * Note that `a <= b` does _not_ imply `steps_between(&a, &b) != None`;
/// this is the case wheen it would require more than `usize::MAX` steps to get to `b`
/// * `steps_between(&a, &b) == None` if `a > b`
fn steps_between(start: &Self, end: &Self) -> Option<usize>;
/// Returns the value that would be obtained by taking the *successor*
/// of `self` `count` times.
///
/// If this would overflow the range of values supported by `Self`, returns `None`.
///
/// # Invariants
///
/// For any `a`, `n`, and `m`:
///
/// * `Step::forward_checked(a, n).and_then(|x| Step::forward_checked(x, m)) == Step::forward_checked(a, m).and_then(|x| Step::forward_checked(x, n))`
///
/// For any `a`, `n`, and `m` where `n + m` does not overflow:
///
/// * `Step::forward_checked(a, n).and_then(|x| Step::forward_checked(x, m)) == Step::forward_checked(a, n + m)`
///
/// For any `a` and `n`:
///
/// * `Step::forward_checked(a, n) == (0..n).try_fold(a, |x, _| Step::forward_checked(&x, 1))`
/// * Corollary: `Step::forward_checked(&a, 0) == Some(a)`
fn forward_checked(start: Self, count: usize) -> Option<Self>;
/// Returns the value that would be obtained by taking the *successor*
/// of `self` `count` times.
///
/// If this would overflow the range of values supported by `Self`,
/// this function is allowed to panic, wrap, or saturate.
/// The suggested behavior is to panic when debug assertions are enabled,
/// and to wrap or saturate otherwise.
///
/// Unsafe code should not rely on the correctness of behavior after overflow.
///
/// # Invariants
///
/// For any `a`, `n`, and `m`, where no overflow occurs:
///
/// * `Step::forward(Step::forward(a, n), m) == Step::forward(a, n + m)`
///
/// For any `a` and `n`, where no overflow occurs:
///
/// * `Step::forward_checked(a, n) == Some(Step::forward(a, n))`
/// * `Step::forward(a, n) == (0..n).fold(a, |x, _| Step::forward(x, 1))`
/// * Corollary: `Step::forward(a, 0) == a`
/// * `Step::forward(a, n) >= a`
/// * `Step::backward(Step::forward(a, n), n) == a`
fn forward(start: Self, count: usize) -> Self {
Step::forward_checked(start, count).expect("overflow in `Step::forward`")
}
/// Returns the value that would be obtained by taking the *successor*
/// of `self` `count` times.
///
/// # Safety
///
/// It is undefined behavior for this operation to overflow the
/// range of values supported by `Self`. If you cannot guarantee that this
/// will not overflow, use `forward` or `forward_checked` instead.
///
/// # Invariants
///
/// For any `a`:
///
/// * if there exists `b` such that `b > a`, it is safe to call `Step::forward_unchecked(a, 1)`
/// * if there exists `b`, `n` such that `steps_between(&a, &b) == Some(n)`,
/// it is safe to call `Step::forward_unchecked(a, m)` for any `m <= n`.
///
/// For any `a` and `n`, where no overflow occurs:
///
/// * `Step::forward_unchecked(a, n)` is equivalent to `Step::forward(a, n)`
#[unstable(feature = "unchecked_math", reason = "niche optimization path", issue = "none")]
unsafe fn forward_unchecked(start: Self, count: usize) -> Self {
Step::forward(start, count)
}
/// Returns the value that would be obtained by taking the *successor*
/// of `self` `count` times.
///
/// If this would overflow the range of values supported by `Self`, returns `None`.
///
/// # Invariants
///
/// For any `a`, `n`, and `m`:
///
/// * `Step::backward_checked(a, n).and_then(|x| Step::backward_checked(x, m)) == n.checked_add(m).and_then(|x| Step::backward_checked(a, x))`
/// * `Step::backward_checked(a, n).and_then(|x| Step::backward_checked(x, m)) == try { Step::backward_checked(a, n.checked_add(m)?) }`
///
/// For any `a` and `n`:
///
/// * `Step::backward_checked(a, n) == (0..n).try_fold(a, |x, _| Step::backward_checked(&x, 1))`
/// * Corollary: `Step::backward_checked(&a, 0) == Some(a)`
fn backward_checked(start: Self, count: usize) -> Option<Self>;
/// Returns the value that would be obtained by taking the *predecessor*
/// of `self` `count` times.
///
/// If this would overflow the range of values supported by `Self`,
/// this function is allowed to panic, wrap, or saturate.
/// The suggested behavior is to panic when debug assertions are enabled,
/// and to wrap or saturate otherwise.
///
/// Unsafe code should not rely on the correctness of behavior after overflow.
///
/// # Invariants
///
/// For any `a`, `n`, and `m`, where no overflow occurs:
///
/// * `Step::backward(Step::backward(a, n), m) == Step::backward(a, n + m)`
///
/// For any `a` and `n`, where no overflow occurs:
///
/// * `Step::backward_checked(a, n) == Some(Step::backward(a, n))`
/// * `Step::backward(a, n) == (0..n).fold(a, |x, _| Step::backward(x, 1))`
/// * Corollary: `Step::backward(a, 0) == a`
/// * `Step::backward(a, n) <= a`
/// * `Step::forward(Step::backward(a, n), n) == a`
fn backward(start: Self, count: usize) -> Self {
Step::backward_checked(start, count).expect("overflow in `Step::backward`")
}
/// Returns the value that would be obtained by taking the *predecessor*
/// of `self` `count` times.
///
/// # Safety
///
/// It is undefined behavior for this operation to overflow the
/// range of values supported by `Self`. If you cannot guarantee that this
/// will not overflow, use `backward` or `backward_checked` instead.
///
/// # Invariants
///
/// For any `a`:
///
/// * if there exists `b` such that `b < a`, it is safe to call `Step::backward_unchecked(a, 1)`
/// * if there exists `b`, `n` such that `steps_between(&b, &a) == Some(n)`,
/// it is safe to call `Step::backward_unchecked(a, m)` for any `m <= n`.
///
/// For any `a` and `n`, where no overflow occurs:
///
/// * `Step::backward_unchecked(a, n)` is equivalent to `Step::backward(a, n)`
#[unstable(feature = "unchecked_math", reason = "niche optimization path", issue = "none")]
unsafe fn backward_unchecked(start: Self, count: usize) -> Self {
Step::backward(start, count)
}
}
```
Note that all of these are associated functions and not callable via method syntax; the calling syntax is always `Step::forward(start, n)`. This version of the trait additionally changes the stepping functions to talk their arguments by value.
As opposed to previous attempts which provided a "step by one" method directly, this version of the trait only exposes "step by n". There are a few reasons for this:
- `Range*`, the primary consumer of `Step`, assumes that the "step by n" operation is cheap. If a single step function is provided, it will be a lot more enticing to implement "step by n" as n repeated calls to "step by one". While this is not strictly incorrect, this behavior would be surprising for anyone used to using `Range<{primitive integer}>`.
- With a trivial default impl, this can be easily added backwards-compatibly later.
- The debug-wrapping "step by n" needs to exist for `RangeFrom` to be consistent between "step by n" and "step by one" operation. (Note: the behavior is not changed by this PR, but making the behavior consistent is made tenable by this PR.)
Three "kinds" of step are provided: `_checked`, which returns an `Option` indicating attempted overflow; (unsuffixed), which provides "safe overflow" behavior (is allowed to panic, wrap, or saturate, depending on what is most convenient for a given type); and `_unchecked`, which is a version which assumes overflow does not happen.
Review is appreciated to check that:
- The invariants as described on the `Step` functions are enough to specify the "common sense" consistency for successor/predecessor.
- Implementation of `Step` functions is correct in the face of overflow and the edges of representable integers.
- Added tests of `Step` functions are asserting the correct behavior (and not just the implemented behavior).
When an associated type is found when a specific type was expected, if
possible provide a structured suggestion constraining the associated
type in a bound.
```
error[E0271]: type mismatch resolving `<T as Foo>::Y == i32`
--> $DIR/associated-types-multiple-types-one-trait.rs:13:5
|
LL | want_y(t);
| ^^^^^^ expected `i32`, found associated type
...
LL | fn want_y<T:Foo<Y=i32>>(t: &T) { }
| ----- required by this bound in `want_y`
|
= note: expected type `i32`
found associated type `<T as Foo>::Y`
help: consider constraining the associated type `<T as Foo>::Y` to `i32`
|
LL | fn have_x_want_y<T:Foo<X=u32, Y = i32>>(t: &T)
| ^^^^^^^^^
```
```
error[E0308]: mismatched types
--> $DIR/trait-with-missing-associated-type-restriction.rs:12:9
|
LL | qux(x.func())
| ^^^^^^^^ expected `usize`, found associated type
|
= note: expected type `usize`
found associated type `<impl Trait as Trait>::A`
help: consider constraining the associated type `<impl Trait as Trait>::A` to `usize`
|
LL | fn foo(x: impl Trait<A = usize>) {
| ^^^^^^^^^^
```
extend NLL checker to understand `'empty` combined with universes
This PR extends the NLL region checker to understand `'empty` combined with universes. In particular, it means that the NLL region checker no longer considers `exists<R2> { forall<R1> { R1: R2 } }` to be provable. This is work towards https://github.com/rust-lang/rust/issues/59490, but we're not all the way there. One thing in particular it does not address is error messages.
The modifications to the NLL region inference code turned out to be simpler than expected. The main change is to require that if `R1: R2` then `universe(R1) <= universe(R2)`.
This constraint follows from the region lattice (shown below), because we assume then that `R2` is "at least" `empty(Universe(R2))`, and hence if `R1: R2` (i.e., `R1 >= R2` on the lattice) then `R1` must be in some universe that can name `'empty(Universe(R2))`, which requires that `Universe(R1) <= Universe(R2)`.
```
static ----------+-----...------+ (greatest)
| | |
early-bound and | |
free regions | |
| | |
scope regions | |
| | |
empty(root) placeholder(U1) |
| / |
| / placeholder(Un)
empty(U1) -- /
| /
... /
| /
empty(Un) -------- (smallest)
```
I also made what turned out to be a somewhat unrelated change to add a special region to represent `'empty(U0)`, which we use (somewhat hackily) to indicate well-formedness checks in some parts of the compiler. This fixes#68550.
I did some investigation into fixing the error message situation. That's a bit trickier: the existing "nice region error" code around placeholders relies on having better error tracing than NLL currently provides, so that it knows (e.g.) that the constraint arose from applying a trait impl and things like that. I feel like I was hoping *not* to do such fine-grained tracing in NLL, and it seems like we...largely...got away with that. I'm not sure yet if we'll have to add more tracing information or if there is some sort of alternative.
It's worth pointing out though that I've not kind of shifted my opinion on whose job it should be to enforce lifetimes: I tend to think we ought to be moving back towards *something like* the leak-check (just not the one we *had*). If we took that approach, it would actually resolve this aspect of the error message problem, because we would be resolving 'higher-ranked errors' in the trait solver itself, and hence we wouldn't have to thread as much causal information back to the region checker. I think it would also help us with removing the leak check while not breaking some of the existing crates out there.
Regardless, I think it's worth landing this change, because it was relatively simple and it aligns the set of programs that NLL accepts with those that are accepted by the main region checker, and hence should at least *help* us in migration (though I guess we still also have to resolve the existing crates that rely on leak check for coherence).
r? @matthewjasper
When the return type is `!Sized` we look for all the returned
expressions in the body to fetch their types and provide a reasonable
suggestion. The tail expression of the body is normally evaluated after
checking whether the return type is `Sized`. Changing the order of the
evaluation produces undesirable knock down effects, so we detect the
specific case that newcomers are likely to encounter ,returning a single
bare trait object, and only in that case we evaluate the tail
expression's type so that the suggestion will be accurate.
During development, a function could have a return type set that is a
bare trait object by accident. We already suggest using either a boxed
trait object or `impl Trait` if the return paths will allow it. We now
do so too when there are *no* return paths or they all resolve to `!`.
We still don't handle cases where the trait object is *not* the entirety
of the return type gracefully.
Use `PredicateObligation`s instead of `Predicate`s
Keep more information about trait binding failures. Use more specific spans by pointing at bindings that introduce obligations.
Subset of #69709.
r? @eddyb
Polonius: update to 0.12.1, fix more move errors false positives, update test expectations
This PR:
- updates `polonius-engine` to version 0.12.1 to fix some move errors false positives
- fixes a fact generation mistake creating the other move errors false positives
- updates the test expectations for the polonius compare-mode so that all (minus the 2 OOMs) ui tests pass again (matching the [analysis doc](https://hackmd.io/CjYB0fs4Q9CweyeTdKWyEg?view) starting at case 34)
In my opinion, this is safe to rollup.
r? @nikomatsakis
Previously, we threw away the `Span` associated with a definition's
identifier when we encoded crate metadata, causing us to lose location
and hygiene information.
We now store the identifier's `Span` in the crate metadata.
When we decode items from the metadata, we combine
the name and span back into an `Ident`.
This improves the output of several tests, which previously had messages
suppressed due to dummy spans.
This is a prerequisite for #68686, since throwing away a `Span` means
that we lose hygiene information.
Improve reporting errors and suggestions for trait bounds
Fix#66802
- When printing errors for unsized function parameter, properly point at the parameter instead of function's body.
- Improve `consider further restricting this bound` (and related) messages by separating human-oriented hints from the machine-oriented ones.
When a type error involves a `dyn Trait` as the return type, do not emit
the type error, as the "return type is not `Sized`" error will provide
enough information to the user.
When we normalize the types in a function signature, we may end up
resolving a projection to an opaque type (e.g. `Self::MyType` when
we have `type MyType = impl SomeTrait`). When the projection is
resolved, we will instantiate the generic parameters into fresh
inference variables.
While we do want to normalize projections to opaque types, we don't want
to replace the explicit generic parameters (e.g. `T` in `impl
MyTrait<T>`) with inference variables. We want the opaque type in the
function signature to be eligible to be a defining use of that opaque
type - adding inference variables prevents this, since the opaque type
substs now appears to refer to some specific type, rather than a generic
type.
To resolve this issue, we inspect the opaque types in the function
signature after normalization. Any inference variables in the substs are
replaced with the corresponding generic parameter in the identity substs
(e.g. `T` in `impl MyTrait<T>`). Note that normalization is the only way
that we can end up with inference variables in opaque substs in a
function signature - users have no way of getting inference variables
into a function signature.
Note that all of this refers to the opaque type (ty::Opaque) and its
subst - *not* to the underlying type.
Fixes#59342
Esteban Küber
Matthew Jasper
Dylan DPC
Aaron Hill
csmoe
Yuki Okushi
Ralf Jung
bors
Andy Russell
Niko Matsakis
RoccoDev
Mazdak Farrokhzad
Alex Aktsipetrov
CAD97
Eduard-Mihai Burtescu
Remy Rakic
Matthias Krüger
Matthias Prechtl
Patryk Wychowaniec
Tyler Lanphear
Jonas Schievink
Vadim Petrochenkov
Ohad Ravid