Skip to content

Commit

Permalink
GROOVY-5001, GROOVY-5491, GROOVY-6144, GROOVY-8065, GROOVY-8788
Browse files Browse the repository at this point in the history
  • Loading branch information
@eric-milles
eric-milles committed Nov 13, 2023
1 parent 7f5b698 commit 596bd72
Show file tree
Hide file tree
Showing 6 changed files with 138 additions and 46 deletions.
25 changes: 24 additions & 1 deletion ...re.tests.compiler/src/org/eclipse/jdt/groovy/core/tests/xform/StaticCompilationTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -897,7 +897,30 @@ public void testCompileStatic1521() {
runNegativeTest(sources, "");
}

@Test // see GROOVY-5001, GROOVY-5491, GROOVY-6144, GROOVY-8788
@Test
public void testCompileStatic5491() {
for (String mode : new String[] {"", "public", "protected", "@groovy.transform.PackageScope"}) {
//@formatter:off
String[] sources = {
"Main.groovy",
"class M extends HashMap<String,Number> {\n" +
mode + " void setFoo(foo) { print foo }\n" +
"}\n" +
"@groovy.transform.CompileStatic\n" +
"void test() {\n" +
" def map = new M()\n" +
" map.foo = 123\n" + // setter
" print map.foo\n" + // get(K)
"}\n" +
"test()\n",
};
//@formatter:on

runConformTest(sources, "123null");
}
}

