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

Syntax error on token "RestrictedIdentifierWhen" #456

Closed
JanMosigItemis opened this issue Oct 7, 2022 · 21 comments · Fixed by #1436 · May be fixed by #1428
Closed

Syntax error on token "RestrictedIdentifierWhen" #456

JanMosigItemis opened this issue Oct 7, 2022 · 21 comments · Fixed by #1436 · May be fixed by #1428
Assignees
Labels
bug Something isn't working regression Something was broken by a previous change
Milestone

Comments

@JanMosigItemis
Copy link

JanMosigItemis commented Oct 7, 2022

Hi there,

I am working on projects that use the new java.lang.foreign Preview API. However the other day I encountered an issue wrt. Mockito's when method. One of my tests stopped compiling, when I migrated the project from JDK17 to JDK19.

Although static imports are there and other code in the same java file works, one call is marked red in the UI and the error tool tip says "Syntax error on token "RestrictedIdentifierWhen", e. g.:

   // Works
   private void addKnownSymbol(String symbolName) {
        var seg = allocateNative(1, MemorySession.global());
        when(libMock.lookup(symbolName)).thenReturn(Optional.of(seg));
    }

    // Error
    private void removeSymbol(String symbolName) {
        when(libMock.lookup(symbolName)).thenReturn(Optional.empty());
    }

The project compiles just fine with maven on the command line.

To make reproducing the problem easier, I provided a repo with a demo project: https://github.com/JanMosigItemis/eclipse.jdk19.when.bug

Not sure if this is a JDT Core issue but I figured, I should give it a try. Seems like some collision on reserved symbols maybe in conjunction with --enable-native-access=ALL-UNNAMED --enable-preview.

Any hints would be appreciated.

System:

  • Eclipse 4.25 with Java19 JDT Patch
  • Running with external JDK17 (17.0.4.1)
  • Windows 10.0.19043.2075
  • Project is configured to use JDK19 (2022-09-20)
@stephan-herrmann
Copy link
Contributor

This looks like a duplicate of #441. Are you in the position to re-test with a recent integration build?

@nlisker
Copy link

nlisker commented Oct 22, 2022

I had the same error described in #441 (a method named when causes "Syntax error on token "RestrictedIdentifierWhen", Identifier expected"). Tried with Eclipse 4.26.0 Integration Build: I20221022-0140 and the problem is resolved. A clean and rebuild is needed. I also checked that switch expression accepts the when guard (case Client c when isOn() -> ...) and I get no compilation errors.

Didn't try to run anything yet, but compilation works at least.

@stephan-herrmann
Copy link
Contributor

Thanks, @nlisker , for reporting success.

@JanMosigItemis could you confirm that current integration builds fix the issue also for you? FYI, you'll find those builds of Eclipse SDK on https://download.eclipse.org/eclipse/downloads/index.html

@JanMosigItemis
Copy link
Author

Thx for working on this @nlisker @stephan-herrmann, I'll try to confirm things on friday if you don't mind me being a bit late.

@JanMosigItemis
Copy link
Author

@stephan-herrmann I can only confirm a partial win here.

When playing around with my example project, I found:

  • Mockito.when now works.
  • Using the "Convert to static import" quickfix does not work, i. e. after the quickfix did his thing, the "Syntax error on token "RestrictedIdentifierWhen" error happens again.

Tested with 2022-12 | 4.26 | I20221027-1800

@alban-auzeill
Copy link

Hi @stephan-herrmann, I confirm this is still an issue part of the previous release R4_27.

To reproduce the problem, you can add the following unit tests in:

org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/SwitchPatternTest.java

	public void testDisambiguatedRestrictedIdentifierWhenAsFirstMethodInvokation() {
		runConformTest(
				new String[] {
					"X.java",
					"public class X {\n"
					+ "	public static void main(String argv[]) {\n"
					+ "		when(\"Pass\");\n"
					+ "	}\n"
					+ "	static void when(String arg) {\n"
					+ "		System.out.println(arg);\n"
					+ "	}\n"
					+ "}"
				},
				"Pass");
	}
