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

Where bounds with associated types equality cause overflow. #89503

Open
mixal-iirec opened this issue Oct 3, 2021 · 13 comments
Open

Where bounds with associated types equality cause overflow. #89503

mixal-iirec opened this issue Oct 3, 2021 · 13 comments
Assignees
Labels
A-trait-system Area: Trait system P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-types Relevant to the types team, which will review and decide on the PR/issue.
Milestone

Comments

@mixal-iirec
Copy link

Code

I tried this code:

trait Assoc {
    type B;
}
struct Test<U, T>
where
    U: Assoc,
    T: Assoc<B = <U as Assoc>::B>,
{
    a: U,
    c: T,
}
impl<U, T> Assoc for Test<U, T>
where
    U: Assoc,
    T: Assoc<B = <U as Assoc>::B>,
{
    type B = <U as Assoc>::B;
}
trait TestTrait: Assoc {}
impl<U, T> TestTrait for Test<U, T>
where
    U: Assoc,
    T: Assoc<B = <Self as Assoc>::B>,
{
}

It used to compile but now it doesn't because:

error[E0275]: overflow evaluating the requirement `<T as Assoc>::B == <U as Assoc>::B`
  --> src/main.rs:20:1
   |
20 | / impl<U: Assoc, T> TestTrait for Test<U, T>
21 | | where
22 | |     U: Assoc,
23 | |     T: Assoc<B = <Self as Assoc>::B>,
24 | | {
25 | | }
   | |_^
   |
note: required because of the requirements on the impl of `Assoc` for `Test<U, T>`
  --> src/main.rs:12:12
   |
12 | impl<U, T> Assoc for Test<U, T>
   |

Version it worked on

It most recently worked on:
Stable: 1.55.0
Nightly: rustc 1.56.0-nightly (50171c310 2021-09-01)

Version with regression

rustc --version --verbose:

rustc 1.57.0-nightly (f03eb6bef 2021-10-02)
rustc 1.56.0-beta.3 (deef86610 2021-09-17)

(Probably all nightly versions after 2021-09-01)

@rustbot modify labels: +regression-from-stable-to-beta -regression-untriaged

@mixal-iirec mixal-iirec added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Oct 3, 2021
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-untriaged Untriaged performance or correctness regression. labels Oct 3, 2021
@apiraino
Copy link
Contributor

apiraino commented Oct 4, 2021

if it helps, a bisection seems to lead to #85868

searched nightlies: from nightly-2021-08-15 to nightly-2021-10-04
regressed nightly: nightly-2021-09-03
searched commits: from 50171c3 to 371f3cd
regressed commit: 371f3cd

bisected with cargo-bisect-rustc v0.6.0

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --start=2021-08-15 

@apiraino
Copy link
Contributor

apiraino commented Oct 4, 2021

Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

@rustbot label -I-prioritize +P-critical

@rustbot rustbot added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Oct 4, 2021
@Aaron1011 Aaron1011 self-assigned this Oct 4, 2021
@Aaron1011
Copy link
Member

It appears that 371f3cd exposed a bug in the handling of projection sub-obligations. If I modify the code to unconditionally cache all sub-obligations (or avoid updating the projection cache entirely), the overflow error still occurs. It looks like the code previous compiled because we used to only store a small subset of the projection subobligations in the cache. Some of the previously-discarded subobligations appear to cause an overflow error when we process them again (when we read the cached projection result).

@Aaron1011
Copy link
Member

Aaron1011 commented Oct 4, 2021

Minimized:

trait Assoc {
    type B;
}
struct Test<U, T>(U, T);
impl<U, T> Assoc for Test<U, T>
where
    U: Assoc,
    T: Assoc<B = <U as Assoc>::B>,
{
    type B = <U as Assoc>::B;
}
trait TestTrait {}
impl<U, T> TestTrait for Test<U, T>
where
    U: Assoc,
    T: Assoc<B = <Self as Assoc>::B>,
{
}

fn main() {}

