Skip to content

Commit

Permalink
Fixes 3 bugs in JsProperty and adds tests.
Browse files Browse the repository at this point in the history 
Getters of the form "getFoo()" were being improperly allowed to be 
declared to return void. Fixed.

Is functions of the form "isFoo()" were being improperly translated to 
JS as reads and writes of the "oo" property instead of the "foo" 
property, which lead to failures to round trip values between 
"setFoo()" and "isFoo()". Fixed.

Has function translation to JS was broken since "bar.hasFoo()" became
"foo in bar" rather than "'foo' in bar". This slight syntax error 
lead to undefined property errors at runtime for any property that did
not yet have a value.

Change-Id: I24cedfd4ff1beb085cd8343a5a8966e34d3c6b02
Review-Link: https://gwt-review.googlesource.com/#/c/11740/
  • Loading branch information
@stalcup
stalcup committed Feb 20, 2015
1 parent 0b8e1d1 commit f8bbd15
Show file tree
Hide file tree
Showing 4 changed files with 175 additions and 85 deletions.
138 changes: 58 additions & 80 deletions dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
View file
Expand Up @@ -1408,41 +1408,33 @@ public void endVisit(JMethodCall x, Context ctx) {
} else {
JsName polyName = polymorphicNames.get(method);
// potentially replace method call with property access
JMethod target = x.getTarget();
for (JMethod overrideMethod : target.getOverriddenMethods()) {
JMethod targetMethod = x.getTarget();
for (JMethod overrideMethod : targetMethod.getOverriddenMethods()) {
if (overrideMethod.isJsProperty()) {
isJsProperty = true;
break;
}
}
if (isJsProperty || target.isJsProperty()) {
String getter = isGetter(target);
String setter = isSetter(target);
String has = isHas(target);

// if fluent
JType type = target.getType();

boolean isFluent = type instanceof JReferenceType
&& type != program.getTypeJavaLangObject() && typeOracle.canTriviallyCast(
x.getTarget().getEnclosingType(), type.getUnderlyingType());
if (isJsProperty || targetMethod.isJsProperty()) {
JsExpression qualExpr = pop();

if (getter != null) {
result = dispatchAsGetter(x, unnecessaryQualifier, getter, qualExpr);
} else if (setter != null) {
result = dispatchAsSetter(x, jsInvocation, setter, isFluent, qualExpr);
} else if (has != null) {
result = dispatchAsHas(x, has, qualExpr);
String methodName = targetMethod.getName();
if (targetMethod.getParams().size() == 0) {
if (startsWithCamelCase(methodName, "has")) {
result = createHasDispatch(x, methodName, qualExpr);
} else {
result = createGetterDispatch(x, unnecessaryQualifier, methodName, qualExpr);
}
} else if (targetMethod.getParams().size() == 1) {
result = createSetterDispatch(x, jsInvocation, targetMethod, qualExpr);
} else {
throw new InternalCompilerException("JsProperty not a setter, getter, or has.");
}
} else {
// insert trampoline (_ = instance, trampoline(_, _.jsBridgeMethRef,
// _.javaMethRef)).bind(_)(args)
if (typeOracle.needsJsInteropBridgeMethod(method)) {
maybeDispatchViaTrampolineToBridgeMethod(x, method, jsInvocation,
unnecessaryQualifier, result, polyName);
maybeDispatchViaTrampolineToBridgeMethod(x, method, jsInvocation, unnecessaryQualifier,
result, polyName);

return;
} else {
Expand Down Expand Up @@ -1517,35 +1509,47 @@ private JsName createTmpLocal() {
return tmpName;
}

private JsExpression dispatchAsHas(JMethodCall x, String has, JsExpression qualExpr) {
JsExpression result;JsNameRef property = new JsNameRef(x.getSourceInfo(), has);
result = new JsBinaryOperation(x.getSourceInfo(), JsBinaryOperator.INOP,
property, qualExpr);
return result;
private JsExpression createHasDispatch(JMethodCall x, String methodName,
JsExpression qualExpr) {
String propertyName = toCamelCase(methodName.substring(3));
JsStringLiteral propertyNameLiteral = new JsStringLiteral(x.getSourceInfo(), propertyName);
return new JsBinaryOperation(x.getSourceInfo(), JsBinaryOperator.INOP, propertyNameLiteral,
qualExpr);
}

private JsExpression dispatchAsSetter(JMethodCall x, JsInvocation jsInvocation, String setter, boolean isFluent, JsExpression qualExpr) {
JsExpression result;JsNameRef property = new JsNameRef(x.getSourceInfo(), setter);
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;

JsExpression result;
JsNameRef propertyReference = new JsNameRef(x.getSourceInfo(), propertyName);
// either qualExpr.prop or _.prop depending on fluent or not
property.setQualifier(isFluent ? globalTemp.makeRef(x.getSourceInfo()) : qualExpr);
propertyReference.setQualifier(fluent ? globalTemp.makeRef(x.getSourceInfo()) : qualExpr);
// propExpr = arg
result = createAssignment(property, jsInvocation.getArguments().get(0));
if (isFluent) {
result = createAssignment(propertyReference, jsInvocation.getArguments().get(0));
if (fluent) {
// (_ = qualExpr, _.prop = arg, _)
result = createCommaExpression(
createAssignment(globalTemp.makeRef(x.getSourceInfo()), qualExpr),
createCommaExpression(result,
globalTemp.makeRef(x.getSourceInfo())));
createCommaExpression(result, globalTemp.makeRef(x.getSourceInfo())));
}
return result;
}

private JsExpression dispatchAsGetter(JMethodCall x, JsExpression unnecessaryQualifier, String getter, JsExpression qualExpr) {
JsExpression result;// replace with qualExpr.property
JsNameRef property = new JsNameRef(x.getSourceInfo(), getter);
property.setQualifier(qualExpr);
result = createCommaExpression(unnecessaryQualifier, property);
return result;
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;
JsNameRef propertyReference = new JsNameRef(x.getSourceInfo(), propertyName);
propertyReference.setQualifier(qualExpr);
return createCommaExpression(unnecessaryQualifier, propertyReference);
}

/**
Expand Down Expand Up @@ -1582,46 +1586,6 @@ private JsExpression dispatchToSuperPrototype(JMethodCall x, JMethod method, JsN
return JsNullLiteral.INSTANCE;
}

private String isGetter(JMethod method) {
String name = method.getName();
// zero arg non-void getX()
if (name.length() > 3 && name.startsWith("get") && Character.isUpperCase(name.charAt(3)) &&
method.getType() != JPrimitiveType.VOID && method.getParams().size() == 0) {
String propName = Character.toLowerCase(name.charAt(3)) + name.substring(4);
return propName;
} else if (name.length() > 3 && name.startsWith("is")
&& Character.isUpperCase(name.charAt(2)) && method.getType() == JPrimitiveType.BOOLEAN
&& method.getParams().size() == 0) {
String propName = Character.toLowerCase(name.charAt(3)) + name.substring(4);
return propName;
} else if (method.getParams().size() == 0 && method.getType() != JPrimitiveType.VOID) {
return name;
}
return null;
}

private String isSetter(JMethod method) {
String name = method.getName();
if (name.length() > 3 && name.startsWith("set") && Character.isUpperCase(name.charAt(3))
&& method.getParams().size() == 1) {
String propName = Character.toLowerCase(name.charAt(3)) + name.substring(4);
return propName;
} else if (method.getParams().size() == 1) {
return name;
}
return null;
}

private String isHas(JMethod method) {
String name = method.getName();
if (name.length() > 3 && name.startsWith("has") && Character.isUpperCase(name.charAt(3))
&& method.getParams().size() == 0 && method.getType() == JPrimitiveType.BOOLEAN) {
String propName = Character.toLowerCase(name.charAt(3)) + name.substring(4);
return propName;
}
return null;
}

@Override
public void endVisit(JMultiExpression x, Context ctx) {
List<JsExpression> exprs = popList(x.getNumberOfExpressions());
Expand Down Expand Up @@ -3638,6 +3602,20 @@ 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
15 changes: 10 additions & 5 deletions user/src/com/google/gwt/core/client/js/JsProperty.java
View file
Expand Up @@ -22,17 +22,22 @@
import java.lang.annotation.Target;

/**
* JsProperty marks a method in a {@link JsType} as a property accessor and recognizes
* JavaBean style naming convention. Instead of translating method calls to JsProperty methods
* as method calls in JS, they will be replaced with dotted property lookups.
*<p> Examples:
* JsProperty marks a method in a {@link JsType} as a property accessor and recognizes JavaBean
* style naming convention. Instead of translating method calls to JsProperty methods as method
* calls in JS, they will be replaced with dotted property lookups.
* <p>
* In case of JsType with JsProperties implemented by Java classes, the property access still
* triggers the execution of the matching getter or setter methods as they will be translated into
* custom property setter and getter in JavaScript.
* <p>
* Examples:
* <ul>
* <li> {@code @JsProperty getX()} translates as <tt>this.x</tt>
* <li> {@code @JsProperty x()} translates as <tt>this.x</tt>
* <li> {@code @JsProperty setX(int y)} translates as <tt>this.x=y</tt>
* <li> {@code @JsProperty x(int x)} translates as <tt>this.x=y</tt>
* <li> {@code @JsProperty hasX(int x)} translates as <tt>x in this</tt>
*</ul>
* </ul>
* <p>
* In addition, fluent style <tt>return this</tt> syntax is supported for setters, so
* {@code @JsProperty T setX(int x)} translates as <tt>this.x=x, return this</tt>.
Expand Down
62 changes: 62 additions & 0 deletions user/test/com/google/gwt/core/client/interop/JsPoint.java
View file
@@ -0,0 +1,62 @@
/*
* Copyright 2015 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations under
* the License.
*/
package com.google.gwt.core.client.interop;

import com.google.gwt.core.client.js.JsProperty;
import com.google.gwt.core.client.js.JsType;

/**
* An interface to model a raw JS Point type.
*/
@JsType
public interface JsPoint {

@JsProperty
boolean hasX();

@JsProperty
boolean isX();

@JsProperty
int getX();

@JsProperty
int x();

@JsProperty
void setX(int x);

@JsProperty
JsPoint x(int x);

@JsProperty
boolean hasY();

@JsProperty
boolean isY();

@JsProperty
int getY();

@JsProperty
int y();

@JsProperty
void setY(int y);

@JsProperty
JsPoint y(int y);
}
45 changes: 45 additions & 0 deletions user/test/com/google/gwt/core/client/interop/JsTypeTest.java
View file
Expand Up @@ -17,6 +17,7 @@

import static com.google.gwt.core.client.ScriptInjector.TOP_WINDOW;

import com.google.gwt.core.client.JavaScriptObject;
import com.google.gwt.core.client.ScriptInjector;
import com.google.gwt.junit.client.GWTTestCase;

Expand Down Expand Up @@ -129,6 +130,46 @@ public void testJsProperties() {
assertEquals(58, mc.sum(0));
}

public void testJsPropertyIsX() {
JsPoint point = (JsPoint) JavaScriptObject.createObject();

assertFalse(point.isX());
point.setX(10);
assertTrue(point.isX());
point.y(999).x(0);
assertFalse(point.isX());
}

public void testJsPropertyHasX() {
JsPoint point = (JsPoint) JavaScriptObject.createObject();

assertFalse(point.hasX());
point.setX(10);
assertTrue(point.hasX());
point.y(999).x(0);
assertTrue(point.hasX());
}

public void testJsPropertyGetX() {
JsPoint point = (JsPoint) JavaScriptObject.createObject();

assertTrue(isUndefined(point.getX()));
point.setX(10);
assertEquals(10, point.getX());
point.y(999).x(0);
assertEquals(0, point.getX());
}

public void testJsPropertyX() {
JsPoint point = (JsPoint) JavaScriptObject.createObject();

assertTrue(isUndefined(point.x()));
point.setX(10);
assertEquals(10, point.x());
point.y(999).x(0);
assertEquals(0, point.x());
}

public void testCasts() {
MyJsInterface myClass;
assertNotNull(myClass = (MyJsInterface) createMyJsInterface());
Expand Down Expand Up @@ -245,6 +286,10 @@ private static native Object createMyWrongNamespacedJsInterface() /*-{
return new $wnd['testfoo.bar.MyJsInterface']();
}-*/;

private static native boolean isUndefined(int value) /*-{
return value === undefined;
}-*/;

private static native boolean hasField(Object object, String fieldName) /*-{
return object[fieldName] != undefined;
}-*/;
Expand Down

0 comments on commit f8bbd15

Please sign in to comment.