Streamline tryNormalize with underlyingMatchType

[EugeneFlesselle]

Based on #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 tryNormalize rather than the three overrides, making the logic easier to follow.

It also now reuses underlyingMatchType for it, which not only has a caching benefit but also ensures consistent results between them. In particular, making tryNormalize.exists imply underlyingMatchType.exists, which one might assume as true but did not hold in general previously. And yet, we use isMatchAlias := underlyingMatchType.exists in 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.

[odersky]

Looks good to me. Let's just run a performance test to make sure there is no regression.

[odersky]

Test performance please

[EugeneFlesselle]

@sjrd this still depends on eef62f7

[mbovel]

test performance please

[dottybot]

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

[dwijnand]

In particular, making tryNormalize.exists imply underlyingMatchType.exists, which one might assume as true but did not hold in general previously.

I don't understand this premise, seeing as there's more than match types involved in normalizing: constant folds.

[EugeneFlesselle]

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 isMatchAlias guard.

[dwijnand]

Now that you've put back constant folding, we should probably keep the handleRecursive. Or, even better, put one in tryCompiletimeConstantFold.

[dottybot]

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/20268/ to see the changes.

Benchmarks is based on merging with main (79500f7)

[odersky]

test performance please

[dottybot]

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

[dwijnand]

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 isMatchAlias guard.

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.

[dottybot]

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/20268/ to see the changes.

Benchmarks is based on merging with main (8825b07)

[EugeneFlesselle]

I disabled i974 from the best-effort-pickling tests, at least to test if it is the only failing case.

I'm not sure how the --Ybest-effort affects things here, but I saw there are already a few blacklisted tests. @jchyb any idea what might be happening ?

The error appears starting from 94a7765, I suspect it has to do with the stripping of a LazyRef which did not happen before.

[jchyb]

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 ctx.isBestEffort

[EugeneFlesselle]

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 ctx.isBestEffort

Sounds good, thanks for the detailed explanations Jan.

[EugeneFlesselle]

@dwijnand I know the related changes/comments have been on varying places, sorry about that. Everything should be up to date and rebased on this PR. Do you think it would worth going back to an overloading setup here regarding performance, or have other concerns with the changes ?