Merge isInstanceOfEvaluator with TypeTestCasts #2820
Conversation
As the saying goes: "let erasure handle it... " The reason for the change is that things were a mess before - isInstanceOfEvaluator did not handle &/| types. It would be hard to change that since it was looking at erased types. - isInstanceEvaluator, but not TypeTestCasts added null-checks - therefore, necessary null-checks were not emitted on some tests of the form `x.isInstanceOf[A & B]`. - the pattern matcher patched this up by inserting some null checks on its own, whereas it should just have let the translation of isInstanceOf do the job.
To be documented once we figured out whether it works.
The optimizer seems to do some optimizations regarding simplifications of pattern matchings. This masks errors to be reported later at erasure. The optimizer should perform these actions only after erasure.
Adding those null checks is not necessary: Am I missing some other case? |
|
isInstanceOfEvaluator does not work if the tested type is of the form A | B or A & B. |
|
Aaah, I missed that those points rely on the first one. Thanks! |
- drop a test which prevented a check in isInstanceOfEvaluator but which can be dropped with the more precise types in TypeTestCasts - move a test which was only partially effective in RefChecks to TypeTestCasts (problem is again testing against &/| types).
odersky
force-pushed
the
fix-isInstanceOf
branch
from
1005e69 to
11961bb
Compare
|
I believe we have now concentrated all logic around isInstanceOf in TypeTestCasts. Before this was distributed in various checks and rewrites in RefChecks, isInstanceOfEvaluator and TypeTestCasts. There remain some null checks in the pattern matcher which should now be redundant. There's also the outer test generated by pattern matcher, but inserting that at Erasure is too late (we'd need to do an ensureExplicitOuter before). Local opts before erasure should be changed so that always failing instance tests are not replaced by |
|
Looking at line counts, IsInstanceOfEvaluator is now no longer used, even though it is left in the repo for now, in case we want to transfer some of its bits. That's a savings of 173 lines, for a total reduction of ~130 lines. |
I suggest getting rid of it, otherwise it's too easy to get confused and think it's used code. Also a big 👍 from me for this PR, this is much more sane than the previous situation. |
|
See also scala/bug#10379 which also affects dotty |
|
@smarter We actually have quite a few miniphases which are not linked but are still in the repo. Maybe it's time for a global sweep to remove all of them? |
|
Sounds good to me. |
| cpy.TypeApply(tree)(expr1.select(sym).withPos(expr.pos), List(TypeTree(tp))) | ||
|
|
||
| def foundCls = expr.tpe.widen.classSymbol | ||
| // println(i"ta $tree, found = $foundCls") |
There was a problem hiding this comment.
Forgotten removal - or leave in as a nice point for debug?
As the saying goes: "let erasure handle it... "
The reason for the change is that things were a mess before
be hard to change that since it was looking at erased types.
the form
x.isInstanceOf[A & B].on its own, whereas it should just have let the translation of
isInstanceOf do the job.