Skip to content

Commit

Permalink
Fix for issue #377: apply closure resolve strategy recursively
Browse files Browse the repository at this point in the history
  • Loading branch information
@eric-milles
eric-milles committed Nov 10, 2017
1 parent f7c9c27 commit afd444f
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 53 deletions.
56 changes: 44 additions & 12 deletions ...e.tests.builder/src/org/eclipse/jdt/core/groovy/tests/search/ClosureInferencingTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,18 +109,14 @@ public void testClosure7() {
" this\n" +
" super\n" +
" owner\n" +
" getOwner()\n" +
" delegate\n" +
" getDelegate()\n" +
" }\n" +
" }\n" +
"}";
assertExprType(contents, "this", "Bar");
assertExprType(contents, "super", "Foo");
assertExprType(contents, "owner", "Bar");
assertExprType(contents, "getOwner", "Bar");
assertExprType(contents, "delegate", "Bar");
assertExprType(contents, "getDelegate", "Bar");
}

@Test // closure with non-default resolve strategy
Expand All @@ -133,18 +129,14 @@ public void testClosure8() {
" this\n" +
" super\n" +
" owner\n" +
" getOwner()\n" +
" delegate\n" +
" getDelegate()\n" +
" }\n" +
" }\n" +
"}";
assertExprType(contents, "this", "Bar");
assertExprType(contents, "super", "java.lang.Object");
assertExprType(contents, "owner", "Bar");
assertExprType(contents, "getOwner", "Bar");
assertExprType(contents, "delegate", "Foo");
assertExprType(contents, "getDelegate", "Foo");
}

@Test // closure within static scope wrt owner
Expand All @@ -155,16 +147,12 @@ public void testClosure9() {
" static void main(args) {\n" +
" def closure = {\n" +
" owner\n" +
" getOwner()\n" +
" delegate\n" +
" getDelegate()\n" +
" }\n" +
" }\n" +
"}";
assertExprType(contents, "owner", "java.lang.Class<Bar>");
assertExprType(contents, "getOwner", "java.lang.Class<Bar>");
assertExprType(contents, "delegate", "java.lang.Class<Bar>");
assertExprType(contents, "getDelegate", "java.lang.Class<Bar>");
}

@Test
Expand Down Expand Up @@ -596,6 +584,50 @@ public void testDoubleClosure9() {
assertExprType(contents, "this", "Search");
}

@Test // https://github.com/groovy/groovy-eclipse/issues/377
public void testDoubleClosure10() {
String contents =
"import java.beans.*\n" +
"class B extends PropertyChangeSupport {\n" +
" Number info\n" +
"}\n" +
"class C {\n" +
" B bean\n" +
" void init() {\n" +
" bean.with {\n" +
" addPropertyChangeListener('name') { PropertyChangeEvent event ->\n" +
" info\n" + // from outer delegate
" }" +
" }\n" +
" }\n" +
"}";
int offset = contents.lastIndexOf("info");
assertType(contents, offset, offset + 4, "java.lang.Number");
assertDeclaringType(contents, offset, offset + 4, "B"); // outer delegate
}

@Test // https://github.com/groovy/groovy-eclipse/issues/377
public void testDoubleClosure11() {
String contents =
"import java.beans.*\n" +
"class B extends PropertyChangeSupport {\n" +
" boolean meth(args) { }\n" +
"}\n" +
"class C {\n" +
" B bean\n" +
" void init() {\n" +
" bean.with {\n" +
" addPropertyChangeListener('name') { PropertyChangeEvent event ->\n" +
" meth(1, '2', ~/3/)\n" + // from outer delegate
" }" +
" }\n" +
" }\n" +
"}";
int offset = contents.lastIndexOf("meth");
assertType(contents, offset, offset + 4, "java.lang.Boolean");
assertDeclaringType(contents, offset, offset + 4, "B"); // outer delegate
}

