Wrong code navigation with switch and def

[mauromol]

Consider the following:

package test15;
public enum MyEnum {
  A, B;
}

And:

package test15
class Test15 {
	void foo(String s, MyEnum e) {
		def d1, d2
		switch(e) {
			case MyEnum.A:
			 d1 = new Date()
			 d2 = new Date()
			 break
			case MyEnum.B:
			  d1 = null
			  d2 = null
			  break
		}
		foo(s, d1, d2)
	}

	void foo(String s, Date d1, Date d2) {
		
	}
}

Hit F3 over the call to foo(String, Date, Date) in foo(String, MyEnum) and/or look at the hover: Greclipse thinks that call is for foo(String, MyEnum) itself.
The cause of the problem seems to be d1 and d2 be declared as def together with the presence of the switch.
I think this case is a bit complicated to correctly analyse, but probably something can be done, because if you invoke Call Hierarchy (Ctrl+Alt+H) from foo(String, Date, Date), the call from foo(String, MyEnum) is correctly detected!

[eric-milles]

The selection from multiple candidates is implemented here in SimpleTypeLookup.

    protected MethodNode findMethodDeclaration0(List<MethodNode> candidates, List<ClassNode> arguments) {
        // remember first entry in case exact match not found
        MethodNode closestMatch = candidates.get(0);
        if (arguments == null) {
            arguments = Collections.emptyList();
        }

        // prefer retrieving the method with the same number of args as specified in the parameter.
        // if none exists, or parameter is -1, then arbitrarily choose the first.
        for (Iterator<MethodNode> iterator = candidates.iterator(); iterator.hasNext();) {
            MethodNode maybeMethod = iterator.next();
            Parameter[] parameters = maybeMethod.getParameters();
            if (parameters.length == 0 && arguments.isEmpty()) {
                return maybeMethod.getOriginal();
            }
            if (parameters.length == arguments.size()) {
                Boolean suitable = isTypeCompatible(arguments, parameters);
                if (Boolean.TRUE.equals(suitable)) {
                    return maybeMethod.getOriginal();
                }
                if (!Boolean.FALSE.equals(suitable)) {
                    closestMatch = maybeMethod.getOriginal();
                    continue; // don't remove
                }
            }
            iterator.remove();
        }
        return closestMatch;
    }

[eric-milles]

I can make a simple fix to handle your specific case. That is, overloads where the desired match has number of parameters matching arguments and all other overloads do not have matching number of parameters. This does not generalize to type inferencing for def to Date in the sample code. The variables d1 and d2 are still seen as Object type, which has a number of implications for type inferencing. For example, you could not have completions on d1 after the switch for Date methods.

This will also not fix the ambiguous case of variadic methods. For example, if you had void foo(String s, Date... d) it would not be matched.

[mauromol]

I see, but then (when multiple overloads have the same number of parameters, but of different types) probably also compilation with @CompileStatic fails, or at least an unexpected result is given at runtime... If Greclipse is coherent with what the Groovy compiler/runtime does, it's fine for me, because at that point any ambiguity is my own responsibility.

But, at least in this specific case, there's no ambiguity, at least for the Groovy compiler/runtime.

[eric-milles]

Part of the issue is that the selection of foo (unless @CompileStatic is on) is determined at run time, when type information is readily available. Greclipse is trying to figure this out statically and so imperfect choices have to be made due to the dynamic nature of Groovy.

Note: If @CompileStatic is enabled, then Greclipse uses the type information that Groovy saves in the AST to select the method.

[mauromol]

I perfectly understand this is not a trivial case, thank you!

[eric-milles]

Ready to test in Eclipse Oxygen

[mauromol]

Verified in 3.0.0.xx-201801112223-e47, thank you! 👍

I see you're changing the name of this plugin to "Eclipse Groovy Development Tools", which sounds good to me. Why not then taking the opportunity to also change the packaging away from org.codehaus then? :-)