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

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

Merged
merged 1 commit into from Aug 21, 2018

Conversation

@lrytz
Copy link
Member

lrytz commented Aug 14, 2018

With name-based pattern matching, unapplySeq can return any type that
has an isEmpty and get methods. The object returned by get needs
to have apply, length or lengthCompare and drop methods.

This PR changes the type of x @ _* bound variables to scala.Seq. To support
that change, the object returned by unapplySeq.get is converted by
calling .toSeq or drop(n).

This means there are two changes in the interface for name-based pattern
matching:

  • the object needs to define a toSeq method
  • the drop method needs to return a scala.Seq

The unapplySeq method defined in collection.SeqFactory now returns
a value class wrapper that delegates to the collection. toSeq no
longer exposes mutable collections.

Array.unapplySeq uses a similar value class wrapper.

Fixes scala/bug#11040

@scala-jenkins scala-jenkins added this to the 2.13.0-RC1 milestone Aug 14, 2018

@lrytz lrytz force-pushed the lrytz:t11040 branch from d06a3a2 to 3cbda60 Aug 15, 2018

@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented Aug 15, 2018

@szeiger this should be ready, I didn't run the full test suite locally though.

@lrytz lrytz modified the milestones: 2.13.0-RC1, 2.13.0-M5 Aug 15, 2018

@SethTisue SethTisue self-assigned this Aug 15, 2018

@SethTisue

This comment has been minimized.

Copy link
Member

SethTisue commented Aug 15, 2018

tentatively marked as "prio:blocker"... at least, deciding whether it goes in is a blocker

@lrytz lrytz force-pushed the lrytz:t11040 branch 2 times, most recently from 00104ff to 45abc69 Aug 15, 2018

@retronym
Copy link
Member

retronym left a comment

This looks like a sensible way out of this corner.

@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented Aug 16, 2018

cc @odersky in case you'd like to share your opinion on this

private def genTake(binder: Symbol, n: Int): List[Tree] = (0 until n).toList map (codegen index seqTree(binder))
private def genDrop(binder: Symbol, n: Int): List[Tree] = codegen.drop(seqTree(binder))(expectedLength) :: Nil
private def productElemsToN(binder: Symbol, n: Int): List[Tree] = if (n == 0) Nil else 1 to n map tupleSel(binder) toList
private def genTake(binder: Symbol, n: Int): List[Tree] = if (n == 0) Nil else (0 until n).toList map (codegen index seqTree(binder, forceImmutable = false))

This comment has been minimized.

@viktorklang

viktorklang Aug 19, 2018

Contributor

Seems we could avoid creating both a Range + a List + another List with List.tabulate(n)(codegen index seqTree(binder, forceImmutable = false)) ?

This comment has been minimized.

@lrytz

lrytz Aug 20, 2018

Member

Thanks, pushed that suggestion.

Explicitly call `toSeq` on wildcard-star patterns
With name-based pattern matching, unapplySeq can return any type that
has an `isEmpty` and `get` methods. The object returned by `get` needs
to have `apply`, `length` or `lengthCompare` and `drop` methods.

This PR changes the type of `x @ _*` bound to `scala.Seq`. To support
that change, the object returned by `unapplySeq.get` is converted by
calling `.toSeq` or `drop(n)`.

This means there are two changes in the interface for name-based pattern
matching:
  - the object needs to define a `toSeq` method
  - the `drop` method needs to return a `scala.Seq`

The `unapplySeq` method defined in `collection.SeqFactory` now returns
a value class wrapper that delegates to the collection. `toSeq` no
longer exposes mutable collections.

`Array.unapplySeq` uses a similar value class wrapper.

@SethTisue SethTisue force-pushed the lrytz:t11040 branch from cd2905c to 308ae2d Aug 21, 2018

@SethTisue
Copy link
Member

SethTisue left a comment

squashed. I'll merge as soon as CI likes it

@SethTisue SethTisue merged commit 53c4be1 into scala:2.13.x Aug 21, 2018

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
cla @lrytz signed the Scala CLA. Thanks!
Details
validate-main [4049] SUCCESS. Took 46 min.
Details

SethTisue added a commit to scalacommunitybuild/scala-xml that referenced this pull request Aug 21, 2018

@SethTisue SethTisue changed the title Explicitly call `toSeq` on wildcard-star patterns `x @ _*` bound variables are now `scala.Seq`; name-based pattern matching changes to enable this Aug 22, 2018

@liufengyun

This comment has been minimized.

Copy link

liufengyun commented Sep 4, 2018

I'm looking at supporting this in Dotty. I have a little question regarding the specification for name-based unapplySeq. Given the following result type from get:

  class Data {
    def lengthCompare(len: Int): Int
    def apply(i: Int): T1 = a(i)
    def drop(n: Int): scala.Seq[T2]
    def toSeq: scala.Seq[T3]
  }

My question: does the specification imposes that T1, T2 and T3 are the same type, or the element type is going to be lub(T1, T2, T3)? I prefer the former, which is simple and doesn't seem to harm expressiveness.

Ping @lrytz

@allanrenucci

This comment has been minimized.

Copy link
Contributor

allanrenucci commented Sep 4, 2018

This looks like a bug in scalac:

object Array2 {
  def unapplySeq[T](x: Array[T]): UnapplySeqWrapper[T] = new UnapplySeqWrapper(x)

  class UnapplySeqWrapper[T](a: Array[T]) {
    def isEmpty: Boolean = false
    def get: UnapplySeqWrapper[T] = this
    def lengthCompare(len: Int): Int = ???
    def apply(i: Int): Int = ???
    def drop(n: Int): scala.Seq[String] = ???
    def toSeq: scala.Seq[Double] = ???
  }
}

class Test {
  import Array2._
  def test(xs: Array[Int]) = xs match {
    case Array2(1, 2, ys @ _*) => ys
  }
}
> scalac Test.scala
Test.scala:16: error during expansion of this match (this is a scalac bug).
The underlying error was: type mismatch;
  found   : Seq[String]
  required: Seq[Int]
   def test(xs: Array[Int]) = xs match {
                                 ^
 one error found
@liufengyun

This comment has been minimized.

Copy link

liufengyun commented Sep 4, 2018

Another possibility is not to require T1, T2, T3 to be the same, but designate one of them to be the element type, and the other two types should conform to the element type.

The motivation for this spec is that it seems to simplify implementation lampepfl/dotty#5078 .

@szeiger

This comment has been minimized.

Copy link
Contributor

szeiger commented Sep 4, 2018

Do we need a special relationship between these types at all? The shape of the pattern determines statically which methods are called and therefore which one of T1, T2 and T3 is the actual element type.

@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented Sep 5, 2018

The implementation as of this PR takes the apply method's return type T as element type (see here), and requires the result of toSeq and drop to conform to Seq[T] (see here). So the example by @allanrenucci fails as expected.

I'm happy to discuss other / better ways to skin that cat, of course.

@allanrenucci

This comment has been minimized.

Copy link
Contributor

allanrenucci commented Sep 5, 2018

I'd like to point out that the scalac error message says: "error during expansion of this match (this is a scalac bug)"

So if it is expected, I would not report it as a scalac bug 😄

@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented Sep 5, 2018

Yes, you're right about the message being confusing :-) See scala/bug#11102

@liufengyun

This comment has been minimized.

Copy link

liufengyun commented Sep 6, 2018

Thanks a lot @lrytz , making the result type of apply as element type works well, I've made it explicit in the Dotty spec.

@liufengyun

This comment has been minimized.

Copy link

liufengyun commented Sep 6, 2018

@szeiger It matters for Dotty (I'm not sure about Scalac) as semantic translation of patmat happens after type checking. The typer just assumes an element type. If the types T1, T2 and T3 are not constrained, there may be run-time exceptions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment