Implement avoid in terms of AproximatingTypeMap #2945

odersky · 2017-08-02T16:00:52Z

Based on #2908. Last two commits are new.

smarter · 2017-08-13T21:14:26Z

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

@@ -64,9 +64,19 @@ trait TypeAssigner {
  }

  /** An upper approximation of the given type `tp` that does not refer to any symbol in `symsToAvoid`.
+   *  We need to approximate with ranges:
+   *
+   *    term references to symbols in `symsToAvoid`,


This would be easier to read with - in front of each list item.

smarter · 2017-08-13T21:14:50Z

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

-      /** Needs to handle the case where the prefix type does not have a member
-       *  named `tp.name` anymmore.
+      /** Two deviations from standard derivedSelect:
+       *   1. The teh approximation result is a singleton references C#x.type, we


Extra teh in the sentence ?

smarter · 2017-08-13T21:21:20Z

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

+    val parentType = info.parentsWithArgs.reduceLeft(ctx.typeComparer.andType(_, _))
+    def addRefinement(parent: Type, decl: Symbol) = {
+      val inherited =
+        parentType.findMember(decl.name, info.cls.thisType, Private)


I would explicitly write excluded = Private for clarity.

smarter · 2017-08-13T21:38:38Z

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

          }
+          avoided


I guess the intermediate avoided was used for debugging, but its definition could be inlined here.

smarter · 2017-08-13T21:46:58Z

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

+    def apply(tp: Type): Type = {
+
+      /** Map a `C.this` type to the right prefix. If the prefix is unstable and
+      *  the `C.this` occurs in nonvariant or contravariant position, mark the map


I don't think we mark the map to be unstable anymore.

smarter · 2017-08-13T21:47:31Z

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

-        case _ =>
-          if (thiscls.derivesFrom(cls) && pre.baseTypeRef(thiscls).exists) {
-            if (theMap != null && theMap.currentVariance <= 0 && !isLegalPrefix(pre)) {
-              ctx.base.unsafeNonvariant = ctx.runId


The field unsafeNonvariant isn't used anymore and should be removed.

smarter · 2017-08-13T21:55:14Z

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

+          case tp: NamedType => // inlined for performance; TODO: factor out into inline method
+            if (tp.symbol.isStatic) tp
+            else {
+              val saved = variance


Should use atVariance like the base case does.

smarter · 2017-08-13T21:55:27Z

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

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


Add // Inlined for performance ? Same for the two cases below.

smarter · 2017-08-13T22:05:41Z

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

+              if (v1 > 0 && v2 > 0) propagate(infoLo, infoHi)
+              else if (v1 < 0 && v2 < 0) propagate(infoHi, infoLo)
+              else if (!infoLo.isAlias && !infoHi.isAlias) propagate(infoLo, infoHi)
+              else range(tp.bottomType, tp.topType)


Could parent be used instead of tp.topType ?

I added a comment explaining why not:

// Using `parent` instead of `tp.topType` would be better for normal refinements, // but it would also turn *-types to a hk-types, which is not what we want. // We should revisit this point in case we represent applied types not as refinements anymore.

it would also turn *-types to a hk-types

Would it? TypeApplications#isHK only returns true for HKTypeLambda, and before this PR avoid used to return the parent for refinement types and it worked fine.

Alternatively we could return derivedRefinedType(tp, parent, WildcardType) I think

smarter · 2017-08-13T22:07:41Z

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

+              if (distributeArgs(args, tp.typeParams))
+                range(tp.derivedAppliedType(tycon, loBuf.toList),
+                      tp.derivedAppliedType(tycon, hiBuf.toList))
+              else range(tp.bottomType, tp.topType)


Could tp.superType be used instead of tp.topType ?

Not sure. tp.superType is not necessarily a supertype of the derived application because the arguments have changed. We could try to re-do the application on the upper bound of the constructor, but this looks murky with what we know right now. Theoretical underpinnings would be good to have here!

smarter · 2017-08-14T14:54:49Z

The new avoid is a bit too strict I think:

object Test {
  def test = {
    object Hi {
      type A = Int
    }

    val x: Hi.A = 1

    List(x)
  }

  val hi: List[Int] = test // Fails because `test` has type `List[Any]` instead of `List[Int]`
}

Because when we avoid Hi we return Object. This can easily be fixed by modifying classBound to return Object { type A = Int }:

diff --git a/compiler/src/dotty/tools/dotc/typer/TypeAssigner.scala b/compiler/src/dotty/tools/dotc/typer/TypeAssigner.scala
index 4340cae22c..11849b870e 100644
--- a/compiler/src/dotty/tools/dotc/typer/TypeAssigner.scala
+++ b/compiler/src/dotty/tools/dotc/typer/TypeAssigner.scala
@@ -49,7 +49,7 @@ trait TypeAssigner {
         parentType.findMember(decl.name, info.cls.thisType, excluded = Private)
           .suchThat(decl.matches(_))
       val inheritedInfo = inherited.info
-      if (inheritedInfo.exists && decl.info <:< inheritedInfo && !(inheritedInfo <:< decl.info)) {
+      if (!inheritedInfo.exists || (decl.info <:< inheritedInfo && !(inheritedInfo <:< decl.info))) {
         val r = RefinedType(parent, decl.name, decl.info)
         typr.println(i"add ref $parent $decl --> " + r)
         r

Do you think that's a good way to go?

odersky · 2017-08-16T10:09:51Z

@smarter I developed another fix, which is more direct.

smarter · 2017-08-16T12:15:56Z

For a Block, TypeAssigner does an avoid just for term symbols whereas
Typer does one for all local symbols.

How come we don't avoid type symbols in TypeAssigner anyway?

odersky · 2017-08-16T12:25:49Z

How come we don't avoid type symbols in TypeAssigner anyway?

I think some of the later phases might allow anonymous classes defined in one block to be used in another. As long as they are still in the same method, that's OK. But I am not sure about specifics.

smarter · 2017-08-16T12:30:27Z

OK, opened #2987 to track this.

smarter

LGTM!

smarter · 2017-08-16T13:32:38Z

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

+       *   1. We first try a widening conversion to the type's info with
+       *      the original prefix. Since the original prefix is known to
+       *      be a subtype of the returned prefix, this can improve results.
+       *   2. IThen, if the approximation result is a singleton reference C#x.type, we


typo: IThen -> Then

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.

Test case in i2945.scala.

For a Block, TypeAssigner does an avoid just for term symbols whereas Typer does one for all local symbols. Since Typer called TypeAssigner, we got a double avoidance pass. This is eliminated by assigning types directly in Typer without passing through TypeAssigner.

liufengyun · 2017-08-17T20:41:15Z

test performance please

dottybot · 2017-08-17T20:42:16Z

performance test scheduled: 1 jobs in total.

dottybot · 2017-08-17T21:20:11Z

Performance test finished successfully:

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

odersky requested a review from smarter August 2, 2017 16:51

odersky changed the title ~~[WIP] Implement avoid in terms of AproximatingTypeMap~~ Implement avoid in terms of AproximatingTypeMap Aug 2, 2017

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

odersky mentioned this pull request Aug 2, 2017

Fix #2928: Add special mode for computing the type of a LHS #2946

Merged

odersky mentioned this pull request Aug 10, 2017

Fix #2948: Use symbol's info when mapping inherited denotations #2970

Merged

odersky force-pushed the change-avoid-approx branch 2 times, most recently from 48def35 to 139b248 Compare August 13, 2017 17:45

odersky mentioned this pull request Aug 13, 2017

Generalize Approximating Type Maps #2908

Closed

smarter reviewed Aug 13, 2017

View reviewed changes

smarter force-pushed the change-avoid-approx branch from 137431b to de19138 Compare August 14, 2017 18:24

felixmulder assigned odersky Aug 15, 2017

odersky assigned smarter and unassigned odersky Aug 16, 2017

smarter mentioned this pull request Aug 16, 2017

Investigate why the Block TypeAssigner avoids types but not terms #2987

Open

smarter approved these changes Aug 16, 2017

View reviewed changes

odersky force-pushed the change-avoid-approx branch from e7e643c to 45ffdf0 Compare August 16, 2017 16:17

odersky added 9 commits August 17, 2017 17:56

Factor out variance manipulation in TypeMap and TypeAccumulator

1f04bba

Fix derivedRefinedType in ApproximatingTypeMap

104516b

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.

Add comment explaining approximating derivedAppliedType

8d17f57

Polishings and more comments

6e08737

Fix review comments re homogenizedArg

826c96f

Address reviewers comments

46a462b

Drop unused field unsafeNonvariant

dbd2e2c

Fix typo

bc5cbb0

Fix derivedSelect in avoid

b27dadc

Test case in i2945.scala.

odersky force-pushed the change-avoid-approx branch from 45ffdf0 to 491fe97 Compare August 17, 2017 15:57

odersky merged commit 076df81 into scala:master Aug 21, 2017

allanrenucci deleted the change-avoid-approx branch December 14, 2017 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement avoid in terms of AproximatingTypeMap #2945

Implement avoid in terms of AproximatingTypeMap #2945

odersky commented Aug 2, 2017 •

edited

Loading

smarter Aug 13, 2017

smarter Aug 13, 2017

smarter Aug 13, 2017

smarter Aug 13, 2017

smarter Aug 13, 2017

smarter Aug 13, 2017

smarter Aug 13, 2017

smarter Aug 13, 2017

smarter Aug 13, 2017

odersky Aug 14, 2017

smarter Aug 14, 2017

smarter Aug 14, 2017

smarter Aug 13, 2017

odersky Aug 14, 2017 •

edited

Loading

smarter commented Aug 14, 2017

odersky commented Aug 16, 2017

smarter commented Aug 16, 2017

odersky commented Aug 16, 2017 •

edited

Loading

smarter commented Aug 16, 2017

smarter left a comment

smarter Aug 16, 2017

liufengyun commented Aug 17, 2017

dottybot commented Aug 17, 2017

dottybot commented Aug 17, 2017

Implement avoid in terms of AproximatingTypeMap #2945

Implement avoid in terms of AproximatingTypeMap #2945

Conversation

odersky commented Aug 2, 2017 • edited Loading

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 Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

smarter commented Aug 14, 2017

odersky commented Aug 16, 2017

smarter commented Aug 16, 2017

odersky commented Aug 16, 2017 • edited Loading

smarter commented Aug 16, 2017

smarter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liufengyun commented Aug 17, 2017

dottybot commented Aug 17, 2017

dottybot commented Aug 17, 2017

odersky commented Aug 2, 2017 •

edited

Loading

odersky Aug 14, 2017 •

edited

Loading

odersky commented Aug 16, 2017 •

edited

Loading