Simplify merging in lub/glb, avoid unnecessary constraints #9053
Conversation
In `mergeIfSuper`, to simplify `tp1 | tp2`, we first check if `tp2` can be made a subtype of `tp1`. If so we could just return `tp1` but this isn't what we did before this commit: at that point we checked if `tp1` could be made a subtype of `tp2` and in that case prefered returning `tp2` to `tp1`. I haven't been able to find a reason for this (the comment says "keep existing type if possible" which I don't understand). On the other hand, I've found cases where this logic broke type inference because the second subtype check inferred extraneous constraints (see added testcase). So this commit simply removes this logic, it does the same for `mergeIfSub` which contains similar logic to simplify `tp1 & tp2`, though this one is less problematic since it always runs with frozen constraints.
|
check performance please |
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/9053/ to see the changes. Benchmarks is based on merging with master (587945b) |
|
Let's try the performance test again. I really wish we had more precision here. |
|
test performance please |
1 similar comment
|
test performance please |
|
performance test scheduled: 2 job(s) in queue, 1 running. |
|
test performance please |
|
performance tests seem to have a mind of their own whether they agree to run something 😉 |
|
@liufengyun is the benchmark test stuck? |
|
I have manually scheduled the job. The benchmark test encountered a problem related to a mistake in the PR #9055 (see lampepfl/bench#201), I've fixed that manually. We fixed one problem related to the bot being not responsive recently (lampepfl/bench#170). But it seems there might be other problems (most likely related to network). I'll have a look. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/9053/ to see the changes. Benchmarks is based on merging with master (e5e489e) |
In
mergeIfSuper, to simplifytp1 | tp2, we first check iftp2canbe made a subtype of
tp1. If so we could just returntp1but thisisn't what we did before this commit: at that point we checked if
tp1could be made a subtype of
tp2and in that case prefered returningtp2totp1. I haven't been able to find a reason for this (thecomment says "keep existing type if possible" which I don't understand).
On the other hand, I've found cases where this logic broke type
inference because the second subtype check inferred extraneous
constraints (see added testcase). So this commit simply removes this
logic, it does the same for
mergeIfSubwhich contains similar logic tosimplify
tp1 & tp2, though this one is less problematic since italways runs with frozen constraints.