@Test // Closure type inference without @CompileStatic
public void testCompileStaticClosure0() {
assumeTrue(isAtLeastGroovy(22));
Expand Down
70 changes: 45 additions & 25 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 @@ -159,18 +159,24 @@ protected ClassNode findDeclaringType(Expression node, VariableScope scope, Type
return ((StaticMethodCallExpression) node).getOwnerType();

} else if (node instanceof ConstantExpression && scope.isMethodCall()) {
// method call without an object expression; TODO: requires same handling as a free variable
return scope.getDelegateOrThis();
// method call without an object expression; requires same handling as a free variable
ClassNode ownerType;
if (scope.getEnclosingClosure() != null) {
ownerType = scope.getOwner();
} else {
ownerType = scope.getEnclosingTypeDeclaration();
}
return ownerType;

} else if (node instanceof VariableExpression) {
Variable var = ((VariableExpression) node).getAccessedVariable();
if (var != null && !(var instanceof Parameter || var instanceof VariableExpression)) {
ClassNode ownerType;
/*if (scope.getEnclosingClosure() != null) {
ownerType = scope.lookupName("getOwner").type;
} else {*/
if (scope.getEnclosingClosure() != null) {
ownerType = scope.getOwner();
} else {
ownerType = scope.getEnclosingTypeDeclaration();
/*}*/
}
return ownerType;
}
// else local variable
Expand All @@ -188,27 +194,33 @@ protected TypeLookupResult findType(Expression node, ClassNode declaringType, Va

if (node instanceof VariableExpression) {
return findTypeForVariable((VariableExpression) node, scope, confidence, declaringType);
}

// short-circuit if object expression is part of direct field access (aka AttributeExpression)
if (!isPrimaryExpression && node instanceof ConstantExpression && scope.getEnclosingNode() instanceof AttributeExpression) {
ClassNode clazz = !isStaticObjectExpression ? declaringType : declaringType.getGenericsTypes()[0].getType();
FieldNode field = clazz.getDeclaredField(node.getText()); // don't search super types (see GROOVY-8167)
if (isCompatible(field, isStaticObjectExpression)) {
return new TypeLookupResult(field.getType(), clazz, field, TypeConfidence.EXACT, scope);
} else {
return new TypeLookupResult(VariableScope.VOID_CLASS_NODE, clazz, null, TypeConfidence.UNKNOWN, scope);
} else if (node instanceof ConstantExpression && isPrimaryExpression && scope.isMethodCall()) {
VariableExpression expr = new VariableExpression(new DynamicVariable(node.getText(), false));
TypeLookupResult result = findTypeForVariable(expr, scope, confidence, declaringType);
if (isCompatible((AnnotatedNode) result.declaration, isStaticObjectExpression)) {
return result;
}
}

// NOTE: method calls with no object expression go here instead of findTypeForVariable
ClassNode nodeType = node.getType();
if ((!isPrimaryExpression || scope.isMethodCall()) && node instanceof ConstantExpression) {
return findTypeForNameWithKnownObjectExpression(node.getText(), nodeType, declaringType, scope, confidence,
isStaticObjectExpression, isPrimaryExpression, /*isLhsExpression:*/(scope.getWormhole().remove("lhs") == node));
}

if (node instanceof ConstantExpression) {
if (!isPrimaryExpression) {
// short-circuit if object expression is part of direct field access (aka AttributeExpression)
if (scope.getEnclosingNode() instanceof AttributeExpression) {
ClassNode clazz = !isStaticObjectExpression ? declaringType : declaringType.getGenericsTypes()[0].getType();
FieldNode field = clazz.getDeclaredField(node.getText()); // don't search super types (see GROOVY-8167)
if (isCompatible(field, isStaticObjectExpression)) {
return new TypeLookupResult(field.getType(), clazz, field, TypeConfidence.EXACT, scope);
} else {
return new TypeLookupResult(VariableScope.VOID_CLASS_NODE, clazz, null, TypeConfidence.UNKNOWN, scope);
}
}
}
if (!isPrimaryExpression || scope.isMethodCall()) {
return findTypeForNameWithKnownObjectExpression(node.getText(), nodeType, declaringType, scope, confidence,
isStaticObjectExpression, isPrimaryExpression, /*isLhsExpression:*/(scope.getWormhole().remove("lhs") == node));
}

ConstantExpression cexp = (ConstantExpression) node;
if (cexp.isNullExpression()) {
return new TypeLookupResult(VariableScope.VOID_CLASS_NODE, null, null, confidence, scope);
Expand Down Expand Up @@ -462,11 +474,19 @@ protected ASTNode findDeclarationForDynamicVariable(VariableExpression var, Clas
boolean isLhsExpr = (scope.getWormhole().remove("lhs") == var);

if (resolveStrategy == Closure.DELEGATE_FIRST || resolveStrategy == Closure.DELEGATE_ONLY) {
// TODO: If strategy is DELEGATE_ONLY and delegate is enclosing closure, do outer search.
candidate = findDeclaration(var.getName(), scope.getDelegate(), isLhsExpr, false, false, callArgs);
}
if (candidate == null && resolveStrategy < Closure.DELEGATE_ONLY) {
candidate = findDeclaration(var.getName(), owner, isLhsExpr, scope.isOwnerStatic(), scope.isFieldAccessDirect(), callArgs);

VariableScope outer = owner.getNodeMetaData("outer.scope");
if (outer != null) { // owner is an enclosing closure
if (isLhsExpr) scope.getWormhole().put("lhs", var);
VariableScope.CallAndType cat = outer.getEnclosingMethodCallExpression();
int enclosingResolveStrategy = (cat == null ? 0 : cat.getResolveStrategy(outer.getEnclosingClosure()));
candidate = findDeclarationForDynamicVariable(var, outer.getOwner(), outer, enclosingResolveStrategy);
} else {
candidate = findDeclaration(var.getName(), owner, isLhsExpr, scope.isOwnerStatic(), scope.isFieldAccessDirect(), callArgs);
}
if (candidate == null && resolveStrategy < Closure.DELEGATE_FIRST && scope.getEnclosingClosure() != null) {
candidate = findDeclaration(var.getName(), scope.getDelegate(), isLhsExpr, false, false, callArgs);
}
Expand Down Expand Up @@ -754,7 +774,7 @@ protected static boolean isCompatible(AnnotatedNode declaration, boolean isStati
} else if (declaration instanceof PropertyNode) {
isStatic = ((PropertyNode) declaration).isStatic();
}
if (!isStaticExpression || isStatic || declaration.getDeclaringClass().equals(VariableScope.CLASS_CLASS_NODE)) {
if (!isStaticExpression || isStatic || VariableScope.CLASS_CLASS_NODE.equals(declaration.getDeclaringClass())) {
return true;
}
}
Expand Down
4 changes: 3 additions & 1 deletion ...dt.groovy.core/src/org/eclipse/jdt/groovy/search/TypeInferencingVisitorWithRequestor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1031,6 +1031,8 @@ public void visitClosureExpression(ClosureExpression node) {
// if enclosing closure, owner type is 'Closure', otherwise it's 'typeof(this)'
if (parent.getEnclosingClosure() != null) {
ClassNode closureType = GenericsUtils.nonGeneric(VariableScope.CLOSURE_CLASS_NODE);
closureType.putNodeMetaData("outer.scope", parent.getEnclosingClosureScope());

scope.addVariable("owner", closureType, VariableScope.CLOSURE_CLASS_NODE);
scope.addVariable("getOwner", closureType, VariableScope.CLOSURE_CLASS_NODE);
} else {
Expand Down Expand Up @@ -1062,7 +1064,7 @@ public void visitClosureExpression(ClosureExpression node) {
scope.addVariable("delegate", delegateType, VariableScope.CLOSURE_CLASS_NODE);
scope.addVariable("getDelegate", delegateType, VariableScope.CLOSURE_CLASS_NODE);
} else {
ClassNode delegateType = scope.lookupName("getOwner").type;
ClassNode delegateType = scope.getOwner();
// GRECLIPSE-1348: if someone is silly enough to have a variable named "delegate"; don't override it
VariableScope.VariableInfo inf = scope.lookupName("delegate");
if (inf == null || inf.scopeNode instanceof ClosureExpression) {
Expand Down
26 changes: 13 additions & 13 deletions base/org.eclipse.jdt.groovy.core/src/org/eclipse/jdt/groovy/search/VariableScope.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -562,30 +562,30 @@ public VariableInfo lookupNameInCurrentScope(String name) {
}

public ClassNode getThis() {
VariableInfo thiz = lookupName("this");
return thiz != null ? thiz.type : null;
VariableInfo info = lookupName("this");
return info != null ? info.type : null;
}

public ClassNode getDelegate() {
VariableInfo delegate = lookupName("delegate");
return delegate != null ? delegate.type : null;
public ClassNode getOwner() {
VariableInfo info = lookupName("getOwner");
return info != null ? info.type : null;
}

public ClassNode getDelegateOrThis() {
VariableInfo info = getDelegateOrThisInfo();
public ClassNode getDelegate() {
VariableInfo info = lookupName("getDelegate");
return info != null ? info.type : null;
}

/**
* @return the current delegate type if exists, or this type if exists.
* Returns null if in top level scope (i.e. in import statement).
*/
public VariableInfo getDelegateOrThisInfo() {
VariableInfo info = lookupName("delegate");
if (info == null) {
info = lookupName("this");
public ClassNode getDelegateOrThis() {
ClassNode type = getDelegate();
if (type == null) {
type = getThis();
}
return info; // might be null if in imports
return type;
}

/**
Expand Down Expand Up @@ -674,7 +674,7 @@ public int getEnclosingClosureResolveStrategy() {
return resolveStrategy;
}

private VariableScope getEnclosingClosureScope() {
/*package*/ VariableScope getEnclosingClosureScope() {
VariableScope scope = this;
do {
if (scope.scopeNode instanceof ClosureExpression) {
Expand Down
4 changes: 2 additions & 2 deletions ...c/org/codehaus/groovy/eclipse/codeassist/processors/LocalVariableCompletionProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ private List<ICompletionProposal> createClosureProposals() {
ContentAssistContext context = getContext();
if (context.currentScope != null && context.currentScope.getEnclosingClosure() != null) {
org.eclipse.jdt.groovy.search.VariableScope scope = context.currentScope;
org.eclipse.jdt.groovy.search.VariableScope.VariableInfo ownerInfo = scope.lookupName("owner");
org.eclipse.jdt.groovy.search.VariableScope.VariableInfo delegateInfo = scope.lookupName("delegate");
org.eclipse.jdt.groovy.search.VariableScope.VariableInfo ownerInfo = scope.lookupName("getOwner");
org.eclipse.jdt.groovy.search.VariableScope.VariableInfo delegateInfo = scope.lookupName("getDelegate");

List<ICompletionProposal> proposals = new ArrayList<ICompletionProposal>();
maybeAddClosureProperty(proposals, "owner", ownerInfo.declaringType, ownerInfo.type, false);
Expand Down

0 comments on commit afd444f

Please sign in to comment.