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

Further regression with a Recursion Limit Exceeded error #16311

Closed
jchyb opened this issue Nov 9, 2022 · 6 comments · Fixed by #16317 or #16353
Closed

Further regression with a Recursion Limit Exceeded error #16311

jchyb opened this issue Nov 9, 2022 · 6 comments · Fixed by #16317 or #16353
Assignees
Labels
area:typer itype:bug regression This worked in a previous version but doesn't anymore stat:cannot reproduce
Milestone

Comments

@jchyb
Copy link
Contributor

jchyb commented Nov 9, 2022

Regression reproduced based on the failure in Open CB #17825. Problem found in sangria-graphql/sangria

Compiler version

3.2.2-RC1-bin-20221103-bf808b3-NIGHTLY

Minimized code

trait Tagged[U]
type WithTag[+T, U] = T & Tagged[U]

trait FromInput[Val]
implicit def coercedScalaInput[T]: FromInput[WithTag[T, Int]] = ???
implicit def optionInput[T](implicit ev: FromInput[T]): FromInput[Option[T]] = ???

trait WithoutInputTypeTags[T]
implicit def coercedOptArgTpe[T]: WithoutInputTypeTags[Option[WithTag[T, Int]]] = ???

trait InputType[+T]
class OptionInputType[T](ofType: InputType[T]) extends InputType[Option[T]]

type Argument[T]
def argument[T](argumentType: InputType[T])(implicit fromInput: FromInput[T], res: WithoutInputTypeTags[T]): Argument[Option[T]] = ???

def test = argument(OptionInputType(??? : InputType[WithTag[Boolean, Int]]))

Output

-- Error: sangria.scala:17:4 ---------------------------------------------------
17 |def test = argument(OptionInputType(??? : InputType[WithTag[Boolean, Int]]))
   |^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |Recursion limit exceeded.
   |Maybe there is an illegal cyclic reference?
   |If that's not the case, you could also try to increase the stacksize using the -Xss JVM option.
   |For the unprocessed stack trace, compile with -Yno-decode-stacktraces.
   |A recurring operation is (inner to outer):
   |
   |  check fully defined T
   |  check fully defined T & Tagged[Int]
   |  check fully defined T & Tagged[Int]
   |  check fully defined T & Tagged[Int] & Tagged[Int]
   |  check fully defined Option[T & Tagged[Int] & Tagged[Int]]
   |  check fully defined Option[Option[T & Tagged[Int] & Tagged[Int]]]
   |  check fully defined Argument[Option[Option[T & Tagged[Int] & Tagged[Int]]]]

Expectation

Should compile, as it did before in 3.2.1. This issue is ver closely related to #15365, to the point where I could basically reuse the minimization, except now the issue is unrelated to the typings introduced by the :: class . Simply trying to create an Argument object is enough to trigger the error (which also means the workarounds sangria used for the previous error will no longer work). The bisect script has shown efdfe0e to be the first problematic commit.

@jchyb jchyb added itype:bug area:typer regression This worked in a previous version but doesn't anymore labels Nov 9, 2022
@jchyb jchyb changed the title Further regression with a Recursion Limit Exceedederror Further regression with a Recursion Limit Exceeded error Nov 9, 2022
@odersky odersky self-assigned this Nov 10, 2022
@odersky
Copy link
Contributor

odersky commented Nov 10, 2022

I can't reproduce this using the nightly 3.3.0-RC1-bin-SNAPSHOT-nonbootstrapped-git-d8a6751.

odersky added a commit to dotty-staging/dotty that referenced this issue Nov 10, 2022
odersky added a commit that referenced this issue Nov 11, 2022
@jchyb
Copy link
Contributor Author

jchyb commented Nov 12, 2022

I checked, and it turns out this was fixed in the recently merged #15642, commit f19de96. Very cool part is that #15365 was also fixed by the same PR (thank you!). I'll ask Paweł later what to do with the 3.2.2 regression

@odersky
Copy link
Contributor

odersky commented Nov 12, 2022

One can probably construct counter examples then by defining a non-transparent superclass of some union type. But for now I don't even see which union type is the problematic one. Also, it could well be that the recursion overflow for fully defined type is correct.

In principle, if you get a "Recursion Limit Exceeded" that's not a bug. It means that the program has an illegal cyclic reference. An illegal cyclic reference happens if the compiler needs to know a fact in order to establish the same fact. So what is "needs to know"? That's where it gets interesting. Essentially, the type checker makes a best effort to not investigate facts if they are not necessary, playing various tricks with lazy evaluation and memoization. But the details can change from one version of the next. For instance, if fixing one bug requires looking somewhere that was not considered before, this can trigger a cyclic reference error. I should also mention that Scala is not alone in that; Java and I am sure many other languages are very similar in that respect.

So, lot's of unknowns. We have to realize we are at the fuzzy and bleeding edge of type inference here. But it seems to be the case that #15642 makes type inference more powerful and useful in practice, and that by itself can address some regressions,

odersky added a commit to dotty-staging/dotty that referenced this issue Nov 15, 2022
Two fixes:

 1. Don't forget about refinements
 2. Don't dealias

Fixes scala#16342

The first fix is essential for $16342. The second fix is just to keep
types tidy and not open aliases needlessly.

The previous incorrect version hid errors in previous regressions scala#15365 and scala#16311
which will need to be re-opened now.
@odersky
Copy link
Contributor

odersky commented Nov 15, 2022

Need to re-open because of #16344

@odersky odersky reopened this Nov 15, 2022
@odersky
Copy link
Contributor

odersky commented Nov 15, 2022

We get a constraint system with cyclic constraints here

bounds:
     T#1251897263 >: WithTag#2810[T#1168079523, Int#76]
     T#1168079523 >: T#1251897263 & Tagged#2793[Int#76]

where WithTag[T, U] = T & Tagged[U]. So the type variables depend mutually on each other.

It would be very important to see which commit first introduced this. Can we get a bisect? (based on #16344).

odersky added a commit that referenced this issue Nov 15, 2022
Two fixes:

 1. Don't forget about refinements
 2. Don't dealias

Fixes #16342
Fixes #16338

The first fix is essential for #16342. The second fix is just to keep
types tidy and not open aliases needlessly.

It probably fixes issues #16337 and #16336 as well, but the test cases
were not self-contained, so I could not try them out. It might fix other
recent regressions as well.

The previous incorrect version hid errors in previous regressions #15365
and #16311 which will need to be re-opened now.
@nicolasstucki
Copy link
Contributor

It seems that this issue started in efdfe0e.

odersky added a commit to dotty-staging/dotty that referenced this issue Nov 16, 2022
In OrderingConstraint#replace we moved the actual replacement
of a parameter with a type from the start of replace to its end,
since that was more convenient for dependency adjustments. It turns
out that doing so causes infinite recursion in instantiations in
some cases, specifically if a parameter contains itself as an indirect
lower bound that goes through an alias. Here is a situation that arises
in i16311.scala:
```
type WithTag[T, U] = T & Tagged[U]

  T1 >: WithTag[T2, Int]
  T2 >: T1 & Tagged[Int]
```
The current instantiation for T1 and T2 is Nothing. But we ran into a cycle
instead.

The fix is to move the parameter replacement back to the start of `replace`,
and to account for that in the dependency adjustment logic.

Fixes scala#16311
smarter added a commit that referenced this issue Nov 18, 2022
## 1. Fix replace operation

In OrderingConstraint#replace we moved the actual replacement of a
parameter with a type from the start of replace to its end, since that
was more convenient for dependency adjustments. It turns out that doing
so causes infinite recursion in instantiations in some cases,
specifically if a parameter contains itself as an indirect lower bound
that goes through an alias. Here is a situation that arises in
i16311.scala:
```scala
  type WithTag[T, U] = T & Tagged[U]

  T1 >: WithTag[T2, Int]
  T2 >: T1 & Tagged[Int]
```
The correct instantiation for T1 and T2 is Nothing. But we ran into a
cycle instead.

The fix is to move the parameter replacement back to the start of
`replace`, and to account for that in the dependency adjustment logic.

Fixes #16311 (with failing Ycheck)

## 2. See through aliases before decomposing And/Or in isSubType

There seem to be two missing cases in TypeComparer where we
have a TypeParamRef on one side and an And/Or type under an alias on the
other.
Examples:

    type AND = A & B
    type OR  = A | B
    p <:< AND
    OR <:< p

In this case we missed the decomposition into smaller types that would
happen
otherwise. This broke i16311.scala in Ycheck and broke i15365.scala with
an
infinite recursion in avoidance.

I verified that having an AndType as super or subtype of an abstract
type works
as expected. So if in the example above

    type AND >: A & B

or

    type AND <: A & B

it worked before. It was just aliases that were the problem (I assume
it's the same for OrTypes
as lower bounds).

This fixes #16311 completely and also
Fixes #15365
little-inferno pushed a commit to little-inferno/dotty that referenced this issue Jan 25, 2023
little-inferno pushed a commit to little-inferno/dotty that referenced this issue Jan 25, 2023
Two fixes:

 1. Don't forget about refinements
 2. Don't dealias

Fixes scala#16342

The first fix is essential for $16342. The second fix is just to keep
types tidy and not open aliases needlessly.

The previous incorrect version hid errors in previous regressions scala#15365 and scala#16311
which will need to be re-opened now.
little-inferno pushed a commit to little-inferno/dotty that referenced this issue Jan 25, 2023
In OrderingConstraint#replace we moved the actual replacement
of a parameter with a type from the start of replace to its end,
since that was more convenient for dependency adjustments. It turns
out that doing so causes infinite recursion in instantiations in
some cases, specifically if a parameter contains itself as an indirect
lower bound that goes through an alias. Here is a situation that arises
in i16311.scala:
```
type WithTag[T, U] = T & Tagged[U]

  T1 >: WithTag[T2, Int]
  T2 >: T1 & Tagged[Int]
```
The current instantiation for T1 and T2 is Nothing. But we ran into a cycle
instead.

The fix is to move the parameter replacement back to the start of `replace`,
and to account for that in the dependency adjustment logic.

Fixes scala#16311
@Kordyjan Kordyjan added this to the 3.3.0 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:typer itype:bug regression This worked in a previous version but doesn't anymore stat:cannot reproduce
Projects
None yet
4 participants