Check live options when determine what java level is used for parsing#4856
Check live options when determine what java level is used for parsing#4856iloveeclipse merged 2 commits intoeclipse-jdt:masterfrom
Conversation
8fb97ea to
94a948e
Compare
|
@stephan-herrmann can you review? This now passes the enhanced test-case with check for new language levels. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the Parser class to dynamically check Java version options instead of caching them in static fields. This enables proper multi-release JAR support where different source folders need different Java Language Specification (JLS) levels.
Changes:
- Removed static boolean fields (
parsingJava*Plus,previewEnabled) from Parser that were set once in the constructor - Replaced field access with dynamic getter methods that check
this.options.sourceLevelon each call - Added a test case with Java 21 features (pattern matching, text blocks) to verify multi-release compilation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Parser.java | Removes cached parser version fields, adds dynamic getter methods that check options live, enabling proper multi-release support |
| CompletionParser.java | Updates single reference from field to method call for preview feature check |
| MultiReleaseTests.java | Adds test case with Java 21 features to verify multi-release compilation works correctly |
Comments suppressed due to low confidence (1)
org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/parser/Parser.java:13331
- Inconsistent spacing around null comparison operator. The codebase predominantly uses spaces around != (e.g.,
!= null), but this code uses!=nullwithout spaces. For consistency, consider using spaces:this.options != null.
return this.options!=null && this.options.sourceLevel >= ClassFileConstants.JDK9;
}
protected boolean isParsingJava10Plus() {
return this.options!=null && this.options.sourceLevel >= ClassFileConstants.JDK10;
}
protected boolean isParsingJava11Plus() {
return this.options!=null && this.options.sourceLevel >= ClassFileConstants.JDK11;
}
protected boolean isParsingJava14Plus() {
return this.options!=null && this.options.sourceLevel >= ClassFileConstants.JDK14;
}
protected boolean isParsingJava15Plus() {
return this.options!=null && this.options.sourceLevel >= ClassFileConstants.JDK15;
}
protected boolean isParsingJava17Plus() {
return this.options!=null && this.options.sourceLevel >= ClassFileConstants.JDK17;
}
protected boolean isParsingJava18Plus() {
return this.options!=null && this.options.sourceLevel >= ClassFileConstants.JDK18;
}
protected boolean isParsingJava21Plus() {
return this.options!=null && this.options.sourceLevel >= ClassFileConstants.JDK21;
}
protected boolean isParsingJava22Plus() {
return this.options!=null && this.options.sourceLevel >= ClassFileConstants.JDK22;
}
protected boolean isPreviewEnabled() {
return this.options!=null && this.options.sourceLevel == ClassFileConstants.getLatestJDKLevel() && this.options.enablePreviewFeatures;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/parser/Parser.java
Show resolved
Hide resolved
srikanth-sankaran
left a comment
There was a problem hiding this comment.
Looks good other than that many null checks are provably redundant.
Didn't check the test, only the code changes
iloveeclipse
left a comment
There was a problem hiding this comment.
Moving fields access to methods revealed unused code. I would not add unused methods, we have enough dead code in JDT. Beside this should be OK.
org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/parser/Parser.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/parser/Parser.java
Outdated
Show resolved
Hide resolved
| return this.options != null && this.options.sourceLevel >= ClassFileConstants.JDK11; | ||
| } | ||
|
|
||
| protected boolean isParsingJava14Plus() { |
There was a problem hiding this comment.
This method can be removed, all callers too, but in a later PR
org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/parser/Parser.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/parser/Parser.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/parser/Parser.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/parser/Parser.java
Outdated
Show resolved
Hide resolved
|
@laeubi : could you please squash three last commits into one, so that PR has two commits - one for tests and one for the fix? Thanks. |
Currently the Parser determines what java version is parsed once in the constructor and store them into (modifiable) fields. This causes issues when the options are modified as everything else works but this particular fields are not updated. Instead of having a way to update those fields and as they do not cache any complex computations, this fields are now replaced with appropriate getters like it is already done in some cases. Fix eclipse-jdt#4849
b567a5c to
40df6b0
Compare
|
Squashed and rebased now. |
|
OK all tests green, let's merge for RC1. |
Currently the Parser determines what java version is parsed once in the
constructor and store them into (modifiable) fields. This causes issues
when the options are modified as everything else works but this
particular fields are not updated.
Instead of having a way to update those fields and as they do not cache
any complex computations, this fields are now replaced with appropriate
getters like it is already done in some cases.
Fix #4849