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

Anonymous Function expression treated as a PartialFunction (spec needs broadened) #4940

Open
scabug opened this issue Aug 24, 2011 · 24 comments

Comments

@scabug
Copy link

scabug commented Aug 24, 2011

The following compiles, although I think the conversion to type PartialFunction from this anonymous function expression is disallowed (there is no mention in SLS, Section 6.23, Anonymous Functions)

val g: PartialFunction[Int, Int] = (x => x match { case 2 => 3 })

Section 8.5, Pattern Matching Anonymous Functions, introduces the syntax {case ...} for defining a PartialFunction. If a Function type is expected, then the expression {case ...} is reinterpreted as x => x match {case ...}. The strange thing here is that the opposite conversion is happening: a Function expression x => x match {case ...} is given, and the compiler is interpreting it as a PartialFunction, {case ...}.

Here's another example:

Seq(1, "a").collect(x => x match { case s: String => s }) // evaluates to Seq(a)

If the Function expression gets a little more complicated, the conversion from Function to PartialFunction fails:

// this compiles
val g: PartialFunction[Int, Int] = (x: Int) => {x match { case 2 => 3 }}

// this fails; found Function[Int, Int], required PartialFunction[Int, Int]
val g: PartialFunction[Int, Int] = (x: Int) => {(); x match { case 2 => 3 }}

Apologies if I am misunderstanding the Spec.

@scabug
Copy link
Author

scabug commented Aug 24, 2011

Imported From: https://issues.scala-lang.org/browse/SI-4940?orig=1
Reporter: Kipton Barros (kbarros)

@scabug
Copy link
Author

scabug commented Aug 24, 2011

Kipton Barros (kbarros) said:
To clarify, the issue is that the first code sample compiles, although it should be illegal according to (my reading of) the Spec. An anonymous function expression should not be a valid PartialFunction.

@scabug
Copy link
Author

scabug commented Jan 31, 2012

@lrytz said:
re-word the spec

@scabug
Copy link
Author

scabug commented Jun 6, 2015

@som-snytt said:
Arguably, 8.5 on pattern matching anon funcs already says that if the expected type is Func, then the block of cases is equivalent to the function. If additionally the expected type is narrower, a PartialFunc, then the expression is equivalent to the PF. Maybe substitute s/is/conforms to/.

@scabug
Copy link
Author

scabug commented Jun 17, 2015

@SethTisue said:
Why would be it desirable here to change the spec to match the compiler's behavior? My intuition matches Kipton's; it seems like a compiler bug to me.

@scabug
Copy link
Author

scabug commented Jun 17, 2015

@som-snytt said:
Bumping the bug part.

SKALA 2.11.7-20150616-093756-43a56fb5a1> val g: PartialFunction[String, Int] = (x: Int) => x match { case "x" => 3 }
g: PartialFunction[String,Int] = <function1>

@SethTisue
Copy link
Member

someone want to take a look and see whether this should remain open? I wonder what Dotty does here.

@dwijnand
Copy link
Member

dwijnand commented Mar 19, 2020

// this fails; found Function[Int, Int], required PartialFunction[Int, Int]
val g: PartialFunction[Int, Int] = (x: Int) => {(); x match { case 2 => 3 }}

This one compiles now 😄

Welcome to Scala 2.13.1 (OpenJDK 64-Bit Server VM, Java 11.0.6).
Type in expressions for evaluation. Or try :help.

scala> val g: PartialFunction[Int, Int] = (x: Int) => {(); x match { case 2 => 3 }}
g: PartialFunction[Int,Int] = <function1>

In dotty it doesn't, and Som's variant doesn't work in Dotty:

scala> val g: PartialFunction[Int, Int] = (x: Int) => {(); x match { case 2 => 3 }}
1 |val g: PartialFunction[Int, Int] = (x: Int) => {(); x match { case 2 => 3 }}
  |                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |                                    Found:    Int => Int
  |                                    Required: PartialFunction[Int, Int]

scala> val g: PartialFunction[String, Int] = (x: Int) => x match { case "x" => 3 }
1 |val g: PartialFunction[String, Int] = (x: Int) => x match { case "x" => 3 }
  |                                                                 ^^^
  |           Values of types String and Int cannot be compared with == or !=

I don't think this issue is worth fixing (but I'm fine with spec changes, less sure about compiler changes). voteForClose += 1

@som-snytt
Copy link

som-snytt commented Mar 19, 2020

I think the bug where Scala 2 ignores the type of the parameter is just a bug. It overdoes the equivalence of _ match { case _ => } and functionish things.