@Test // see GROOVY-5001, GROOVY-6144, GROOVY-8065, GROOVY-8788
public void testCompileStatic5517() {
//@formatter:off
String[] sources = {
Expand Down
27 changes: 17 additions & 10 deletions ...ovy30/src/org/codehaus/groovy/transform/sc/transformers/StaticCompilationTransformer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@
import java.util.Iterator;
import java.util.Map;

import static org.codehaus.groovy.ast.tools.GeneralUtils.callX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.isOrImplements;
import static org.apache.groovy.ast.tools.ExpressionUtils.isThisOrSuper;

/**
* Some expressions use symbols as aliases to method calls (&lt;&lt;, +=, ...). In static compilation,
* if such a method call is found, we transform the original binary expression into a method
Expand Down Expand Up @@ -116,21 +120,24 @@ public Expression transform(final Expression expr) {
if (expr instanceof ConstructorCallExpression) {
return constructorCallTransformer.transformConstructorCall((ConstructorCallExpression) expr);
}
// GRECLIPSE add -- GROOVY-6097, GROOVY-7300, GROOVY-9089, et al.
// GRECLIPSE add -- GROOVY-6097, GROOVY-7300, GROOVY-8065, GROOVY-9089, GROOVY-10557, et al.
if (expr instanceof PropertyExpression) {
MethodNode dmct = expr.getNodeMetaData(org.codehaus.groovy.transform.stc.StaticTypesMarker.DIRECT_METHOD_CALL_TARGET);
// NOTE: BinaryExpressionTransformer handles the setter
if (dmct != null && dmct.getParameters().length == 0) {
PropertyExpression pe = (PropertyExpression) expr;

MethodCallExpression mce = new MethodCallExpression(transform(pe.getObjectExpression()), pe.getPropertyAsString(), MethodCallExpression.NO_ARGUMENTS);
mce.setImplicitThis(pe.isImplicitThis());
mce.setMethodTarget(dmct);
mce.setSourcePosition(pe);
mce.setSpreadSafe(pe.isSpreadSafe());
mce.setSafe(pe.isSafe());
mce.copyNodeMetaData(pe);
return mce;
if (!isOrImplements(getTypeChooser().resolveType(pe.getObjectExpression(),getClassNode()), ClassHelper.MAP_TYPE)) {
MethodCallExpression mce = callX(transform(pe.getObjectExpression()), pe.getPropertyAsString());
mce.setImplicitThis(pe.isImplicitThis());
mce.setMethodTarget(dmct);
mce.setSourcePosition(pe);
mce.setSpreadSafe(pe.isSpreadSafe());
mce.setSafe(pe.isSafe());
mce.copyNodeMetaData(pe);
return mce;
} else if (!isThisOrSuper(pe.getObjectExpression())) {
expr.removeNodeMetaData(org.codehaus.groovy.transform.stc.StaticTypesMarker.DIRECT_METHOD_CALL_TARGET);
}
}
return super.transform(expr);
}
Expand Down
23 changes: 14 additions & 9 deletions ...vy40/src/org/codehaus/groovy/transform/sc/transformers/PropertyExpressionTransformer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import static org.codehaus.groovy.ast.ClassHelper.MAP_TYPE;
import static org.codehaus.groovy.ast.tools.GeneralUtils.callX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.isOrImplements;
import static org.apache.groovy.ast.tools.ExpressionUtils.isThisOrSuper;

class PropertyExpressionTransformer {

Expand All @@ -39,15 +40,19 @@ class PropertyExpressionTransformer {
Expression transformPropertyExpression(final PropertyExpression pe) {
MethodNode dmct = pe.getNodeMetaData(StaticTypesMarker.DIRECT_METHOD_CALL_TARGET);
// NOTE: BinaryExpressionTransformer handles the setter
if (dmct != null && dmct.getParameters().length == 0 && !isOrImplements(scTransformer.getTypeChooser().resolveType(pe.getObjectExpression(), scTransformer.getClassNode()), MAP_TYPE)) {
MethodCallExpression mce = callX(scTransformer.transform(pe.getObjectExpression()), pe.getPropertyAsString()); // GRECLIPSE edit
mce.setImplicitThis(pe.isImplicitThis());
mce.setMethodTarget(dmct);
mce.setSourcePosition(pe);
mce.setSpreadSafe(pe.isSpreadSafe());
mce.setSafe(pe.isSafe());
mce.copyNodeMetaData(pe);
return mce;
if (dmct != null && dmct.getParameters().length == 0) {
if (!isOrImplements(scTransformer.getTypeChooser().resolveType(pe.getObjectExpression(), scTransformer.getClassNode()), MAP_TYPE)) {
MethodCallExpression mce = callX(scTransformer.transform(pe.getObjectExpression()), pe.getPropertyAsString()); // GRECLIPSE edit
mce.setImplicitThis(pe.isImplicitThis());
mce.setMethodTarget(dmct);
mce.setSourcePosition(pe);
mce.setSpreadSafe(pe.isSpreadSafe());
mce.setSafe(pe.isSafe());
mce.copyNodeMetaData(pe);
return mce;
} else if (!isThisOrSuper(pe.getObjectExpression())) { // GRECLIPSE add
pe.removeNodeMetaData(StaticTypesMarker.DIRECT_METHOD_CALL_TARGET);
}
}

return scTransformer.superTransform(pe);
Expand Down
10 changes: 4 additions & 6 deletions base/org.eclipse.jdt.groovy.core/src/org/eclipse/jdt/groovy/search/SimpleTypeLookup.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -617,7 +617,6 @@ protected ASTNode findDeclarationForDynamicVariable(final VariableExpression var
* Looks for the named member in the declaring type. Also searches super types.
* The result can be a field, method, or property.
*
* @param name the name of the field, method, constant or property to find
* @param declaringType the type in which the named member's declaration resides
* @param isLhsExpression {@code true} if named member is being assigned a value
* @param isStaticExpression {@code true} if member is being accessed statically
Expand Down Expand Up @@ -652,7 +651,7 @@ protected ASTNode findDeclaration(final String name, final ClassNode declaringTy

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

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

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

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

Expand Down
93 changes: 76 additions & 17 deletions ...vy.eclipse.tests/src/org/codehaus/groovy/eclipse/test/ui/SemanticHighlightingTests.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3613,12 +3613,16 @@ final class SemanticHighlightingTests extends GroovyEclipseTestSuite {
new HighlightedTypedPosition(contents.lastIndexOf('key'), 3, MAP_KEY))
}

@Test
@Test // GROOVY-5001
void testMapKey5() {
String contents = '''\
addGroovySource '''\
|abstract class A extends HashMap {
| def one, two = { -> }
| def getSomething() {}
|}
|'''.stripMargin()

String contents = '''\
|class C extends A {
| def three
| void test() {
Expand All @@ -3627,34 +3631,71 @@ final class SemanticHighlightingTests extends GroovyEclipseTestSuite {
| three
| empty
| isEmpty()
| something
| }
|}
|'''.stripMargin()

assertHighlighting(contents,
new HighlightedTypedPosition(contents.indexOf('A'), 1, ABSTRACT_CLASS),
new HighlightedTypedPosition(contents.indexOf('HashMap'), 7, CLASS),
new HighlightedTypedPosition(contents.indexOf('one'), 3, FIELD),
new HighlightedTypedPosition(contents.indexOf('two'), 3, FIELD),
new HighlightedTypedPosition(contents.indexOf('C'), 1, CLASS),
new HighlightedTypedPosition(contents.lastIndexOf('A'), 1, ABSTRACT_CLASS),
new HighlightedTypedPosition(contents.indexOf('A'), 1, ABSTRACT_CLASS),
new HighlightedTypedPosition(contents.indexOf('three'), 5, FIELD),
new HighlightedTypedPosition(contents.indexOf('test'), 4, METHOD),
new HighlightedTypedPosition(contents.lastIndexOf('one'), 3, MAP_KEY),
new HighlightedTypedPosition(contents.lastIndexOf('one'), 3, isAtLeastGroovy(50) ? FIELD : MAP_KEY),
new HighlightedTypedPosition(contents.lastIndexOf('two'), 3, FIELD),
new HighlightedTypedPosition(contents.lastIndexOf('three'), 5, FIELD),
new HighlightedTypedPosition(contents.lastIndexOf('empty'), 5, MAP_KEY),
new HighlightedTypedPosition(contents.lastIndexOf('isEmpty'), 7, METHOD_CALL))
new HighlightedTypedPosition(contents.lastIndexOf('isEmpty'), 7, METHOD_CALL),
new HighlightedTypedPosition(contents.lastIndexOf('something'), 9, isAtLeastGroovy(50) ? METHOD_CALL : MAP_KEY))
}

@Test // GROOVY-5491
@Test // GROOVY-5001
void testMapKey6() {
addGroovySource '''\
|abstract class A extends HashMap {
| def one, two = { -> }
| def getSomething() {}
|}
|'''.stripMargin()

String contents = '''\
|class C extends A {
| def three
| void test() {{ ->
| one
| two()
| three
| empty
| isEmpty()
| something
| }}
|}
|'''.stripMargin()

assertHighlighting(contents,
new HighlightedTypedPosition(contents.indexOf('C'), 1, CLASS),
new HighlightedTypedPosition(contents.indexOf('A'), 1, ABSTRACT_CLASS),
new HighlightedTypedPosition(contents.indexOf('three'), 5, FIELD),
new HighlightedTypedPosition(contents.indexOf('test'), 4, METHOD),
new HighlightedTypedPosition(contents.lastIndexOf('one'), 3, isAtLeastGroovy(50) ? FIELD : MAP_KEY),
new HighlightedTypedPosition(contents.lastIndexOf('two'), 3, FIELD),
new HighlightedTypedPosition(contents.lastIndexOf('three'), 5, isAtLeastGroovy(50) ? FIELD : MAP_KEY),
new HighlightedTypedPosition(contents.lastIndexOf('empty'), 5, MAP_KEY),
new HighlightedTypedPosition(contents.lastIndexOf('isEmpty'), 7, METHOD_CALL),
new HighlightedTypedPosition(contents.lastIndexOf('something'), 9, isAtLeastGroovy(50) ? METHOD_CALL : MAP_KEY))
}

@Test // GROOVY-5491
void testMapKey7() {
addGroovySource '''\
|abstract class A extends HashMap {
| def one
| protected two
| private xxx
|}
|'''.stripMargin()

String contents = '''\
|class C extends A {
| final three
| void test() {
Expand All @@ -3668,13 +3709,8 @@ final class SemanticHighlightingTests extends GroovyEclipseTestSuite {
|'''.stripMargin()

assertHighlighting(contents,
new HighlightedTypedPosition(contents.indexOf('A'), 1, ABSTRACT_CLASS),
new HighlightedTypedPosition(contents.indexOf('HashMap'), 7, CLASS),
new HighlightedTypedPosition(contents.indexOf('one'), 3, FIELD),
new HighlightedTypedPosition(contents.indexOf('two'), 3, FIELD),
new HighlightedTypedPosition(contents.indexOf('xxx'), 3, FIELD),
new HighlightedTypedPosition(contents.indexOf('C'), 1, CLASS),
new HighlightedTypedPosition(contents.lastIndexOf('A'), 1, ABSTRACT_CLASS),
new HighlightedTypedPosition(contents.indexOf('A'), 1, ABSTRACT_CLASS),
new HighlightedTypedPosition(contents.indexOf('three'), 5, FIELD),
new HighlightedTypedPosition(contents.indexOf('test'), 4, METHOD),
new HighlightedTypedPosition(contents.lastIndexOf('one'), 3, FIELD),
Expand All @@ -3685,7 +3721,7 @@ final class SemanticHighlightingTests extends GroovyEclipseTestSuite {
}

@Test // GROOVY-5491
void testMapKey7() {
void testMapKey8() {
addGroovySource '''\
|import groovy.transform.PackageScope
|abstract class A {
Expand Down Expand Up @@ -3731,6 +3767,29 @@ final class SemanticHighlightingTests extends GroovyEclipseTestSuite {
new HighlightedTypedPosition(contents.lastIndexOf('eight'), 5, METHOD_CALL))
}

@Test // GROOVY-8065
void testMapKey9() {
addGroovySource '''\
|class C extends HashMap {
| def getSomething() {}
|}
|'''.stripMargin()

String contents = '''\
|@groovy.transform.CompileStatic
|void test(C c) {
| c.something
|}
|'''.stripMargin()

assertHighlighting(contents,
new HighlightedTypedPosition(contents.indexOf('test'), 4, METHOD),
new HighlightedTypedPosition(contents.indexOf('C '), 1, CLASS),
new HighlightedTypedPosition(contents.indexOf('c)'), 1, PARAMETER),
new HighlightedTypedPosition(contents.lastIndexOf('c'), 1, PARAMETER),
new HighlightedTypedPosition(contents.lastIndexOf('something'), 9, isAtLeastGroovy(50) ? METHOD_CALL : MAP_KEY))
}

@Test
void testSpread() {
String contents = '''\
Expand Down
6 changes: 3 additions & 3 deletions ...dt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/ProblemReporter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4903,7 +4903,7 @@ public void invalidParenthesizedExpression(ASTNode reference) {
reference.sourceEnd);
}
public void invalidType(ASTNode location, TypeBinding type) {
try (this) { // ensure clean-up despite the many exits without calling handle(..)
try (ProblemReporter that = this) { // ensure clean-up despite the many exits without calling handle(..)
if (type instanceof ReferenceBinding) {
if (isRecoveredName(((ReferenceBinding)type).compoundName)) return;
}
Expand Down Expand Up @@ -9856,7 +9856,7 @@ private boolean validateRestrictedKeywords(char[] name, int start, int end, bool
}
public boolean validateRestrictedKeywords(char[] name, ASTNode node) {
// ensure clean-up because inner method is not guaranteed to invoke handle(..)
try (this) {
try (ProblemReporter that = this) {
return validateRestrictedKeywords(name, node.sourceStart, node.sourceEnd, false);
}
}
Expand Down Expand Up @@ -12584,7 +12584,7 @@ public boolean scheduleProblemForContext(Runnable problemComputation) {
if (this.referenceContext != null) {
CompilationResult result = this.referenceContext.compilationResult();
if (result != null) {
try (this) {
try (ProblemReporter that = this) {
ReferenceContext capturedContext = this.referenceContext;
result.scheduleProblem(() -> {
ReferenceContext save = this.referenceContext;
Expand Down

0 comments on commit 596bd72

Please sign in to comment.