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

Which kind of ImmutableMultimap does ImmutableMultimap.builder().build() create? #5888

Open
h908714124 opened this issue Jan 21, 2022 · 1 comment
Labels
P3 package=collect type=api-docs Change/add API documentation

Comments

@h908714124
Copy link

h908714124 commented Jan 21, 2022

Here's a quote from the class-level javadoc of ImmutableMultimap:

Warning: avoid direct usage of ImmutableMultimap as a type (as with Multimap itself). Prefer subtypes such as ImmutableSetMultimap or ImmutableListMultimap, which have well-defined Multimap.equals(java.lang.Object) semantics, thus avoiding a common source of bugs and confusion.

In this regard, it seems unfortunate that ImmutableMultimap.builder().build() returns ImmutableMultimap. Is this an ImmutableSetMultimap or a ImmutableListMultimap? At least the Javadoc should be clear about that. Or better yet:

  • ImmutableMultimap.Builder should be an abstract class, to avoid the confusion (this would be a breaking change)
  • ImmutableMultimap.builder() should return ImmutableListMultimap.Builder (probably not a breaking change)
  • For clarity, consider adding explicit methods ImmutableMultimap.setMultimapBuilder() and ImmutableMultimap.listMultimapBuilder() (not a breaking change)

https://guava.dev/releases/snapshot-jre/api/docs/com/google/common/collect/ImmutableMultimap.html

https://guava.dev/releases/snapshot-jre/api/docs/com/google/common/collect/ImmutableMultimap.Builder.html

@cpovirk
Copy link
Member

cpovirk commented Jan 21, 2022

If we had the guts, we would deprecate all ImmutableMultimap creation APIs :( Documentation of the behavior would be a good idea, but there's never a reason to prefer the ambiguously named ImmutableMultimap creation APIs over the ImmutableListMultimap ones.

One particularly unfortunate wrinkle is that ImmutableMultimap.copyOf bucks the trend by returning an ImmutableSetMultimap if you pass it one as input.

We can't actually change any return types because the ImmutableSetMultimap APIs are checked as overrides. (That may also complicate even deprecation, but perhaps not?) Still, the docs should be improved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 package=collect type=api-docs Change/add API documentation
Projects
None yet
Development

No branches or pull requests

2 participants