Skip to content

Commit 596bd72

Browse files
eric-milleseric-milles
committed
GROOVY-5001, GROOVY-5491, GROOVY-6144, GROOVY-8065, GROOVY-8788
1 parent 7f5b698 commit 596bd72

6 files changed

Lines changed: 138 additions & 46 deletions

File tree

‎base-test/org.eclipse.jdt.groovy.core.tests.compiler/src/org/eclipse/jdt/groovy/core/tests/xform/StaticCompilationTests.java‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -897,7 +897,30 @@ public void testCompileStatic1521() {
897897
runNegativeTest(sources, "");
898898
}
899899

900-
@Test // see GROOVY-5001, GROOVY-5491, GROOVY-6144, GROOVY-8788
900+
@Test
901+
public void testCompileStatic5491() {
902+
for (String mode : new String[] {"", "public", "protected", "@groovy.transform.PackageScope"}) {
903+
//@formatter:off
904+
String[] sources = {
905+
"Main.groovy",
906+
"class M extends HashMap<String,Number> {\n" +
907+
mode + " void setFoo(foo) { print foo }\n" +
908+
"}\n" +
909+
"@groovy.transform.CompileStatic\n" +
910+
"void test() {\n" +
911+
" def map = new M()\n" +
912+
" map.foo = 123\n" + // setter
913+
" print map.foo\n" + // get(K)
914+
"}\n" +
915+
"test()\n",
916+
};
917+
//@formatter:on
918+
919+
runConformTest(sources, "123null");
920+
}
921+
}
922+
923+
@Test // see GROOVY-5001, GROOVY-6144, GROOVY-8065, GROOVY-8788
901924
public void testCompileStatic5517() {
902925
//@formatter:off
903926
String[] sources = {

‎base/org.codehaus.groovy30/src/org/codehaus/groovy/transform/sc/transformers/StaticCompilationTransformer.java‎

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,10 @@
4646
import java.util.Iterator;
4747
import java.util.Map;
4848

49+
import static org.codehaus.groovy.ast.tools.GeneralUtils.callX;
50+
import static org.codehaus.groovy.ast.tools.GeneralUtils.isOrImplements;
51+
import static org.apache.groovy.ast.tools.ExpressionUtils.isThisOrSuper;
52+
4953
/**
5054
* Some expressions use symbols as aliases to method calls (&lt;&lt;, +=, ...). In static compilation,
5155
* if such a method call is found, we transform the original binary expression into a method
@@ -116,21 +120,24 @@ public Expression transform(final Expression expr) {
116120
if (expr instanceof ConstructorCallExpression) {
117121
return constructorCallTransformer.transformConstructorCall((ConstructorCallExpression) expr);
118122
}
119-
// GRECLIPSE add -- GROOVY-6097, GROOVY-7300, GROOVY-9089, et al.
123+
// GRECLIPSE add -- GROOVY-6097, GROOVY-7300, GROOVY-8065, GROOVY-9089, GROOVY-10557, et al.
120124
if (expr instanceof PropertyExpression) {
121125
MethodNode dmct = expr.getNodeMetaData(org.codehaus.groovy.transform.stc.StaticTypesMarker.DIRECT_METHOD_CALL_TARGET);
122126
// NOTE: BinaryExpressionTransformer handles the setter
123127
if (dmct != null && dmct.getParameters().length == 0) {
124128
PropertyExpression pe = (PropertyExpression) expr;
125-
126-
MethodCallExpression mce = new MethodCallExpression(transform(pe.getObjectExpression()), pe.getPropertyAsString(), MethodCallExpression.NO_ARGUMENTS);
127-
mce.setImplicitThis(pe.isImplicitThis());
128-
mce.setMethodTarget(dmct);
129-
mce.setSourcePosition(pe);
130-
mce.setSpreadSafe(pe.isSpreadSafe());
131-
mce.setSafe(pe.isSafe());
132-
mce.copyNodeMetaData(pe);
133-
return mce;
129+
if (!isOrImplements(getTypeChooser().resolveType(pe.getObjectExpression(),getClassNode()), ClassHelper.MAP_TYPE)) {
130+
MethodCallExpression mce = callX(transform(pe.getObjectExpression()), pe.getPropertyAsString());
131+
mce.setImplicitThis(pe.isImplicitThis());
132+
mce.setMethodTarget(dmct);
133+
mce.setSourcePosition(pe);
134+
mce.setSpreadSafe(pe.isSpreadSafe());
135+
mce.setSafe(pe.isSafe());
136+
mce.copyNodeMetaData(pe);
137+
return mce;
138+
} else if (!isThisOrSuper(pe.getObjectExpression())) {
139+
expr.removeNodeMetaData(org.codehaus.groovy.transform.stc.StaticTypesMarker.DIRECT_METHOD_CALL_TARGET);
140+
}
134141
}
135142
return super.transform(expr);
136143
}

‎base/org.codehaus.groovy40/src/org/codehaus/groovy/transform/sc/transformers/PropertyExpressionTransformer.java‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import static org.codehaus.groovy.ast.ClassHelper.MAP_TYPE;
2828
import static org.codehaus.groovy.ast.tools.GeneralUtils.callX;
2929
import static org.codehaus.groovy.ast.tools.GeneralUtils.isOrImplements;
30+
import static org.apache.groovy.ast.tools.ExpressionUtils.isThisOrSuper;
3031

3132
class PropertyExpressionTransformer {
3233

@@ -39,15 +40,19 @@ class PropertyExpressionTransformer {
3940
Expression transformPropertyExpression(final PropertyExpression pe) {
4041
MethodNode dmct = pe.getNodeMetaData(StaticTypesMarker.DIRECT_METHOD_CALL_TARGET);
4142
// NOTE: BinaryExpressionTransformer handles the setter
42-
if (dmct != null && dmct.getParameters().length == 0 && !isOrImplements(scTransformer.getTypeChooser().resolveType(pe.getObjectExpression(), scTransformer.getClassNode()), MAP_TYPE)) {
43-
MethodCallExpression mce = callX(scTransformer.transform(pe.getObjectExpression()), pe.getPropertyAsString()); // GRECLIPSE edit
44-
mce.setImplicitThis(pe.isImplicitThis());
45-
mce.setMethodTarget(dmct);
46-
mce.setSourcePosition(pe);
47-
mce.setSpreadSafe(pe.isSpreadSafe());
48-
mce.setSafe(pe.isSafe());
49-
mce.copyNodeMetaData(pe);
50-
return mce;
43+
if (dmct != null && dmct.getParameters().length == 0) {
44+
if (!isOrImplements(scTransformer.getTypeChooser().resolveType(pe.getObjectExpression(), scTransformer.getClassNode()), MAP_TYPE)) {
45+
MethodCallExpression mce = callX(scTransformer.transform(pe.getObjectExpression()), pe.getPropertyAsString()); // GRECLIPSE edit
46+
mce.setImplicitThis(pe.isImplicitThis());
47+
mce.setMethodTarget(dmct);
48+
mce.setSourcePosition(pe);
49+
mce.setSpreadSafe(pe.isSpreadSafe());
50+
mce.setSafe(pe.isSafe());
51+
mce.copyNodeMetaData(pe);
52+
return mce;
53+
} else if (!isThisOrSuper(pe.getObjectExpression())) { // GRECLIPSE add
54+
pe.removeNodeMetaData(StaticTypesMarker.DIRECT_METHOD_CALL_TARGET);
55+
}
5156
}
5257

5358
return scTransformer.superTransform(pe);

‎base/org.eclipse.jdt.groovy.core/src/org/eclipse/jdt/groovy/search/SimpleTypeLookup.java‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -617,7 +617,6 @@ protected ASTNode findDeclarationForDynamicVariable(final VariableExpression var
617617
* Looks for the named member in the declaring type. Also searches super types.
618618
* The result can be a field, method, or property.
619619
*
620-
* @param name the name of the field, method, constant or property to find
621620
* @param declaringType the type in which the named member's declaration resides
622621
* @param isLhsExpression {@code true} if named member is being assigned a value
623622
* @param isStaticExpression {@code true} if member is being accessed statically
@@ -652,7 +651,7 @@ protected ASTNode findDeclaration(final String name, final ClassNode declaringTy
652651

653652
boolean dynamicProperty = (!isCallExpression && !isStaticExpression && isOrImplements(declaringType, VariableScope.MAP_CLASS_NODE));
654653

655-
if (dynamicProperty && directFieldAccess == 0 && !isLhsExpression) { // GROOVY-5491
654+
if (dynamicProperty && !isLhsExpression && (GroovyUtils.getGroovyVersion().getMajor() < 5 || name.matches("empty|class|metaClass"))) { // GROOVY-5001, GROOVY-5491, GROOVY-6144
656655
return createDynamicProperty(name, getMapPropertyType(declaringType), declaringType, isStaticExpression);
657656
}
658657

@@ -674,9 +673,8 @@ protected ASTNode findDeclaration(final String name, final ClassNode declaringTy
674673
.flatMap(list -> list.stream().filter(prop -> prop.getName().equals(name)).findFirst()).orElse(null);
675674
}
676675
if (isCompatible(property, isStaticExpression) &&
677-
(!isLhsExpression || !Flags.isFinal(property.getModifiers())) && // GROOVY-8065
678-
(!(accessor.isPresent() || (dynamicProperty && !isLhsExpression)) || // GROOVY-5491
679-
directFieldAccess == 1 && declaringType.equals(property.getDeclaringClass()))) {
676+
(!isLhsExpression || !Flags.isFinal(property.getModifiers())) && // GROOVY-8065
677+
(!accessor.isPresent() || (directFieldAccess == 1 && declaringType.equals(property.getDeclaringClass())))) {
680678
return property;
681679
}
682680
if (property != null) break;
@@ -692,7 +690,7 @@ protected ASTNode findDeclaration(final String name, final ClassNode declaringTy
692690
return field;
693691
}
694692

695-
if (dynamicProperty && !(isLhsExpression && nonPrivateAccessor)) { // GROOVY-5491
693+
if (dynamicProperty && !(nonPrivateAccessor && (isLhsExpression || GroovyUtils.getGroovyVersion().getMajor() >= 5))) { // GROOVY-5001, GROOVY-5491
696694
return createDynamicProperty(name, getMapPropertyType(declaringType), declaringType, isStaticExpression);
697695
}
698696

‎ide-test/org.codehaus.groovy.eclipse.tests/src/org/codehaus/groovy/eclipse/test/ui/SemanticHighlightingTests.groovy‎

Lines changed: 76 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -3613,12 +3613,16 @@ final class SemanticHighlightingTests extends GroovyEclipseTestSuite {
36133613
new HighlightedTypedPosition(contents.lastIndexOf('key'), 3, MAP_KEY))
36143614
}
36153615

3616-
@Test
3616+
@Test // GROOVY-5001
36173617
void testMapKey5() {
3618-
String contents = '''\
3618+
addGroovySource '''\
36193619
|abstract class A extends HashMap {
36203620
| def one, two = { -> }
3621+
| def getSomething() {}
36213622
|}
3623+
|'''.stripMargin()
3624+
3625+
String contents = '''\
36223626
|class C extends A {
36233627
| def three
36243628
| void test() {
@@ -3627,34 +3631,71 @@ final class SemanticHighlightingTests extends GroovyEclipseTestSuite {
36273631
| three
36283632
| empty
36293633
| isEmpty()
3634+
| something
36303635
| }
36313636
|}
36323637
|'''.stripMargin()
36333638

36343639
assertHighlighting(contents,
3635-
new HighlightedTypedPosition(contents.indexOf('A'), 1, ABSTRACT_CLASS),
3636-
new HighlightedTypedPosition(contents.indexOf('HashMap'), 7, CLASS),
3637-
new HighlightedTypedPosition(contents.indexOf('one'), 3, FIELD),
3638-
new HighlightedTypedPosition(contents.indexOf('two'), 3, FIELD),
36393640
new HighlightedTypedPosition(contents.indexOf('C'), 1, CLASS),
3640-
new HighlightedTypedPosition(contents.lastIndexOf('A'), 1, ABSTRACT_CLASS),
3641+
new HighlightedTypedPosition(contents.indexOf('A'), 1, ABSTRACT_CLASS),
36413642
new HighlightedTypedPosition(contents.indexOf('three'), 5, FIELD),
36423643
new HighlightedTypedPosition(contents.indexOf('test'), 4, METHOD),
3643-
new HighlightedTypedPosition(contents.lastIndexOf('one'), 3, MAP_KEY),
3644+
new HighlightedTypedPosition(contents.lastIndexOf('one'), 3, isAtLeastGroovy(50) ? FIELD : MAP_KEY),
36443645
new HighlightedTypedPosition(contents.lastIndexOf('two'), 3, FIELD),
36453646
new HighlightedTypedPosition(contents.lastIndexOf('three'), 5, FIELD),
36463647
new HighlightedTypedPosition(contents.lastIndexOf('empty'), 5, MAP_KEY),
3647-
new HighlightedTypedPosition(contents.lastIndexOf('isEmpty'), 7, METHOD_CALL))
3648+
new HighlightedTypedPosition(contents.lastIndexOf('isEmpty'), 7, METHOD_CALL),
3649+
new HighlightedTypedPosition(contents.lastIndexOf('something'), 9, isAtLeastGroovy(50) ? METHOD_CALL : MAP_KEY))
36483650
}
36493651

3650-
@Test // GROOVY-5491
3652+
@Test // GROOVY-5001
36513653
void testMapKey6() {
3654+
addGroovySource '''\
3655+
|abstract class A extends HashMap {
3656+
| def one, two = { -> }
3657+
| def getSomething() {}
3658+
|}
3659+
|'''.stripMargin()
3660+
36523661
String contents = '''\
3662+
|class C extends A {
3663+
| def three
3664+
| void test() {{ ->
3665+
| one
3666+
| two()
3667+
| three
3668+
| empty
3669+
| isEmpty()
3670+
| something
3671+
| }}
3672+
|}
3673+
|'''.stripMargin()
3674+
3675+
assertHighlighting(contents,
3676+
new HighlightedTypedPosition(contents.indexOf('C'), 1, CLASS),
3677+
new HighlightedTypedPosition(contents.indexOf('A'), 1, ABSTRACT_CLASS),
3678+
new HighlightedTypedPosition(contents.indexOf('three'), 5, FIELD),
3679+
new HighlightedTypedPosition(contents.indexOf('test'), 4, METHOD),
3680+
new HighlightedTypedPosition(contents.lastIndexOf('one'), 3, isAtLeastGroovy(50) ? FIELD : MAP_KEY),
3681+
new HighlightedTypedPosition(contents.lastIndexOf('two'), 3, FIELD),
3682+
new HighlightedTypedPosition(contents.lastIndexOf('three'), 5, isAtLeastGroovy(50) ? FIELD : MAP_KEY),
3683+
new HighlightedTypedPosition(contents.lastIndexOf('empty'), 5, MAP_KEY),
3684+
new HighlightedTypedPosition(contents.lastIndexOf('isEmpty'), 7, METHOD_CALL),
3685+
new HighlightedTypedPosition(contents.lastIndexOf('something'), 9, isAtLeastGroovy(50) ? METHOD_CALL : MAP_KEY))
3686+
}
3687+
3688+
@Test // GROOVY-5491
3689+
void testMapKey7() {
3690+
addGroovySource '''\
36533691
|abstract class A extends HashMap {
36543692
| def one
36553693
| protected two
36563694
| private xxx
36573695
|}
3696+
|'''.stripMargin()
3697+
3698+
String contents = '''\
36583699
|class C extends A {
36593700
| final three
36603701
| void test() {
@@ -3668,13 +3709,8 @@ final class SemanticHighlightingTests extends GroovyEclipseTestSuite {
36683709
|'''.stripMargin()
36693710

36703711
assertHighlighting(contents,
3671-
new HighlightedTypedPosition(contents.indexOf('A'), 1, ABSTRACT_CLASS),
3672-
new HighlightedTypedPosition(contents.indexOf('HashMap'), 7, CLASS),
3673-
new HighlightedTypedPosition(contents.indexOf('one'), 3, FIELD),
3674-
new HighlightedTypedPosition(contents.indexOf('two'), 3, FIELD),
3675-
new HighlightedTypedPosition(contents.indexOf('xxx'), 3, FIELD),
36763712
new HighlightedTypedPosition(contents.indexOf('C'), 1, CLASS),
3677-
new HighlightedTypedPosition(contents.lastIndexOf('A'), 1, ABSTRACT_CLASS),
3713+
new HighlightedTypedPosition(contents.indexOf('A'), 1, ABSTRACT_CLASS),
36783714
new HighlightedTypedPosition(contents.indexOf('three'), 5, FIELD),
36793715
new HighlightedTypedPosition(contents.indexOf('test'), 4, METHOD),
36803716
new HighlightedTypedPosition(contents.lastIndexOf('one'), 3, FIELD),
@@ -3685,7 +3721,7 @@ final class SemanticHighlightingTests extends GroovyEclipseTestSuite {
36853721
}
36863722

36873723
@Test // GROOVY-5491
3688-
void testMapKey7() {
3724+
void testMapKey8() {
36893725
addGroovySource '''\
36903726
|import groovy.transform.PackageScope
36913727
|abstract class A {
@@ -3731,6 +3767,29 @@ final class SemanticHighlightingTests extends GroovyEclipseTestSuite {
37313767
new HighlightedTypedPosition(contents.lastIndexOf('eight'), 5, METHOD_CALL))
37323768
}
37333769

3770+
@Test // GROOVY-8065
3771+
void testMapKey9() {
3772+
addGroovySource '''\
3773+
|class C extends HashMap {
3774+
| def getSomething() {}
3775+
|}
3776+
|'''.stripMargin()
3777+
3778+
String contents = '''\
3779+
|@groovy.transform.CompileStatic
3780+
|void test(C c) {
3781+
| c.something
3782+
|}
3783+
|'''.stripMargin()
3784+
3785+
assertHighlighting(contents,
3786+
new HighlightedTypedPosition(contents.indexOf('test'), 4, METHOD),
3787+
new HighlightedTypedPosition(contents.indexOf('C '), 1, CLASS),
3788+
new HighlightedTypedPosition(contents.indexOf('c)'), 1, PARAMETER),
3789+
new HighlightedTypedPosition(contents.lastIndexOf('c'), 1, PARAMETER),
3790+
new HighlightedTypedPosition(contents.lastIndexOf('something'), 9, isAtLeastGroovy(50) ? METHOD_CALL : MAP_KEY))
3791+
}
3792+
37343793
@Test
37353794
void testSpread() {
37363795
String contents = '''\

‎jdt-patch/e430/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/ProblemReporter.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4903,7 +4903,7 @@ public void invalidParenthesizedExpression(ASTNode reference) {
49034903
reference.sourceEnd);
49044904
}
49054905
public void invalidType(ASTNode location, TypeBinding type) {
4906-
try (this) { // ensure clean-up despite the many exits without calling handle(..)
4906+
try (ProblemReporter that = this) { // ensure clean-up despite the many exits without calling handle(..)
49074907
if (type instanceof ReferenceBinding) {
49084908
if (isRecoveredName(((ReferenceBinding)type).compoundName)) return;
49094909
}
@@ -9856,7 +9856,7 @@ private boolean validateRestrictedKeywords(char[] name, int start, int end, bool
98569856
}
98579857
public boolean validateRestrictedKeywords(char[] name, ASTNode node) {
98589858
// ensure clean-up because inner method is not guaranteed to invoke handle(..)
9859-
try (this) {
9859+
try (ProblemReporter that = this) {
98609860
return validateRestrictedKeywords(name, node.sourceStart, node.sourceEnd, false);
98619861
}
98629862
}
@@ -12584,7 +12584,7 @@ public boolean scheduleProblemForContext(Runnable problemComputation) {
1258412584
if (this.referenceContext != null) {
1258512585
CompilationResult result = this.referenceContext.compilationResult();
1258612586
if (result != null) {
12587-
try (this) {
12587+
try (ProblemReporter that = this) {
1258812588
ReferenceContext capturedContext = this.referenceContext;
1258912589
result.scheduleProblem(() -> {
1259012590
ReferenceContext save = this.referenceContext;

0 commit comments

Comments
 (0)