Merge pull request #1091 from paulp/topic/patmat-error-messages

Better pattern matcher error message.

1 files changed, 70 insertions, 10 deletions

diff --git a/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala b/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala
index 1b502025c2..08c78706fe 100644
--- a/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala
@@ -198,6 +198,69 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL
     import typer.{typed, context, silent, reallyExists}
     // import typer.infer.containsUnchecked
 
+    // Why is it so difficult to say "here's a name and a context, give me any
+    // matching symbol in scope" ? I am sure this code is wrong, but attempts to
+    // use the scopes of the contexts in the enclosing context chain discover
+    // nothing. How to associate a name with a symbol would would be a wonderful
+    // linkage for which to establish a canonical acquisition mechanism.
+    def matchingSymbolInScope(pat: Tree): Symbol = {
+      def declarationOfName(tpe: Type, name: Name): Symbol = tpe match {
+        case PolyType(tparams, restpe)  => tparams find (_.name == name) getOrElse declarationOfName(restpe, name)
+        case MethodType(params, restpe) => params find (_.name == name) getOrElse declarationOfName(restpe, name)
+        case ClassInfoType(_, _, clazz) => clazz.rawInfo member name
+        case _                          => NoSymbol
+      }
+      pat match {
+        case Bind(name, _) =>
+          context.enclosingContextChain.foldLeft(NoSymbol: Symbol)((res, ctx) =>
+            res orElse declarationOfName(ctx.owner.rawInfo, name))
+        case _ => NoSymbol
+      }
+    }
+
+    // Issue better warnings than "unreachable code" when people mis-use
+    // variable patterns thinking they bind to existing identifiers.
+    //
+    // Possible TODO: more deeply nested variable patterns, like
+    //   case (a, b) => 1 ; case (c, d) => 2
+    // However this is a pain (at least the way I'm going about it)
+    // and I have to think these detailed errors are primarily useful
+    // for beginners, not people writing nested pattern matches.
+    def checkMatchVariablePatterns(m: Match) {
+      // A string describing the first variable pattern
+      var vpat: String = null
+      // Using an iterator so we can recognize the last case
+      val it = m.cases.iterator
+
+      def addendum(pat: Tree) = {
+        matchingSymbolInScope(pat) match {
+          case NoSymbol   => ""
+          case sym        =>
+            val desc = if (sym.isParameter) s"parameter ${sym.nameString} of" else sym + " in"
+            s"\nIf you intended to match against $desc ${sym.owner}, you must use backticks, like: case `${sym.nameString}` =>"
+        }
+      }
+
+      while (it.hasNext) {
+        val cdef = it.next
+        // If a default case has been seen, then every succeeding case is unreachable.
+        if (vpat != null)
+          context.unit./*error*/warning(cdef.body.pos, "unreachable code due to " + vpat + addendum(cdef.pat))
+        // If this is a default case and more cases follow, warn about this one so
+        // we have a reason to mention its pattern variable name and any corresponding
+        // symbol in scope.  Errors will follow from the remaining cases, at least
+        // once we make the above warning an error.
+        else if (it.hasNext && (treeInfo isDefaultCase cdef)) {
+          val vpatName = cdef.pat match {
+            case Bind(name, _)   => s" '$name'"
+            case _               => ""
+          }
+          vpat = s"variable pattern$vpatName on line ${cdef.pat.pos.line}"
+          context.unit.warning(cdef.pos, s"patterns after a variable pattern cannot match (SLS 8.1.1)" + addendum(cdef.pat))
+        }
+      }
+    }
+
     /** Implement a pattern match by turning its cases (including the implicit failure case)
       * into the corresponding (monadic) extractors, and combining them with the `orElse` combinator.
       *
@@ -210,6 +273,7 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL
       */
     def translateMatch(match_ : Match): Tree = {
       val Match(selector, cases) = match_
+      checkMatchVariablePatterns(match_)
 
       // we don't transform after uncurry
       // (that would require more sophistication when generating trees,
@@ -3358,16 +3422,12 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL
             case Some(cds) => cds
           }
 
-          val allReachable =
-            if (unchecked) true
-            else {
-              val unreachables = unreachableCase(caseDefsWithGuards)
-              unreachables foreach {cd => reportUnreachable(cd.body.pos)}
-              // a switch with duplicate cases yields a verify error,
-              // and a switch with duplicate cases and guards cannot soundly be rewritten to an unguarded switch
-              // (even though the verify error would disappear, the behaviour would change)
-              unreachables.isEmpty
-            }
+          val allReachable = unchecked || {
+            // a switch with duplicate cases yields a verify error,
+            // and a switch with duplicate cases and guards cannot soundly be rewritten to an unguarded switch
+            // (even though the verify error would disappear, the behaviour would change)
+            unreachableCase(caseDefsWithGuards) map (cd => reportUnreachable(cd.body.pos)) isEmpty
+          }
 
           if (!allReachable) Nil
           else if (noGuards(caseDefsWithGuards)) {