fix java source lang record handling #3048
Conversation
org.eclipse.xtext.java/src/org/eclipse/xtext/java/resource/JavaDerivedStateComputer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
In general, I'd like to have a test that would fail without this change. Instead, org.eclipse.xtext.java.tests.JavaSourceLanguageTest.testJava21Record() still succeeds without this change.
For example, in the current shape, if I comment
javaConfig.setJavaSourceLevel(JavaVersion.JAVA21);
javaConfig.setJavaTargetLevel(JavaVersion.JAVA21);the test fails; so I was misled that everything else would work by just setting the Java version.
Or did I miss something?
See also the other small changes.
org.eclipse.xtext.java.tests/src/org/eclipse/xtext/java/tests/JavaSourceLanguageTest.java
Outdated
Show resolved
Hide resolved
org.eclipse.xtext.java.tests/src/org/eclipse/xtext/java/tests/JavaSourceLanguageTest.java
Outdated
Show resolved
Hide resolved
org.eclipse.xtext.java/src/org/eclipse/xtext/java/resource/JavaDerivedStateComputer.java
Outdated
Show resolved
Hide resolved
|
|
for me it fails without change |
cdietrich
force-pushed
the
cd-java-source-lang-21
branch
3 times, most recently
from
1ae01d3
to
19acf4b
Compare
| @@ -126,6 +126,9 @@ public JvmDeclaredType createType(TypeDeclaration type, String packageName) { | |||
| case TypeDeclaration.ANNOTATION_TYPE_DECL: | |||
| jvmType = TypesFactory.eINSTANCE.createJvmAnnotationType(); | |||
| break; | |||
| case TypeDeclaration.RECORD_DECL: | |||
There was a problem hiding this comment.
its a final. so it will get inlined anyway. afaik
Yes, I verified that in your branch. I was referring to the previous shape of the test: without the JAVA_21 specification it was failing, with the specification it was succeeding, but it wasn't testing much as you said. Maybe it's worthwhile to leave a comment in the From what you said in the previous comments also in the other issues, my mistake was not thinking about the install stub step which doesn't report errors, and then the binary class is found. Or did I misunderstand that as well? In general, it would be better to avoid such problems in the future, that's why I'd leave a comment in the test. Basically, I didn't understand how that test was supposed to work. As for the rest, I'm fine with the PR. |
|
@LorenzoBettini have no longer access to a dev setup. feel free to push a comment with a commit to this brAnch |
Yes I can do that, but I still don't understand what was wrong with the previous version of the test. |
|
@LorenzoBettini i have understood that in the previos version the resourceset method already triggers the install stubs thing and thus does it with java 8 (or 5) or whatever default is and there it does not fail |
Signed-off-by: Christian Dietrich <christian.dietrich.opensource@gmail.com>
cdietrich
force-pushed
the
cd-java-source-lang-21
branch
from
19acf4b
to
4de0539
Compare
Signed-off-by: Christian Dietrich <christian.dietrich.opensource@gmail.com>
|
Thank you @cdietrich for adding the comment and fixing this! |
|
@szarnekow could you please explain where the JavaDerivedStateComputer is used? |
|
Cause gradle uses the Java source language under the hood to get index entries for Java files in the project |
|
Ok! That's why it never came into play in my tests and PRs! So Gradle doesn't use Standalone builder. So, even more sensible, please try the experiments I said in my comment in the reference projects issue. |
|
Yes it uses the new builder org.eclipse.xtext.build.IncrementalBuilder |
No description provided.