Skip to content

Infix sequence wildcard can't match empty sequence in 2.13 (empty multiarg infix not supported) #12110

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

Closed
semkagtn opened this issue Aug 11, 2020 · 16 comments · Fixed by scala/scala#10098

Comments

@semkagtn
Copy link

reproduction steps

Scala version: 2.13.3

object Main
  extends App {

  class Unapplyer {
    def unapplySeq(seq: Seq[Int]): Option[(Int, Seq[Int])] =
      if (seq.isEmpty) None else Some((seq.head, seq.tail))
  }
  val unapplyer = new Unapplyer

  /* 
     v2.12.12:
       prints "1"
     v2.13.3:
       type mismatch;
        found   : Unit
        required: Int
           case value unapplyer () => println(value)
   */
  Seq(1) match {
    case value unapplyer () => println(value)
  }
}

problem

The code above can't be compiled with Scala 2.13.

@dwijnand
Copy link
Member

2.13:

$ rm -r out
$ mkdir out
$ scalac -d out $(f 'class Unapplyer { def unapplySeq(seq: Seq[Int]): Option[(Int, Seq[Int])] = if (seq.isEmpty) None else Some((seq.head, seq.tail)) }')
$ scalac -cp out -Vprint:typer $(f 'class C { val unapplyer = new Unapplyer; def m = Seq(1) match { case value unapplyer () => value } }')

/var/folders/yh/qsn4cxqj47gbhh9_prlxqy3r0000gn/T/scala-test.HDllnj05.scala:1: error: type mismatch;
 found   : Unit
 required: Int
class C { val unapplyer = new Unapplyer; def m = Seq(1) match { case value unapplyer () => value } }
                                                                           ^
[[syntax trees at end of                     typer]] // scala-test.HDllnj05.scala
package <empty> {
  class C extends scala.AnyRef {
    def <init>(): C = {
      C.super.<init>();
      ()
    };
    private[this] val unapplyer: Unapplyer = new Unapplyer();
    <stable> <accessor> def unapplyer: Unapplyer = C.this.unapplyer;
    def m: Int = scala.collection.immutable.Seq.apply[Int](1) match {
      case C.this.unapplyer.unapplySeq(<unapply-selector>) <unapply> ((value @ _), ()) => value
    }
  }
}

1 error

2.12:

$ rm -r out212
$ mkdir out212
$ scalac -212 -d out212 $(f 'class Unapplyer { def unapplySeq(seq: Seq[Int]): Option[(Int, Seq[Int])] = if (seq.isEmpty) None else Some((seq.head, seq.tail)) }')
$ scalac -212 -cp out212 -Xprint:typer $(f 'class C { val unapplyer = new Unapplyer; def m = Seq(1) match { case value unapplyer () => value } }')

[[syntax trees at end of                     typer]] // scala-test.2MnsWGH7.scala
package <empty> {
  class C extends scala.AnyRef {
    def <init>(): C = {
      C.super.<init>();
      ()
    };
    private[this] val unapplyer: Unapplyer = new Unapplyer();
    <stable> <accessor> def unapplyer: Unapplyer = C.this.unapplyer;
    def m: Int = scala.collection.Seq.apply[Int](1) match {
      case C.this.unapplyer.unapplySeq(<unapply-selector>) <unapply> ((value @ _)) => value
    }
  }
}

First of all I'm surprised that def unapplySeq(seq: Seq[Int]): Option[(Int, Seq[Int])] is even considered, as I thought (and https://docs.scala-lang.org/tour/extractor-objects.html describes) that it needs to be Option[Seq[T]].

Secondly I'd never seen infix notation for unapplySeq - does that follows SLS 8.1.11?

8.1.11 Infix Operation Patterns

  Pattern3  ::=  SimplePattern {id [nl] SimplePattern}

An infix operation pattern 𝑝;op;𝑞 is a shorthand for the constructor or extractor pattern op(𝑝,𝑞). The precedence and associativity of operators in patterns is the same as in expressions.

An infix operation pattern 𝑝;op;(𝑞1,…,𝑞𝑛) is a shorthand for the constructor or extractor pattern op(𝑝,𝑞1,…,𝑞𝑛).

So unapplyer.unapplySeq(value, ()), what 2.13 is doing, seems to follow the spec, right?

My feeling and vote right now is that the code shouldn't've compiled in 2.12, so it's not a bug in 2.13.

@niktrop
Copy link

niktrop commented Aug 12, 2020

An infix operation pattern 𝑝;op;(𝑞1,…,𝑞𝑛) is a shorthand for the constructor or extractor pattern op(𝑝,𝑞1,…,𝑞𝑛)

I would say replacing p op () with op(p) is a natural interpretation for n = 0.

Secondly I'd never seen infix notation for unapplySeq - does that follows SLS 8.1.11?

SLS 8.1.11 is purely syntactical transformation, unapplySeq is chosen after it according to SLS 8.1.10

