Closes #4300 adds better worded pattern match unreachability warning #4552

zoltanelek · 2018-05-21T16:27:05Z

Used when isInstanceOf is called inside a match case
The new message is "this case will always be taken because the scrutinee has type <type>"
Highlights the position of x: Int instead of x

…ning - Used when isInstanceOf is called inside a match case - The new message is "this case will always be taken because the scrutinee has type `<type>`" - Highlights the position of `x: Int` instead of `x`

dottybot

Hello, and thank you for opening this PR! 🎉

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

Have an awesome day! ☀️

liufengyun · 2018-05-22T07:46:36Z

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

+              if (inMatch)
+                ctx.warning(
+                  em"this case will always be taken because the scrutinee has type `$testType`",
+                  tree.pos)


This error message doesn't make sense for the following test case:

object Foo { def unapply(x: Int): Option[Int] = if (x % 2 == 0) Some(x) else None } class Test { def foo(a: Int) = a match { case Foo(x: Int) => x } }

To address both @zoltanelek and @liufengyun's concerns, I'd at least say "this pattern will always succeed", where "this" refers to x: Int (as shown by the position) — I'd prefer in fact "the highlighted pattern match".

I think both this message and the current one can still be a bit confusing because they might only refer to one pattern out of many nested ones, but maybe that can be clarified in the message explanation.

I'd also please keep showing both $foundCls and $testCls, since in the presence of type aliases they might be nontrivial to deduce from the scrutinee type and the pattern match.

Separately, "since $foundCls is a subclass of $testCls" says more than "because the scrutinee has type $testType": who wrote the code and fixes the warning might know the scrutinee's type, but what they probably missed is that $foundCls is a subclass of $testCls.

Blaisorblade

This must be revised to address the comments. @zoltanelek do you think you can give it a try?

zoltanelek · 2018-05-27T15:48:34Z

@Blaisorblade sure!

…atches on tuples

allanrenucci

It should probably be a proper message case class. See https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

zoltanelek · 2018-06-08T19:00:02Z

Hey guys! I went with
the type check in the highlighted part of the pattern match case statement will always succeed, since $foundCls is a subclass of $testCls

I think this is much more meaningful when matching on tuples.
E.g. here: "the highlighted pattern match will always succeed" would be actually false, only the first parameter, x is matched

|def foo(a: (Int,Int)) = a match { case (x: Int, y: String) => x }
|                                        ^^^^^^
|the type check in the highlighted part of the pattern match case statement will always succeed, since `class Int` is a subclass of `class Int`

And 'the type check in the highlighted part' would not imply that there can't be an unapply behind the scenes, which will fail to match.

scala> object Foo {
           def unapply(x: Int): Option[Int] = if (x % 2 == 0) Some(x) else None
         } 
// defined object Foo

scala> class Test {
           def foo(a: Int) = a match { case Foo(x: Int) => x }
         } 
2 |    def foo(a: Int) = a match { case Foo(x: Int) => x }
  |                                         ^^^^^^
  |the type check in the highlighted part of the pattern match case statement will always succeed, since `class Int` is a subclass of `class Int`
// defined class Test

liufengyun

LGTM, thanks a lot @zoltanelek 🎉

BTW, could you please squash the history to keep one commit?

allanrenucci

@liufengyun @Blaisorblade Why not something like:

The highlighted type test will always succeed since the scrutiny type (${expr.tpe}) is a subtype of ${testType}

This wording is applicable to both isIntanceOf test and type test in pattern match. One could go into more detail (if needed) in the explanation part of the message

allanrenucci · 2018-06-09T14:37:28Z

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

+              else
+                ctx.warning(
+                  em"this will always yield true, since `$foundCls` is a subclass of `$testCls`",
+                  expr.pos)


I would merge the two together by making inMatch a parameter of the message class and name the class something like TypeTestAlwaysSucceeds

allanrenucci · 2018-06-09T14:38:08Z

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

@@ -2117,4 +2117,38 @@ object messages {
    val explanation: String = ""
  }

