Skip to content
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.

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

Restore unreachable warnings by converting literals #13290

Merged
merged 7 commits into from
Aug 30, 2021

Conversation

dwijnand
Copy link
Member

@dwijnand dwijnand commented Aug 12, 2021

The fix implemented (in #4253) to deal with primitive types auto-converting was
suppressing a series of legitimate unreachable warnings. So we removed
that fix and tried to mirror the auto-conversion of the type, such
that the space intersection calculation ends up with the correct
results.

@dwijnand dwijnand force-pushed the unreachable-literals branch 2 times, most recently from c473af9 to 1c1788a Compare August 18, 2021 15:42
@SethTisue
Copy link
Member

SethTisue commented Aug 18, 2021

The fix implemented (in #4253) to deal with primitive types auto-converting [...]

namely:

        // `covered == Empty` may happen for primitive types with auto-conversion
         // see tests/patmat/reader.scala  tests/patmat/byte.scala
         if (covered == Empty && !isNullLit(pat)) covered = curr

we removed this and handled primitive types instead by inserting a call to convertConstantType elsewhere.

But as it turned out, the covered == Empty check above wasn't only preventing false positives for primitive types, it was also allowing some other kinds of code to compile, including a piece of code in TreeTypeMap.scala in the compiler itself, so our patch made the compiler fail to bootstrap.

In 1c1788a we boil that piece of code in TreeTypeMap down to a failing test case. (It could certainly be minimized even further, but for now we stopped at the point where the intent behind the code remains clear.)

One peculiar thing about the failure is that the reachability warning is emitted strangely late. The match in the test case reads:

... match {
   case Ident()  => 1
   case DefDef() => 2
   ...

the reachability warning is emitted on the DefDef case, but if DefDef is considered unreachable here, surely Ident ought to be considered unreachable as well.

I believe this should be considered a false positive. Evidence:

  • Scala 2 doesn't warn
  • at present, before our changes, Scala 3 doesn't warn either
  • the actual code in TreeTypeMap was written by somebody who apparently thought what they were writing made sense and shouldn't cause reachability warnings
  • it seems to me that the a reachability warning should never be issued unless the code is provably unreachable. but we know in this case that the value can be an Ident or DefDef at runtime

But Dale has some doubts about this that I'll leave it to him to present himself; he notes that code in question is using asInstanceOf as a loophole.

Leaving that aside for a moment, and assuming that we don't want the reachability warning, how would we proceed? I haven't worked out the details, but it seems intuitively right to me to widen an abstract type (such as ThisTree) to its upper bound (here, Tree) before proceeding with reachability analysis. In other words, use ThisTree's upper bound of Tree as sufficient evidence for considering any subtype of Tree (including Ident and DefDef) to be reachable here.

@dwijnand
Copy link
Member Author

Leaving that aside for a moment, and assuming that we don't want the reachability warning, how would we proceed? I haven't worked out the details, but it seems to me, intuitively, like it ought to be possible to use ThisTree's upper bound of Tree as sufficient evidence for considering any subtype of Tree (including Ident and DefDef) to be reachable here.

Perhaps that's the direction to go, if a good balance on the conditions to do that type widening are found. But I like how reachability is defined in the Scala 2 compiler:

    // computes the first 0-based case index that is unreachable (if any)
    // a case is unreachable if it implies its preceding cases
    // call C the formula that is satisfiable if the considered case matches
    // call P the formula that is satisfiable if the cases preceding it match
    // the case is reachable if there is a model for -P /\ C,
    // thus, the case is unreachable if there is no model for -(-P /\ C),
    // or, equivalently, P \/ -C, or C => P
    def unreachableCase(prevBinder: Symbol, cases: List[List[TreeMaker]], pt: Type): Option[Int] = {

Here the preceding case is P = case Ident and the considered case is C = case DefDef. There's no value of type ThisTree[Type] for which P \/ -C would be true (the ThisTree is Ident OR the ThisTree is not DefDef), therefore we don't declare case DefDef unreachable.


Going back to the "is the warning right?" question:

  • it seems to me that the a reachability warning should never be issued unless the code is provably unreachable. but we know in this case that the value can be an Ident or DefDef at runtime

But Dale has some doubts about this that I'll leave it to him to present himself; he notes that code in question is using asInstanceOf as a loophole.

The use of a non-class type for ThisTree and the cast allows for values of type ThisTree to actually be of some other subtype of Tree, such as DefDef or Ident. If we were to try and use a nested class ThisTree[-T >: Untyped] extends Tree[T] and a cast, then the cast would rightly fail at runtime. So, as said above, the reachability analysis might want to use this non-class type detail to widen, but I'm not sure if it doesn't ruin the experience for some other users that are pattern matching on some abstract types.

dwijnand and others added 6 commits August 26, 2021 16:31
The fix implemented to deal with primitive types auto-converting was
suppressing a series of legitimate unreachable warnings.  So we removed
that fix and tried to mirror the auto-conversion of the type, such that
the space intersection calculation ends up with the correct results.

In doing so we triggered reachability warnings being emitted by
sealed trait companion object (or enum) `ordinal` synthetic methods, in
some weird test cases.  So we added an `@unchecked` annotation to
suppress those.

Co-authored-by: Seth Tisue <seth@tisue.net>
In the event that the first case is unreachable, the logic descended to
`isSubspace(covered, prevs)` where both are the Empty Space, emitting
warnings.  Instead we implement reachability like the Scala 2 compiler
defines it:

    // a case is unreachable if it implies its preceding cases
@dwijnand
Copy link
Member Author

@liufengyun this makes a few slightly big changes, do you think you'd have time in the near future to review this, please? I'm happy to jump in a call if that would help.

@dwijnand
Copy link
Member Author

dwijnand commented Aug 29, 2021

This PR starts with unions of literals and missing reachable cases. When you have even smaller types, like singletons, the lack of reachability warnings is even more obvious, like

def t5 = Day.Mon match { case Day.Mon => 1 case Day.Tue => 2 case Day.Wed => 3 }

which is a test case in #13409, and hindered by the underlying bug fixed here.

Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

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

LGTM, nice improvement 👍

&& simplify(intersect(prevs, targetSpace)) != Empty
// it's required that at one of the previous cases is reachable (its intersected Space isn't Empty)
// because if all the previous cases are unreachable then case i can't be unreachable
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: maybe use if/then syntax instead of parenthesis 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

because if all the previous cases are unreachable then case i can't be unreachable

The case i can still be unreachable if it's impossible to match the scrutinee. I'm thinking if it's possible to avoid the computation to make code simpler and faster for common cases --- The intended performance improvement only holds for abnormal cases anyway.

Copy link
Member Author

@dwijnand dwijnand Aug 30, 2021

Choose a reason for hiding this comment

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

Oh, this isn't a performance improvement, this is correctness!

As explored with Seth there's two levels of unreachable: there's "this case can never be reached for this scrutinee" and there's "this case is never reached, because it's covered by something previous". We're only doing the second, but in order to do that properly we need to make sure that the previous, including the first(!), is actually possibly reached. Personally I think the first level should be covered by typing rather than here. But it's also possible, with aliases and casting, for passing code to throw warnings - so it's a bit of a judgement call.

Anyways, rethinking this code, I can make this cheaper by reusing and accumulating, so I avoid re-doing lots of intersecting and simplifying.

compiler/src/dotty/tools/dotc/transform/patmat/Space.scala Outdated Show resolved Hide resolved
def isSubType(tp1: Type, tp2: Type): Boolean = {
debug.println(TypeComparer.explained(_.isSubType(tp1, tp2)))
def isSubType(_tp1: Type, tp2: Type): Boolean = {
val tp1 = maybeBox(convertConstantType(_tp1, tp2), tp2)
Copy link
Contributor

@liufengyun liufengyun Aug 29, 2021

Choose a reason for hiding this comment

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

I am wondering if it's possible to write a helper method isSubtypeWithConversion or isSubtypeWithConversionAndBoxing:

def isSubtypeWithConversion(tp: Type, pt: Type): Boolean = ...

The protocol of the method will be simpler, the code in isSubType will be more clear and of the same performance as the code before for common cases:

tp1 <:< tp2 || isSubtypeWithConversion(tp1, tp2)

Copy link
Member Author

Choose a reason for hiding this comment

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

Where does that leave the explicitNulls code? And what does that code even do?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of keeping the code for explicitNulls as it's --- duplicate tp1 <:< tp2 || isSubtypeWithConversion(tp1, tp2) for that branch as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I might leave that for another rotation on the code, so I can get the other fixes in.

@dwijnand dwijnand merged commit 7d1cddb into scala:master Aug 30, 2021
@dwijnand dwijnand deleted the unreachable-literals branch August 30, 2021 20:50
@Kordyjan Kordyjan added this to the 3.1.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unions of Singletons unreachable literals not being checked in pattern match.
4 participants