Commit Graph

4413 Commits

Author SHA1 Message Date
Matthias Krüger
33b4947554
Rollup merge of #131112 - jswrenn:fix-130413, r=compiler-errors
TransmuteFrom: Gracefully handle unnormalized types and normalization errors

~~Refactor to share code between `TransmuteFrom`'s trait selection and error reporting code paths. Additionally normalizes the source and destination types, and gracefully handles normalization errors.~~

Fixes #130413

r​? `@compiler-errors`
2024-10-03 21:52:45 +02:00
Michael Goulet
bc5f9520c1 Remove crashes, add comment 2024-10-03 15:19:23 -04:00
Deadbeef
7f6150b577 Improve const traits diagnostics for new desugaring 2024-10-02 19:45:17 +08:00
Jack Wrenn
5b1a2b8712 TransmuteFrom: Gracefully handle unnormalized types and normalization errors
Fixes #130413
2024-10-01 20:52:17 +00:00
Michael Goulet
7c552d56b2 Also fix first_method_vtable_slot 2024-09-30 13:17:33 -04:00
Michael Goulet
d87e0ca497 Extract trait_refs_are_compatible, make it instantiate binders 2024-09-30 13:17:33 -04:00
Michael Goulet
af3f212453 Instantiate binders in supertrait_vtable_slot 2024-09-30 13:17:33 -04:00
Matthias Krüger
4e510daed7
Rollup merge of #130866 - compiler-errors:dyn-instantiate-binder, r=lcnr
Allow instantiating object trait binder when upcasting

This PR fixes two bugs (that probably need an FCP).

### We use equality rather than subtyping for upcasting dyn conversions

This code should be valid:

```rust
#![feature(trait_upcasting)]

trait Foo: for<'h> Bar<'h> {}
trait Bar<'a> {}

fn foo(x: &dyn Foo) {
    let y: &dyn Bar<'static> = x;
}
```
But instead:

```
error[E0308]: mismatched types
 --> src/lib.rs:7:32
  |
7 |     let y: &dyn Bar<'static> = x;
  |                                ^ one type is more general than the other
  |
  = note: expected existential trait ref `for<'h> Bar<'h>`
             found existential trait ref `Bar<'_>`
```

And so should this:

```rust
#![feature(trait_upcasting)]

