Generalize Approximating Type Maps #2908

odersky · 2017-07-24T09:54:54Z

Approximating type maps now work with bounds instead of NoTypes. This
gives more precision (in particular for type applications), and also
makes it possible to combine approximation with other mappings.

The biggest win here is that we can now express AsSeenFrom soundly in terms of ApproximatingTypeMaps. We can therefore remove the UnsafeNonVariant annotations and
retroactive skolemizations which felt kludgey.

liufengyun

Very nice refactoring 👍

liufengyun · 2017-08-04T23:49:20Z

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

+          case tp: ThisType =>
+            toPrefix(pre, cls, tp.cls)
+          case _: BoundType | NoPrefix =>
+            tp


tp == NoType will go all the way up to TypeMap.mapOver and test for all the cases. Maybe handle it here or even earlier?

I believe tp == NoType will only happen in exceptional cases. So we don't want to slow down the general logic by testing for it early.

Actually during the debugging, it's quite common, triggered from the Namer: https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/typer/Namer.scala#L1011

That's probably still a lot less than the other cases. To be sure either way we'd have to measure (i.e. add counters).

I agree, it's better to handle that special case in Namer instead of AsSeenFromMap.

liufengyun · 2017-08-04T23:51:25Z

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

+
+    override def toText(printer: Printer): Text =
+      lo.toText(printer) ~ ".." ~ hi.toText(printer)
+  }


Maybe add a note to Range -- its usage is limited to ApproximatingTypeMap, which is very special compared to other types.

Yes, good idea.

liufengyun · 2017-08-05T00:09:33Z

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

+              def distributeArgs(args: List[Type], tparams: List[ParamInfo]): Boolean = args match {
+                case Range(lo, hi) :: args1 =>
+                  val v = tparams.head.paramVariance
+                  if (v == 0) false


Missing "return" before "false"? Otherwise, it seems distributeArgs always return true.

Good catch. Wrong logic here.

liufengyun · 2017-08-05T00:16:33Z

compiler/src/dotty/tools/dotc/typer/Typer.scala

+                    else
+                      curOwner
+                  effectiveOwner.thisType.select(name, defDenot)
+                }


I still have no idea why this change becomes necessary despite the comments. Is it due to the change of variance for NamedTypes in TypeMaps?

I am not sure.

Approximating type maps now work with bounds instead of NoTypes. This gives more precision (in particular for type applications), and also makes it possible to combine approximation with other mappings. As a side-effect we provide the hooks for preventing constructing illegal types C#A where C is non-singleton and A is abstract by setting variance to 0 for the prefix of an abstract type selection. There's one test case that fails: One of the types in dependent-exractors.scala does not check out anymore. This has likely to do with the loss of precision incurred by the maps. Exact cause remains to be tracked down.

A Range, if it survives, will always lead to a situation where the upper bound appears in a covariant position in the result, and the lower bound appears in a contravariant position. Hence, when we apply a type map to the argument in a range we should take this into account. Fixes a previous failure in t2435.scala and dependent-extractors.scala.

Supersedes old scheme of dealing with unstable prefixes in non-variant positions.

It's no longer needed.

- Use range instead of Range in AsSeenFromMap#apply. We needed Range before because we did an incorrect variance computation for NamedTypes. - Refine derivedSelect

smarter

Overall I really like the simplification!

smarter · 2017-08-02T20:38:46Z

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

