Fix #4661: Missing unreachable case warnings #4869

abeln · 2018-07-30T13:20:11Z

No description provided.

abeln · 2018-07-30T13:31:24Z

@liufengyun this is the fix we discussed in #4661 but it introduces a failure in https://github.com/lampepfl/dotty/blob/ba2eafcf9e7f4d166fe3721d81506a0ab0fb3adf/tests/patmat/file.scala

The failure is that we now issue a warning that only null is matched for the last case:

-- [E120] Only null matched Warning: tests/patmat/file.scala:13:9 --------------
13 |    case _ =>
   |         ^
   |         Only null is matched. Consider using `case null =>` instead.

However, looking at the code it's clear that the wildcard case matches more than null (to start with, AbstractFile is not sealed).

If we look at that last wildcard case, we can print what's covered before and what's covered by the wildcard:

prev: _: ZipArchive.Entry, _: PlainFile
covered: Or(List(Typ(TypeRef(ThisType(TypeRef(NoPrefix,module class <empty>)),class AbstractFile),false), Typ(ConstantType(Constant(null)),true)))

which seems correct. However, the problem appears when we substract what's already covered from what the _ adds:

minus: a = Or(List(Typ(TypeRef(ThisType(TypeRef(NoPrefix,module class <empty>)),class AbstractFile),false), Typ(ConstantType(Constant(null)),true))) b = Or(List(Typ(TypeRef(TypeRef(ThisType(TypeRef(NoPrefix,module class <empty>)),class ZipArchive),class Entry),true), Typ(TypeRef(ThisType(TypeRef(NoPrefix,module class <empty>)),class PlainFile),true)))

minus: a = Or(List(Typ(TypeRef(ThisType(TypeRef(NoPrefix,module class <empty>)),class AbstractFile),false), Typ(ConstantType(Constant(null)),true))) b = Typ(TypeRef(TypeRef(ThisType(TypeRef(NoPrefix,module class <empty>)),class ZipArchive),class Entry),true)

minus: a = Typ(TypeRef(ThisType(TypeRef(NoPrefix,module class <empty>)),class AbstractFile),false) b = Typ(TypeRef(TypeRef(ThisType(TypeRef(NoPrefix,module class <empty>)),class ZipArchive),class Entry),true)

We can see that the Ors are unrolled from both sides, until we're subtracting Typ(TypeRef(TypeRef(ThisType(TypeRef(NoPrefix,module class <empty>)),class ZipArchive),class Entry) from Typ(TypeRef(ThisType(TypeRef(NoPrefix,module class <empty>)),class AbstractFile),false).

The result is empty, which is incorrect.

In particular, when we try to decompose the ZipArchive#Entry (because it's sealed), we get

candidates for ZipArchive#Entry : [class DirEntry]
class DirEntry instantiated ------> WildcardType(NoType)

The first line seems correct, but I don't understand the second line, and I think it's incorrect and caused by a bug in refine/instantiate. However, I don't understand well enough either of those functions to debug the issue.

In summary, I think this change uncovered an already-existing bug. Thoughts?

abeln · 2018-07-30T13:33:34Z

I'd propose that I patch the failing test and file a separate bug to investigate instantiate/refine. I'd be happy to look into it with some guidance.

allanrenucci · 2018-07-30T14:24:53Z

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

