Merge pull request #8317 from dotty-staging/fix-unapply-reporting

Better error reporting for patterns

Author: anatoliykmetyuk

compiler/src/dotty/tools/dotc/reporting/ConsoleReporter.scala

@@ -24,19 +24,19 @@ class ConsoleReporter(
   def doReport(m: MessageContainer)(implicit ctx: Context): Unit = {
     val didPrint = m match {
       case m: Error =>
-        printMessage(messageAndPos(m.contained(), m.pos, diagnosticLevel(m)))
+        printMessage(messageAndPos(m.contained, m.pos, diagnosticLevel(m)))
         if (ctx.settings.Xprompt.value) Reporter.displayPrompt(reader, writer)
         true
       case m: ConditionalWarning if !m.enablingOption.value =>
         false
       case m =>
-        printMessage(messageAndPos(m.contained(), m.pos, diagnosticLevel(m)))
+        printMessage(messageAndPos(m.contained, m.pos, diagnosticLevel(m)))
         true
     }
 
     if (didPrint && ctx.shouldExplain(m))
-      printMessage(explanation(m.contained()))
-    else if (didPrint && m.contained().explanation.nonEmpty)
+      printMessage(explanation(m.contained))
+    else if (didPrint && m.contained.explanation.nonEmpty)
       printMessage("\nlonger explanation available when compiling with `-explain`")
   }
 

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

@@ -15,7 +15,7 @@ object MessageContainer {
   implicit class MessageContext(val c: Context) extends AnyVal {
     def shouldExplain(cont: MessageContainer): Boolean = {
       implicit val ctx = c
-      cont.contained().explanation match {
+      cont.contained.explanation match {
         case "" => false
         case _ => ctx.settings.explain.value
       }
@@ -39,7 +39,7 @@ class MessageContainer(
   /** The message to report */
   def message: String = {
     if (myMsg == null) {
-      myMsg = contained().msg.replaceAll("\u001B\\[[;\\d]*m", "")
+      myMsg = contained.msg.replaceAll("\u001B\\[[;\\d]*m", "")
       if (myMsg.contains(nonSensicalStartTag)) {
         myIsNonSensical = true
         // myMsg might be composed of several d"..." invocations -> nested
@@ -54,7 +54,7 @@ class MessageContainer(
   }
 
   /** This function forces the contained message and returns it */
-  def contained(): Message = {
+  def contained: Message = {
     if (myContained == null)
       myContained = msgFn
 

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

@@ -28,7 +28,7 @@ import collection.mutable
 import config.Printers.{overload, typr, unapp}
 import TypeApplications._
 
-import reporting.diagnostic.Message
+import reporting.diagnostic.{Message, messages}
 import reporting.trace
 import Constants.{Constant, IntTag, LongTag}
 import dotty.tools.dotc.reporting.diagnostic.messages.{UnapplyInvalidReturnType, NotAnExtractor, UnapplyInvalidNumberOfArguments}
@@ -1058,12 +1058,29 @@ trait Applications extends Compatibility {
     record("typedUnApply")
     val Apply(qual, args) = tree
 
-    def notAnExtractor(tree: Tree) =
+    def notAnExtractor(tree: Tree): Tree =
       // prefer inner errors
       // e.g. report not found ident instead of not an extractor in tests/neg/i2950.scala
       if (!tree.tpe.isError && tree.tpe.isErroneous) tree
       else errorTree(tree, NotAnExtractor(qual))
 
+    /** Report errors buffered in state.
+     *  @pre state has errors to report
+     *  If there is a single error stating that "unapply" is not a member, print
+     *  the more informative "notAnExtractor" message instead.
+     */
+    def reportErrors(tree: Tree, state: TyperState): Tree =
+      assert(state.reporter.hasErrors)
+      val msgs = state.reporter.removeBufferedMessages
+      msgs match
+        case msg :: Nil =>
+          msg.contained match
+            case messages.NotAMember(_, nme.unapply, _, _) => return notAnExtractor(tree)
+            case _ =>
+        case _ =>
+      msgs.foreach(ctx.reporter.report)
+      tree
+
     /** If this is a term ref tree, try to typecheck with its type name.
      *  If this refers to a type alias, follow the alias, and if
      *  one finds a class, reference the class companion module.
@@ -1097,42 +1114,52 @@ trait Applications extends Compatibility {
      *  overloaded unapply does *not* need to be applicable to its argument
      *  whereas overloaded variants need to have a conforming variant.
      */
-    def trySelectUnapply(qual: untpd.Tree)(fallBack: Tree => Tree): Tree = {
+    def trySelectUnapply(qual: untpd.Tree)(fallBack: (Tree, TyperState) => Tree): Tree = {
       // try first for non-overloaded, then for overloaded ocurrences
-      def tryWithName(name: TermName)(fallBack: Tree => Tree)(implicit ctx: Context): Tree = {
+      def tryWithName(name: TermName)(fallBack: (Tree, TyperState) => Tree)(implicit ctx: Context): Tree = {
         def tryWithProto(pt: Type)(implicit ctx: Context) = {
           val result = typedExpr(untpd.Select(qual, name), new UnapplyFunProto(pt, this))
-          if (!result.symbol.exists || result.symbol.name == name) result
+          if !result.symbol.exists
+             || result.symbol.name == name
+             || ctx.reporter.hasErrors
+          then result
           else notAnExtractor(result)
           	// It might be that the result of typedExpr is an `apply` selection or implicit conversion.
           	// Reject in this case.
         }
         tryEither {
           tryWithProto(selType)
         } {
-          (sel, _) =>
+          (sel, state) =>
             tryEither {
               tryWithProto(WildcardType)
             } {
-              (_, _) => fallBack(sel)
+              (_, _) => fallBack(sel, state)
             }
         }
       }
 
       // try first for unapply, then for unapplySeq
       tryWithName(nme.unapply) {
-        sel => tryWithName(nme.unapplySeq)(_ => fallBack(sel)) // for backwards compatibility; will be dropped
+        (sel, state) =>
+          tryWithName(nme.unapplySeq) {
+            (_, _) => fallBack(sel, state)
+          }
       }
     }
 
     /** Produce a typed qual.unapply or qual.unapplySeq tree, or
      *  else if this fails follow a type alias and try again.
      */
-    var unapplyFn = trySelectUnapply(qual) { sel =>
-      val qual1 = followTypeAlias(qual)
-      if (qual1.isEmpty) notAnExtractor(sel)
-      else trySelectUnapply(qual1)(_ => notAnExtractor(sel))
-    }
+    var unapplyFn =
+      trySelectUnapply(qual) {
+        (sel, state) =>
+          val qual1 = followTypeAlias(qual)
+          if (qual1.isEmpty) reportErrors(sel, state)
+          else trySelectUnapply(qual1) {
+            (_, state) => reportErrors(sel, state)
+          }
+      }
 
     /** Add a `Bind` node for each `bound` symbol in a type application `unapp` */
     def addBinders(unapp: Tree, bound: List[Symbol]) = unapp match {

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

@@ -4,7 +4,7 @@ package typer
 
 import core._
 import Contexts._, Types._, Symbols._, Names._, Decorators._, ProtoTypes._
-import Flags._
+import Flags._, SymDenotations._
 import NameKinds.FlatName
 import config.Printers.implicits
 import util.Spans.Span
@@ -82,8 +82,11 @@ trait ImportSuggestions:
               || refSym == defn.JavaLangPackageClass // ... or java.lang.
           then Nil
           else refSym.info.decls.filter(lookInside)
+        else if refSym.infoOrCompleter.isInstanceOf[StubInfo] then
+          Nil // Don't chase roots that do not exist
         else
-          if !refSym.is(Touched) then refSym.ensureCompleted() // JavaDefined is reliably known only after completion
+          if !refSym.is(Touched) then
+            refSym.ensureCompleted() // JavaDefined is reliably known only after completion
           if refSym.is(JavaDefined) then Nil
           else nestedRoots(site)
       nested

compiler/src/dotty/tools/repl/ReplDriver.scala

@@ -381,7 +381,7 @@ class ReplDriver(settings: Array[String],
 
   /** Render messages using the `MessageRendering` trait */
   private def renderMessage(cont: MessageContainer): Context => String =
-    messageRenderer.messageAndPos(cont.contained(), cont.pos, messageRenderer.diagnosticLevel(cont))(_)
+    messageRenderer.messageAndPos(cont.contained, cont.pos, messageRenderer.diagnosticLevel(cont))(_)
 
   /** Output errors to `out` */
   private def displayErrors(errs: Seq[MessageContainer])(implicit state: State): State = {

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

@@ -39,7 +39,7 @@ trait ErrorMessagesTest extends DottyTest {
     if (!runCtx.reporter.hasErrors) new EmptyReport
     else {
       val rep = runCtx.reporter.asInstanceOf[StoreReporter]
-      val msgs = rep.removeBufferedMessages(runCtx).map(_.contained()).reverse
+      val msgs = rep.removeBufferedMessages(runCtx).map(_.contained).reverse
       new Report(msgs, runCtx)
     }
   }

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

@@ -54,7 +54,7 @@ extends Reporter with UniqueMessagePositions with HideNonSensicalMessages with M
 
   /** Prints the message with the given position indication. */
   def printMessageAndPos(m: MessageContainer, extra: String)(implicit ctx: Context): Unit = {
-    val msg = messageAndPos(m.contained(), m.pos, diagnosticLevel(m))
+    val msg = messageAndPos(m.contained, m.pos, diagnosticLevel(m))
     val extraInfo = inlineInfo(m.pos)
 
     if (m.level >= logLevel) {
@@ -69,7 +69,7 @@ extends Reporter with UniqueMessagePositions with HideNonSensicalMessages with M
   override def doReport(m: MessageContainer)(implicit ctx: Context): Unit = {
 
     // Here we add extra information that we should know about the error message
-    val extra = m.contained() match {
+    val extra = m.contained match {
       case pm: PatternMatchExhaustivity => s": ${pm.uncovered}"
       case _ => ""
     }
@@ -127,7 +127,7 @@ object TestReporter {
       /** Prints the message with the given position indication in a simplified manner */
       override def printMessageAndPos(m: MessageContainer, extra: String)(implicit ctx: Context): Unit = {
         def report() = {
-          val msg = s"${m.pos.line + 1}: " + m.contained().kind + extra
+          val msg = s"${m.pos.line + 1}: " + m.contained.kind + extra
           val extraInfo = inlineInfo(m.pos)
 
           writer.println(msg)

doc-tool/test/dotty/tools/dottydoc/DottyDocTest.scala

@@ -54,7 +54,7 @@ trait DottyDocTest extends MessageRendering {
             System.err.println("reporter had errors:")
             ctx.reporter.removeBufferedMessages.foreach { msg =>
               System.err.println {
-                messageAndPos(msg.contained(), msg.pos, diagnosticLevel(msg))
+                messageAndPos(msg.contained, msg.pos, diagnosticLevel(msg))
               }
             }
           }

language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala

@@ -709,7 +709,7 @@ object DottyLanguageServer {
         }
       }
 
-      val message = mc.contained()
+      val message = mc.contained
       if (displayMessage(message, mc.pos.source)) {
         val code = message.errorId.errorNumber.toString
         range(mc.pos).map(r =>

tests/neg/bad-unapplies.check

@@ -0,0 +1,39 @@
+-- [E051] Reference Error: tests/neg/bad-unapplies.scala:22:9 ----------------------------------------------------------
+22 |    case A("2") => // error (cannot resolve overloading)
+   |         ^
+   |         Ambiguous overload. The overloaded alternatives of method unapply in object A with types
+   |          (x: B): Option[String]
+   |          (x: A): Option[String]
+   |         both match arguments (C)
+
+longer explanation available when compiling with `-explain`
+-- [E127] Syntax Error: tests/neg/bad-unapplies.scala:23:9 -------------------------------------------------------------
+23 |    case B("2") => // error (cannot be used as an extractor)
+   |         ^
+   |         B cannot be used as an extractor in a pattern because it lacks an unapply or unapplySeq method
+
+longer explanation available when compiling with `-explain`
+-- [E127] Syntax Error: tests/neg/bad-unapplies.scala:24:9 -------------------------------------------------------------
+24 |    case D("2") => // error (cannot be used as an extractor)
+   |         ^
+   |         D cannot be used as an extractor in a pattern because it lacks an unapply or unapplySeq method
+
+longer explanation available when compiling with `-explain`
+-- [E050] Reference Error: tests/neg/bad-unapplies.scala:25:9 ----------------------------------------------------------
+25 |    case E("2") => // error (value unapply in object E does not take parameters)
+   |         ^
+   |         value unapply in object E does not take parameters
+
+longer explanation available when compiling with `-explain`
+-- [E107] Syntax Error: tests/neg/bad-unapplies.scala:26:10 ------------------------------------------------------------
+26 |    case F("2") => // error (Wrong number of argument patterns for F; expected: ())
+   |         ^^^^^^
+   |         Wrong number of argument patterns for F; expected: ()
+
+longer explanation available when compiling with `-explain`
+-- [E006] Unbound Identifier Error: tests/neg/bad-unapplies.scala:27:9 -------------------------------------------------
+27 |    case G("2") => // error (Not found: G)
+   |         ^
+   |         Not found: G
+
+longer explanation available when compiling with `-explain`

tests/neg/bad-unapplies.scala

@@ -0,0 +1,28 @@
+trait A
+trait B
+class C extends A, B
+object A:
+  def unapply(x: A): Option[String] = Some(x.toString)
+  def unapply(x: B): Option[String] = Some(x.toString)
+
+object B
+
+object D:
+  def unapply(x: A, y: B): Option[String] = Some(x.toString)
+
+object E:
+  val unapply: Option[String] = Some("")
+
+object F:
+  def unapply(x: Int): Boolean = true
+
+
+@main def Test =
+  C() match
+    case A("2") => // error (cannot resolve overloading)
+    case B("2") => // error (cannot be used as an extractor)
+    case D("2") => // error (cannot be used as an extractor)
+    case E("2") => // error (value unapply in object E does not take parameters)
+    case F("2") => // error (Wrong number of argument patterns for F; expected: ())
+    case G("2") => // error (Not found: G)
+end Test

tests/neg/i5101.check

@@ -1,6 +1,6 @@
--- [E006] Unbound Identifier Error: tests/neg/i5101.scala:11:13 --------------------------------------------------------
+-- [E006] Unbound Identifier Error: tests/neg/i5101.scala:11:11 --------------------------------------------------------
 11 |      case A0(_) =>     // error
-   |           ^^^^^
+   |           ^^
    |           Not found: A0
 
 longer explanation available when compiling with `-explain`