Fix for spurious warning.

Eliminates spurious "catch block may intercept non-local return"
seen in recent builds of master.  Unified some catch logic
in TreeInfo, and removed some which never worked.

6 files changed, 53 insertions, 31 deletions

diff --git a/src/compiler/scala/tools/nsc/transform/UnCurry.scala b/src/compiler/scala/tools/nsc/transform/UnCurry.scala
index f68cbfc141..ea93ad1bd4 100644
--- a/src/compiler/scala/tools/nsc/transform/UnCurry.scala
+++ b/src/compiler/scala/tools/nsc/transform/UnCurry.scala
@@ -205,11 +205,8 @@ abstract class UnCurry extends InfoTransform
         val keyDef   = ValDef(key, New(ObjectClass.tpe))
         val tryCatch = Try(body, pat -> rhs)
 
-        body foreach {
-          case Try(t, catches, _) if catches exists treeInfo.catchesThrowable =>
-            unit.warning(body.pos, "catch block may intercept non-local return from " + meth)
-          case _ =>
-        }
+        for (Try(t, catches, _) <- body ; cdef <- catches ; if treeInfo catchesThrowable cdef)
+          unit.warning(body.pos, "catch block may intercept non-local return from " + meth)
 
         Block(List(keyDef), tryCatch)
       }
@@ -691,16 +688,16 @@ abstract class UnCurry extends InfoTransform
         else
           tree
       }
-      
+
       def isThrowable(pat: Tree): Boolean = pat match {
-        case Typed(Ident(nme.WILDCARD), tpt) => 
+        case Typed(Ident(nme.WILDCARD), tpt) =>
           tpt.tpe =:= ThrowableClass.tpe
-        case Bind(_, pat) => 
+        case Bind(_, pat) =>
           isThrowable(pat)
         case _ =>
           false
       }
