Skip to content

Commit

Permalink
Fixes bugs and adds tests for @JsProperty naming.
Browse files Browse the repository at this point in the history 
There was a bug where users were being allowed to define getters (that 
started with "get") that returned void and there was another bug where 
getters were allowed to be named like setters and setters were allowed 
to be named like getters.

I corrected these bugs and added tests to show that each of the various
disallowed forms now result in a compile error.

Change-Id: I800e3842b23ad3c4becbe5dfa1d89131ec00918a
Review-Link: https://gwt-review.googlesource.com/#/c/11671/
  • Loading branch information
@stalcup
stalcup committed Feb 19, 2015
1 parent e668108 commit ffeca5d
Show file tree
Hide file tree
Showing 2 changed files with 159 additions and 51 deletions.
85 changes: 46 additions & 39 deletions dev/core/src/com/google/gwt/dev/javac/JSORestrictionsChecker.java
View file
Expand Up @@ -294,47 +294,12 @@ private void checkJsProperty(MethodBinding mb, boolean allowed) {
return;
}
String methodName = String.valueOf(mb.selector);
if (!isGetter(methodName, mb) && !isSetter(methodName, mb) && !isHas(methodName, mb)) {
if (!isGetter(methodName, mb) && !isSetter(methodName, mb)) {
errorOn(mb, ERR_JSPROPERTY_ONLY_BEAN_OR_FLUENT_STYLE_NAMING);
}
}
}

private boolean isGetter(String name, MethodBinding mb) {
// zero arg non-void getX()
if (name.length() > 3 && name.startsWith("get") && Character.isUpperCase(name.charAt(3)) &&
mb.returnType == TypeBinding.VOID && mb.parameters.length == 0) {
return true;
} else if (name.length() > 3 && name.startsWith("is")
&& Character.isUpperCase(name.charAt(2)) && mb.returnType == TypeBinding.BOOLEAN
&& mb.parameters.length == 0) {
return true;
} else if (mb.parameters.length == 0 && mb.returnType != TypeBinding.VOID) {
return true;
}
return false;
}

private boolean isSetter(String name, MethodBinding mb) {
if (mb.returnType == TypeBinding.VOID || mb.returnType == mb.declaringClass) {
if (name.length() > 3 && name.startsWith("set") && Character.isUpperCase(name.charAt(3))
&& mb.parameters.length == 1) {
return true;
} else if (mb.parameters.length == 1) {
return true;
}
}
return false;
}

private boolean isHas(String name, MethodBinding mb) {
if (name.length() > 3 && name.startsWith("has") && Character.isUpperCase(name.charAt(3))
&& mb.parameters.length == 0 && mb.returnType == TypeBinding.BOOLEAN) {
return true;
}
return false;
}

