Report fewer spurious cyclic errors #10626
Conversation
|
Dotty does not count / limit the recursion depth, the nicer error message is crafted when catching a In this light, this PR might be the right tradeoff. Some code with diverging types that previously triggered to a |
|
Catching SOE is a bit risky because the exception handler can itself trigger another SOE before when it is doing its logging/reporting/throwing of a "better" exception. But perhaps for a compiler its okay. |
| @@ -425,19 +428,12 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper | |||
| } | |||
| } | |||
|
|
|||
| def checkNonCyclic(pos: Position, tp: Type, lockedSym: Symbol): Boolean = try { | |||
| if (!lockedSym.lock(CyclicReferenceError(pos, tp, lockedSym))) false | |||
There was a problem hiding this comment.
lock always succeeded. This overload of checkNonCyclic is invoked right after checkNotLocked.
Added a depth limit for Updated the PR description, ready for review @som-snytt @retronym. |
Remove old fix / workaround for t798.scala, no longer needed http://lrytz.github.io/scala-aladdin-bugtracker/displayItem.do%3Fid=798.html
When checking type params / abstract types for cyclic subtyping,
don't use `symbol.lock` to detect cycles.
The same symbol may occur multiple times and expand to a different
type, for example `a.Id[a.Id[Int]]`.
Seen types need to be compared with `==` (not `=:=`), otherwise
`a.Id[Int]` is matches `a.Id[a.Id[Int]]`.
I tried using `symbol.lock` in a first step and only adding types
to the stack from the second iteration, but some of the new test
cases then still fail to compile because the same symbol lock is
also used for type completers (detecting cyclic references).
An issue was that diverging types were no longer reported as cyclic,
as in neg/t510.scala. I added a recursion depth limit for this case.
```
abstract class C { type T }
abstract class D[S <: C](val c: S) extends C {
type T <: c.T
}
abstract class E(e: E) extends D[E](e)
object Test {
def f(e: E): Unit = {
def g(t: e.T): Unit = ()
()
}
}
```
`e.T` keeps expanding to `e.c.T` to `e.c.c.T` and so on.
Note that removing `object Test` from the example and compiling
only the definitions runs into a stack overflow in a later phase,
so the compiler is vulnerable to diverging types in other places.
For the record, in Scala 3 catches stack overflows and reports
"Recursion limit exceeded" errors.
There was a problem hiding this comment.
Thank you for reviving this. Having a separate recursion counter seems like a good approach.
On the way to a failure, we have a O(N^2) performance with the tps.contains(tp). I think the limit of the list length is small enough to not worry about this.
It is easy to write code that that unwittngly teeters on the edge of the previous implementation's cliff of spuriousness. If that edge slightly shifts, as happened with Scala 2.13, it can be nigh on impossible to refactor the code in such a a way to retreat to safety.
|
community build kicked off — I'll report back if there's a problem, otherwise no news is good news |
Fixes scala/bug#12814, fixes scala/bug#12622, fixes scala/bug#10669, fixes scala/bug#9568, fixes scala/bug#8252.
Co-authored by @retronym.
When checking type params / abstract types for cyclic subtyping, don't use
symbol.lockto detect cycles.The same symbol may occur multiple times and expand to a different type, for example
a.Id[a.Id[Int]].Seen types need to be compared with
==(not=:=), otherwisea.Id[Int]is matchesa.Id[a.Id[Int]].I tried using
symbol.lockin a first step and only adding types to the stack from the second iteration, but some of the new test cases then still fail to compile because the same symbol lock is also used for type completers (detecting cyclic references).An issue was that diverging types were no longer reported as cyclic, as in neg/t510.scala. I added a recursion depth limit for this case.
e.Tkeeps expanding toe.c.Ttoe.c.c.Tand so on.Note that removing
object Testfrom the example and compiling only the definitions runs into a stack overflow in a later phase, so the compiler is vulnerable to diverging types in other places.For the record, in Scala 3 catches stack overflows and reports "Recursion limit exceeded" errors.