-      
+
       def isDefaultCatch(cdef: CaseDef) = isThrowable(cdef.pat) && cdef.guard.isEmpty
 
       def postTransformTry(tree: Try) = {
@@ -764,10 +761,10 @@ abstract class UnCurry extends InfoTransform
 
         case tree: Try =>
           postTransformTry(tree)
-          
+
         case Apply(Apply(fn, args), args1) =>
           treeCopy.Apply(tree, fn, args ::: args1)
-          
+
         case Ident(name) =>
           assert(name != tpnme.WILDCARD_STAR, tree)
           applyUnary()
diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
index cf9a07a7e4..9c6f6a0f99 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -5133,14 +5133,12 @@ trait Typers extends Modes with Adaptations with Tags {
         var block1 = typed(tree.block, pt)
         var catches1 = typedCases(tree.catches, ThrowableClass.tpe, pt)
 
-        for (cdef <- catches1 if cdef.guard.isEmpty) {
-          def warn(name: Name) = context.warning(cdef.pat.pos, s"This catches all Throwables. If this is really intended, use `case ${name.decoded} : Throwable` to clear this warning.")
-          def unbound(t: Tree) = t.symbol == null || t.symbol == NoSymbol
-          cdef.pat match {
-            case Bind(name, i @ Ident(_)) if unbound(i) => warn(name)
-            case i @ Ident(name) if unbound(i) => warn(name)
-            case _ =>
-          }
+        for (cdef <- catches1; if treeInfo catchesThrowable cdef) {
+          val name = (treeInfo assignedNameOfPattern cdef).decoded
+          context.warning(cdef.pat.pos,
+            s"""|This catches all Throwables, which often has undesirable consequences.
+                |If intentional, use `case $name : Throwable` to clear this warning.""".stripMargin
+          )
         }
 
         val finalizer1 =
diff --git a/src/reflect/scala/reflect/internal/TreeInfo.scala b/src/reflect/scala/reflect/internal/TreeInfo.scala
index db062d138f..d4a22886dd 100644
--- a/src/reflect/scala/reflect/internal/TreeInfo.scala
+++ b/src/reflect/scala/reflect/internal/TreeInfo.scala
@@ -397,19 +397,31 @@ abstract class TreeInfo {
     case _                          => false
   }
 
-  /** Does this CaseDef catch Throwable? */
-  def catchesThrowable(cdef: CaseDef) = catchesAllOf(cdef, ThrowableClass.tpe)
+  private def hasNoSymbol(t: Tree) = t.symbol == null || t.symbol == NoSymbol
 
-  /** Does this CaseDef catch everything of a certain Type? */
-  def catchesAllOf(cdef: CaseDef, threshold: Type) = {
-    def unbound(t: Tree) = t.symbol == null || t.symbol == NoSymbol
+  /** If this CaseDef assigns a name to its top-level pattern,
+   *  in the form 'expr @ pattern' or 'expr: pattern', returns
+   *  the name. Otherwise, nme.NO_NAME.
+   *
+   *  Note: in the case of Constant patterns such as 'case x @ "" =>',
+   *  the pattern matcher eliminates the binding and inlines the constant,
+   *  so as far as this method is likely to be able to determine,
+   *  the name is NO_NAME.
+   */
+  def assignedNameOfPattern(cdef: CaseDef): Name = cdef.pat match {
+    case Bind(name, _)  => name
+    case Ident(name)    => name
+    case _              => nme.NO_NAME
+  }
+
+  /** Does this CaseDef catch Throwable? */
+  def catchesThrowable(cdef: CaseDef) = (
     cdef.guard.isEmpty && (unbind(cdef.pat) match {
       case Ident(nme.WILDCARD)       => true
-      case i@Ident(name)             => unbound(i)
-      case Typed(_, tpt)             => (tpt.tpe != null) && (threshold <:< tpt.tpe)
+      case i@Ident(name)             => hasNoSymbol(i)
       case _                         => false
     })
-  }
+  )
 
   /** Is this pattern node a catch-all or type-test pattern? */
   def isCatchCase(cdef: CaseDef) = cdef match {
diff --git a/test/files/neg/catch-all.check b/test/files/neg/catch-all.check
index aaf51480c3..2d58dd99a8 100644
--- a/test/files/neg/catch-all.check
+++ b/test/files/neg/catch-all.check
@@ -1,10 +1,13 @@
-catch-all.scala:2: warning: This catches all Throwables. If this is really intended, use `case _ : Throwable` to clear this warning.
+catch-all.scala:2: warning: This catches all Throwables, which often has undesirable consequences.
+If intentional, use `case _ : Throwable` to clear this warning.
   try { "warn" } catch { case _ => }
                               ^
-catch-all.scala:4: warning: This catches all Throwables. If this is really intended, use `case x : Throwable` to clear this warning.
+catch-all.scala:4: warning: This catches all Throwables, which often has undesirable consequences.
+If intentional, use `case x : Throwable` to clear this warning.
   try { "warn" } catch { case x => }
                               ^
-catch-all.scala:6: warning: This catches all Throwables. If this is really intended, use `case x : Throwable` to clear this warning.
+catch-all.scala:6: warning: This catches all Throwables, which often has undesirable consequences.
+If intentional, use `case x : Throwable` to clear this warning.
   try { "warn" } catch { case _: RuntimeException => ; case x => }
                                                             ^
 error: No warnings can be incurred under -Xfatal-warnings.
diff --git a/test/files/neg/nonlocal-warning.check b/test/files/neg/nonlocal-warning.check
index 5202df655a..67b3b10095 100644
--- a/test/files/neg/nonlocal-warning.check
+++ b/test/files/neg/nonlocal-warning.check
@@ -1,4 +1,5 @@
-nonlocal-warning.scala:4: warning: This catches all Throwables. If this is really intended, use `case x : Throwable` to clear this warning.
+nonlocal-warning.scala:4: warning: This catches all Throwables, which often has undesirable consequences.
+If intentional, use `case x : Throwable` to clear this warning.
     catch { case x => 11 }
                  ^
 nonlocal-warning.scala:2: warning: catch block may intercept non-local return from method foo
diff --git a/test/files/neg/nonlocal-warning.scala b/test/files/neg/nonlocal-warning.scala
index cc98bd631a..f908a86302 100644
--- a/test/files/neg/nonlocal-warning.scala
+++ b/test/files/neg/nonlocal-warning.scala
@@ -4,4 +4,15 @@ class Foo {
     catch { case x => 11 }
     22
   }
+
+  val pf: PartialFunction[Throwable, Unit] = {
+    case x if false => ()
+  }
+
+  def bar(l: List[Int]): Int = {
+    try l foreach { _ => return 5 }
+    catch pf
+    finally println()
+    22
+  }
 }