fn foo(x: &dyn for<'h> Fn(&'h ())) {
    let y: &dyn FnOnce(&'static ()) = x;
}
```

But instead:

```
error[E0308]: mismatched types
 --> src/lib.rs:4:39
  |
4 |     let y: &dyn FnOnce(&'static ()) = x;
  |                                       ^ one type is more general than the other
  |
  = note: expected existential trait ref `for<'h> FnOnce<(&'h (),)>`
             found existential trait ref `FnOnce<(&(),)>`
```

Specifically, both of these fail because we use *equality* when comparing the supertrait to the *target* of the unsize goal. For the first example, since our supertrait is `for<'h> Bar<'h>` but our target is `Bar<'static>`, there's a higher-ranked type mismatch even though we *should* be able to instantiate that supertrait binder when upcasting. Similarly for the second example.

### New solver uses equality rather than subtyping for no-op (i.e. non-upcasting) dyn conversions

This code should be valid in the new solver, like it is with the old solver:

```rust
// -Znext-solver

fn foo<'a>(x: &mut for<'h> dyn Fn(&'h ())) {
   let _: &mut dyn Fn(&'a ()) = x;
}
```

But instead:

```
error: lifetime may not live long enough
 --> <source>:2:11
  |
1 | fn foo<'a>(x: &mut dyn for<'h> Fn(&'h ())) {
  |        -- lifetime `'a` defined here
2 |    let _: &mut dyn Fn(&'a ()) = x;
  |           ^^^^^^^^^^^^^^^^^^^ type annotation requires that `'a` must outlive `'static`
  |
  = note: requirement occurs because of a mutable reference to `dyn Fn(&())`
```

Specifically, this fails because we try to coerce `&mut dyn for<'h> Fn(&'h ())` to `&mut dyn Fn(&'a ())`, which registers an `dyn for<'h> Fn(&'h ()): dyn Fn(&'a ())` goal. This fails because the new solver uses *equating* rather than *subtyping* in `Unsize` goals.

This is *mostly* not a problem... You may wonder why the same code passes on the new solver for immutable references:

```
// -Znext-solver

fn foo<'a>(x: &dyn Fn(&())) {
   let _: &dyn Fn(&'a ()) = x; // works
}
```

That's because in this case, we first try to coerce via `Unsize`, but due to the leak check the goal fails. Then, later in coercion, we fall back to a simple subtyping operation, which *does* work.

Since `&T` is covariant over `T`, but `&mut T` is invariant, that's where the discrepancy between these two examples crops up.

---

r? lcnr or reassign :D
2024-09-28 09:35:09 +02:00
Michael Goulet
d753aba3b3 Get rid of a_is_expected from ToTrace 2024-09-27 15:43:18 -04:00
Michael Goulet
4fb097a5de Instantiate binders when checking supertrait upcasting 2024-09-27 15:43:18 -04:00
Matthias Krüger
a935064fae
Rollup merge of #130826 - fmease:compiler-mv-obj-safe-dyn-compat, r=compiler-errors
Compiler: Rename "object safe" to "dyn compatible"

Completed T-lang FCP: https://github.com/rust-lang/lang-team/issues/286#issuecomment-2338905118.
Tracking issue: https://github.com/rust-lang/rust/issues/130852

Excludes `compiler/rustc_codegen_cranelift` (to be filed separately).
Includes Stable MIR.

Regarding https://github.com/rust-lang/rust/labels/relnotes, I guess I will manually open a https://github.com/rust-lang/rust/labels/relnotes-tracking-issue since this change affects everything (compiler, library, tools, docs, books, everyday language).

r? ghost
2024-09-27 21:35:08 +02:00
Jubilee
6b0c897499
Rollup merge of #130911 - notriddle:notriddle/suggest-wrap-parens-fn-pointer, r=compiler-errors
diagnostics: wrap fn cast suggestions in parens when needed

Fixes #121632
2024-09-26 22:20:56 -07:00
Michael Goulet
d4ee408afc Check allow instantiating object trait binder when upcasting and in new solver 2024-09-26 22:26:29 -04:00
Michael Howell
c48b0d4eb4 diagnostics: wrap fn cast suggestions in parens
Fixes #121632
2024-09-26 18:17:52 -07:00
León Orell Valerian Liehr
01a063f9df
Compiler: Rename "object safe" to "dyn compatible" 2024-09-25 13:26:48 +02:00
Virginia Senioria
986e20d5bb Fixed diagnostics for coroutines with () as input. 2024-09-25 08:45:40 +00:00
Matthias Krüger
2bca5c4fc1
Rollup merge of #130714 - compiler-errors:try-structurally-resolve-const, r=BoxyUwU
Introduce `structurally_normalize_const`, use it in `rustc_hir_typeck`

Introduces `structurally_normalize_const` to typecking to separate the "eval a const" step from the "try to turn a valtree into a target usize" in HIR typeck, where we may still have infer vars and stuff around.

I also changed `check_expr_repeat` to move a double evaluation of a const into a single one. I'll leave inline comments.

r? ```@BoxyUwU```

I hesitated to really test this on the new solver where it probably matters for unevaluated consts. If you're worried about the side-effects, I'd be happy to craft some more tests 😄
2024-09-23 06:45:36 +02:00
Matthias Krüger
82060368e6
Rollup merge of #130712 - compiler-errors:const-eval-error-reporting, r=BoxyUwU
Don't call `ty::Const::normalize` in error reporting

We do this to ensure that trait refs with unevaluated consts have those consts simplified to their evaluated forms. Instead, use `try_normalize_erasing_regions`.

**NOTE:** This has the side-effect of erasing regions from all of our trait refs. If this is too much to review or you think it's too opinionated of a diagnostics change, then I could split out the effective change (i.e. erasing regions from this impl suggestion) into another PR and have someone else review it.
2024-09-23 06:45:34 +02:00
Michael Goulet
c682aa162b Reformat using the new identifier sorting from rustfmt 2024-09-22 19:11:29 -04:00
Michael Goulet
01d19d7be9 Don't call try_eval_target_usize in error reporting 2024-09-22 13:55:06 -04:00
Michael Goulet
8f579497f7 Don't call const normalize in error reporting 2024-09-22 13:55:06 -04:00
Michael Goulet
3b8089a320 Introduce structurally_normalize_const, use it in hir_typeck 2024-09-22 13:54:16 -04:00
bors
1d68e6dd1d Auto merge of #127546 - workingjubilee:5-level-paging-exists, r=saethlin
Correct outdated object size limit

The comment here about 48 bit addresses being enough was written in 2016 but was made incorrect in 2019 by 5-level paging, and then persisted for another 5 years before being noticed and corrected.

The bolding of the "exclusive" part is merely to call attention to something I missed when reading it and doublechecking the math.

try-job: i686-msvc
try-job: test-various
2024-09-21 16:20:10 +00:00
Jubilee Young
325af25c94 TL note: current means target 2024-09-20 10:02:14 -07:00
Lukas Markeffsky
1999d065b7 skip normalizing param env if it is already normalized 2024-09-19 15:56:24 +02:00
Matthias Krüger
09b255d3d4
Rollup merge of #130116 - veera-sivarajan:freeze-suggestions, r=chenyukang
Implement a Method to Seal `DiagInner`'s Suggestions

This PR adds a method on `DiagInner` called `.seal_suggestions()` to prevent new suggestions from being added while preserving existing suggestions.

This is useful because currently there is no way to prevent new suggestions from being added to a diagnostic. `.disable_suggestions()` is the closest but it gets rid of all suggestions before and after the call.

Therefore, `.seal_suggestions()` can be used when, for example, misspelled keyword is detected and reported. In such cases, we may want to prevent other suggestions from being added to the diagnostic, as they would likely be meaningless once the misspelled keyword is identified. For context: https://github.com/rust-lang/rust/pull/129899#discussion_r1741307132

To store an additional state, the type of the `suggestions` field in `DiagInner` was changed into a three variant enum. While this change affects files across different crates, care was taken to preserve the existing code's semantics. This is validated by the fact that all UI tests pass without any modifications.

r? chenyukang
2024-09-18 04:42:31 +02:00
Matthias Krüger
62f2ec9b59
Rollup merge of #130275 - compiler-errors:extern-crate, r=lcnr
Don't call `extern_crate` when local crate name is the same as a dependency and we have a trait error

#124944 implemented logic to point out when a trait bound failure involves a *trait* and *type* who come from identically named but different crates. This logic calls the `extern_crate` query which is not valid on `LOCAL_CRATE` cnum, so let's filter that out eagerly.

Fixes #130272
Fixes #129184
2024-09-17 17:28:33 +02:00
León Orell Valerian Liehr
03e8b6bbfa
Rollup merge of #130294 - nnethercote:more-lifetimes, r=lcnr
Lifetime cleanups

The last commit is very opinionated, let's see how we go.

r? `@oli-obk`
2024-09-14 18:12:13 +02:00
Stuart Cook
517e7ce37f
Rollup merge of #130311 - heiseish:issue-70849-fix, r=fmease
(fix) conflicting negative impl marker

## Context

This MR fixes the error message for conflicting negative trait impls by adding the corresponding the polarity marker to the trait name.

## Issues

- closes #70849

r​? `@fmease`
2024-09-14 20:22:41 +10:00
Giang Dao
b0db3a7bed (fix) conflicting negative impl marker and add tests 2024-09-14 09:25:06 +08:00
Nicholas Nethercote
8d32578fe1 Rename and reorder lots of lifetimes.
- Replace non-standard names like 's, 'p, 'rg, 'ck, 'parent, 'this, and
  'me with vanilla 'a. These are cases where the original name isn't
  really any more informative than 'a.
- Replace names like 'cx, 'mir, and 'body with vanilla 'a when the lifetime
  applies to multiple fields and so the original lifetime name isn't
  really accurate.
- Put 'tcx last in lifetime lists, and 'a before 'b.
2024-09-13 15:46:20 +10:00
Veera
741005792e Implement a Method to Seal DiagInner's Suggestions 2024-09-12 21:27:44 -04:00
Matthias Krüger
cb1d80d1e5
Rollup merge of #130273 - lcnr:overflow-no-constraints, r=compiler-errors
more eagerly discard constraints on overflow

We always discard the results of overflowing goals inside of the trait solver. We previously did so when instantiating the response in `evaluate_goal`. Canonicalizing results only to later discard them is also  inefficient 🤷

It's simpler and nicer to debug to eagerly discard constraints inside of the query itself.

r? ``@compiler-errors``
2024-09-12 19:03:43 +02:00
Michael Goulet
9d5d03b7de Don't call extern_crate when local crate name is the same as a dependency and we have a trait error 2024-09-12 09:07:44 -04:00
bors
394c4060d2 Auto merge of #130269 - Zalathar:rollup-coxzt2t, r=Zalathar
Rollup of 8 pull requests

Successful merges:

 - #125060 (Expand documentation of PathBuf, discussing lack of sanitization)
 - #129367 (Fix default/minimum deployment target for Aarch64 simulator targets)
 - #130156 (Add test for S_OBJNAME & update test for LF_BUILDINFO cl and cmd)
 - #130160 (Fix `slice::first_mut` docs)
 - #130235 (Simplify some nested `if` statements)
 - #130250 (Fix `clippy::useless_conversion`)
 - #130252 (Properly report error on `const gen fn`)
 - #130256 (Re-run coverage tests if `coverage-dump` was modified)

r? `@ghost`
`@rustbot` modify labels: rollup
2024-09-12 12:56:55 +00:00
lcnr
675c99f4d5 more eagerly discard constraints on overflow 2024-09-12 14:32:44 +02:00
Stuart Cook
57020e0f8c
Rollup merge of #130250 - compiler-errors:useless-conversion, r=jieyouxu
Fix `clippy::useless_conversion`

Self-explanatory. Probably the last clippy change I'll actually put up since this is the only other one I've actually seen in the wild.
2024-09-12 20:37:17 +10:00
Michael Goulet
e866f8a97d Revert 'Stabilize -Znext-solver=coherence' 2024-09-11 17:57:04 -04:00
Michael Goulet
6d064295c8 clippy::useless_conversion 2024-09-11 17:52:53 -04:00
Michael Goulet
af8d911d63 Also fix if in else 2024-09-11 17:24:01 -04:00
Michael Goulet
954419aab0 Simplify some nested if statements 2024-09-11 13:45:23 -04:00
Jubilee
68ae3b27f5
Rollup merge of #130149 - GrigorenkoPV:lifetime-suggestion, r=cjgillot
Helper function for formatting with `LifetimeSuggestionPosition`
2024-09-09 19:20:38 -07:00
Matthias Krüger
3b0221bf63
Rollup merge of #130137 - gurry:master, r=cjgillot
Fix ICE caused by missing span in a region error

Fixes #130012

The ICE occurs on line 634 in this error handling code: 085744b7ad/compiler/rustc_trait_selection/src/error_reporting/infer/region.rs (L617-L637) It is caused by the span being a dummy span and `!span.is_dummy()` on line 628 evaluating to `false`.

A dummy span, however, is expected here thanks to the `Self: Trait` predicate from `predicates_of` (see line 61): 085744b7ad/compiler/rustc_hir_analysis/src/collect/predicates_of.rs (L61-L69)

This PR changes the error handling code to omit the note which needed the span instead of ICE'ing in the presence of a dummy span.
2024-09-09 20:20:20 +02:00
Pavel Grigorenko
db6361184e Helper function for formatting with LifetimeSuggestionPosition 2024-09-09 14:39:04 +03:00
Jubilee
09373b997d
Rollup merge of #130070 - gurry:rename-regionkind-addof-to-ref, r=compiler-errors
Rename variant `AddrOfRegion` of `RegionVariableOrigin` to `BorrowRegion`

because "Borrow" is the more idiomatic Rust term than "AddrOf".
2024-09-09 00:17:49 -07:00
Gurinder Singh
0f8efb3b5c Fix ICE caused by missing span in a region error 2024-09-09 12:27:36 +05:30
bors
ec867f03bc Auto merge of #126161 - Bryanskiy:delegation-generics-4, r=petrochenkov
Delegation: support generics in associated delegation items

This is a continuation of https://github.com/rust-lang/rust/pull/125929.

[design](https://github.com/Bryanskiy/posts/blob/master/delegation%20in%20generic%20contexts.md)

Generic parameters inheritance was implemented in all contexts. Generic arguments are not yet supported.

r? `@petrochenkov`
2024-09-07 18:12:05 +00:00
Gurinder Singh
c0b06273f2 Rename variant AddrOfRegion of RegionVariableOrigin to BorrowRegion
because "Borrow" is the more idiomatic Rust term than "AddrOf".
2024-09-07 18:50:51 +05:30
bors
26b5599e4d Auto merge of #128776 - Bryanskiy:deep-reject-ctxt, r=lcnr
Use `DeepRejectCtxt` to quickly reject `ParamEnv` candidates

The description is on the [zulip thread](https://rust-lang.zulipchat.com/#narrow/stream/144729-t-types/topic/.5Basking.20for.20help.5D.20.60DeepRejectCtxt.60.20for.20param.20env.20candidates)

r? `@lcnr`
2024-09-06 19:50:48 +00:00
bors
17b322fa69 Auto merge of #121848 - lcnr:stabilize-next-solver, r=compiler-errors
stabilize `-Znext-solver=coherence`

r? `@compiler-errors`

---

This PR stabilizes the use of the next generation trait solver in coherence checking by enabling `-Znext-solver=coherence` by default. More specifically its use in the *implicit negative overlap check*. The tracking issue for this is https://github.com/rust-lang/rust/issues/114862. Closes #114862.

## Background

### The next generation trait solver

The new solver lives in [`rustc_trait_selection::solve`](https://github.com/rust-lang/rust/blob/master/compiler/rustc_trait_selection/src/solve/mod.rs) and is intended to replace the existing *evaluate*, *fulfill*, and *project* implementation. It also has a wider impact on the rest of the type system, for example by changing our approach to handling associated types.

For a more detailed explanation of the new trait solver, see the [rustc-dev-guide](https://rustc-dev-guide.rust-lang.org/solve/trait-solving.html). This does not stabilize the current behavior of the new trait solver, only the behavior impacting the implicit negative overlap check. There are many areas in the new solver which are not yet finalized. We are confident that their final design will not conflict with the user-facing behavior observable via coherence. More on that further down.

Please check out [the chapter](https://rustc-dev-guide.rust-lang.org/solve/significant-changes.html) summarizing the most significant changes between the existing and new implementations.

### Coherence and the implicit negative overlap check

Coherence checking detects any overlapping impls. Overlapping trait impls always error while overlapping inherent impls result in an error if they have methods with the same name. Coherence also results in an error if any other impls could exist, even if they are currently unknown. This affects impls which may get added to upstream crates in a backwards compatible way and impls from downstream crates.

Coherence failing to detect overlap is generally considered to be unsound, even if it is difficult to actually get runtime UB this way. It is quite easy to get ICEs due to bugs in coherence.

It currently consists of two checks:

The [orphan check] validates that impls do not overlap with other impls we do not know about: either because they may be defined in a sibling crate, or because an upstream crate is allowed to add it without being considered a breaking change.

The [overlap check] validates that impls do not overlap with other impls we know about. This is done as follows:
- Instantiate the generic parameters of both impls with inference variables
- Equate the `TraitRef`s of both impls. If it fails there is no overlap.
- [implicit negative]: Check whether any of the instantiated `where`-bounds of one of the impls definitely do not hold when using the constraints from the previous step. If a `where`-bound does not hold, there is no overlap.
- *explicit negative (still unstable, ignored going forward)*: Check whether the any negated `where`-bounds can be proven, e.g. a `&mut u32: Clone` bound definitely does not hold as an explicit `impl<T> !Clone for &mut T` exists.

The overlap check has to *prove that unifying the impls does not succeed*. This means that **incorrectly getting a type error during coherence is unsound** as it would allow impls to overlap: coherence has to be *complete*.

Completeness means that we never incorrectly error. This means that during coherence we must only add inference constraints if they are definitely necessary. During ordinary type checking [this does not hold](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=01d93b592bd9036ac96071cbf1d624a9), so the trait solver has to behave differently, depending on whether we're in coherence or not.

The implicit negative check only considers goals to "definitely not hold" if they could not be implemented downstream, by a sibling, or upstream in a backwards compatible way. If the goal is is "unknowable" as it may get added in another crate, we add an ambiguous candidate: [source](bea5bebf3d/compiler/rustc_trait_selection/src/solve/assembly/mod.rs (L858-L883)).

[orphan check]: fd80c02c16/compiler/rustc_trait_selection/src/traits/coherence.rs (L566-L579)
[overlap check]: fd80c02c16/compiler/rustc_trait_selection/src/traits/coherence.rs (L92-L98)
[implicit negative]: fd80c02c16/compiler/rustc_trait_selection/src/traits/coherence.rs (L223-L281)

## Motivation

Replacing the existing solver in coherence fixes soundness bugs by removing sources of incompleteness in the type system. The new solver separately strengthens coherence, resulting in more impls being disjoint and passing the coherence check. The concrete changes will be elaborated further down. We believe the stabilization to reduce the likelihood of future bugs in coherence as the new implementation is easier to understand and reason about.

It allows us to remove the support for coherence and implicit-negative reasoning in the old solver, allowing us to remove some code and simplifying the old trait solver. We will only remove the old solver support once this stabilization has reached stable to make sure we're able to quickly revert in case any unexpected issues are detected before then.

Stabilizing the use of the next-generation trait solver expresses our confidence that its current behavior is intended and our work towards enabling its use everywhere will not require any breaking changes to the areas used by coherence checking. We are also confident that we will be able to replace the existing solver everywhere, as maintaining two separate systems adds a significant maintainance burden.

## User-facing impact and reasoning

### Breakage due to improved handling of associated types

The new solver fixes multiple issues related to associated types. As these issues caused coherence to consider more types distinct, fixing them results in more overlap errors. This is therefore a breaking change.

#### Structurally relating aliases containing bound vars

Fixes https://github.com/rust-lang/rust/issues/102048. In the existing solver relating ambiguous projections containing bound variables is structural. This is *incomplete* and allows overlapping impls. These was mostly not exploitable as the same issue also caused impls to not apply when trying to use them. The new solver defers alias-relating to a nested goal, fixing this issue:
```rust
// revisions: current next
//[next] compile-flags: -Znext-solver=coherence
trait Trait {}

trait Project {
    type Assoc<'a>;
}

impl Project for u32 {
    type Assoc<'a> = &'a u32;
}

// Eagerly normalizing `<?infer as Project>::Assoc<'a>` is ambiguous,
// so the old solver ended up structurally relating
//
//     (?infer, for<'a> fn(<?infer as Project>::Assoc<'a>))
//
// with
//
//     ((u32, fn(&'a u32)))
//
// Equating `&'a u32` with `<u32 as Project>::Assoc<'a>` failed, even
// though these types are equal modulo normalization.
impl<T: Project> Trait for (T, for<'a> fn(<T as Project>::Assoc<'a>)) {}

impl<'a> Trait for (u32, fn(&'a u32)) {}
//[next]~^ ERROR conflicting implementations of trait `Trait` for type `(u32, for<'a> fn(&'a u32))`
```

A crater run did not discover any breakage due to this change.

#### Unknowable candidates for higher ranked trait goals

This avoids an unsoundness by attempting to normalize in `trait_ref_is_knowable`, fixing https://github.com/rust-lang/rust/issues/114061. This is a side-effect of supporting lazy normalization, as that forces us to attempt to normalize when checking whether a `TraitRef` is knowable: [source](47dd709bed/compiler/rustc_trait_selection/src/solve/assembly/mod.rs (L754-L764)).

```rust
// revisions: current next
//[next] compile-flags: -Znext-solver=coherence
trait IsUnit {}
impl IsUnit for () {}

