Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 573702

Summary: [content assist] Wrong completions for parameter on a method chain
Product: [Eclipse Project] JDT Reporter: Gayan Perera <gayanper>
Component: CoreAssignee: Stephan Herrmann <stephan.herrmann>
Status: VERIFIED FIXED QA Contact: Jay Arthanareeswaran <jarthana>
Severity: normal    
Priority: P3 CC: loskutov, mauromol, stephan.herrmann
Version: 4.20   
Target Milestone: 4.20 RC1   
Hardware: All   
OS: All   
See Also: https://bugs.eclipse.org/bugs/show_bug.cgi?id=573632
https://bugs.eclipse.org/bugs/show_bug.cgi?id=539685
https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/180899
https://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=e211a44735f4142c81518de31a4da690328c1c35
Whiteboard:

Description Gayan Perera CLA 2021-05-21 12:18:56 EDT
Take the following code

package workbench;

import java.util.Collection;
import java.util.Map;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import javafx.beans.InvalidationListener;
import javafx.beans.Observable;
import javafx.beans.property.ObjectProperty;
import javafx.beans.property.SimpleObjectProperty;

public class App
{
  private static final Logger LOGGER = LoggerFactory.getLogger(App.class);

  private static ObjectProperty<Map<String, String>> dataProperty = new SimpleObjectProperty<Map<String, String>>();

  public static void boo(Map<String, String> data)
  {
    LOGGER.debug("PopOver direct buffer delayed patch installation started");
    App.getDataProperty().addListener(new SingleFireInvalidationListener(data.values()));
    LOGGER.debug("PopOver direct buffer delayed patch installation done");

  }

  public static ObjectProperty<Map<String, String>> getDataProperty()
  {
    return dataProperty;
  }

  private static class SingleFireInvalidationListener implements InvalidationListener
  {
    public SingleFireInvalidationListener(Collection<String> list)
    {
    }

    public void invalidated(Observable observable)
    {
    }
  }
}

Try to invoke completion behind "SingleFireInvalidationListener(data" which will give you wrong completions which are method completions for the Observable class.
Comment 1 Andrey Loskutov CLA 2021-05-21 12:25:37 EDT
Gayan, probably this is related to bug 573632.
Comment 2 Gayan Perera CLA 2021-05-21 12:53:23 EDT
No @Andrey even with the patch for bug 573632 this issue can still be reproduced.
Comment 3 Gayan Perera CLA 2021-05-21 13:02:24 EDT
Seems something really wrong in completion parser.

The CompletionOnMessageSendName looks like this

<CompleteOnMessageSendName:CompletioError.getDataProperty().addListener()>

And the recieverType is resolved to ObjectProperty which is the reason i get addListener methods as completion.
Comment 4 Andrey Loskutov CLA 2021-05-21 13:14:13 EDT
(In reply to Gayan Perera from comment #2)
> No @Andrey even with the patch for bug 573632 this issue can still be
> reproduced.

The patxh is probably not complete yet, but common root cause could be bug 539685 changes. Could you check if the problem disappears if you undo bug 539685 changes?
Comment 5 Gayan Perera CLA 2021-05-21 14:04:54 EDT
Seems like caused by Bug 539685.

Without this fix the completion parser doesn't parse beyond the cursor location. So we endup with a expression like

    new SingleFireInvalidationListener(<CompleteOnName:dat>);


But with the fix it looks like this

    <CompleteOnMessageSendName:CompletioError.getDataProperty().addListener()>;

This happens because it start consuming identifiers beyond the completion point and at the end the parser thinks the first node is the node which belongs the cursor position since the start is always less than cursor position.


After Bug 539685 fix i don't get completion at all until the last fix on completion parser.
Comment 6 Gayan Perera CLA 2021-05-21 14:05:25 EDT
(In reply to Andrey Loskutov from comment #4)
> (In reply to Gayan Perera from comment #2)
> > No @Andrey even with the patch for bug 573632 this issue can still be
> > reproduced.
> 
> The patxh is probably not complete yet, but common root cause could be bug
> 539685 changes. Could you check if the problem disappears if you undo bug
> 539685 changes?

Yes
Comment 7 Gayan Perera CLA 2021-05-22 06:12:00 EDT
I looked at this and found the following

Previously we only scan upto the cusorLocation so we find the "data" as a identifier on the constructor call. But now since we scan the whole method we endup creating expressions for data.values() as a argument for the constructor call. Ideally we should try to see if we are creating expressions which should be part of the completion position and make it a CompletionOn* node instead of a normal AST.
Comment 8 Stephan Herrmann CLA 2021-05-22 06:29:49 EDT
(In reply to Gayan Perera from comment #7)
> I looked at this and found the following
> 
> Previously we only scan upto the cusorLocation so we find the "data" as a
> identifier on the constructor call. But now since we scan the whole method
> we endup creating expressions for data.values() as a argument for the
> constructor call. Ideally we should try to see if we are creating
> expressions which should be part of the completion position and make it a
> CompletionOn* node instead of a normal AST.

I have a patch coming up in just a few minutes, which extends CompletionParser.getUnspecifiedReference() to do exactly what you describe: create CompletionOn*NameReference nodes, when we cross cursorLocation. 

(Creating CompletionOnMessageSendName was simply caused by this.assistNode == null, where it should have been set already).
Comment 9 Eclipse Genie CLA 2021-05-22 06:39:00 EDT
New Gerrit change created: https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/180899
Comment 10 Stephan Herrmann CLA 2021-05-22 06:41:40 EDT
(In reply to Eclipse Genie from comment #9)
> New Gerrit change created:
> https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/180899

Here you go.

@Gayan, your feedback on the patch would be appreciated.

@Jay, may I burden you (again) with driving the approval process for RC1?
Comment 11 Stephan Herrmann CLA 2021-05-22 06:43:23 EDT
I should note that the gerrit was intentionally created on-top of https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/180822/5 
For the unlikely case that only this one gets approval, not the other one, the gerrit would need to be rebased on master.
Comment 13 Jay Arthanareeswaran CLA 2021-05-26 00:43:50 EDT
Verified for 4.20 RC1 using build I20210525-1800