Skip to content

Commit

Permalink
Removes auto conversion of longs to double in jsinterop
Browse files Browse the repository at this point in the history
Such implicit conversion is dangerous for many apps.
The new implementation of LongEmul handles all cases
where such conversion is safe.

Change-Id: I2b38db281f65ec71b5facd7c24a28171fe30b1fd
Review-Link: https://gwt-review.googlesource.com/#/c/13473/
  • Loading branch information
gkdn committed Sep 5, 2015
1 parent 67a31be commit cb823e4
Show file tree
Hide file tree
Showing 6 changed files with 24 additions and 97 deletions.
18 changes: 2 additions & 16 deletions dev/core/src/com/google/gwt/dev/jjs/ast/JTypeOracle.java
Expand Up @@ -133,10 +133,8 @@ public static StandardTypes createFrom(JProgram program) {
}

/**
* A method needs a JsInterop bridge if any of the following are true:
* 1) the method name conflicts with a method name of a non-JsType/JsExport method in a superclass
* 2) the method returns or accepts Single-Abstract-Method types
* 3) the method returns or accepts JsAware/JsConvert types.
* A method needs a JsInterop bridge if the method name conflicts with a method name of a
* non-JsType/JsExport method in a superclass.
*/
public boolean needsJsInteropBridgeMethod(JMethod x) {
/*
Expand Down Expand Up @@ -209,18 +207,6 @@ public boolean needsJsInteropBridgeMethod(JMethod x) {
}
}

// implicit builtin @JsConvert, longs are converted
if (x.isOrOverridesJsMethod()) {
if (x.getOriginalReturnType() == JPrimitiveType.LONG) {
return true;
}
for (JParameter p : xParams) {
if (p.getType() == JPrimitiveType.LONG) {
return true;
}
}
}
// TODO (cromwellian): add SAM and JsAware/Convert cases in follow up
return false;
}

Expand Down
Expand Up @@ -2024,8 +2024,7 @@ private void generateExports(List<JsStatement> globalStmts) {
} else {
JMember member = (JMember) exportedEntity;
maybeHoistClinit(globalStmts, generatedClinits, member);
exportGenerator.exportMember(member, createBridgeMethodOrReturnAlias(member,
names.get(member)));
exportGenerator.exportMember(member, createAlias(member, names.get(member)));
}
}
}
Expand Down Expand Up @@ -2777,7 +2776,7 @@ private void generateVTables(JClassType x, List<JsStatement> globalStmts) {
// _.exportedName = [obfuscatedMethodReference | function() { bridge code }]
exportedName.setObfuscatable(false);
generateVTableAssignment(globalStmts, method, exportedName,
createBridgeMethodOrReturnAlias(method, polyJsName));
createAlias(method, polyJsName));
}
if (method.exposesOverriddenPackagePrivateMethod()) {
// This method exposes a package private method that is actually live, hence it needs
Expand Down Expand Up @@ -2874,52 +2873,20 @@ private void collectExports(JDeclaredType x) {
* function is generated which invokes the method and coerces each argument or return type
* as needed.
*/
JsExpression createBridgeMethodOrReturnAlias(JMember member, JsName aliasedMethodName) {
JsExpression createAlias(JMember member, JsName aliasedMethodName) {
SourceInfo info = member.getSourceInfo();
if (member instanceof JConstructor || member instanceof JField) {
return aliasedMethodName.makeRef(info);
} else {
JMethod method = (JMethod) member;
boolean needsLongConversions = method.getType() == JPrimitiveType.LONG ||
Iterables.any(method.getParams(), new Predicate<JParameter>() {
@Override
public boolean apply(JParameter jParameter) {
return jParameter.getType() == JPrimitiveType.LONG;
}
});

JsNameRef aliasedMethodRef = aliasedMethodName.makeRef(info);

if (!needsLongConversions) {
if (!method.isStatic()) {
// simply return Foo.prototype.aliasedMethod or _.aliasedMethod
aliasedMethodRef.setQualifier(getPrototypeQualifierOf(method));
}
return aliasedMethodRef;
}

// Construct function($arg1, $arg2, ...){ return this.meth($arg1, $arg2); }
JsFunction bridgeMethod = JsUtils.createEmptyFunctionLiteral(info, topScope, null);

// Bridge requires conversions.
// return coerceFromLong([this.]?aliasedMethodRef(coerceToLong(arg1), arg2, ...))
if (!method.isStatic()) {
aliasedMethodRef.setQualifier(new JsThisRef(info));
}
JsInvocation aliasedMethodInvocation = new JsInvocation(info, aliasedMethodRef);
for (JParameter param : method.getParams()) {
JsName argJsName = bridgeMethod.getScope().declareName("$arg_" + param.getName());
bridgeMethod.getParameters().add(new JsParameter(info, argJsName));
aliasedMethodInvocation.getArguments().add(param.getType() == JPrimitiveType.LONG ?
constructInvocation(info, "JavaClassHierarchySetupUtil.coerceToLong",
argJsName.makeRef(info)) : argJsName.makeRef(info));
// simply return Foo.prototype.aliasedMethod or _.aliasedMethod
aliasedMethodRef.setQualifier(getPrototypeQualifierOf(method));
}

bridgeMethod.getBody().getStatements().add(new JsReturn(info,
method.getType() == JPrimitiveType.LONG ? constructInvocation(info,
"JavaClassHierarchySetupUtil.coerceFromLong", aliasedMethodInvocation) :
aliasedMethodInvocation));
return bridgeMethod;
return aliasedMethodRef;
}
}

Expand Down
Expand Up @@ -215,21 +215,4 @@ static native void emptyMethod() /*-{
static native JavaScriptObject uniqueId(String id) /*-{
return jsinterop.closure.getUniqueId(id);
}-*/;

/**
* Converts an input object (LongEmul) to a double, otherwise return the value.
*/
public static native Object coerceToLong(Object o) /*-{
if (typeof(o) == 'number') {
return @com.google.gwt.lang.LongLib::fromDouble(D)(o);
}
return o;
}-*/;

/**
* Convert a double to a long.
*/
public static native Object coerceFromLong(Object o) /*-{
return @com.google.gwt.lang.LongLib::toDouble(*)(o);
}-*/;
}
14 changes: 14 additions & 0 deletions user/test/com/google/gwt/core/client/interop/JsExportTest.java
Expand Up @@ -66,6 +66,20 @@ public void testMethodExport() {
assertTrue(MyClassExportsMethod.calledFromCallMe1);
}

public void testMethodExportWithLong() {
assertEquals(42.0, callLongMethod(40.0, 2.0));
assertEquals(82.0, callStaticLongMethod(80.0, 2.0));
}

private native double callLongMethod(double a, double b) /*-{
var obj = new $global.woo.MyJsTypeThatUsesLongType();
return obj.addLong(a,b);
}-*/;

private native double callStaticLongMethod(double a, double b) /*-{
return $global.woo.MyJsTypeThatUsesLongType.addLongStatic(a,b);
}-*/;

public void testMethodExport_noTypeTightenParams() {

// If we type-tighten, java side will see no calls and think that parameter could only be null.
Expand Down
16 changes: 0 additions & 16 deletions user/test/com/google/gwt/core/client/interop/JsTypeTest.java
Expand Up @@ -348,22 +348,6 @@ public void testEnumSubclassEnumeration() {
assertEquals(1, callPublicMethodFromEnumerationSubclass(MyEnumWithSubclassGen.C));
}

public void testBridgeMethodLongCoercion() {
assertEquals(42.0, callLongMethod(40.0, 2.0));
assertEquals(82.0, callStaticLongMethod(80.0, 2.0));
}

private native double callLongMethod(double a, double b) /*-{
var global = window.goog && window.goog.global || $wnd;
var bridgeMethodClass = global.createLongCoercionBridgeMethod();
return bridgeMethodClass.addLong(a,b);
}-*/;

private native double callStaticLongMethod(double a, double b) /*-{
var global = window.goog && window.goog.global || $wnd;
return global.addLongStatic(a,b);
}-*/;

private static native boolean alwaysTrue() /*-{
return !!$wnd;
}-*/;
Expand Down
Expand Up @@ -16,26 +16,19 @@
package com.google.gwt.core.client.interop;

import com.google.gwt.core.client.js.JsExport;
import com.google.gwt.core.client.js.JsNamespace;
import com.google.gwt.core.client.js.JsType;

/**
* A class with a method that takes and returns longs.
*/
@JsType
public class ClassWithLongCoercionBridgeMethod {
@JsNamespace(JsNamespace.GLOBAL)
@JsExport("createLongCoercionBridgeMethod")
public static ClassWithLongCoercionBridgeMethod create() {
return new ClassWithLongCoercionBridgeMethod();
}
@JsExport
public class MyJsTypeThatUsesLongType {

public long addLong(long a, long b) {
return a + b;
}

@JsNamespace(JsNamespace.GLOBAL)
@JsExport("addLongStatic")
public static long addLongStatic(long a, long b) {
return a + b;
}
Expand Down

0 comments on commit cb823e4

Please sign in to comment.