Add restrictions to the exporting of ctors.

With this patch, now a constructor could only be exported either if it
is the only constructor in the class or if all other constructors are
delegating to it.

Also the exported can no longer supply a name. It will be always exported
by the qualified name of the type.

Exporting multiple constructors for the same object in different
namespace is problematic due to js instanceof checks and placing
static methods. Also in the long run, we are already moving towards
generating single constructor per class while generating user-defined
constructors as factory methods so this is making JsInterop inline
with that.

Change-Id: I4008afe6d86261c6de2c7bda3aa1dfad992123a4
Review-Link: https://gwt-review.googlesource.com/#/c/12634/

dev/core/src/com/google/gwt/dev/jjs/impl/JsInteropRestrictionChecker.java

@@ -17,23 +17,29 @@
 import com.google.gwt.core.ext.UnableToCompleteException;
 import com.google.gwt.dev.MinimalRebuildCache;
 import com.google.gwt.dev.jjs.ast.Context;
+import com.google.gwt.dev.jjs.ast.JConstructor;
 import com.google.gwt.dev.jjs.ast.JDeclaredType;
+import com.google.gwt.dev.jjs.ast.JExpressionStatement;
 import com.google.gwt.dev.jjs.ast.JField;
 import com.google.gwt.dev.jjs.ast.JInterfaceType;
 import com.google.gwt.dev.jjs.ast.JMember;
 import com.google.gwt.dev.jjs.ast.JMethod;
 import com.google.gwt.dev.jjs.ast.JMethod.JsPropertyType;
+import com.google.gwt.dev.jjs.ast.JMethodCall;
 import com.google.gwt.dev.jjs.ast.JPrimitiveType;
 import com.google.gwt.dev.jjs.ast.JProgram;
+import com.google.gwt.dev.jjs.ast.JStatement;
 import com.google.gwt.dev.jjs.ast.JType;
 import com.google.gwt.dev.jjs.ast.JVisitor;
 import com.google.gwt.thirdparty.guava.common.base.Function;
+import com.google.gwt.thirdparty.guava.common.base.Predicate;
 import com.google.gwt.thirdparty.guava.common.collect.FluentIterable;
 import com.google.gwt.thirdparty.guava.common.collect.Iterables;
 import com.google.gwt.thirdparty.guava.common.collect.Maps;
 import com.google.gwt.thirdparty.guava.common.collect.Ordering;
 import com.google.gwt.thirdparty.guava.common.collect.Sets;
 
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.SortedSet;
@@ -43,6 +49,8 @@
  */
 // TODO: handle custom JsType field/method names when that feature exists.
 // TODO: move JsInterop checks from JSORestrictionsChecker to here.
+// TODO: provide more information in global name collisions as it could be difficult to pinpoint in
+// big projects.
 public class JsInteropRestrictionChecker extends JVisitor {
 
   public static void exec(TreeLogger logger, JProgram jprogram,
@@ -94,6 +102,8 @@ public boolean visit(JDeclaredType x, Context ctx) {
     checkJsFunctionJsTypeCollision(x);
     if (currentType instanceof JInterfaceType) {
       checkJsTypeHierarchy((JInterfaceType) currentType);
+    } else {
+      checkConstructors(x);
     }
 
     // Perform custom class traversal to examine fields and methods of this class and all
@@ -108,6 +118,51 @@ public boolean visit(JDeclaredType x, Context ctx) {
     return false;
   }
 
+  private void checkConstructors(JDeclaredType x) {
+    List<JMethod> exportedCtors = FluentIterable
+        .from(x.getMethods())
+        .filter(new Predicate<JMethod>() {
+           @Override
+           public boolean apply(JMethod m) {
+             return m.isExported() && m instanceof JConstructor;
+           }
+        }).toList();
+
+    if (exportedCtors.isEmpty()) {
+      return;
+    }
+
+    if (exportedCtors.size() > 1) {
+      logError("More than one constructor exported for %s.", x.getName());
+    }
+
+    final JConstructor exportedCtor = (JConstructor) exportedCtors.get(0);
+    if (!exportedCtor.getExportName().isEmpty()) {
+      logError("Constructor '%s' cannot have an export name.", exportedCtor.getQualifiedName());
+    }
+
+    boolean anyNonDelegatingConstructor = Iterables.any(x.getMethods(), new Predicate<JMethod>() {
+      @Override
+      public boolean apply(JMethod method) {
+        return method != exportedCtor && method instanceof JConstructor
+            && !isDelegatingToConstructor((JConstructor) method, exportedCtor);
+      }
+    });
+
+    if (anyNonDelegatingConstructor) {
+      logError("Constructor '%s' can only be exported if all constructors in the class are "
+          + "delegating to it.", exportedCtor.getQualifiedName());
+    }
+  }
+
+  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);
+  }
+
   @Override
   public boolean visit(JField x, Context ctx) {
     if (currentType == x.getEnclosingType() && jprogram.typeOracle.isExportedField(x)) {

dev/core/test/com/google/gwt/dev/jjs/impl/JsInteropRestrictionCheckerTest.java

@@ -661,6 +661,41 @@ public void testMultiplePrivateConstructorsExportSucceeds() throws Exception {
     assertBuggySucceeds();
   }
 
+  public void testMultiplePublicConstructorsAllDelegatesToExportedConstructorSucceeds()
+      throws Exception {
+    addSnippetImport("com.google.gwt.core.client.js.JsExport");
+    addSnippetImport("com.google.gwt.core.client.js.JsNoExport");
+    addSnippetClassDecl(
+        "@JsExport",
+        "public static class Buggy {",
+        "  public Buggy() {}",
+        "  @JsNoExport",
+        "  public Buggy(int a) {",
+        "    this();",
+        "  }",
+        "}");
+
+    assertBuggySucceeds();
+  }
+
+  public void testMultipleConstructorsNotAllDelegatedToExportedConstructorFails()
+      throws Exception {
+    addSnippetImport("com.google.gwt.core.client.js.JsExport");
+    addSnippetImport("com.google.gwt.core.client.js.JsNoExport");
+    addSnippetClassDecl(
+        "@JsExport",
+        "public static class Buggy {",
+        "  public Buggy() {}",
+        "  private Buggy(int a) {",
+        "    new Buggy();",
+        "  }",
+        "}");
+
+    assertBuggyFails(
+        "Constructor 'test.EntryPoint$Buggy.EntryPoint$Buggy() <init>' can only be exported if all "
+        + "constructors in the class are delegating to it.");
+  }
+
   public void testMultiplePublicConstructorsExportFails() throws Exception {
     addSnippetImport("com.google.gwt.core.client.js.JsExport");
     addSnippetClassDecl(
@@ -671,6 +706,9 @@ public void testMultiplePublicConstructorsExportFails() throws Exception {
         "}");
 
     assertBuggyFails(
+        "More than one constructor exported for test.EntryPoint$Buggy.",
+        "Constructor 'test.EntryPoint$Buggy.EntryPoint$Buggy() <init>' can only be exported if all "
+        + "constructors in the class are delegating to it.",
         "Member 'test.EntryPoint$Buggy.EntryPoint$Buggy(I) <init>' can't be "
         + "exported because the global name 'test.EntryPoint.Buggy' is already taken.");
   }
@@ -697,6 +735,19 @@ public void testNonCollidingAccidentalOverrideSucceeds() throws Exception {
     assertBuggySucceeds();
   }
 
+  public void testSingleConstructortExportWithNameFails() throws Exception {
+    addSnippetImport("com.google.gwt.core.client.js.JsExport");
+    addSnippetClassDecl(
+        "public static class Buggy {",
+        "  @JsExport(\"Create\")",
+        "  public Buggy() {}",
+        "}");
+
+    assertBuggyFails(
+        "Constructor 'test.EntryPoint$Buggy.EntryPoint$Buggy() <init>' cannot have an export "
+        + "name.");
+  }
+
   public void testSingleExportSucceeds() throws Exception {
     addSnippetImport("com.google.gwt.core.client.js.JsExport");
     addSnippetClassDecl(

user/test/com/google/gwt/core/client/interop/JsExportTest.java

@@ -138,52 +138,13 @@ private native Object createMyExportedClassWithImplicitConstructor() /*-{
     return new $global.woo.MyExportedClassWithImplicitConstructor();
   }-*/;
 
-  public void testExportClass_multipleConstructors() {
-    assertEquals(3, getSumByDefaultConstructor());
-    assertEquals(30, getSumByConstructor());
-  }
-
-  private native int getSumByDefaultConstructor() /*-{
-    var obj = new $global.woo.MyExportedClassWithMultipleConstructors.MyClassConstructor1();
-    return obj.sum();
-  }-*/;
-
-  private native int getSumByConstructor() /*-{
-    var obj = new $global.woo.MyExportedClassWithMultipleConstructors.MyClassConstructor2(10, 20);
-    return obj.sum();
-  }-*/;
-
-  public void testExportClass_instanceOf() {
-    assertTrue(createMyExportedClassWithMultipleConstructors1()
-        instanceof MyExportedClassWithMultipleConstructors);
-    assertTrue(createMyExportedClassWithMultipleConstructors2()
-        instanceof MyExportedClassWithMultipleConstructors);
-  }
-
-  private native Object createMyExportedClassWithMultipleConstructors1() /*-{
-    return new $global.woo.MyExportedClassWithMultipleConstructors.MyClassConstructor1();
-  }-*/;
-
-  private native Object createMyExportedClassWithMultipleConstructors2() /*-{
-    return new $global.woo.MyExportedClassWithMultipleConstructors.MyClassConstructor2(10, 20);
-  }-*/;
-
   public void testExportConstructors() {
-    assertEquals(4, createMyClassExportsConstructors().foo());
-    assertTrue(isNotExportedConstructorExported());
+    assertEquals(4, createMyClassExportsConstructor().foo());
+    assertEquals(2, new MyClassExportsConstructor().foo());
   }
 
-  private native MyClassExportsConstructors createMyClassExportsConstructors() /*-{
-    return new $global.woo.MyClassExportsConstructors.MyClassExportsConstructors1(2);
-  }-*/;
-
-  private native boolean isNotExportedConstructorExported() /*-{
-    var namespace = $global.woo.MyClassExportsConstructors;
-    if (typeof(namespace) == 'function')
-      return false;
-    if (Object.getOwnPropertyNames($global.woo.MyClassExportsConstructors).length != 1)
-      return false;
-    return true;
+  private native MyClassExportsConstructor createMyClassExportsConstructor() /*-{
+    return new $global.woo.MyClassExportsConstructor(2);
   }-*/;
 
   public void testExportedField() {

user/test/com/google/gwt/core/client/interop/MyClassExportsConstructors.java → user/test/com/google/gwt/core/client/interop/MyClassExportsConstructor.java

@@ -20,16 +20,16 @@
 /**
  * A test class that exhibits a variety of @JsExports on constructors.
  */
-public class MyClassExportsConstructors {
+public class MyClassExportsConstructor {
   private int a;
 
-  @JsExport("MyClassExportsConstructors1")
-  public MyClassExportsConstructors(int a) {
+  @JsExport
+  public MyClassExportsConstructor(int a) {
     this.a = a;
   }
 
-  public MyClassExportsConstructors() {
-    this.a = 1;
+  public MyClassExportsConstructor() {
+    this(1);
   }
 
   public int foo() {