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

Deprecate and replace Guava-using APIs in Scopes and QualifiedName #2978

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HannesWell
Copy link
Contributor

Add new API methods in Scopes that usage java.util.function.Function instead of com.google.common.base.Function and Map<K, Collection<V>> instead of Multimap<K, V> and replace internal usages of Guava where simply possible.

Part of #2975

@@ -486,7 +494,7 @@ public boolean startsWithIgnoreCase(QualifiedName prefix) {
}

protected boolean startsWith(QualifiedName prefix, boolean ignoreCase) {
Preconditions.checkArgument(prefix != null, "prefix must not be null");
Objects.requireNonNull(prefix, "prefix must not be null");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now throws a NullPointerException instead of an IllegalArgumentException. I assume that's the reason for the test-failure.
Is that an acceptable change or should it continue to throw a IllegalArgumentException if the argument is null?

@HannesWell HannesWell force-pushed the replace-guava-in-scopes branch 2 times, most recently from 87334e3 to 91bee50 Compare April 5, 2024 21:12
@HannesWell HannesWell force-pushed the replace-guava-in-scopes branch from 91bee50 to 8e0c981 Compare April 5, 2024 22:02
@LorenzoBettini
Copy link
Contributor

Just a quick comment, though I have yet to try that.

If you pass a lambda expression to these methods, wouldn't you now get an ambiguous error from the Java compiler?
I think the currently used types from Guava are functional interfaces, so Java wouldn't know which overloaded method to call now? Or am I missing something? cc @szarnekow

@szarnekow
Copy link
Contributor

If you pass a lambda expression to these methods, wouldn't you now get an ambiguous error from the Java compiler?

The Guava interfaces extend the regular Java declarations. Thus the compiler would pick the more specific method and bind to the deprecated method. To get rid of deprecation warnings, client code would need to insert casts whenever a lambda expression was used.

@LorenzoBettini
Copy link
Contributor

@szarnekow thanks! I'm not at a computer and I missed the inheritance relation. So no compilation error, but you need to cast to use the non deprecated version. Personally, I don't like that ;)

@HannesWell
Copy link
Contributor Author

If you pass a lambda expression to these methods, wouldn't you now get an ambiguous error from the Java compiler?

The Guava interfaces extend the regular Java declarations. Thus the compiler would pick the more specific method and bind to the deprecated method. To get rid of deprecation warnings, client code would need to insert casts whenever a lambda expression was used.

Unfortunately, yes. I even had to add casts in this PR and I agree that that's not nice. The only alternative I see is to change the name of the replacement method or change its arguments if there are multiple. The latter is in my onion not so nice and not always possible. From a user-perspective, the former is not that bad. 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.
So the hard work is mainly for the developers to find a suitable other name.

@szarnekow
Copy link
Contributor

The only alternative I see

I see another one: Accept the fact that Xtext and Xtext languages have a dependency on Guava. I may sound like a broken record, but please please pretty please let's carefully weigh our options wrt #2975 before jumping to conclusions and wasting time to change and/or review a ton of code.

@HannesWell
Copy link
Contributor Author

HannesWell commented Apr 6, 2024

The only alternative I see

I see another one: Accept the fact that Xtext and Xtext languages have a dependency on Guava. I may sound like a broken record, but please please pretty please let's carefully weigh our options wrt #2975 before jumping to conclusions and wasting time to change and/or review a ton of code.

I'm all for discussing this, I just answered here first and my answer in #2975 took a bit time to write :)

@HannesWell HannesWell force-pushed the replace-guava-in-scopes branch from 8e0c981 to 3eccb8a Compare April 14, 2024 07:27
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

Successfully merging this pull request may close these issues.

3 participants