Fix #6288: Allow Singletons in Unions #6299

odersky · 2019-04-13T14:17:46Z

No description provided.

smarter · 2019-04-13T14:19:51Z

For reference, here's the open issue about this: #1551

odersky · 2019-04-13T19:39:56Z

test performance please

dottybot · 2019-04-13T19:40:20Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2019-04-13T21:32:53Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/6299/ to see the changes.

Benchmarks is based on merging with master (c3e88b7)

odersky · 2019-04-14T09:02:39Z

test performance please

dottybot · 2019-04-14T09:02:42Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2019-04-14T10:52:08Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/6299/ to see the changes.

Benchmarks is based on merging with master (c3e88b7)

odersky · 2019-04-15T08:34:30Z

test performance please

dottybot · 2019-04-15T08:36:21Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2019-04-15T10:25:58Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/6299/ to see the changes.

Benchmarks is based on merging with master (25a3773)

odersky · 2019-04-15T10:33:05Z

test performance please

dottybot · 2019-04-15T10:35:07Z

performance test scheduled: 2 job(s) in queue, 1 running.

dottybot · 2019-04-15T15:01:47Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/6299/ to see the changes.

Benchmarks is based on merging with master (8e323b1)

odersky · 2019-04-16T11:50:34Z

@nicolasstucki I had to disable compiling tasty-interpreters in BootstrappedOnlyCompile since I got errors of this form:

51 |    else jvmReflection.interpretNew(fn.symbol, evaluatedArgss(argss))
   |                                    ^^^^^^^^^
   |                                    Found:    Interpreter.this.reflect.Symbol
   |                                    Required: Interpreter.this.jvmReflection.reflect'.Symbol
   |                                    
   |                                    where:    reflect  is a value in class TreeInterpreter
   |                                              reflect' is a value in class JVMReflection

Can you take a look what went wrong here, and hopefully bring them back?

odersky · 2019-04-17T12:26:53Z

test performance please

dottybot · 2019-04-17T12:27:46Z

performance test scheduled: 2 job(s) in queue, 1 running.

dottybot · 2019-04-17T16:19:31Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/6299/ to see the changes.

Benchmarks is based on merging with master (afb7a8b)

nicolasstucki

Otherwise LGTM

nicolasstucki · 2019-04-24T12:59:10Z

compiler/src/dotty/tools/dotc/core/TypeComparer.scala

+  def canCompare(atoms: Set[Type]): Boolean =
+    ctx.phase.isTyper || {
+      val hasSkolems = new ExistsAccumulator(_.isInstanceOf[SkolemType]) {
+        override val stopAtStatic = true


This should be a def

nicolasstucki · 2019-04-24T13:06:21Z

tests/pos/i6288.scala

+  val x = 3
+  opaque type T = x.type
+  object T {
+    def wrap(a: x.type): T = a   // Error


Is this intended to be a neg test?

nicolasstucki · 2019-04-24T13:22:07Z

I added a commit fixing the checkfile of tasty-extractors-1 and then reverted it because there was a conflict with master. I will fix it once this is merged.

smarter

Please rebase to make sure this doesn't conflict with recent changes such as the full bootstrap.

smarter · 2019-04-24T13:25:42Z

compiler/src/dotty/tools/dotc/core/TypeComparer.scala

+   *  two different skolems in all phases `p`, where `p.isTyper` is false.
+   *  But in that case comparing two sets of atoms that contain skolems
+   *  for equality would give the wrong result, so we should not use the sets
+   *  for comparisons.


Isn't not using the sets a potential performance problem ? Have you considered having both OrType#atoms and OrType#atomsUnifySkolems instead, where the latter would replace all skolems referring to the same type by a single instance ?

I believe skolems are sufficiently rare. If it turns out that we get a performance problem, what you suggest could be a good alternative.

smarter · 2019-04-24T13:29:42Z

compiler/src/dotty/tools/dotc/core/TypeComparer.scala

@@ -1499,30 +1542,38 @@ class TypeComparer(initctx: Context) extends ConstraintHandling[AbsentContext] {
  /** The greatest lower bound of a list types */
  final def glb(tps: List[Type]): Type = ((AnyType: Type) /: tps)(glb)

+  def widenInUnions(implicit ctx: Context): Boolean = ctx.scala2Mode || ctx.erasedTypes


Do we have examples of Scala 2 code that breaks without the scala2Mode here ?

I haven't tried. But it's better to be conservative, I think.

I'm not sure, all these inference differences (that we'll have to document at some point) make it more risky for a codebase to turn off Scala2 mode since there's a non-zero chance any of them could cause a slient change of runtime behavior (e.g. because a different implicit ends up being selected). They're also only lightly tested since most of our tests are run without Scala2 mode enabled, meaning a higher risk of unforeseen interactions. So I'd limit them to things that significantly help in cross-compilation, but we can revisit this some other time.

smarter · 2019-04-24T13:42:54Z

compiler/src/dotty/tools/dotc/core/TypeOps.scala

+        val isSingle1 = tp1.isInstanceOf[SingletonType]
+        val isSingle2 = tp2.isInstanceOf[SingletonType]
+        return {
+          if (tp2w eq tp2) orDominator(tp1w | tp2)


Would be helpful to add comments like // 2.1 above conditions to relate them with the comment above and make sure they stay in sync in the future.

smarter · 2019-04-24T13:47:49Z

compiler/src/dotty/tools/dotc/core/TypeOps.scala

+          else if (tp2w frozen_<:< tp1w) orDominator(tp1 | tp2w)
+          else if (isSingle1 && !isSingle2) orDominator(tp1w | tp2)
+          else if (isSingle2 && !isSingle1) orDominator(tp1 | tp2w)
+          else if (tp1 frozen_<:< tp2w) tp2w


Is a case for 2.5 looking like else if (tp2 frozen_<:< tp1w) tp1w missing here ?

smarter · 2019-04-24T13:48:52Z

compiler/src/dotty/tools/dotc/core/TypeOps.scala

+          else if (isSingle1 && !isSingle2) orDominator(tp1w | tp2)
+          else if (isSingle2 && !isSingle1) orDominator(tp1 | tp2w)
+          else if (tp1 frozen_<:< tp2w) tp2w
+          else orDominator(tp1w | tp2)


2.6 says "pick tp1", that doesn't seem to match the code.

smarter · 2019-04-24T13:49:21Z

compiler/src/dotty/tools/dotc/core/Types.scala

+        tp
+    }
+
+    /** Widen all top-level singletons reachable by dealising


Suggested change

/** Widen all top-level singletons reachable by dealising

/** Widen all top-level singletons reachable by dealiasing

odersky · 2019-04-29T21:06:12Z

@nicolasstucki I tried to update the check file but still get errors for the runMacros test. Can you take a look? Thanks!

Widen in lubs only if - the type is unstable, or - we are after erasure This need to be complemented by performance work as otherwise complexity of subtype checking will explode.

We also need to undo the RefinedType self type of opaque type companions. Otherwise, programs die in erasure if tracing is enabled.

Since we do not widen singletons in unions, it is now easy to get large union types consisting of singletons. We need to specialize the code dealing with them in order not to get deep subtype recursions and drastic compilation speed slowdowns.

This was the situation before the PR. I thought that simply checking whether the instance is a subtype of the bound is equivalent, but compare-singletons.scala shows that this is not the case. Here, we have a type variable X such that `r.type <:< X <:< Singleton` where `r: R` and `R <: Singleton`. With the previous changes in this PR, we instantiate `X` to `R`. With the revert in this commit we instantiate it to `r.type` instead.

This relieves some of the load of `join` and offers more opportunities to improve union types by constraining further. Without this step we might lose some improvements compared to the situation before where we always widened singletons in OrTypes. I don't have a test case that demonstrates the difference, though.

This was detected when failing to compile the 2.13 library. The problem is minimized in test pos/padTo.scala. The failure arose because with the introduction of singletons in unions we get more "interesting" union types to approximate. But it can be also produced without singleton types. Further join refinements

Using --update-checkfiles

odersky force-pushed the fix-#6288 branch from 03b5a7c to 80d0e17 Compare April 13, 2019 17:29

odersky marked this pull request as ready for review April 15, 2019 10:33

odersky force-pushed the fix-#6288 branch from 7019e9a to 8586c02 Compare April 16, 2019 10:55

odersky assigned nicolasstucki Apr 16, 2019

odersky requested a review from nicolasstucki April 16, 2019 11:48

odersky force-pushed the fix-#6288 branch 2 times, most recently from 263091b to 2e9084e Compare April 17, 2019 11:58

nicolasstucki reviewed Apr 24, 2019

View reviewed changes

nicolasstucki force-pushed the fix-#6288 branch from 6343bb7 to 50154cf Compare April 24, 2019 13:12

smarter requested changes Apr 24, 2019

View reviewed changes

odersky force-pushed the fix-#6288 branch from 50154cf to 3367d5b Compare April 29, 2019 17:51

odersky and others added 20 commits May 2, 2019 15:30

Don't widen in lubs

8e2221d

Widen in lubs only if - the type is unstable, or - we are after erasure This need to be complemented by performance work as otherwise complexity of subtype checking will explode.

Another fix to ElimOpaque

4c5988f

We also need to undo the RefinedType self type of opaque type companions. Otherwise, programs die in erasure if tracing is enabled.

Specialize lub for unions over singletons

f484851

WIP: Try to simplify widenInferred algorithm

d95a239

Combine new widenInferred scheme with singleton unions

73eb00b

Remove duplication in widenInferred

a3933f3

Update docs

8a06b0b

Tweak for safe atoms comparisons after Typer

f7bf314

Print stack trace only under -Ydebug

be6eb06

More test cases

ca73e1d

Normalize atoms in depth

eed8f2d

Clarify comment

5d41c0a

Update check file

e46bcd1

Address review comments

227dfa9

Fix check file

f15016a

Using --update-checkfiles

Fix comment

1083d9d

odersky force-pushed the fix-#6288 branch from 09063fd to 1083d9d Compare May 2, 2019 13:32

odersky added 2 commits May 2, 2019 15:39

Last fixes

ef8ccf7

Blacklist a from tasty test

b940f11

smarter approved these changes May 2, 2019

View reviewed changes

odersky merged commit 63d8546 into scala:master May 2, 2019

smarter mentioned this pull request May 2, 2019

Allow Singleton types in Union types #1551

Closed

joost-de-vries mentioned this pull request May 3, 2019

Using union types together with literal singleton types #1495

Closed

allanrenucci deleted the fix-#6288 branch May 3, 2019 08:08

anatoliykmetyuk added this to the 0.15 Tech Preview milestone May 21, 2019

smarter mentioned this pull request May 27, 2019

Union types LUB Object in nested function application #6565

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #6288: Allow Singletons in Unions #6299

Fix #6288: Allow Singletons in Unions #6299

odersky commented Apr 13, 2019

smarter commented Apr 13, 2019

odersky commented Apr 13, 2019

dottybot commented Apr 13, 2019

dottybot commented Apr 13, 2019

odersky commented Apr 14, 2019

dottybot commented Apr 14, 2019

dottybot commented Apr 14, 2019

odersky commented Apr 15, 2019

dottybot commented Apr 15, 2019

dottybot commented Apr 15, 2019

odersky commented Apr 15, 2019

dottybot commented Apr 15, 2019

dottybot commented Apr 15, 2019

odersky commented Apr 16, 2019

odersky commented Apr 17, 2019

dottybot commented Apr 17, 2019

dottybot commented Apr 17, 2019

nicolasstucki left a comment

nicolasstucki Apr 24, 2019

nicolasstucki Apr 24, 2019

nicolasstucki commented Apr 24, 2019

smarter left a comment

smarter Apr 24, 2019

odersky Apr 29, 2019

smarter Apr 24, 2019

odersky Apr 29, 2019

smarter Apr 29, 2019

smarter Apr 24, 2019

smarter Apr 24, 2019

smarter Apr 24, 2019

smarter Apr 24, 2019

odersky commented Apr 29, 2019

	/** Widen all top-level singletons reachable by dealising
	/** Widen all top-level singletons reachable by dealiasing

Fix #6288: Allow Singletons in Unions #6299

Fix #6288: Allow Singletons in Unions #6299

Conversation

odersky commented Apr 13, 2019

smarter commented Apr 13, 2019

odersky commented Apr 13, 2019

dottybot commented Apr 13, 2019

dottybot commented Apr 13, 2019

odersky commented Apr 14, 2019

dottybot commented Apr 14, 2019

dottybot commented Apr 14, 2019

odersky commented Apr 15, 2019

dottybot commented Apr 15, 2019

dottybot commented Apr 15, 2019

odersky commented Apr 15, 2019

dottybot commented Apr 15, 2019

dottybot commented Apr 15, 2019

odersky commented Apr 16, 2019

odersky commented Apr 17, 2019

dottybot commented Apr 17, 2019

dottybot commented Apr 17, 2019

nicolasstucki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolasstucki commented Apr 24, 2019

smarter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odersky commented Apr 29, 2019