Merge pull request scala#10424 from som-snytt/issue/12800-enum-exhaustivity

Avoid brittle flags encoding for Java enums

Author: lrytz

src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala

@@ -305,24 +305,16 @@ abstract class BTypesFromSymbols[G <: Global](val global: G) extends BTypes {
     def javaClassfileFlags(classSym: Symbol): Int = {
       assert(classSym.isJava, s"Expected Java class symbol, got ${classSym.fullName}")
       import asm.Opcodes._
-      def enumFlags = ACC_ENUM | {
-        // Java enums have the `ACC_ABSTRACT` flag if they have a deferred method.
-        // We cannot trust `hasAbstractFlag`: the ClassfileParser adds `ABSTRACT` and `SEALED` to all
-        // Java enums for exhaustiveness checking.
-        val hasAbstractMethod = classSym.info.decls.exists(s => s.isMethod && s.isDeferred)
-        if (hasAbstractMethod) ACC_ABSTRACT else 0
-      }
-        // scala/bug#9393: the classfile / java source parser make java annotation symbols look like classes.
-        // here we recover the actual classfile flags.
-        ( if (classSym.hasJavaAnnotationFlag)                        ACC_ANNOTATION | ACC_INTERFACE | ACC_ABSTRACT else 0) |
-      ( if (classSym.isPublic)                                     ACC_PUBLIC    else 0) |
-      ( if (classSym.isFinal)                                      ACC_FINAL     else 0) |
+      // scala/bug#9393: the classfile / java source parser make java annotation symbols look like classes.
+      // here we recover the actual classfile flags.
+      (if (classSym.hasJavaAnnotationFlag) ACC_ANNOTATION | ACC_INTERFACE | ACC_ABSTRACT else 0) |
+      (if (classSym.isPublic)              ACC_PUBLIC    else 0) |
+      (if (classSym.isFinal)               ACC_FINAL     else 0) |
        // see the link above. javac does the same: ACC_SUPER for all classes, but not interfaces.)
-      ( if (classSym.isInterface)                                  ACC_INTERFACE else ACC_SUPER) |
-       // for Java enums, we cannot trust `hasAbstractFlag` (see comment in enumFlags))
-      ( if (!classSym.hasJavaEnumFlag && classSym.hasAbstractFlag) ACC_ABSTRACT  else 0) |
-      ( if (classSym.isArtifact)                                   ACC_SYNTHETIC else 0) |
-      ( if (classSym.hasJavaEnumFlag)                              enumFlags     else 0)
+      (if (classSym.isInterface)           ACC_INTERFACE else ACC_SUPER) |
+      (if (classSym.hasAbstractFlag)       ACC_ABSTRACT  else 0) |
+      (if (classSym.isArtifact)            ACC_SYNTHETIC else 0) |
+      (if (classSym.hasJavaEnumFlag)       ACC_ENUM      else 0)
     }
 
     // Check for hasAnnotationFlag for scala/bug#9393: the classfile / java source parsers add

src/compiler/scala/tools/nsc/javac/JavaParsers.scala

@@ -1030,11 +1030,14 @@ trait JavaParsers extends ast.parser.ParsersCommon with JavaScanners {
       accept(RBRACE)
       val superclazz =
         AppliedTypeTree(javaLangDot(tpnme.Enum), List(enumType))
+      val hasAbstractMember = body.exists {
+        case m: MemberDef => m.mods.hasFlag(Flags.DEFERRED)
+        case _ => false
+      }
       val finalFlag = if (enumIsFinal) Flags.FINAL else 0L
+      val abstractFlag = if (hasAbstractMember) Flags.ABSTRACT else 0L
       addCompanionObject(consts ::: statics ::: predefs, atPos(pos) {
-        // Marking the enum class SEALED | ABSTRACT enables exhaustiveness checking. See also ClassfileParser.
-        // This is a bit of a hack and requires excluding the ABSTRACT flag in the backend, see method javaClassfileFlags.
-        ClassDef(mods | Flags.JAVA_ENUM | Flags.SEALED | Flags.ABSTRACT | finalFlag, name, List(),
+        ClassDef(mods | Flags.JAVA_ENUM | Flags.SEALED | abstractFlag | finalFlag, name, List(),
                  makeTemplate(superclazz :: interfaces, body))
       })
     }

src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala

@@ -593,19 +593,13 @@ abstract class ClassfileParser(reader: ReusableInstance[ReusableDataReader]) {
       getScope(jflags) enter sym
 
       // sealed java enums
-      if (jflags.isEnum) {
-        val enumClass = sym.owner.linkedClassOfClass
-        enumClass match {
+      if (jflags.isEnum)
+        sym.owner.linkedClassOfClass match {
           case NoSymbol =>
             devWarning(s"no linked class for java enum $sym in ${sym.owner}. A referencing class file might be missing an InnerClasses entry.")
-          case linked =>
-            if (!linked.isSealed)
-              // Marking the enum class SEALED | ABSTRACT enables exhaustiveness checking. See also JavaParsers.
-              // This is a bit of a hack and requires excluding the ABSTRACT flag in the backend, see method javaClassfileFlags.
-              linked setFlag (SEALED | ABSTRACT)
-            linked addChild sym
+          case enumClass =>
+            enumClass.addChild(sym)
         }
-      }
     }
   }
 
@@ -1382,9 +1376,11 @@ abstract class ClassfileParser(reader: ReusableInstance[ReusableDataReader]) {
         ClassInfoType(superTpe1 :: ifacesTypes, instanceScope, clazz)
       }
       sym.setInfo(info)
-      for (k <- permittedSubclasses)
-        if (k.parentSymbols.contains(sym))
-          sym.addChild(k)
+      // enum children are its enum fields, so don't register subclasses (which are listed as permitted)
+      if (!sym.hasJavaEnumFlag)
+        for (k <- permittedSubclasses)
+          if (k.parentSymbols.contains(sym))
+            sym.addChild(k)
     }
   }
 

