Simplify handling of union types #2330

odersky · 2017-04-30T11:08:45Z

This is an attempt at simplifying the way we handle type unions. The central idea is that type unions should be treated analogous to singleton types: We should eliminate them in inferred types unless
they are explicitly given.

We already do a lot of that in many different contexts and by different means because type unions
have turned out to have problematic interactions with type inference. The aim of this PR is to simplify and generalize what we already do.

Risk of confusing with Types.SingletonType is too high.

The idea is that or-types should never be inferred, so we widen them - when instantiating type variables - when computing the type of valdef or defdefs from the rhs. This is already done in harmonizeUnion. However, harmonizeUnion had the restrictions that it joined or-types only in Scala2 mode. This restriction is now dropped.

- `ctx.harmonizeUnion(tp)` now becomes `tp.widenUnion`. - Same order of operations in local type inference of Namer and type variable instantiation. First, simplify, then widen, then widenUnion.

smarter · 2017-04-30T13:40:13Z

compiler/test/dotc/tests.scala

@@ -167,6 +167,7 @@ class tests extends CompilerTest {
  @Test def rewrites = compileFile(posScala2Dir, "rewrites", "-rewrite" :: scala2mode)

  @Test def pos_t8146a = compileFile(posSpecialDir, "t8146a")(allowDeepSubtypes)
+  @Test def pos_jon = compileFile(posSpecialDir, "jon")(allowDeepSubtypes)


Isn't it surprising that this requires deep subtypes?

I did not look too deeply. But SeqFactory has a funky recursive type

SeqFactory[CC[X] <: Seq[X] with GenericTraversableTemplate[X, CC]] extends GenSeqFactory[CC] with TraversableFactory[CC]

so it's not so implausible.

smarter · 2017-04-30T13:44:47Z

I guess this is a way to get:

val xs3 = mutable.ListBuffer(Right("foo"), Left(0), Right("bar"))

From the collection strawman to be inferred without unions. If so, it'd be good to add it as a testcase.

Suggestion: Never widen a union if the widened type would be Any, AnyRef or AnyVal, this is more likely to hide a bug than to be helpful, here's a (simplified) example where not widening discovered a bug in dotty:

sealed trait NegTestState
case class CompFailed() extends NegTestState
case class CompSucceeded() extends NegTestState

object Test { 
  val failureStates /* : List[CompSucceeded | CompFailed.type] */ = List(true, false) map {
    case true => CompSucceeded()
    case false => CompFailed // BUG: should be CompFailed()
  }

  failureStates.exists({
    case CompFailed() => // error: CompFailed is neither subtype
      true               // nor supertype of selector type
    case _ =>
      false
  })
}

smarter · 2017-04-30T14:08:15Z

Just updated my example in the previous comment so that it actually compiles with scalac but not with dotty, it was incorrect before.

smarter · 2017-04-30T14:15:26Z

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

+     *  re-lubbing it while allowing type parameters to be constrained further.
+     *  Any remaining union types are replaced by their joins.
+     *
+	   *  For instance, if `A` is an unconstrained type variable, then


Incorrect indentation around here.

smarter · 2017-04-30T14:15:51Z

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

@@ -830,23 +830,23 @@ object Types {
     *  def o: Outer
     *  <o.x.type>.widen = o.C
     */
-    @tailrec final def widen(implicit ctx: Context): Type = widenSingleton match {


Why was the tailrec annotation removed?

It was noise. I was against adding it, but it got merged before I could comment.

As I wrote then: Add a @tailrec only if it is important that the method is in fact tail-recursive. Either it is super-performance critical or can potentially recurse deeply. Do not add @tailrec just because the method happens to be tail recursive.

odersky · 2017-04-30T15:21:16Z

@smarter I don't think it's systematic to treat Any or AnyRef differently here. There is code out there that uses Any types pervasively (Spark comes to mind, as do some of our Lisp interpreters). It would be surprising if type inference started to fail.

odersky · 2017-04-30T15:22:05Z

Just updated my example in the previous comment so that it actually compiles with scalac but not with dotty, it was incorrect before.

It also compiles with dotty now 😄

odersky · 2017-04-30T15:23:52Z

From the collection strawman to be inferred without unions. If so, it'd be good to add it as a testcase.

Except that its type will change with the ProductN removal which needs to get in first. In any case we should get to a point where dotty and the collection strawman are always tested together.

smarter · 2017-04-30T15:27:48Z

It would be surprising if type inference started to fail.

On the other hand it might motivate people to avoid using Any :).

smarter · 2017-04-30T15:30:02Z

scalac even has a -Ywarn-infer-any that some people recommend using.

odersky · 2017-05-01T07:00:56Z

Yes, I think a warning is the right way to deal with this. We can implement that later.

smarter

LGTM

smarter · 2017-05-03T20:02:31Z

FWIW, timely discussion on the kind of issues I mentioned above: https://www.reddit.com/r/programming/comments/690fpx/when_the_scala_compiler_doesnt_help/

odersky · 2017-05-03T20:07:14Z

Yes, I guess we should implement this "warn on infer Any" flag ;-)

…

On Wed, May 3, 2017 at 10:02 PM, Guillaume Martres ***@***.*** > wrote: FWIW, timely discussion on the kind of issues I mentioned above: https://www.reddit.com/r/programming/comments/690fpx/ when_the_scala_compiler_doesnt_help/ — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2330 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAwlVlAqeG-N-5qFu1-flDB_DcolJJdLks5r2N1YgaJpZM4NMifh> . {"api_version":"1.0","publisher":{"api_key":" 05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity": {"external_key":"github/lampepfl/dotty","title":" lampepfl/dotty","subtitle":"GitHub repository","main_image_url":" https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88- 11e6-95fc-7290892c7bb5.png","avatar_image_url":"https:// cloud.githubusercontent.com/assets/143418/15842166/ 7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/lampepfl/dotty"}}," ***@***.*** in #2330: FWIW, timely discussion on the kind of issues I mentioned above: https://www.reddit.com/r/programming/comments/690fpx/ when_the_scala_compiler_doesnt_help/"}],"action":{"name":"View Pull Request","url":"#2330 (comment) 299019267"}}}

-- Prof. Martin Odersky LAMP/IC, EPFL

odersky added 4 commits April 30, 2017 11:47

More info on h comparisons in ExplainingTypeComparer

0aeb02c

Drop defn.SingletonType

54dd50c

Risk of confusing with Types.SingletonType is too high.

Polishings

cdc4eec

- `ctx.harmonizeUnion(tp)` now becomes `tp.widenUnion`. - Same order of operations in local type inference of Namer and type variable instantiation. First, simplify, then widen, then widenUnion.

odersky added stat:needs review stat:needs spec labels Apr 30, 2017

odersky changed the title ~~[WIP] Change handling of or-types~~ Simplify handling of union types Apr 30, 2017

smarter reviewed Apr 30, 2017

View reviewed changes

odersky mentioned this pull request Apr 30, 2017

Bug fixes to make collection strawman compile #2331

Merged

smarter reviewed Apr 30, 2017

View reviewed changes

smarter approved these changes May 1, 2017

View reviewed changes

smarter merged commit 9567fc4 into scala:master May 1, 2017

smarter mentioned this pull request May 17, 2017

Numeric widening and weak conformance #2451

Closed

allanrenucci deleted the change-minor-types branch December 14, 2017 16:58

allanrenucci mentioned this pull request Apr 11, 2018

Add some missing tailrec annotations for clarity #4280

Closed

abeln mentioned this pull request Sep 2, 2018

Don't widen T|JavaNull abeln/dotty#3

Closed

Simplify handling of union types #2330

Simplify handling of union types #2330

Uh oh!

Conversation

odersky commented Apr 30, 2017

Uh oh!

smarter Apr 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odersky Apr 30, 2017

Choose a reason for hiding this comment

Uh oh!

smarter commented Apr 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarter commented Apr 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarter Apr 30, 2017

Choose a reason for hiding this comment

Uh oh!

smarter Apr 30, 2017

Choose a reason for hiding this comment

Uh oh!

odersky Apr 30, 2017

Choose a reason for hiding this comment

Uh oh!

odersky commented Apr 30, 2017

Uh oh!

odersky commented Apr 30, 2017

Uh oh!

odersky commented Apr 30, 2017

Uh oh!

smarter commented Apr 30, 2017

Uh oh!

smarter commented Apr 30, 2017

Uh oh!

odersky commented May 1, 2017

Uh oh!

smarter left a comment

Choose a reason for hiding this comment

Uh oh!

smarter commented May 3, 2017

Uh oh!

odersky commented May 3, 2017 via email

Uh oh!

Uh oh!

smarter Apr 30, 2017 •

edited

Loading

smarter commented Apr 30, 2017 •

edited

Loading

smarter commented Apr 30, 2017 •

edited

Loading