-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Re-lub also hard union types in simplify #20027
Conversation
A plausible theory is that hard unions were not re-lubbed as an optimization. The thinking was: the union is given explicitly - why second guess and check again whether we can simplify the type? But that's clearly not true after variable instantiation, as #10693 shows. |
In retrospect I feel stupid. Soft vs hard unions were introduced in #10198, and with it the restriction that hard unions would not be re-lubbed. #10693 arrived barely a month after this PR was merged. I should have seen a connection. That said, it's a bit hard to see what our lub operation can do using mergeIfSuper, and how much in compute time that costs. So maybe I should try to simplify and clarify that next. |
val v3 = test(v1, v0) | ||
val v4 = test(v2, v3) | ||
val v5 = test(v3, v2) | ||
val v6 = test(v4, v5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm glad the issue is being resolved, but I don't understand this test.
How can we see that for instance the type of v2
isn't String | Int | Int | String
? Don't we want to show that it is inferred as Int | String
or String | Int
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now: manual inspection after -Xprint:typer. It would be good if someone could add a better test. We don't have good infrastructure for inferred types complexity yet. Maybe the presentation compiler tests have some of the necessary setup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now this bit I had figured out!
Its a bit of a kludge, but In #20014 I had a test that was actually validating this via the repl tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! That's a good idea to use repl tests for that. I added your test to the PR.
Simplify had some elaborate condition that prevented hard union types to be recomputed with a lub. I am not sure why that was. In the concrete scenario of i10693.scala, we had an explicitly union result type `B | A` where `A` and `B` are type parameters. So that is a hard union type. Then `A` was instantiated by `Int | String` and `B` was instantiated by `String | Int`. Re-forming the lub of that union would have eliminated one pair, but since the union type was hard tyat was not done. On the other hand I see no reason why hard unions should not be re-lubbed. Hard unions are about preventing the widening of or types with a join. I don't see a connection with avoiding re-lubbing. Fixes scala#10693
Simplify had some elaborate condition that prevented hard union types to be recomputed with a lub. I am not sure why that was. In the concrete scenario of i10693.scala, we had an explicitly written union result type
B | A
whereA
andB
are type parameters. So that is a hard union type. ThenA
was instantiated toInt | String
andB
was instantiated toString | Int
. Re-forming the lub of that union would have eliminated one pair, but since the union type was hard that was not done. On the other hand I see no reason why hard unions should not be re-lubbed. Hard unions are about preventing the widening of or types with a join. I don't see a connection with avoiding re-lubbing.Fixes #10693