Skip to content

Harden type checking for enum values #11526

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 3 commits into from
Mar 9, 2021
Merged

Harden type checking for enum values #11526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
35b70a9
Harden type checking for enum values
liufengyun Feb 24, 2021
71aac7f
Remove unreachable branch
liufengyun Feb 24, 2021
f7e0d31
Fix tests
liufengyun Feb 24, 2021
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
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/typer/Typer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3803,7 +3803,7 @@ class Typer extends Namer
mapOver(tp)
}

if tree.symbol.is(Module)
if tree.symbol.isOneOf(Module | Enum)
&& !(tree.tpe frozen_<:< pt) // fast track
&& !(tree.tpe frozen_<:< approx(pt))
then
Expand Down
2 changes: 0 additions & 2 deletions scaladoc/src/dotty/tools/scaladoc/translators/ScalaSignatureProvider.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ object ScalaSignatureProvider:
givenClassSignature(documentable, cls, builder)
case Kind.Given(d: Kind.Def, _, _) =>
givenMethodSignature(documentable, d, builder)
case Kind.Given(Kind.Val, _, _) =>
givenPropertySignature(documentable, builder)
case cls: Kind.Class =>
classSignature(documentable, cls, builder)
case enm: Kind.Enum =>
Expand Down
19 changes: 19 additions & 0 deletions tests/neg/i9740b.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
enum Recovery:
case RecoveryCompleted

enum TypedRecovery:
case TypedRecoveryCompleted

import Recovery.*
import TypedRecovery.*

class Test {
TypedRecoveryCompleted match {
case RecoveryCompleted => println("Recovery completed") // error
case TypedRecoveryCompleted => println("Typed recovery completed")
}

def foo(x: TypedRecovery) = x match
case RecoveryCompleted => // error
case TypedRecoveryCompleted =>
}
2 changes: 1 addition & 1 deletion tests/run/option-extract.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ enum Option[+A]:
opaque type ExtractResult[B] = (=> B) => B

def extract[B](f: A => B): ExtractResult[B] =
def result(default: => B): B = this match
def result(default: => B): B = (this: Option[A]) match
Copy link
Member

Choose a reason for hiding this comment

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

Why is this change needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

@liufengyun Here, I was asking myself the same question !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. We need to widen the scrutinee type instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, we have the following error without the change:

-- [E007] Type Mismatch Error: tests/run/option-extract.scala:10:11 ----------------------------------------------------
10 |      case None => default
   |           ^^^^
   |           Found:    (Option.None : Option[Nothing])
   |           Required: Any{ExtractResult = LazyRef([B] =>> (=> B) => B)} & Option[A]
   |           pattern type is incompatible with expected type

This seems to be a subtle interaction with opaque types in subtype checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to play with the opaque types with the following change:

--- a/compiler/src/dotty/tools/dotc/core/Types.scala
+++ b/compiler/src/dotty/tools/dotc/core/Types.scala
@@ -4548,6 +4548,7 @@ object Types {

     private var selfTypeCache: Type = null
     private var appliedRefCache: Type = null
+    private var selfTypeWithoutOpaqueCache: Type = null

     /** The self type of a class is the conjunction of
      *   - the explicit self type if given (or the info of a given self symbol), and
@@ -4566,6 +4567,23 @@ object Types {
       selfTypeCache
     }

+    /** The self type without opaque members refinements */
+    def selfTypeWithoutOpaque(using Context): Type = {
+      def dropRefinement(tp: Type): Type = tp match
+        case RefinedType(parent, _, _: TypeAlias) =>
+          // TODO: maybe strengthen the check for Opaque flag
+          parent
+        case AndType(tp1, tp2) =>
+          dropRefinement(tp1) & dropRefinement(tp2)
+        case _ =>
+          tp
+      end dropRefinement
+
+      if selfTypeWithoutOpaqueCache == null then
+        selfTypeWithoutOpaqueCache = dropRefinement(selfType)
+      selfTypeWithoutOpaqueCache
+    }

But found no place where we could sensibly use selfTypeWithoutOpaque in the code.

Copy link
Member

Choose a reason for hiding this comment

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

Currently, the refinemen is stripped in widening, and kept in member selection. What cases you have mind that this PR will be a problem?

I don't know, but I see things the other way around: we shouldn't do more stripping than we need to, and we shouldn't special-case opaque types more than we have to.

The example you posted above is another orthogonal issue --- it comes from the fact that we always widen the scrutinee in exhaustivity check.

I believe the following should also compile without warnings:

trait Mixin

sealed trait Foo { self: Mixin =>
  def foo = this match {
    case _: Foo =>
  }
}

It really seems that the cleanest fix for this sort of issues is for the exhaustiveness checker to always strip self types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kind of agree that stripping of opaque refinements should be avoided if possible. Among the following 3 choices:

  1. Close this PR and revert Fix #9740: harden type checking for pattern match on objects #11327 for consistency
  2. Keep this PR w/o stripping and change the test case tests/run/option-extract.scala with (this: Option[A])
  3. Keep this PR with stripping

Maybe the 2nd is a good compromise?

BTW, this PR and #11327 are not fixing any existing issues. They intend to harden type checking and align with Scala 2 behavoir in type checking patterns.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the 2nd is a good compromise?

I'm fine with that if we also open an issue to keep track of the issue with the exhaustivity checker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with that if we also open an issue to keep track of the issue with the exhaustivity checker.

Agreed. I'll create an issue for the test tests/run/option-extract.scala (it's a typing error, not an exhaustivity warning).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The last commit removed and we created an issue #11669 for tests/run/option-extract.scala.

case None => default
case Some(elem) => f(elem)
result
Expand Down