@@ -294,16 +294,21 @@ class SpaceEngine(implicit ctx: Context) extends SpaceLogic {

  override def intersectUnrelatedAtomicTypes(tp1: Type, tp2: Type) = {
    val and = AndType(tp1, tp2)


This could be moved to the else branch

Blaisorblade · 2018-07-30T17:11:09Z

My 2 cents: even if this uncovers an existing bug (and I agree with you that it seems to be the case), that bug seems serious enough (or at least, worse than the one fixed here) that we might want to block this fix, at least until we understand what is going on. After all, the failing test does not look that esoteric.

EDIT: after looking at the code and at #3939, it seems that it is meant to return WildcardType under some conditions (which I don't get yet).

EDIT2: this PR looks unobjectionable itself, but I'd still prefer to wait for a clue on how to fix the underlying issue.

liufengyun · 2018-07-30T21:09:42Z

@abeln I confirm that there is a problem with the handling of ZipArchive#Entry in tests/patmat/file.scala. I think as long as we open an issue for tests/patmat/file.scala, it's fine to disable the test in this PR.

Thanks a lot.

abeln · 2018-07-31T12:31:27Z

Filed #4880 for the related bug.

abeln · 2018-07-31T12:33:40Z

@liufengyun disabled the problematic part of the failing test.

abeln · 2018-07-31T13:00:00Z

There are a bunch of warnings (that I guess are converted into errors) from TastyImpl.scala (https://github.com/lampepfl/dotty/blob/e72b80b85d2f939cb24707058fbb0c6ce657398f/compiler/src/dotty/tools/dotc/tastyreflect/TastyImpl.scala#L64)

For example

  type Id = untpd.Ident
...
  object Id extends IdExtractor {
    def unapply(x: Id): Option[String] = x match {
      case x: untpd.Ident => Some(x.name.toString) // TODO how to make sure it is not a Ident or TypeIdent? Check x.tpe?
      case _ => None
    }
}

Why is the code written in this style (i.e. the type alias and then the pattern matching)? What's the goal of the wildcard case? Is it really to match null? Looping in @nicolasstucki who I think wrote that code.

allanrenucci · 2018-07-31T13:25:20Z

Why is the code written in this style (i.e. the type alias and then the pattern matching)? What's the goal of the wildcard case? Is it really to match null? Looping in @nicolasstucki who I think wrote that code.

This seems to be for historical reasons. You can update the code. E.g.

def unapply(x: Id) = Some(x.name.toString)

nicolasstucki · 2018-07-31T13:54:32Z

You can also remove the comment.

abeln · 2018-08-03T09:14:39Z

@liufengyun
I fixed all the warnings in TastyImpl, but the one in ExtractDependencies line 132 is a "real life" version of the test I previously disabled: file.scala: https://github.com/lampepfl/dotty/blob/36c2e34bc33a39a41ec383e997185613bf88c64c/compiler/src/dotty/tools/dotc/sbt/ExtractDependencies.scala#L132

I re-enabled the test.

So I think this PR is blocked by #4880

nicolasstucki · 2018-08-03T11:52:55Z

The changes in TastyImpl.scala look good.

liufengyun · 2018-08-05T18:48:38Z

@abeln can you try rebase against #4897 ?

nicolasstucki · 2018-08-07T13:37:23Z

@abeln #4904 will change the code in TastyImpl.scala a large part of the changes you did in that file might not be needed.

abeln · 2018-08-09T16:00:00Z

@liufengyun after rebasing over #4897 both the failing test and the bootstrapped compilation now succeed. So I'll wait for #4897 to get merged before updating the PR.

Blaisorblade · 2018-08-13T18:27:28Z

@abeln Just merged #4897, and #4904 was merged few days ago.

Might be good to also test this keeps working when types are defined under a prefix, for instance

trait Prefix {
  trait Foo
  class One extends Foo
  class Two extends Foo
  class Three extends Foo
} 
class Test(p: Prefix) {
  import p._
  def test(f: Foo) = f match {
    case f: Foo =>
    case f: One =>
    case f: Two =>
    case f: Three =>
  }
}

Matching against Prefix#Foo and/or Prefix#One (and combinations thereof) is also interesting, since that stresses out the subtle logic in #4897.

abeln · 2018-08-13T20:46:58Z

@Blaisorblade Just updated the PR. Things still seem to work. I also expanded the new unit test with your prefix cases. Thanks!

abeln · 2018-08-13T20:52:16Z

tests/patmat/i4880a.check

@@ -0,0 +1 @@
+14: Match case Unreachable


This is new, and related to #4880, but seems correct: https://github.com/lampepfl/dotty/blob/master/tests/patmat/i4880a.scala#L14

abeln · 2018-08-13T20:53:26Z

tests/patmat/t4661.scala

@@ -0,0 +1,61 @@
+trait Foo


@Blaisorblade I can break this up into smaller tests, if you want. Not sure want the convention is.

liufengyun

Otherwise, LGTM

liufengyun · 2018-08-13T21:06:17Z

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

+    // Precondition: !isSubType(tp1, tp2) && !isSubType(tp2, tp1)
+    if (tp1 == nullType || tp2 == nullType) {
+      // Since projections of types don't include null, intersection with null is empty.
+      Empty


A minor comment: it seems more readable if we just return Empty for the exceptional case, fewer braces and indentations.

abeln · 2018-08-13T21:45:26Z

@liufengyun you're right. That's better. Done.

liufengyun · 2018-08-14T05:05:57Z

LGTM, thanks a lot @abeln !

allanrenucci reviewed Jul 30, 2018

View reviewed changes

abeln force-pushed the unreachable-case branch from b084eb1 to 8388d48 Compare July 30, 2018 16:46

Blaisorblade requested a review from liufengyun July 30, 2018 17:23

abeln mentioned this pull request Jul 31, 2018

Incorrect Space difference in pattern matching exhaustivity #4880

Closed

abeln force-pushed the unreachable-case branch from 8388d48 to ff44c5f Compare July 31, 2018 12:32

abeln force-pushed the unreachable-case branch 2 times, most recently from 05b667c to 278ce9c Compare August 3, 2018 09:12

liufengyun mentioned this pull request Aug 13, 2018

Fix #4880: always instantiate prefix tvar #4897

Merged

abeln force-pushed the unreachable-case branch from 278ce9c to 1f836e5 Compare August 13, 2018 20:45

abeln commented Aug 13, 2018

View reviewed changes

liufengyun approved these changes Aug 13, 2018

View reviewed changes

Fix scala#4661: Missing unreachable case warnings

a1be0cd

abeln force-pushed the unreachable-case branch from 1f836e5 to a1be0cd Compare August 13, 2018 21:44

liufengyun merged commit 25b15bf into scala:master Aug 14, 2018

liufengyun mentioned this pull request Aug 14, 2018

Unreachable case warning could point to previous matching case #4660

Closed

abeln deleted the unreachable-case branch August 15, 2018 15:52

		@@ -294,16 +294,21 @@ class SpaceEngine(implicit ctx: Context) extends SpaceLogic {

		override def intersectUnrelatedAtomicTypes(tp1: Type, tp2: Type) = {
		val and = AndType(tp1, tp2)

Fix #4661: Missing unreachable case warnings #4869

Fix #4661: Missing unreachable case warnings #4869

Uh oh!

Conversation

abeln commented Jul 30, 2018

Uh oh!

abeln commented Jul 30, 2018

Uh oh!

abeln commented Jul 30, 2018

Uh oh!

allanrenucci Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

abeln Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

Blaisorblade commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liufengyun commented Jul 30, 2018

Uh oh!

abeln commented Jul 31, 2018

Uh oh!

abeln commented Jul 31, 2018

Uh oh!

abeln commented Jul 31, 2018

Uh oh!

allanrenucci commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolasstucki commented Jul 31, 2018

Uh oh!

abeln commented Aug 3, 2018

Uh oh!

nicolasstucki commented Aug 3, 2018

Uh oh!

liufengyun commented Aug 5, 2018

Uh oh!

nicolasstucki commented Aug 7, 2018

Uh oh!

abeln commented Aug 9, 2018

Uh oh!

Blaisorblade commented Aug 13, 2018

Uh oh!

abeln commented Aug 13, 2018

Uh oh!

abeln Aug 13, 2018

Choose a reason for hiding this comment

Uh oh!

abeln Aug 13, 2018

Choose a reason for hiding this comment

Uh oh!

liufengyun left a comment

Choose a reason for hiding this comment

Uh oh!

liufengyun Aug 13, 2018

Choose a reason for hiding this comment

Uh oh!

abeln commented Aug 13, 2018

Uh oh!

liufengyun commented Aug 14, 2018

Uh oh!

Uh oh!

Blaisorblade commented Jul 30, 2018 •

edited

Loading

allanrenucci commented Jul 31, 2018 •

edited

Loading