Skip to content

[Experiment] Do not widen singletons in hard unions #14347

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

Closed

Conversation

mbovel
Copy link
Member

@mbovel mbovel commented Jan 25, 2022

This PR disables widening of union types if they are "hard". A "hard" union type—a union type that is not soft—is a union explicitly written by the user, as opposed to being created by the compiler (for example as the result of condition or a match).

Before:

val x: 2 | 3 = 2
val y /*: Int */ = x

After:

val x: 2 | 3 = 2
val y /*: 2 | 3 */ = x

In both cases, soft unions are widened:

val c /*: Int */  = if b then 2 else 3

@mbovel mbovel force-pushed the mb/do-not-widen-hard-unions branch from 7f2c5f0 to f81d2c8 Compare January 25, 2022 17:46
@mbovel mbovel marked this pull request as ready for review January 25, 2022 22:34
@mbovel mbovel requested review from smarter and odersky January 25, 2022 22:35
@mbovel mbovel self-assigned this Jan 25, 2022
@mbovel
Copy link
Member Author

mbovel commented Jan 25, 2022

So with my current changes, soft unions inside hard unions would be left untouched. Is that a problem?

If yes, I can probably do something more fine-grained by adding an isSoft parameter to widenSingletons indicating if the inner-most parent union is soft or not. If it is the case, then encountered singletons should be widened, otherwise they should not.

@@ -3266,7 +3267,7 @@ object Types {
TypeComparer.lub(tp1.widenUnionWithoutNull, tp2.widenUnionWithoutNull, canConstrain = true) match
case union: OrType => union.join
case res => res
else derivedOrType(tp1.widenUnionWithoutNull, tp2.widenUnionWithoutNull)
else this
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth flipping the order with your change:

if isSoft then
  if myUnionPeriod != ctx.period then
    ...
  myUnion
else this

@dwijnand
Copy link
Member

So with my current changes, soft unions inside hard unions would be left untouched. Is that a problem?

Is that even possible? Do you have an example? How does the user get their hands on a type that is a soft union in such a way they can explicitly write a hard union with it?

@mbovel
Copy link
Member Author

mbovel commented Jan 26, 2022

Is that even possible? Do you have an example? How does the user get their hands on a type that is a soft union in such a way they can explicitly write a hard union with it?

With my current changes:

def f(x: String): x.type | 3 = ???

val x /* : String */ = if b then "a" else "b"

val b: Boolean = ???
val y /* : "a" | "b" | 3 */ = f(if b then "a" else "b")
// Should it be : String | 3 ?

@dwijnand
Copy link
Member

Interesting, thank you. I would have thought that, unless x were an inline perhaps, that it could never be as narrow as "a" | "b". I would have thought you at least would have needed a [S <: String](x: S) for that. But that's just going off of memory and intuition, so either that behaviour is wrong or TIL. 😄

@mbovel
Copy link
Member Author

mbovel commented Jan 26, 2022

Yes we already have these precise types, but we rarely see them because they are often widened away. They are currently only preserved when the expected type is itself precise.

scala> def id(x: Any): x.type = x
def id(x: Any): x.type
                                                                                
scala> val b: Boolean = true
val b: Boolean = true
                                                                                
scala> val y: 2 | 3 = id(if b then 2 else 3)
val y: 2 | 3 = 2

scala> val z = id(if b then 2 else 3)
val z: Int = 2

scala> def id2[T](x: T): T = x
def id2[T](x: T): T
                                                                                
scala> val y2: 2 | 3 = id2(if b then 2 else 3)
val y2: 2 | 3 = 2

@dwijnand
Copy link
Member

scala> val y: 2 | 3 = id(if b then 2 else 3)
val y: 2 | 3 = 2

scala> val z = id(if b then 2 else 3)
val z: Int = 2

Given this I'd say, yeah, we should change it. I'm thinking ideally to:

scala> val y = f(if b then "a" else "b")
val y: String | 3 = f(if b then "a" else "b")

scala> val y: "a" | "b" | 3 = f(if b then "a" else "b")
val y: "a" | "b" | 3 = f(if b then "a" else "b")

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Yes, I think that's a net improvement for type inference!

object Test4:
def f(a: 2 | 3) = a

def test() =
Copy link
Contributor

@odersky odersky Jan 26, 2022

Choose a reason for hiding this comment

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

Consider adding a test where the union is the result type of a def.

@mbovel mbovel changed the title Do not widen hard unions Do not widen singletons in hard unions Jan 31, 2022
@mbovel
Copy link
Member Author

mbovel commented Jan 31, 2022

Closing in favor of #14360.

@mbovel mbovel closed this Jan 31, 2022
@mbovel mbovel changed the title Do not widen singletons in hard unions [Experiment] Do not widen singletons in hard unions Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants