-
Notifications
You must be signed in to change notification settings - Fork 320
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
Emit java.util.Objects in generated code instead of Guava's Objects #2993
Emit java.util.Objects in generated code instead of Guava's Objects #2993
Conversation
Many changes in the generated code could be avoided if the (some of) the compiler tests would be migrated to pure Java classes using text-blocks, like in #2991. |
710494a
to
26bb015
Compare
@@ -170,6 +170,6 @@ public void testIssue230() { | |||
_builder.newLine(); | |||
_builder.append("}"); | |||
_builder.newLine(); | |||
this.assertCompilesTo("\n\t\tpackage b\n\t\t\n\t\timport org.eclipse.xtend.lib.annotations.Accessors\n\t\t\n\t\tclass MyTest {\n\t\t\t\n\t\t\t@Accessors\n\t\t\tstatic class A1 {String name}\n\t\t\t@Accessors\n\t\t\tstatic class A2 {String name}\n\t\t\t\n\t\t\tdef doItWithNumber(Object n) \'\'\'\n\t\t\t�IF n instanceof A1�\n\t\t\t\tA1: �n.name�\n\t\t\t�ELSEIF n instanceof A2�\n\t\t\t\tA2: �n.name�\n\t\t\t�ELSE�\n\t\t\t\tElse: �n�\n\t\t\t�ENDIF�\n\t\t\t\'\'\'\n\t\t}\n\t\t", _builder); | |||
this.assertCompilesTo("\r\n\t\tpackage b\r\n\t\t\r\n\t\timport org.eclipse.xtend.lib.annotations.Accessors\r\n\t\t\r\n\t\tclass MyTest {\r\n\t\t\t\r\n\t\t\t@Accessors\r\n\t\t\tstatic class A1 {String name}\r\n\t\t\t@Accessors\r\n\t\t\tstatic class A2 {String name}\r\n\t\t\t\r\n\t\t\tdef doItWithNumber(Object n) \'\'\'\r\n\t\t\t�IF n instanceof A1�\r\n\t\t\t\tA1: �n.name�\r\n\t\t\t�ELSEIF n instanceof A2�\r\n\t\t\t\tA2: �n.name�\r\n\t\t\t�ELSE�\r\n\t\t\t\tElse: �n�\r\n\t\t\t�ENDIF�\r\n\t\t\t\'\'\'\r\n\t\t}\r\n\t\t", _builder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WARNING: "\r" Windows EOLs appeared! If you use Windows and use Xtend strings that span several lines you run into this (there's an open issue for that). I don't mean multiline strings ''' ... '''.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh. Yes, I noticed already that this can happen as the changed java-files poluted my git staging. I actually wanted to check it before creating creating the PR, but forgot it. Sorry for that and thanks for spotting this.
I'm about to fix that.
In general this problem could be avoided if those tests would be converted to plain java classes using text-blocks, similar to #2991, but this would require a BREE of Java-17.
As far as I understand the plan for Java-21 you intend to raise the required java version to 17 with the support of Java-21.
I don't know how far your work on the Java-21 support has already reached. But if you are fine with it, I can provide a PR already now to raise the required Java-version for all Xtext bundles (in the MANIFEST.MF
, .classpath
and JDT settings, plus root pom.xml) and then a subsequent one to migrate the tests as suggested above.
This would also reduce the number of changes in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd wait for the transition to Java text blocks and Java 17.
In particular, similar to the others you experienced here, this issue is due to tests not using Xtend template strings ('''
) but standard quoted strings that keep the \r
.
I don't think those tests were meant to use standard strings, and maybe at that time, it went unnoticed because most developers were using macOS or Linux.
I've opened an issue to track that: #2999
...ests/xtend-gen/org/eclipse/xtend/core/tests/macro/AbstractReusableActiveAnnotationTests.java
Outdated
Show resolved
Hide resolved
org.eclipse.xtend.core.tests/xtend-gen/org/eclipse/xtend/core/tests/smoke/SmokeTestCase.java
Outdated
Show resolved
Hide resolved
@@ -225,48 +225,48 @@ private Object getDefaultValue(final Class<?> clazz) { | |||
boolean _isPrimitive = clazz.isPrimitive(); | |||
if (_isPrimitive) { | |||
boolean _matched = false; | |||
if (Objects.equal(clazz, Boolean.TYPE)) { | |||
if (com.google.common.base.Objects.equal(clazz, Boolean.TYPE)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why here there's still Objects.equal? I've seen that also below in another file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That remained since it is generated by the XbaseCompiler
which is adapted just in this PR and my development-eclipse doesn't have this fix yet. I oversaw that since when I didn't have #2992 in the same branch and also had my search configured to not contain derived resources.
I have now used a secondary Eclipse launched from my Xtext workspace to regenerated all the java files that still contained Guava's Objects class.
Now, with this change I don't find any occurrence of com.google.common.base.Objects
in my Xtext workspace, not even in derived resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually we do this update with a milestone update for the xtend version in the root pom
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you prefer.
Of course it would be bad to have mismatches in the generated code by the current XBaseCompiler.
So I think it mainly depends on which version you Xtext developers use. If you only use the latest milestone, then I agree this should be postponed, if you use the latest nightly build it is probably ok to change it now in since you would adapt it the next day after this is submitted any ways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HannesWell I didn't think about that.
I typically use the latest nightly in my Xtext workspace.
@cdietrich do you prefer to keep the old version of the generated files for the moment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have Cappa to regen all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also is this about fragment or about xtend
If xtend regen with the m2 boot strap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cdietrich this is not about the fragment but about a change in XbaseCompiler, so the regenerated Java code from Xtend could be done once we install the new Xtend in the development workspace (i.e., when this is merged to master, the nightly p2 site is published, and the development workspace takes Xtend from the nightly p2 site). When we publish M1, the xtend maven plugin will also behave accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is why I hesitate to regen rn. If we can burn milestones we could build it now after merge and then for the real milestone but I guess it’s not Wort or
I've just left a few comments, I see @szarnekow is already reviewing that. |
26bb015
to
720e7a9
Compare
M2 is approaching so I wonder if this should be landed before it and if there is anything I can do to make this to completion? |
As for me, we could merge this; the updated generated Java files can be done in another PR, once this update is part of the nightly update site. |
Ok. But let's make sure we regen Xtend before M2 |
So should I revert the anticipatory changes of generated code or leave it as it is? |
I assume you can leave it as the build will replace then anyway |
I don’t care. Repo code differs from shipped code for m2 |
It's necessary in any case. So no worries. Thank you for the modernization of the generated code! |
Great and you are welcome. I'm glad to get more modern code generated. :) Since everbody seem to be fine with this change can it be submitted or should I rebase it on the current master first? |
@HannesWell Thanks! I've just merged. |
The first one who updates the development environment to the nightly update site can then create a PR with the regenerated code. |
@cdietrich I don't understand what you were proposing here:
|
Thank you. :) |
Normally I would wait after the milestone or build an extra milestone but I have no Cappa so I don’t care |
This PR adjusts
XbaseCompiler
andObjectExtensions
to emitjava.util.Objects
instead Guava'sObjects
class for generated code.java.util.Objects.equals
is a drop-in replacement for Guava'sObjects.equal()
.All other changes are either just adoption of compiler tests or updates of xtend-generated code committed into this repository.
Some changes in the code generated for the examples is done in anticipation of this change. Without this change being submitted the compiler respectivly the Xtend-Container cause the old code to be generated.
This is the complement of #2992.
Part of #2975