Add error message for missing unapply on pattern matching #5180

sleepiecappy · 2018-09-30T17:05:44Z

this commit adds a message and explanation by
#1589 instructions for when the user do not implement the
unapply method on class' object companion, causing the compiler
not founding a way to extract the arguments.

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

sleepiecappy · 2018-09-30T17:09:45Z

Note that I wanted to include only the class name which does not declare the method, so I could use in the explanation section, but I could not find a way from acquiring it from Tree, I could only grab MyClass.unapply from tree.show thus I adapted the message.

Also, I had to add one missing parameter at handleUnexpectedFunType(tree: untpd.Apply, fun: Tree)(implicit ctx: Context): Tree so I could compile the project.

Any notes on how I can improve this and especially the explanation let me know! Thanks <3

OlivierBlanvillain · 2018-10-02T16:41:51Z

vscode-dotty/package-lock.json

-        "fast-deep-equal": "^1.0.0",
-        "fast-json-stable-stringify": "^2.0.0",
-        "json-schema-traverse": "^0.3.0"
+        "co": "4.6.0",


Could you remove this part of the diff?

OlivierBlanvillain

LGTM otherwise

OlivierBlanvillain · 2018-10-02T17:19:54Z

compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

+           |
+           | class Foo(val name: String)
+           | object Foo {
+           |   def unapply(foo: Foo): Option[String] = Option(foo.name)


I would put Some(foo.name) here.

compiler/test/dotty/tools/dotc/reporting/ErrorMessagesTests.scala

allanrenucci · 2018-10-02T17:45:33Z

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

@@ -896,7 +896,7 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>
    val Apply(qual, args) = tree

    def notAnExtractor(tree: Tree) =
-      errorTree(tree, s"${qual.show} cannot be used as an extractor in a pattern because it lacks an unapply or unapplySeq method")
+      errorTree(tree, NotAnExtractor(tree), tree.pos)


Why change from qual to tree? Also, no need to specify the position. It defaults to tree.pos.

allanrenucci · 2018-10-02T17:53:20Z

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

@@ -843,7 +843,7 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>

  /** Overridden in ReTyper to handle primitive operations that can be generated after erasure */
  protected def handleUnexpectedFunType(tree: untpd.Apply, fun: Tree)(implicit ctx: Context): Tree =
-    throw new Error(i"unexpected type.\n  fun = $fun,\n  methPart(fun) = ${methPart(fun)},\n  methPart(fun).tpe = ${methPart(fun).tpe},\n  tpe = ${fun.tpe}")
+    throw new Error(i"unexpected type.\n  fun = $fun,\n  methPart(fun) = ${methPart(fun)},\n  methPart(fun).tpe = ${methPart(fun).tpe},\n  tpe = ${fun.tpe}", tree.pos)


This change is very suspicious. This is not the same Error as before. This is due to the wildcard import:

import dotty.tools.dotc.reporting.diagnostic.messages._

Only import what you need

allanrenucci · 2018-10-02T17:57:39Z

compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

@@ -2117,4 +2117,22 @@ object messages {
    override def msg: String = hl"""normal case class cannot extend an enum. case $cls in ${cls.owner} is extending enum ${parent.name}."""
    override def explanation: String = ""
  }
+
+  case class NotAnExtractor(tree: tpd.Tree)(implicit ctx: Context) extends Message(NotAnExtractorID) {
+    override def msg: String = hl"""${tree.show} cannot be used as an extractor in a pattern because it is not implemented"""


No need for triple quote. Also .show is not needed with the hl interpolator

allanrenucci · 2018-10-02T18:11:17Z

compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

+      hl"""| ${tree.show} is not a valid extractor for use in pattern matching,
+           | this happens when the method unapply or unapplySeq it is not declared
+           | on it's object companion.
+           | An unapply method take an object and tries to give back the arguments


This is one use case among others. E.g.

object PositiveInt { def unapply(x: Int) = x >= 0 } def foo(x: Int) = x match { case PositiveInt() => ...

I would replace your explanation and example by something that explain what is a valid extractor object. A brief summary of https://docs.scala-lang.org/tour/extractor-objects.html would be awesome

I replaced the example with some usages of Extractors, I did not include code examples for each to not become length. Also, I thought about adding the required return types but this error is already checked at Applications.scala#L99

Sorry, I didn't mean to add more use cases but explain how one can write a valid extractor object. E.g.

def explanation = hl"""|An `unapply` method should be defined as follow: | - If it is just a test, return a `Boolean`. For example `case even()` | - If it returns a single sub-value of type T, return an `Option[T]` | - If it returns several sub-values T1,...,Tn, group them in an optional tuple `Option[(T1,...,Tn)]` | |Sometimes, the number of sub-values isn’t fixed and we would like to return a sequence. |For this reason, you can also define patterns through `unapplySeq` which returns `Option[Seq[T]]`. |This mechanism is used for instance in pattern `case List(x1, ..., xn)`"""

Got it! Should we also point that the unapply method should be defined in an object ? E.g.

An `unapply` method should be defined in an `object` as follow:

I think of cases where the user uses a plain class instead of an object, then tries to define the method inside the class and still get the error

Yes, sounds good!

allanrenucci · 2018-10-02T18:13:43Z

compiler/test/dotty/tools/dotc/reporting/ErrorMessagesTests.scala

+    }.expect { (ictx, messages) =>
+      implicit val ctx: Context = ictx
+      assertMessageCount(2, messages)
+      assertTrue(messages(1).isInstanceOf[NotAnExtractor])


We also try to test the content of the error message. In your case, you could do something like

val NotAnExtractor(tree) = messages.head assertEqual("Foo", tree.show)

sleepiecappy · 2018-10-02T18:37:21Z

Thanks for the reviews guys, I'll look into each and push the changes

sleepiecappy · 2018-10-03T01:27:40Z

@allanrenucci Any further tips, especially on the explanation, are welcome!

allanrenucci · 2018-10-03T08:11:12Z

compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

@@ -2118,21 +2118,19 @@ object messages {
    override def explanation: String = ""
  }

-  case class NotAnExtractor(tree: tpd.Tree)(implicit ctx: Context) extends Message(NotAnExtractorID) {
-    override def msg: String = hl"""${tree.show} cannot be used as an extractor in a pattern because it is not implemented"""
+  case class NotAnExtractor(tree: Trees.Tree[Untyped])(implicit ctx: Context) extends Message(NotAnExtractorID) {


Trees.Tree[Untyped] is untpd.Tree

allanrenucci · 2018-10-03T08:15:23Z

compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

    override def kind: String = "Syntax"
    override def explanation: String =
-      hl"""| ${tree.show} is not a valid extractor for use in pattern matching,
+      hl"""| $tree is not a valid extractor for use in pattern matching,
           | this happens when the method unapply or unapplySeq it is not declared
           | on it's object companion.


I would remove the first sentence of the explanation. When the error is reported with -explain, we print both msg and explanation. So the explanation should give additional information and not rephrase msg

allanrenucci · 2018-10-03T08:27:42Z

compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

+      hl"""| ${tree.show} is not a valid extractor for use in pattern matching,
+           | this happens when the method unapply or unapplySeq it is not declared
+           | on it's object companion.
+           | An unapply method take an object and tries to give back the arguments


Sorry, I didn't mean to add more use cases but explain how one can write a valid extractor object. E.g.

def explanation = hl"""|An `unapply` method should be defined as follow: | - If it is just a test, return a `Boolean`. For example `case even()` | - If it returns a single sub-value of type T, return an `Option[T]` | - If it returns several sub-values T1,...,Tn, group them in an optional tuple `Option[(T1,...,Tn)]` | |Sometimes, the number of sub-values isn’t fixed and we would like to return a sequence. |For this reason, you can also define patterns through `unapplySeq` which returns `Option[Seq[T]]`. |This mechanism is used for instance in pattern `case List(x1, ..., xn)`"""

allanrenucci

LGTM. Thanks @sleepiejohn 🎉! Can you squash your commits into a single one?

this commit adds a message and explanation by unapply method on class' object companion, causing the compiler not founding a way to extract the arguments

sleepiecappy · 2018-10-03T16:35:46Z

Done! 😀

dottybot reviewed Sep 30, 2018

View reviewed changes

OlivierBlanvillain assigned allanrenucci Oct 2, 2018

OlivierBlanvillain reviewed Oct 2, 2018

View reviewed changes

OlivierBlanvillain suggested changes Oct 2, 2018

View reviewed changes

OlivierBlanvillain approved these changes Oct 2, 2018

View reviewed changes

allanrenucci suggested changes Oct 2, 2018

View reviewed changes

allanrenucci assigned sleepiecappy and unassigned allanrenucci Oct 2, 2018

allanrenucci reviewed Oct 3, 2018

View reviewed changes

allanrenucci approved these changes Oct 3, 2018

View reviewed changes

add error message for missing unapply on pattern matching

08373f4

this commit adds a message and explanation by unapply method on class' object companion, causing the compiler not founding a way to extract the arguments

allanrenucci merged commit 3eee37d into scala:master Oct 3, 2018

sleepiecappy deleted the error-msg/not-extractor branch October 3, 2018 18:21

Add error message for missing unapply on pattern matching #5180

Add error message for missing unapply on pattern matching #5180

Uh oh!

Conversation

sleepiecappy commented Sep 30, 2018

Uh oh!

dottybot left a comment

Choose a reason for hiding this comment

Uh oh!

sleepiecappy commented Sep 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OlivierBlanvillain Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlivierBlanvillain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sleepiecappy Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sleepiecappy commented Oct 2, 2018

Uh oh!

sleepiecappy commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allanrenucci left a comment

Choose a reason for hiding this comment

Uh oh!

sleepiecappy commented Oct 3, 2018

Uh oh!

Uh oh!

sleepiecappy commented Sep 30, 2018 •

edited

Loading

OlivierBlanvillain Oct 2, 2018 •

edited

Loading

sleepiecappy Oct 3, 2018 •

edited

Loading

sleepiecappy commented Oct 3, 2018 •

edited

Loading