A different view:

scala> val g: PartialFunction[String, Int] = (x: Int) => x match { case 42 => 27 }
                                                                        ^
       error: type mismatch;
        found   : Int(42)
        required: String

I think my sense that 8.5 already justifies the working behavior is:

scala> { case 42 => 0 }: Comparable[Int]
val res7: Comparable[Int] = $Lambda$2315/0x00000008019ea440@37e64e37

scala> ((i: Int) => i match { case 42 => 0 }): Comparable[Int]
val res8: Comparable[Int] = $Lambda$2316/0x000000080183e840@6d5de79a

scala> ((i: Int) => i match { case 42 => 0 }): PartialFunction[Int, Int]
val res9: PartialFunction[Int,Int] = <function1>

Obviously, isDefinedAt reflects the cases.

@SethTisue
Copy link
Member

SethTisue commented Oct 29, 2020

relevant passage from SLS 6.23.1 that I don't recall having seen before:

When a PartialFunction is required, an additional member isDefinedAt is synthesized, which simply returns true. However, if the function literal has the shape x => x match { … }, then isDefinedAt is derived from the pattern match in the following way: each case from the match expression evaluates to true, and if there is no default case, a default case is added that evaluates to false.

@nigredo-tori
Copy link

Another weird case:

scala> val f: PartialFunction[String, Int] = _.toInt match {
  case 0 => 0
}
f: PartialFunction[String,Int] = <function1>

This is the resulting PartialFunction (thanks, @SethTisue):

private[this] val f: PartialFunction[String,Int] = ({
  @SerialVersionUID(value = 0) final <synthetic> class $anonfun extends scala.runtime.AbstractPartialFunction[String,Int] with java.io.Serializable {
    def <init>(): <$anon: String => Int> = {
      $anonfun.super.<init>();
      ()
    };
    final override def applyOrElse[A1 <: String, B1 >: Int](x$1: A1, default: A1 => B1): B1 = (scala.Predef.augmentString(x$1).toInt: Int @unchecked) match {
      case 0 => 0
      case (defaultCase$ @ _) => default.apply(x$1)
    };
    final def isDefinedAt(x$1: String): Boolean = (scala.Predef.augmentString(x$1).toInt: Int @unchecked) match {
      case 0 => true
      case (defaultCase$ @ _) => false
    }
  };
  new $anonfun()
}: PartialFunction[String,Int]);
<stable> <accessor> def f: PartialFunction[String,Int] = $iw.this.f

@SethTisue
Copy link
Member

SethTisue commented Oct 29, 2020

So the original ticket is about the compiler interpreting the 6.23.1 language too loosely, but it appears to me that @nigredo-tori has found that the compiler is WAY looser than we realized.

Rather to my astonishment, Dotty (0.27.0-RC1) accepts it as well:

Starting dotty REPL...
scala> val f: PartialFunction[String, Int] = _.toInt match { case 0 => 0 }
val f: PartialFunction[String, Int] = <function1>

scala> f.isDefinedAt("1")
val res0: Boolean = false

@som-snytt
Copy link

som-snytt commented Oct 29, 2020

Seth was the quoting the spec added by Dale. That's why Dale was grinning in his previous comment.

Possibly, Dale intended :trollface: .

In the context of the preceding discussion, I think f is "normal":

      val f: PartialFunction[String, Int] = ((x$1) => x$1.toInt match {
        case 0 => 0
      })

My pre-pandemic comment, of which I have no recollection, would suggest

ForClose.vote--

@dwijnand
Copy link
Member

dwijnand commented Oct 29, 2020

I've not tested this or anything, but I'd put some easy money on that possibly being because of https://github.com/scala/scala/blob/df25f7ab79baa3d566755bf41572cb8f6454a9c3/src/compiler/scala/tools/nsc/transform/patmat/MatchTranslation.scala#L611-L614

            // scala/bug#7868 Account for numeric widening, e.g. <unapplySelector>.toInt
            case Apply(x, List(i @ (sel @ Select(Ident(nme.SELECTOR_DUMMY), name)))) =>
              // not substituting `binder` for `i.symbol`: widening conversion implies the binder could not be used as a path
              treeCopy.Apply(t, x, treeCopy.Select(sel, binderRef(i.pos), name) :: Nil)

i.e. #7868, i.e. it's interacting with the implicit numeric widening in the typer?

@nigredo-tori
Copy link

nigredo-tori commented Oct 29, 2020