pub trait WithAssoc<'a> {
    type Assoc;
}

// We considered `for<'a> <T as WithAssoc<'a>>::Assoc: IsUnit`
// to be knowable, even though the projection is ambiguous.
pub trait Trait {}
impl<T> Trait for T
where
    T: 'static,
    for<'a> T: WithAssoc<'a>,
    for<'a> <T as WithAssoc<'a>>::Assoc: IsUnit,
{
}
impl<T> Trait for Box<T> {}
//[next]~^ ERROR conflicting implementations of trait `Trait`
```
The two impls of `Trait` overlap given the following downstream crate:
```rust
use dep::*;
struct Local;
impl WithAssoc<'_> for Box<Local> {
    type Assoc = ();
}
```

There a similar coherence unsoundness caused by our handling of aliases which is fixed separately in https://github.com/rust-lang/rust/pull/117164.

This change breaks the [`derive-visitor`](https://crates.io/crates/derive-visitor) crate. I have opened an issue in that repo: nikis05/derive-visitor#16.

### Evaluating goals to a fixpoint and applying inference constraints

In the old implementation of the implicit-negative check, each obligation is [checked separately without applying its inference constraints](bea5bebf3d/compiler/rustc_trait_selection/src/traits/coherence.rs (L323-L338)). The new solver instead [uses a `FulfillmentCtxt`](bea5bebf3d/compiler/rustc_trait_selection/src/traits/coherence.rs (L315-L321)) for this, which evaluates all obligations in a loop until there's no further inference progress.

This is necessary for backwards compatibility as we do not eagerly normalize with the new solver, resulting in constraints from normalization to only get applied by evaluating a separate obligation. This also allows more code to compile:
```rust
// revisions: current next
//[next] compile-flags: -Znext-solver=coherence
trait Mirror {
    type Assoc;
}
impl<T> Mirror for T {
    type Assoc = T;
}