1. ERROR in X.java (at line 3)
	when(\"Pass\");
	^^^^
Syntax error on token "RestrictedIdentifierWhen", while expected

	public void testDisambiguatedRestrictedIdentifierWhenAsFirstVariableDeclaration() {
		runConformTest(
				new String[] {
					"when.java",
					"public class when {\n"
					+ "	public static void main(String argv[]) {\n"
					+ "		when x = new when();\n"
					+ "		System.out.println(x);\n"
					+ "	}\n"
					+ "	public String toString() {\n"
					+ "		return \"Pass\";\n"
					+ "	}\n"
					+ "}"
				},
				"Pass");
	}
1. ERROR in when.java (at line 3)
	when x = new when();
	^^^^
Syntax error on token \"RestrictedIdentifierWhen\", delete this token

	public void testDisambiguatedRestrictedIdentifierWhenAsTypeInACase() {
		runConformTest(
				new String[] {
					"when.java",
					"public class when {\n"
					+ "	public String toString() {\n"
					+ "		return switch((Object) this) {\n"
					+ "			case when x -> \"Pass\";\n"
					+ "			default -> \"Fail\";\n"
					+ "		};\n"
					+ "	}\n"
					+ "	public static void main(String argv[]) {\n"
					+ "		System.out.println(new when());\n"
					+ "	}\n"
					+ "}"
				},
				"Pass");
	}
1. ERROR in when.java (at line 4)
	case when x -> \"Pass\";
	          ^
Syntax error on token "x", delete this token

	public void testDisambiguatedRestrictedIdentifierWhenAfterAParenthesis() {
		runConformTest(
				new String[] {
					"X.java",
					"public class X {\n"
					+ "	public static void main(String argv[]) {\n"
					+ "		System.out.println( (Boolean) when(true) );\n"
					+ "	}\n"
					+ "	static Object when(Object arg) {\n"
					+ "		return arg;\n"
					+ "	}\n"
					+ "}"
				},
				"true");
	}
1. ERROR in X.java (at line 3)
	System.out.println( (Boolean) when(true) );
	                              ^^^^
Syntax error on token "RestrictedIdentifierWhen", delete this token

	public void testValidCodeWithVeryAmbiguousUsageOfWhen() {
		runConformTest(
				new String[] {
					"when.java",
					"class when {\n"
					+ "  boolean when = true;\n"
					+ "  static boolean when(when arg) {\n"
					+ "    return switch(arg) {\n"
					+ "      case when when when when.when && when.when(null) -> when.when;\n"
					+ "      case null -> true;\n"
					+ "      default -> false;\n"
					+ "    };\n"
					+ "  }\n"
					+ "  public static void main(String[] args) {\n"
					+ "    System.out.println(when(new when()));\n"
					+ "  }\n"
					+ "}"
				},
				"true");
	}
2. ERROR in when.java (at line 5)
	case when when when when.when && when.when(null) -> when.when;
	^^^^
Syntax error on token \"case\", BeginCaseElement expected after this token

I did some investigations and my understanding is:

  • this issue comes from the wrong logic of the PR Improve scanner "when" token disambiguation #441
  • the method mayBeAtGuard returns true if the previous token is TokenNameNotAToken at Scanner.java:5092
  • This problem occurs during the execution of org.eclipse.jdt.internal.compiler.parser.Parser#getMethodBodies(CompilationUnitDeclaration unit)
  • It calls Parser#parse(MethodDeclaration md, CompilationUnitDeclaration unit) which reset the scanner with this.scanner.resetTo(md.bodyStart, md.bodyEnd);
  • So resetLookBack(); is called and the int lookBack[] state of the Scanner contains 2 x TokenNameNotAToken
  • And when the first token of a method body is when, mayBeAtGuard is called and tries to check the previous token, and unfortunately this.lookBack[1] is equals to TokenNameNotAToken instead of TokenNameLBRACE
	Scanner.mayBeAtGuard(int) line: 5089	
	0x00000008001ec840.test(Object) line: not available	
	Scanner.disambiguatesRestrictedIdentifierWithLookAhead(Predicate<Integer>, int, Goal) line: 5458	
	Scanner.disambiguatedRestrictedIdentifierWhen(int) line: 5407	
	Scanner.internalScanIdentifierOrKeyword(int, int, char[]) line: 3923	
	Scanner.scanIdentifierOrKeyword() line: 3268	
	Scanner.getNextToken0() line: 1902	
	Scanner.getNextToken() line: 1419	
	Parser.fetchNextToken() line: 13230	
	Parser.parse() line: 13122	
	Parser.parse(MethodDeclaration, CompilationUnitDeclaration) line: 13508	
	MethodDeclaration.parseStatements(Parser, CompilationUnitDeclaration) line: 251	
	TypeDeclaration.parseMethods(Parser, CompilationUnitDeclaration) line: 1143	
	Parser.getMethodBodies(CompilationUnitDeclaration) line: 12170	
	AbstractRegressionTest$5(Compiler).process(CompilationUnitDeclaration, int) line: 891	

I don't know if removing the case TokenNameNotAToken: in mayBeAtGuard is the best solution (it is suggested by @MaisiKoleni in the comments). It fixes the 3 first tests above but not testDisambiguatedRestrictedIdentifierWhenAfterAParenthesis and not testValidCodeWithVeryAmbiguousUsageOfWhen

@iloveeclipse iloveeclipse added bug Something isn't working regression Something was broken by a previous change labels Apr 24, 2023
@MaisiKoleni
Copy link
Contributor

MaisiKoleni commented Apr 24, 2023

I will have a look at it again as well, I have to admit I had a bad feeling about the not a token token but not enough experience / insight to back it up.
Interesting that it is still not enough in extreme cases, though.

@alban-auzeill
Copy link

Hi @MaisiKoleni, I am speaking on behalf of Sonar's Java analyzer team.
We faced this when keyword issue with org.eclipse.jdt.core version 3.33 and noticed that 3.34 still has this issue.
For us, it will be very important to have this issue fixed in 3.35.

  • Do you already know when the issue should be fixed?
  • Would you recommend us to try to fix the issue ourselves by contributing to org.eclipse.jdt.core?

@MaisiKoleni
Copy link
Contributor

I am not a regular contributor (made about 2 PRs so far), so I cannot say anything about the timeline or releases. I will open a PR thought that fixes the not a token token problem.
But as you wrote, there are still problems left, e.g. when there is a cast before when. Here, I don't know a quick solution.
The tests are great, but since I am not the author, you will probably need to contribute them yourself under the project license and contributor agreement. (Again, I am not an expert, so I'm cautious here.)

@MaisiKoleni
Copy link
Contributor

MaisiKoleni commented Jun 30, 2023

I guess this is important because the pattern matching will exit preview with 21, right? So our fix should be geared towards Java 21 and not Java 20. Sadly, I couldn't find a preview build of the JLS for 21, but the JEP should contain the most important changes as well.

In particular, JEP 441 mentions:

  • Remove parenthesized patterns, since they did not have sufficient value

However, record patterns exit preview with 21 as well, and those require parentheses, too. So in that regard, the logic could stay mostly the same (apart from removing the TokenNameNotAToken). But as your tests showed, the disambiguation needs more work, and the JLS 20 (which should - apart from the parenthesized patterns - match the JLS 21 pattern syntax).
For reference, the link I used: https://docs.oracle.com/javase/specs/jls/se20/preview/specs/patterns-switch-record-patterns-jls.html#jls-14.30

The important bits for us:

CasePattern:

  • Pattern [ Guard ]

Guard:

  • when Expression

Pattern:

  • TypePattern
  • RecordPattern
  • ParenthesizedPattern // will get dropped in 21

TypePattern:

  • LocalVariableDeclaration

RecordPattern:

  • ReferenceType ( [ PatternList ] )

PatternList :

  • Pattern { , Pattern }

ParenthesizedPattern:

  • ( Pattern )

The local variable declaration is restricted as follows:

  • The LocalVariableType in a top-level type pattern denotes a reference type (and furthermore is not var).
  • The VariableDeclaratorList consists of a single VariableDeclarator.
  • The VariableDeclarator has no initializer.
  • The VariableDeclaratorId has no bracket pairs.

Which results in the following grammar:

LocalVariableDeclaration:

  • UnannReferenceType Identifier

As a consequence, looking back from when permits ...

  1. either ) or Identifier
    This is the current logic, but not good enough, as this is true for casts (PotenitallyComplexType) when
    and variable declarations of type when, like when when
  2. In case of 1. being ...
    1. ) then (, ) as well as Identifier is allowed
      • RecordEmpty()
      • Rec1(Rec2(Rec3(Rec4(...))))
      • Record(int x, int y)
    2. Identifier then only UnannReferenceType is allowed, which ends with
      • ] for array types
      • > for types with type arguments (which recursively contain type syntax again)
      • TypeIdentifier for non-array types without any type arguments

