Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

elaborate obligations in coherence #124532

Closed
wants to merge 3 commits into from

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Apr 29, 2024

The following test currently does not pass coherence:

trait Super {}
trait Sub<T>: Super {}

trait Overlap<T> {}
impl<T, U: Sub<T>> Overlap<T> for U {}
impl<T> Overlap<T> for () {}

fn main() {}

We check whether (): Sub<?t> holds. This stalls with ambiguity as downstream crates may add an impl for (): Sub<Local>. However, its super trait bound (): Super cannot be implemented downstream, so this one is known not to hold.

By elaborating the bounds in the implicit negative overlap check, this now compiles. This is necessary to prevent breakage from enabling -Znext-solver=coherence (#121848), see tests/ui/coherence/super-traits/super-trait-knowable-2.rs for more details.

r? @compiler-errors

@lcnr

This comment was marked as outdated.

@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. labels Apr 29, 2024
@compiler-errors compiler-errors added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 29, 2024
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.

r=me with TODO and FCP

@compiler-errors
Copy link
Member

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Apr 29, 2024

Team member @compiler-errors has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 29, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-cloud-vms rust-cloud-vms bot force-pushed the elaborate-coherence branch 3 times, most recently from f9ab26e to cbee778 Compare April 29, 2024 17:54
@lcnr lcnr added I-types-nominated Nominated for discussion during a types team meeting. and removed I-types-nominated Nominated for discussion during a types team meeting. labels Apr 29, 2024
@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

Notably it's already a breaking change to remove a supertrait.

@compiler-errors compiler-errors added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 29, 2024
@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Apr 30, 2024
@rfcbot
Copy link

rfcbot commented Apr 30, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@BoxyUwU
Copy link
Member

BoxyUwU commented May 2, 2024

do we want to skip the 10 day waiting period so that this is included in the same beta bumps as new solver in coherence stabilization?

@lcnr
Copy link
Contributor Author

lcnr commented May 2, 2024

the last beta bump was on the 26th of April, so this PR and the stabilization PR will definitely be on the same beta version unless we somehow delay this PR by 5 weeks

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 10, 2024
@rfcbot
Copy link

rfcbot commented May 10, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label May 10, 2024
@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels May 10, 2024
@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented May 13, 2024

📌 Commit 03d9e84 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 13, 2024
fmease added a commit to fmease/rust that referenced this pull request May 13, 2024
…er-errors

elaborate obligations in coherence

The following test currently does not pass coherence:
```rust
trait Super {}
trait Sub<T>: Super {}

trait Overlap<T> {}
impl<T, U: Sub<T>> Overlap<T> for U {}
impl<T> Overlap<T> for () {}

fn main() {}
```

We check whether `(): Sub<?t>` holds. This stalls with ambiguity as downstream crates may add an impl for `(): Sub<Local>`. However, its super trait bound `(): Super` cannot be implemented downstream, so this one is known not to hold.

By elaborating the bounds in the implicit negative overlap check, this now compiles. This is necessary to prevent breakage from enabling `-Znext-solver=coherence` (rust-lang#121848), see tests/ui/coherence/super-traits/super-trait-knowable-2.rs for more details.

r? `@compiler-errors`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 13, 2024
…er-errors

elaborate obligations in coherence

The following test currently does not pass coherence:
```rust
trait Super {}
trait Sub<T>: Super {}

trait Overlap<T> {}
impl<T, U: Sub<T>> Overlap<T> for U {}
impl<T> Overlap<T> for () {}

fn main() {}
```

We check whether `(): Sub<?t>` holds. This stalls with ambiguity as downstream crates may add an impl for `(): Sub<Local>`. However, its super trait bound `(): Super` cannot be implemented downstream, so this one is known not to hold.

By elaborating the bounds in the implicit negative overlap check, this now compiles. This is necessary to prevent breakage from enabling `-Znext-solver=coherence` (rust-lang#121848), see tests/ui/coherence/super-traits/super-trait-knowable-2.rs for more details.

r? ``@compiler-errors``
bors added a commit to rust-lang-ci/rust that referenced this pull request May 13, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#119515 (style-guide: Format single associated type `where` clauses on the same line)
 - rust-lang#119959 ([meta] Clarify prioritization alert)
 - rust-lang#123817 (Stabilize `seek_seek_relative`)
 - rust-lang#124532 (elaborate obligations in coherence)
 - rust-lang#125063 (Don't call `env::set_var` in `rustc_driver::install_ice_hook`)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr
Copy link
Member

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 13, 2024
@bors
Copy link
Contributor

bors commented May 16, 2024

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

@lcnr
Copy link
Contributor Author

lcnr commented Jul 10, 2024

closing in favor of #127574

@lcnr lcnr closed this Jul 10, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 30, 2024
…=compiler-errors

elaborate unknowable goals

A reimplemented version of rust-lang#124532 affecting only the new solver. Always trying to prove super traits ends up causing a fatal overflow error in diesel, so we cannot land this in the old solver.

The following test currently does not pass coherence:
```rust
trait Super {}
trait Sub<T>: Super {}

trait Overlap<T> {}
impl<T, U: Sub<T>> Overlap<T> for U {}
impl<T> Overlap<T> for () {}

fn main() {}
```

We check whether `(): Sub<?t>` holds. This stalls with ambiguity as downstream crates may add an impl for `(): Sub<Local>`. However, its super trait bound `(): Super` cannot be implemented downstream, so this one is known not to hold.

By trying to prove that all the super bounds of a trait before adding a coherence unknowable candidate, this compiles. This is necessary to prevent breakage from enabling `-Znext-solver=coherence` (rust-lang#121848), see tests/ui/coherence/super-traits/super-trait-knowable-2.rs for more details. The idea is that while there may be an impl of the trait itself we don't know about, if we're able to prove that a super trait is definitely not implemented, then that impl would also never apply/not be well-formed.

This approach is different from rust-lang#124532 as it allows tests/ui/coherence/super-traits/super-trait-knowable-3.rs to compile. The approach in rust-lang#124532 only elaborating the root obligations while this approach tries it for all unknowable trait goals.

r? `@compiler-errors`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 30, 2024
Rollup merge of rust-lang#127574 - lcnr:coherence-check-supertrait, r=compiler-errors

elaborate unknowable goals

A reimplemented version of rust-lang#124532 affecting only the new solver. Always trying to prove super traits ends up causing a fatal overflow error in diesel, so we cannot land this in the old solver.

The following test currently does not pass coherence:
```rust
trait Super {}
trait Sub<T>: Super {}

trait Overlap<T> {}
impl<T, U: Sub<T>> Overlap<T> for U {}
impl<T> Overlap<T> for () {}

fn main() {}
```

We check whether `(): Sub<?t>` holds. This stalls with ambiguity as downstream crates may add an impl for `(): Sub<Local>`. However, its super trait bound `(): Super` cannot be implemented downstream, so this one is known not to hold.

By trying to prove that all the super bounds of a trait before adding a coherence unknowable candidate, this compiles. This is necessary to prevent breakage from enabling `-Znext-solver=coherence` (rust-lang#121848), see tests/ui/coherence/super-traits/super-trait-knowable-2.rs for more details. The idea is that while there may be an impl of the trait itself we don't know about, if we're able to prove that a super trait is definitely not implemented, then that impl would also never apply/not be well-formed.

This approach is different from rust-lang#124532 as it allows tests/ui/coherence/super-traits/super-trait-knowable-3.rs to compile. The approach in rust-lang#124532 only elaborating the root obligations while this approach tries it for all unknowable trait goals.

r? `@compiler-errors`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants