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

Avoid brittle flags encoding for Java enums #10424

Merged
merged 2 commits into from
Jun 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
26 changes: 9 additions & 17 deletions src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 6 additions & 3 deletions src/compiler/scala/tools/nsc/javac/JavaParsers.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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))
})
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
}
}

Expand Down Expand Up @@ -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)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Here I don't follow, when is isClass true, when is it false?

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 enum member fields get the flag. (I believe I did not imagine that.)

case JAVA_ACC_ANNOTATION => JAVA_ANNOTATION
case _ => 0L
}
Expand Down
2 changes: 1 addition & 1 deletion test/files/neg/sealed-java-enums.scala
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// scalac: -Xfatal-warnings
// scalac: -Werror
//
import java.lang.Thread.State
import java.lang.Thread.State._
Expand Down
7 changes: 7 additions & 0 deletions test/files/neg/t12800.check
Original file line number Diff line number Diff line change
@@ -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
13 changes: 13 additions & 0 deletions test/files/neg/t12800/JetBrains.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@

public enum JetBrains {
APPLE {
@Override public String text() {
return "Cupertino tech company";
}
},
ORANGE
;
public String text() {
return "Boring default";
}
}
11 changes: 11 additions & 0 deletions test/files/neg/t12800/matcher_1.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@

// scalac: -Werror -Xsource:3

import JetBrains.*

class C {
def f(jb: JetBrains): Int =
jb match {
case APPLE => 42
}
}
11 changes: 0 additions & 11 deletions test/files/neg/t8700b-new.check

This file was deleted.

11 changes: 0 additions & 11 deletions test/files/neg/t8700b-new/Bar_2.scala

This file was deleted.

12 changes: 0 additions & 12 deletions test/files/neg/t8700b-new/Baz_1.java

This file was deleted.

5 changes: 0 additions & 5 deletions test/files/neg/t8700b-new/Foo_1.java

This file was deleted.

11 changes: 0 additions & 11 deletions test/files/neg/t8700b-old/Bar_2.scala

This file was deleted.

5 changes: 0 additions & 5 deletions test/files/neg/t8700b-old/Foo_1.java

This file was deleted.

Original file line number Diff line number Diff line change
@@ -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
11 changes: 11 additions & 0 deletions test/files/neg/t8700b/Bar_2.scala
Original file line number Diff line number Diff line change
@@ -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
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
// javaVersion: 8
public enum Baz_1 {
public enum Baz {
A {
public void baz1() {}
},
Expand Down
4 changes: 4 additions & 0 deletions test/files/neg/t8700b/Foo.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
public enum Foo {
A,
B
}
14 changes: 14 additions & 0 deletions test/files/pos/t12800/JetBrains.java
Original file line number Diff line number Diff line change
@@ -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();
}
12 changes: 12 additions & 0 deletions test/files/pos/t12800/matcher_1.scala
Original file line number Diff line number Diff line change
@@ -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
}
}