produces the following error on Nightly (and succeeds on stable):

error[E0275]: overflow evaluating the requirement `<T as Assoc>::B == <U as Assoc>::B`
  --> src/main.rs:13:1
   |
13 | / impl<U, T> TestTrait for Test<U, T>
14 | | where
15 | |     U: Assoc,
16 | |     T: Assoc<B = <Self as Assoc>::B>,
17 | | {
18 | | }
   | |_^
   |
note: required because of the requirements on the impl of `Assoc` for `Test<U, T>`
  --> src/main.rs:5:12
   |
5  | impl<U, T> Assoc for Test<U, T>
   |            ^^^^^     ^^^^^^^^^^

For more information about this error, try `rustc --explain E0275`.
error: could not compile `playground` due to previous error

@Aaron1011
Copy link
Member

I think that this error is actually legitimate:

  1. During WF checking of the TestTrait impl, we try to normalize T: Assoc<B = <Self as Assoc>::B>, which requires normalizing <Self as Assoc>::B.
  2. We find the Assoc impl, and try to use it. This requires evaluating its where clauses - specifically, T: Assoc<B = <U as Assoc>::B>.
  3. However, we are already trying to project <T as Assoc>::B from the first step, so this is a cycle.

cc @rust-lang/wg-traits

@jackh726
Copy link
Member

jackh726 commented Oct 4, 2021

Yeah, this is rough. Essentially the same underlying issue as #87755 and #87758 (among other non-GAT issues) (I think). I've tried to look into this and it's really difficult to solve.

@jackh726
Copy link
Member

jackh726 commented Oct 4, 2021

Oh I see...this is maybe slightly different.

@jackh726
Copy link
Member

jackh726 commented Oct 4, 2021

This could potentially be fixed by removing this:

if let Some(ProjectionCacheEntry::Recur) = map.get(&key) {

But that has other implications.

@Mark-Simulacrum Mark-Simulacrum added this to the 1.56.0 milestone Oct 15, 2021
@Mark-Simulacrum
Copy link
Member

@rust-lang/wg-traits @Aaron1011 This was marked as P-critical by wg-pri, but it doesn't look like we've seen much progress on resolving this issue (I don't see a linked PR). We missed it in our earlier T-release triage as it wasn't milestoned, but the release is next week -- do we think there's a minimal patch likely that we could backport in time?

We also discuss this in today's release team meeting and there was a general feeling I think that if the crater run we just started doesn't show this to be widespread breakage we're probably OK to accept this breakage (rather than e.g. delaying the release or trying to revert 371f3cd which seems pretty difficult and introduces other regressions, I believe).

@jackh726
Copy link
Member

Yeah, this is really rough. #89503 (comment) is one potential patch, but I've tried it before and it ends up causing other fallout that needs its own fixes. Unfortunately, through a series of relatively small bug fixes, we've ended up in a rut that needs larger design decisions/discussions to get out of.

I think we need to see what comes out of the crater run. If this isn't widespread, then I think accepting that this has regressed into a larger class of bugs is better than reverting and running into incremental bugs.

@pietroalbini
Copy link
Member

The crater run only reported one crate failing due to this: https://crater-reports.s3.amazonaws.com/beta-1.56-3/beta-2021-10-15/reg/mpstthree-0.0.13/log.txt

@apiraino
Copy link
Contributor

apiraino commented Dec 2, 2021

Demoting to P-high after reading the latest comments

@rustbot label -P-critical +P-high

@rustbot rustbot added P-high High priority and removed P-critical Critical priority labels Dec 2, 2021
@apiraino apiraino added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Dec 9, 2021
@jackh726 jackh726 added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-trait-system Area: Trait system and removed C-bug Category: This is a bug. labels Jan 28, 2022
@jackh726 jackh726 added the S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue label Nov 22, 2022
@pnkfelix
Copy link
Member

@rustbot label: +T-types -T-compiler

@rustbot rustbot 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 Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trait-system Area: Trait system P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants