Fixes and enforces restrictions on @JsExport

- Adds support for @JsNoExport on fields - Adds check for validness of JsExport on fields - Adds check for validness of JsExport on methods defined in non-jstypes - Adds check for JsExport and JsNotExport mixed together - Adds many missing related tests Also now explicit @JsExport of non-final static fields is banned and implicit export of non-final static fields via class-wide JsExport produces a warning (to keep it forward compatible). This is because as we copy the fields on export, they no longer effect the actual declaration on set so they are effectively final until we fix that. TODO: add missing visibility checks Change-Id: I9addc4f9f7fe8de8172697cee3e60789c18896df Review-Link: https://gwt-review.googlesource.com/#/c/11170/
gwtproject · Jan 27, 2015 · e775da6 · e775da6
1 parent 1ba8887
commit e775da6
Show file tree

Hide file tree

Showing 15 changed files with 216 additions and 43 deletions.
diff --git a/dev/core/src/com/google/gwt/dev/javac/JSORestrictionsChecker.java b/dev/core/src/com/google/gwt/dev/javac/JSORestrictionsChecker.java
@@ -33,6 +33,7 @@
 import org.eclipse.jdt.internal.compiler.lookup.BlockScope;
 import org.eclipse.jdt.internal.compiler.lookup.ClassScope;
 import org.eclipse.jdt.internal.compiler.lookup.CompilationUnitScope;
+import org.eclipse.jdt.internal.compiler.lookup.FieldBinding;
 import org.eclipse.jdt.internal.compiler.lookup.MethodBinding;
 import org.eclipse.jdt.internal.compiler.lookup.MethodScope;
 import org.eclipse.jdt.internal.compiler.lookup.ReferenceBinding;
