Fix #4884: Fix handling of bounds of type lambdas #4902

odersky · 2018-08-07T11:28:16Z

Type lambdas have their own type parameters which are correctly transformed during an as-seen-from. The problem was that these type parameters were not always retrieved when computing the type parameters of a higher-kinded type. This used to pick the type parameters of the symbol but that is wrong since it loses as-seen-from information.

It would be most straightforward to always compute the type parameters of the info instead, but this
runs afoul of cyclic errors. So we have a subtle danse now in TypeApplications#typeParams do decide what strategy to use when.

Only last two commits are new wrt #4881.

liufengyun · 2018-08-07T13:28:58Z

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

+      case prefix: ThisType => prefix.cls `eq` tycon.owner
+      case NoPrefix => true
+      case _ => false
+    }


Can we further refine isTrivial to cover the case when prefix is static?

I am not sure we want to do this. The purpose of isTrivial is to fall back to a special case to avoid cyclic reference errors. It seems we don't need to include static this types to achieve this. On the other hand, the isStatic test could prompt cyclic references itself.

Blaisorblade · 2018-08-07T17:35:30Z

@odersky I got a new asSeenFrom bug by playing with this branch. EDIT: fixed

object Test {
  trait A
  trait B
  trait TestConstructor1 { type F[X <: A]
    trait FromSet[C[_ <: A with B]]

    trait MSetLike[X <: A with B, This <: MSet[X] with MSetLike[X, This]] {
      def to[C[X <: A with B] <: MSet[X] with MSetLike[X, C[X]]](fi: FromSet[C]): C[X] = ???
    }
    trait MSet[X <: A with B] extends MSetLike[X, MSet[X]]
    object MSetFactory extends FromSet[MSet]
  }

  trait TestConstructor4[D] {
    trait TestConstructor5[E] {
      trait FromSet[C[_ <: D with E]]

      trait MSetLike[X <: D with E, This <: MSet[X] with MSetLike[X, This]] {
        def to[C[X <: D with E] <: MSet[X] with MSetLike[X, C[X]]](fi: FromSet[C]): C[X] = ???
      }
      trait MSet[X <: D with E] extends MSetLike[X, MSet[X]]
      object MSetFactory extends FromSet[MSet]
    }
  }

  type C = A & B
  val v1: TestConstructor1 => Unit = { f =>
    type P[a <: A] = f.F[a]

    type P1[c <: C] = f.MSet[c]
      (f.MSetFactory: f.FromSet[f.MSet]): Unit
      (x: P1[C]) => x.to(f.MSetFactory)
  }

  def f3(f: TestConstructor4[A])(g: f.TestConstructor5[B]): Unit = {
    type P1[c <: C] = g.MSet[c]
      (g.MSetFactory: g.FromSet[g.MSet]): Unit
      (x: P1[C]) => x.to(g.MSetFactory)
      (x: P1[C]) => x.to[g.MSet](g.MSetFactory)
  }
}

Beware that x.to(g.MSetFactory) works in v1 but not in f3, so it's a bug. Sadly Scalac can't handle this either.

Misc notes:

FWIW, that's from some code in tests/run/colltest5/ that failed when I oversimplified typeApplications.
writing f3 as a val triggers new Failure with path-dependent function values, asSeenFrom stress-tests and type inference #4906, but that seems unrelated.
~~the rebase went wrong, Fixes to transparent functions #4881 was merged so those commits should disappear.~~

