Jep 445 grammar #1521

mickaelistria · 2023-10-27T12:34:38Z

What it does

How to test

Author checklist

I have thoroughly tested my changes
The change is following the coding conventions
I have signed the Eclipse Contributor Agreement (ECA)

mpalat

Thanks @mickaelistria - please find the code review comments attached.

org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/parser/Parser.java

mpalat · 2023-10-31T03:12:05Z

org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/parser/Parser.java

+			} else if (astNode instanceof TypeDeclaration type) {
+				if (!Arrays.equals(this.compilationUnit.getMainTypeName(), type.name)) {
+					types.addFirst(type);
+				} else {


Looks good as per spec

org.eclipse.jdt.core.compiler.batch/grammar/java.g

mpalat · 2023-10-31T03:29:19Z

org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/UnnamedClass.java

+	public UnnamedClass(CompilationResult result) {
+		super(result);
+		this.modifiers = ClassFileConstants.AccDefault | ClassFileConstants.AccFinal;
+		this.name = "<unnamed class>".toCharArray(); //$NON-NLS-1$


Instead of embedding the string in the code, it may be better(?) to put it in a static variable.

With this change, it seems running ecj as a jar on an unnamed class in a source file called MySourceFile.java will produce a file called <unnamed class>.class. However, javac will produce MySourceFile.class. According to the specification, it's okay that the class file is not named the same thing as the source file. However, you are also allowed to have multiple unnamed classes, kind of like how you can have multiple classes with public static void main(String... args). For instance, I can build a maven project from the command line with multiple unnamed classes. Currently, the output file for these classes would be the same file, which likely will cause issues when doing a full project build.

The easiest way to resolve this (IMO) would be to mimic javac's behaviour. Maybe this should be handled as a part of the code generation change, though.

At the moment, the codegen is not really supported in this PR. It was part of #1487 but Manoj asked for those bits to be removed. In #1487, there are tests checking that the right .class is generated

org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/parser/Parser.java

datho7561 · 2023-11-09T16:21:25Z

org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/parser/Parser.java

+				fields.addFirst(field);
+			}
+		}
+		if ((!methods.isEmpty() || !fields.isEmpty()) && problemReporter().validateJavaFeatureSupport(JavaFeature.UNNAMMED_CLASSES_AND_INSTANCE_MAIN_METHODS, 0, 0)) {


I think this should be !problemReporter().validateJavaFeatureSupport(..., since it seems like validateJavaFeatureSupport returns true when a diagnostic is reported and false otherwise.

mpalat requested a review from srikanth-sankaran now

@srikanth-sankaran - adding you as a reviewer once you are done with String templates. thanks

mpalat · 2023-11-13T12:49:36Z

...re.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/parser/ComplianceDiagnoseTest.java

+			1. ERROR in X.java (at line 1)
+				void ___eval() {
+				^
+			The preview feature Unnamed Classes and Instance Main Methods is only available with source level 21 and above


It may not make sense to throw this error when compiling at a level below 21. Please handle this to throw an appropriate error

It can make sense for the scenario of a user who's using Unnamed Classes and has not configured the project to Java 21 + early-access features. This problem is a way to remind them that maybe what they want is newer Java.

@mickaelistria let's say I am compiling at 17 and if I get this error then does it make sense to throw an error with a java language feature much later? Please check

Only one error is reported if you try to create an unnamed class when the project is set to Java 17 (or any Java less than 21). A quick fix is presented which can update the Java version of the project and enable preview features.

mpalat · 2023-11-13T12:50:31Z

...jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/parser/DietRecoveryTest.java

+  }
+}
+""";
+


Are these changes just changing to text block or anything more?

The source code that's being tested has not changed, but the expected output has changed. Since the file contains methods that are not in any class body, the parser parses the code as an unnamed class, and the printed parsed class reflects this change

mpalat · 2023-11-13T12:52:18Z

...re.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/parser/JEP445UnnamedClassTest.java

+import org.eclipse.jdt.internal.compiler.problem.ProblemReporter;
+import org.junit.Test;
+
+public class JEP445UnnamedClassTest  extends AbstractCompilerTest {


nit: Please rename it withput JEP445 - you can mention it in the comments. You might have seen JEPxx earlier, but nowadays we follow this feature based naming since the JEPS change across different preview versions

I've addressed this

mpalat · 2023-11-13T12:55:35Z

...re.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/parser/JEP445UnnamedClassTest.java

+	public static boolean optimizeStringLiterals = false;
+	public static long sourceLevel = ClassFileConstants.JDK21; //$NON-NLS-1$
+
+	public JEP445UnnamedClassTest(String testName){


Please add
public static Class<?> testClass() {
return UnnammedClassTest.class;
}
public static Test suite() {
return buildMinimalComplianceTestSuite(testClass(), F_21);
}

refer other junit tests in 21 - say SwitchPatternTes/RecPatTes

I've addressed this

mpalat

Please have a look at the comments inline

srikanth-sankaran · 2023-11-15T09:14:14Z

Thanks for your patience. I am starting my review of this change set now. I will come back with comments and questions as I chew on this.

akurtakov · 2023-11-15T09:20:27Z

@srikanth-sankaran As per #1517 (comment) - they guys considered the other PR in better shape than this one so it makes sense to review it first.

srikanth-sankaran · 2023-11-15T09:28:53Z

@srikanth-sankaran As per #1517 (comment) - they guys considered the other PR in better shape than this one so it makes sense to review it first.

I'll make a high level pass over both and make a deeper dive into the one considered more ready. Just this morning I was having a conversation with other committers that early and more frequent agile style feedback may work better to avoid rewrites and expensive backtracks.

datho7561 · 2023-11-21T15:32:35Z

FYI, there is a new version of the preview feature for Java 22. Mickael told me about this. It changes the name "unnamed class" to "implicitly declared class", and later down in the proposal it says that you still can't reference these implicitly declared classes from other classes. It seems like implicitly declared classes might support some things that aren't allowed in the current unnamed classes preview feature, such as having static initializer blocks in the implicitly declared class, but I think it's best to see the final spec changes to understand what the difference is.

This PR will aim to support the Java 21 version of the preview feature.

datho7561 · 2023-12-13T19:29:48Z

@srikanth-sankaran @mpalat got the tests passing

datho7561 · 2024-01-09T15:33:57Z

@jarthana when you have time, could you please review this PR?

datho7561 · 2024-01-22T18:39:52Z

@srikanth-sankaran @mpalat @jarthana could you please take a look at this PR when you have a chance? I'll take a look into rebasing it today

srikanth-sankaran · 2024-01-23T12:41:50Z

@srikanth-sankaran @mpalat @jarthana could you please take a look at this PR when you have a chance? I'll take a look into rebasing it today

Sorry, I have been too busy with some rewrite of pattern matching implementation, That is mostly done and I will look into this, I am trying to determine the best order of reviews given there are multiple PRs open

srikanth-sankaran · 2024-01-26T09:04:27Z

With #1517 approved for merge by @jarthana and myself, we can start on this.
@mpalat, next week I can review the grammar changes - for other areas, I would request other reviewers. I have somewhat limited cycles

Updated some tests accordingly: * Some DietRecoveryTests could be entirely removed as the case are now parse-able; some other were totally changes.

Signed-off-by: David Thompson <davthomp@redhat.com>

- sometimes valid Java < 21 code needs to go through these rules - we can manually generate the error if we detect an unnamed class in Java < 21 Signed-off-by: David Thompson <davthomp@redhat.com>

Signed-off-by: David Thompson <davthomp@redhat.com>

This should resolve some issues related to formatting Signed-off-by: David Thompson <davthomp@redhat.com>

Signed-off-by: David Thompson <davthomp@redhat.com>

mpalat · 2024-02-01T03:29:05Z

With #1517 approved for merge by @jarthana and myself, we can start on this. @mpalat, next week I can review the grammar changes - for other areas, I would request other reviewers. I have somewhat limited cycles

@srikanth-sankaran @datho7561 would you want me to review some changes here? I see a merge - so checking is there any other PR that addresses the rest of the phases (post parsing)?

srikanth-sankaran · 2024-02-01T03:43:18Z

With #1517 approved for merge by @jarthana and myself, we can start on this. @mpalat, next week I can review the grammar changes - for other areas, I would request other reviewers. I have somewhat limited cycles

@srikanth-sankaran @datho7561 would you want me to review some changes here? I see a merge - so checking is there any other PR that addresses the rest of the phases (post parsing)?

I am asking someone (you?) to take the lead on driving this review to merge. I can review the grammar/parser changes. Reviews have to be scheduled in other areas.

mpalat · 2024-02-02T06:01:17Z

@datho7561 - am assuming that you would not be continuing on this but on the Java 22 part - #1640 (comment). Can you please create a PR against that issue (or a subissue created under that -say for grammar changes) - thanks.

datho7561 · 2024-02-02T13:55:10Z

@datho7561 - am assuming that you would not be continuing on this but on the Java 22 part

This PR is for Java 21 preview support of unnamed classes, not Java 22 support

mpalat · 2024-02-06T07:17:24Z

@datho7561 - am assuming that you would not be continuing on this but on the Java 22 part

This PR is for Java 21 preview support of unnamed classes, not Java 22 support

I understand that. Let me explain the query more.. Refer the discussions of #1106 - So you are planning to support the Java 21 version (here) and then the Java 22 version (1640) as well rather than choosing the Java 22 over Java 21. Is this understanding correct?

datho7561 · 2024-02-06T13:32:34Z

Is this understanding correct?

Yes, this is correct

srikanth-sankaran · 2024-02-06T14:06:06Z

My recommendation is to leverage this work and bridge to the preview in 22. That would be a better path than releasing outdated preview mode.

datho7561 · 2024-02-06T15:23:44Z

I have to switch to other tasks now, feel free to continue work on this PR and add whatever is needed for Java 22. We've released this as a Java 21 preview feature in JDT-LS as part of our incubator.

Regardless, I still think this is worth adding as a Java 21 preview feature instead of a Java 22 preview feature.

The reason is we cannot claim to support Java 22 until lists of patterns in case statements is merged, since unnamed variables and patterns is a full feature in Java 22. However, lists of patterns in case statements is not yet merged, since Srikanth is working on refactoring the code generation for patterns. I will have to rebase my "lists of patterns in case statements" PR on top of those changes before it's ready for merge, since the changes I made for code generation will conflict with Srikanth's changes.

This PR (unnamed classes) is orthogonal to pattern code generation and unnamed variables and patterns, so I think we can merge this before the pattern code generation refactoring is complete. If this PR is merged first, we should list this as a Java 21 preview feature instead of a Java 22 preview feature, since we won't support Java 22 until the "lists of patterns in case statements" feature is in.

akurtakov · 2024-05-16T06:01:19Z

@mpalat This one should be closed, right?

mpalat · 2024-05-16T11:42:35Z

@mpalat This one should be closed, right?

@akurtakov : yes. Thanks @mickaelistria for closing

mickaelistria mentioned this pull request Oct 27, 2023

[21] Grammar support for Unnammed Classes #1511

Open

mickaelistria force-pushed the jep-445-grammar branch from 8e5e4ac to 5f093e9 Compare October 30, 2023 07:55

mpalat requested changes Oct 31, 2023

View reviewed changes

mickaelistria force-pushed the jep-445-grammar branch 3 times, most recently from 0cdeed9 to cf67147 Compare November 6, 2023 16:06

datho7561 mentioned this pull request Nov 8, 2023

[21] AST To CodeGeneration Part #1512

Open

mickaelistria force-pushed the jep-445-grammar branch from cf67147 to 20d43ed Compare November 8, 2023 15:41

datho7561 reviewed Nov 9, 2023

View reviewed changes

mickaelistria force-pushed the jep-445-grammar branch from 20d43ed to db1446d Compare November 13, 2023 09:21

mpalat requested a review from srikanth-sankaran November 13, 2023 09:33

mpalat reviewed Nov 13, 2023

View reviewed changes

mpalat requested changes Nov 13, 2023

View reviewed changes

mickaelistria force-pushed the jep-445-grammar branch from db1446d to 8643f8d Compare November 13, 2023 14:00

datho7561 force-pushed the jep-445-grammar branch 3 times, most recently from 6cf0445 to 9942a54 Compare December 8, 2023 22:15

datho7561 force-pushed the jep-445-grammar branch 3 times, most recently from 8d6fdac to 1debc42 Compare December 13, 2023 18:25

datho7561 force-pushed the jep-445-grammar branch from f2840be to dfda508 Compare January 24, 2024 22:02

srikanth-sankaran mentioned this pull request Jan 26, 2024

Parser changes for JEP 443 #1517

Merged

3 tasks

datho7561 force-pushed the jep-445-grammar branch from dfda508 to b918e38 Compare January 26, 2024 19:35

mickaelistria and others added 11 commits January 29, 2024 16:52

[JEP-445] Grammar, parser and AST for unnamed classes

900b515

Updated some tests accordingly: * Some DietRecoveryTests could be entirely removed as the case are now parse-able; some other were totally changes.

[WIP] fix some of the tests

e37063e

Signed-off-by: David Thompson <davthomp@redhat.com>

[WIP] fix more tests

b2b8adf

Signed-off-by: David Thompson <davthomp@redhat.com>

[WIP] fix even more tests

ccf3c54

Signed-off-by: David Thompson <davthomp@redhat.com>

Fix another test case

5d06855

Signed-off-by: David Thompson <davthomp@redhat.com>

[WIP] remove compliance information from unnamed class rules

30f3519

- sometimes valid Java < 21 code needs to go through these rules - we can manually generate the error if we detect an unnamed class in Java < 21 Signed-off-by: David Thompson <davthomp@redhat.com>

[WIP] fix formatting-related tests

5ebc1f9

Signed-off-by: David Thompson <davthomp@redhat.com>

[WIP] add new API ASTNode for UnnamedClass

4ffb39f

This should resolve some issues related to formatting Signed-off-by: David Thompson <davthomp@redhat.com>

[WIP] fix formatter

c147f7e

Signed-off-by: David Thompson <davthomp@redhat.com>

[WIP] fix type hierarchy tests

35ce2ac

Signed-off-by: David Thompson <davthomp@redhat.com>

[WIP] add type parameters to new DOM Node class

5265b65

Signed-off-by: David Thompson <davthomp@redhat.com>

datho7561 force-pushed the jep-445-grammar branch from b918e38 to 5265b65 Compare January 30, 2024 13:55

[WIP] fix infinite loop in parser

0f56e93

Signed-off-by: David Thompson <davthomp@redhat.com>

rgrunber mentioned this pull request Mar 6, 2024

[21] Paste action does not preserve Unnamed classes eclipse-jdt/eclipse.jdt.ui#1234

Open

mickaelistria closed this May 16, 2024

Jep 445 grammar #1521

Jep 445 grammar #1521

Conversation

mickaelistria commented Oct 27, 2023

What it does

How to test

Author checklist

mpalat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpalat Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpalat left a comment

Choose a reason for hiding this comment

srikanth-sankaran commented Nov 15, 2023

akurtakov commented Nov 15, 2023

srikanth-sankaran commented Nov 15, 2023

datho7561 commented Nov 21, 2023

datho7561 commented Dec 13, 2023

datho7561 commented Jan 9, 2024

datho7561 commented Jan 22, 2024

srikanth-sankaran commented Jan 23, 2024

srikanth-sankaran commented Jan 26, 2024

mpalat commented Feb 1, 2024

srikanth-sankaran commented Feb 1, 2024

mpalat commented Feb 2, 2024

datho7561 commented Feb 2, 2024

mpalat commented Feb 6, 2024

datho7561 commented Feb 6, 2024

srikanth-sankaran commented Feb 6, 2024

datho7561 commented Feb 6, 2024

akurtakov commented May 16, 2024

mpalat commented May 16, 2024

mpalat Nov 13, 2023 •

edited

Loading