src/compiler/scala/tools/nsc/transform/patmat/MatchAnalysis.scala

@@ -129,7 +129,9 @@ trait TreeAndTypeAnalysis extends Debugging {
       def filterAndSortChildren(children: Set[Symbol]) = {
         // symbols which are both sealed and abstract need not be covered themselves,
         // because all of their children must be and they cannot otherwise be created.
-        val children1 = children.toList.filterNot(child => child.isSealed && child.isAbstractClass).sortBy(_.sealedSortName)
+        val children1 = children.toList
+          .filterNot(child => child.isSealed && (child.isAbstractClass || child.hasJavaEnumFlag))
+          .sortBy(_.sealedSortName)
         children1.filterNot { child =>
           // remove private abstract children that are superclasses of other children, for example in t6159 drop X2
           child.isPrivate && child.isAbstractClass && children1.exists(sym => (sym ne child) && sym.isSubClass(child))
@@ -874,7 +876,7 @@ trait MatchAnalysis extends MatchApproximation {
                       case args                            => args
                     }.map(ListExample)
                   case _ if isTupleSymbol(cls)                  => args(brevity = true).map(TupleExample)
-                  case _ if cls.isSealed && cls.isAbstractClass =>
+                  case _ if cls.isSealed && (cls.isAbstractClass || cls.hasJavaEnumFlag) =>
                     // don't report sealed abstract classes, since
                     // 1) they can't be instantiated
                     // 2) we are already reporting any missing subclass (since we know the full domain)

src/reflect/scala/reflect/internal/ClassfileConstants.scala

@@ -353,7 +353,7 @@ object ClassfileConstants {
       case JAVA_ACC_STATIC     => STATIC
       case JAVA_ACC_ABSTRACT   => if (isClass) ABSTRACT else DEFERRED
       case JAVA_ACC_INTERFACE  => TRAIT | INTERFACE | ABSTRACT
-      case JAVA_ACC_ENUM       => JAVA_ENUM
+      case JAVA_ACC_ENUM       => if (isClass) JAVA_ENUM | SEALED else JAVA_ENUM
       case JAVA_ACC_ANNOTATION => JAVA_ANNOTATION
       case _                   => 0L
     }

test/files/neg/sealed-java-enums.scala

@@ -1,4 +1,4 @@
-// scalac: -Xfatal-warnings
+// scalac: -Werror
 //
 import java.lang.Thread.State
 import java.lang.Thread.State._

test/files/neg/t12800.check

@@ -0,0 +1,7 @@
+matcher_1.scala:8: warning: match may not be exhaustive.
+It would fail on the following input: ORANGE
+    jb match {
+    ^
+error: No warnings can be incurred under -Werror.
+1 warning
+1 error

test/files/neg/t12800/JetBrains.java

@@ -0,0 +1,13 @@
+
+public enum JetBrains {
+	APPLE {
+		@Override public String text() {
+			return "Cupertino tech company";
+		}
+	},
+	ORANGE
+	;
+	public String text() {
+		return "Boring default";
+	}
+}

test/files/neg/t12800/matcher_1.scala

@@ -0,0 +1,11 @@
+
+// scalac: -Werror -Xsource:3
+
+import JetBrains.*
+
+class C {
+  def f(jb: JetBrains): Int =
+    jb match {
+      case APPLE  => 42
+    }
+}

test/files/neg/t8700b-new.check

@@ -1,11 +1,11 @@
 Bar_2.scala:4: warning: match may not be exhaustive.
 It would fail on the following input: B
-  def bar1(foo: Foo_1) = foo match {
-                         ^
+  def bar1(foo: Foo) = foo match {
+                       ^
 Bar_2.scala:8: warning: match may not be exhaustive.
 It would fail on the following input: B
-  def bar2(foo: Baz_1) = foo match {
-                         ^
+  def bar2(foo: Baz) = foo match {
+                       ^
 error: No warnings can be incurred under -Werror.
 2 warnings
 1 error

test/files/neg/t8700b-new/Bar_2.scala

@@ -0,0 +1,11 @@
+// scalac: -Werror
+//
+object Bar {
+  def bar1(foo: Foo) = foo match {
+    case Foo.A => 1
+  }
+
+  def bar2(foo: Baz) = foo match {
+    case Baz.A => 1
+  }
+}

test/files/neg/t8700b-new/Baz_1.java

@@ -1,5 +1,4 @@
-// javaVersion: 8
-public enum Baz_1 {
+public enum Baz {
     A {
     	public void baz1() {}
     },

test/files/neg/t8700b-new/Foo_1.java

@@ -0,0 +1,4 @@
+public enum Foo {
+    A,
+    B
+}

test/files/neg/t8700b-old/Bar_2.scala

@@ -0,0 +1,14 @@
+
+public enum JetBrains {
+	APPLE {
+		@Override public String text() {
+			return "Cupertino tech company";
+		}
+	},
+	ORANGE {
+		@Override public String text() {
+			return "SoCal county";
+		}
+	};
+	public abstract String text();
+}

test/files/neg/t8700b-old/Foo_1.java

@@ -0,0 +1,12 @@
+
+// scalac: -Werror -Xsource:3
+
+import JetBrains.*
+
+class C {
+  def f(jb: JetBrains): Int =
+    jb match {
+      case APPLE  => 42
+      case ORANGE => 27
+    }
+}