Fix #4984: support name-based unapplySeq #5078

liufengyun · 2018-09-04T12:31:49Z

Fix #4984: support name-based unapplySeq

allanrenucci · 2018-09-05T13:19:35Z

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

+        val tps = List(
+          MethodType(List("len".toTermName))(_ => defn.IntType :: Nil, _ => defn.IntType),
+          MethodType(List("i".toTermName))(_ => defn.IntType :: Nil, _ => elemTp),
+          MethodType(List("n".toTermName))(_ => defn.IntType :: Nil, _ => defn.SeqType.appliedTo(elemTp)),


Does this mean that the argument name must conform to len, i and n?

Param names are not meaningful in comparing method types.

allanrenucci · 2018-09-05T13:30:10Z

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

+        val names = List(nme.lengthCompare, nme.apply, nme.drop, nme.toSeq)
+        RefinedType.make(defn.AnyType, names, tps)
+      }
+      getTp <:< superType(WildcardType)  && {


What does this test?

You are right, we can simplify by removing the first test.

allanrenucci · 2018-09-05T13:36:23Z

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

+          MethodType(List("n".toTermName))(_ => defn.IntType :: Nil, _ => defn.SeqType.appliedTo(elemTp)),
+          ExprType(defn.SeqType.appliedTo(elemTp)),
+        )
+        val names = List(nme.lengthCompare, nme.apply, nme.drop, nme.toSeq)


scalac require lengthCompare or length. If there's no lengthCompare, the translation uses length

And we should add tests for cases where we only have one or the other

allanrenucci · 2018-09-05T13:47:46Z

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

