Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SI-884, allow type arguments on patterns. #5774

Closed
wants to merge 1 commit into from
Closed

SI-884, allow type arguments on patterns. #5774

wants to merge 1 commit into from

Conversation

@paulp
Copy link
Contributor

@paulp paulp commented Mar 13, 2017

Also offers a way to achieve the goal in SI-6517, seen in the test.

The mechanism is to rewrite casedefs which contain type applications
so that they no longer contain type applications, by pulling the
unapply call into a lazy val which precedes the match. Example using
an extractor which parses a string based on a specified type:

  expr match {
    case FromString[Int](n)                   => n
    case FromString[(Double, Double)]((a, b)) => (a + b).toInt
  }

becomes (with the generated names unique of course)

  {
    val Scrutinee = expr;
    lazy val TypeArgsPattern1 = FromString.unapply[Int](Scrutinee);
    lazy val TypeArgsPattern2 = FromString.unapply[(Double, Double)](Scrutinee);

    Scrutinee match {
      case _ if !TypeArgsPattern1.isEmpty =>
        val n = TypeArgsPattern1.get
        n
      case _ if !TypeArgsPattern2.isEmpty =>
        val x$1 = TypeArgsPattern2.get
        val a = x$1._1
        val b = x$1._2
        (a + b).toInt
    }
  }

This approach means that the various machinery which swirls around
type application should work (e.g. and especially, implicit selection.)

Also offers a way to achieve the goal in SI-6517, seen in the test.

The mechanism is to rewrite casedefs which contain type applications
so that they no longer contain type applications, by pulling the
unapply call into a lazy val which precedes the match. Example using
an extractor which parses a string based on a specified type:

  expr match {
    case FromString[Int](n)                   => n
    case FromString[(Double, Double)]((a, b)) => (a + b).toInt
  }

becomes (with the generated names unique of course)

  {
    val Scrutinee = expr;
    lazy val TypeArgsPattern1 = FromString.unapply[Int](Scrutinee);
    lazy val TypeArgsPattern2 = FromString.unapply[(Double, Double)](Scrutinee);

    Scrutinee match {
      case _ if !TypeArgsPattern1.isEmpty =>
        val n = TypeArgsPattern1.get
        n
      case _ if !TypeArgsPattern2.isEmpty =>
        val x$1 = TypeArgsPattern2.get
        val a = x$1._1
        val b = x$1._2
        (a + b).toInt
    }
  }

This approach means that the various machinery which swirls around
type application should work (e.g. and especially, implicit selection.)
@scala-jenkins scala-jenkins added this to the 2.12.2 milestone Mar 13, 2017
var valDefs = Vector[Tree]()

/** Memoize the scrutinee expression. */
lazy val exprVal = currentUnit.freshTermName("Scrutinee")

This comment has been minimized.

@retronym

retronym Mar 14, 2017
Member

Needs a trailing $ in the prefix:

scala> currentUnit.freshTermName("foo")
res0: $r.intp.global.TermName = foo1

/** Memoize the scrutinee expression. */
lazy val exprVal = currentUnit.freshTermName("Scrutinee")
lazy val expr1 = q"val $exprVal = $expr"

This comment has been minimized.

@retronym

retronym Mar 14, 2017
Member

We don't use quasiquotes in the compiler implementation elsewhere, partly because of the lack of ability to the component trees precisely, partly because of the somewhat bloated expansions, and partly keep one fewer dependency in the bootstrap. So we'd need to roll up our sleeves and do the manual treeCopy.XXX-s in their place.

}
override def transform(t: Tree): Tree = t match {
case m: Match => new TranslateMatch(super.transform(m).asInstanceOf[Match]) resultTree
case _ => super.transform(t)

This comment has been minimized.

@retronym

retronym Mar 14, 2017
Member

Too much recursion, here.

class Test {
  1 match {
    case 1 => 1 match {
      case 1 => 1 match { 
        case 1 => 1 match {
          case 1 => 1 match {
            case 1 => 1 match {
              case 1 => 1 match {
                case 1 =>
                case _ => "7"
              }
              case _ => "6"
            }
            case _ => "5"
          }
          case _ => "4"
        }
        case _ => "3"
      }
      case _ => "2"
    }
    case _ => "1"
  }
}
diff --git a/src/compiler/scala/tools/nsc/ast/parser/MatchRewriter.scala b/src/compiler/scala/tools/nsc/ast/parser/MatchRewriter.scala
index d27ae1fe14..13837be767 100644
--- a/src/compiler/scala/tools/nsc/ast/parser/MatchRewriter.scala
+++ b/src/compiler/scala/tools/nsc/ast/parser/MatchRewriter.scala
@@ -9,9 +9,15 @@ class MatchRewriter[G <: scala.tools.nsc.Global](val global: G) {
     def apply(t: Tree): Tree = {
       logResult("Transforming: " + t)(transform(t))
     }
-    override def transform(t: Tree): Tree = t match {
-      case m: Match => new TranslateMatch(super.transform(m).asInstanceOf[Match]) resultTree
-      case _        => super.transform(t)
+    override def transform(t: Tree): Tree = {
+      t match {
+        case Literal(Constant(s: String)) => println(s)
+        case _ =>
+      }
+      t match {
+        case m: Match => new TranslateMatch(super.transform(m).asInstanceOf[Match]) resultTree
+        case _        => super.transform(t)
+      }
     }
   }

Prints:

7
7
6
7
6
5
7
6
5
4
7
6
5
4
3
7
6
5
4
3
2
7
6
5
4
3
2
1
@retronym
Copy link
Member

@retronym retronym commented Mar 14, 2017

Overall, I'd prefer to see this translation in the pattern match translation, rather than as a rewrite of the parse tree

@paulp
Copy link
Contributor Author

@paulp paulp commented Mar 14, 2017

Yeah. I ported this from a macro, where I don't exactly have the same options. I got it to a non-macro working state but I'm going to defer to @milessabin as to how to advance from here.

@retronym
Copy link
Member

@retronym retronym commented Mar 14, 2017

More work would also be needed to support type apply extractor patterns in sub pattern position

scala> object Extractor { def unapply[T](a: Any) = Some(a) }
defined object Extractor

scala> 0 match { case Extractor[Int](x) => x }
res2: Any = 0

scala> (0, 0) match { case (Extractor[Int](x), _) => x }
<console>:12: error: not found: type Extractor
       (0, 0) match { case (Extractor[Int](x), _) => x }
                            ^
@retronym retronym modified the milestones: WIP, 2.12.2 Mar 14, 2017
@retronym
Copy link
Member

@retronym retronym commented Mar 14, 2017

I'm unassigning the 2.12.2 milestone for now as that release is pretty close and this will take some more time to nail down.

@milessabin
Copy link
Contributor

@milessabin milessabin commented Mar 14, 2017

I'm going to defer to @milessabin as to how to advance from here.

Happy to help, but I don't know what you're asking for here ...

@Blaisorblade
Copy link
Contributor

@Blaisorblade Blaisorblade commented Mar 14, 2017

Not to complain—I'm extremely happy this is even under the radar and don't want to derail it—but is there a chance for binding type variables in these patterns while looking at this, or to keep that in mind while looking at options?

Example use case:

trait Exp[T]
case class MapOption[T, U](f: Exp[T => U], arg: Exp[Option[T]]) extends Exp[Option[U]]
def f(term: Exp[T]) = term match {
  case Foo[t, u](f, arg) => ... // doesn't work
  case Foo(f, arg) => ... // works, no way to say `t`
  case term1: Foo[t, u] => // works but clumsy
}
@paulp
Copy link
Contributor Author

@paulp paulp commented Mar 14, 2017

@milessabin I was referring to our conversation in the comments of SI-8901 in which you said "If you open a PR against scala/scala then I'll commit to getting it across the finishing line," which successfully motivated me to take it this far. There are some straightforward improvements I will make based on @retronym's helpful analysis, but they won't add up to the finish line and that is all the time I have allocated for compiler hacking.

@paulp
Copy link
Contributor Author

@paulp paulp commented Mar 14, 2017

@Blaisorblade I'll take a look.

@milessabin
Copy link
Contributor

@milessabin milessabin commented Mar 15, 2017

@paulp understood.

@paulp
Copy link
Contributor Author

@paulp paulp commented Jul 2, 2017

I think this isn't going to fly in its present form and I don't anticipate ever working on it again so I'm closing. Anyone interested is welcome to pick it back up.

@paulp paulp closed this Jul 2, 2017
@SethTisue SethTisue removed this from the WIP milestone Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
7 participants