search

eclipse-jdt / eclipse.jdt.core

Eclipse Public License 2.0
149 stars 118 forks source link

Faulty/Undesired CodeCompletion behaviour #2620

Open angelickite opened 2 weeks ago

angelickite commented 2 weeks ago

Hello. This situation has been driving me (and others) crazy every minute(!) I am coding with eclipse since forever, so I finally took some time out to at least pinpoint the offending plugin and commit. Hope this helps!

This behaviour started back with eclipe 2021-06 while 2021-03 still behaved "well".

Code completion is behaving in an undesired manner in the following situation:

public class Demo {
    public static void main(String[] args) {
        System.out.println("hello");
        if (true) {
            System.out.print|cursor|ln("world");
        }
    }
}

Triggering code completion (ctrl+space) while the cursor is located as indicated gives a list of proposals, so far so good. Now, after selecting the first proposal (in my case the "print" method), the code gets transformed as follows:

public class Demo {
    public static void main(String[] args) {
        System.out.println("hello");
        if (true) {
            System.out.print(false);
        }
    }
}

The expected behaviour would be that the existing parameter "world" remains in place and only the "ln" part of "println" gets removed, i.e. the method call is updated from "println" to "print", like so:

public class Demo {

    public static void main(String[] args) {
        System.out.println("hello");
        if (true) {
            System.out.print("world");
        }
    }
}

Note that I have enclosed the second println statement in an if block (the true doesnt matter). An empty block does not show the bad behaviour. Here is an (incomplete) overview:

public class Test {

  public static void main (String[] args) {
    // good behaviour
    System.out.println("hello");

    { // good behaviour
      System.out.println("world");
    }

    if (true) { // bad behaviour
      System.out.println("world");
    }

    while (true) { // bad behaviour
      System.out.println("world");
      break;
    }

    for (int i = 0; i < 10; i++) { // bad behaviour
      System.out.println("world");
    }

    Runnable a = () -> { // bad behaviour
      System.out.println("world");
    };

    Nested n = new Nested();
    // good behaviour for deeper1, bad behaviour for deeper2
    n.deeper1(1, 2).deeper2(3, 4);
  }

  public static class Nested {
    public Nested deeper;

    public Nested deeper1 (int a, int b) {
      return deeper;
    }

    public Nested deeper2 (int a, int b) {
      return deeper;
    }
  }

}

My content assist settings: settings

I was able to pinpoint the issue to the jdt.core, specifically the following commit: e3517ddc41f3c9536a29ef9be4e7dd3104993ab2

To arrive at this commit I used a git bisect:

Due to the complexity of that specific commit, me looking into the eclipse source code for the first time, and my lack of time, I was hoping someone more experienced could pick it up from here.

There are some other completion issues I have been encountering which are possibly related to this, so looking into it is hopefully time well spent.

Additionally possibly related:

1770

1016

Thank you!

srikanth-sankaran commented 2 weeks ago

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

stephan-herrmann commented 2 weeks ago

@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.

angelickite commented 2 weeks ago

@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.

stephan-herrmann commented 2 weeks ago

Version: 2024-06 (4.32.0)

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

first bad: https://github.com/eclipse-jdt/eclipse.jdt.core/commit/e3517ddc41f3c9536a29ef9be4e7dd3104993ab2

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.

angelickite commented 2 weeks ago

I did some more digging.

Here is where the completion gets recognized in the "bad" case: https://github.com/eclipse-jdt/eclipse.jdt.core/blob/a62c3798eeea82e0c59207e913c986bef66ed8bf/org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/complete/CompletionParser.java#L5873-L5875

Here is why the source gets overwriten: https://github.com/eclipse-jdt/eclipse.jdt.core/blob/a62c3798eeea82e0c59207e913c986bef66ed8bf/org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/CompletionEngine.java#L3397-L3398

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!

angelickite commented 2 weeks ago

It looks like the new strategy introduced with e3517ddc41f3c9536a29ef9be4e7dd3104993ab2 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:

https://github.com/eclipse-jdt/eclipse.jdt.core/blob/a62c3798eeea82e0c59207e913c986bef66ed8bf/org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/CompletionEngine.java#L3714

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.

stephan-herrmann commented 2 weeks ago

@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:

stephan-herrmann commented 2 weeks ago

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:

angelickite commented 2 weeks ago

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:

https://github.com/eclipse-jdt/eclipse.jdt.core/blob/a62c3798eeea82e0c59207e913c986bef66ed8bf/org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/complete/CompletionParser.java#L5873-L5875

        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;

https://github.com/eclipse-jdt/eclipse.jdt.core/blob/a62c3798eeea82e0c59207e913c986bef66ed8bf/org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/CompletionEngine.java#L3397-L3398

            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);
            }

https://github.com/eclipse-jdt/eclipse.jdt.core/blob/a62c3798eeea82e0c59207e913c986bef66ed8bf/org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/CompletionEngine.java#L3714

        // 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;