Changes necessary to make the collection strawman compile #2240

odersky · 2017-04-12T17:45:24Z

Several bug fix, and one language change:

We now instantiate type variables of implicits to the or-dominator of their lower bound.
This is necessary to compile code like this:

def useOrd[T: math.Ordering](xs: T*) = ()
useOrd(Some(1), None)

Code like this occurs in the collection strawman. It should be pretty common also elsewhere.

odersky · 2017-04-12T18:12:43Z

Test fails in vulpix with

java.lang.NullPointerException: null, took 0.0 sec
578s
2888
[error] at dotty.tools.vulpix.SummaryReport$$anonfun$echoSummary$5.apply(SummaryReport.scala:122)
578s
2889
[error] at dotty.tools.vulpix.SummaryReport$$anonfun$echoSummary$5.apply(SummaryReport.scala:122)
578s
2890
[error] at scala.collection.mutable.ResizableArray$class.foreach(ResizableArray.scala:59)
578s
2891
[error] at scala.collection.mutable.ArrayBuffer.foreach(ArrayBuffer.scala:48)
578s
2892
[error] at dotty.tools.vulpix.SummaryReport.echoSummary(SummaryReport.scala:122)
578s
2893
[error] at dotty.tools.dotc.CompilationTests$.cleanup(CompilationTests.scala:257)
578s
2894
[error] at dotty.tools.dotc.CompilationTests.cleanup(CompilationTests.scala)

smarter · 2017-04-12T18:16:27Z

@odersky Try rebasing, I've made a PR recently to fix an NPE in vulpix.

smarter · 2017-04-12T18:24:43Z

I think the real issue here is that Ordering is not contravariant in its type parameter. The collection strawman might be a good time to experiment with changing this, although this will have different (and hopefully better) semantics in dotty for implicit search. What do you think?

smarter · 2017-04-12T18:35:55Z

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

+            //
+            // The problem is that there is no implicit Ordering instance for the otherwise inferred
+            // lower bound of T, which is `Some[Int] | None`.
+            ctx.orDominator(ctx.typeComparer.bounds(tvar.origin).lo) <:< tvar.origin


Using a subtype check here to change how instantiation proceeds feels a bit too indirect. How about instead we add an implicitMode: Boolean parameter (or it could be called widenUnion maybe) to TypeVar#instantiate instead?

I don't want to poison more code with the implicitMode distinction. It's better to see this not as a check, but as establishing a constraint. Then it makes sense where it is, IMO.

smarter · 2017-04-12T18:49:13Z

One really ugly thing we could do would be to hardcode scala.math.Ordering as contravariant in its type parameter, I'm sure could would only work better as a result :).

smarter · 2017-04-12T19:27:07Z

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

+  /** Instantiate selected type variables `tvars` in type `tp`. Instantiation
+   *  proceeds with `implicitMode = true`, that is, type variables are approximated
+   *  from below using or-dominators.
+   */
  def instantiateSelected(tp: Type, tvars: List[Type])(implicit ctx: Context): Unit =


This method is now specific to implicit search but its name doesn't indicate that. I would either rename the method or move it next to its sole usage in Typer#adaptNoArgs

odersky · 2017-04-12T19:43:46Z

I agree, the issue is that Ordering is not contravariant. Unfortunately,contravariant type classes were so far not that practical because implicit search would then prefer a more general instance over a more specific one. I.e. Ordering[Any] would have been chosen over Ordering[Option[T]]. We changed that in dotty, but it will be a while until common libraries adapt.

smarter · 2017-04-12T19:55:42Z

Yes, I was suggesting we pretend Ordering is contravariant in dotty even though it isn't, if only so that we can get more experience with contravariant type classes, but as I said it would be a very ugly hack :).

julienrf · 2017-04-12T20:29:44Z

The collection strawman might be a good time to experiment with changing this

👍

odersky · 2017-04-13T11:13:49Z

Looks like another NPE in vulpix

odersky · 2017-04-26T16:56:23Z