trait Foo {}
trait Bar {}

// The self type starts out as `?0` but is constrained to `()`
// due to the where-clause below. Because `(): Bar` is known to
// not hold, we can prove the impls disjoint.
impl<T> Foo for T where (): Mirror<Assoc = T> {}
//[current]~^ ERROR conflicting implementations of trait `Foo` for type `()`
impl<T> Foo for T where T: Bar {}

fn main() {}
```
The old solver does not run nested goals to a fixpoint in evaluation. The new solver does do so, strengthening inference and improving the overlap check:
```rust
// revisions: current next
//[next] compile-flags: -Znext-solver=coherence
trait Foo {}
impl<T> Foo for (u8, T, T) {}
trait NotU8 {}
trait Bar {}
impl<T, U: NotU8> Bar for (T, T, U) {}

trait NeedsFixpoint {}
impl<T: Foo + Bar> NeedsFixpoint for T {}
impl NeedsFixpoint for (u8, u8, u8) {}

trait Overlap {}
impl<T: NeedsFixpoint> Overlap for T {}
impl<T, U: NotU8, V> Overlap for (T, U, V) {}
//[current]~^ ERROR conflicting implementations of trait `Foo`
```

### Breakage due to removal of incomplete candidate preference

Fixes #107887. In the old solver we incompletely prefer the builtin trait object impl over user defined impls. This can break inference guidance, inferring `?x` in `dyn Trait<u32>: Trait<?x>` to `u32`, even if an explicit impl of `Trait<u64>` also exists.

This caused coherence to incorrectly allow overlapping impls, resulting in ICEs and a theoretical unsoundness. See https://github.com/rust-lang/rust/issues/107887#issuecomment-1997261676. This compiles on stable but results in an overlap error with `-Znext-solver=coherence`:

```rust
// revisions: current next
//[next] compile-flags: -Znext-solver=coherence
struct W<T: ?Sized>(*const T);