+  case class PatternMatchAlwaysSucceeds(foundCls: Symbol, testCls: Symbol)(implicit ctx: Context) extends Message(PatternMatchAlwaysSucceedsID) {
+    val kind = "Syntax"
+    val msg: String =


type annotation not needed

allanrenucci · 2018-06-09T14:39:25Z

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

+      hl"""
+         |The type check in the highlighted part of the pattern match case statement will always succeed,
+         |since `$foundCls` is a subclass of `$testCls`.
+       """


Rewrite as:

hl"""... |..."""

Otherwise the resulting string has unnecessary line breaks before and after

allanrenucci · 2018-06-09T15:27:02Z

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

+         |since `$foundCls` is a subclass of `$testCls`.
+       """
+    val codeExampleValidWarning = "def foo(a: Int) = a match { case x: Int => x }"
+    val codeExampleDisregardWarning =


The user should not disregard the warning. The warning is legitimate, he should have written case Foo(x) instead of case Foo(x: Int)

allanrenucci · 2018-06-09T15:31:25Z

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

+        |  def foo(a: Int) = a match { case Foo(x: Int) => x }
+        |}
+      """
+    val explanation: String =


I would remove the current explanation. I don't think it gives you more information than the msg.

…d isInstanceOf

zoltanelek · 2018-06-10T08:24:31Z

@liufengyun sure, I'll squash them when everybody approves.

liufengyun · 2018-06-11T14:50:36Z

@Blaisorblade @allanrenucci I think it looks good now, do you have more comments?

allanrenucci · 2018-06-13T12:55:13Z

Thanks @zoltanelek!

zoltanelek · 2018-06-13T14:46:52Z

Cool! Thanks for the help guys!

Closes scala#4300 adds better worded pattern match unreachability war…

becafe4

…ning - Used when isInstanceOf is called inside a match case - The new message is "this case will always be taken because the scrutinee has type `<type>`" - Highlights the position of `x: Int` instead of `x`

dottybot reviewed May 21, 2018

View reviewed changes

nicolasstucki requested a review from liufengyun May 22, 2018 07:34

nicolasstucki assigned liufengyun May 22, 2018

liufengyun reviewed May 22, 2018

View reviewed changes

Blaisorblade suggested changes May 27, 2018

View reviewed changes

changed pattern match reachability warning, to reflect more complex m…

6834571

…atches on tuples

allanrenucci suggested changes Jun 8, 2018

View reviewed changes

clearer error message for pattern matching on objects

f55d497

Created a proper error message case class

215f0bf

liufengyun approved these changes Jun 9, 2018

View reviewed changes

allanrenucci suggested changes Jun 9, 2018

View reviewed changes

zoltanelek added 2 commits June 10, 2018 10:20

Common TypeTestAlwaysSucceeds warning case class for pattern match an…

bf7c85c

…d isInstanceOf

Fixed formatting

1370dd1

allanrenucci approved these changes Jun 13, 2018

View reviewed changes

allanrenucci merged commit c13422f into scala:master Jun 13, 2018

zoltanelek deleted the 4300_zoltanelek-better_worded_pattern_match_reachability_warning branch June 13, 2018 14:46

Closes #4300 adds better worded pattern match unreachability warning #4552

Closes #4300 adds better worded pattern match unreachability warning #4552

Uh oh!

Conversation

zoltanelek commented May 21, 2018

Uh oh!

dottybot left a comment

Choose a reason for hiding this comment

Uh oh!

liufengyun May 22, 2018

Choose a reason for hiding this comment

Uh oh!

Blaisorblade May 27, 2018

Choose a reason for hiding this comment

Uh oh!

Blaisorblade left a comment

Choose a reason for hiding this comment

Uh oh!

zoltanelek commented May 27, 2018

Uh oh!

allanrenucci left a comment

Choose a reason for hiding this comment

Uh oh!

zoltanelek commented Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liufengyun left a comment

Choose a reason for hiding this comment

Uh oh!

allanrenucci left a comment

Choose a reason for hiding this comment

Uh oh!

allanrenucci Jun 9, 2018

Choose a reason for hiding this comment

Uh oh!

allanrenucci Jun 9, 2018

Choose a reason for hiding this comment

Uh oh!

allanrenucci Jun 9, 2018

Choose a reason for hiding this comment

Uh oh!

allanrenucci Jun 9, 2018

Choose a reason for hiding this comment

Uh oh!

allanrenucci Jun 9, 2018

Choose a reason for hiding this comment

Uh oh!

zoltanelek commented Jun 10, 2018

Uh oh!

liufengyun commented Jun 11, 2018

Uh oh!

allanrenucci commented Jun 13, 2018

Uh oh!

zoltanelek commented Jun 13, 2018

Uh oh!

Uh oh!

zoltanelek commented Jun 8, 2018 •

edited

Loading