Fix #10217: Avoid exponential behavior in derivesFrom #10271
Conversation
|
|
| loop(tp.tp1) && loop(tp.tp2) | ||
| || tp.tp1.isNullType && loop(tp.tp2) | ||
| || tp.tp2.isNullType && loop(tp.tp2) |
There was a problem hiding this comment.
Ther's a typo in here (loop(tp.tp2) should be loop(tp.tp1)), and this isn't quite the same thing as before; it doesn't handle T | Nothing, just T | Null. Instead of changing this I suggest changing isBottomType / isBottomTypeAfterErasure to use tp.isRef instead of tp.derivesFrom (you can't have a subclass of Null or Nothing anyway).
There was a problem hiding this comment.
Changing derivesFrom to isRef does not work for unions and intersections though.
| if defn.isBottomType(tp.tp1) then loop(tp.tp2) | ||
| else loop(tp.tp1) && (defn.isBottomType(tp.tp2) || loop(tp.tp2)) | ||
| loop(tp.tp1) && loop(tp.tp2) | ||
| || tp.tp1.isNullOrNothingType && loop(tp.tp2) |
There was a problem hiding this comment.
isBottomType doesn't return true for null when ctx.explicitNulls && !ctx.phase.erasedTypes is true but this will, breaking null-safety.
There was a problem hiding this comment.
Right. I changed it so that isBottomType no longer calls derivesFrom.
| if defn.isBottomType(tp.tp1) then loop(tp.tp2) | ||
| else loop(tp.tp1) && (defn.isBottomType(tp.tp2) || loop(tp.tp2)) | ||
| loop(tp.tp1) && (loop(tp.tp2) || defn.isBottomType(tp.tp2)) | ||
| || defn.isBottomType(tp.tp1) && loop(tp.tp2) |
There was a problem hiding this comment.
I think the behaviour of this is same as before, but if tp = Null | (Null | (Null | ( ... | A), wouldn't tp.derivesFrom(NullClass) cause the expensive loop(tp.tp2) to run two times? So I think use if is better here.
There was a problem hiding this comment.
Also, do we need to make sure OrType(NullType, NothingType).derivesFrom(NothingClass) == false?
tests/pos/i10217.scala
Outdated
|
|
||
| class Foo[T] | ||
|
|
||
| val f = Foo[A | B | C | D | E | F | G | H | I | J | K | L | M | N | O | P | Q | R | S | T | U] |
There was a problem hiding this comment.
Can we add another test Null | (Null | (Null | ( ... | A) here?
Also test right-associative combinations and combinations with Null
Need to widen before testing
Now uses a new method hasClassSymbol instead of recursively calling derivesFrom.
isNothing -> isExactlyNothing isTopType -> isExactlyAny Note: there is alrady an isAny, which is isRef(AnyClass) and therefore more general. Also: Move isBottomType from defn to Type.
|
I changed isBottomType to use a new method, which is linked with the I also did some moving around and renaming to standardize the way we test for top and bottom types. |
No description provided.