+    }
+
+    override protected def derivedClassInfo(tp: ClassInfo, pre: Type): Type = {
+      assert(!pre.isInstanceOf[Range])


This could use isRange defined above. Also, why is this condition always true?

smarter · 2017-08-02T20:39:13Z

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

-      if (tp1.exists && tp2.exists) tp.derivedAndOrType(tp1, tp2)
-      else if (tp.isAnd) approx(hi = tp1 & tp2)  // if one of tp1d, tp2d exists, it is the result of tp1d & tp2d
-      else approx(lo = tp1 & tp2)
+      if (tp1.isInstanceOf[Range] || tp2.isInstanceOf[Range])


This could use isRange defined above.

smarter · 2017-08-02T20:40:43Z

compiler/src/dotty/tools/dotc/printing/PlainPrinter.scala

+    try lo =:= hi
+    catch { case ex: Throwable => false }
+
+  private def homogenizeArg(tp: Type) = tp match {


I wouldn't use the word homogenize here since it's already used in the printer for the specific usecase of comparing trees before and after pickling.

In fact, the name was correct, but the condition to do this only under homogenizedView was lacking.

smarter · 2017-08-02T20:41:11Z

compiler/src/dotty/tools/dotc/printing/PlainPrinter.scala

@@ -68,6 +68,15 @@ class PlainPrinter(_ctx: Context) extends Printer {
      }
    else tp

+  private def sameBound(lo: Type, hi: Type): Boolean =
+    try lo =:= hi


Shouldn't this be frozen_=:= ?

smarter · 2017-08-10T00:03:46Z

compiler/src/dotty/tools/dotc/printing/PlainPrinter.scala

@@ -68,6 +68,15 @@ class PlainPrinter(_ctx: Context) extends Printer {
      }
    else tp

+  private def sameBound(lo: Type, hi: Type): Boolean =
+    try lo =:= hi
+    catch { case ex: Throwable => false }


I think this is a bit sloppy :). I would like NonFatal(ex) more, or even better: define a base trait that all non-fatal exceptions we throw in the typechecker must extend. (Or even even better: make the high-level APIs of the typer deal with exceptions themselves, clearly there is no value in propagating the exceptions up more if we're just ignoring them).

smarter · 2017-08-10T00:05:50Z

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

+                range(derivedRefinedType(tp, parent, lo), derivedRefinedType(tp, parent, hi))
+              tp.refinedInfo match {
+                case rinfo: TypeBounds =>
+                  val v = if (rinfo.isAlias) rinfo.variance * variance else variance


I don't understand what's going on here, could you add some comments? I've also checked and we have zero test cases where v does not end up being 0 here, so it's hard to judge the validity of the code.

smarter · 2017-08-10T00:06:29Z

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

+                case rinfo: TypeBounds =>
+                  val v = if (rinfo.isAlias) rinfo.variance * variance else variance
+                  if (v > 0) tp.derivedRefinedType(parent, tp.refinedName, rangeToBounds(info))
+                  else if (v < 0) propagate(infoHi, infoLo)


I find this especially suspicious, this seems to violate the invariant that Range#lo is a subtype of Range#hi, am I missing something?

smarter · 2017-08-10T00:07:31Z

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

+          if (args.exists(isRange)) {
+            if (variance > 0) tp.derivedAppliedType(tycon, args.map(rangeToBounds))
+            else {
+              val loBuf, hiBuf = new mutable.ListBuffer[Type]


I think this case is also complicated enough to warrant a comment :).

smarter · 2017-08-10T00:10:01Z

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

+      case _ => tp
+    }
+
+    protected def atVariance[T](v: Int)(op: => T): T = {


I think this is worth putting in TypeMap itself, considering how common the pattern val saved = variance; variance = ... is. Also maybe worth marking inline ?

smarter · 2017-08-10T00:12:25Z

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


+    protected def range(lo: Type, hi: Type) =


Calling this method range makes it a bit too easy to confuse it with Range. Perhaps call it between ?

The current situation is analogous to range / Range for collection ranges. I believe that's a good thing.

The logic before was overcomplicated since I did not take into account that Range refinedInfos could only happen at variance 0. On the othet hand, it did not take into account all the subtleties of alias types with variances.

odersky · 2017-08-13T18:45:01Z

I'll roll the comment fixes into #2945. Can you review that one next? 😄

smarter · 2017-08-13T18:53:42Z

Sure!

liufengyun · 2017-08-17T19:20:50Z

test performance please

dottybot · 2017-08-17T19:21:03Z

performance test scheduled: 1 jobs in total.

dottybot · 2017-08-17T20:05:18Z

Performance finished successfully:

Visit https://liufengyun.github.io/bench/2908 to see the changes.

smarter · 2017-08-22T13:25:37Z

Merged as part of #2970 (and also later as part of #2945)

odersky changed the title ~~Generalize Approximating Type Maps~~ [WIP] Generalize Approximating Type Maps Jul 25, 2017

odersky force-pushed the change-approx-typemap branch 2 times, most recently from 34dfff1 to 2c46850 Compare July 31, 2017 15:58

odersky requested review from smarter and liufengyun August 2, 2017 15:55

odersky changed the title ~~[WIP] Generalize Approximating Type Maps~~ Generalize Approximating Type Maps Aug 2, 2017

odersky mentioned this pull request Aug 2, 2017

Implement avoid in terms of AproximatingTypeMap #2945

Merged

odersky added the stat:needs review label Aug 2, 2017

liufengyun reviewed Aug 5, 2017

View reviewed changes

odersky added 8 commits August 9, 2017 10:23

Make asSeenFrom use an ApproximatingTypeMap

91ee02f

Supersedes old scheme of dealing with unstable prefixes in non-variant positions.

Drop UnsafeNonvariant

a26a309

It's no longer needed.

Simplifications

3459627

- Use range instead of Range in AsSeenFromMap#apply. We needed Range before because we did an incorrect variance computation for NamedTypes. - Refine derivedSelect

Refine derivedRefinedType

fa79a0e

Fix comment in test

08de553

Address reviewers comments

89180b5

odersky force-pushed the change-approx-typemap branch from a5a59b5 to 89180b5 Compare August 9, 2017 08:43

smarter reviewed Aug 10, 2017

View reviewed changes

odersky added 2 commits August 13, 2017 12:57

Factor out variance manipulation in TypeMap and TypeAccumulator

8c1a6dc

Fix derivedRefinedType in ApproximatingTypeMap

4160921

The logic before was overcomplicated since I did not take into account that Range refinedInfos could only happen at variance 0. On the othet hand, it did not take into account all the subtleties of alias types with variances.

odersky force-pushed the change-approx-typemap branch from 7818ed3 to 4160921 Compare August 13, 2017 17:24

odersky added 3 commits August 13, 2017 19:33

Add comment explaining approximating derivedAppliedType

396acbf

Polishings and more comments

e332bd3

Fix review comments re homogenizedArg

efbb1c6

felixmulder closed this Aug 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize Approximating Type Maps #2908

Generalize Approximating Type Maps #2908

odersky commented Jul 24, 2017 •

edited

Loading

liufengyun left a comment

liufengyun Aug 4, 2017

odersky Aug 9, 2017

liufengyun Aug 9, 2017

odersky Aug 9, 2017

liufengyun Aug 9, 2017

liufengyun Aug 4, 2017

odersky Aug 9, 2017

liufengyun Aug 5, 2017

odersky Aug 9, 2017

liufengyun Aug 5, 2017

odersky Aug 9, 2017

smarter left a comment

smarter Aug 2, 2017

smarter Aug 2, 2017

smarter Aug 2, 2017

odersky Aug 13, 2017

smarter Aug 2, 2017

smarter Aug 10, 2017

smarter Aug 10, 2017

smarter Aug 10, 2017

smarter Aug 10, 2017

smarter Aug 10, 2017

smarter Aug 10, 2017

odersky Aug 13, 2017

odersky commented Aug 13, 2017

smarter commented Aug 13, 2017

liufengyun commented Aug 17, 2017

dottybot commented Aug 17, 2017

dottybot commented Aug 17, 2017

smarter commented Aug 22, 2017 •

edited

Loading

Generalize Approximating Type Maps #2908

Generalize Approximating Type Maps #2908

Conversation

odersky commented Jul 24, 2017 • edited Loading

liufengyun 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odersky commented Aug 13, 2017

smarter commented Aug 13, 2017

liufengyun commented Aug 17, 2017

dottybot commented Aug 17, 2017

dottybot commented Aug 17, 2017

smarter commented Aug 22, 2017 • edited Loading

odersky commented Jul 24, 2017 •

edited

Loading

smarter commented Aug 22, 2017 •

edited

Loading