DSLD: Context information popup highlights wrong parameter name in case of a trailing closure

[ybayk]

Consider the following DSLD:
method name: "foo", type: "void", noParens: true, namedParams: [bar: String], params: [block: 'groovy.lang.Closure']

If we select the foo method from the auto-complete menu, 'bar' is initially selected in the editor as expected, but the context popup highlights the 'block' in bold:

After hitting tab, the 'block' gets selected, but 'bar' gets highlighted:

The context popup is rendered deep inside Eclipse/Java logic which is not aware of Groovy nature of a trailing closure parameter.
I would propose to "fix" the order of parameters just in the context popup, so at least switching and highlighting would work consistently with the editor, like this:

[ybayk]

PR is coming along with fixes for #603

[eric-milles]

In general, no use of named parameters has highlighting that matches the context information. Please be sure to test non-DSLD cases where Map is first method parameter.

[eric-milles]

If you plan on developing PR immediately, issue serves no purpose. All discussion and review can happen as part of PR.

[ybayk]

I just followed the protocol: https://github.com/groovy/groovy-eclipse/blob/master/docs/Getting-Started-with-Groovy-Eclipse-Source-Code.md#track-work

[ybayk]

yes, will make sure non-DSLD cases are not affected.

[ybayk]

@eric-milles The problem seems to be more wide. When mixing named and regular params, DSLD signatures are generated with named params in front, but core Java signatures (based on which context popup is rendered) are other way around (regular params first).
I wonder if this is a bug or intentional behavior:

groovy-eclipse/ide/org.codehaus.groovy.eclipse.codeassist.completion/src/org/codehaus/groovy/eclipse/codeassist/completions/GroovyJavaMethodCompletionProposal.java

Lines 503 to 505 in 02993bf

	char[] name = (i < npc ? namedParameterNames[i] : positionalParameterNames[i - npc]);
	// NOTE: named parameters come after positional parameters in the parameterTypes array
	char[] type = (i < npc ? parameterTypes[i + positionalParameterNames.length] : parameterTypes[i - npc]);

The resulting behavior of the context popup is similar to the trailing closure issue:

[ybayk]

The above snippet contradicts the following code, where it generates a param list for a method signature: regular params first, following by named params:

groovy-eclipse/ide/org.codehaus.groovy.eclipse.codeassist.completion/src/org/codehaus/groovy/eclipse/codeassist/completions/NamedArgsMethodNode.java

Lines 44 to 46 in ebd9549

	System.arraycopy(params, 0, allParams, 0, params.length);
	System.arraycopy(namedParams, 0, allParams, params.length, namedParams.length);
	System.arraycopy(optionalParams, 0, allParams, params.length + namedParams.length, optionalParams.length);

I believe the order should be consistent, so one of the two must be valid, and the other must be wrong.
I think the ultimate order in the proposed signature should be:

1. Regular (positional) params (except an optionally tagged trailing closure)
2. Required named params
3. Optional named params
4. Optionally tagged trailing closure

[eric-milles]

Re issue vs. pull request: I prefer an issue followed by discussion before any coding takes place. If you submit a comment immediately stating you have a PR forthcoming, I assume you are not receptive to discussion of direction and options. So why not just a PR. That said, let me restate that I prefer discussion here or on slack before moving to coding.

As I mentioned above, yes the issue of context hover of method call parameters with named parameters is wider than just DSLD. When you have def x(Map np, int one, def two), if using named arguments, you typically do not pass the map and you may have the named arguments interleaved with the positional ones: x(one, named:three, two, named2:four).

The Map being a faux-positional param and the interleaving makes deciding which parameter to highlight in the hover non-trivial.

As for the DSL NamedArgsMethodNode, I don't know why the named params are displayed first. It may have something to do with recognizing that Closure is the last parameter and so may be completed outside the parentheses.

[ybayk]

Sorry just meant that I started work on a PR to avoid other contributors jump on an the issue.
After I got more familiar with the code, I tend to agree you. Named params come first because it was easy to deal with a potential trailing closure which is handled as a special case.

