Try to fix yield with expression containing dots by koppor · Pull Request #3361 · javaparser/javaparser

koppor · 2021-09-02T08:58:01Z

Details at #3364.

This PR adds a test case for the case that after yield a method is called. Example: yield Collections.emptyList().

        List<Integer> result = switch (s) {
            case "Foo":
                yield List.of(1);
            case "Bar":
                yield List.of(2);
            default:
                System.out.println("Neither Foo nor Bar, hmmm...");
                yield Collections.emptyList();
        };

Error:

org.opentest4j.AssertionFailedError: [(line 3,col 15) Parse error. Found ".", expected one of  "," ";" "=" "@" "["

Expected: No error

koppor · 2021-09-02T16:04:58Z

On the CI, I get following error:

ParseErrorRecoveryTest.labeledStatementSemicolonRecovery:62 » IllegalState ???...

In IntelliJ, this test is green. Locally, with ./mvnw, I also get this error. Not sure, how to proceed.

Longer error:

>
[ERROR]   JavaRecordNameTest.recordIsAValidVariableName:32 [(line 1,col 8) 'record' cannot be used as an identifier as it is a keyword.]
[ERROR]   TransformationsTest.exampleParam5:169->AbstractLexicalPreservingTest.assertTransformed:79  -- failed due to line separator differ

ences -- Expected: \n, but actual:  (system eol: \r\n) ==> expected: <// This is my class, with my comment
class A {
    int foo(int p1, char p2) {
        return p1;
    }
}> but was: <// This is my class, with my comment
class A {
    int foo(int p1, char p2) {
        return p1;
    }
}>
[ERROR] Errors:
[ERROR]   ParseErrorRecoveryTest.labeledStatementSemicolonRecovery:62 ; IllegalState ???...

jlerbsc · 2021-09-02T18:59:44Z

Hi @koppor you have to use TestUtils.assertEqualsStringIgnoringEol to compare 2 string without the test being sensitive at the end of the line.

koppor · 2021-09-02T21:25:07Z

I added the string comparison.

The other error still is there - maybe my grammar "fix" messed up some special case. Currently, I have no clue how to continue here.

 [INFO] Running com.github.javaparser.ParseErrorRecoveryTest
Error:  Tests run: 7, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.05 s <<< FAILURE! - in com.github.javaparser.ParseErrorRecoveryTest
Error:  com.github.javaparser.ParseErrorRecoveryTest.testBodyRecoverLevel  Time elapsed: 0.003 s
Error:  com.github.javaparser.ParseErrorRecoveryTest.labeledStatementSemicolonRecovery  Time elapsed: 0.006 s  <<< ERROR!
java.lang.IllegalStateException: ???; is not LabeledStmt, it is UnparsableStmt
	at com.github.javaparser.ParseErrorRecoveryTest.labeledStatementSemicolonRecovery(ParseErrorRecoveryTest.java:62)

jlerbsc · 2021-09-03T06:16:34Z


        // Note that we explicitly care about line endings when handling lexical preservation.
-        assertEqualsString(expectedCode, actualCode);
+        TestUtils.assertEqualsStringIgnoringEol(expectedCode, actualCode);


You are changing behavior that is intentional!

Note that we explicitly care about line endings when handling lexical preservation.

Then I did not get your comment #3361 (comment) right. Reverted bbfc0d7.

Apologies, without context (note the comment on line 79) it wasn't obvious from the quoted errors that this was a place where line endings were important.

koppor · 2021-09-04T07:00:48Z

I updated the PR to contain two commits:

Add test case for valid Java Code: 4a0123f (#3361)
Naive try to fix the issue: 693079d (#3361)

jlerbsc · 2022-03-18T17:16:26Z

@MysterAitch The PR goal was to fix a grammar issue. Can you approve the @koppor 's proposal?

matozoid · 2022-03-19T18:36:47Z

+        MethodCallExpr methodCallExpr = (MethodCallExpr) yieldStmt.getExpression();
+        IntegerLiteralExpr firstArgument = (IntegerLiteralExpr) methodCallExpr.getArguments().get(0);
+        assertEquals(1, firstArgument.asNumber());
+    }


Hmmm, these ought to go to YieldStmtTest

matozoid · 2022-03-19T18:41:31Z

I find it weird that the BlockStatement grammar has to be changed for this - it has been fine for years. Can you explain the fix, @koppor ?

MysterAitch · 2022-04-18T22:01:40Z

I find it weird that the BlockStatement grammar has to be changed for this - it has been fine for years. Can you explain the fix, @koppor ?

Within a SwitchEntry, the element to the right of a colon is one-or-more BlockStatement.

I am still investigating, but it appears that our definition of a BlockStatement isn't wholly correct per #3364 whereby the Yield.of(1) is first being interpreted/parsed as a VariableDeclaration before being considered as a Statement -- hence (I presume) the change in order/precedence suggested within @koppor 's PR.

Consider the following grammar entries from the JLS:

SwitchBlockStatementGroup:
    SwitchLabel : {SwitchLabel :} BlockStatements

BlockStatements:
    BlockStatement {BlockStatement}
BlockStatement:
    LocalVariableDeclarationStatement
    ClassDeclaration
    Statement

Statement:
    StatementWithoutTrailingSubstatement
    LabeledStatement
    IfThenStatement
    IfThenElseStatement
    WhileStatement
    ForStatement
StatementNoShortIf:
    StatementWithoutTrailingSubstatement
    LabeledStatementNoShortIf
    IfThenElseStatementNoShortIf
    WhileStatementNoShortIf
    ForStatementNoShortIf
StatementWithoutTrailingSubstatement:
    Block
    EmptyStatement
    ExpressionStatement
    AssertStatement
    SwitchStatement
    DoStatement
    BreakStatement
    ContinueStatement
    ReturnStatement
    SynchronizedStatement
    ThrowStatement
    TryStatement
    YieldStatement

MysterAitch · 2022-04-18T23:01:50Z

The PR appears to fix the yield problem, but breaks the error recovery code (multiple tests no longer pass).

@matozoid -- Perhaps you are able to advise on this, please?

matozoid · 2022-05-11T15:55:09Z

What's the error recovery problem you see, @MysterAitch ?

koppor · 2022-06-06T16:45:30Z

I find it weird that the BlockStatement grammar has to be changed for this - it has been fine for years. Can you explain the fix, @koppor ?

I was trying to use the LOOKAHEAD feature to ensure that yield is properly captured. However, it does not work.

Maybe, the added test cases are useful for a "proper" fix?

jlerbsc · 2022-10-27T05:52:37Z

This PR is closed because JavaParser 3.24.7 correctly parse yield statement.

koppor mentioned this pull request Sep 2, 2021

Fix yield koppor/javaparser#2

Open

koppor changed the title ~~Add test for yield with expression containing dots~~ Try to fix yield with expression containing dots Sep 2, 2021

koppor mentioned this pull request Sep 2, 2021

"yield List.of(1);" should be valid in Java 16 #3364

Closed

koppor mentioned this pull request Sep 2, 2021

De-localize preferences JabRef/jabref#8050

Closed

5 tasks

jlerbsc reviewed Sep 3, 2021

View reviewed changes

koppor added 2 commits September 4, 2021 08:59

Add test cases for valid Java code

4a0123f

Try to fix parsing of yield List.of(1)

693079d

koppor force-pushed the yield-issue branch from bbfc0d7 to 693079d Compare September 4, 2021 06:59

koppor marked this pull request as draft September 4, 2021 07:01

Merge branch 'master' into yield-issue

bd38fb4

matozoid reviewed Mar 19, 2022

View reviewed changes

Merge branch 'master' into yield-issue

500916e

Merge branch 'master' into yield-issue

7486fde

jlerbsc closed this Oct 27, 2022

Uh oh!

Conversation

koppor commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

koppor commented Sep 2, 2021

Uh oh!

jlerbsc commented Sep 2, 2021

Uh oh!

koppor commented Sep 2, 2021

Uh oh!

jlerbsc Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

koppor Sep 4, 2021

Choose a reason for hiding this comment

Uh oh!

MysterAitch Sep 16, 2021

Choose a reason for hiding this comment

Uh oh!

koppor commented Sep 4, 2021

Uh oh!

jlerbsc commented Mar 18, 2022

Uh oh!

matozoid Mar 19, 2022

Choose a reason for hiding this comment

Uh oh!

matozoid commented Mar 19, 2022

Uh oh!

MysterAitch commented Apr 18, 2022

Uh oh!

MysterAitch commented Apr 18, 2022

Uh oh!

matozoid commented May 11, 2022

Uh oh!

koppor commented Jun 6, 2022

Uh oh!

jlerbsc commented Oct 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

koppor commented Sep 2, 2021 •

edited

Loading