Fix the checking for JsProperty override name clash

Change-Id: Iec0e0d524157fc639cd9e7e9af0c7a3ba6c532b7 Review-Link: https://gwt-review.googlesource.com/#/c/13994/
gwtproject · Nov 21, 2015 · 8958cd4 · 8958cd4
1 parent 77940d9
commit 8958cd4
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 47 deletions.
diff --git a/dev/core/src/com/google/gwt/dev/jjs/ast/JMethod.java b/dev/core/src/com/google/gwt/dev/jjs/ast/JMethod.java
@@ -16,14 +16,14 @@
 package com.google.gwt.dev.jjs.ast;
 
 import com.google.gwt.dev.common.InliningMode;
-import com.google.gwt.dev.javac.JsInteropUtil;
 import com.google.gwt.dev.jjs.InternalCompilerException;
 import com.google.gwt.dev.jjs.SourceInfo;
 import com.google.gwt.dev.jjs.SourceOrigin;
 import com.google.gwt.dev.jjs.ast.js.JsniMethodBody;
 import com.google.gwt.dev.jjs.impl.JjsUtils;
 import com.google.gwt.dev.util.StringInterner;
 import com.google.gwt.dev.util.collect.Lists;
+import com.google.gwt.thirdparty.guava.common.collect.Iterables;
 import com.google.gwt.thirdparty.guava.common.collect.Sets;
 
 import java.io.IOException;