Blaisorblade · 2018-08-07T14:21:54Z

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

    try self match {
      case self: TypeRef =>
        val tsym = self.symbol
        if (tsym.isClass) tsym.typeParams
-        else if (!tsym.exists) self.info.typeParams
+        else if (!tsym.exists || !isTrivial(self.prefix, tsym)) self.info.typeParams


Does this prevent cyclic errors, or just reduce them enough? Otherwise, I agree this does the correct thing.

There will always be cyclic errors. The challenge is to reduce them sufficiently that

they occur only rarely

they can be communicated so that they make sense to the user

there is a way to work around them

and, probably most importantly

they don't break existing code. What worked with the current compiler should continue working.

So it's really quite fuzzy. Ideally, we should have a spec that says in exactly what situations cyclic errors are OK. But we are currently quite far away from this point. I believe most mainstream languages that allow unlimited recursion face the same fundamental problem.

Blaisorblade · 2018-08-07T16:39:39Z

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

                // wildcard identifiers `_` instead.
-                res = res.withType(avoid(tparam.paramInfo, tpt1.tpe.typeParamSymbols))
+                res = res.withType(tparamBounds)


IIUC, avoiding C's parameters was a hack, and substituting them with asSeenFrom was always the correct fix? Makes sense to me.

Yes, I believe that's correct.

Blaisorblade · 2018-08-07T17:49:09Z

My actual goal with that test was to stress both asSeenFrom and a lazy denotation.

It seems likely that the new code will trigger cyclic errors when tsym.infoOrCompleter is a LazyType but the prefix isn't recognized as trivial. Taking those as errors could be fine (if stable under separate compilation), but none of the relevant tests need tricky asSeenFrom use.

Blaisorblade · 2018-08-07T17:52:31Z

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

@@ -1238,7 +1238,7 @@ class Typer extends Namer
          args = args.take(tparams.length)


The test failure seems genuinely to appear with this new commit (not with my changes) — everything works with the parent.

sbt:dotty> dotc -d out -Ycheck:all tests/pos/t1786-cycle.scala -Yforce-sbt-phases [warn] Multiple main classes detected. Run 'show discoveredMainClasses' to see the list [info] Running (fork) dotty.tools.dotc.Main -classpath /Users/pgiarrusso/git/dotty/library/../out/bootstrap/dotty-library-bootstrapped/scala-0.10/dotty-library_0.10-0.10.0-bin-SNAPSHOT.jar -d out -Ycheck:all tests/pos/t1786-cycle.scala -Yforce-sbt-phases checking tests/pos/t1786-cycle.scala after phase frontend exception occurred while compiling tests/pos/t1786-cycle.scala Exception in thread "main" dotty.tools.dotc.core.TypeError: bad parameter reference LongTraversableLike#Repr at sbt-deps the parameter is type Repr in trait LongTraversableLike but the prefix LongTraversableLike does not define any corresponding arguments. at dotty.tools.dotc.core.Types$NamedType.argDenot(Types.scala:1834) at dotty.tools.dotc.core.Types$NamedType.fromDesignator$1(Types.scala:1751) at dotty.tools.dotc.core.Types$NamedType.computeDenot(Types.scala:1769) at dotty.tools.dotc.core.Types$NamedType.denot(Types.scala:1725) at dotty.tools.dotc.core.Types$NamedType.info(Types.scala:1714)

EDIT: cut stacktrace.

This is fixed in the latest commit.

Blaisorblade

Overall: I'd like to understand why the new typeParams shouldn't trigger a cyclic error, if that's the goal. Can we safely assume that non-trivial prefixes are already forced? Or do we consider such cyclic errors valid type errors (unless we can improve isTrivial?).
(And sorry for the premature approval, wrong button).

odersky · 2018-08-08T09:14:54Z

@Blaisorblade

@odersky I got a new asSeenFrom bug by playing with this branch.

There are several undefined identifiers in the example as given. Can you check whether some errors still remain with the latest commit?

odersky · 2018-08-08T09:22:54Z

Overall: I'd like to understand why the new typeParams shouldn't trigger a cyclic error, if that's the goal. Can we safely assume that non-trivial prefixes are already forced? Or do we consider such cyclic errors valid type errors (unless we can improve isTrivial?).

I believe we have to consider them valid type errors, since the fallback strategy of computing parameters from symbols is not sound. But we are in a grey area here. For instance, one could introduce more laziness by delaying computation of the bounds of type parameters somehow. The question is always whether it's worth it. There are no hard truths to be had here. Essentially we are saying, given a evaluation strategy of the compiler, be as lazy as possible to prevent spurious cyclic references. The problem is there are many possible evaluation strategies, each with their different trade-offs. So we can never reach an optimum, we can only get sometimes gradual improvements (and even those might trade one source of cyclicity for another).

- Type parameters of hk lambdas depend on prefix, can't be computed from symbol if prefix is non-trivial - Parameter bounds of such type parameters are already computed, so can be retrieved with paramInfo. The doc comment of paramInfoAsSeenFrom indicates this, but the implementation did something different. - Typer and Checking need to call paramInfoAsSeenFrom to get the right bounds in general. This also fixes scala#4884.

Using `tsym.info.typeParams` appears unnecessary in testing, and `info` seems always available so it's unclear it gives any savings.

The asSeenFrom is always wrt the type-constructor, not its prefix. This is because the parameter is treated as a member of the type constructor. It is strange that no test case caught this.

…ypeTree We need to take care that the asSeenFrom is from a * type, not a type constructor.

allanrenucci · 2018-08-08T09:39:42Z

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

-          case info: LazyType => info.completerTypeParams(tsym)
-          case info => info.typeParams
+          case info: LazyType if tsym.exists && isTrivial(self.prefix, tsym) => info.completerTypeParams(tsym)
+          case _ => self.info.typeParams


Why change this line from case info => info.typeParams? If the info is already computed I would just reuse it

Edit: I realise it is not equivalent

To clarify (for @allanrenucci and future readers): if tsym.exists && isTrivial(self.prefix, tsym), self.info.typeParams and info.typeParams should coincide and self.info.typeParams is more clearly right, so I tried to simplify things.

Actually I tried: info.typeParams and self.info.typeParams are not equivalent, the newly added test was failing. I think this is because in the former, info may refer to the completer

They’re only equivalent under those conditions, not in this pattern match branch.
I know that my commit didn’t change behavior in any scenario I tested with.
To play with this, you can go to the parent commit and replace info.typeParams with self.info.typeParams.
EDIT: that is, you can replace info.typeParams by self.info.typeParams in https://github.com/dotty-staging/dotty/blob/373902673f6a96018581ba2472d770686d718005/compiler/src/dotty/tools/dotc/core/TypeApplications.scala#L183, and it'll make no difference. At that point, since the conditional has two branches using self.info.typeParams, this commit refactors the conditional to have one branch.

Blaisorblade · 2018-08-08T12:31:41Z

@odersky My example still doesn't work; sorry for the copy-paste mistakes, fixed them. Here are the errors I get:

- [E007] Type Mismatch Error: tests/pos/i4884c.scala:37:9 ---------------------
37 |      (g.MSetFactory: g.FromSet[g.MSet]): Unit
   |       ^^^^^^^^^^^^^
   |       found:    g.MSetFactory.type
   |       required: g.FromSet[g.MSet]
   |
-- [E007] Type Mismatch Error: tests/pos/i4884c.scala:38:27 --------------------
38 |      (x: P1[C]) => x.to(g.MSetFactory)
   |                         ^^^^^^^^^^^^^
   |                         found:    g.MSetFactory.type
   |                         required: g.FromSet[g.MSet]
   |
-- [E007] Type Mismatch Error: tests/pos/i4884c.scala:39:35 --------------------
39 |      (x: P1[C]) => x.to[g.MSet](g.MSetFactory)
   |                                 ^^^^^^^^^^^^^
   |                                 found:    g.MSetFactory.type
   |                                 required: g.FromSet[g.MSet]
   |
three errors found

Blaisorblade

Overall the approach makes sense, and looks sound in the non-cyclic case. For the new error, dunno yet if it belongs here or to a separate issue, so I'll leave it to you @odersky.

odersky · 2018-08-08T12:38:40Z

Let's make a separate issue for the new error.

Blaisorblade · 2018-08-08T12:48:03Z

OK, filed #4908.

odersky requested a review from Blaisorblade August 7, 2018 11:28

odersky mentioned this pull request Aug 7, 2018

Fix #4884: asSeenFrom on incorrect prefix in type applications #4900

Closed

liufengyun reviewed Aug 7, 2018

View reviewed changes

allanrenucci added the stat:needs rebase label Aug 7, 2018

Blaisorblade mentioned this pull request Aug 7, 2018

Failure with path-dependent function values, asSeenFrom stress-tests and type inference #4906

Closed

Blaisorblade reviewed Aug 7, 2018

View reviewed changes

Blaisorblade approved these changes Aug 7, 2018 •

edited

Loading

View reviewed changes

Blaisorblade suggested changes Aug 7, 2018

View reviewed changes

odersky removed the stat:needs rebase label Aug 8, 2018

allanrenucci force-pushed the fix-transparent-2 branch from 8f91a82 to 0d13ba6 Compare August 8, 2018 08:09

odersky and others added 7 commits August 8, 2018 11:31

Always avoid local symbols in tpd.seq

0a280b3

Improve testcase

deecd9f

Testcase: more recursion

210c6ed

Simplify typeParams

3eb6d92

Using `tsym.info.typeParams` appears unnecessary in testing, and `info` seems always available so it's unclear it gives any savings.

Fix paramInfoAsSeenFrom argument in typedAppliedTypeTree

900982c

The asSeenFrom is always wrt the type-constructor, not its prefix. This is because the parameter is treated as a member of the type constructor. It is strange that no test case caught this.

Another fix to compute the bounds of type parameters in typedAppliedT…

e25e286

…ypeTree We need to take care that the asSeenFrom is from a * type, not a type constructor.

odersky force-pushed the fix-transparent-2 branch from 0d13ba6 to e25e286 Compare August 8, 2018 09:32

Drop redundant test

3f639b6

allanrenucci reviewed Aug 8, 2018

View reviewed changes

Blaisorblade approved these changes Aug 8, 2018

View reviewed changes

Blaisorblade assigned odersky Aug 8, 2018

Blaisorblade approved these changes Aug 8, 2018

View reviewed changes

odersky merged commit 6485f89 into scala:master Aug 8, 2018

Blaisorblade deleted the fix-transparent-2 branch August 8, 2018 12:41

Blaisorblade mentioned this pull request Aug 8, 2018

Subtyping breaks under a dependent prefix? #4908

Open

		@@ -1238,7 +1238,7 @@ class Typer extends Namer
		args = args.take(tparams.length)

Fix #4884: Fix handling of bounds of type lambdas #4902

Fix #4884: Fix handling of bounds of type lambdas #4902

Uh oh!

Conversation

odersky commented Aug 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Blaisorblade commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Blaisorblade commented Aug 7, 2018

Uh oh!

Blaisorblade Aug 7, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Blaisorblade left a comment • 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 commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odersky commented Aug 8, 2018

Uh oh!

allanrenucci Aug 8, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Blaisorblade Aug 8, 2018 • 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!

Blaisorblade commented Aug 8, 2018

Uh oh!

Blaisorblade left a comment

Choose a reason for hiding this comment

Uh oh!

odersky commented Aug 8, 2018

Uh oh!

Blaisorblade commented Aug 8, 2018

Uh oh!

Uh oh!

Blaisorblade commented Aug 7, 2018 •

edited

Loading

Blaisorblade Aug 7, 2018 •

edited

Loading

Blaisorblade left a comment •

edited

Loading

odersky commented Aug 8, 2018 •

edited

Loading

allanrenucci Aug 8, 2018 •

edited

Loading

Blaisorblade Aug 8, 2018 •

edited

Loading