Preserve singletons in unions when they're explicitly written in the code

[smarter]

There are two cases where we should not widen singletons in unions:

• When we explicitly write the type, like val x: 1 | 2
• When pattern matching binds an alternative, like case x @ (1 | 2) =>

Fixes #829.

Review by @odersky .

[smarter]

Keeping singletons in unions for pattern alternatives causes very deep subtype recursions in https://github.com/lampepfl/dotty/blob/master/src/dotty/tools/dotc/parsing/JavaScanners.scala#L56 which we disallow in our tests by default, so I'll disable that for now.

@odersky : do you think we should preserve singleton types in alternatives, even if it causes deep subtyping checks?

[odersky]

I'd tend to not preserve singletons in alternatives. In

x @ (1 | 2)

you don't write the singleton type explicitly. I think it is fair to say x
has type Int here. Q: If you want the union of singleton types, can you get
it by writing

x: (1 | 2)

?

• Martin

On Thu, Oct 22, 2015 at 5:56 PM, Guillaume Martres <notifications@github.com

wrote:

Keeping singletons in unions for pattern alternatives causes very deep
subtype recursions in
https://github.com/lampepfl/dotty/blob/master/src/dotty/tools/dotc/parsing/JavaScanners.scala#L56
which we disallow in our tests by default, so I'll disable that for now.

@odersky https://github.com/odersky : do you think we should preserve
singleton types in alternatives, even if it causes deep subtyping checks?

—
Reply to this email directly or view it on GitHub
#844 (comment).

Martin Odersky
EPFL

[smarter]

Q: If you want the union of singleton types, can you get it by writing x: (1 | 2) ?

Yes, that now works: https://github.com/smarter/dotty/blob/fix/lub-singletons/tests/pos/singletons-lubs.scala#L7

[odersky]

Simplified is called in lots of places for types that are not written. So it should not do the lub with keepSingletons = true.

[smarter]

I think this is OK, because if we call simplified on a type not written by a user which contains an OrType, then this OrType should have been constructed in the code by calling | or lub, so this OrType should not have a singleton in it. And if we do not keep singletons in simplified, then we will always lose them because we call simplified in Typer#adapt.

[odersky]

I am not sure about that. Constraint handling does not do lubs. The purpose of simplify is to clean up after it. See it this way: If simplify only gets code where ortypes are computed by lubs, why would it do the lubs again?

[smarter]

If simplify only gets code where ortypes are computed by lubs, why would it do the lubs again?

Because we call simplify on the types inside the OrType, and the lub of the result might not be an OrType?

I am not sure about that. Constraint handling does not do lubs.

OK, so this should only have an impact when we instantiate type variables, but we already approximate unions in that case: https://github.com/lampepfl/dotty/blob/master/src/dotty/tools/dotc/core/Types.scala#L2481 isn't that enough? If it's not, we can always special case the .simplified call when instantiating type variables to not keep singletons: https://github.com/lampepfl/dotty/blob/master/src/dotty/tools/dotc/core/Types.scala#L2475

[odersky]

Thinking about it, I found a different way to do it. Drop the boolean parameters (boolean parameters that have to be passed through several intermediaries are a code smell anyways). Create a new function

simplify(tree: Tree)(implicit ctx: Context): Tree = tree match {
  case OrTypeTree(l, r) if l.tpe.isConstant && r.tpe.isConstant => 
  case _ => tree.overwriteType(tree.tpe.simplified)
}

Then replace

tree.overwriteType(tree.tpe.simplified)

in adapt with simplify(tree).

That should do it, right? And we are sure it would not do too much.

[smarter]

  case OrTypeTree(l, r) if l.tpe.isConstant && r.tpe.isConstant => 

We have to deal with explicit singleton types which are not constant types too if we want this to not compile:

val a: Int = 1
val b: Int = 2
val c: Int = 3
val d: Int = 4
val foo: a.type | b.type = 1
val bar: c.type | d.type = foo

[odersky]

OK, replace isConstant with .isInstanceOf[SingletonType] then.

On Mon, Oct 26, 2015 at 1:06 PM, Guillaume Martres <notifications@github.com

wrote:

case OrTypeTree(l, r) if l.tpe.isConstant && r.tpe.isConstant =>

We have to deal with explicit singleton types which are not constant types
too if we want this to not compile:

val a: Int = 1val b: Int = 2val c: Int = 3val d: Int = 4val foo: a.type | b.type = 1val bar: c.type | d.type = foo

—
Reply to this email directly or view it on GitHub
#844 (comment).

Martin Odersky
EPFL

[smarter]

OK, but then:

case OrTypeTree(l, r) if l.tpe.isInstanceOf[SingletonType] && r.tpe.isInstanceOf[SingletonType] =>

What about String | 1 ? What if the singleton is deep inside the type, like Foo[A & (B | 1)] ?

I still think that the PR as is should work, could you give an example where we could end up with a tree whose type contains a singleton type not written by the user?

[odersky]

In fact, I think we need to change simplify to not touch types at all, except for mapping PolyParams to type variables. But if I write A & B I expect to get what I write and not a simplified version of it.

[odersky]

My main issue with this approach is that it is operational only. We need a spec that says clearly when singletons are widened. I can't see how to develop such a spec from the current implementation. So how about we do this first, and then decide on the PR?

[smarter]

Sure, that sounds like a good idea.

[odersky]

I opened a new issue #1551 that explains the problem from the perspective of #1550.