Fix #4368: Avoid stackoverflows for some cyclic definitions #4385

odersky · 2018-04-26T11:29:45Z

We use a two-pronged approach:

First, we now detect some cycles that span inherited or intersected classes or traits. Unfortunately, this operation forces the types of type members, which means it can lead to spurious cycles itself. So we can do this only after a lot of types are already computed, which means some cycles lead to stackoverflows before they are detected.

Hence the second strategy: give more useful information when deep recursions happen.

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Commit Messages

We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).

Please stick to these guidelines for commit messages:

Separate subject from body with a blank line

When fixing an issue, start your commit message with Fix #<ISSUE-NBR>:

Limit the subject line to 72 characters

Capitalize the subject line

Do not end the subject line with a period

Use the imperative mood in the subject line ("Add" instead of "Added")

Wrap the body at 80 characters

Use the body to explain what and why vs. how

adapted from https://chris.beams.io/posts/git-commit

Have an awesome day! ☀️

odersky · 2018-04-26T12:26:20Z

test performance please

dottybot · 2018-04-26T12:26:49Z

performance test scheduled: 2 job(s) in queue, 1 running.

Blaisorblade · 2018-04-26T15:03:59Z

compiler/src/dotty/tools/dotc/core/Types.scala

      try go(this)
      catch {
        case ex: Throwable =>
-          core.println(i"findMember exception for $this member $name, pre = $pre")
-          throw ex // DEBUG
+          core.println(s"findMember exception for $this member $name, pre = $pre, recCount = $recCount")


Nit: why change formatter to s?

Avoid possible crashes when trying to show pre.

dottybot · 2018-04-26T15:16:09Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/4385/ to see the changes.

Benchmarks is based on merging with master (d73d9bc)

Blaisorblade · 2018-04-26T15:23:19Z

Cool that you're looking at this! I suppose this also fixes #318, and that you already looked at Samuel's proposals from back then and they don't work?

odersky · 2018-04-26T17:02:15Z

Yes, #318 is handled, albeit crudely.

odersky · 2018-04-26T17:42:43Z

test performance please

dottybot · 2018-04-26T17:43:01Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2018-04-26T19:35:33Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/4385/ to see the changes.

Benchmarks is based on merging with master (d73d9bc)

odersky · 2018-04-27T08:41:28Z

So, this is a rather sneaky way to fix a whole class of problems that cause the compiler to stackoverflow in some way or another. One would hope there was a principled way to detect and repair all illegal cyclic references before they cause stackoverflows. Unfortunately, that looks impossible or at least impossibly hard: Scala's type system subsumes F_sub and is therefore undecidable. So some infinite loops/stackoverflows are unavoidable. What's more, checking for cycles early has the unfortunate consequence that we force types and thereby can cause spurious cyclic references! So the mere act of checking generates spurious errors. This is a real problem in practice, observed in both the compiler codebase and stdlib. Another possibility would be to attach a cycle detection component to every operation that might stackoverflow. But these tend to be also operations that are most time-critical, so doing the extra work is prohibitive.

The idea in this pull request is to act only if there is a stackoverflow. In that case we unroll the stack until we hit a safepoint (typically: a point where we typecheck an AST node and therefore have an error position to record). While unrolling we keep a count of all important method frames that were unrolled. Important means: the method contains a try/catch that does the unrolling and counting. At the safepoint, we check which method appeared most often in the unrolled frames. We then compose a nice error message showing only calls info for that method and leaving out the middle of the stack if it is too large. A typical error message looks like this:

|  val b: y.B = a 
|         ^
|         Recursion limit exceeded.
|         Maybe there is an illegal cyclic reference?
|         A recurring operation is (inner to outer):
|         
|           find-member y.B
|           find-member y.A
|           find-member y.B
|           find-member y.A
|           find-member y.B
|           find-member y.A
|           find-member y.B
|           find-member y.A
|           find-member y.B
|           find-member y.A
|           ...
|         
|           find-member y.A
|           find-member y.B
|           find-member y.A
|           find-member y.B
|           find-member y.A
|           find-member y.B
|           find-member y.A
|           find-member y.B
|           find-member y.A
|           find-member y.B

Of course, it's still nicer to pinpoint the cycle where it arises, and we try to do this also, as long as it can be done safely and efficiently. But we have transformed the game from a bug that needs fixing, (a stackoverflow is a compiler crash, so it's wrong and embarrassing) to a quality of error message question. Such a question can be answered on a case-by-case basis, taking into account the frequency of the error (in my experience most of these cyclic reference errors causing infinite loops are synthetic and don't tend to occur in practice) and the effort needed to get a more specific error message.

odersky · 2018-04-27T09:02:12Z

test performance please

dottybot · 2018-04-27T09:02:42Z

performance test scheduled: 1 job(s) in queue, 0 running.

odersky · 2018-04-27T09:05:55Z

This PR fixes #4368, #4369, #4370, #4371, #4372 and #318.

odersky · 2018-04-27T10:32:12Z

test performance please

dottybot · 2018-04-27T10:32:50Z

performance test scheduled: 3 job(s) in queue, 1 running.

dottybot · 2018-04-27T10:55:45Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/4385/ to see the changes.

Benchmarks is based on merging with master (d73d9bc)

dottybot · 2018-04-27T12:48:31Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/4385/ to see the changes.

Benchmarks is based on merging with master (1e9430a)

sir-wabbit · 2018-05-09T00:23:08Z

While fuzzing #4489 I found these two samples that produce rather unintelligible error messages:

trait I0 {
type Dynamic <: Any with i1
trait i1
type I2 >: i1
type I3 <: I0 {
type i1 <: I2;
val I4: I2; }
def I5(implicit I5: => I5.I6): Unit = {}
}
class I5 {
assert(I3(, I2 == I5.i1))
}
}
}
}

