Fix #1045 #1053
Fix #1045 #1053
Conversation
This gives in general a supertype, that's OK for approximation. See ee76fda for an explanation.
See comment in Typer#approximateUnion for an explanation. Fixes scala#1045.
The test checks that Scala collections perform within 10x of Java collections. That's not something we need to test for dotty. And because of the heavily parallel execution of the tests it does not always hold. This is the second time in a a month that this particular test failed on jenkins. I think we lost enough cycles on it.
| return tp1.derivedRefinedType( | ||
| approximateUnion(OrType(tp1.parent, tp2.parent)), | ||
| tp1.refinedName, | ||
| homogenizedUnion(tp1.refinedInfo, tp2.refinedInfo).substRefinedThis(tp2, RefinedThis(tp1))) |
There was a problem hiding this comment.
Why not homogenizedUnion(tp1.refinedInfo, tp2.refinedInfo.substRefinedThis(tp2, RefinedThis(tp1))) ? This should require less traversals and is closer to what we were doing before ee76fda
|
Have you considered adding a parameter to |
|
@smarter After having thought about it, I believe it's better to leave approximateUnion and distributeOr separate. |
| tp.tp2 match { | ||
| case tp2: TypeProxy if !isClassRef(tp2) => | ||
| approximateUnion(tp.tp1 | next(tp2)) | ||
| def approximateOr(tp1: Type, tp2: Type)(implicit ctx: Context): Type = { |
There was a problem hiding this comment.
The implicit Context argument is unnecessary: TypeOps self-type is already a Context.
There was a problem hiding this comment.
Indeed. That was a leftover of a previous version where something got added to the context.
|
Otherwise LGTM |
2 participants
Thanks for putting me on the right track here @smarter. approximateUnion was indeed to blame.
In addition to distributing `|' inside refinements I also had to work on unifying type arguments.
Review by @smarter