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

Emit java.util.Objects in generated code instead of Guava's Objects #2993

Conversation

HannesWell
Copy link
Member

@HannesWell HannesWell commented Apr 14, 2024

This PR adjusts XbaseCompiler and ObjectExtensions to emit java.util.Objects instead Guava's Objects class for generated code. java.util.Objects.equals is a drop-in replacement for Guava's Objects.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

@HannesWell
Copy link
Member Author

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.

@szarnekow szarnekow self-requested a review April 14, 2024 14:23
@szarnekow szarnekow added this to the Release_2.35 milestone Apr 14, 2024
@HannesWell HannesWell force-pushed the emit_java.utils.Objects_in_generate_code branch from 710494a to 26bb015 Compare April 14, 2024 14:32
@@ -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);
Copy link
Contributor

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 ''' ... '''.

Copy link
Member Author

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.

Copy link
Contributor

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

@@ -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)) {
Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member

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

Copy link
Member

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

Copy link
Contributor

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.

Copy link
Member

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

@LorenzoBettini
Copy link
Contributor

I've just left a few comments, I see @szarnekow is already reviewing that.

@HannesWell
Copy link
Member Author

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?

@LorenzoBettini
Copy link
Contributor

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.

@szarnekow
Copy link
Contributor

Ok. But let's make sure we regen Xtend before M2

@HannesWell
Copy link
Member Author

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.

So should I revert the anticipatory changes of generated code or leave it as it is?

@cdietrich
Copy link
Member

I assume you can leave it as the build will replace then anyway

@HannesWell
Copy link
Member Author

I assume you can leave it as the build will replace then anyway

Yes the build replaces it automatically, but at the moment this PR already contains the changes the XtendBuilder will do once this change is available in the development Eclipses (I created the current state with a secondary Eclipse launched with this PR).
So without this change available, i.e. if you don't update to the nightly build immediatly you will get the following changes in your WS that undo parts of this PR:
grafik

But from your previous comments I understand it that you don't want to have that anticipatory changes now but apply it once you update the dev-Eclipses to M2?

@cdietrich
Copy link
Member

I don’t care. Repo code differs from shipped code for m2

@szarnekow
Copy link
Contributor

if you don't update to the nightly build immediatly

It's necessary in any case. So no worries. Thank you for the modernization of the generated code!

@HannesWell
Copy link
Member Author

if you don't update to the nightly build immediatly

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?

@LorenzoBettini LorenzoBettini merged commit 385e329 into eclipse:main Apr 27, 2024
6 checks passed
@LorenzoBettini
Copy link
Contributor

@HannesWell Thanks! I've just merged.

@LorenzoBettini
Copy link
Contributor

The first one who updates the development environment to the nightly update site can then create a PR with the regenerated code.

@LorenzoBettini
Copy link
Contributor

@cdietrich I don't understand what you were proposing here:

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

@HannesWell HannesWell deleted the emit_java.utils.Objects_in_generate_code branch April 27, 2024 21:20
@HannesWell
Copy link
Member Author

@HannesWell Thanks! I've just merged.

Thank you. :)

@cdietrich
Copy link
Member

Normally I would wait after the milestone or build an extra milestone but I have no Cappa so I don’t care

LorenzoBettini added a commit that referenced this pull request Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants