Enforce single non-delegating constructor restriction.
Subclasses of JsConstructor declaring JsTypes need to have
only one non-delegating constructor to allow for
straightforward generation of future ES6 code.
Bug: #9335
Bug-Link: https://github.com/gwtproject/gwt/issues/9335
Change-Id: Ic35a700ad4db3b7ba8b5152e2c352b5eb44115b4
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/JjsUtils.java b/dev/core/src/com/google/gwt/dev/jjs/impl/JjsUtils.java
index 7415529..0b081db 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/JjsUtils.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/JjsUtils.java
@@ -32,6 +32,7 @@
import com.google.gwt.dev.jjs.ast.JDeclaredType;
import com.google.gwt.dev.jjs.ast.JDoubleLiteral;
import com.google.gwt.dev.jjs.ast.JExpression;
+import com.google.gwt.dev.jjs.ast.JExpressionStatement;
import com.google.gwt.dev.jjs.ast.JField;
import com.google.gwt.dev.jjs.ast.JFloatLiteral;
import com.google.gwt.dev.jjs.ast.JIntLiteral;
@@ -537,6 +538,83 @@
return emptyMethod;
}
+ /**
+ * Extracts the this(..) or super(..) call from a statement if the statement is of the expected
+ * form. Otherwise returns null.
+ */
+ public static JMethodCall getThisOrSuperConstructorCall(
+ JStatement statement) {
+ if (!(statement instanceof JExpressionStatement)) {
+ return null;
+ }
+
+ JExpressionStatement expressionStatement = (JExpressionStatement) statement;
+ if (!(expressionStatement.getExpr() instanceof JMethodCall)
+ || expressionStatement.getExpr() instanceof JNewInstance) {
+ return null;
+ }
+
+ JMethodCall call = (JMethodCall) expressionStatement.getExpr();
+ if (call.getTarget() instanceof JConstructor && call.isStaticDispatchOnly()) {
+ return call;
+ }
+ return null;
+ }
+
+ /**
+ * Returns the JsConstructor for a class or null if it does not have any.
+ */
+ public static JConstructor getJsConstructor(JDeclaredType type) {
+ return
+ FluentIterable
+ .from(type.getConstructors())
+ .filter(new Predicate<JConstructor>() {
+ @Override
+ public boolean apply(JConstructor constructor) {
+ return constructor.isJsConstructor();
+ }
+ }).first().orNull();
+ }
+
+ /**
+ * Returns the constructor which this constructor delegates or null if none.
+ */
+ public static JConstructor getDelegatedThisOrSuperConstructor(JConstructor constructor) {
+ JStatement contructorInvocaton = FluentIterable
+ .from(constructor.getBody().getStatements())
+ .filter(new Predicate<JStatement>() {
+ @Override
+ public boolean apply(JStatement statement) {
+ return getThisOrSuperConstructorCall(statement) != null;
+ }
+ }).first().orNull();
+
+ return contructorInvocaton != null
+ ? (JConstructor) getThisOrSuperConstructorCall(contructorInvocaton).getTarget()
+ : null;
+ }
+
+ /**
+ * Returns the constructor which all others delegate to if any, otherwise null.
+ */
+ public static JConstructor getPrimaryConstructor(final JDeclaredType type) {
+ List<JConstructor> delegatedSuperConstructors = FluentIterable
+ .from(type.getConstructors())
+ .filter(new Predicate<JConstructor>() {
+ @Override
+ public boolean apply(JConstructor constructor) {
+ // Calls super constructor.
+ return getDelegatedThisOrSuperConstructor(constructor).getEnclosingType() != type;
+ }
+ })
+ .limit(2)
+ .toList();
+ if (delegatedSuperConstructors.size() == 1) {
+ return delegatedSuperConstructors.get(0);
+ }
+ return null;
+ }
+
private JjsUtils() {
}
}
\ No newline at end of file
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/JsInteropRestrictionChecker.java b/dev/core/src/com/google/gwt/dev/jjs/impl/JsInteropRestrictionChecker.java
index 7c1dbb8..2dca64b 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/JsInteropRestrictionChecker.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/JsInteropRestrictionChecker.java
@@ -134,17 +134,10 @@
|| ((JMethodBody) type.getInitMethod().getBody()).getStatements().isEmpty();
}
- private void checkJsConstructors(JDeclaredType x) {
- List<JMethod> jsConstructors = FluentIterable
- .from(x.getMethods())
- .filter(new Predicate<JMethod>() {
- @Override
- public boolean apply(JMethod m) {
- return m.isJsConstructor();
- }
- }).toList();
+ private void checkJsConstructors(JDeclaredType type) {
+ List<JConstructor> jsConstructors = getJsConstructors(type);
- if (x.isJsNative()) {
+ if (type.isJsNative()) {
return;
}
@@ -153,32 +146,76 @@
}
if (jsConstructors.size() > 1) {
- logError(x, "More than one JsConstructor exists for %s.", JjsUtils.getReadableDescription(x));
+ logError(type,
+ "More than one JsConstructor exists for %s.", getDescription(type));
}
final JConstructor jsConstructor = (JConstructor) jsConstructors.get(0);
- boolean anyNonDelegatingConstructor = Iterables.any(x.getMethods(), new Predicate<JMethod>() {
- @Override
- public boolean apply(JMethod method) {
- return method != jsConstructor && method instanceof JConstructor
- && !isDelegatingToConstructor((JConstructor) method, jsConstructor);
- }
- });
-
- if (anyNonDelegatingConstructor) {
+ if (JjsUtils.getPrimaryConstructor(type) != jsConstructor) {
logError(jsConstructor,
"Constructor %s can be a JsConstructor only if all constructors in the class are "
+ "delegating to it.", getMemberDescription(jsConstructor));
}
}
+ private List<JConstructor> getJsConstructors(JDeclaredType type) {
+ return FluentIterable
+ .from(type.getConstructors())
+ .filter(new Predicate<JConstructor>() {
+ @Override
+ public boolean apply(JConstructor m) {
+ return m.isJsConstructor();
+ }
+ }).toList();
+ }
+
+ private void checkJsConstructorSubtype(JDeclaredType type) {
+ if (!isJsConstructorSubtype(type)) {
+ return;
+ }
+ if (Iterables.isEmpty(type.getConstructors())) {
+ // No constructors in the type; type is not instantiable.
+ return;
+ }
+
+ if (type.isJsNative()) {
+ return;
+ }
+
+ JClassType superClass = type.getSuperClass();
+ JConstructor superPrimaryConsructor = JjsUtils.getPrimaryConstructor(superClass);
+ if (!superClass.isJsNative() && superPrimaryConsructor == null) {
+ // Superclass has JsConstructor but does not satisfy the JsConstructor restrictions, no need
+ // to report more errors.
+ return;
+ }
+
+ JConstructor primaryConstructor = JjsUtils.getPrimaryConstructor(type);
+ if (primaryConstructor == null) {
+ logError(type,
+ "Class %s should have only one constructor delegating to the superclass since it is "
+ + "subclass of a a type with JsConstructor.", getDescription(type));
+ return;
+ }
+
+ JConstructor delegatedConstructor =
+ JjsUtils.getDelegatedThisOrSuperConstructor(primaryConstructor);
+
+ if (delegatedConstructor.isJsConstructor() ||
+ delegatedConstructor == superPrimaryConsructor) {
+ return;
+ }
+
+ logError(primaryConstructor,
+ "Constructor %s can only delegate to super constructor %s since it is a subclass of a "
+ + "type with JsConstructor.",
+ getDescription(primaryConstructor),
+ getDescription(superPrimaryConsructor));
+ }
+
private boolean isDelegatingToConstructor(JConstructor ctor, JConstructor targetCtor) {
- List<JStatement> statements = ctor.getBody().getBlock().getStatements();
- JExpressionStatement statement = (JExpressionStatement) statements.get(0);
- JMethodCall call = (JMethodCall) statement.getExpr();
- assert call.isStaticDispatchOnly() : "Every ctor should either have this() or super() call";
- return call.getTarget().equals(targetCtor);
+ return JjsUtils.getDelegatedThisOrSuperConstructor(ctor) == targetCtor;
}
private void checkMember(
@@ -745,6 +782,18 @@
return !hasErrors;
}
+ private boolean isJsConstructorSubtype(JDeclaredType type) {
+ JClassType superClass = type.getSuperClass();
+ if (superClass == null) {
+ return false;
+ }
+
+ if (JjsUtils.getJsConstructor(superClass) != null) {
+ return true;
+ }
+ return isJsConstructorSubtype(superClass);
+ }
+
private void checkType(JDeclaredType type) {
minimalRebuildCache.removeExportedNames(type.getName());
@@ -769,6 +818,7 @@
} else {
checkJsFunctionSubtype(type);
checkJsConstructors(type);
+ checkJsConstructorSubtype(type);
}
Map<String, JsMember> ownGlobalNames = Maps.newHashMap();
@@ -832,7 +882,8 @@
return Maps.newLinkedHashMap();
}
- LinkedHashMap<String, JsMember> memberByLocalMemberNames = collectLocalNames(type.getSuperClass());
+ LinkedHashMap<String, JsMember> memberByLocalMemberNames =
+ collectLocalNames(type.getSuperClass());
for (JMember member : type.getMembers()) {
if (isCheckedLocalName(member)) {
updateJsMembers(memberByLocalMemberNames, member);
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/RemoveEmptySuperCalls.java b/dev/core/src/com/google/gwt/dev/jjs/impl/RemoveEmptySuperCalls.java
index 35cced8..1cc5829 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/RemoveEmptySuperCalls.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/RemoveEmptySuperCalls.java
@@ -20,7 +20,6 @@
import com.google.gwt.dev.jjs.ast.JExpressionStatement;
import com.google.gwt.dev.jjs.ast.JMethodCall;
import com.google.gwt.dev.jjs.ast.JModVisitor;
-import com.google.gwt.dev.jjs.ast.JNewInstance;
import com.google.gwt.dev.jjs.ast.JProgram;
import com.google.gwt.dev.jjs.ast.js.JMultiExpression;
@@ -35,22 +34,22 @@
public static class EmptySuperCallVisitor extends JModVisitor {
@Override
public void endVisit(JExpressionStatement x, Context ctx) {
- if (x.getExpr() instanceof JMethodCall && !(x.getExpr() instanceof JNewInstance)) {
- JMethodCall call = (JMethodCall) x.getExpr();
- if (call.getTarget() instanceof JConstructor) {
- JConstructor ctor = (JConstructor) call.getTarget();
- if (ctor.isEmpty() && !ctor.isJsNative()) {
- // TODO: move this 3-way into Simplifier.
- if (call.getArgs().isEmpty()) {
- ctx.removeMe();
- } else if (call.getArgs().size() == 1) {
- ctx.replaceMe(call.getArgs().get(0).makeStatement());
- } else {
- JMultiExpression multi = new JMultiExpression(call.getSourceInfo());
- multi.addExpressions(call.getArgs());
- ctx.replaceMe(multi.makeStatement());
- }
- }
+ JMethodCall call = JjsUtils.getThisOrSuperConstructorCall(x);
+ if (call == null) {
+ return;
+ }
+
+ JConstructor ctor = (JConstructor) call.getTarget();
+ if (ctor.isEmpty() && !ctor.isJsNative()) {
+ // TODO: move this 3-way into Simplifier.
+ if (call.getArgs().isEmpty()) {
+ ctx.removeMe();
+ } else if (call.getArgs().size() == 1) {
+ ctx.replaceMe(call.getArgs().get(0).makeStatement());
+ } else {
+ JMultiExpression multi = new JMultiExpression(call.getSourceInfo());
+ multi.addExpressions(call.getArgs());
+ ctx.replaceMe(multi.makeStatement());
}
}
}
diff --git a/dev/core/test/com/google/gwt/dev/jjs/impl/JsInteropRestrictionCheckerTest.java b/dev/core/test/com/google/gwt/dev/jjs/impl/JsInteropRestrictionCheckerTest.java
index 10f0325..f1b872f 100644
--- a/dev/core/test/com/google/gwt/dev/jjs/impl/JsInteropRestrictionCheckerTest.java
+++ b/dev/core/test/com/google/gwt/dev/jjs/impl/JsInteropRestrictionCheckerTest.java
@@ -1010,17 +1010,114 @@
addSnippetImport("jsinterop.annotations.JsIgnore");
addSnippetClassDecl(
"@JsType",
- "public static class Buggy {",
+ "static class Buggy {",
" public Buggy() {}",
" @JsIgnore",
" public Buggy(int a) {",
" this();",
" }",
+ "}",
+ "static class SubBuggy extends Buggy {",
+ " public SubBuggy() { this(1);}",
+ " public SubBuggy(int a) { super();}",
+ "}",
+ "@JsType",
+ "static class JsSubBuggy extends Buggy {",
+ " @JsIgnore",
+ " public JsSubBuggy() { this(1);}",
+ " public JsSubBuggy(int a) { super();}",
+ "}",
+ "@JsType (isNative = true)",
+ "static class NativeBuggy {",
+ " public NativeBuggy() {}",
+ " public NativeBuggy(int a) {}",
+ "}",
+ "@JsType (isNative = true)",
+ "static class NativeSubNativeBuggy extends NativeBuggy{",
+ " public NativeSubNativeBuggy() { super(1); }",
+ " public NativeSubNativeBuggy(int a) { super();}",
+ "}",
+ "static class SubNativeBuggy extends NativeBuggy {",
+ " public SubNativeBuggy() { this(1);}",
+ " public SubNativeBuggy(int a) { super();}",
+ "}",
+ "static class SubSubNativeBuggy extends NativeBuggy {",
+ " public SubSubNativeBuggy() { super(1);}",
+ " public SubSubNativeBuggy(int a) { this(); }",
+ "}",
+ "static class SubNativeBuggyImplicitConstructor extends NativeBuggy {",
"}");
assertBuggySucceeds();
}
+ public void testMultipleConstructorsNonJsSubtypeRestrictionFails() {
+ addSnippetImport("jsinterop.annotations.JsConstructor");
+ addSnippetImport("jsinterop.annotations.JsIgnore");
+ addSnippetImport("jsinterop.annotations.JsType");
+ addSnippetClassDecl(
+ "@JsType",
+ "static class BuggyJsType {",
+ " public BuggyJsType() {}",
+ " @JsIgnore",
+ " public BuggyJsType(int a) { this(); }",
+ "}",
+ "static class Buggy extends BuggyJsType {",
+ // Error: two non-delegation constructors"
+ " public Buggy() {}",
+ " public Buggy(int a) { super(a); }",
+ "}",
+ "static class SubBuggyJsType extends BuggyJsType {",
+ // Correct: one non-delegating constructor targeting super primary constructor
+ " public SubBuggyJsType() { this(1); }",
+ " public SubBuggyJsType(int a) { super(); }",
+ "}",
+ "static class SubSubBuggyJsType extends SubBuggyJsType {",
+ // Error: non-delegating constructor target the wrong super constructor.
+ " public SubSubBuggyJsType() { this(1);}",
+ " public SubSubBuggyJsType(int a) { super(); }",
+ "}",
+ "static class JsConstructorSubBuggyJsType extends SubBuggyJsType {",
+ // Error: non-delegating constructor target the wrong super constructor.
+ " public JsConstructorSubBuggyJsType() { super(1);}",
+ " @JsConstructor",
+ " public JsConstructorSubBuggyJsType(int a) { super(); }",
+ "}",
+ "static class OtherSubBuggyJsType extends BuggyJsType {",
+ // Error: JsConstructor not delegating to super primary constructor.
+ " public OtherSubBuggyJsType() { super();}",
+ " @JsConstructor",
+ " public OtherSubBuggyJsType(int a) { this(); }",
+ "}",
+ "static class AnotherSubBuggyJsType extends BuggyJsType {",
+ // Error: Multiple JsConstructors in JsConstructor subclass.
+ " @JsConstructor",
+ " public AnotherSubBuggyJsType() { super();}",
+ " @JsConstructor",
+ " public AnotherSubBuggyJsType(int a) { this(); }",
+ "}");
+
+ assertBuggyFails(
+ "Line 12: Class 'EntryPoint.Buggy' should have only one constructor delegating to the "
+ + "superclass since it is subclass of a a type with JsConstructor.",
+ "Line 22: Constructor 'EntryPoint.SubSubBuggyJsType.EntryPoint$SubSubBuggyJsType(int)' "
+ + "can only delegate to super constructor "
+ + "'EntryPoint.SubBuggyJsType.EntryPoint$SubBuggyJsType(int)' since it is a subclass "
+ + "of a type with JsConstructor.",
+ "Line 24: Class 'EntryPoint.JsConstructorSubBuggyJsType' should have only one constructor "
+ + "delegating to the superclass since it is subclass of a a type with JsConstructor.",
+ "Line 27: Constructor "
+ + "'EntryPoint.JsConstructorSubBuggyJsType.EntryPoint$JsConstructorSubBuggyJsType(int)'"
+ + " can be a JsConstructor only if all constructors in the class are delegating to it.",
+ "Line 32: Constructor 'EntryPoint.OtherSubBuggyJsType.EntryPoint$OtherSubBuggyJsType(int)' "
+ + "can be a JsConstructor only if all constructors in the class are delegating to it.",
+ "Line 34: More than one JsConstructor exists for 'EntryPoint.AnotherSubBuggyJsType'.",
+ "Line 38: 'EntryPoint.AnotherSubBuggyJsType.EntryPoint$AnotherSubBuggyJsType(int)' cannot "
+ + "be exported because the global name 'test.EntryPoint.AnotherSubBuggyJsType' is "
+ + "already taken by "
+ + "'EntryPoint.AnotherSubBuggyJsType.EntryPoint$AnotherSubBuggyJsType()'.");
+ }
+
public void testMultipleConstructorsNotAllDelegatedToJsConstructorFails()
throws Exception {
addSnippetImport("jsinterop.annotations.JsType");
@@ -1050,7 +1147,7 @@
"}");
assertBuggyFails(
- "Line 5: More than one JsConstructor exists for EntryPoint.Buggy.",
+ "Line 5: More than one JsConstructor exists for 'EntryPoint.Buggy'.",
"Line 7: 'EntryPoint.Buggy.EntryPoint$Buggy(int)' cannot be exported because the global "
+ "name 'test.EntryPoint.Buggy' is already taken by "
+ "'EntryPoint.Buggy.EntryPoint$Buggy()'.");
diff --git a/user/test/com/google/gwt/core/interop/JsTypeVarargsTest.java b/user/test/com/google/gwt/core/interop/JsTypeVarargsTest.java
index c9a70d0..1b72674 100644
--- a/user/test/com/google/gwt/core/interop/JsTypeVarargsTest.java
+++ b/user/test/com/google/gwt/core/interop/JsTypeVarargsTest.java
@@ -114,10 +114,6 @@
}
static class SubclassNativeWithVarargsConstructor extends NativeJsTypeWithVarargsConstructor {
- SubclassNativeWithVarargsConstructor(String s, Object... args) {
- super(1, args[0], args[1], null);
- }
-
SubclassNativeWithVarargsConstructor(int i, Object... args) {
super(i, args);
}
@@ -214,11 +210,6 @@
assertSame(someNativeObject, object.a);
assertEquals(3, object.b);
- object = new SubclassNativeWithVarargsConstructor("", someNativeObject, null);
-
- assertSame(someNativeObject, object.a);
- assertEquals(4, object.b);
-
object = new SubclassNativeWithVarargsConstructor(1, someNativeObject, null);
assertSame(someNativeObject, object.a);