@felixmulder We get:

/bin/sh: ./scripts/update-scala-library: not found

smarter · 2017-04-26T16:59:49Z

@odersky Try rebasing

felixmulder · 2017-04-26T17:00:22Z

Rebase! It is gone :)

…

On Wed, Apr 26, 2017, 18:56 odersky ***@***.***> wrote: @felixmulder <https://github.com/felixmulder> We get: /bin/sh: ./scripts/update-scala-library: not found — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2240 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdYwRRXebsiNuI-GcWxc7YCb-WxkHNMks5rz3c6gaJpZM4M7wAB> .

Instantiate type variables of implicits to or-dominators of their lower bound. This is necessary to compile code like this: def useOrd[T: math.Ordering](xs: T*) = () useOrd(Some(1), None)

In a non-variant situation, we have to guess, which is difficult. This commit tries to improve guessing. With it, we can compile the either case of the TraverseTest in the collection strawman.

A new setting, -Ysuppress-param-forwarding is used to make forwarding type parameters confihurable. Forwarding type parameters from base class to subclass instance is done in `normalizeToClassRefs` for optimization, but I just found out this optimization is not always type-preserving and in fact can lead to type errors. SortedMap.scala in the collection strawman is an example. With parameter forwarding enabled, we get: Error: immutable/SortedMap.scala -------------------------------------------- | with MapValuePolyTransforms[K, V, C] { | ^ |Type argument C does not conform to upper bound [=X, +Y] => | strawman.collection.immutable.Map[=X, +Y] & | strawman.collection.immutable.MapValuePolyTransforms[=X', +Y', LazyRef(C)] | |where: +Y is a type variable | +Y' is a type variable | =X is a type variable | =X' is a type variable If we turn off forwarding with the option, the strawman compiles OK. Unfortunately turning the option off altogether does not work (yet). It leads to increased compile times, and more tests fail with deep subtype recursions. Dotty bootstrap seems to not terminate at all. Related: Refine notion of self in RefChecks. This is Needed to make pos/i1401.scala compile when forwardTypeParams is off.

We might get a wrong baseTypeRef cache when basetypes get populated while parent type infos of a class are not yet installed. So at the end of initializing a class type in namer, we should invalidate the caches of all its base types. Without the fix, BitSet in the collection strawman fails to compile.

Cycles can also arise for initialized type refs that appear in their own types.

That way we can trace where paramrefs come from.

Could create orphan paramrefs before.

We might need it in other places (e.g. self type comparisons).

We previously always failed for a situation like [X] => T <:< C unless the lhs was an eta-expansion.

We failed to take into account that a parameter might be forwarded to several types, in which case these types need to be conjoined. This led to a failure to compile the collection strawman unless forwarding was disabled.

It was a command line setting before. But since we do not need to selective suppress param forwarding for making things compile anymore, it's better not to expose it as such.

odersky · 2017-04-27T15:24:34Z

Current status if forwardParamBindings is OFF:

compileMixed issues a CyclicReference error
tools.io and compile-stdlib fail with deep subtype recursions
run/i2818.scala also dies with a deep subtype recursion. For this one I checked that it compiles
OK with -Yno-deep-subtypes not set.

All other tests succeed. The compileMixed failure is not surprising as major wizardry was needed to make it compile without cyclic references in the first place.

odersky · 2017-04-28T16:44:07Z

I'd like to get this in soon, because I fear that if we wait, collections will get even more code that will fail in dotty. So we need to get to a point to do CI of the strawman with dotty and scalac. For this we need:

this PR to go in.
Remove ProductN parent on case classes #2314 to be fixed and merged.
The bridge issue discovered by @julienrf when running the strawman to be fixed.

/cc @julienrf @OlivierBlanvillain @DarkDimius

odersky · 2017-04-30T14:09:18Z

Superseded by #2331.

odersky requested a review from smarter April 12, 2017 17:45

odersky force-pushed the fix-implicit-or branch from 05a0134 to edc1b5d Compare April 12, 2017 17:46

smarter reviewed Apr 12, 2017

View reviewed changes

odersky force-pushed the fix-implicit-or branch from edc1b5d to 9c4d5cc Compare April 12, 2017 18:46

smarter reviewed Apr 12, 2017

View reviewed changes

odersky force-pushed the fix-implicit-or branch from 9c4d5cc to 3f52c58 Compare April 13, 2017 10:52

odersky mentioned this pull request Apr 25, 2017

Compile under dotty, V2 scala/collection-strawman#63

Merged

odersky added 12 commits April 26, 2017 21:43

Instantiate type variables of implicits to or-dominators

7aa2b89

Instantiate type variables of implicits to or-dominators of their lower bound. This is necessary to compile code like this: def useOrd[T: math.Ordering](xs: T*) = () useOrd(Some(1), None)

Refine instantiation scheme in the case of non-variance

50e78c3

In a non-variant situation, we have to guess, which is difficult. This commit tries to improve guessing. With it, we can compile the either case of the TraverseTest in the collection strawman.

Better documentation of new instanceType algorithm

4d3ba08

Split explain-types and explain-implicits options

cd2cab5

Fix typo in setting name

5a13913

Better implicit reporting under -explain-implicits

7a5bdff

Fix stupid error in isSubArgs

4feb8f3

Turn off verboseExplainSubtype by default

d82e0a2

Improve cycle detection

7e5c223

Cycles can also arise for initialized type refs that appear in their own types.

Print hashCode of type lambdas under -uniqid

f5b0894

That way we can trace where paramrefs come from.

odersky added 5 commits April 26, 2017 21:43

Fix liftIfHK

de9f642

Could create orphan paramrefs before.

Factor out upwardsThisType in RefChecks

acd41b2

We might need it in other places (e.g. self type comparisons).

Add missing case in TypeComparer

659ede8

We previously always failed for a situation like [X] => T <:< C unless the lhs was an eta-expansion.

Fix type parameter forwarding

9ee8602

We failed to take into account that a parameter might be forwarded to several types, in which case these types need to be conjoined. This led to a failure to compile the collection strawman unless forwarding was disabled.

Make forwardParamBindings a Config setting

47efe0e

It was a command line setting before. But since we do not need to selective suppress param forwarding for making things compile anymore, it's better not to expose it as such.

odersky force-pushed the fix-implicit-or branch from 11210c0 to 47efe0e Compare April 26, 2017 19:43

odersky changed the title ~~Instantiate type variables of implicits to or-dominators~~ Changes necessary to make the collection strawman compile Apr 27, 2017

odersky mentioned this pull request Apr 30, 2017

Bug fixes to make collection strawman compile #2331

Merged

odersky closed this Apr 30, 2017

Changes necessary to make the collection strawman compile #2240

Changes necessary to make the collection strawman compile #2240

Uh oh!

Conversation

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

Uh oh!

Uh oh!

odersky commented Apr 12, 2017

Uh oh!

smarter commented Apr 12, 2017

Uh oh!

smarter commented Apr 12, 2017

Uh oh!

smarter Apr 12, 2017

Choose a reason for hiding this comment

Uh oh!

odersky Apr 12, 2017

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

smarter Apr 12, 2017

Choose a reason for hiding this comment

Uh oh!

odersky commented Apr 12, 2017

Uh oh!

smarter commented Apr 12, 2017

Uh oh!

julienrf commented Apr 12, 2017

Uh oh!

odersky commented Apr 13, 2017

Uh oh!

odersky commented Apr 26, 2017

Uh oh!

smarter commented Apr 26, 2017

Uh oh!

felixmulder commented Apr 26, 2017 via email

Uh oh!

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

Uh oh!

Uh oh!

odersky commented Apr 28, 2017

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

odersky commented Apr 12, 2017 •

edited

Loading

smarter commented Apr 12, 2017 •

edited

Loading

odersky commented Apr 27, 2017 •

edited

Loading

odersky commented Apr 30, 2017 •

edited

Loading