We have two look back steps available, so the question now is, if it solves the ambiguity problems:

  • The problem with case when when when is solved now, the second when is no more recognized as restricted identifier when, only the third one is, as the case keyword token does not match anything in 2.ii
  • The problem with the cast is not solved because Identifier ) when could be both a record pattern and when as well as a cast of variable when (or something similar).
    Here, I cannot see a limited amount of look back steps solving disambiguation. )) and () are not valid suffixes of a cast,
    so only Identifier ) is the problem. Valid predecessors are:
    • For patterns:
      1. Identifier as last part of UnannReferenceType
    • For casts:
      1. ( for a cast with simple name
      2. & for type intersections
      3. . for qualified names
      4. ) for a cast with annotated type (@SuppressWarnings("") Object)
      5. Identifier as last part of @TypeName
        This is the conflict we cannot solve: both annotation type and pattern type allow qualified names, which are recursively defined (PackageOrTypeName) and thereby allow an arbitrary number of Identifier and . alternations like a.b.c.d.e.f.g...

It does not look much better in the other direction because (Cast) when (BooleanExpression) and (TypePattern) when (BooleanExpression) are extremely close together and cannot be disambiguated in a predefined number of tokens around the when in question. (For the look ahead, the proof is easy: nesting boolean expressions in parentheses).

