Skip to content

Commit

Permalink
Adds more JsInterop verifications and tests.
Browse files Browse the repository at this point in the history 
Now verifies that getters and setters for a JsProperty operate on a 
consistent type.

Detects and prevents @jstype interfaces from extending non-@jstype 
interfaces.

Change-Id: I728ab4e93b886dd726450ffa10963f7f4fa19f12
Review-Link: https://gwt-review.googlesource.com/#/c/12080/
  • Loading branch information
@stalcup
stalcup committed Mar 13, 2015
1 parent 42b35b0 commit dd61498
Show file tree
Hide file tree
Showing 6 changed files with 294 additions and 181 deletions.
46 changes: 3 additions & 43 deletions dev/core/src/com/google/gwt/dev/javac/JSORestrictionsChecker.java
View file
Expand Up @@ -19,7 +19,6 @@
import com.google.gwt.dev.util.InstalledHelpInfo;
import com.google.gwt.dev.util.collect.Stack;
import com.google.gwt.thirdparty.guava.common.base.Strings;
import com.google.gwt.thirdparty.guava.common.collect.Lists;

import org.eclipse.jdt.core.compiler.CharOperation;
import org.eclipse.jdt.internal.compiler.ast.ASTNode;
Expand All @@ -42,7 +41,6 @@
import org.eclipse.jdt.internal.compiler.lookup.TypeBinding;

import java.util.HashMap;
import java.util.List;
import java.util.Map;