trait Trait<T: ?Sized> {
    type Assoc;
}

// This would trigger the check for overlap between automatic and custom impl.
// They actually don't overlap so an impl like this should remain possible
// forever.
//
// impl Trait<u64> for dyn Trait<u32> {}
trait Indirect {}
impl Indirect for dyn Trait<u32, Assoc = ()> {}
impl<T: Indirect + ?Sized> Trait<u64> for T {
    type Assoc = ();
}

// Incomplete impl where `dyn Trait<u32>: Trait<_>` does not hold, but
// `dyn Trait<u32>: Trait<u64>` does.
trait EvaluateHack<U: ?Sized> {}
impl<T: ?Sized, U: ?Sized> EvaluateHack<W<U>> for T
where
    T: Trait<U, Assoc = ()>, // incompletely constrains `_` to `u32`
    U: IsU64,
    T: Trait<U, Assoc = ()>, // incompletely constrains `_` to `u32`
{
}

trait IsU64 {}
impl IsU64 for u64 {}

trait Overlap<U: ?Sized> {
    type Assoc: Default;
}
impl<T: ?Sized + EvaluateHack<W<U>>, U: ?Sized> Overlap<U> for T {
    type Assoc = Box<u32>;
}
impl<U: ?Sized> Overlap<U> for dyn Trait<u32, Assoc = ()> {
//[next]~^ ERROR conflicting implementations of trait `Overlap<_>`
    type Assoc = usize;
}
```

### Considering region outlives bounds in the `leak_check`

For details on the `leak_check`, see the FCP proposal in #119820.[^leak_check]

[^leak_check]: which should get moved to the dev-guide once that PR lands :3

In both coherence and during candidate selection, the `leak_check` relies on the region constraints added in `evaluate`. It therefore currently does not register outlives obligations: [source](ccb1415eac/compiler/rustc_trait_selection/src/traits/select/mod.rs (L792-L810)). This was likely done as a performance optimization without considering its impact on the `leak_check`. This is the case as in the old solver, *evaluatation* and *fulfillment* are split, with evaluation being responsible for candidate selection and fulfillment actually registering all the constraints.

This split does not exist with the new solver. The `leak_check` can therefore eagerly detect errors caused by region outlives obligations. This improves both coherence itself and candidate selection:

```rust
// revisions: current next
//[next] compile-flags: -Znext-solver=coherence
trait LeakErr<'a, 'b> {}
// Using this impl adds an `'b: 'a` bound which results
// in a higher-ranked region error. This bound has been
// previously ignored but is now considered.
impl<'a, 'b: 'a> LeakErr<'a, 'b> for () {}

trait NoOverlapDir<'a> {}
impl<'a, T: for<'b> LeakErr<'a, 'b>> NoOverlapDir<'a> for T {}
impl<'a> NoOverlapDir<'a> for () {}
//[current]~^ ERROR conflicting implementations of trait `NoOverlapDir<'_>`

// --------------------------------------

// necessary to avoid coherence unknowable candidates
struct W<T>(T);

trait GuidesSelection<'a, U> {}
impl<'a, T: for<'b> LeakErr<'a, 'b>> GuidesSelection<'a, W<u32>> for T {}
impl<'a, T> GuidesSelection<'a, W<u8>> for T {}

trait NotImplementedByU8 {}
trait NoOverlapInd<'a, U> {}
impl<'a, T: GuidesSelection<'a, W<U>>, U> NoOverlapInd<'a, U> for T {}
impl<'a, U: NotImplementedByU8> NoOverlapInd<'a, U> for () {}
//[current]~^ conflicting implementations of trait `NoOverlapInd<'_, _>`
```

### Removal of `fn match_fresh_trait_refs`

The old solver tries to [eagerly detect unbounded recursion](b14fd2359f/compiler/rustc_trait_selection/src/traits/select/mod.rs (L1196-L1211)), forcing the affected goals to be ambiguous. This check is only an approximation and has not been added to the new solver.

The check is not necessary in the new solver and it would be problematic for caching. As it depends on all goals currently on the stack, using a global cache entry would have to always make sure that doing so does not circumvent this check.

This changes some goals to error - or succeed - instead of failing with ambiguity. This allows more code to compile:

```rust
// revisions: current next
//[next] compile-flags: -Znext-solver=coherence

