Skip to content

Commit

Permalink
Adds more JsInterop name collision checking.
Browse files Browse the repository at this point in the history 
Ensures that there is at most 1 flavor of getter implemented for any 1 
JsProperty property.

Ensures that there is at most 1 flavor of setter implemented for any 1 
JsProperty property.

Ensures that no JsType nonJsProperty function name collides with the 
property name for any JsProperty.

Change-Id: I86aab47a89a16e219177c6fde062cf4ffc1fe349
Review-Link: https://gwt-review.googlesource.com/#/c/12060/
  • Loading branch information
@stalcup
stalcup committed Mar 9, 2015
1 parent bee2e84 commit 72b2e63
Show file tree
Hide file tree
Showing 13 changed files with 547 additions and 135 deletions.
42 changes: 40 additions & 2 deletions dev/core/src/com/google/gwt/dev/javac/JsInteropUtil.java
View file
Expand Up @@ -20,11 +20,14 @@
import com.google.gwt.dev.jjs.ast.JField;
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 org.eclipse.jdt.internal.compiler.ast.Annotation;
import org.eclipse.jdt.internal.compiler.ast.TypeDeclaration;
import org.eclipse.jdt.internal.compiler.lookup.AnnotationBinding;

import java.beans.Introspector;

/**
* Utility functions to interact with JDT classes for JsInterop.
*/
Expand All @@ -40,7 +43,9 @@ public final class JsInteropUtil {

public static void maybeSetJsInteropProperties(JMethod method, Annotation... annotations) {
setJsInteropProperties(method, annotations);
method.setJsProperty(JdtUtil.getAnnotation(annotations, JSPROPERTY_CLASS) != null);
if (JdtUtil.getAnnotation(annotations, JSPROPERTY_CLASS) != null) {
setJsPropertyProperties(method);
}
}

public static void maybeSetJsInteropProperties(JField field, Annotation... annotations) {
Expand All @@ -64,7 +69,6 @@ private static void setJsInteropProperties(JMember member, Annotation... annotat
JDeclaredType enclosingType = member.getEnclosingType();

if (enclosingType.isJsType() && member.needsVtable()) {
// TODO(goktug): correctly calculate member names for JsProperties.
member.setJsMemberName(member.getName());
}

Expand All @@ -73,6 +77,35 @@ private static void setJsInteropProperties(JMember member, Annotation... annotat
}
}

private static void setJsPropertyProperties(JMethod method) {
String methodName = method.getName();
if (method.getParams().isEmpty()) {
if (startsWithCamelCase(method.getName(), "has")) {
// Has
method.setJsPropertyType(JsPropertyType.HAS);
method.setJsMemberName(Introspector.decapitalize(methodName.substring(3)));
} else {
// Getter
method.setJsPropertyType(JsPropertyType.GET);
if (startsWithCamelCase(methodName, "is")) {
method.setJsMemberName(Introspector.decapitalize(methodName.substring(2)));
} else if (startsWithCamelCase(methodName, "get")) {
method.setJsMemberName(Introspector.decapitalize(methodName.substring(3)));
} else {
method.setJsMemberName(methodName);
}
}
} else {
// Setter
method.setJsPropertyType(JsPropertyType.SET);
if (startsWithCamelCase(methodName, "set")) {
method.setJsMemberName(Introspector.decapitalize(methodName.substring(3)));
} else {
method.setJsMemberName(methodName);
}
}
}

// TODO(goktug): Move other namespace logic to here as well after we get access to package
// annotations in GwtAstBuilder.
private static void setExportInfo(JMember member, String exportName) {
Expand Down Expand Up @@ -116,4 +149,9 @@ public static String maybeGetJsTypePrototype(TypeDeclaration x) {
AnnotationBinding jsType = JdtUtil.getAnnotation(x.annotations, JSTYPE_CLASS);
return JdtUtil.getAnnotationParameterString(jsType, "prototype");
}

private static boolean startsWithCamelCase(String string, String prefix) {
return string.length() > prefix.length() && string.startsWith(prefix)
&& Character.isUpperCase(string.charAt(prefix.length()));
}
}
48 changes: 44 additions & 4 deletions dev/core/src/com/google/gwt/dev/jjs/ast/JMethod.java
View file
Expand Up @@ -37,16 +37,25 @@
*/
public class JMethod extends JNode implements JMember, CanBeAbstract, CanBeNative {

/**
* Indicates whether a JsProperty method is a getter or setter. Getters come with names like x(),
* isX(), hasX() and getX() while setters have signatures like x(int a) and setX(int a).
*/
public static enum JsPropertyType {
HAS, GET, SET
}

public static final Comparator<JMethod> BY_SIGNATURE_COMPARATOR = new Comparator<JMethod>() {
@Override
public int compare(JMethod m1, JMethod m2) {
return m1.getSignature().compareTo(m2.getSignature());
}
};

private String jsTypeName;
private String exportName;
private String exportNamespace;
private boolean jsProperty;
private JsPropertyType jsPropertyType;
private Specialization specialization;
private boolean inliningAllowed = true;
private boolean hasSideEffects = true;
Expand Down Expand Up @@ -97,6 +106,21 @@ public String getJsMemberName() {
return jsTypeName;
}

public String getImmediateOrTransitiveJsMemberName() {
String jsMemberName = getJsMemberName();
for (JMethod override : getOverriddenMethods()) {
String jsMemberOverrideName = override.getJsMemberName();
if (jsMemberOverrideName == null) {
continue;
}
if (jsMemberName != null && !jsMemberName.equals(jsMemberOverrideName)) {
return null;
}
jsMemberName = jsMemberOverrideName;
}
return jsMemberName;
}

@Override
public boolean isJsTypeMember() {
return jsTypeName != null;
Expand All @@ -114,12 +138,28 @@ public boolean isOrOverridesJsTypeMethod() {
return false;
}

public void setJsProperty(boolean jsProperty) {
this.jsProperty = jsProperty;
public void setJsPropertyType(JsPropertyType jsPropertyType) {
this.jsPropertyType = jsPropertyType;
}

public JsPropertyType getJsPropertyType() {
return jsPropertyType;
}

public JsPropertyType getImmediateOrTransitiveJsPropertyType() {
if (isJsProperty()) {
return getJsPropertyType();
}
for (JMethod overriddenMethod : getOverriddenMethods()) {
if (overriddenMethod.isJsProperty()) {
return overriddenMethod.getJsPropertyType();
}
}
return null;
}

public boolean isJsProperty() {
return jsProperty;
return jsPropertyType != null;
}

public boolean isOrOverridesJsProperty() {
Expand Down
7 changes: 7 additions & 0 deletions dev/core/src/com/google/gwt/dev/jjs/ast/JTypeOracle.java
View file
Expand Up @@ -1137,6 +1137,13 @@ public boolean isJsTypeMethod(JMethod x) {
return isJsInteropEnabled() && x.isOrOverridesJsTypeMethod();
}

/**
* Returns whether the given method is directly marked with an @JsProperty annotation.
*/
public boolean isJsPropertyMethod(JMethod x) {
return isJsInteropEnabled() && x.isJsProperty();
}

/**
* Returns whether the given field is exported by an @JsType annotation.
* <p>
Expand Down
55 changes: 19 additions & 36 deletions dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
View file
Expand Up @@ -573,7 +573,7 @@ public boolean visit(JMethod x, Context ctx) {
polyName.setObfuscatable(false);
// if a JsType and we can set set the interface method to non-obfuscatable
} else if (typeOracle.isJsTypeMethod(x) && !typeOracle.needsJsInteropBridgeMethod(x)) {
if (x.isJsProperty()) {
if (x.isOrOverridesJsProperty()) {
// Prevent JsProperty functions like x() from colliding with intended JS native
// properties like .x;
polyName = interfaceScope.declareName(mangleNameForJsProperty(x), name);
Expand Down Expand Up @@ -1422,17 +1422,18 @@ public void endVisit(JMethodCall x, Context ctx) {

if (isJsProperty) {
JsExpression qualExpr = pop();
String methodName = method.getName();
if (method.getParams().size() == 0) {
if (startsWithCamelCase(methodName, "has")) {
result = createHasDispatch(x, methodName, qualExpr);
} else {
result = createGetterDispatch(x, unnecessaryQualifier, methodName, qualExpr);
}
} else if (method.getParams().size() == 1) {
result = createSetterDispatch(x, jsInvocation, method, qualExpr);
} else {
throw new InternalCompilerException("JsProperty not a setter, getter, or has.");
switch (method.getImmediateOrTransitiveJsPropertyType()) {
case HAS:
result = createHasDispatch(x, method, qualExpr);
break;
case GET:
result = createGetterDispatch(x, unnecessaryQualifier, method, qualExpr);
break;
case SET:
result = createSetterDispatch(x, jsInvocation, method, qualExpr);
break;
default:
throw new InternalCompilerException("JsProperty not a setter, getter, or has.");
}
} else {
// insert trampoline (_ = instance, trampoline(_, _.jsBridgeMethRef,
Expand Down Expand Up @@ -1514,23 +1515,21 @@ private JsName createTmpLocal() {
return tmpName;
}

private JsExpression createHasDispatch(JMethodCall x, String methodName,
private JsExpression createHasDispatch(JMethodCall x, JMethod targetMethod,
JsExpression qualExpr) {
String propertyName = toCamelCase(methodName.substring(3));
String propertyName = targetMethod.getImmediateOrTransitiveJsMemberName();
JsStringLiteral propertyNameLiteral = new JsStringLiteral(x.getSourceInfo(), propertyName);
return new JsBinaryOperation(x.getSourceInfo(), JsBinaryOperator.INOP, propertyNameLiteral,
qualExpr);
}

private JsExpression createSetterDispatch(JMethodCall x, JsInvocation jsInvocation,
JMethod targetMethod, JsExpression qualExpr) {
String methodName = targetMethod.getName();
JType returnType = targetMethod.getType();
boolean fluent = returnType instanceof JReferenceType
&& returnType != program.getTypeJavaLangObject() && typeOracle.canTriviallyCast(
x.getTarget().getEnclosingType(), returnType.getUnderlyingType());
String propertyName = startsWithCamelCase(methodName, "set") ? toCamelCase(
methodName.substring(3)) : methodName;
String propertyName = targetMethod.getImmediateOrTransitiveJsMemberName();

JsExpression result;
JsNameRef propertyReference = new JsNameRef(x.getSourceInfo(), propertyName);
Expand All @@ -1548,10 +1547,8 @@ private JsExpression createSetterDispatch(JMethodCall x, JsInvocation jsInvocati
}

private JsExpression createGetterDispatch(JMethodCall x, JsExpression unnecessaryQualifier,
String methodName, JsExpression qualExpr) {
String propertyName = startsWithCamelCase(methodName, "is") ? toCamelCase(
methodName.substring(2)) : startsWithCamelCase(methodName, "get") ? toCamelCase(
methodName.substring(3)) : methodName;
JMethod targetMethod, JsExpression qualExpr) {
String propertyName = targetMethod.getImmediateOrTransitiveJsMemberName();
JsNameRef propertyReference = new JsNameRef(x.getSourceInfo(), propertyName);
propertyReference.setQualifier(qualExpr);
return createCommaExpression(unnecessaryQualifier, propertyReference);
Expand Down Expand Up @@ -3477,7 +3474,7 @@ String mangleNameForPrivatePoly(JMethod x) {
}

String mangleNameForJsProperty(JMethod x) {
assert x.isJsProperty();
assert x.isOrOverridesJsProperty();
StringBuilder sb = new StringBuilder();
sb.append("jsproperty$");
sb.append(JjsUtils.getNameString(x));
Expand Down Expand Up @@ -3535,20 +3532,6 @@ private void contructTypeToClassLiteralDeclarationMap() {
}
}

private static boolean startsWithCamelCase(String string, String prefix) {
return string.length() > prefix.length() && string.startsWith(prefix)
&& Character.isUpperCase(string.charAt(prefix.length()));
}

private static String toCamelCase(String string) {
if (string.length() == 0) {
return string;
} else if (string.length() == 1) {
return String.valueOf(Character.toLowerCase(string.charAt(0)));
}
return Character.toLowerCase(string.charAt(0)) + string.substring(1);
}

private Pair<JavaToJavaScriptMap, Set<JsNode>> execImpl() {
new FixNameClashesVisitor().accept(program);
uninitializedValuePotentiallyObservable = optimize ?
Expand Down

0 comments on commit 72b2e63

Please sign in to comment.