Never mark enums as abstract
Using enum constant members as a heuristic for abstract enums was
broken, because of cases like:
public enum E {
ONE {
public void f() {}
}
}
javac drops the inaccessible member f() and doesn't generate an instance
class for ONE, so E doesn't need to be abstract.
Assuming all enums are final is safe, because nothing outside the
compilation unit can extend abstract enums anyways, and refactoring an
existing enum to implement methods in the container class instead of the
constants is not a breaking change.
MOE_MIGRATED_REVID=138207374
diff --git a/java/com/google/turbine/binder/HierarchyBinder.java b/java/com/google/turbine/binder/HierarchyBinder.java
index 9daba59..06fcee2 100644
--- a/java/com/google/turbine/binder/HierarchyBinder.java
+++ b/java/com/google/turbine/binder/HierarchyBinder.java
@@ -103,11 +103,11 @@
switch (decl.tykind()) {
case ENUM:
superclass = ClassSymbol.ENUM;
- if (isEnumAbstract(decl)) {
- access |= TurbineFlag.ACC_ABSTRACT;
- } else {
- access |= TurbineFlag.ACC_FINAL;
- }
+ // Assuming all enums are final is safe, because nothing outside
+ // the compilation unit can extend abstract enums anyways, and
+ // refactoring an existing enum to implement methods in the container
+ // class instead of the constants is not a breaking change.
+ access |= TurbineFlag.ACC_FINAL;
break;
case INTERFACE:
case ANNOTATION:
@@ -183,24 +183,6 @@
}
/**
- * If any enum constants have a class body (which is recorded in the parser by setting ENUM_IMPL),
- * the class generated for the enum needs to have ACC_ABSTRACT set.
- */
- private static boolean isEnumAbstract(Tree.TyDecl decl) {
- for (Tree t : decl.members()) {
- if (t.kind() != Tree.Kind.VAR_DECL) {
- continue;
- }
- Tree.VarDecl var = (Tree.VarDecl) t;
- if (!var.mods().contains(TurbineModifier.ENUM_IMPL)) {
- continue;
- }
- return true;
- }
- return false;
- }
-
- /**
* Resolves the {@link ClassSymbol} for the given {@link Tree.ClassTy}, with handling for
* non-canonical qualified type names.
*/
diff --git a/java/com/google/turbine/model/TurbineFlag.java b/java/com/google/turbine/model/TurbineFlag.java
index 572aa9d..fcd2a1e 100644
--- a/java/com/google/turbine/model/TurbineFlag.java
+++ b/java/com/google/turbine/model/TurbineFlag.java
@@ -48,8 +48,5 @@
public static final int ACC_DEFAULT = 1 << 16;
/** Synthetic constructors (e.g. of inner classes and enums). */
- public static final int ACC_ENUM_IMPL = 1 << 17;
-
- /** Synthetic constructors (e.g. of inner classes and enums). */
public static final int ACC_SYNTH_CTOR = 1 << 18;
}
diff --git a/java/com/google/turbine/parse/Parser.java b/java/com/google/turbine/parse/Parser.java
index ab05511..e25ccb5 100644
--- a/java/com/google/turbine/parse/Parser.java
+++ b/java/com/google/turbine/parse/Parser.java
@@ -274,16 +274,14 @@
dropParens();
}
// TODO(cushon): consider desugaring enum constants later
- EnumSet<TurbineModifier> access = EnumSet.copyOf(ENUM_CONSTANT_MODIFIERS);
if (token == Token.LBRACE) {
- access.add(TurbineModifier.ENUM_IMPL);
dropBlocks();
}
maybe(Token.COMMA);
result.add(
new VarDecl(
position,
- access,
+ ENUM_CONSTANT_MODIFIERS,
annos.build(),
new ClassTy(
position,
diff --git a/java/com/google/turbine/tree/Pretty.java b/java/com/google/turbine/tree/Pretty.java
index 7cf9354..820126d 100644
--- a/java/com/google/turbine/tree/Pretty.java
+++ b/java/com/google/turbine/tree/Pretty.java
@@ -481,7 +481,6 @@
case ACC_ANNOTATION:
case ACC_SYNTHETIC:
case ACC_BRIDGE:
- case ENUM_IMPL:
break;
default:
throw new AssertionError(mod);
diff --git a/java/com/google/turbine/tree/TurbineModifier.java b/java/com/google/turbine/tree/TurbineModifier.java
index 2b2495e..3d0e60d 100644
--- a/java/com/google/turbine/tree/TurbineModifier.java
+++ b/java/com/google/turbine/tree/TurbineModifier.java
@@ -44,8 +44,7 @@
ACC_ANNOTATION(TurbineFlag.ACC_ANNOTATION),
ACC_SYNTHETIC(TurbineFlag.ACC_SYNTHETIC),
ACC_BRIDGE(TurbineFlag.ACC_BRIDGE),
- DEFAULT(TurbineFlag.ACC_DEFAULT),
- ENUM_IMPL(TurbineFlag.ACC_ENUM_IMPL);
+ DEFAULT(TurbineFlag.ACC_DEFAULT);
private final int flag;
diff --git a/javatests/com/google/turbine/binder/BinderTest.java b/javatests/com/google/turbine/binder/BinderTest.java
index 1ceeceb..5c2988b 100644
--- a/javatests/com/google/turbine/binder/BinderTest.java
+++ b/javatests/com/google/turbine/binder/BinderTest.java
@@ -122,40 +122,6 @@
}
@Test
- public void enums() throws Exception {
- List<Tree.CompUnit> units = new ArrayList<>();
- units.add(
- parseLines(
- "package test;", //
- "public enum E1 {",
- " ONE,",
- " TWO {",
- " @Override public void f() {}",
- " },",
- " THREE;",
- " public void f() {}",
- "}"));
- units.add(
- parseLines(
- "package test;", //
- "public enum E2 {",
- " ONE, TWO, THREE;",
- " public void g() {}",
- "}"));
-
- ImmutableMap<ClassSymbol, SourceTypeBoundClass> bound =
- Binder.bind(units, Collections.emptyList(), BOOTCLASSPATH).units();
-
- SourceTypeBoundClass e1 = bound.get(new ClassSymbol("test/E1"));
- assertThat((e1.access() & TurbineFlag.ACC_ABSTRACT) == TurbineFlag.ACC_ABSTRACT).isTrue();
- assertThat(e1.superclass()).isEqualTo(new ClassSymbol("java/lang/Enum"));
- assertThat(e1.interfaces()).isEmpty();
-
- SourceTypeBoundClass e2 = bound.get(new ClassSymbol("test/E2"));
- assertThat((e2.access() & TurbineFlag.ACC_ABSTRACT) == 0).isTrue();
- }
-
- @Test
public void imports() throws Exception {
List<Tree.CompUnit> units = new ArrayList<>();
units.add(
diff --git a/javatests/com/google/turbine/lower/IntegrationTestSupport.java b/javatests/com/google/turbine/lower/IntegrationTestSupport.java
index 0edfe30..f9fadc9 100644
--- a/javatests/com/google/turbine/lower/IntegrationTestSupport.java
+++ b/javatests/com/google/turbine/lower/IntegrationTestSupport.java
@@ -104,12 +104,27 @@
for (ClassNode n : classes) {
removeImplementation(n);
removeUnusedInnerClassAttributes(infos, n);
+ makeEnumsFinal(n);
sortMembersAndAttributes(n);
}
return toByteCode(classes);
}
+ private static void makeEnumsFinal(ClassNode n) {
+ n.innerClasses.forEach(
+ x -> {
+ if ((x.access & Opcodes.ACC_ENUM) == Opcodes.ACC_ENUM) {
+ x.access &= ~Opcodes.ACC_ABSTRACT;
+ x.access |= Opcodes.ACC_FINAL;
+ }
+ });
+ if ((n.access & Opcodes.ACC_ENUM) == Opcodes.ACC_ENUM) {
+ n.access &= ~Opcodes.ACC_ABSTRACT;
+ n.access |= Opcodes.ACC_FINAL;
+ }
+ }
+
private static Map<String, byte[]> toByteCode(List<ClassNode> classes) {
Map<String, byte[]> out = new LinkedHashMap<>();
for (ClassNode n : classes) {
diff --git a/javatests/com/google/turbine/lower/LowerIntegrationTest.java b/javatests/com/google/turbine/lower/LowerIntegrationTest.java
index e262873..99d4123 100644
--- a/javatests/com/google/turbine/lower/LowerIntegrationTest.java
+++ b/javatests/com/google/turbine/lower/LowerIntegrationTest.java
@@ -262,6 +262,7 @@
"anno_repeated.test",
"long_expression.test",
"const_nonfinal.test",
+ "enum_abstract.test",
};
List<Object[]> tests =
ImmutableList.copyOf(testCases).stream().map(x -> new Object[] {x}).collect(toList());
diff --git a/javatests/com/google/turbine/lower/testdata/enum_abstract.test b/javatests/com/google/turbine/lower/testdata/enum_abstract.test
new file mode 100644
index 0000000..e7ef3a4
--- /dev/null
+++ b/javatests/com/google/turbine/lower/testdata/enum_abstract.test
@@ -0,0 +1,13 @@
+=== Test.java ===
+class Test {
+ enum E {
+ ONE,
+ TWO {
+ // javac drops the inaccessible member f() and doesn't generate
+ // an instance class for ONE, so E doesn't need to be abstract.
+ public void f() {
+ System.err.println("hello");
+ }
+ };
+ }
+}
diff --git a/javatests/com/google/turbine/parse/ParserTest.java b/javatests/com/google/turbine/parse/ParserTest.java
deleted file mode 100644
index 03b7d2b..0000000
--- a/javatests/com/google/turbine/parse/ParserTest.java
+++ /dev/null
@@ -1,59 +0,0 @@
-/*
- * Copyright 2016 Google Inc. All Rights Reserved.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package com.google.turbine.parse;
-
-import static com.google.common.truth.Truth.assertThat;
-
-import com.google.common.base.Joiner;
-import com.google.common.collect.Iterables;
-import com.google.turbine.tree.Tree;
-import com.google.turbine.tree.TurbineModifier;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-
-@RunWith(JUnit4.class)
-public class ParserTest {
-
- // regression test: only enum constants with bodies should have ENUM_IMPL set
- @Test
- public void enumConstantDeclaration() {
- String[] input = {
- "enum E {", //
- " ONE {",
- " public void f() {}",
- " },",
- " TWO;",
- "}",
- };
- Tree.CompUnit unit = Parser.parse(Joiner.on('\n').join(input));
- Tree.TyDecl decl = Iterables.getOnlyElement(unit.decls());
- assertThat(((Tree.VarDecl) decl.members().get(0)).mods())
- .containsExactly(
- TurbineModifier.PUBLIC,
- TurbineModifier.STATIC,
- TurbineModifier.ACC_ENUM,
- TurbineModifier.FINAL,
- TurbineModifier.ENUM_IMPL);
- assertThat(((Tree.VarDecl) decl.members().get(1)).mods())
- .containsExactly(
- TurbineModifier.PUBLIC,
- TurbineModifier.STATIC,
- TurbineModifier.ACC_ENUM,
- TurbineModifier.FINAL);
- }
-}