@stephan-herrmann can you take a look please ?? Thanks

@angelickite thanks for narrowing it down so far! What is the most recent version of Eclipse that you tested?

@gayanper this smells a bit like work were we let assistNodeParent point to a control structure, which then affects the expected type (to expect boolean) etc. What's really strange, how could anything like that cause println("world") to be changed to print(false), the false appears in a position that should not be influenced by the assist node parent.

@stephan-herrmann I am usually and currently running on the latest publicly available version of eclipse. Also just grabbed a fresh copy from https://www.eclipse.org/downloads/ with

Eclipse IDE for Enterprise Java and Web Developers (includes Incubating components)
Version: 2024-06 (4.32.0)
Build id: 20240606-1231

The situation with that version is exactly as described so far. As I didn't figure out yet how to fully build eclipse from source I can not test any farther.

Version: 2024-06 (4.32.0)

Thanks, @angelickite So my hopes this might have been fixed in the meanwhile are void :)

first bad: e3517dd

Many thanks for this pointer! Unfortunately, it points to a major refactoring, which at that time fixed many bugs at once, but apparently, there are negative effects as well :(

This implies there will be little use in delta-debugging before and after states. There won't be a short cut around debugging the "new strategy" in detail.

I did some more digging.

Here is where the completion gets recognized in the "bad" case:

Here is why the source gets overwriten:

• It seems like the recognition site is already aware of the special case of the cursor being left to the paren
• The messageSend.statementEnd is not desired for that special case

Please note that I did all the debugging around eclipse 2021-06 so far, though the findings should be applicable for the current codebase.

Hope this helps!

It looks like the new strategy introduced with e3517dd is potentially more "potent" overall but lost some of its previous cursor awareness.

Here is another example that got broken with the new strategy that has been bugging me daily over the years:

public class Demo2 {
	public static void main(String[] args) {
		Test test;
		if (true) {
			test.|cursor|.test;
		}
	}

	public static class Test {
		public Test test;
	}
}

Triggering autocomplete at the given cursor location by typing "tes" and then confirming with enter leads to the following result:

public class Demo2 {
	public static void main(String[] args) {
		Test test;
		if (true) {
			test.tetest;
		}
	}

	public static class Test {
		public Test test;
	}
}

The completion engine correctly triggers a CompletionOnQualifiedNameReference, however sometime (during Parsing?) the sourcePositions that the completion works with gets filled up right to the end of the expression, i.e. in the above example there are 3 source positions with the cursor being placed on the second one, whereas the old strategy only had 2 source positions (ending at the cursor?). The completion handler then fails here:

In this case the cursor is located on the second qualified name at sourcePosition[1], so I would expect that one to be the source boundary.

Not sure what the correct way forward is, be it changing the overall strategy or bringing back cursor awareness to the new strategy, but I would love for the autocomplete to be an enhancer again, not a continuous struggle during development.

@angelickite I'm amazed by the depth of your analysis :)

While currently I can't make this issue my main focus, I'd be more than happy to help you getting even closer to the root of the matter.

Some general links you may or may not have seen yet:

• In case you want to continue debugging on current HEAD instead of using smth "around eclipse 2021-06":
  • https://github.com/eclipse-jdt/eclipse.jdt.core/wiki/Code-Setup-Using-Oomph
• In case you want to work from a unit test instead of firing up a runtime workbench, you may take inspiration from existing tests like:
  • https://github.com/eclipse-jdt/eclipse.jdt.core/blob/master/org.eclipse.jdt.core.tests.model/src/org/eclipse/jdt/core/tests/model/CompletionTests3.java
    • If you look at the last test in the class, you'll see that when using text blocks, those tests are quite friendly to the eye :)
    • these exercise all of CompletionEngine, but do not perform the actual text modifications
  • https://github.com/eclipse-jdt/eclipse.jdt.core/blob/master/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/parser/CompletionParserTest18.java (and friends)
    • Tests in this group also check the expectedReplacedSource
• For general information on (the complexity of) code completion:
  • https://github.com/eclipse-jdt/eclipse.jdt.core/wiki/ECJ-Completion

In this case the cursor is located on the second qualified name at sourcePosition[1], so I would expect that one to be the source boundary.

Here's a concrete thing you could try:

• have a look at CompletionEngine.actualCompletionPosition
  • quickly check if this is the same as this.parser.scanner.cursorLocation
• then identify the name segment that contains this position
  • have you seen how the long position is split into start and end locations (int)?
    See, e.g., invocations of setSourceAndTokenRange().
• after code modification run "all" tests :)

Thank you for your pointers.

