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.

Sign up for GitHub

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

Jump to bottom

Invalid syntax in completion test for record pattern destructuring #3589

Open
datho7561 opened this issue Jan 21, 2025 · 8 comments · May be fixed by #3595
Open

Invalid syntax in completion test for record pattern destructuring #3589

datho7561 opened this issue Jan 21, 2025 · 8 comments · May be fixed by #3595

Comments

@datho7561
Copy link
Contributor

datho7561 commented Jan 21, 2025 •
edited
Loading

In a few of the completion tests for record pattern destructuring, a record pattern is placed in the first part of an enhanced for statement, eg:

import java.util.List;
public class Main {
  private static record MyRecord(String a, int b) {}
  public static void main(String... args) {
    List<MyRecord> myList = List.of(new MyRecord("hello", 12), new MyRecord("world", 42));
    for (MyRecord(String j, int k) : myList) {
      System.out.println(j);
    }
  }
}

Here is an example of such a test:

eclipse.jdt.core/org.eclipse.jdt.core.tests.model/src/org/eclipse/jdt/core/tests/model/CompletionTestsForRecordPattern.java

Line 461 in 57ae0e9

public void test012() throws JavaModelException {

After reading the specification, I believe this is invalid syntax. I confirmed the given snippet doesn't compile under ECJ or javac.

I reread 6.3.2.5, the section that seems the most relevant to this test. My understanding is that the scope for for statements only applies to the condition portion of regular for loops, and there is no special syntax for pattern variables in enhanced for loops. eg. the following is allowed:

import java.util.List;
public class Main {
  private static record MyRecord(String a, int b) {}
  public static void main(String... args) {
    List myList = List.of(new MyRecord("hello", 12), new Object());
    for (int i = 0; i < myList.size() && myList.get(i) instanceof MyRecord(String j, int k); i++) {
      System.out.println(j);
    }
  }
}

I think the wording of 6.3.2.5 could have caused some confusion, which lead to this test to be implemented. (After all, I think the "erroneous" syntax looks legitimate at first glance. If it was valid syntax, it would be very helpful when iterating over collections of records). It's also possible the spec changed after the feature was developed and this was a leftover from that.

Either way, I think we should adjust the tests to use valid syntax and test for the pattern variable scoping as described in 6.3.2.5.
@datho7561
Copy link
Contributor Author

@jarthana do you have any more context for this? It seems like you implemented the logic for pattern variable scoping for foreach and these test cases.

@datho7561
Copy link
Contributor Author

I double checked and the scoping for completion for the second snippet doesn't work as expected. j is not only suggested inside the foreach block, but also everywhere in main, even before the for loop.

@srikanth-sankaran
Copy link
Contributor

There was such a construct at preview time that didn't make it to the Final Cut. These tests should be withdrawn too

@srikanth-sankaran
Copy link
Contributor

See #1907

May be some vestiges of this features are still left in tests.

@datho7561
Copy link
Contributor Author

Thanks for the context, @srikanth-sankaran . I can look into removing these tests.

Do you think it makes sense to do a separate issue and PR to fix #3589 (comment) ? eg. currently, j and k will be suggested at the | in the following snippet.

import java.util.List;
public class Main {
  private static record MyRecord(String a, int b) {}
  public static void main(String... args) {
    List myList = List.of(new MyRecord("hello", 12), new Object());
    for (int i = 0; i < myList.size() && myList.get(i) instanceof MyRecord(String j, int k); i++) {
      System.out.println(j);
    }
    |
  }
}

@srikanth-sankaran
Copy link
Contributor

I am fairly certain we have removed all code. Tests can be adjusted right here without the overhead of another ticket IMO

datho7561 added a commit to datho7561/eclipse.jdt.core that referenced this issue Jan 22, 2025
@datho7561
Rewrite for-each pattern variable completion tests to use regular for
d8530f6 
The syntax used in these tests was withdrawn before the feature was
delivered (see eclipse-jdt#1907).
I've altered the tests to use a regular for loop with an `instanceof` in it,
so that the tests still check that the scoping of the pattern variables
is working properly for completion.

Fixes eclipse-jdt#3589

Signed-off-by: David Thompson <[email protected]>
@datho7561 datho7561 linked a pull request Jan 22, 2025 that will close this issue
Open
3 tasks
@datho7561
Copy link
Contributor Author

I can adjust the tests to use normal, non-enhanced for loops with instanceof in them. This will be a pretty quick change, in fact here it is: #3595

However, there is a bug in the completion for pattern variables: if you open completion after the for loop, the pattern variables are still suggested as possible completions. They are also suggested before the for loop. In order to fix this, it would require debugging to figure out why, code changes somewhere (not sure where yet), and some new tests for this.

I can look into handling this in the same PR, but it will require a bit more work to get it done.

@datho7561 datho7561 moved this to Todo in Java Tooling Jan 22, 2025
@datho7561 datho7561 moved this from Todo to In Progress in Java Tooling Jan 22, 2025
datho7561 added a commit to datho7561/eclipse.jdt.core that referenced this issue Jan 22, 2025
@datho7561
Rewrite for-each pattern variable completion tests to use regular for
21e9497 
The syntax used in these tests was withdrawn before the feature was
delivered (see eclipse-jdt#1907).
I've altered the tests to use a regular for loop with an `instanceof` in it,
so that the tests still check that the scoping of the pattern variables
is working properly for completion.

Fixes eclipse-jdt#3589

Signed-off-by: David Thompson <[email protected]>
@datho7561
Copy link
Contributor Author

I filed a separate issue for the second part: #3600

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Rewrite for-each pattern variable completion tests to use regular for datho7561/eclipse.jdt.core
2 participants
@datho7561 @srikanth-sankaran