We use such extractors in Scala plugin to match AST nodes of method invocations. It is very convenient to have a single extractor to work with any number of argument expressions.

@dwijnand
Copy link
Member

Oh, wow, TIL

scala> object Unapplyer { def unapplySeq(seq: Seq[Int]): Option[(Int, Seq[Int])] = if (seq.isEmpty) None else Some((seq.head, seq.tail)) }
object Unapplyer

scala> Seq(1) match { case Unapplyer(value) => value }
val res1: Int = 1

@Jasper-M
Copy link

FWIW Dotty only seems to support infix notation for binary extractors. So that's only the first kind of infix operation pattern that the spec mentions.

@som-snytt
Copy link

The expression spec says "several arguments in parens", which excludes n = 0.

I don't think the pattern was discussed, but x op () is x.op(()), where the paradigm is () == ().

I see attention was paid to the community build to get a sense of whether the obsolete usage was common.

@dwijnand
Copy link
Member

I wondered if x op (()) is x.op(()), and today's encounter with a -> (b, c) needing to be a -> ((b, c)) kind of reinforced it for me... 🤔 🤷

@niktrop
Copy link

niktrop commented Aug 13, 2020

@som-snytt I never saw () as a pattern for Unit value, but it is indeed in the spec. I still find it inconsistent that unapplySeq extractor cannot be used in infix form if sequence is empty, but I understand the ambiguity now.

@hmemcpy
Copy link

hmemcpy commented Oct 9, 2020

Hey, sorry for bumping this thread. This infix style is used heavily in the IntelliJ Scala plugin for extracting values in their ASTs. The Scala plugin for IntelliJ historically was targeting Scala 2.12, and for their latest release they are finally targeting 2.13.

Hmm, I just noticed that the OP is an IntelliJ Scala plugin developer who sent the 2.13 PR :)
JetBrains/intellij-scala@a54c2a7

(for example:)
image

Anyway, I was upgrading my own IntelliJ plugin for ZIO to 2.13 and ran into the same issue. In my case I also had to fix a bunch of:

