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

Reduce usage of Guava #2975

Open
HannesWell opened this issue Apr 5, 2024 · 6 comments
Open

Reduce usage of Guava #2975

HannesWell opened this issue Apr 5, 2024 · 6 comments

Comments

@HannesWell
Copy link
Member

Guava is a dependency that's often difficult to handle (one reason is that Xtext is re-exporting it) and many of the enhancements Guava offered are part of the Java's standard library in recent versions. For the most commonly used elements in Guava there are often (almost) drop-in replacements or equivalent alternatives that are simple to migrate to. Many elements in Guava even recommend to use standard alternatives in their doc.
Therefore I want to suggest to replace Guava where it is possible.

In general the usage is split into two parts:

  • Internal usages, which can be replaced by a technical suitable solution at any time.
  • Usage in APIs.
    For APIs alternatives would have to be added and the existing methods would need to be deprecated for some time before they can be removed? This also relates to Xtext and (binary) backwards compatibility #2668 and should probably be handled together.

In general Guava is heavily used by Xtext, searching the *.java files in this repo for com.google.common gives 3535 hits. So replacing it is a major task and probably more a journey. At its end maybe Guava can even be dropped completely.
But since it is hard to even get just a completely overview of its usage, my suggestion is to start with simple cases like replacing Lists.newArrayList() by new ArrayList<>() etc. only in internal usages and see how far we can get.

HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Apr 5, 2024
In Java-19 there is even static factory method HashMap.newHashMap(int)
that can be used as a drop-in replacement once Java-19 or later is
required.

Part of eclipse#2975
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Apr 5, 2024
In Java-19 there is even static factory method HashMap.newHashMap(int)
that can be used as a drop-in replacement once Java-19 or later is
required.
For now I calculate the necessary capacity explicitly using the formula:
expected-size *4/3+1

Part of eclipse#2975
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Apr 5, 2024
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Apr 5, 2024
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Apr 5, 2024
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Apr 5, 2024
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Apr 5, 2024
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Apr 5, 2024
@szarnekow
Copy link
Contributor

Before putting more effort into this, let's analyze the impact of such a change and agree it's worthwhile to pursue.

Can we agree on a few things:

  • Backwards compatibility is an asset - both source and binary compatibility
  • Disruption for users without any benefit from the users' perspective is counter-productive
  • Guava is a mature library with a ton of useful APIs, some (not all) of which have nowadays and equivalent in Java
  • Mistakes were made in the past (most severe is probably the habbit of re-exporting dependencies)

What is the net-effect of overloading existing APIs with java.util.function types where currently Guava's pseudo-deprecated types are used? Likely warnings in source code. Is it any better for an existing code-base? I've doubts.

Before we follow through and look at the detailled implications of whatever change we're going to make, I'd like to understand why it is better. Better beyond an academic perception of better. I want to see that the disruption is carrying its own weight.

Of course this is only my opinion, so others are clearly invited to share their thoughts, too. But before more time is (maybe wasted, maybe not) on code changes, let's agree on a goal and let's make a plan that'll lead to that goal.

@LorenzoBettini
Copy link
Contributor

In general, I'd have no problem replacing things like Lists.newArrayList() with new ArrayList<>(), and that's what I've already done and doing when changing existing code.