What we currently have is a disagreement in parameter order between NamedArgsMethodNode and GroovyJavaMethodCompletionProposal
Former: 1) positional 2) trailing closure 3) named
Latter: 1) named, 2) positional 3) trailing closure

I believe either is actually bad, and the ideal order should be:

1. positional (excluding trailing closure), 2) named, 3) trailing closure

What I propose to fix:

1. In NamedArgsMethodNode: Change the logic of concatParams(), so that the trailing closure (if present) is appended at the end of the parameter list

2. In GroovyJavaMethodCompletionProposal: Move positional parameters to the front, but keep the trailing closure at the end

Yes, the changes are not trivial, and I now think this work should be separate from #603

@eric-milles, Does it all make sense ?

[eric-milles]

You can assign the issue to yourself. If that option does not sow for you, I can assign it.

If you try changing just GroovyJavaMethodCompletionProposal to your proposed order, do any of the tests fail (besides ones that perform the proposal replacement)? I'm not sure I would suggest separating the trailing closure from the positional params. It would require that you check the type of the last param for every proposal. And I don't think NamedArgsMethodNode needs to be altered for content assist to do what your looking for.

[ybayk]

I don't have write access to the repository, so I can't assign it to myself.

If NamedArgsMethodNode is not touched, another place where the signature can be altered is here:

groovy-eclipse/ide/org.codehaus.groovy.eclipse.codeassist.completion/src/org/codehaus/groovy/eclipse/codeassist/ProposalUtils.java

Lines 158 to 167 in bbb8942

	if (node instanceof NamedArgsMethodNode) {
	parameters = ((NamedArgsMethodNode) node).getVisibleParams();
	} else {
	parameters = node.getParameters();
	}
	String[] parameterTypes = new String[parameters.length - ignoreParameters];
	for (int i = 0, n = parameterTypes.length; i < n; i += 1) {
	parameterTypes[i] = createTypeSignatureStr(parameters[i + ignoreParameters].getType());
	}
	return node.getName() + Signature.createMethodSignature(parameterTypes, returnType);

But it does not feel like a right place, it calls getVisibleParams() which is already expected to return final signature order.

Are you suggesting to keep named parameters in front? In that case only a signature generation logic needs to be changed and GroovyJavaMethodCompletionProposal would remain as is.

[eric-milles]

Just wondering what the minimal change would be. And if you could run an experiment or two to see if an improvement could be made with just one participant altered. Not saying that's the end state, but wanting to see if we can get a look at something quickly.

Can you try a couple of your proposed changes and see what you see in terms of trade-offs?

[eric-milles]

Also, can you find a good test suite to add some tests to illustrate the issue? Or is it 100% to do with the little context info hover?

I agree with the original post that the proposal replacement text and the context information box are not in agreement. What I'm thinking about at the same time is if you typed out foo {...}, bar: xyz and then brought up the context hover (Ctrl+Space after a comma). In that case the parameter highlighting would line up.

I'm a tad worried that the named argument support is not very complete and we are going to end up with a lot of use cases to understand.

[ybayk]

The issue manifests itself in 2 places:

1. Context popup issue which messes up parameter selection behavior so I think it is more critical (see images attached in comments above)

2. Proposed method signatures in the autocompletion menu:
   

So it is visual inconsistency between what you see and what you get after selection:

The signatures in auto-complete popup (as well as params in context popup ) are controlled by concat() helper in NamedArgsMethodNode.
And the inserted statement is fully controlled by GroovyJavaMethodCompletionProposal

[eric-milles]

I can see grouping all the named params first followed by the positional ones for two reasons:

1. Trailing closure (positional parameter) will always be at the end
2. Named params will group up into first Map param for the case of traditional method accepting named arguments

To have proposal, replacement and context list named first followed by positional, what changes are required?

[eric-milles]

Do you think it is worth exploring including the named param label in the proposal and context popups?

[ybayk]

I will issue PR soon, I think ended up in minimal changes.
Not sure what you mean regarding named parameter label - all named parameters are already present in both popups, but they are indistinguishable from positional ones.