I gave the whole contribution thing a quick shot but quickly came up against walls that I am currently not willing to put up with. I know eclipse project is at a high level of complexity, however from my point of view enabling contributing is the number one priority for a project of its kind, so anything beyond a "git clone, checkout, run tests, build" (each a single action) is just very off putting. Please don't take the nagging in a bad taste, just a personal pet peeve I needed to get out.

Here is an attempt of mine in case someone else is able to pick it up from here:

eclipse.jdt.core/org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/complete/CompletionParser.java

Lines 5873 to 5875 in a62c379

	if (this.assistNode == null && this.lParenPos > this.cursorLocation && nameStart <= this.cursorLocation + 1) {
	boolean nextIsCast = this.expressionPtr > -1 && this.expressionStack[this.expressionPtr] instanceof CastExpression;
	m = new CompletionOnMessageSendName(null, 0, 0, nextIsCast); // positions will be set in consumeMethodInvocationName(), if that's who called us

		CompletionOnMessageSendName completion = new CompletionOnMessageSendName(null, 0, 0, nextIsCast); // positions will be set in consumeMethodInvocationName(), if that's who called us
		completion.cursorIsToTheLeftOfTheLParen = true;
		m = completion;

eclipse.jdt.core/org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/CompletionEngine.java

Lines 3397 to 3398 in a62c379

	if (messageSend.statementEnd > messageSend.sourceStart)
	setSourceRange(messageSend.sourceStart, messageSend.statementEnd);

			if (messageSend.cursorIsToTheLeftOfTheLParen) {
				// take into consideration the cursor location, i.e. it being to the left of lParen. (GH-2620 @https://github.com/eclipse-jdt/eclipse.jdt.core/issues/2620)
				setSourceAndTokenRange(messageSend.sourceStart, messageSend.sourceEnd);
			} else {
				setTokenRange(messageSend.sourceStart, messageSend.sourceEnd);
				if (messageSend.statementEnd > messageSend.sourceStart)
					setSourceRange(messageSend.sourceStart, messageSend.statementEnd);
			}

eclipse.jdt.core/org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/CompletionEngine.java

Line 3714 in a62c379

long completionPosition = sourcePositions[sourcePositions.length - 1];

		// take into consideration the cursor location. (GH-2620 @https://github.com/eclipse-jdt/eclipse.jdt.core/issues/2620)
		// void x(LinkedListNode n) {
		//   LinkedListNode other = n.ch|cursor|.child.child.parent;
		//   // ...
		// }
		// in the above example, bestPosition will match against the cursor with "ch", i.e. sourcePositions[1].
		// (sourcePositions[0] points to 'n', sourcePositions[4] to 'parent')
		long bestPosition;
		{
			bestPosition = -1L;
			for (int i = 0; i < sourcePositions.length; i++) {
				long p = sourcePositions[i];
				int start = (int) (p >>> 32);
				int end = (int) (p);
				if (start >= this.actualCompletionPosition && end <= this.actualCompletionPosition) {
					bestPosition = p;
					// we take the matching position that is the "farthest to the right".
					// whether that is correct in all future cases remains to be seen.
					// for the time of this writing the last match suffices.
					// (if for example the first match is desired, we could "break;" here)
				}
			}
		}
		if (bestPosition == -1L) {
			// fallback to the previous behaviour, just in case.
			bestPosition = sourcePositions[sourcePositions.length - 1];
		}
		long completionPosition = bestPosition;

I know eclipse project is at a high level of complexity, however from my point of view enabling contributing is the number one priority for a project of its kind, so anything beyond a "git clone, checkout, run tests, build" (each a single action) is just very off putting.

Please don't tell this to people like Ed Merks, who have been working very, very hard to make workspace setup as simple as possible (but not simpler than that ;-P ).

Have you tried the eclipse installer? It performs all necessary steps with just a few clicks: select your eclipse version, select the eclipse projects you want to work on, select an installation folder, and swooop, after a coffee break your shiny new workspace should be waiting for you :)

And if something goes wrong during setup, there are friendly folks who will be happy to accept your report (knowing that your reports will show enough details work investigation).

I have observed that in some cases the sourcePositions of a completion behave oddly:

public class Demo2 {
	public static void main(String[] args) {
		Test test;
		test.|.test; // (1)
		if (true) {
			test.|.test; // (2)
		}
	}
	public static class Test {
		public Test test;
	}
}

With the cursor at '|' the parser packs the sourcePosition at the cursor location such that start > end, e.g. if start is 89, then end is 88. Seems to happen only specifically in a dot-cursor-dot situation.

Is this an known/accepted oddity of the parser or a new issue by itself?

I have encountered this behaviour during my work on this issue. For now I will proceed by locally swapping start and end in such an edge-case as to not blow up this whole issue into a full rework of the underlying system.