Skip to content

True union types #1550

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

Merged
merged 15 commits into from
Oct 11, 2016
Merged

True union types #1550

merged 15 commits into from
Oct 11, 2016

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Oct 1, 2016

Keep union types, instead of approximating them by their dominators. This change surfaced
several other errors which were due to interactions of union types with other features.

Review by @smarter

@@ -2,7 +2,7 @@ class B(val x: Int)
class C(val x: Double)

object Test{
def bar(x: B | C): Int | Double = x.x
def bar(x: B | C): Int | Double = x.x // error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it an error now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is caused by the change to findMember, we now always approximate union prefixes, I think this should be reconsidered since it considerably diminishes the usability of union types, it also breaks the nice symmetry with selection on intersection types.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@odersky If you don't have the time to investigate the issues with union-types-narrow-prefix I could have a look next week.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smarter Do you have a realistic scenario where the utility of union types is diminished?

Copy link
Contributor Author

@odersky odersky Oct 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DarkDimius The previous findMember operation on union types would often fail to instantiate member types properly. I wonder whether that's an issue in the linker as well, since the linker seems to use union types pervasively.

Copy link
Contributor Author

@odersky odersky Oct 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To summarize it, the change is on the safe, conservative side. It's a simple way to avoid bugs and keep result types small (no propagation of disjunctions). It requires sometimes manual splitting, hence more verbose code. But it seems that code can always be transformed through manual splitting to fit the new model.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the linker, if disjunction propagation through selection is required, the linker could do the splitting itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another note: The change led to a 5% increase in compilation speed on the junit tests. Not a scientific benchmark, so it might not be a very exact number. But I was fearing it would slow compilation down, so it's nice to see an indication to the contrary.

Copy link
Contributor Author

@odersky odersky Oct 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smarter Actually, even splitting Select nodes is non-trivial. Can you come up with a safe splitting scheme that guarantees all types make sense? I couldn't, because splitting changes path identity. I think that highlights some of the problem we are facing here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also assume that there might be nice use cases when interfacing with JS APIs

I don't think widening the prefix of member selection will be an issue for JS APIs. Also, I agree with @odersky that the use cases look like code smell to me.

@nicolasstucki
Copy link
Contributor

Regarding dotty.language you should also remove it from Definitions. There is currently a lazy val LanguageModuleRef = ctx.requiredModule("dotty.language") and a lazy val Scala2LanguageModuleRef = ctx.requiredModule("scala.language").

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, there are some issues with the singleton type restriction but that's something that will be discussed in #1551

Replacing or types by their dominators and implicit conversions
caused the code to do the right thing anyway, but with the arrival
of true or-types, this became a static error.
Don't replace them by their dominators, unless one of the following holds:

 - language:Scala2 mode is on
 - we are at the point of findMember selection
 - we compare with a higher-kinded application

This means approximateUnion is now split into harmonizeUnion and
orDominator which each implement one of the former's two functionalities.
If entries are type variables, we have to check their
instances for equality.

This came up onder the new or handling scheme.
In fact all of dotty.language can be removed.
Makes sure the symbol in the tree can be approximately reconstructed by
calling member on the qualifier type.

Approximately means: The two symbols might be different but one still overrides
the other.
Splitting or types is no longer needed with new scheme.

Replacing idents with This nodes is better done in ExplicitSelf.

So splitter now just distributes applications into and ifs.
For the moment, we do not know how to handle something like

    1 | 2

or

    x.type | y.type

correctly. So it's better to disallow these situations until we find a proper
solution.
This happened for singletonOrs, and led to spurious errors there.
Test case: orInf.scala. This showed a problem where an `either` operation
had to arbitrarily pick one constraint over another, leading to a type
error down the line. What happened was that a `constrainResult` generated
the constraint

    Set[A] <: Set[String] | Set[Int]

But this constraint cannot be simplified without a cut and a resulting
loss of information. We avoid the problem by not constraining the result
if the prototype is a disjunction.
@smarter
Copy link
Member

smarter commented Oct 11, 2016

Rebasing before merging to make sure the tests still pass.

@smarter smarter merged commit f738201 into scala:master Oct 11, 2016
smarter added a commit to scala/scala3-example-project that referenced this pull request Oct 24, 2016
A method called on a union type has to be present in the supertype of
the union since scala/scala3#1550
@allanrenucci allanrenucci deleted the union-types branch December 14, 2017 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants