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

[Sealed Types] Strange error from ECJ: Syntax error on token "permits", permits expected #2672

Closed
noopur2507 opened this issue Jul 4, 2024 · 12 comments · Fixed by #2675
Closed
Assignees
Milestone

Comments

@noopur2507
Copy link
Member

See test failures in JDT UI in I20240703-1800 in QuickFixTest17 and UtilitiesTest:

https://download.eclipse.org/eclipse/downloads/drops4/I20240703-1800/testresults/html/org.eclipse.jdt.ui.tests_ep433I-unit-mac64-java17_macosx.cocoa.x86_64_17.html

Looks like one additional compile error is being reported now. For example, in QuickFixTest17#testAddSealedAsDirectSuperInterface1:

package test;

public sealed interface IShape permits Circle {\n
}

class Circle {
    
}

was expecting 2 errors but is getting 3 errors now:

Pb(204) Syntax error on token "permits", permits expected[46 ,52]
Pb(1863) Permitted type Circle does not declare test.IShape as direct super interface [54 ,59]
Pb(233) Syntax error on tokens, delete these tokens[62 ,63]
@noopur2507
Copy link
Member Author

Pb(204) Syntax error on token "permits", permits expected[46 ,52]

This is the extra error now. It can be seen by pasting the given code snippet in an old I-build vs the latest I-build.

@srikanth-sankaran srikanth-sankaran self-assigned this Jul 4, 2024
@srikanth-sankaran srikanth-sankaran added this to the 4.33 M1 milestone Jul 4, 2024
@srikanth-sankaran srikanth-sankaran changed the title 7 tests failing in jdt.ui related to sealed classes and permits clause [Sealed Types] Strange error from ECJ: Syntax error on token "permits", permits expected Jul 4, 2024
@noopur2507
Copy link
Member Author

Another test failure is coming from Bug 567770 - [15] ASTResolving#ASTResolving.getPossibleTypeKinds() should handle sealed classes.

package test1;
public sealed interface E permits X {}
public sealed class F permits X {}
}

For the above code, the AST generated by parser is coming as:

package test1;
public sealed interface E {
  permits X;
{
  }
public sealed class F {
    permits X;
{
    }
  }
}

resulting in the test failure.

I see the same additional compile error, "Syntax error on token "permits", permits expected" in the given code snippet also.

@srikanth-sankaran
Copy link
Contributor

Another test failure is coming from Bug 567770 - [15] ASTResolving#ASTResolving.getPossibleTypeKinds() should handle sealed classes.

package test1;
public sealed interface E permits X {}
public sealed class F permits X {}
}

For the above code, the AST generated by parser is coming as:

For completeness can you share the original AST expected ? Thanks

@noopur2507
Copy link
Member Author

noopur2507 commented Jul 4, 2024

Another test failure...
For completeness can you share the original AST expected ? Thanks

I am not sure how to get that.

With the given PR, 6 tests related to \n are fixed but no difference is seen for this UtilitiesTest#testGetPossibleTypeKindsForTypes test failure.

@srikanth-sankaran
Copy link
Contributor

Another test failure...
For completeness can you share the original AST expected ? Thanks

I am not sure how to get that.

With the given PR, 6 tests related to \n are fixed but no difference is seen for this UtilitiesTest#testGetPossibleTypeKindsForTypes test failure.

Meaning the test is continuing to fail - right ? although the additional error goes away ?

@noopur2507
Copy link
Member Author

Yes, with the PR, the test is still failing. The additional error has gone away. The AST being generated is still the same.

srikanth-sankaran added a commit to srikanth-sankaran/eclipse.jdt.core that referenced this issue Jul 4, 2024
@srikanth-sankaran
Copy link
Contributor

Yes, with the PR, the test is still failing. The additional error has gone away. The AST being generated is still the same.

Thanks, alternate fix for the same PR under test that retains all the grammar simplifications but reverses the scanner simplifications made in #2652 -

The scanner simplifications are not incorrect, they simply don't play well when the CU has syntax errors - in terms of recovery and repair.

@noopur2507
Copy link
Member Author

The test is still failing. I see the AST being generated is also the same.

Adding @rgrunber who added this test and related code to see if he has any comments.

@srikanth-sankaran
Copy link
Contributor

The test is still failing. I see the AST being generated is also the same.

Adding @rgrunber who added this test and related code to see if he has any comments.

FTR, @noopur2507 and I discussed this and found out that there is a problem on JDT/UI side also. The offending test is running at JDK16 - Sealed types entered with JDK17.

@noopur2507
Copy link
Member Author

The test is still failing. I see the AST being generated is also the same.
Adding @rgrunber who added this test and related code to see if he has any comments.

FTR, @noopur2507 and I discussed this and found out that there is a problem on JDT/UI side also. The offending test is running at JDK16 - Sealed types entered with JDK17.

I have released the JDT UI change via eclipse-jdt/eclipse.jdt.ui#1489.

@rgrunber
Copy link
Contributor

It makes sense to have compiler compliance at 17 if we're dealing with sealed classes. The feature was in preview for 15 & 16.

@rgrunber
Copy link
Contributor

rgrunber commented Jul 18, 2024

FWIW I ran into similar issues on JDT-LS, where a compiler compliance of 16 seemed to be enough to use sealed classes. Bumping to 17 fixed the issue. The only slightly annoying part is the (nicer) error message that informs about the lack of support for sealed classes seems to have changed.

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