@dwijnand, toInt is only as an example of a transformation that can throw. This also compiles:

val f: PartialFunction[String, Unit] = _.toUpperCase match {
  case "" => ()
}

UPD: although I can see that the code you've quoted is not specific to widening, so please disregard this comment.

@Jasper-M
Copy link
Member

Jasper-M commented Oct 29, 2020

Regarding @som-snytt's 5 year old example:

scala> val g: PartialFunction[String, Int] = (x: Int) => x match { case "x" => 3 }
val g: PartialFunction[String,Int] = <function1>

scala> g("x")
val res2: Int = 3

That looks like a clear bug.

I don't really see how most other examples in this issue are problematic. The ones where the input is transformed before the pattern match are pretty liberal but whether that's a real problem is debatable.

@Jasper-M
Copy link
Member

It's actually kind of a feature IMHO:

scala> List("1","two","3").collect{ x => 
     |   Try(x.toInt) match { 
     |     case Success(int) => int 
     |   }
     | }
val res28: List[Int] = List(1, 3)

@martijnhoekstra
Copy link

How does that desugar exactly?

@dwijnand
Copy link
Member

dwijnand commented Nov 19, 2020

$ scala -Vprint:patmat -e 'List("1", "two", "3").collect { x => scala.util.Try(x.toInt) match { case scala.util.Success(int) => int } }'
[[syntax trees at end of                    patmat]] // scalacmd7634219973344927097.scala
  object Main extends scala.AnyRef {
    def main(args: Array[String]): Unit = {
      final class $anon extends scala.AnyRef {
        scala.collection.immutable.List.apply[String]("1", "two", "3").collect[Int](({
          @SerialVersionUID(value = 0) final <synthetic> class $anonfun extends scala.runtime.AbstractPartialFunction[String,Int] with java.io.Serializable {
            final override def applyOrElse[A1 <: String, B1 >: Int](x: A1, default: A1 => B1): B1 = {
              case <synthetic> val x1: scala.util.Try[Int] = (scala.util.Try.apply[Int](scala.Predef.augmentString(x).toInt): scala.util.Try[Int] @unchecked);
              case5() {
                if (x1.isInstanceOf[scala.util.Success[Int]]) {
                  <synthetic> val x2: scala.util.Success[Int] = x1.asInstanceOf[scala.util.Success[Int]];
                  val int: Int = x2.value;
                  matchEnd4(int)
                } else
                  case6()
              };
              case6() { matchEnd4(default.apply(x)) };
              matchEnd4(x: B1) { x }
            };
            final def isDefinedAt(x: String): Boolean = {
              case <synthetic> val x1: scala.util.Try[Int] = (scala.util.Try.apply[Int](scala.Predef.augmentString(x).toInt): scala.util.Try[Int] @unchecked);
              case5() {
                if (x1.isInstanceOf[scala.util.Success[Int]])
                  matchEnd4(true)
                else
                  case6()
              };
              case6() { matchEnd4(false) };
              matchEnd4(x: Boolean) { x }
            }
          };
          new $anonfun()
        }: PartialFunction[String,Int]))
      };
      new $anon();
      ()
    }
  }

@SethTisue
Copy link
Member

It seems that we have a rough consensus, here and on scala/scala#10483 , that the behavior is plausibly desirable, and since Scala 3 allows it too, we should leave well enough alone in Scala 2 as well.

If someone were to succeed in persuading the Scala 3 folks to deprecate or disallow, we could then warn in Scala 2 under -Xsource:3. But that process would have to start with Scala 3.

@SethTisue SethTisue closed this as not planned Won't fix, can't repro, duplicate, stale Sep 19, 2023
@SethTisue SethTisue reopened this Sep 19, 2023
@SethTisue
Copy link
Member

On second thought, reopening but reclassifying as a spec ticket.

@SethTisue SethTisue changed the title Anonymous Function expression treated as a PartialFunction Anonymous Function expression treated as a PartialFunction (spec needs broadened) Sep 19, 2023
@som-snytt
Copy link

The linked PR shows the bug List(42).collect((x: String) => x match { case "42" => 27 })

@SethTisue
Copy link
Member

Ah, I hadn't seen scala/scala#10486 yet.

I'm still comfortable with the disposition of this ticket though — 10486 is narrower, though under the same umbrella.

@lrytz
Copy link
Member

lrytz commented Sep 19, 2023

I think we should align with Scala 3, it does support operations on the scrutinee but gives a clear error message for List(42).collect((x: String) => x match { case "42" => 27 })

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