deprecated adaptation: class StaticMemberReference expects 2 patterns to hold (zio.intellij.utils.types.ZioType, org.jetbrains.plugins.scala.lang.psi.api.expr.ScExpression) but crushing into 2-tuple to fit single pattern (scala/bug#6675)

warnings when using those extractors.
Not sure those are related, but I suspect the infix syntax no longer works due to the tuple no longer being adapted.

Is there an alternative syntax that may be considered? I'd really like the "before" example in the above image to work :)

@dwijnand
Copy link
Member

dwijnand commented Oct 9, 2020

Not sure those are related, but I suspect the infix syntax no longer works due to the tuple no longer being adapted.

Looks at my notes above it looks the other way round? 2.13 is adapting where 2.12 wasn't:

2.13

    def m: Int = scala.collection.immutable.Seq.apply[Int](1) match {
      case C.this.unapplyer.unapplySeq(<unapply-selector>) <unapply> ((value @ _), ()) => value
    }

2.12

    def m: Int = scala.collection.Seq.apply[Int](1) match {
      case C.this.unapplyer.unapplySeq(<unapply-selector>) <unapply> ((value @ _)) => value
    }

If someone could bisect the commit that causes this to no longer work, I could look into seeing whether its reversible or not.

Is there an alternative syntax that may be considered? I'd really like the "before" example in the above image to work :)

For new syntaxes, you must ask Dotty, on https://contributors.scala-lang.org/.

@som-snytt
Copy link

som-snytt commented Oct 9, 2020

To summarize, expression e1 op e2 is e1.op(e2) for binary and e1 op (e2, e3) is e1.op(e2, e3).

The spec doesn't mention a hypothetical unary case with only one expression, but the language for "multiarg infix" legislates against it (there must be several args).

There was an earlier "style" edge case [not in the spec] where e1 op () was taken as e1.op(), but this means you can't write () == (), which is not very useful but looks very regular.

Patterns look the same as the corresponding expressions. It's worth noting that e1 op () expression syntax was also found (in one spot) in the compiler for AST construction, so it's not surprising that the pattern syntax was exploited for that use case.

In Scala 3:

scala> Seq(1, 2, 3) match { case value unapplyer (rest: _*) => (value, rest) }
1 |Seq(1, 2, 3) match { case value unapplyer (rest: _*) => (value, rest) }
  |                                                 ^
  |            `_*` can be used only for last argument of method application.
  |            It is no longer allowed in operands of infix operations.

In Scala 2, the syntax for sequence wildcard means you can't say case X(Nil @ _*) => (which used to be rejected, but now allows you to introduce a name). The adjusted syntax in Scala 3 might have allowed Nil: _* as the desired alternative to match Seq.empty in infix. But I guess sequence wildcard doesn't work as the syntax suggests anyway:

scala> Seq(1) match { case unapplyer(h, Nil: _*) => }
1 |Seq(1) match { case unapplyer(h, Nil: _*) => }
  |                                 ^^^
  |                                 Found:    collection.immutable.Nil.type
  |                                 Required: Int

scala> Seq(1) match { case unapplyer(h, X @ _: _*) => }
1 |Seq(1) match { case unapplyer(h, X @ _: _*) => }
  |                                     ^
  |                                     Found:    any2stringadd.type
  |                                     Required: Int

Somehow I sense a puzzler coming on in that last example.

I will create a feature request on Scala 3 for the infix _*, just for target practice; but perhaps they will not shoot it down.

There was some discussion whether Scala 3 will support "multiarg infix" for expressions, and as noted in a previous comment, it is not supported in patterns. I don't know if that makes "infix sequence wildcard" even less likely.

@som-snytt
Copy link

Old Scala:

scala> List(42) match { case h List () => }

scala> List(42) match { case h :: Nil => }
<console>:8: warning: match may not be exhaustive.
It would fail on the following inputs: List(), List(_, _), Nil
              List(42) match { case h :: Nil => }
                  ^

@som-snytt
Copy link

Scala 3 post-pandemic:

scala> object X { def unapplySeq[A](as: Seq[A]): Option[(A, Seq[A])] = if (as.isEmpty) None else Some((as.head, as.tail)) }
// defined object X

scala> List(42) match { case h X (t*) => (h, t) case _ => ??? }
val res0: (Int, Seq[Int]) = (42,List())

scala> List(42, 17) match { case h X (t*) => (h, t) case _ => ??? }
val res1: (Int, Seq[Int]) = (42,List(17))

scala> List(42) match { case h X (_*) => h case _ => ??? }
val res2: Int = 42

scala> List(42, 17) match { case h X (Nil*) => (h, t) case _ => ??? }
-- Error:
1 |List(42, 17) match { case h X (Nil*) => (h, t) case _ => ??? }
  |                                   ^
  |                                   `*` must follow pattern variable

@som-snytt som-snytt changed the title Infix notation in pattern matching for unapplySeq in Scala 2.13 doesn't work in the specific case Infix sequence wildcard can't match empty sequence in 2.13 (empty multiarg infix not supported) Oct 31, 2021
@SethTisue SethTisue removed this from the Backlog milestone Aug 3, 2022
@SethTisue SethTisue reopened this Aug 3, 2022
@SethTisue SethTisue added this to the 2.13.9 milestone Aug 3, 2022
@SethTisue
Copy link
Member

@som-snytt I merged the PR adding tests of the status quo.

This may be implicit in what you have already written, but would you mind explicitly stating what the status is here, in both Scala 2 and 3? Did you add the status-quo tests because you believe it's a wontfix for 2.13? Is Scala 3's behavior the same or different than 2.13's?

@som-snytt
Copy link

This is wontfix for 2.13 despite our wont for fixing things.

As @Jasper-M noted, Scala 3 supports only binary case x X y =>. #12110 (comment)

The dotty feature request is lampepfl/dotty-feature-requests#144

Innovative solutions should be posted there. Maybe the answer is a spec tweak to give case x X () => the desired semantics.

There is an open dotty ticket for "improved" support for multiarg infix expressions:
scala/scala3#14418

which links to the stalled effort to remove multiarg infix syntax altogether.

Maybe also restore expr x.op() as x op () where x != (), which will come to be known as Lausanne notation.

@som-snytt
Copy link

I neglected to commit a test, which I expected to warn under -Xsource:3

--- a/test/files/neg/t12110.scala
+++ b/test/files/neg/t12110.scala
@@ -1,4 +1,4 @@
-
+// scalac: -Werror -Xsource:3
 object Main extends App {

   class Unapplyer {
@@ -19,6 +19,9 @@ object Main extends App {
   Seq(1) match {
     case value unapplyer () => println(value)
   }
+  Seq(42, 28, 17) match {
+    case value unapplyer (x, y) => println(x + y)
+  }
 }

Dotty is especially obscure because of the tuple:

-- Error: test/files/neg/t12110.scala:20:25 ----------------------------------------------------------------------------
20 |    case value unapplyer () => println(value)
   |                         ^^
   |                         Values of types Unit and Int cannot be compared with == or !=
-- [E008] Not Found Error: test/files/neg/t12110.scala:23:45 -----------------------------------------------------------
23 |    case value unapplyer (x, y) => println(x + y)
   |                                           ^^^
   |                             value + is not a member of Any, but could be made available as an extension method.
   |
   |                             One of the following imports might make progress towards fixing the problem:
   |
   |                               import math.Fractional.Implicits.infixFractionalOps
   |                               import math.Integral.Implicits.infixIntegralOps
   |                               import math.Numeric.Implicits.infixNumericOps
   |
2 errors found

@SethTisue SethTisue removed this from the 2.13.9 milestone Aug 3, 2022
@SethTisue
Copy link
Member

Thank you!

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

Successfully merging a pull request may close this issue.

7 participants