@@ -76,11 +76,8 @@ public boolean isJsInteropEntryPoint() {
 
   @Override
   public boolean canBeReferencedExternally() {
-    if (exported || isJsFunctionMethod()) {
+    for (JMethod method : getOverriddenMethodsIncludingSelf()) {
-      return true;
+      if (method.exported || method.isJsFunctionMethod()) {
-    }
-    for (JMethod overriddenMethod : getOverriddenMethods()) {
-      if (overriddenMethod.exported || overriddenMethod.isJsFunctionMethod()) {
         return true;
       }
     }
@@ -124,18 +121,12 @@ public String getQualifiedJsName() {
 
   @Override
   public String getJsName() {
-    String jsMemberName = jsName;
+    for (JMethod method : getOverriddenMethodsIncludingSelf()) {
-    for (JMethod override : getOverriddenMethods()) {
+      if (method.jsName != null) {
-      String jsMemberOverrideName = override.jsName;
+        return method.jsName;
-      if (jsMemberOverrideName == null) {
-        continue;
       }
-      if (jsMemberName != null && !jsMemberName.equals(jsMemberOverrideName)) {
-        return JsInteropUtil.INVALID_JSNAME;
-      }
-      jsMemberName = jsMemberOverrideName;
     }
-    return jsMemberName;
+    return null;
   }
 
   public boolean isJsConstructor() {
@@ -166,12 +157,9 @@ public boolean exposesNonJsMember() {
 
   @Override
   public JsMemberType getJsMemberType() {
-    if (jsMemberType != JsMemberType.NONE) {
+    for (JMethod method : getOverriddenMethodsIncludingSelf()) {
-      return jsMemberType;
+      if (method.jsMemberType != JsMemberType.NONE) {
-    }
+        return method.jsMemberType;
-    for (JMethod overriddenMethod : getOverriddenMethods()) {
-      if (overriddenMethod.jsMemberType != JsMemberType.NONE) {
-        return overriddenMethod.jsMemberType;
       }
     }
     return JsMemberType.NONE;
@@ -182,11 +170,8 @@ private boolean isJsFunctionMethod() {
   }
 
   public boolean isOrOverridesJsFunctionMethod() {
-    if (isJsFunctionMethod()) {
+    for (JMethod method : getOverriddenMethodsIncludingSelf()) {
-      return true;
+      if (method.isJsFunctionMethod()) {
-    }
-    for (JMethod overriddenMethod : getOverriddenMethods()) {
-      if (overriddenMethod.isJsFunctionMethod()) {
         return true;
       }
     }
@@ -521,6 +506,13 @@ public Set<JMethod> getOverriddenMethods() {
     return overriddenMethods;
   }
 
+  /**
+   * Return all overridden methods including the method itself (where it is the first item).
+   */
+  private Iterable<JMethod> getOverriddenMethodsIncludingSelf() {
+    return Iterables.concat(Collections.singleton(this), overriddenMethods);
+  }
+
   /**
    * Returns the transitive closure of all the methods that override this method; caveat this
    * list is only complete in monolithic compiles and should not be used in incremental compiles.

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
@@ -246,13 +246,10 @@ private void checkMember(
       return;
     }
 
-    if (member.getJsName().equals(JsInteropUtil.INVALID_JSNAME)) {
+    if (!checkJsPropertyAccessor(member)) {
-      logInvalidName(member);
       return;
     }
 
-    checkJsPropertyAccessor(member);
-
     checkMemberQualifiedJsName(member);
 
     if (isCheckedLocalName(member)) {
@@ -340,21 +337,16 @@ private void checkMemberOfNativeJsType(JMember member) {
     }
   }
 
-  private void logInvalidName(JMember member) {
+  private boolean checkJsPropertyAccessor(JMember member) {
-    if (member.getJsMemberType().isPropertyAccessor()) {
+    if (member.getJsName().equals(JsInteropUtil.INVALID_JSNAME)) {
+      assert member.getJsMemberType().isPropertyAccessor();
       logError(
           member,
           "JsProperty %s should either follow Java Bean naming conventions or provide a name.",
           getMemberDescription(member));
-    } else {
+      return false;
-      logError(
-          member,
-          "%s cannot be assigned a different JavaScript name than the method it overrides.",
-          getMemberDescription(member));
     }
-  }
 
-  private void checkJsPropertyAccessor(JMember member) {
     if (member.getJsMemberType() == JsMemberType.UNDEFINED_ACCESSOR) {
       logError(member, "JsProperty %s should have a correct setter or getter signature.",
           getMemberDescription(member));
@@ -366,6 +358,7 @@ private void checkJsPropertyAccessor(JMember member) {
             getMemberDescription(member));
       }
     }
+    return true;
   }
 
   private void checkMemberQualifiedJsName(JMember member) {
@@ -414,6 +407,7 @@ private void checkLocalName(Map<String, JsMember> localNames, JMember member) {
     JsMember oldJsMember = oldAndNewJsMember.left;
     JsMember newJsMember = oldAndNewJsMember.right;
 
+    checkNameConsistency(member);
     checkJsPropertyConsistency(member, newJsMember);
 
     if (oldJsMember == null || oldJsMember == newJsMember) {
@@ -460,6 +454,22 @@ private void checkJsPropertyConsistency(JMember member, JsMember newMember) {
     }
   }
 
+  private void checkNameConsistency(JMember member) {
+    if (member instanceof JMethod) {
+      String jsName = member.getJsName();
+      for (JMethod jMethod : ((JMethod) member).getOverriddenMethods()) {
+        String parentName = jMethod.getJsName();
+        if (parentName != null && !parentName.equals(jsName)) {
+          logError(
+              member,
+              "%s cannot be assigned a different JavaScript name than the method it overrides.",
+              getMemberDescription(member));
+          break;
+        }
+      }
+    }
+  }
+
   private void checkStaticJsPropertyCalls() {
     new JVisitor() {
       @Override

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
@@ -711,21 +711,18 @@ public void testAccidentallyRenamedSuperInterfaceJsMethodFails() {
         + "cannot be assigned a different JavaScript name than the method it overrides.");
   }
 
-  // TODO(goktug): enable once the property names are handled.
+  public void testRenamedSuperclassJsPropertyFails() {
-  public void __disabled__testRenamedSuperclassJsPropertyFails() {
-    addSnippetImport("jsinterop.annotations.JsType");
     addSnippetImport("jsinterop.annotations.JsProperty");
     addSnippetClassDecl(
-        "@JsType",
         "public static class ParentBuggy {",
         "  @JsProperty public int getFoo() { return 0; }",
         "}",
         "public static class Buggy extends ParentBuggy {",
-        "  @JsProperty(name = \"bar\") public int getFoo() { return 0;}",
+        "  @JsProperty(name = \"bar\") public int getFoo() { return 0; }",
         "}");
 
-    assertBuggyFails("'EntryPoint.Buggy.getFoo()I' cannot be exported because the method "
+    assertBuggyFails("Line 8: 'int EntryPoint.Buggy.getFoo()' "
-        + "overrides a method with different name.");
+        + "cannot be assigned a different JavaScript name than the method it overrides.");
   }
 
   public void testJsPropertyDifferentFlavourInSubclassFails() {