produces

5 |type I3 <: i0 {
  |           ^
  |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.
  |A recurring operation is (inner to outer):
  |
  |  subtype Object{...} <:< [cannot display due to dotty.tools.dotc.core.RecursionOverflow: , raw string = TypeBounds(TypeRef(ThisType(TypeRef(NoPrefix,module class scala)),class Nothing),TypeRef(RecThis(3689420),I2))]
  |  subtype Object{...} <:< [cannot display due to dotty.tools.dotc.core.RecursionOverflow: , raw string = TypeBounds(TypeRef(ThisType(TypeRef(NoPrefix,module class scala)),class Nothing),TypeRef(RecThis(3689420),I2))]
  |  subtype Object{...} <:< [cannot display due to dotty.tools.dotc.core.RecursionOverflow: , raw string = TypeBounds(TypeRef(ThisType(TypeRef(NoPrefix,module class scala)),class Nothing),TypeRef(RecThis(3689420),I2))]
  |  subtype Object{...} <:< [cannot display due to dotty.tools.dotc.core.RecursionOverflow: , raw string = TypeBounds(TypeRef(ThisType(TypeRef(NoPrefix,module class scala)),class Nothing),TypeRef(RecThis(3689420),I2))]

and

object i0 {
def I1(i4: Any) = i4 match {
case _: Some[String] => case _ => println()
case _ => println
class i0 {
def I1[i4](i4: i4): i4 = ???
type i4[i4] = I1[i4]
type i4 = i4[i4]
type i4 = String
val i4: i4[i4]
def I1(I1: i4[Int]) = i4
val I3 = I1 _
val I1 = i4
val i4 = I1
val I1 = new i4
val I1 = i4
val I2 = I1
val I3 = I3
val i4 = 1
val i4 = I1
def I1[i4](i4: i4[i4]): I1[I3] = ???
val I2 = i4[String](1)
val i4: i4[I1] = ???
implicit def I1[i4](I2: I1)(implicit i4: i4[I2], I1: i4[I1]): Unit = {}
class I1 extends i4[i4] with i4[i4, I1] with i4[I1, I1] with i0[I1, i4];
trait i4 extends I1 {
type i4 <: i0[I1]
implicit val i4: I1[i4] = new i4[I1] {
type i4[I1] = i4[I1]
}
def I2[I3](I1: i4[I1]): i4[i4] = new I1[i4](I1)
val I2 = new i4[Int]
}

produces

22 |val I2 = i3[String](1)
   |         ^^
   |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.
   |A recurring operation is (inner to outer):
   |
   |  subtype i0.this.i3[<error Type argument i0.this.i3 has not the same kind as its bound >](i0.this.i3) <:< PolyProto(String): FunProto(1):?
   |  subtype i0(i0.this) <:< ?{ i3: PolyProto(String): FunProto(1):? }

odersky · 2018-05-09T10:36:17Z

I won't invest more time in this. So, ready to get this in?

Blaisorblade · 2018-05-09T12:04:15Z

Hm. Especially since RecursionOverflow appears in the error, it seems worth renaming that (and friends) as RecursionOverflowException.

But otherwise, it seems reasonable to classify those as requests for enhancements. I'll have to look at the first more closely, because the output contradicts quite a few of my expectations and one of the two must be corrected.

smarter · 2018-05-09T12:05:35Z

I think it'd be good to have a -Y setting to turn off the exception catching, so that the root cause of issues can be investigated more easily.

odersky · 2018-05-09T13:11:50Z

@smarter The recommended way to find out more is to turn the cyclic printer on.

smarter · 2018-05-09T13:27:05Z

@odersky The issue is that by turning everything into errors, @alexknvl fuzzer will never detect any stackoverflow again, even though some of them might be legitimate bugs we can fix.

Blaisorblade · 2018-05-09T19:21:11Z

I plan to implement that flag. I've started playing more with this, running a few experiments and talking with @allanrenucci and @smarter — I have a few examples where stack overflows disappear altogether from the output, such as:

class i0 {
  class i1 { type i2 }
  type i3 = i1.i2 // error, comment this out to _see_ the stack overflow
  type i4 = i0 { type i1 <: i4 } // stack overflow, hidden by the above error
}

but I have pushed fixes for that. EDIT: gotta review the few extra errors from testcases, but they seem generally reasonable. I'll probably make that a separate PR.

allanrenucci · 2018-05-09T19:24:31Z

compiler/src/dotty/tools/dotc/reporting/UniqueMessagePositions.scala

@@ -26,7 +26,7 @@ trait UniqueMessagePositions extends Reporter {
            case _ => positions((ctx.source, pos)) = m.level
          }
        }
-        shouldHide
+        shouldHide && !ctx.settings.YshowSuppressedErrors.value


I guess it would make sense to test this first

- Move them all to TypeErrors.scala - Systematically use `toMessage` for reporting - Catch stack overflows instead of counting recursions

This now handles all errors from scala#4368 to scala#4372 and also scala#318.

Not needed here, but I missed the functionality elsewhere.

Once we are at it, let's try to bullet-proof everything.

Move it from `transformNode` to `goNamed` and `goUnnamed`. This might be faster since that way `transformNode` should be an inline candidate since it is short.

Thanks `@smarter` for guidance.

As a followup to the earlier bugfix.

Ensures stackoverflows aren't hidden by errors, without needing scala#4511. Also add relevant testcase from scala#4385 (comment); I confirmed this fails without the position fix.

Blaisorblade · 2018-05-31T09:02:56Z

@smarter: can you take a look? This can now be disabled, and the remaining issues here (in comments above) don't seem blockers, so I think this should be mergeable.

Blaisorblade · 2018-05-04T04:54:52Z

compiler/src/dotty/tools/dotc/core/TypeErrors.scala

+  private def showType(tp: Type)(implicit ctx: Context) = tp match {
+    case ClassInfo(_, cls, _, _, _) => cls.showLocated
+    case _ => tp.show


Does this refactoring intentionally drop the case for type bounds here (search and compare with the removed showType)? But guess yes, the old output wasn’t ideal either.

Blaisorblade · 2018-06-01T10:02:47Z

Or @odersky can you review my changes? Whoever's faster between you and @smarter, since this was blocking other stuff.

allanrenucci

Otherwise LGTM

allanrenucci · 2018-06-04T16:36:19Z

compiler/src/dotty/tools/dotc/config/ScalaSettings.scala

@@ -146,6 +146,7 @@ class ScalaSettings extends Settings.SettingGroup {
  val Xlink = BooleanSetting("-Xlink", "Recompile library code with the application.")
  val YoptPhases = PhasesSetting("-Yopt-phases", "Restrict the optimisation phases to execute under -optimise.")
  val YoptFuel = IntSetting("-Yopt-fuel", "Maximum number of optimisations performed under -optimise.", -1)
+  val YnoDecodeStacktraces = BooleanSetting("-Yno-decode-stacktraces", "Show operations that triggered StackOverflows instead of printing the raw stacktraces.")


Comment is misleading. How I understand it: "if you pass the flag, you won't see original stack traces". I guess it is the opposite

Oh right, fixing it

Probably fixed by scala#4385

dottybot reviewed Apr 26, 2018

View reviewed changes

Blaisorblade reviewed Apr 26, 2018

View reviewed changes

odersky force-pushed the fix-#4368 branch from d50a9bb to 8207b76 Compare April 26, 2018 17:03

odersky assigned Blaisorblade Apr 27, 2018

Blaisorblade force-pushed the fix-#4368 branch from 9474bd1 to ec460a1 Compare May 7, 2018 16:32

allanrenucci reviewed May 9, 2018

View reviewed changes

Blaisorblade force-pushed the fix-#4368 branch from efc83c6 to edb39a4 Compare May 11, 2018 12:35

odersky and others added 15 commits May 30, 2018 02:31

Rearrange TypeError exceptions

8240d44

- Move them all to TypeErrors.scala - Systematically use `toMessage` for reporting - Catch stack overflows instead of counting recursions

Further refactoring of stackoverflow handling

e6af770

This now handles all errors from scala#4368 to scala#4372 and also scala#318.

Fix CyclicError reporting

01b5ecf

Fix context for cyclic error message

25c6abb

Fix^2 context in CyclicReference toMessage

e9ef0a7

Fix error annotation in test

0daaab0

MergeError refactorings

03b9eb3

Add append method to Message

e989cad

Not needed here, but I missed the functionality elsewhere.

Also catch TypeErrors in all transforms

c05e353

Once we are at it, let's try to bullet-proof everything.

rearrange try-catch in MegaPhase

a84b75f

Move it from `transformNode` to `goNamed` and `goUnnamed`. This might be faster since that way `transformNode` should be an inline candidate since it is short.

Preload handleRecursive to prevent nested stackoverflows

5ade7fe

Thanks `@smarter` for guidance.

Ensure -YshowSuppressedErrors also affects UniqueMessagePositions

ecbd692

As a followup to the earlier bugfix.

Fix typo in modified (commented-out) code

5ef9263

Add a flag to disable handleRecursive

34c4107

Typer: Refine position when catching top-level TypeError

7814a44

Ensures stackoverflows aren't hidden by errors, without needing scala#4511. Also add relevant testcase from scala#4385 (comment); I confirmed this fails without the position fix.

Blaisorblade force-pushed the fix-#4368 branch from a727545 to 7d3a462 Compare May 30, 2018 00:51

Address review feedback

640e0a8

Blaisorblade force-pushed the fix-#4368 branch from 7d3a462 to 640e0a8 Compare May 30, 2018 00:52

Blaisorblade assigned smarter May 31, 2018

Blaisorblade approved these changes May 31, 2018

View reviewed changes

allanrenucci approved these changes Jun 4, 2018

View reviewed changes

Negate description of negated option

7fea207

Blaisorblade merged commit 1399889 into scala:master Jun 5, 2018

Blaisorblade deleted the fix-#4368 branch June 5, 2018 11:50

This was referenced Jul 17, 2018

Infinite loop in subtyping check #318

Closed

Dotty does not yet implement the Kennedy and Pierce restriction #4805

Closed

Blaisorblade mentioned this pull request Jul 24, 2018

Fix #4709: report if local implicits need type annotation #4729

Merged

nicolasstucki added a commit to dotty-staging/dotty that referenced this pull request Jan 18, 2019

Fix scala#4372: Add regression test

fa75f19

Probably fixed by scala#4385

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4368: Avoid stackoverflows for some cyclic definitions #4385

Fix #4368: Avoid stackoverflows for some cyclic definitions #4385

odersky commented Apr 26, 2018 •

edited

Loading

dottybot left a comment

odersky commented Apr 26, 2018

dottybot commented Apr 26, 2018

Blaisorblade Apr 26, 2018

odersky Apr 26, 2018

dottybot commented Apr 26, 2018

Blaisorblade commented Apr 26, 2018

odersky commented Apr 26, 2018

odersky commented Apr 26, 2018

dottybot commented Apr 26, 2018

dottybot commented Apr 26, 2018

odersky commented Apr 27, 2018 •

edited

Loading

odersky commented Apr 27, 2018

dottybot commented Apr 27, 2018

odersky commented Apr 27, 2018

odersky commented Apr 27, 2018

dottybot commented Apr 27, 2018

dottybot commented Apr 27, 2018

dottybot commented Apr 27, 2018

sir-wabbit commented May 9, 2018

odersky commented May 9, 2018

Blaisorblade commented May 9, 2018

smarter commented May 9, 2018

odersky commented May 9, 2018

smarter commented May 9, 2018

Blaisorblade commented May 9, 2018 •

edited

Loading

allanrenucci May 9, 2018

Blaisorblade commented May 31, 2018

Blaisorblade May 4, 2018

Blaisorblade commented Jun 1, 2018 •

edited

Loading

allanrenucci left a comment

allanrenucci Jun 4, 2018

Blaisorblade Jun 4, 2018

Fix #4368: Avoid stackoverflows for some cyclic definitions #4385

Fix #4368: Avoid stackoverflows for some cyclic definitions #4385

Conversation

odersky commented Apr 26, 2018 • edited Loading

dottybot left a comment

Choose a reason for hiding this comment

Commit Messages

odersky commented Apr 26, 2018

dottybot commented Apr 26, 2018

Blaisorblade Apr 26, 2018

Choose a reason for hiding this comment

odersky Apr 26, 2018

Choose a reason for hiding this comment

dottybot commented Apr 26, 2018

Blaisorblade commented Apr 26, 2018

odersky commented Apr 26, 2018

odersky commented Apr 26, 2018

dottybot commented Apr 26, 2018

dottybot commented Apr 26, 2018

odersky commented Apr 27, 2018 • edited Loading

odersky commented Apr 27, 2018

dottybot commented Apr 27, 2018

odersky commented Apr 27, 2018

odersky commented Apr 27, 2018

dottybot commented Apr 27, 2018

dottybot commented Apr 27, 2018

dottybot commented Apr 27, 2018

sir-wabbit commented May 9, 2018

odersky commented May 9, 2018

Blaisorblade commented May 9, 2018

smarter commented May 9, 2018

odersky commented May 9, 2018

smarter commented May 9, 2018

Blaisorblade commented May 9, 2018 • edited Loading

allanrenucci May 9, 2018

Choose a reason for hiding this comment

Blaisorblade commented May 31, 2018

Blaisorblade May 4, 2018

Choose a reason for hiding this comment

Blaisorblade commented Jun 1, 2018 • edited Loading

allanrenucci left a comment

Choose a reason for hiding this comment

allanrenucci Jun 4, 2018

Choose a reason for hiding this comment

Blaisorblade Jun 4, 2018

Choose a reason for hiding this comment

odersky commented Apr 26, 2018 •

edited

Loading

odersky commented Apr 27, 2018 •

edited

Loading

Blaisorblade commented May 9, 2018 •

edited

Loading

Blaisorblade commented Jun 1, 2018 •

edited

Loading