Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

completion returns no results with pattern matching #2107

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

snjeza
Copy link
Contributor

@snjeza snjeza commented Mar 6, 2024

What it does

Fixes #2106, https://bugs.eclipse.org/bugs/show_bug.cgi?id=568934#c8 and https://bugs.eclipse.org/bugs/show_bug.cgi?id=568934#c6

How to test

try CA at |

import java.util.ArrayList;
import java.util.List;

public class SimpleJavaClass {
  public static void main(String[] args) {
    Object unknown = new ArrayList<>();

    if (unknown instanceof List things) {
      things.is| // try CA here
      Object nothing = null;
      things.is|
    }
  }
}

Without the patch:
patternwithout

With the patch:
pattern

I will add a test.

@snjeza
Copy link
Contributor Author

snjeza commented Mar 11, 2024

@srikanth-sankaran @iloveeclipse I have updated the PR.

the fix proposed in #2107 is suspect. You should never have to fiddle with ExtraCompilerModifiers.AccOutOfFlowScope bits.

I left this part out. The PR doesn't touch the bits field.

See also https://bugs.eclipse.org/bugs/show_bug.cgi?id=568934

It also fixes https://bugs.eclipse.org/bugs/show_bug.cgi?id=568934#c8 and https://bugs.eclipse.org/bugs/show_bug.cgi?id=568934#c6

If you agree, I can add a test.

@srikanth-sankaran
Copy link
Contributor

No, this fix is not correct. It is kludgy and may work but I can't predict what other problems it may cause down the road.

Basically you are pairing a LocalVariableBinding where a TypeBinding would be the appropriate one. That is recipe for CCE. Even if it doesn't trigger CCE in testing, do we want to resort to such hacky fixes is the question.

The real problem as I outlined on the bug earlier is due to wrong parse tree recovery. Can you check why the scanner doesn't declare EOF upon seeing the completion at things.is| and why it returns the token Object at all ?

As you can see the two statements get jammed into one

things.is| Object

so it appears as though you are completing on a nested type, while you are actually completing on a QNR.

@snjeza
Copy link
Contributor Author

snjeza commented Mar 28, 2024

and the parse tree recovered in the non-working case is:

import java.util.ArrayList;
import java.util.List;
public class SimpleJavaClass {
  String things;
  public SimpleJavaClass() {
  }
  public static void main(String[] args) {
    Object unknown;
    {
      if ((unknown instanceof List things))
          <CompleteOnType:things.is> Object;
    }
  }
}

I have tried it and I get:

import java.util.ArrayList;
import java.util.List;
public class SimpleJavaClass {
  public SimpleJavaClass() {
  }
  public static void main(String[] args) {
    Object unknown;
    {
      <CompleteOnType:things.is> Object;
    }
  }
}

There isn't if ((unknown instanceof List things))

Completion is being attempted on a Name in both instances. Why do we think it is being attempted on a Type ? I can help with reviewing an alternate fix. Thanks
The real problem as I outlined on the bug earlier is due to wrong parse tree recovery. Can you check why the scanner doesn't declare EOF upon seeing the completion at things.is| and why it returns the token Object at all ?
The parser returns CompletionOnQualifiedNameReference in line 10 (it works) and CompletionOnQualifiedTypeReference in line 8 (it doesn't work)
Right, that is the bug that needs fixing.

I have checked the following code:

import java.util.ArrayList;
import java.util.List;
public class SimpleJavaClass2 {
  public static void main(String[] args) {
    Object unknown = new ArrayList<>();
    List list = new ArrayList<>();
    if (unknown instanceof List things) {
      list.is|
      Object nothing = null;
      list.is
    }
    
  }
}

the parse tree recovered is

import java.util.ArrayList;
import java.util.List;
public class SimpleJavaClass2 {
  public SimpleJavaClass2() {
  }
  public static void main(String[] args) {
    Object unknown;
    List list;
    {
      <CompleteOnType:list.is> Object;
    }
  }
}

The parser also returns CompletionOnQualifiedTypeReference. It works because the parser finds the list variable.

Based on it and the fix for https://bugs.eclipse.org/bugs/show_bug.cgi?id=287939 I have created a new PR - 970acaf

Basically you are pairing a LocalVariableBinding where a TypeBinding would be the appropriate one. That is recipe for CCE. Even if it doesn't trigger CCE in testing, do we want to resort to such hacky fixes is the question.

This part is removed.

The recovered parse tree with the PR is:

import java.util.ArrayList;
import java.util.List;
public class SimpleJavaClass {
  public SimpleJavaClass() {
  }
  public static void main(String[] args) {
    Object unknown;
    {
      List things;
      if ((unknown instanceof List things))
          <CompleteOnType:things.is> Object;
    }
  }
}

@srikanth-sankaran Could you, please, review.

@snjeza
Copy link
Contributor Author

snjeza commented Mar 28, 2024

The completion with the PR

pattern2

@snjeza
Copy link
Contributor Author

snjeza commented Apr 2, 2024

I have updated the PR. It works similar to CA in

import java.util.ArrayList;
import java.util.List;
public class SimpleJavaClass2 {
  public static void main(String[] args) {
    Object unknown = new ArrayList<>();
    List list = new ArrayList<>();
    if (unknown instanceof List things) {
      list.is|
      Object nothing = null;
      list.is
    }
    
  }
}

I have also added some tests.

@snjeza snjeza changed the title [WIP] completion returns no results with pattern matching completion returns no results with pattern matching Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

completion returns no results with pattern matching
2 participants