@@ -61,8 +62,10 @@ public class JSORestrictionsChecker {
 
   public static final String ERR_JSTYPE_OVERLOADS_NOT_ALLOWED =
       "JsType methods cannot overload another method.";
-  public static final String ERR_JSEXPORT_ONLY_CTORS_AND_STATIC_METHODS =
+  public static final String ERR_JSEXPORT_ONLY_CTORS_STATIC_METHODS_AND_STATIC_FINAL_FIELDS =
-      "@JsExport may only be applied to constructors and static methods.";
+      "@JsExport may only be applied to constructors and static methods and static final fields.";
+  public static final String ERR_EITHER_JSEXPORT_JSNOEXPORT =
+      "@JsExport and @JsNoExport is not allowed at the same time.";
   public static final String ERR_JSPROPERTY_ONLY_BEAN_OR_FLUENT_STYLE_NAMING =
       "@JsProperty is only allowed on JavaBean-style or fluent-style named methods";
   public static final String ERR_MUST_EXTEND_MAGIC_PROTOTYPE_CLASS =
@@ -173,6 +176,8 @@ public void endVisit(ConstructorDeclaration meth, ClassScope scope) {
 
     @Override
     public void endVisit(FieldDeclaration field, MethodScope scope) {
+      checkJsExport(field.binding);
+
       if (!isJso()) {
         return;
       }
@@ -183,6 +188,8 @@ public void endVisit(FieldDeclaration field, MethodScope scope) {
 
     @Override
     public void endVisit(MethodDeclaration meth, ClassScope scope) {
+      checkJsExport(meth.binding);
+
       if (!isJso()) {
         return;
       }
@@ -246,20 +253,31 @@ private void checkJsType(TypeDeclaration type, TypeBinding typeBinding) {
         }
       }
       Map<String, MethodBinding> methodSignatures = new HashMap<String, MethodBinding>();
-      Map<String, MethodBinding> noExports = new HashMap<String, MethodBinding>();
 
-      checkJsTypeMethodsForOverloads(methodSignatures, noExports, binding);
+      checkJsTypeMethodsForOverloads(methodSignatures, binding);
       for (MethodBinding mb : binding.methods()) {
         checkJsProperty(mb, true);
-        checkJsExport(mb, !binding.isInterface());
       }
     }
 
-    private void checkJsExport(MethodBinding mb, boolean allowed) {
+    private void checkJsExport(MethodBinding mb) {
-      AnnotationBinding jsExport = JdtUtil.getAnnotation(mb, JsInteropUtil.JSEXPORT_CLASS);
+      if (JdtUtil.getAnnotation(mb, JsInteropUtil.JSEXPORT_CLASS) != null) {
-      if (jsExport != null && allowed) {
         if (!mb.isConstructor() && !mb.isStatic()) {
-          errorOn(mb, ERR_JSEXPORT_ONLY_CTORS_AND_STATIC_METHODS);
+          errorOn(mb, ERR_JSEXPORT_ONLY_CTORS_STATIC_METHODS_AND_STATIC_FINAL_FIELDS);
+        }
+        if (JdtUtil.getAnnotation(mb, JsInteropUtil.JSNOEXPORT_CLASS) != null) {
+          errorOn(mb, ERR_EITHER_JSEXPORT_JSNOEXPORT);
+        }
+      }
+    }
+
+    private void checkJsExport(FieldBinding fb) {
+      if (JdtUtil.getAnnotation(fb, JsInteropUtil.JSEXPORT_CLASS) != null) {
+        if (!fb.isStatic() || !fb.isFinal()) {
+          errorOn(fb, ERR_JSEXPORT_ONLY_CTORS_STATIC_METHODS_AND_STATIC_FINAL_FIELDS);
+        }
+        if (JdtUtil.getAnnotation(fb, JsInteropUtil.JSNOEXPORT_CLASS) != null) {
+          errorOn(fb, ERR_EITHER_JSEXPORT_JSNOEXPORT);
         }
       }
     }
@@ -314,15 +332,10 @@ private boolean isHas(String name, MethodBinding mb) {
     }
 
     private void checkJsTypeMethodsForOverloads(Map<String, MethodBinding> methodNamesAndSigs,
-                                                Map<String, MethodBinding> noExports,
                                                 ReferenceBinding binding) {
       if (isJsType(binding)) {
         for (MethodBinding mb : binding.methods()) {
           String methodName = String.valueOf(mb.selector);
-          if (JdtUtil.getAnnotation(mb, JsInteropUtil.JSNOEXPORT_CLASS) != null) {
-            noExports.put(methodName, mb);
-            continue;
-          }
           if (mb.isConstructor() || mb.isStatic()) {
             continue;
           }
@@ -334,10 +347,6 @@ private void checkJsTypeMethodsForOverloads(Map<String, MethodBinding> methodNam
           }
           if (methodNamesAndSigs.containsKey(methodName)) {
             if (!methodNamesAndSigs.get(methodName).areParameterErasuresEqual(mb)) {
-              if (noExports.containsKey(methodName)
-                  && noExports.get(methodName).areParameterErasuresEqual(mb)) {
-                continue;
-              }
               errorOn(mb, ERR_JSTYPE_OVERLOADS_NOT_ALLOWED);
             }
           } else {
@@ -346,7 +355,7 @@ private void checkJsTypeMethodsForOverloads(Map<String, MethodBinding> methodNam
         }
       }
       for (ReferenceBinding rb : binding.superInterfaces()) {
-        checkJsTypeMethodsForOverloads(methodNamesAndSigs, noExports, rb);
+        checkJsTypeMethodsForOverloads(methodNamesAndSigs, rb);
       }
     }
 
@@ -398,7 +407,6 @@ private boolean checkClassImplementingJsType(TypeDeclaration type) {
       }
 
       for (MethodBinding mb : type.binding.methods()) {
-        checkJsExport(mb, true);
         checkJsProperty(mb, false);
       }
 
@@ -595,4 +603,9 @@ private void errorOn(MethodBinding mb, String error) {
     }
     errorOn(node, cud, error);
   }
+
+  private void errorOn(FieldBinding fb, String error) {
+    ASTNode node = fb.sourceField();
+    errorOn(node, cud, error);
+  }
 }
diff --git a/dev/core/src/com/google/gwt/dev/javac/JsInteropUtil.java b/dev/core/src/com/google/gwt/dev/javac/JsInteropUtil.java
@@ -51,6 +51,9 @@ public static void maybeSetExportedField(FieldDeclaration x, JField field) {
         }
         field.setExportName(value);
       }
+      if (JdtUtil.getAnnotation(x.binding, JSNOEXPORT_CLASS) != null) {
+        field.setNoExport(true);
+      }
     }
   }
 

diff --git a/dev/core/src/com/google/gwt/dev/jjs/ast/JField.java b/dev/core/src/com/google/gwt/dev/jjs/ast/JField.java
@@ -50,6 +50,15 @@ private boolean isVolatile() {
   }
 
   private String exportName;
+  private boolean noExport = false;
+
+  public boolean isNoExport() {
+    return noExport;
+  }
+
+  public void setNoExport(boolean noExport) {
+    this.noExport = noExport;
+  }
 
   public void setExportName(String exportName) {
     this.exportName = exportName;

diff --git a/dev/core/src/com/google/gwt/dev/jjs/ast/JTypeOracle.java b/dev/core/src/com/google/gwt/dev/jjs/ast/JTypeOracle.java
@@ -256,7 +256,7 @@ public boolean needsJsInteropBridgeMethod(JMethod x) {
   }
 
   public boolean isExportedField(JField field) {
-    return isJsInteropEnabled() && field.getExportName() != null;
+    return isJsInteropEnabled() && field.getExportName() != null && !field.isNoExport();
   }
 
   public boolean isExportedMethod(JMethod method) {
@@ -797,7 +797,7 @@ public void computeBeforeAST(StandardTypes standardTypes, Collection<JDeclaredTy
         }
       }
       for (JField field : type.getFields()) {
-        if (field.getExportName() != null) {
+        if (isExportedField(field)) {
           exportedFields.add(field);
         }
       }

diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java b/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
@@ -2764,6 +2764,7 @@ private void generateVTables(JClassType x, List<JsStatement> globalStmts) {
     }
 
     private void generateExports(JDeclaredType x, List<JsStatement> globalStmts) {
+      TreeLogger branch = logger.branch(TreeLogger.Type.INFO, "Exporting " + x.getName());
 
       String lastProvidedNamespace = "";
       boolean createdClinit = false;
@@ -2798,6 +2799,11 @@ private void generateExports(JDeclaredType x, List<JsStatement> globalStmts) {
 
       for (JField f : x.getFields()) {
         if (f.isStatic() && f.getExportName() != null) {
+          if (!f.isFinal()) {
+            branch.log(TreeLogger.Type.WARN, "Exporting effectively non-final field " + f.getName()
+                + " is discouraged. Due to the way exporting works, the value of the exported field"
+                + " will not be reflected across Java&JavaScript border.");
+          }
           createdClinit = maybeHoistClinit(exportStmts, createdClinit,
               maybeCreateClinitCall(f, true));
           JsNameRef exportRhs = names.get(f).makeRef(f.getSourceInfo());

diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java b/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java
@@ -2475,7 +2475,7 @@ protected void endVisit(TypeDeclaration x) {
           if (f.getExportName() != null) {
             continue;
           }
-          if (f.isStatic() || type instanceof JInterfaceType) {
+          if (f.isStatic()) {
             f.setExportName("");
           }
         }

diff --git a/dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java b/dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java
@@ -380,19 +380,93 @@ public void testJsTypeNoOverloadsHierarchy() {
         "Line 3: " + JSORestrictionsChecker.ERR_JSTYPE_OVERLOADS_NOT_ALLOWED);
   }
 
-  public void testJsExportNotOnMethod() {
+  public void testJsExport() {
+    StringBuilder goodCode = new StringBuilder();
+    goodCode.append("import com.google.gwt.core.client.js.JsExport;\n");
+    goodCode.append("public class Buggy {\n");
+    goodCode.append("@JsExport public static final String field = null;\n");
+    goodCode.append("@JsExport public static void method() {}\n");
+    goodCode.append("public interface Foo {\n");
+    goodCode.append("@JsExport String field1 = null;\n");
+    // TODO: enable after java 8 becomes default
+    // goodCode.append("@JsExport static void method1() {}\n");
+    goodCode.append("}\n");
+    goodCode.append("}\n");
+
+    shouldGenerateNoError(goodCode);
+  }
+
+  public void testJsExportOnClass() {
+    StringBuilder goodCode = new StringBuilder();
+    goodCode.append("import com.google.gwt.core.client.js.JsExport;\n");
+    goodCode.append("@JsExport public class Buggy {}");
+
+    shouldGenerateNoError(goodCode);
+  }
+
+  public void testJsExportOnInterface() {
+    StringBuilder goodCode = new StringBuilder();
+    goodCode.append("import com.google.gwt.core.client.js.JsExport;\n");
+    goodCode.append("@JsExport public interface Buggy {}");
+
+    shouldGenerateNoError(goodCode);
+  }
+
+  public void testJsExportNotOnObjectMethod() {
     StringBuilder buggyCode = new StringBuilder();
-    buggyCode.append("import com.google.gwt.core.client.js.JsType;\n");
     buggyCode.append("import com.google.gwt.core.client.js.JsExport;\n");
     buggyCode.append("public class Buggy {\n");
-    buggyCode.append("@JsType interface Foo {}\n");
+    buggyCode.append("@JsExport public void foo() {}\n");
-    buggyCode.append("static class BuggyFoo implements Foo {\n");
+    buggyCode.append("}\n");
-    buggyCode.append("@JsExport void foo() {}\n");
+
+    shouldGenerateError(buggyCode, "Line 3: "
+        + JSORestrictionsChecker.ERR_JSEXPORT_ONLY_CTORS_STATIC_METHODS_AND_STATIC_FINAL_FIELDS);
+  }
+
+  public void testJsExportNotOnObjectField() {
+    StringBuilder buggyCode = new StringBuilder();
+    buggyCode.append("import com.google.gwt.core.client.js.JsExport;\n");
+    buggyCode.append("public class Buggy {\n");
+    buggyCode.append("@JsExport public final String foo = null;\n");
     buggyCode.append("}\n");
+
+    shouldGenerateError(buggyCode, "Line 3: "
+        + JSORestrictionsChecker.ERR_JSEXPORT_ONLY_CTORS_STATIC_METHODS_AND_STATIC_FINAL_FIELDS);
+  }
+
+  public void testJsExportNotOnNonFinalField() {
+    StringBuilder buggyCode = new StringBuilder();
+    buggyCode.append("import com.google.gwt.core.client.js.JsExport;\n");
+    buggyCode.append("public class Buggy {\n");
+    buggyCode.append("@JsExport public static String foo = null;\n");
     buggyCode.append("}\n");
 
-    shouldGenerateError(buggyCode, "Line 6: "
+    shouldGenerateError(buggyCode, "Line 3: "
-        + JSORestrictionsChecker.ERR_JSEXPORT_ONLY_CTORS_AND_STATIC_METHODS);
+        + JSORestrictionsChecker.ERR_JSEXPORT_ONLY_CTORS_STATIC_METHODS_AND_STATIC_FINAL_FIELDS);
+  }
+
+  public void testJsExportAndJsNotExportNotOnField() {
+    StringBuilder buggyCode = new StringBuilder();
+    buggyCode.append("import com.google.gwt.core.client.js.JsExport;\n");
+    buggyCode.append("import com.google.gwt.core.client.js.JsNoExport;\n");
+    buggyCode.append("public class Buggy {\n");
+    buggyCode.append("@JsExport @JsNoExport public final static String foo = null;\n");
+    buggyCode.append("}\n");
+
+    shouldGenerateError(buggyCode, "Line 4: "
+        + JSORestrictionsChecker.ERR_EITHER_JSEXPORT_JSNOEXPORT);
+  }
+
+  public void testJsExportAndJsNotExportNotOnMethod() {
+    StringBuilder buggyCode = new StringBuilder();
+    buggyCode.append("import com.google.gwt.core.client.js.JsExport;\n");
+    buggyCode.append("import com.google.gwt.core.client.js.JsNoExport;\n");
+    buggyCode.append("public class Buggy {\n");
+    buggyCode.append("@JsExport @JsNoExport public static void method() {}\n");
+    buggyCode.append("}\n");
+
+    shouldGenerateError(buggyCode, "Line 4: "
+        + JSORestrictionsChecker.ERR_EITHER_JSEXPORT_JSNOEXPORT);
   }
 
   public void testJsPrototypeNotOnClass() {

diff --git a/user/src/com/google/gwt/core/client/js/JsExport.java b/user/src/com/google/gwt/core/client/js/JsExport.java
@@ -22,12 +22,23 @@
 import java.lang.annotation.Target;
 
 /**
- * JsExport marks a constructor, static method, or static field as creating a an unobfuscated alias
+ * JsExport marks a constructor, static method, or static field, creating an unobfuscated alias in
- * in the global scope. JsExport acts as an entry-point from the standpoint of the optimizer,
+ * the global scope.
- * and all code reachable from an exported method is also considered live, so use with care.
+ * <p>
+ * When JsExport is applied to an entire class or interface, it is syntactic sugar for applying
+ * JsExport to every public static field and method of the class, except for constructors. When
+ * JsExport is applied to an entire class that is a java enum, all enumarations are exported as
+ * well. JsNoExport may be used to opt-out a public method or field if JsExport has been applied to
+ * an entire class.
+ * <p>
+ * Exported members act as an entry-point from the standpoint of the optimizer, and all code
+ * reachable from an exported method is also considered live, so use with care.
+ *
+ * @see JsNoExport
+ * @see JsNamespace
  */
 @Retention(RetentionPolicy.RUNTIME)
-@Target({ ElementType.CONSTRUCTOR, ElementType.METHOD, ElementType.FIELD, ElementType.TYPE })
+@Target({ElementType.CONSTRUCTOR, ElementType.METHOD, ElementType.FIELD, ElementType.TYPE})
 @Documented
 public @interface JsExport {
   String value() default "";

diff --git a/user/test/com/google/gwt/core/client/interop/JsTypeTest.java b/user/test/com/google/gwt/core/client/interop/JsTypeTest.java
@@ -68,16 +68,37 @@ private native int getWOO() /*-{
           && $wnd.woo.PackageNamespaceTester.WOO || 0;
   }-*/;
 
-  private native Object getStaticInitializerStaticField() /*-{
+  private native Object getStaticInitializerStaticField1() /*-{
       return $wnd && $wnd.woo && $wnd.woo.StaticInitializerStaticField
-          && $wnd.woo.StaticInitializerStaticField.STATIC;
+          && $wnd.woo.StaticInitializerStaticField.EXPORTED_1;
+  }-*/;
+
+  private native Object getStaticInitializerStaticField2() /*-{
+    return $wnd && $wnd.woo && $wnd.woo.StaticInitializerStaticField
+        && $wnd.woo.StaticInitializerStaticField.EXPORTED_2;
+  }-*/;
+
+  private native Object getExportedFieldOnInterface() /*-{
+    return $wnd && $wnd.woo && $wnd.woo.StaticInitializerStaticField
+        && $wnd.woo.StaticInitializerStaticField.InterfaceWithField
+        && $wnd.woo.StaticInitializerStaticField.InterfaceWithField.STATIC;
   }-*/;
 
   private native Object getStaticInitializerStaticMethod() /*-{
       return $wnd && $wnd.woo && $wnd.woo.StaticInitializerStaticMethod
           && $wnd.woo.StaticInitializerStaticMethod.getInstance();
   }-*/;
 
+  private native Object getNotExportedFields() /*-{
+    return $wnd.woo.StaticInitializerStaticField.NOT_EXPORTED_1
+        || $wnd.woo.StaticInitializerStaticField.NOT_EXPORTED_2;
+  }-*/;
+
+  private native Object getNotExportedMethods() /*-{
+    return $wnd.woo.StaticInitializerStaticMethod.notExported_1
+        || $wnd.woo.StaticInitializerStaticMethod.notExported_2;
+  }-*/;
+
   private native Object getStaticInitializerVirtualMethod() /*-{
       if($wnd && $wnd.woo && $wnd.woo.StaticInitializerVirtualMethod) {
           var obj = new $wnd.woo.StaticInitializerVirtualMethod();
@@ -95,7 +116,9 @@ public void testPackageNamespace() {
   }
 
   public void testStaticInitializerStaticField() {
-    assertNotNull(getStaticInitializerStaticField());
+    assertNotNull(getStaticInitializerStaticField1());
+    assertNotNull(getStaticInitializerStaticField2());
+    assertNotNull(getExportedFieldOnInterface());
   }
 
   public void testStaticInitializerStaticMethod() {
@@ -106,6 +129,11 @@ public void testStaticInitializerVirtualMethod() {
     assertNotNull(getStaticInitializerVirtualMethod());
   }
 
+  public void testNotExport() {
+    assertNull(getNotExportedMethods());
+    assertNull(getNotExportedFields());
+  }
+
   public void testVirtualUpRefs() {
     ListImpl l2 = new ListImpl();
     FooImpl f2 = new FooImpl(); // both inherit .add(), but this one shouldn't be exported