Streamline tryNormalize with underlyingMatchType
#20268
Conversation
EugeneFlesselle
changed the title
tryNormalize with underlyingMatchTypetryNormalize with underlyingMatchType
|
Test performance please |
|
test performance please |
|
performance test scheduled: 3 job(s) in queue, 1 running. |
I don't understand this premise, seeing as there's more than match types involved in normalizing: constant folds. |
Yes indeed, that's my point. Even though it is not inherently wrong, it is another discrepancy which makes them harder to work with than necessary. It's also certainly not obvious for someone unfamiliar with match types who's adding an |
| reduced.normalized | ||
| catch | ||
| case ex: Throwable => | ||
| handleRecursive("normalizing", s"${scrutinee.show} match ..." , ex) |
There was a problem hiding this comment.
Now that you've put back constant folding, we should probably keep the handleRecursive. Or, even better, put one in tryCompiletimeConstantFold.
|
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/20268/ to see the changes. Benchmarks is based on merging with main (79500f7) |
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
I don't event think it makes sense to call it a discrepancy, one is related to a subset of the other. But I'm not against improving things or just straight changing them under some vision on how we should do things, but it's useful to understand the motivating reasons (and verify those reasons). Also, on the overriding methods things, one reason is to cache things on private vars of the subclass, but I also think there might be some minutiae performance reasons, where using virtual method dispatch is faster and/or works better with the JIT, compared to a pattern match approach. Unlikely to be crucial here, but other times might be the reason for the setup. |
|
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/20268/ to see the changes. Benchmarks is based on merging with main (8825b07) |
Delay their normalization until it is needed. Avoids overflows from infinite match types that did not need to normalize. Also improves MatchTypeTraces as a side effect. It appears to have been added to avoid some separate issue, which seems to have been fixed. It is no longer needed since the previous fix with constant folding in disjointnessBoundary.
Also fixes underlyingMatchType to not use the resType of HKTypeLambdas It should only be in `isMatch` used for `AliasingBounds`, not `isMatchAlias`
There is already a `handleRecursive` in `reduced` Having the two makes error messages undeterministic, see scala#20269
|
test performance please (Just trying again to debug the bot, you can safely ignore this, sorry for the noise!) |
|
performance test scheduled: 2 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/20268/ to see the changes. Benchmarks is based on merging with main (48aac2c) |
EugeneFlesselle
force-pushed
the
match-types-tryNormalize
branch
from
49ddc83
to
70326f2
Compare
|
I disabled I'm not sure how the The error appears starting from 94a7765, I suspect it has to do with the stripping of a |
|
In -Ybest-effort we try to pickle and unpickle the erroring trees after typer, so it makes sense that a slightly different tree returned after typer could cause issues (usually with correct programs we know what we are going to pickle, with incorrect not really, especially that we are skipping PostTyper). With that said here in a weird way it makes more sense for it to fail, since there are those other issues with cyclic references that fail. As a rule of thumb I would not want to stop any progress in the compiler (which is more important than best-effort), which is part of why the blacklist is there. Please use it whenever you need to and do not worry about it! The important part for me is not to change the code gated by stuff like |
Based on #20168, #20257
This PR has multiple small but significant changes to
tryNormalize, I would recommend reviewing the commits individually.The overall goal was to have a more uniform treatment of
tryNormalizerather than the three overrides, making the logic easier to follow.It also now reuses
underlyingMatchTypefor it, which not only has a caching benefit but also ensures consistent results between them. In particular, makingtryNormalize.existsimplyunderlyingMatchType.exists, which one might assume as true but did not hold in general previously. And yet, we useisMatchAlias := underlyingMatchType.existsin a bunch of places with the assumption that there is nothing to normalise if it returns false.The next step will be to rework the MatchTypeTrace, which still overlooks some cases. But doing so should be simpler from the proposed setup.