+        RefinedType.make(defn.AnyType, names, tps)
+      }
+      getTp <:< superType(WildcardType)  && {
+        val seqArg = extractorMemberType(getTp, nme.toSeq).elemType.hiBound


Let's use the result type of apply like scalac

allanrenucci · 2018-09-05T14:01:45Z

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

@@ -100,10 +100,27 @@ object Applications {
      Nil
    }

+    def validUnapplySeqType(getTp: Type): Boolean = {


I am wondering if we should do something along the line of (inspired by implementation of isGetMatch):

def isValidUnapplySeqType(tp: Type, errorPos: Position)(implicit ctx: Context) = isGetMatch(tp, errorPos) && { val getTpe = extractorMemberType(tp, nme.get, errorPos) val applyTpe = extractorMemberType(getTpe, nme.apply, errorPos) applyTpe.exists && { def dropTpe = extractorMemberType(getTpe, nme.applyTpe, errorPos) def toSeqTpe = extractorMemberType(getTpe, nme.toSeq, errorPos) def lengthCompareTpe = extractorMemberType(getTpe, nme.lengthCompare, errorPos) val lengthTpe = extractorMemberType(getTpe, nme.length, errorPos) (lengthCompareTpe.isRef(defn.BooleanClass) || lengthTpe.isRef(defn.IntClass)) && { val expectedSeqTpe = defn.SeqType.appliedTo(applyTpe) dropTpe <:< expectedSeqTpe && toSeqTpe <:< expectedSeqTpe } } }

The original implementation is a concise way to implement the same logic. In the above, extractorMemberType doesn't work for methods with parameters, and we will need to check both NoType and param types of the methods.

allanrenucci · 2018-09-05T14:03:56Z

tests/run/i4984b.scala

+object Array2 {
+  def unapplySeq(x: Array[Int]): Data = new Data
+
+  final class Data {


Can we also a add test with:

class Data1 { def isEmpty: Boolean = false def get: Data2 = new Data2 def lengthCompare(len: Int): Int = 0 } } class Data2 { def apply(i: Int): Int = 3 def drop(n: Int): scala.Seq[Int] = Seq(2, 5) def toSeq: scala.Seq[Int] = Seq(6, 7) }

allanrenucci · 2018-09-05T14:06:19Z

compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala

-              patternPlan(getResult, arg, onSuccess)
+            if (args.length == 1) {
+              val toSeq = ref(getResult)
+                .select(defn.Seq_toSeq.matchingMember(getResult.info))


Why not simply: .select(nme.toSeq)?

The code is defensive here to support possible overloading of toSeq.

We should add tests for this then. Would it work to do .select(nme.toSeq).appliedToNone()?

My hunch is that it will crash the compiler, as appliedToNone will not do overloading resolution.

But there is no need right? You cannot have two overloads of a method that takes no parameter

Never mind, your solution seems better. matchingMember is already used in multiple places in this file (I suppose for reasons)

smarter · 2018-09-06T00:12:18Z

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

+      def types2 = List(lengthTp, applyTp(elemTp), dropTp(elemTp), toSeqTp(elemTp))
+
+      val valid = elemTp.exists &&
+                  (getTp <:< RefinedType.make(defn.AnyType, names1, types1) ||


This seems like a rather complicated (and probably inefficient) way of checking if a type has a given member, why not use extractorMemberType like in other places in this file?

extractorMemberType only checks the existence of a parameterless method, here we need to check method types for conformance. If we extract the members one by one and check param & return type, the code would be tedious.

I agree with @smarter. It seems inefficient. In particular the second subtype test getTp <:< RefinedType.make(defn.AnyType, names2, types2)) will do a lot of unnecessary work that has already been done in the previous subtype test (i.e. checking members apply, drop and toSeq).

I think the right ways is to perform members lookup even if it is more verbose

smarter · 2018-09-06T00:14:15Z

Before we forget, it'd be good to update the spec for name-based pattern matching: https://github.com/lampepfl/dotty/blob/master/docs/docs/reference/changed/pattern-matching.md#name-based-pattern

allanrenucci

We should also add test cases with overloaded versions of lengthCompare, apply, drop and toSeq

allanrenucci · 2018-09-06T10:59:46Z

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

    if (unapplyName == nme.unapplySeq) {
-      if (unapplyResult derivesFrom defn.SeqClass) seqSelector :: Nil


Why this case removed?

There seems to be no spec for this, I've no idea what it is intended for.

allanrenucci · 2018-09-06T11:06:06Z

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

-        if (seqArg.exists) args.map(Function.const(seqArg))
-        else fail
+      if (isGetMatch(unapplyResult, pos) && validUnapplySeqType(getTp)) {
+        val elemTp = unapplySeqTypeElemTp(getTp)


unapplySeqTypeElemTp is also called by validUnapplySeqType and seems very expensive. There is no need to call it twice. We could rewrite to something like:

if (isGetMatch(unapplyResult, pos)) { val elemTp = unapplySeqTypeElemTp(getTp) if (elemTp.exists) ... else fail } else fail

allanrenucci · 2018-09-06T11:12:47Z

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

+      def types2 = List(lengthTp, applyTp(elemTp), dropTp(elemTp), toSeqTp(elemTp))
+
+      val valid = elemTp.exists &&
+                  (getTp <:< RefinedType.make(defn.AnyType, names1, types1) ||


I agree with @smarter. It seems inefficient. In particular the second subtype test getTp <:< RefinedType.make(defn.AnyType, names2, types2)) will do a lot of unnecessary work that has already been done in the previous subtype test (i.e. checking members apply, drop and toSeq).

I think the right ways is to perform members lookup even if it is more verbose

allanrenucci

LGTM. Maybe @odersky should have a final look

liufengyun force-pushed the fix-4984 branch from 1d3a945 to 1e3a643 Compare September 4, 2018 13:23

liufengyun mentioned this pull request Sep 4, 2018

x @ _* bound variables are now scala.Seq; name-based pattern matching changes to enable this scala/scala#7068

Merged

allanrenucci suggested changes Sep 5, 2018

View reviewed changes

smarter reviewed Sep 6, 2018

View reviewed changes

allanrenucci suggested changes Sep 6, 2018

View reviewed changes

liufengyun added 9 commits September 9, 2018 16:00

Fix scala#4984: support name-based unapplySeq

2a5e33f

use toSeq type as element type and check type

48b7dbb

add neg test

6999ca8

address review: try length if lengthCompare unavailable

4c12ed3

add more tests

3b93d98

fix failed test neg/i3248.scala

9304ed6

update docs for name-based unapplySeq

255c5bb

address review

94a93e5

fix ci: member types should go through asSeenFrom

4d00b92

allanrenucci force-pushed the fix-4984 branch 2 times, most recently from 0a8aab6 to cf40824 Compare September 9, 2018 14:59

Cleanup + Documentation + More neg tests

6a0fc8c

allanrenucci force-pushed the fix-4984 branch from cf40824 to 6a0fc8c Compare September 9, 2018 15:00

allanrenucci approved these changes Sep 9, 2018

View reviewed changes

allanrenucci assigned odersky Sep 9, 2018

allanrenucci mentioned this pull request Sep 14, 2018

Release Dotty 0.10.0-RC1 and 0.9.0 and sbt-dotty 0.2.5 #5111

Closed

20 tasks

odersky approved these changes Sep 19, 2018

View reviewed changes

odersky merged commit a2a1112 into scala:master Sep 20, 2018

liufengyun deleted the fix-4984 branch September 20, 2018 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4984: support name-based unapplySeq #5078

Fix #4984: support name-based unapplySeq #5078

liufengyun commented Sep 4, 2018

allanrenucci Sep 5, 2018

liufengyun Sep 5, 2018

allanrenucci Sep 5, 2018

liufengyun Sep 5, 2018

allanrenucci Sep 5, 2018

allanrenucci Sep 5, 2018

allanrenucci Sep 5, 2018 •

edited

Loading

liufengyun Sep 5, 2018

allanrenucci Sep 5, 2018

allanrenucci Sep 5, 2018

liufengyun Sep 5, 2018

allanrenucci Sep 5, 2018

liufengyun Sep 5, 2018

allanrenucci Sep 5, 2018

allanrenucci Sep 5, 2018

smarter Sep 6, 2018

liufengyun Sep 6, 2018

allanrenucci Sep 6, 2018

smarter commented Sep 6, 2018

allanrenucci left a comment

allanrenucci Sep 6, 2018

liufengyun Sep 6, 2018

allanrenucci Sep 6, 2018

allanrenucci Sep 6, 2018

allanrenucci left a comment

		if (unapplyName == nme.unapplySeq) {
		if (unapplyResult derivesFrom defn.SeqClass) seqSelector :: Nil

Fix #4984: support name-based unapplySeq #5078

Fix #4984: support name-based unapplySeq #5078

Conversation

liufengyun commented Sep 4, 2018

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

allanrenucci Sep 5, 2018 • 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

smarter commented Sep 6, 2018

allanrenucci 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

allanrenucci left a comment

Choose a reason for hiding this comment

allanrenucci Sep 5, 2018 •

edited

Loading