I have doubts (see #2978 (comment)) that having overloaded methods with functional interfaces would work: the Java compiler would issue an error if you pass a lambda expression, wouldn't it?
In fact, I see explicit casts in @HannesWell 's PR when passing lambda expressions to the new method from the deprecated one. If Java issued compilation errors, that would be a huge problem ;)

If switching to Java functional interfaces instead of the Guava ones is considered, I think it would be better to simply replace the functional interfaces from Guava with the ones of Java without deprecation and overloading. That would be API breakage, but it should go completely unnoticed if clients used to call such methods with lambda expressions. I don't know if it could give problems from a binary point of view, though.

@szarnekow
Copy link
Contributor

I don't know if it could give problems from a binary point of view, though.

That would indeed be source-compatible but not binary-compatible.

@HannesWell
Copy link
Member Author

I have doubts (see #2978 (comment)) that having overloaded methods with functional interfaces would work: the Java compiler would issue an error if you pass a lambda expression, wouldn't it?

In order to resolve the issue that the compiler often picks the 'wrong' deprecated overload another alternative would be to slightly change the name of the method as mentioned in #2978 (comment). IIRC JDT now even offers a quick-fix to use the replacement (if only one method is mentioned in the @deprecated tag doc) and often one has to do some little adjustments anyway.

If switching to Java functional interfaces instead of the Guava ones is considered, I think it would be better to simply replace the functional interfaces from Guava with the ones of Java without deprecation and overloading. That would be API breakage, but it should go completely unnoticed if clients used to call such methods with lambda expressions. I don't know if it could give problems from a binary point of view, though.

Sebastian is right, that would be binary incompatible because the name all arguments and the return type of the method selected by the compiler is written into the byte-code. Because even the return-type is persisted you cannot even narrow the return type of a method without breaking binary-compatibility although it would be fully source-compatible and you cannot have methods in java that are only different by their return type: https://stackoverflow.com/a/58272625/14542697

Can we agree on a few things:

* Backwards compatibility is an asset - both source and binary compatibility

In general I agree with that, but with the restriction that I don't think that binary compatibility has to be retained forever by all means. Even Java itself breaks sometimes by removing functionality (the latest example is the degradation of Thread.stop() and its future removal). If there are good reasons to break something I think it is acceptable, but of course there should be clear path for users to migrate and sufficient time. For example in the Eclipse SDK we have a two years deprecation period and there should of course be a replacement unless some functionality is not necessary anymore at all.

* Disruption for users without any benefit from the users' perspective is counter-productive

Yes. But I would say that the consequences depend on who exactly the users are and what is considered a benefit. In my opinion a benefit for the user is also if the developer's live is made easier so that they can spend more time on other beneficial things or can even continue development at all. And about the users, there are users of xtext that generate new languages and users of that generated languages.
The last group probably does not benefit from the Guava reduction or a maybe fade-out, but the developer-users of Xtext and the Xtext users probably do on the long run with respect of maintenance.

* Guava is a mature library with a ton of useful APIs, some (not all) of which have nowadays and equivalent in Java

It is and I would say it was so successful that many parts made it in the JDK, which often allows to remove it. There are still many useful things in it and as initially written I cannot even tell if it is currently reasonably possible to replace all its usage in Xtext. But starting with trivial things would make it easier to get a complete overview. And if it would not yet be possible to complete this, it would be easier whenever it is possible in the future.

* Mistakes were made in the past (most severe is probably the habbit of re-exporting dependencies)

Yes. And alone removing the re-exports would probably already help a lot to ease the maintenance. And I think there are promising ways to 'deprecate' that using fragments as mentioned in #2668 (comment).
As said above I think one should try to correct such mistakes in a way that causes the least disturbance.
Of course Guava's excessive new Major versions are also a big contribution to the problem.

For more background why the current situation can be problematic:
At the moment Xtext (maybe also other projects in the SimRel but I only can report from Xtext) effectively mandates the most recent version of Guava that can be used in an Eclipse application. Other bundles that require a newer major version of Guava than Xtext does not work and leads to resolution errors by Equinox (I haven't investigated why it is happening exactly).
We have for example also another dependencies in our application that also use Guava and had the situation that a new Guava Major version was resolved so late in the SimRel endgame that it wasn't picked up. But that other dependency picked it up and required it and did a release shortly after that. But we had to wait to upgrade that dependency until the next Xtext release, because otherwise we had the mentioned resolution errors. And since it can also happen that one cannot update to the next SimRel immediately for other reasons that can be quite constraining.

Back then I created #2671 to just widen the Guava version ranges but even that failed and MWE was one issue. I'm not sure if it had worked if MWE would not use Guava, but I think it would at least be one step. The PR's I created so far are mainly to help to complete eclipse/mwe#287 which would already be one intermediate goal.

@merks
Copy link
Contributor

merks commented Apr 6, 2024

When I look at my customer project, it’s hard to imagine how guava can be completely eliminated and it’s hard to see how that would concretely make the world a better place for them. It would most certainly have a significant development impact without a benefit seen by the end user.

It might be the case that reducing the usage is a little like making it a little less pregnant. But pregnant is still pregnant.

@HannesWell
Copy link
Member Author

HannesWell commented Apr 12, 2024

When I look at my customer project, it’s hard to imagine how guava can be completely eliminated and it’s hard to see how that would concretely make the world a better place for them. It would most certainly have a significant development impact without a benefit seen by the end user.

I don't advocate to generally replace Guava, I only suggest to replace it where are (more or less) direct replacement in the Java standard library. There are definitely cases where Guava provides valuables addition.

It might be the case that reducing the usage is a little like making it a little less pregnant. But pregnant is still pregnant.

The situation does look so absolute to me. I think we can still reach intermediate goal that would be valuable.
As already said the most important one would probably be to remove the re-exports. I would try to work-out a compatibility-fragment based approach as mentioned in #2668 (comment), if there is interest for it.
A second intermediate goal would for example be that projects generated by Xtext don't depend or at least depend less on Guava (e.g. MWE via eclipse/mwe#287). The less bundles depend on Guava the lower is the chance of of conflicting version-ranges.

HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Apr 12, 2024
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Apr 14, 2024
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Apr 14, 2024
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Apr 14, 2024
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Apr 14, 2024
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Apr 14, 2024
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Apr 14, 2024
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Apr 14, 2024
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Apr 15, 2024
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Apr 26, 2024
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Aug 8, 2024
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Aug 11, 2024
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Aug 12, 2024
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Aug 14, 2024
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Aug 14, 2024
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this issue Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants