Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

solver cycles are coinductive once they have one coinductive step #136824

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Feb 10, 2025

Implements the new cycle semantics in the new solver, dealing with the fallout from rust-lang/trait-system-refactor-initiative#10.

The first commit has been extensively fuzzed via https://github.com/lcnr/search_graph_fuzz.

A trait solver cycle is now coinductive if it has at least one coinductive step. A step is only considered coinductive if it's a where-clause of an impl of a coinductive trait. The only coinductive traits are Sized and auto traits.

This differs from the current stable because where a cycle had to consist of exclusively coinductive goals. This is overly limiting and wasn't properly enforced as it (mostly) ignored all non-trait goals.

A more in-depth explanation of my reasoning can be found in this separate doc: https://gist.github.com/lcnr/c49d887bbd34f5d05c36d1cf7a1bf5a5. A summary:

  • imagine using dictionary passing style: map where-bounds to additional "dictonary" fn arguments instead of monomorphization
  • impls are the only source of truth and introduce a constructor of the dictionary type
  • a trait goal holds if mapping its proof tree to dictionary passing style results in a valid corecursive function
  • a corecursive function is valid if it is guarded: matching on it should result in a constructor in a finite amount of time. This property should recursively hold for all fields of the constructor
    • a function is guarded if the recursive call is behind a constructor
    • and this constructor is not moved out of, e.g. by accessing a field of the dictionary
  • the "not moved out of" condition is difficult to guarantee in general, e.g. for item bounds of associated types. However, there is no way to move out of an auto trait as there is no information you can get from the inside of an auto trait bound in the trait system
  • if we encounter a cycle/recursive call which involves an auto trait, we can always convert the proof tree into a non-recursive function which calls a corecursive function whose first step is the construction of the auto trait dict and which only recursively depends on itself (by inlining the original function until they reach the uses of the auto trait)

we can therefore make any cycle during which we step into an auto trait (or Sized) impl coinductive


To fix rust-lang/trait-system-refactor-initiative#10 we could go with a more restrictive version which tries to restrict cycles to only allow code already supported on stable, potentially forcing cycles to be ambiguous if they step through an impl-where clause of a non-coinductive trait.

PathKind should be a strictly ordered set to allow merging paths without worry. We could therefore add another variant PathKind::ForceUnknown which is greater than PathKind::Coinductive. We already have to add such a third PathKind in #137314 anyways.

I am not doing this here due to multiple reasons:

  • I cannot think of a principled reason why cycles using an impl to normalize differ in any way from simply using that impl to prove a trait bound. It feels unnecessary and like it makes it more difficult to reason about our cycle semantics :<
  • This PR does not affect stable as coherence doesn't care about whether a goal holds or is ambiguous. So we don't yet have to make a final decision

r? @compiler-errors @nikomatsakis

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Feb 10, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2025
[DO NOT MERGE] bootstrap with `-Znext-solver=globally`

A revival of rust-lang#124812.

Current status:

~~`./x.py b --stage 2` passes 🎉~~

`try` builds succeed 🎉 🎉 🎉

[first perf run](rust-lang#133502 (comment)) 👻

### in-flight changes

- ce66d92 is a rebased version of rust-lang#125334, unsure whether I actually want to land this PR for now
- rust-lang#136824

r? `@ghost`
@rust-log-analyzer

This comment has been minimized.

@lcnr
Copy link
Contributor Author

lcnr commented Feb 13, 2025

@bors try

@bors
Copy link
Contributor

bors commented Feb 13, 2025

⌛ Trying commit 68f2802 with merge d5c3c24...

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 13, 2025
solver cycles are coinductive once they have one coinductive step

Implements the new cycle semantics in the new solver, dealing with the fallout from rust-lang/trait-system-refactor-initiative#10.

I am currently also changing inductive cycles back to an error instead of ambiguity outside of coherence to deal with rust-lang/trait-system-refactor-initiative#114. This should allow nalgebra to compile without affecting anything on stable. Whether a cycle results in ambiguity or success should not matter for coherence, as it only checks for errors.

The first commit has been extensively fuzzed via https://github.com/lcnr/search_graph_fuzz.

TODO:
- [ ] fix issues from https://hackmd.io/JsblAvk4R5y30niSNQVYeA
- [ ] add ui tests
- [ ] explain this approach and why we believe it to be correct

r? `@compiler-errors` `@nikomatsakis`
@bors
Copy link
Contributor

bors commented Feb 13, 2025

☀️ Try build successful - checks-actions
Build commit: d5c3c24 (d5c3c24fef4826690d53e1b54dda32ac9008633b)

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Feb 14, 2025
…rors

rework rigid alias handling

Necessary for rust-lang#136824 if we treat coinductive cycles as errors as we otherwise don't emit an error for

```rust
trait Overflow {
    type Assoc;
}
impl<T> Overflow for T {
    type Assoc = <T as Overflow>::Assoc;
}
```

The important part is that we only add a `RigidAlias` candidate in cases where the alias is actually supposed to be rigid:
- its trait bound has been proven via a `ParamEnv` or `ItemBound` candidate
- it's one of the special builtin traits which have a blanket impl with a `default` assoc type

This means that we now more explicitly control which aliases should rigid to avoid accidentally accepting cyclic aliases. This requires changes to diagnostics as we no longer enter an explicit `RigidAlias` candidate for `NormalizesTo` goals whose trait bound doesn't hold.

To fix this I've modified the `BestObligation` visitor always ignore `RigidAlias` candidates and to instead manually check these requirements if there are no applicable candidates. I also removed the hack for handling `structurally_normalize_ty` failures. This fixes rust-lang#134905 as we no longer continue to use the `EvalCtxt` even though a nested goal failed.

r? `@compiler-errors`
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Feb 14, 2025
…rors

rework rigid alias handling

Necessary for rust-lang#136824 if we treat coinductive cycles as errors as we otherwise don't emit an error for

```rust
trait Overflow {
    type Assoc;
}
impl<T> Overflow for T {
    type Assoc = <T as Overflow>::Assoc;
}
```

The important part is that we only add a `RigidAlias` candidate in cases where the alias is actually supposed to be rigid:
- its trait bound has been proven via a `ParamEnv` or `ItemBound` candidate
- it's one of the special builtin traits which have a blanket impl with a `default` assoc type

This means that we now more explicitly control which aliases should rigid to avoid accidentally accepting cyclic aliases. This requires changes to diagnostics as we no longer enter an explicit `RigidAlias` candidate for `NormalizesTo` goals whose trait bound doesn't hold.

To fix this I've modified the `BestObligation` visitor always ignore `RigidAlias` candidates and to instead manually check these requirements if there are no applicable candidates. I also removed the hack for handling `structurally_normalize_ty` failures. This fixes rust-lang#134905 as we no longer continue to use the `EvalCtxt` even though a nested goal failed.

r? ``@compiler-errors``
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 14, 2025
Rollup merge of rust-lang#136863 - lcnr:treat-as-rigid, r=compiler-errors

rework rigid alias handling

Necessary for rust-lang#136824 if we treat coinductive cycles as errors as we otherwise don't emit an error for

```rust
trait Overflow {
    type Assoc;
}
impl<T> Overflow for T {
    type Assoc = <T as Overflow>::Assoc;
}
```

The important part is that we only add a `RigidAlias` candidate in cases where the alias is actually supposed to be rigid:
- its trait bound has been proven via a `ParamEnv` or `ItemBound` candidate
- it's one of the special builtin traits which have a blanket impl with a `default` assoc type

This means that we now more explicitly control which aliases should rigid to avoid accidentally accepting cyclic aliases. This requires changes to diagnostics as we no longer enter an explicit `RigidAlias` candidate for `NormalizesTo` goals whose trait bound doesn't hold.

To fix this I've modified the `BestObligation` visitor always ignore `RigidAlias` candidates and to instead manually check these requirements if there are no applicable candidates. I also removed the hack for handling `structurally_normalize_ty` failures. This fixes rust-lang#134905 as we no longer continue to use the `EvalCtxt` even though a nested goal failed.

r? ``@compiler-errors``
@lcnr lcnr force-pushed the yeet branch 4 times, most recently from e8a5870 to 4720589 Compare February 17, 2025 14:52
@lcnr lcnr marked this pull request as ready for review February 18, 2025 10:50
@rustbot
Copy link
Collaborator

rustbot commented Feb 18, 2025

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

@lcnr lcnr force-pushed the yeet branch 2 times, most recently from 65fdd7c to a212afd Compare February 21, 2025 09:12
@bors
Copy link
Contributor

bors commented Feb 25, 2025

☔ The latest upstream changes (presumably #137611) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though could possibly use more info written as doc comments 🤔

lcnr added 4 commits February 26, 2025 14:29
A cycle was previously coinductive if all steps were coinductive.
Change this to instead considerm cycles to be coinductive if they
step through at least one where-bound of an impl of a coinductive
trait goal.
they don't detect any bugs in the search graph. We instead check
for these via `search_graph_fuzz`.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

projection obligations were accepted in - otherwise coinductive - global cycles
5 participants