Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jun 13, 2018
Merged

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

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
becafe4
Closes #4300 adds better worded pattern match unreachability warning
zoltanelek May 21, 2018
6834571
changed pattern match reachability warning, to reflect more complex m…
zoltanelek Jun 8, 2018
f55d497
clearer error message for pattern matching on objects
zoltanelek Jun 8, 2018
215f0bf
Created a proper error message case class
zoltanelek Jun 8, 2018
bf7c85c
Common TypeTestAlwaysSucceeds warning case class for pattern match an…
zoltanelek Jun 10, 2018
1370dd1
Fixed formatting
zoltanelek Jun 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions compiler/src/dotty/tools/dotc/transform/TypeTestsCasts.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,14 @@ object TypeTestsCasts {

if (expr.tpe <:< testType)
if (expr.tpe.isNotNull) {
ctx.warning(
em"this will always yield true, since `$foundCls` is a subclass of `$testCls`",
expr.pos)
if (inMatch)
ctx.warning(
em"this case will always be taken because the scrutinee has type `$testType`",
tree.pos)
Copy link
Contributor

@liufengyun liufengyun May 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }
}

Copy link
Contributor

@Blaisorblade Blaisorblade May 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.

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

@allanrenucci allanrenucci Jun 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

constant(expr, Literal(Constant(true)))
}
else expr.testNotNull
Expand Down