Faulty/Undesired CodeCompletion behaviour #2620

[angelickite]

What it does

Add cursor awareness to various auto-completion situations.

Completions for CompletionOnMessageSendName as well as for QualifiedNameReference did not take the cursor position into consideration, leading to various downstream issues like failing to complete at all, not properly overwriting a source range or overwriting too eagerly.

fixes #2620

How to test

I've included some tests. A thorough description can be found in #2620.

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[jukzi]

I am not qualified to make a in-depth review, but i can help with some formals. it would help here if you start the code example with "Example:" otherwise it looks like out commented code

[jukzi]

why start a codeblock here?

[jukzi]

You can use a for-each loop here? would be easier to read.

[jukzi]

if this code should swap start and end, you could add it as comment.

[jukzi]

In future it will not be clear what "previous" refers to. Please instead describe what the previous behavior was.

[jukzi]

instead of using a Consumer for setup it would be easier to understand and maintain if you directly pass the initial values to the constructor. probably "cursorIsToTheLeftOfTheLParen" misses a "t" at the end and could be final.

[stephan-herrmann]

In order to unblock the change I'm applying some of these suggestions, but here's one correction: LParen is correct compiler lingo to refer to '(' (short for left parenthesis), nothing to do with parents :)

[stephan-herrmann]

Trying to unblock this issue I ...

• rebased the existing commit on HEAD (without any changes)
• added a commit of code simplifications (also considering @jukzi's comments) and additional tests.

One more observations, and one remaining task:

(1) We should call out that the change in CompletionTest.testCamelCaseLocalVariable1() fixed a test bug, which would affect proposals of Object.clone() and Object.finalize() in unrelated tests, if run after this one.

(2) I'm seeing a regression when trying the following in the IDE:

public class GH2620_1 {
	public static void main (String[] args) {
		int j = 1;
		Test local;
		if (true) { // the if-statement is crucial for this test.
			/*x*/local.ref.test|();
		}
	}
	public static class Test {
		public Test ref;
		public static void test(int i) {}
	}
}

Cursor location is indicated by |.

• Old behavior: argument j is proposed.
• New behavior: argument j is proposed only when completion operates in 'insert' mode. When selecting 'overwrite' no argument is proposed.

@angelickite can you find out, if this is a bug in JDT/UI, or can/should be fixed here?

[angelickite]

@stephan-herrmann Are we on the same page that this regression is foremost a side effect of the new approach? What I mean by that:

• The new approach is generally well behaved
• Specifically when the completion is operating on any part names (i.e.. local, ref and test in the above example), then the completion should only affect these parts names.
• If the method name is the target, only the method name should be operated on and the argument list not be touched, i.e existing arguments should remain as they are. (e.g. System.out.print|("Hello") -> System.out.println("Hello"))

So the regression then manifests itself in the case where we have an empty argument list and can potentially safely modify the argument list too as a bonus feature as there is "nothing to break"?

If this is the situation you refer to as the regression, looking back I did see this case appear but never gave it any attention or thought. Reason being that the completion behaviour around arguments always felt janky anyways as well as it not negatively impacting my workflow.

If this feature is truly highly important, I will take a look where it originates and whether there is a sensible solution.

Lastly, I personally would prefer this sidegrade every time over having the old behaviour back.

[stephan-herrmann]

• The new approach is generally well behaved

I generally agree.

So the regression then manifests itself in the case where we have an empty argument list and can potentially safely modify the argument list too as a bonus feature as there is "nothing to break"?

I didn't investigate the exact conditions under which arguments are proposed, perhaps my scenario is the only one, perhaps the feature applies much more broadly. I didn't test.

If this is the situation you refer to as the regression, looking back I did see this case appear but never gave it any attention or thought. Reason being that the completion behaviour around arguments always felt janky anyways as well as it not negatively impacting my workflow.

I agree that the behaviour around arguments has room for improvement. But that even more motivates me to be careful that we don't break behaviour that exists at present and may be very important to some users. A somewhat messy situation is no excuse for piling up more inconsistencies - and as I said the current state in this PR is inconsistent between insertion / overwrite modes.

For me personally, support for arguments in calls to methods with long parameter lists is actually very relevant.

If this feature is truly highly important, I will take a look where it originates and whether there is a sensible solution.

I'd appreciate. It would already help if we only find out if the problem is in jdt.core or jdt.ui.

Lastly, I personally would prefer this sidegrade every time over having the old behaviour back.

I wouldn't wont to judge one user's needs against those of another :)

[angelickite]

The regression is here: https://github.com/angelickite/eclipse.jdt.core/blob/aa0c66d368586cdc30c18511f260519c80c59ff7/org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/CompletionEngine.java#L3263-L3270

So it stems directly from the new behaviour where "cursor to left of lparen" is given special consideration.

Something like this just might do the trick (limited testing):

// we take the cursor location into consideration
if (messageSend.cursorIsToTheLeftOfTheLParen && messageSend.arguments != null) {
  setSourceAndTokenRange(messageSend.sourceStart, messageSend.sourceEnd);
else {
  setTokenRange(messageSend.sourceStart, messageSend.sourceEnd);
  if (messageSend.statementEnd > messageSend.sourceStart)
    setSourceRange(messageSend.sourceStart, messageSend.statementEnd);
}

(note the added messageSend.arguments != null)