So either: we remember that we started a switch case or we extend the RestrictedIdentifierWhenGoal of the used VanguardParser to include what comes after the boolean expression of the when guard. The first seems safer to me and is probably more efficient.

@MaisiKoleni
Copy link
Contributor

I noticed that we often create a fork of the Scanner for the VanguardParser, which might make additional state a bit more complicated, we would probably need to hand it down to the VanguardScanner like parser.scanner.caseStartPosition = this.caseStartPosition; for the case pattern. This seems quite fragile to me, but I guess we only set what we expect to need.

What I will definitely do first now is removing the not-a-token token and enhance the look back usage. This will still have problems with the cast and similar constructs. I think it is most important that the common case with Mockito when at the start of a method body gets solved first. And a problematic cast before that is very unlikely, as the method and the downstream chain return the generic type OngoingStubbing<T> which is unlikely cast (especially without type arguments).

Still, we should find a proper solution to the problem that aligns with the JLS.

@MaisiKoleni
Copy link
Contributor

I think I also found and solved a more general bug related to the look back. There might have been many different edge cases where it didn't work properly (or we just had a lot of luck so far).

@stephan-herrmann
Copy link
Contributor

@srikanth-sankaran are you following this issue?

Mentions of lookBack and VanguardScanner/Parser make me think of your work in this area :)

@srikanth-sankaran
Copy link
Contributor

@MaisiKoleni Where are you with this ? I can take over if you wish. I was the original author of VanguardParser and VanguardScanner although I will state I don't understand why they are getting involved here :)

@srikanth-sankaran
Copy link
Contributor

Open heart surgery in progress here: #1428 - It will take a while for the dust to settle. Too late for M1

@srikanth-sankaran
Copy link
Contributor

@stephan-herrmann - heads up, I will ask for a review after the proposed implementation gets cleaned up.

@srikanth-sankaran
Copy link
Contributor

Open heart surgery in progress here: #1428 - It will take a while for the dust to settle. Too late for M1

A lot less ambitious effort here: #1436

@alban-auzeill Many thanks for those test snippets - I have included them in the PR,

srikanth-sankaran added a commit that referenced this issue Oct 2, 2023
Consult the parser's state to disambiguate identifier "when" from restricted key word "when"

* Fixes #609
* Fixes #456
* Fixes eclipse-jdt/eclipse.jdt#59
@runeflobakk
Copy link

runeflobakk commented Oct 15, 2023

I too encountered this exact problem with static importing Mockito.when when upgrading a project to Java 21. This is the first time I see this problem.

I am trying to figure out how I can get this fix in my Eclipse installation, which currently is latest stable 2023-09 (4.29) (Build id: I20230903-1000), and with the Java 21 patch 1.2.300.v20230920-0334_BETA_JAVA21. Must I upgrade to an integration build of JDT, or will it eventually be part of the Java 21 support patch?

@post-svejk
Copy link

post-svejk commented Oct 17, 2023

Same problem as @runeflobakk here. I can confirm that the integration build seems to work, but is cumbersome to install. Would really appreciate an update to the stable version, 2023-09.

@srikanth-sankaran
Copy link
Contributor

@jarthana @mpalat What is your recommendation for @runeflobakk and @post-svejk - TIA

@runeflobakk
Copy link

I updated Eclipse Java Development Tools using the update site for 4.30 integration builds
https://download.eclipse.org/eclipse/updates/4.30-I-builds/

I am currently at version 3.19.300.v20231024-0830, which fixes this error, and everything else seems to work as it should 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment