Skip to content

Commit

Permalink
better method selection for primitive types
Browse files Browse the repository at this point in the history 
#1111
  • Loading branch information
@eric-milles
eric-milles committed May 21, 2020
1 parent c3081f9 commit aff7837
Show file tree
Hide file tree
Showing 6 changed files with 41 additions and 29 deletions.
16 changes: 8 additions & 8 deletions ...e.tests.builder/src/org/eclipse/jdt/core/groovy/tests/search/ClosureInferencingTests.java
View file
Expand Up @@ -696,7 +696,7 @@ public void testNestedClosure5() {
String contents =
//@formatter:off
"''.with {\n" +
" 1.with {\n" +
" 1L.with {\n" +
" abs\n" +
" }\n" +
"}";
Expand All @@ -711,35 +711,35 @@ public void testNestedClosure6() {
String contents =
//@formatter:off
"''.with {\n" +
" 1.with {\n" +
" 1L.with {\n" +
" abs()\n" +
" }\n" +
"}";
//@formatter:on

assertType(contents, "abs", "java.lang.Integer");
assertType(contents, "abs", "java.lang.Long");
}

@Test
public void testNestedClosure7() {
String contents =
//@formatter:off
"''.with {\n" +
" 1.with {\n" +
" 1L.with {\n" +
" delegate.abs()\n" +
" }\n" +
"}";
//@formatter:on

assertType(contents, "abs", "java.lang.Integer");
assertType(contents, "abs", "java.lang.Long");
}

@Test
public void testNestedClosure8() {
String contents =
//@formatter:off
"''.with {\n" +
" 1.with {\n" +
" 1L.with {\n" +
" this.abs()\n" +
" }\n" +
"}";
Expand All @@ -754,7 +754,7 @@ public void testNestedClosure9() {
String contents =
//@formatter:off
"''.with {\n" +
" 1.with {\n" +
" 42.with {\n" +
" this\n" +
" }\n" +
"}";
Expand All @@ -768,7 +768,7 @@ public void testNestedClosure10() {
String contents =
//@formatter:off
"''.with {\n" +
" 1.with {\n" +
" 42.with {\n" +
" owner.thisObject\n" +
" owner.getThisObject()\n" +
" }\n" +
Expand Down
5 changes: 3 additions & 2 deletions ....core.tests.builder/src/org/eclipse/jdt/core/groovy/tests/search/DGMInferencingTests.java
View file
Expand Up @@ -390,6 +390,7 @@ public void testDGM29() {
public void testDGM30() {
String contents =
//@formatter:off
"import groovy.io.FileType\n" +
"new File('test').eachFileMatch(FileType.FILES, 1) { it.name }";
//@formatter:on

Expand Down Expand Up @@ -684,10 +685,10 @@ public void testDGM49() {
public void testDGM50() {
String contents =
//@formatter:off
"def answer = (-42).&abs\n";
"def answer = (-42L).&abs\n";
//@formatter:on

assertExprType(contents, "abs", "java.lang.Integer");
assertExprType(contents, "abs", "java.lang.Long");
}

@Test // https://github.com/groovy/groovy-eclipse/issues/1027
Expand Down
14 changes: 9 additions & 5 deletions base/org.eclipse.jdt.groovy.core/src/org/eclipse/jdt/groovy/core/util/GroovyUtils.java
View file
Expand Up @@ -38,6 +38,7 @@
import org.codehaus.groovy.ast.ClassNode;
import org.codehaus.groovy.ast.FieldNode;
import org.codehaus.groovy.ast.GenericsType;
import org.codehaus.groovy.ast.ImmutableClassNode;
import org.codehaus.groovy.ast.ImportNode;
import org.codehaus.groovy.ast.InnerClassNode;
import org.codehaus.groovy.ast.MethodNode;
Expand All @@ -50,6 +51,7 @@
import org.codehaus.groovy.ast.expr.VariableExpression;
import org.codehaus.groovy.ast.tools.GeneralUtils;
import org.codehaus.groovy.control.CompilePhase;
import org.codehaus.groovy.runtime.MetaClassHelper;
import org.codehaus.groovy.transform.ASTTransformation;
import org.codehaus.groovy.transform.GroovyASTTransformation;
import org.codehaus.groovy.transform.trait.Traits;
Expand Down Expand Up @@ -391,10 +393,7 @@ private static boolean isAssignable(boolean array, ClassNode source, ClassNode t
}

boolean result;
/*if (source.hasClass() && target.hasClass()) {
// this matches primitives more thoroughly, but getTypeClass can fail if class has not been loaded
result = MetaClassHelper.isAssignableFrom(target.getTypeClass(), source.getTypeClass());
} else*/ if (target.isInterface()) {
if (target.isInterface()) {
result = GeneralUtils.isOrImplements(source, target);
} else if (array) {
if (target.isGenericsPlaceHolder() || target.equals(ClassHelper.OBJECT_TYPE)) {
Expand All @@ -404,7 +403,12 @@ private static boolean isAssignable(boolean array, ClassNode source, ClassNode t
result = source.isDerivedFrom(target);
}
} else {
result = getWrapperTypeIfPrimitive(source).isDerivedFrom(getWrapperTypeIfPrimitive(target));
if (source.redirect() instanceof ImmutableClassNode && target.redirect() instanceof ImmutableClassNode) {
// this matches primitives more thoroughly, but getTypeClass fails if class isn't loaded
result = MetaClassHelper.isAssignableFrom(target.getTypeClass(), source.getTypeClass());
} else {
result = getWrapperTypeIfPrimitive(source).isDerivedFrom(target);
}
}

// if target is like <T extends A & B>, check source against B
Expand Down
30 changes: 18 additions & 12 deletions base/org.eclipse.jdt.groovy.core/src/org/eclipse/jdt/groovy/search/CategoryTypeLookup.java
View file
Expand Up @@ -39,7 +39,7 @@
public class CategoryTypeLookup implements ITypeLookup {

@Override
public TypeLookupResult lookupType(Expression node, VariableScope scope, ClassNode objectExpressionType) {
public TypeLookupResult lookupType(final Expression node, final VariableScope scope, final ClassNode objectExpressionType) {
if (node instanceof VariableExpression || isCompatibleConstantExpression(node, scope, objectExpressionType)) {
String simpleName = node.getText();
ClassNode selfType = GroovyUtils.getWrapperTypeIfPrimitive(
Expand Down Expand Up @@ -92,8 +92,8 @@ public TypeLookupResult lookupType(Expression node, VariableScope scope, ClassNo
return null;
}

protected static boolean isCompatibleConstantExpression(Expression node, VariableScope scope, ClassNode selfType) {
if (node instanceof ConstantExpression && !scope.isTopLevel()) {
protected static boolean isCompatibleConstantExpression(final Expression node, final VariableScope scope, final ClassNode selfType) {
if (node instanceof ConstantExpression && !scope.isTopLevel() && !VariableScope.VOID_CLASS_NODE.equals(selfType)) {
org.codehaus.groovy.ast.ASTNode enclosingNode = scope.getEnclosingNode();
if (!(enclosingNode instanceof AttributeExpression || (enclosingNode instanceof MethodPointerExpression &&
VariableScope.CLASS_CLASS_NODE.equals(selfType)))) {
Expand All @@ -103,7 +103,7 @@ protected static boolean isCompatibleConstantExpression(Expression node, Variabl
return false;
}

protected static boolean isCompatibleCategoryMethod(MethodNode method, ClassNode firstArgumentType, VariableScope scope) {
protected static boolean isCompatibleCategoryMethod(final MethodNode method, final ClassNode firstArgumentType, final VariableScope scope) {
if (method.isStatic()) {
Parameter[] paramters = method.getParameters();
if (paramters != null && paramters.length > 0) {
Expand All @@ -119,34 +119,34 @@ protected static boolean isCompatibleCategoryMethod(MethodNode method, ClassNode
return false;
}

protected static boolean isTypeCompatible(ClassNode source, ClassNode target) {
protected static boolean isTypeCompatible(final ClassNode source, final ClassNode target) {
if (SimpleTypeLookup.isTypeCompatible(source, target) != Boolean.FALSE) {
if (!(VariableScope.CLASS_CLASS_NODE.equals(source) && source.isUsingGenerics()) ||
VariableScope.OBJECT_CLASS_NODE.equals(target) || !target.isUsingGenerics()) {
return true;
} else {
source = source.getGenericsTypes()[0].getType();
target = target.getGenericsTypes()[0].getType();
if (SimpleTypeLookup.isTypeCompatible(source, target) != Boolean.FALSE) {
ClassNode sourceGT = source.getGenericsTypes()[0].getType();
ClassNode targetGT = target.getGenericsTypes()[0].getType();
if (SimpleTypeLookup.isTypeCompatible(sourceGT, targetGT) != Boolean.FALSE) {
return true;
}
}
}
return false;
}

protected static boolean isDefaultGroovyMethod(MethodNode method, VariableScope scope) {
protected static boolean isDefaultGroovyMethod(final MethodNode method, final VariableScope scope) {
return (VariableScope.DGM_CLASS_NODE.equals(method.getDeclaringClass()) || scope.isDefaultCategory(method.getDeclaringClass()));
}

protected static boolean isDefaultGroovyStaticMethod(MethodNode method, VariableScope scope) {
protected static boolean isDefaultGroovyStaticMethod(final MethodNode method, final VariableScope scope) {
return (VariableScope.DGSM_CLASS_NODE.equals(method.getDeclaringClass()) || scope.isDefaultStaticCategory(method.getDeclaringClass()));
}

/**
* Selects the candidate that most closely matches the method call arguments.
*/
protected static MethodNode selectBestMatch(List<MethodNode> candidates, List<ClassNode> argumentTypes) {
protected static MethodNode selectBestMatch(final List<MethodNode> candidates, final List<ClassNode> argumentTypes) {
MethodNode method = null;
for (MethodNode candidate : candidates) {
if (argumentTypes.size() == candidate.getParameters().length) {
Expand All @@ -170,8 +170,14 @@ protected static MethodNode selectBestMatch(List<MethodNode> candidates, List<Cl
return method != null ? method : candidates.get(0);
}

protected static long calculateParameterDistance(List<ClassNode> arguments, Parameter[] parameters) {
protected static long calculateParameterDistance(final List<ClassNode> arguments, final Parameter[] parameters) {
try {
if (arguments.size() == 1 && parameters.length == 1) {
Class<?>[] args = {arguments.get(0).getTypeClass()};
Class<?>[] prms = {parameters[0].getType().getTypeClass()};
return MetaClassHelper.calculateParameterDistance(args, new ParameterTypes(prms));
}

// weight self type higher to prevent considering getAt(Map, Object)
// and getAt(Object, String) equally for the arguments (Map, String)

Expand Down
2 changes: 1 addition & 1 deletion ...ing.test/src/org/codehaus/groovy/eclipse/codebrowsing/tests/CodeSelectMethodsTests.groovy
View file
Expand Up @@ -771,7 +771,7 @@ final class CodeSelectMethodsTests extends BrowsingTestSuite {

@Test
void testCodeSelectConstuctorMultipleConstructors3() {
IMethod method = assertConstructor('new Date(0L)', 'Date')
IMethod method = assertConstructor('new Date(0)', 'Date')
assert method.parameters.length == 1: 'Should have found constructor with 1 arg'
assert method.parameterTypes[0] == 'J' : 'Should have found constructor Date(long)'
}
Expand Down
3 changes: 2 additions & 1 deletion ...c/org/codehaus/groovy/eclipse/codeassist/completions/GroovyExtendedCompletionContext.java
View file
Expand Up @@ -158,7 +158,8 @@ protected void addFields(ClassNode targetType, Map<String, IJavaElement> visible
}
for (IMethod method : type.getMethods()) {
ClassNode methodReturnTypeClassNode = toClassNode(method.getReturnType());
if (GroovyUtils.isAssignable(methodReturnTypeClassNode, targetType)) {
if (!VariableScope.VOID_CLASS_NODE.equals(methodReturnTypeClassNode) &&
GroovyUtils.isAssignable(methodReturnTypeClassNode, targetType)) {
if ((method.getParameterTypes() == null || method.getParameterTypes().length == 0) &&
(method.getElementName().startsWith("get") || method.getElementName().startsWith("is"))) {
visibleElements.putIfAbsent(method.getElementName(), method);
Expand Down

0 comments on commit aff7837

Please sign in to comment.