/**
Expand All @@ -69,8 +67,6 @@ public class JSORestrictionsChecker {
+ "static final fields in public classes.";
public static final String ERR_EITHER_JSEXPORT_JSNOEXPORT =
"@JsExport and @JsNoExport is not allowed at the same time.";
public static final String ERR_EXPLICIT_JSEXPORT_OR_JSNOEXPORT_ON_CONSTRUCTORS =
"@JsExport or @JsNoExport should be set on constructors if a class is @JsExported and has more than one non private constructors";
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_JSEXPORT_ON_ENUMERATION =
Expand All @@ -79,8 +75,6 @@ public class JSORestrictionsChecker {
"Classes implementing @JsType with a prototype must extend that interface's Prototype class";
public static final String ERR_CLASS_EXTENDS_MAGIC_PROTOTYPE_BUT_NO_PROTOTYPE_ATTRIBUTE =
"Classes implementing a @JsType without a prototype should not extend the Prototype class";
public static final String ERR_JSPROPERTY_ONLY_ON_INTERFACES =
"@JsProperty not allowed on concrete class methods";
public static final String ERR_CONSTRUCTOR_WITH_PARAMETERS =
"Constructors must not have parameters in subclasses of JavaScriptObject";
public static final String ERR_INSTANCE_FIELD = "Instance fields cannot be used in subclasses of JavaScriptObject";
Expand Down Expand Up @@ -289,39 +283,10 @@ private void checkJsType(TypeDeclaration type, TypeBinding typeBinding) {

checkJsTypeMethodsForOverloads(methodSignatures, noExports, binding);
for (MethodBinding mb : binding.methods()) {
checkJsProperty(mb, true);
checkJsProperty(mb);
}
}

private void checkJsExport(ReferenceBinding rb) {
if (JdtUtil.getAnnotation(rb, JsInteropUtil.JSEXPORT_CLASS) == null) {
return;
}
List<MethodBinding> publicConstructors = getPublicConstructors(rb);
if (publicConstructors.size() <= 1) {
return;
}
for (MethodBinding mb : publicConstructors) {
AnnotationBinding jsexportAnnotation =
JdtUtil.getAnnotation(mb, JsInteropUtil.JSEXPORT_CLASS);
AnnotationBinding jsnoexportAnnotation =
JdtUtil.getAnnotation(mb, JsInteropUtil.JSNOEXPORT_CLASS);
if (jsexportAnnotation == null && jsnoexportAnnotation == null) {
errorOn(mb, ERR_EXPLICIT_JSEXPORT_OR_JSNOEXPORT_ON_CONSTRUCTORS);
}
}
}

private List<MethodBinding> getPublicConstructors(ReferenceBinding rb) {
List<MethodBinding> publicConstructors = Lists.newArrayList();
for (MethodBinding mb : rb.methods()) {
if (mb.isConstructor() && mb.isPublic()) {
publicConstructors.add(mb);
}
}
return publicConstructors;
}

private void checkJsExport(MethodBinding mb) {
if (JdtUtil.getAnnotation(mb, JsInteropUtil.JSEXPORT_CLASS) != null) {
boolean isStatic = mb.isConstructor() || mb.isStatic();
Expand Down Expand Up @@ -354,13 +319,9 @@ private boolean isEnumConstant(FieldDeclaration fd) {
&& ((AllocationExpression) fd.initialization).enumConstant != null);
}

private void checkJsProperty(MethodBinding mb, boolean allowed) {
private void checkJsProperty(MethodBinding mb) {
AnnotationBinding jsProperty = JdtUtil.getAnnotation(mb, JsInteropUtil.JSPROPERTY_CLASS);
if (jsProperty != null) {
if (!allowed) {
errorOn(mb, ERR_JSPROPERTY_ONLY_ON_INTERFACES);
return;
}
String methodName = String.valueOf(mb.selector);
if (!isGetter(methodName, mb) && !isSetter(methodName, mb)) {
errorOn(mb, ERR_JSPROPERTY_ONLY_BEAN_OR_FLUENT_STYLE_NAMING);
Expand Down Expand Up @@ -407,7 +368,6 @@ private void checkJsTypeMethodsForOverloads(Map<String, MethodBinding> methodNam
private ClassState checkType(TypeDeclaration type) {
SourceTypeBinding binding = type.binding;
checkJsFunction(type, binding);
checkJsExport(binding);
if (isJsType(type.binding)) {
checkJsType(type, type.binding);
return ClassState.JSTYPE;
Expand Down Expand Up @@ -453,7 +413,7 @@ private boolean checkClassImplementingJsType(TypeDeclaration type) {
}

for (MethodBinding mb : type.binding.methods()) {
checkJsProperty(mb, false);
checkJsProperty(mb);
}

AnnotationBinding jsinterfaceAnn = JdtUtil.getAnnotation(jsInterface,
Expand Down
79 changes: 59 additions & 20 deletions dev/core/src/com/google/gwt/dev/jjs/impl/JsInteropRestrictionChecker.java
View file
Expand Up @@ -24,7 +24,9 @@
import com.google.gwt.dev.jjs.ast.JMethod;
import com.google.gwt.dev.jjs.ast.JMethod.JsPropertyType;
import com.google.gwt.dev.jjs.ast.JProgram;
import com.google.gwt.dev.jjs.ast.JType;
import com.google.gwt.dev.jjs.ast.JVisitor;
import com.google.gwt.thirdparty.guava.common.collect.Iterables;
import com.google.gwt.thirdparty.guava.common.collect.Maps;
import com.google.gwt.thirdparty.guava.common.collect.Sets;

Expand All @@ -37,9 +39,7 @@
*/
// TODO: handle custom JsType field/method names when that feature exists.
// TODO: move JsInterop checks from JSORestrictionsChecker to here.
// TODO: check setter and getter for the same property has compatible types.
// TODO: check for name collisions between regular members and accidental-override methods.
// TODO: a JsType interface can only extend another JsType interface.
public class JsInteropRestrictionChecker extends JVisitor {

public static void exec(TreeLogger logger, JProgram jprogram,
Expand All @@ -52,10 +52,11 @@ public static void exec(TreeLogger logger, JProgram jprogram,
}
}

private Set<JMethod> currentJsTypeProcessedMethods;
private Map<String, String> currentJsTypeMethodNameByGetterNames;
private Map<String, String> currentJsTypeMethodNameByMemberNames;
private Map<String, String> currentJsTypeMethodNameBySetterNames;
private Set<JMethod> currentJsTypeProcessedMethods;
private Map<String, JType> currentJsTypePropertyTypeByName;
private JDeclaredType currentType;
private boolean hasErrors;
private final JProgram jprogram;
Expand All @@ -79,13 +80,19 @@ public void endVisit(JDeclaredType x, Context ctx) {
public boolean visit(JDeclaredType x, Context ctx) {
assert currentType == null;
currentJsTypeProcessedMethods = Sets.newHashSet();
currentJsTypePropertyTypeByName = Maps.newHashMap();
currentJsTypeMethodNameByMemberNames = Maps.newHashMap();
currentJsTypeMethodNameByGetterNames = Maps.newHashMap();
currentJsTypeMethodNameBySetterNames = Maps.newHashMap();
minimalRebuildCache.removeJsInteropNames(x.getName());
currentType = x;
checkJsFunctionInheritance(x);

checkJsFunctionHierarchy(x);
checkJsFunctionJsTypeCollision(x);
if (currentType instanceof JInterfaceType) {
checkJsTypeHierarchy((JInterfaceType) currentType);
}

// Perform custom class traversal to examine fields and methods of this class and all
// superclasses so that name collisions between local and inherited members can be found.
do {
Expand Down Expand Up @@ -122,12 +129,16 @@ public boolean visit(JMethod x, Context ctx) {
checkJsTypeMethod(x);
}

if (jprogram.typeOracle.isJsPropertyMethod(x)
&& !jprogram.typeOracle.isJsType(x.getEnclosingType())) {
logError("Method '%s' can't be explicitly annotated @JsProperty since enclosing type '%s' "
+ "is not explicitly annotated @JsType. If the method is already overriding "
+ "a JsProperty method then there is no need to restate the annotation.", x.getName(),
x.getEnclosingType().getName());
if (currentType == x.getEnclosingType()) {
if (jprogram.typeOracle.isJsPropertyMethod(x) && !jprogram.typeOracle.isJsType(currentType)) {
if (currentType instanceof JInterfaceType) {
logError("Method '%s' can't be a JsProperty since interface '%s' is not a JsType.",
x.getName(), x.getEnclosingType().getName());
} else {
logError("Method '%s' can't be a JsProperty since '%s' "
+ "is not an interface.", x.getName(), x.getEnclosingType().getName());
}
}
}

return false;
Expand All @@ -137,17 +148,38 @@ private void checkExportName(JMember x) {
boolean success = minimalRebuildCache.addExportedGlobalName(x.getQualifiedExportName(),
currentType.getName());
if (!success) {
logError("'%s' can't be exported because the global name '%s' is already taken.",
logError("Member '%s' can't be exported because the global name '%s' is already taken.",
x.getQualifiedName(), x.getQualifiedExportName());
}
}

private void checkInconsistentPropertyType(String propertyName, String enclosingTypeName,
JType parameterType) {
JType recordedType = currentJsTypePropertyTypeByName.put(propertyName, parameterType);
if (recordedType != null && recordedType != parameterType) {
logError("The setter and getter for JsProperty '%s' in type '%s' must have consistent types.",
propertyName, enclosingTypeName);
}
}

private void checkJsTypeHierarchy(JInterfaceType interfaceType) {
if (jprogram.typeOracle.isJsType(currentType)) {
for (JDeclaredType superInterface : interfaceType.getImplements()) {
if (!jprogram.typeOracle.isJsType(superInterface)) {
logWarning(
"JsType interface '%s' extends non-JsType interface '%s'. This is not recommended.",
interfaceType.getName(), superInterface.getName());
}
}
}
}

private void checkJsTypeFieldName(JField field, String memberName) {
boolean success =
currentJsTypeMethodNameByMemberNames.put(memberName, field.getQualifiedName()) == null;
if (!success) {
logError("'%s' can't be exported because the member name '%s' is already taken.",
field.getQualifiedName(), memberName);
logError("Field '%s' can't be exported in type '%s' because the member name "
+ "'%s' is already taken.", field.getQualifiedName(), currentType.getName(), memberName);
}
}

Expand Down Expand Up @@ -175,23 +207,26 @@ private void checkJsTypeMethod(JMethod method) {
// If it's a getter.
if (currentJsTypeMethodNameByGetterNames.put(jsMemberName, qualifiedMethodName) != null) {
// Don't allow multiple getters for the same property name.
logError("There can't be more than one getter for JS property '%s' in type '%s'.",
logError("There can't be more than one getter for JsProperty '%s' in type '%s'.",
jsMemberName, typeName);
}
checkNameCollisionForGetterAndRegular(jsMemberName, typeName);
checkInconsistentPropertyType(jsMemberName, typeName, method.getOriginalReturnType());
} else if (jsPropertyType == JsPropertyType.SET) {
// If it's a setter.
if (currentJsTypeMethodNameBySetterNames.put(jsMemberName, qualifiedMethodName) != null) {
// Don't allow multiple setters for the same property name.
logError("There can't be more than one setter for JS property '%s' in type '%s'.",
logError("There can't be more than one setter for JsProperty '%s' in type '%s'.",
jsMemberName, typeName);
}
checkNameCollisionForSetterAndRegular(jsMemberName, typeName);
checkInconsistentPropertyType(jsMemberName, typeName,
Iterables.getOnlyElement(method.getParams()).getType());
} else {
// If it's just an regular JsType method.
if (currentJsTypeMethodNameByMemberNames.put(jsMemberName, qualifiedMethodName) != null) {
logError("'%s' can't be exported because the member name '%s' is already taken.",
qualifiedMethodName, jsMemberName);
logError("Method '%s' can't be exported in type '%s' because the member name "
+ "'%s' is already taken.", qualifiedMethodName, currentType.getName(), jsMemberName);
}
checkNameCollisionForGetterAndRegular(jsMemberName, typeName);
checkNameCollisionForSetterAndRegular(jsMemberName, typeName);
Expand All @@ -201,7 +236,7 @@ private void checkJsTypeMethod(JMethod method) {
private void checkNameCollisionForGetterAndRegular(String getterName, String typeName) {
if (currentJsTypeMethodNameByGetterNames.containsKey(getterName)
&& currentJsTypeMethodNameByMemberNames.containsKey(getterName)) {
logError("The JsType member '%s' and JsProperty property '%s' name can't both be named "
logError("The JsType member '%s' and JsProperty '%s' can't both be named "
+ "'%s' in type '%s'.", currentJsTypeMethodNameByMemberNames.get(getterName),
currentJsTypeMethodNameByGetterNames.get(getterName), getterName, typeName);
}
Expand All @@ -210,13 +245,13 @@ private void checkNameCollisionForGetterAndRegular(String getterName, String typ
private void checkNameCollisionForSetterAndRegular(String setterName, String typeName) {
if (currentJsTypeMethodNameBySetterNames.containsKey(setterName)
&& currentJsTypeMethodNameByMemberNames.containsKey(setterName)) {
logError("The JsType member '%s' and JsProperty property '%s' name can't both be named "
logError("The JsType member '%s' and JsProperty '%s' can't both be named "
+ "'%s' in type '%s'.", currentJsTypeMethodNameByMemberNames.get(setterName),
currentJsTypeMethodNameBySetterNames.get(setterName), setterName, typeName);
}
}

private void checkJsFunctionInheritance(JDeclaredType type) {
private void checkJsFunctionHierarchy(JDeclaredType type) {
Collection<JInterfaceType> implementedJsFunctions =
jprogram.typeOracle.getImplementedJsFunctions(type);
if (implementedJsFunctions.size() > 1) {
Expand All @@ -238,4 +273,8 @@ private void logError(String format, Object... args) {
logger.log(TreeLogger.ERROR, String.format(format, args));
hasErrors = true;
}

private void logWarning(String format, Object... args) {
logger.log(TreeLogger.WARN, String.format(format, args));
}
}
27 changes: 0 additions & 27 deletions dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java
View file
Expand Up @@ -493,18 +493,6 @@ public void testJsExportOnClassWithMultipleConstructors() {
shouldGenerateNoError(goodCode);
}

public void testJsExportNotOnClassWithMultipleConstructors() {
StringBuilder buggyCode = new StringBuilder();
buggyCode.append("import com.google.gwt.core.client.js.JsExport;\n");
buggyCode.append("@JsExport public class Buggy {\n");
buggyCode.append(" public Buggy() { }\n");
buggyCode.append(" @JsExport(\"foo\") public Buggy(int a) { }\n");
buggyCode.append("}");

shouldGenerateError(buggyCode, "Line 3: "
+ JSORestrictionsChecker.ERR_EXPLICIT_JSEXPORT_OR_JSNOEXPORT_ON_CONSTRUCTORS);
}

public void testJsExportNotOnNonPublicClass() {
StringBuilder buggyCode = new StringBuilder();
buggyCode.append("import com.google.gwt.core.client.js.JsExport;\n");
Expand Down Expand Up @@ -611,21 +599,6 @@ public void testJsPrototypeNotOnClass() {
+ JSORestrictionsChecker.ERR_JS_TYPE_WITH_PROTOTYPE_SET_NOT_ALLOWED_ON_CLASS_TYPES);
}

public void testJsPropertyNotAllowed() {
StringBuilder buggyCode = new StringBuilder();
buggyCode.append("import com.google.gwt.core.client.js.JsType;\n");
buggyCode.append("import com.google.gwt.core.client.js.JsProperty;\n");
buggyCode.append("public class Buggy {\n");
buggyCode.append("@JsType interface Foo {}\n");
buggyCode.append("static class BuggyFoo implements Foo {\n");
buggyCode.append("@JsProperty void foo() {}\n");
buggyCode.append("}\n");
buggyCode.append("}\n");

shouldGenerateError(buggyCode, "Line 6: "
+ JSORestrictionsChecker.ERR_JSPROPERTY_ONLY_ON_INTERFACES);
}

public void testJsTypePrototypeExtensionNotAllowed() {
StringBuilder buggyCode = new StringBuilder();
buggyCode.append("import com.google.gwt.core.client.js.JsType;\n");
Expand Down

0 comments on commit dd61498

Please sign in to comment.