Skip to content

Commit

Permalink
Fix issue #232 or #233: Class literals in annotation values w/o ".class"
Browse files Browse the repository at this point in the history
  • Loading branch information
@eric-milles
eric-milles committed Dec 3, 2016
1 parent 57eff92 commit 5afa50f
Show file tree
Hide file tree
Showing 10 changed files with 705 additions and 338 deletions.
64 changes: 59 additions & 5 deletions ...ovy.core.tests.compiler/src/org/eclipse/jdt/groovy/core/tests/basic/AnnotationsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,58 @@ public void testLongLiteral() {
runConformTest(sources);
}

public void testBigIntegerLiteral() {
String[] sources = {
"Min.java",
"import java.lang.annotation.*;\n" +
"@Target(ElementType.FIELD)\n" +
"@interface Min {\n" +
" long value();\n" +
"}",

"Main.groovy",
"class Main {\n" +
" @Min(0G)\n" +
" Integer index\n" +
"}"
};

// there should not be an error from the Java model -- org.codehaus.jdt.groovy.internal.compiler.ast.GroovyCompilationUnitDeclaration.UnitPopulator.createConstantExpression(ConstantExpression)
runNegativeTest(sources,
"----------\n" +
"1. ERROR in Main.groovy (at line 2)\n" +
"\t@Min(0G)\n" +
"\t ^" + (GroovyUtils.isAtLeastGroovy(20) ? "^" : "") + "\n" +
"Groovy:Attribute 'value' should have type 'java.lang.Long'; but found type 'java.math.BigInteger' in @Min\n" +
"----------\n");
}

public void testBigDecimalLiteral() {
String[] sources = {
"Min.java",
"import java.lang.annotation.*;\n" +
"@Target(ElementType.FIELD)\n" +
"@interface Min {\n" +
" double value();\n" +
"}",

"Main.groovy",
"class Main {\n" +
" @Min(1.1G)\n" +
" BigDecimal index\n" +
"}"
};

// there should not be an error from the Java model -- org.codehaus.jdt.groovy.internal.compiler.ast.GroovyCompilationUnitDeclaration.UnitPopulator.createConstantExpression(ConstantExpression)
runNegativeTest(sources,
"----------\n" +
"1. ERROR in Main.groovy (at line 2)\n" +
"\t@Min(1.1G)\n" +
"\t ^" + (GroovyUtils.isAtLeastGroovy(20) ? "^^^" : "") + "\n" +
"Groovy:Attribute 'value' should have type 'java.lang.Double'; but found type 'java.math.BigDecimal' in @Min\n" +
"----------\n");
}

public void testClassAnnotationValue() {
String[] sources = {
"Anno.java",
Expand Down Expand Up @@ -172,16 +224,18 @@ public void testDoubleAttributeWithBigDecimalValue() {

"AnnotationDoubleTest.groovy",
"class AnnotationDoubleTest {\n" +
"class FooWithAnnotation { @AnnotationDouble(value=\"test\", width=1.0) double value; }\n" +
"def test = new AnnotationDoubleTest()\n" +
" class FooWithAnnotation {\n" +
" @AnnotationDouble(value='test', width=1.0) double value\n" +
" }\n" +
" def test = new AnnotationDoubleTest()\n" +
"}"
};

runNegativeTest(sources,
"----------\n" +
"1. ERROR in AnnotationDoubleTest.groovy (at line 2)\n" +
"\tclass FooWithAnnotation { @AnnotationDouble(value=\"test\", width=1.0) double value; }\n" +
"\t ^" + (GroovyUtils.isAtLeastGroovy(20) ? "^^" : "") + "\n" +
"1. ERROR in AnnotationDoubleTest.groovy (at line 3)\n" +
"\t@AnnotationDouble(value='test', width=1.0) double value\n" +
"\t ^" + (GroovyUtils.isAtLeastGroovy(20) ? "^^" : "") + "\n" +
"Groovy:Attribute 'width' should have type 'java.lang.Double'; but found type 'java.math.BigDecimal' in @AnnotationDouble\n" +
"----------\n");
}
Expand Down
29 changes: 15 additions & 14 deletions ...groovy.core.tests.compiler/src/org/eclipse/jdt/groovy/core/tests/basic/GenericsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.eclipse.jdt.core.tests.util.GroovyUtils;
import org.eclipse.jdt.internal.compiler.classfmt.ClassFileConstants;
import org.eclipse.jdt.internal.compiler.impl.CompilerOptions;
import org.osgi.framework.Version;

public final class GenericsTests extends AbstractGroovyRegressionTest {

Expand Down Expand Up @@ -1609,14 +1610,20 @@ public void testHalfFinishedGenericsProgramWithSuppressionValueSpeltWrong() {
public void testHalfFinishedGenericsProgramWithMultipleSuppressionValues() {
String[] sources = {
"Demo.groovy",
"public class Demo {\n"+
"\n"+
"@SuppressWarnings([\"rawtypes\",\"cast\"])\n"+
"List myList;\n"+
"class Demo {\n"+
" @SuppressWarnings(['rawtypes','cast'])\n"+
" List list\n"+
"}"
};

runNegativeTest(sources, "");
// Eclipse Oxygen (i.e. JDT Core 3.13) added warning for mixed mode
Version v = Platform.getBundle("org.eclipse.jdt.core").getVersion();
runNegativeTest(sources, (v.getMajor() == 3 && v.getMinor() < 13) ? "" : "----------\n" +
"1. WARNING in Demo.groovy (at line 2)\n" +
"\t@SuppressWarnings(['rawtypes','cast'])\n" +
"\t ^^^^^^\n" +
"At least one of the problems in category 'cast' is not analysed due to a compiler option being ignored\n" +
"----------\n");
}

public void testHalfFinishedGenericsProgramWithMultipleSuppressionValuesWithOneSpeltWrong() {
Expand All @@ -1638,12 +1645,8 @@ public void testHalfFinishedGenericsProgramWithMultipleSuppressionValuesWithOneS
"----------\n");
}

private boolean isEclipse36() {
return Platform.getBundle("org.eclipse.jdt.core").getVersion().toString().startsWith("3.6");
}

public void testJava7() {
if (isEclipse36() || complianceLevel < ClassFileConstants.JDK1_7) return;
if (complianceLevel < ClassFileConstants.JDK1_7) return;

String[] sources = {
"A.java",
Expand All @@ -1668,8 +1671,7 @@ public void testJava7() {
}

public void testJava7_2() {
if (isEclipse36() || complianceLevel >= ClassFileConstants.JDK1_7) return;
// should fail if compliance level < 1.7
if (complianceLevel >= ClassFileConstants.JDK1_7) return;

String[] sources = {
"A.java",
Expand Down Expand Up @@ -1702,8 +1704,7 @@ public void testJava7_2() {
}

public void testJava7_3() {
if (isEclipse36() || complianceLevel >= ClassFileConstants.JDK1_7) return;
// should fail if compliance level < 1.7
if (complianceLevel >= ClassFileConstants.JDK1_7) return;

String[] sources = {
"A.java",
Expand Down
92 changes: 20 additions & 72 deletions ...e/src/org/codehaus/jdt/groovy/internal/compiler/ast/GroovyCompilationUnitDeclaration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -565,10 +565,8 @@ private void recordProblems(List<?> errors) {
eoffset = sourceEnd;
}

char[] filename = getFileName();
p = new DefaultProblemFactory().createProblem(filename, 0, new String[] { msg }, 0, new String[] { msg }, sev, soffset,
eoffset, line, scol);
this.problemReporter.record(p, compilationResult, this, false);
p = new DefaultProblemFactory().createProblem(getFileName(), 0, new String[] {msg}, 0, new String[] {msg}, sev, soffset, eoffset, line, scol);
problemReporter.record(p, compilationResult, this, false);
errorsRecorded.add(message);
log(String.valueOf(compilationResult.getFileName()) + ": " + line + " " + msg);
}
Expand Down Expand Up @@ -1605,10 +1603,10 @@ private org.eclipse.jdt.internal.compiler.ast.Expression createAnnotationMemberE
} else if (expr instanceof PropertyExpression) {
PropertyExpression prop = (PropertyExpression) expr;
assert prop.getProperty() instanceof ConstantExpression;
String name = prop.getPropertyAsString();
if (name.equals("class") || !NON_TYPE_NAME.matcher(name).matches()) {
if (prop.getPropertyAsString().equals("class")) {
return new ClassLiteralAccess(expr.getEnd(), createTypeReferenceForClassLiteral(prop));
}
// could still be a class literal; Groovy does not require ".class" -- resolved in MemberValuePair
char[][] tokens = CharOperation.splitOn('.', prop.getText().toCharArray());
// guess the intermediate positions based on start offset and token lengths
int n = tokens.length, s = prop.getObjectExpression().getStart();
Expand All @@ -1623,12 +1621,8 @@ private org.eclipse.jdt.internal.compiler.ast.Expression createAnnotationMemberE

} else if (expr instanceof VariableExpression) {
String name = ((VariableExpression) expr).getName();
if (NON_TYPE_NAME.matcher(name).matches()) {
// could be a class field or statically imported constant also...
return new SingleNameReference(name.toCharArray(), toPos(expr.getStart(), expr.getEnd() - 1));
}
// likely a class literal; Groovy does not require ".class"
return new ClassLiteralAccess(expr.getEnd(), new SingleTypeReference(name.toCharArray(), toPos(expr.getStart(), expr.getEnd() - 1)));
// could be a class literal; Groovy does not require ".class" -- resolved in MemberValuePair
return new SingleNameReference(name.toCharArray(), toPos(expr.getStart(), expr.getEnd() - 1));

} else if (expr instanceof ClosureExpression) {
// annotation is something like "@Tag(value = { some computation })" return "Closure.class" to appease JDT
Expand All @@ -1642,9 +1636,6 @@ private org.eclipse.jdt.internal.compiler.ast.Expression createAnnotationMemberE
return new NullLiteral(expr.getStart(), expr.getEnd());
}

// TODO: Look up name to determine if it is a class or a constant
private static final Pattern NON_TYPE_NAME = Pattern.compile("[a-z_]\\w*|[A-Z][A-Z_0-9]*");

private org.eclipse.jdt.internal.compiler.ast.MemberValuePair[] createAnnotationMemberValuePairs(Map<String, Expression> memberValuePairs) {
List<org.eclipse.jdt.internal.compiler.ast.MemberValuePair> mvps =
new ArrayList<org.eclipse.jdt.internal.compiler.ast.MemberValuePair>(memberValuePairs.size());
Expand All @@ -1653,7 +1644,7 @@ private org.eclipse.jdt.internal.compiler.ast.MemberValuePair[] createAnnotation
char[] name = memberValuePair.getKey().toCharArray();
int start = memberValuePair.getValue().getStart() - name.length - 1, until = memberValuePair.getValue().getEnd();
org.eclipse.jdt.internal.compiler.ast.Expression value = createAnnotationMemberExpression(memberValuePair.getValue());
mvps.add(new org.eclipse.jdt.internal.compiler.ast.MemberValuePair(name, start, until, value)); // TODO: set binding?
mvps.add(new org.eclipse.jdt.internal.compiler.ast.MemberValuePair(name, start, until, value));
}

return mvps.toArray(new org.eclipse.jdt.internal.compiler.ast.MemberValuePair[mvps.size()]);
Expand All @@ -1678,10 +1669,10 @@ private Literal createConstantExpression(ConstantExpression expr) {
} else if (type.equals("int") || type.equals("java.lang.Integer")) {
char[] chars = value.toString().toCharArray();
return IntLiteral.buildIntLiteral(chars, start, start + chars.length);
} else if (type.equals("long") || type.equals("java.lang.Long")) {
} else if (type.equals("long") || type.equals("java.lang.Long") || type.equals("java.math.BigInteger")) {
char[] chars = (value.toString() + 'L').toCharArray();
return LongLiteral.buildLongLiteral(chars, start, start + chars.length);
} else if (type.equals("double") || type.equals("java.lang.Double")) {
} else if (type.equals("double") || type.equals("java.lang.Double") || type.equals("java.math.BigDecimal")) {
return new DoubleLiteral(value.toString().toCharArray(), start, until);
} else if (type.equals("float") || type.equals("java.lang.Float")) {
return new FloatLiteral(value.toString().toCharArray(), start, until);
Expand All @@ -1691,9 +1682,6 @@ private Literal createConstantExpression(ConstantExpression expr) {
return IntLiteral.buildIntLiteral(chars, start, start + chars.length);
} else if (type.equals("char") || type.equals("java.lang.Character")) {
return new CharLiteral(value.toString().toCharArray(), start, until);
} else if (type.equals("java.math.BigDecimal")) {
// TODO: Return something else for BigDecimal? This is a bit of a cheat...
return new DoubleLiteral(value.toString().toCharArray(), start, until);
} else {
Util.log(IStatus.WARNING, "Unhandled annotation constant type: " + expr.getType().getName());
return null;
Expand Down Expand Up @@ -1993,41 +1981,20 @@ private TypeParameter[] createTypeParametersForGenerics(GenericsType[] generics)
// move past T
offset += length;
source = source.substring(length);
//if (source != null) {
// move past "extends" and w.spaces
Matcher m = EXTENDS.matcher(source);
if (m.find()) {
length = m.group().length();
offset += length;
source = source.substring(length);
}
//else {
// while (Character.isWhitespace(source.charAt(0))) {
// offset += 1;
// source = source.substring(1);
// }
// if (source.startsWith("extends")) {
// offset += 7;
// source = source.substring(7);
// }
// while (Character.isWhitespace(source.charAt(0))) {
// offset += 1;
// source = source.substring(1);
// }
//}
//} else { offset += 9; } // ballpark it

length = upperBounds[0].getName().length();
// TODO: Is this correct for qualified and unqualified occurrences?
String name = GroovyUtils.splitName(upperBounds[0])[1];
assert length == source.indexOf(name) + name.length();
//// check source for name to regain some accuracy
//int idx = source.indexOf(name);
//if (idx != -1) {
// length = idx + name.length();
//}

// ???: Would a ClassNode with its own generics ever be missing sloc?
// Would a ClassNode with its own generics ever be missing sloc?
assert source.length() == length || source.charAt(length) != '<';
}

Expand All @@ -2043,35 +2010,17 @@ private TypeParameter[] createTypeParametersForGenerics(GenericsType[] generics)
offset = _start;
} else { // it appears only MetaClass or GeneratedClosure could get here...
offset += length;
//if (source != null) {
// move past bounds type
// move past bounds type
source = source.substring(length);
// move past "&" and w.spaces
Matcher m = AND.matcher(source);
if (m.find()) {
length = m.group().length();
offset += length;
source = source.substring(length);
// move past "&" and w.spaces
Matcher m = AND.matcher(source);
if (m.find()) {
length = m.group().length();
offset += length;
source = source.substring(length);
}
//else {
// char c;
// while ((c = source.charAt(0)) == '&' || Character.isWhitespace(c)) {
// offset += 1;
// source = source.substring(1);
// }
//}
//} else { offset += 3; } // ballpark it
}

length = upperBounds[0].getName().length();
// TODO: Is this correct for qualified and unqualified occurrences?
//String name = GroovyUtils.splitName(upperBounds[j])[1];
//assert length == source.indexOf(name) + name.length();
//int idx = source.indexOf(name);
//if (idx != -1) {
// length = idx + name.length();
//}
// ???: Would a ClassNode with its own generics ever be missing sloc?
//assert source.length() == length || source.charAt(length) != '<';
}

typeParameter.bounds[j - 1] = createTypeReferenceForClassNode(upperBounds[j], offset, offset + length);
Expand Down Expand Up @@ -2521,9 +2470,8 @@ private void addUnlessDuplicate(List<AbstractMethodDeclaration> accumulatedDecla
boolean argsTheSame = true;
for (int i = 0; i < mdArgsLen; i++) {
// FIXASC this comparison can fail if some are fully qualified and some not - in fact it
// suggests that default param variants should be built by augmentMethod() in a similar way to
// the GroovyObject methods, rather than during type declaration construction - but not super urgent right
// now
// suggests that default param variants should be built by augmentMethod() in a similar
// way to the GroovyObject methods, rather than during type declaration construction
if (!CharOperation.equals(mdArgs[i].type.getTypeName(), vmdArgs[i].type.getTypeName())) {
argsTheSame = false;
break;
Expand Down
Loading

0 comments on commit 5afa50f

Please sign in to comment.