// Need to use this local wrapper for the impls to be fully
// knowable as unknowable candidate result in ambiguity.
struct Local<T>(T);

trait Trait<U> {}
// This impl does not hold, but is ambiguous in the old
// solver due to its overflow approximation.
impl<U> Trait<U> for Local<u32> where Local<u16>: Trait<U> {}
// This impl holds.
impl Trait<Local<()>> for Local<u8> {}

// In the old solver, `Local<?t>: Trait<Local<?u>>` is ambiguous,
// resulting in `Local<?u>: NoImpl`, also being ambiguous.
//
// In the new solver the first impl does not apply, constraining
// `?u` to `Local<()>`, causing `Local<()>: NoImpl` to error.
trait Indirect<T> {}
impl<T, U> Indirect<U> for T
where
    T: Trait<U>,
    U: NoImpl
{}

// Not implemented for `Local<()>`
trait NoImpl {}
impl NoImpl for Local<u8> {}
impl NoImpl for Local<u16> {}

// `Local<?t>: Indirect<Local<?u>>` cannot hold, so
// these impls do not overlap.
trait NoOverlap<U> {}
impl<T: Indirect<U>, U> NoOverlap<U> for T {}
impl<T, U> NoOverlap<Local<U>> for Local<T> {}
//~^ ERROR conflicting implementations of trait `NoOverlap<Local<_>>`
```

### Non-fatal overflow

The old solver immediately emits a fatal error when hitting the recursion limit. The new solver instead returns overflow. This both allows more code to compile and is results in performance and potential future compatability issues.

Non-fatal overflow is generally desirable. With fatal overflow, changing the order in which we evaluate nested goals easily causes breakage if we have goal which errors and one which overflows. It is also required to prevent breakage due to the removal of `fn match_fresh_trait_refs`, e.g. [in `typenum`](https://github.com/rust-lang/trait-system-refactor-initiative/issues/73).

#### Enabling more code to compile

In the below example, the old solver first tried to prove an overflowing goal, resulting in a fatal error. The new solver instead returns ambiguity due to overflow for that goal, causing the implicit negative overlap check to succeed as `Box<u32>: NotImplemented` does not hold.
```rust
// revisions: current next
//[next] compile-flags: -Znext-solver=coherence
//[current] ERROR overflow evaluating the requirement

trait Indirect<T> {}
impl<T: Overflow<()>> Indirect<T> for () {}

trait Overflow<U> {}
impl<T, U> Overflow<U> for Box<T>
where
    U: Indirect<Box<Box<T>>>,
{}

trait NotImplemented {}

trait Trait<U> {}
impl<T, U> Trait<U> for T
where
    // T: NotImplemented, // causes old solver to succeed
    U: Indirect<T>,
    T: NotImplemented,
{}

impl Trait<()> for Box<u32> {}
```

#### Avoiding hangs with non-fatal overflow

Simply returning ambiguity when reaching the recursion limit can very easily result in hangs, e.g.
```rust
trait Recur {}
impl<T, U> Recur for ((T, U), (U, T))
where
    (T, U): Recur,
    (U, T): Recur,
{}

trait NotImplemented {}
impl<T: NotImplemented> Recur for T {}
```
This can happen quite frequently as it's easy to have exponential blowup due to multiple nested goals at each step. As the trait solver is depth-first, this immediately caused a fatal overflow error in the old solver. In the new solver we have to handle the whole proof tree instead, which can very easily hang.

To avoid this we restrict the recursion depth after hitting the recursion limit for the first time. We also **ignore all inference constraints from goals resulting in overflow**. This is mostly backwards compatible as any overflow in the old solver resulted in a fatal error.

### sidenote about normalization

We return ambiguous nested goals of `NormalizesTo` goals to the caller and ignore their impact when computing the `Certainty` of the current goal. See the [normalization chapter](https://rustc-dev-guide.rust-lang.org/solve/normalization.html) for more details.This means we apply constraints resulting from other nested goals and from equating the impl header when normalizing, even if a nested goal results in overflow. This is necessary to avoid breaking the following example:
```rust
trait Trait {
    type Assoc;
}

