Fix #4225: always do reachability check by liufengyun · Pull Request #4253 · scala/scala3

liufengyun · 2018-04-05T13:54:05Z

always do reachability check
handle null in patterns

Note that we only handle null in reachability check, while ignore null in exhaustivity check. Otherwise, the following pattern match will report a warning about null, which is annoying:

    (Some(3): Option[Int]) match {
       case Some(x) =>
       case None    =>
    }

smarter · 2018-04-05T14:28:55Z

compiler/src/dotty/tools/dotc/printing/PlainPrinter.scala

            Text()

        nodeName ~ "(" ~ elems ~ tpSuffix ~ ")" ~ (Str(node.pos.toString) provided ctx.settings.YprintPos.value)
-      case _ =>


Tree extends Positioned so we don't even need a pattern match at all here.

smarter · 2018-04-05T14:29:29Z

compiler/src/dotty/tools/dotc/transform/Splicer.scala

         """.stripMargin
        ctx.error(msg, pos)
        None
-      case ex: Throwable => throw ex


This looks wrong to me, a try/catch can always catch a Throwable.

Yes, we could keep it, and no warning will be reported. I removed it because the compiler will generate a default case anyway.

smarter · 2018-04-05T14:35:39Z

compiler/src/dotty/tools/dotc/transform/patmat/Space.scala

        Typ(ConstantType(c), false)
    case _: BackquotedIdent => Typ(pat.tpe, false)
+    case Ident(nme.WILDCARD) =>
+      Or(Typ(pat.tpe.stripAnnots, false) :: Typ(ConstantType(Constant(null))) :: Nil)


Would be cleaner to have val nullType = ConstantType(Constant(null)) somewhere.

smarter · 2018-04-05T14:40:29Z

tests/patmat/i4225b.scala

+  def test(x: String) =
+    x match {
+      case Bar(a) => a
+      case _ => x // this case is reachable, i.e. test(null)


Suggestion; maybe if the only way to reach a case is through null, we should suggest the user writes case null instead of case _ ?

This is a good idea, it's implemented in the latest commit.

smarter · 2018-04-05T17:43:09Z

compiler/src/dotty/tools/dotc/transform/patmat/Space.scala

  /** Is `tp1` a subtype of `tp2`?  */
  def isSubType(tp1: Type, tp2: Type): Boolean = {
-    val res = tp1 <:< tp2 && (tp1 != ConstantType(Constant(null)) || tp2 == ConstantType(Constant(null)))
+    val res = tp1 <:< tp2 && (tp1 != nullType || tp2 == nullType)


the nullType checks could be moved before the subtype checks since they're cheaper.

smarter · 2018-04-05T17:53:44Z

compiler/src/dotty/tools/dotc/transform/patmat/Space.scala

+        // if last case is `_` and only matches `null`, produce a warning
+        if (i == cases.length - 1) {
+          simplify(minus(covered, prevs)) match {
+            case Typ(ConstantType(Constant(null)), _) =>


You can use `nullType` here

liufengyun · 2018-04-05T19:12:10Z

test performance please

dottybot · 2018-04-05T19:12:36Z

performance test scheduled: 2 job(s) in queue, 1 running.

allanrenucci · 2018-04-05T20:28:29Z

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

  case class MatchCaseUnreachable()(implicit ctx: Context)
  extends Message(MatchCaseUnreachableID) {
-    val kind = s"""Match ${hl"case"} Unreachable"""
+    val kind = s"""Match case Unreachable"""


interpolator not needed

allanrenucci · 2018-04-05T20:28:38Z

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


+  case class MatchCaseOnlyNullWarning()(implicit ctx: Context)
+  extends Message(MatchCaseOnlyNullWarningID) {
+    val kind = s"""Only null matched"""


interpolator not needed

allanrenucci · 2018-04-05T20:28:48Z

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

+  case class MatchCaseOnlyNullWarning()(implicit ctx: Context)
+  extends Message(MatchCaseOnlyNullWarningID) {
+    val kind = s"""Only null matched"""
+    val msg = s"Only ${hl"null"} is matched. Consider use `case null =>` instead."


s/use/using

dottybot · 2018-04-05T22:17:58Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/4253/ to see the changes.

Benchmarks is based on merging with master (b1d9162)

Note that we only handle null in reachability check, while ignore null in exhaustivity check. Otherwise, the following pattern match will report a warning about `null`, which is annoying: (Some(3): Option[Int]) match { case Some(x) => case None => }

liufengyun · 2018-04-06T11:08:35Z

The performance regression on exhaustivity S and exhaustivity T is incurred by reporting a friendly warning about replacing case _ with case null, which requires subtraction of spaces (minus more expensive than isSubSpace).

exhaustivity V doesn't suffer from the problem, as the subtraction finishes with Empty quickly, as there are no overlapping between the cases.

liufengyun requested a review from smarter April 5, 2018 14:17

smarter requested changes Apr 5, 2018

View reviewed changes

smarter approved these changes Apr 5, 2018

View reviewed changes

allanrenucci reviewed Apr 5, 2018

View reviewed changes

nicolasstucki assigned liufengyun Apr 6, 2018

liufengyun added 11 commits April 6, 2018 09:41

Fix scala#4225: always check redundant cases except for @unchecked

d6bf4e4

one null warning

7b4db0e

handle null in reachability check

16cb505

Note that we only handle null in reachability check, while ignore null in exhaustivity check. Otherwise, the following pattern match will report a warning about `null`, which is annoying: (Some(3): Option[Int]) match { case Some(x) => case None => }

code cleanup

199a5e4

address review

c397a5b

don't warn for null

e577520

use nullType for simplicity

f9a56a2

optimize isSubType by checking nullType first

5dccc29

fix CI

ffa3ed7

address review

8bf55fa

fix CI: unreachable warning make isInstanceOf test fail

f679455

liufengyun force-pushed the fix-4225 branch from 0cd676f to f679455 Compare April 6, 2018 07:50

liufengyun merged commit 2f94916 into scala:master Apr 6, 2018

liufengyun deleted the fix-4225 branch April 6, 2018 11:11

allanrenucci mentioned this pull request Apr 14, 2019

Benchmark regressions #6300

Closed

liufengyun mentioned this pull request May 19, 2021

Fix #12530: Wildcard of value types don't cover null #12533

Merged

This was referenced Aug 11, 2021

Unions of Singletons unreachable literals not being checked in pattern match. #12805

Closed

Restore unreachable warnings by converting literals #13290

Merged

Conversation

liufengyun commented Apr 5, 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!

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!

liufengyun commented Apr 5, 2018

Uh oh!

dottybot commented Apr 5, 2018

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!

dottybot commented Apr 5, 2018

Uh oh!

liufengyun commented Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

liufengyun commented Apr 5, 2018 •

edited

Loading

liufengyun commented Apr 6, 2018 •

edited

Loading