private void checkJsTypeMethodsForOverloads(Map<String, MethodBinding> methodNamesAndSigs,
Map<String, MethodBinding> noExports, ReferenceBinding binding) {
if (isJsType(binding)) {
Expand All @@ -348,8 +313,8 @@ private void checkJsTypeMethodsForOverloads(Map<String, MethodBinding> methodNam
continue;
}
if (JdtUtil.getAnnotation(mb, JsInteropUtil.JSPROPERTY_CLASS) != null) {
if (isGetter(methodName, mb) || isSetter(methodName, mb) || isHas(methodName, mb)) {
// js properties are allowed to be overloaded (setter/getter)
if (isGetter(methodName, mb) || isSetter(methodName, mb)) {
// JS properties are allowed to be overloaded (setter/getter)
continue;
}
}
Expand Down Expand Up @@ -454,6 +419,44 @@ private void checkClassExtendsMagicPrototype(TypeDeclaration type, ReferenceBind
}
}

private boolean isGetter(String methodName, MethodBinding methodBinding) {
// Anything that takes parameters or doesn't return a value can't be a getter.
if (methodBinding.parameters.length > 0 || methodBinding.returnType == TypeBinding.VOID) {
return false;
}
// If it sounds like a setter then it can't be a getter.
if (startsWithCamelCase(methodName, "set")) {
return false;
}
// If it sounds like an "is" or "has" getter but doesn't return a boolean it can't be a
// getter.
if ((startsWithCamelCase(methodName, "is") || startsWithCamelCase(methodName, "has"))
&& methodBinding.returnType != TypeBinding.BOOLEAN) {
return false;
}
// Everything else is a getter.
return true;
}

private boolean isSetter(String methodName, MethodBinding methodBinding) {
// Anything that doesn't take exactly 1 parameter can't be a setter.
if (methodBinding.parameters.length != 1) {
return false;
}
// If it sounds like a getter then it can't be a setter.
if (startsWithCamelCase(methodName, "get") || startsWithCamelCase(methodName, "is")
|| startsWithCamelCase(methodName, "has")) {
return false;
}
// If it doesn't return void and isn't fluent then it isn't a setter.
if (methodBinding.returnType != TypeBinding.VOID
&& methodBinding.returnType != methodBinding.declaringClass) {
return false;
}
// Everything else is a setter.
return true;
}

// Roughly parallels JProgram.isJsTypePrototype()
private boolean isMagicPrototype(ReferenceBinding type, ReferenceBinding jsInterface) {
if (isMagicPrototypeStub(type)) {
Expand Down Expand Up @@ -598,8 +601,12 @@ private static void errorOn(ASTNode node, CompilationUnitDeclaration cud,
"jsoRestrictions.html"));
}

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

private final CompilationUnitDeclaration cud;
private final CheckerState state;

private JSORestrictionsChecker(CheckerState state,
Expand Down
125 changes: 113 additions & 12 deletions dev/core/test/com/google/gwt/dev/javac/JSORestrictionsTest.java
View file
Expand Up @@ -445,7 +445,7 @@ public void testJsExportNotOnNonPublicMethod() {
StringBuilder buggyCode = new StringBuilder();
buggyCode.append("import com.google.gwt.core.client.js.JsExport;\n");
buggyCode.append("public class Buggy {\n");
buggyCode.append("@JsExport Object foo() {return null;}\n");
buggyCode.append("@JsExport static Object foo() {return null;}\n");
buggyCode.append("}\n");

shouldGenerateError(buggyCode, "Line 3: "
Expand Down Expand Up @@ -595,44 +595,145 @@ public void testJsTypePrototypeExtensionNotAllowed2() {
+ JSORestrictionsChecker.ERR_MUST_EXTEND_MAGIC_PROTOTYPE_CLASS);
}

public void testJsPropertyBadStyle() {
public void testJsPropertyNoErrors() {
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("@JsType public interface Buggy {\n");
buggyCode.append("@JsProperty void foo();\n");

buggyCode.append("@JsProperty int foo();\n");
buggyCode.append("@JsProperty void foo(int x);\n");

buggyCode.append("@JsProperty int getFoo();\n");
buggyCode.append("@JsProperty void setFoo(int x);\n");

buggyCode.append("@JsProperty boolean hasFoo();\n");
buggyCode.append("@JsProperty boolean isFoo();\n");
buggyCode.append("@JsProperty Buggy setFoo(String x);\n");

buggyCode.append("}\n");

shouldGenerateNoError(buggyCode);
}

public void testJsPropertyGetterCantTakeParameters() {
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("@JsType public interface Buggy {\n");
buggyCode.append("@JsProperty int getFoo(int a, int b);\n");
buggyCode.append("}\n");

shouldGenerateError(buggyCode,
"Line 4: " + JSORestrictionsChecker.ERR_JSPROPERTY_ONLY_BEAN_OR_FLUENT_STYLE_NAMING);
}

public void testJsPropertyBadStyle2() {
public void testJsPropertyGetterCantReturnVoid() {
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("@JsType public interface Buggy {\n");
buggyCode.append("@JsProperty int foo(int x);\n");
buggyCode.append("@JsProperty void getFoo();\n");
buggyCode.append("}\n");

shouldGenerateError(buggyCode,
"Line 4: " + JSORestrictionsChecker.ERR_JSPROPERTY_ONLY_BEAN_OR_FLUENT_STYLE_NAMING);
}

public void testJsPropertyNoErrors() {
public void testJsPropertyGetterCantSoundLikeSetter() {
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("@JsType public interface Buggy {\n");
buggyCode.append("@JsProperty int setFoo();\n");
buggyCode.append("}\n");

shouldGenerateError(buggyCode,
"Line 4: " + JSORestrictionsChecker.ERR_JSPROPERTY_ONLY_BEAN_OR_FLUENT_STYLE_NAMING);
}

public void testJsPropertyHasGetterMustReturnBoolean() {
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("@JsType public interface Buggy {\n");
buggyCode.append("@JsProperty int hasFoo();\n");
buggyCode.append("}\n");

shouldGenerateError(buggyCode,
"Line 4: " + JSORestrictionsChecker.ERR_JSPROPERTY_ONLY_BEAN_OR_FLUENT_STYLE_NAMING);
}

public void testJsPropertyIsGetterMustReturnBoolean() {
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("@JsType public interface Buggy {\n");
buggyCode.append("@JsProperty int isFoo();\n");
buggyCode.append("}\n");

shouldGenerateError(buggyCode,
"Line 4: " + JSORestrictionsChecker.ERR_JSPROPERTY_ONLY_BEAN_OR_FLUENT_STYLE_NAMING);
}

public void testJsPropertySetterCantTakeTwoParameters() {
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("@JsType public interface Buggy {\n");
buggyCode.append("@JsProperty void setFoo(int a, int b);\n");
buggyCode.append("}\n");

shouldGenerateError(buggyCode,
"Line 4: " + JSORestrictionsChecker.ERR_JSPROPERTY_ONLY_BEAN_OR_FLUENT_STYLE_NAMING);
}

public void testJsPropertySetterCantSoundLikeIsGetter() {
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("@JsType public interface Buggy {\n");
buggyCode.append("@JsProperty void isFoo(int x);\n");
buggyCode.append("}\n");

shouldGenerateError(buggyCode,
"Line 4: " + JSORestrictionsChecker.ERR_JSPROPERTY_ONLY_BEAN_OR_FLUENT_STYLE_NAMING);
}

public void testJsPropertySetterCantSoundLikeGetGetter() {
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("@JsType public interface Buggy {\n");
buggyCode.append("@JsProperty int foo();\n");
buggyCode.append("@JsProperty void foo(int x);\n");
buggyCode.append("@JsProperty void setFoo(int x);\n");
buggyCode.append("@JsProperty void getFoo(int x);\n");
buggyCode.append("@JsProperty Buggy setFoo(String x);\n");
buggyCode.append("@JsProperty boolean isFoo();\n");
buggyCode.append("}\n");

shouldGenerateNoError(buggyCode);
shouldGenerateError(buggyCode,
"Line 4: " + JSORestrictionsChecker.ERR_JSPROPERTY_ONLY_BEAN_OR_FLUENT_STYLE_NAMING);
}

public void testJsPropertySetterCantSoundLikeHasGetter() {
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("@JsType public interface Buggy {\n");
buggyCode.append("@JsProperty void hasFoo(int x);\n");
buggyCode.append("}\n");

shouldGenerateError(buggyCode,
"Line 4: " + JSORestrictionsChecker.ERR_JSPROPERTY_ONLY_BEAN_OR_FLUENT_STYLE_NAMING);
}

public void testJsPropertySetterCantReturnInt() {
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("@JsType public interface Buggy {\n");
buggyCode.append("@JsProperty int foo(int x);\n");
buggyCode.append("}\n");

shouldGenerateError(buggyCode,
"Line 4: " + JSORestrictionsChecker.ERR_JSPROPERTY_ONLY_BEAN_OR_FLUENT_STYLE_NAMING);
}

/**
Expand Down

0 comments on commit ffeca5d

Please sign in to comment.