struct W<T: ?Sized>(*mut T);
impl<T: ?Sized> Trait for W<W<T>>
where
    W<T>: Trait,
{
    type Assoc = ();
}

// `W<?t>: Trait<Assoc = u32>` does not hold as
// `Assoc` gets normalized to `()`. However, proving
// the where-bounds of the impl results in overflow.
//
// For this to continue to compile we must not discard
// constraints from normalizing associated types.
trait NoOverlap {}
impl<T: Trait<Assoc = u32>> NoOverlap for T {}
impl<T: ?Sized> NoOverlap for W<T> {}
```

#### Future compatability concerns

Non-fatal overflow results in some unfortunate future compatability concerns. Changing the approach to avoid more hangs by more strongly penalizing overflow can cause breakage as we either drop constraints or ignore candidates necessary to successfully compile. Weakening the overflow penalities instead allows more code to compile and strengthens inference while potentially causing more code to hang.

While the current approach is not perfect, we believe it to be good enough. We believe it to apply the necessary inference constraints to avoid breakage and expect there to not be any desirable patterns broken by our current penalities. Similarly we believe the current constraints to avoid most accidental hangs. Ignoring constraints of overflowing goals is especially useful, as it may allow major future optimizations to our overflow handling. See [this summary](https://hackmd.io/ATf4hN0NRY-w2LIVgeFsVg) and the linked documents in case you want to know more.

### changes to performance

In general, trait solving during coherence checking is not significant for performance. Enabling the next-generation trait solver in coherence does not impact our compile time benchmarks. We are still unable to compile the benchmark suite when fully enabling the new trait solver.

There are rare cases where the new solver has significantly worse performance due to non-fatal overflow, its reliance on fixpoint algorithms and the removal of the `fn match_fresh_trait_refs` approximation. We encountered such issues in [`typenum`](https://crates.io/crates/typenum) and believe it should be [pretty much as bad as it can get](https://github.com/rust-lang/trait-system-refactor-initiative/issues/73).

Due to an improved structure and far better caching, we believe that there is a lot of room for improvement and that the new solver will outperform the existing implementation in nearly all cases, sometimes significantly. We have not yet spent any time micro-optimizing the implementation and have many unimplemented major improvements, such as fast-paths for trivial goals.

TODO: get some rough results here and put them in a table

### Unstable features

#### Unsupported unstable features

The new solver currently does not support all unstable features, most notably `#![feature(generic_const_exprs)]`, `#![feature(associated_const_equality)]` and `#![feature(adt_const_params)]` are not yet fully supported in the new solver. We are confident that supporting them is possible, but did not consider this to be a priority. This stabilization introduces new ICE when using these features in impl headers.

#### fixes to `#![feature(specialization)]`

- fixes #105782
- fixes #118987

#### fixes to `#![feature(type_alias_impl_trait)]`

- fixes #119272
- https://github.com/rust-lang/rust/issues/105787#issuecomment-1750112388
- fixes #124207

## This does not stabilize the whole solver

While this stabilizes the use of the new solver in coherence checking, there are many parts of the solver which will remain fully unstable. We may still adapt these areas while working towards stabilizing the new solver everywhere. We are confident that we are able to do so without negatively impacting coherence.

### goals with a non-empty `ParamEnv`

Coherence always uses an empty environment. We therefore do not depend on the behavior of `AliasBound` and `ParamEnv` candidates. We only stabilizes the behavior of user-defined and builtin implementations of traits. There are still many open questions there.

### opaque types in the defining scope

The handling of opaque types - `impl Trait` - in both the new and old solver is still not fully figured out. Luckily this can be ignored for now. While opaque types are reachable during coherence checking by using `impl_trait_in_associated_types`, the behavior during coherence is separate and self-contained. The old and new solver fully agree here.

### normalization is hard

This stabilizes that we equate associated types involving bound variables using deferred-alias-equality. We also stop eagerly normalizing in coherence, which should not have any user-facing impact.

We do not stabilize the normalization behavior outside of coherence, e.g. we currently deeply normalize all types during writeback with the new solver. This may change going forward

### how to replace `select` from the old solver

We sometimes depend on getting a single `impl` for a given trait bound, e.g. when resolving a concrete method for codegen/CTFE. We do not depend on this during coherence, so the exact approach here can still be freely changed going forward.

## Acknowledgements

This work would not have been possible without `@compiler-errors.` He implemented large chunks of the solver himself but also and did a lot of testing and experimentation, eagerly discovering multiple issues which had a significant impact on our approach. `@BoxyUwU` has also done some amazing work on the solver. Thank you for the endless hours of discussion resulting in the current approach. Especially the way aliases are handled has gone through multiple revisions to get to its current state.

There were also many contributions from - and discussions with - other members of the community and the rest of `@rust-lang/types.` This solver builds upon previous improvements to the compiler, as well as lessons learned from `chalk` and `a-mir-formality`. Getting to this point  would not have been possible without that and I am incredibly thankful to everyone involved. See the [list of relevant PRs](https://github.com/rust-lang/rust/pulls?q=is%3Apr+is%3Amerged+label%3AWG-trait-system-refactor+-label%3Arollup+closed%3A%3C2024-03-22+).
2024-09-06 13:12:14 +00:00