guava icon indicating copy to clipboard operation
guava copied to clipboard

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

Open h908714124 opened this issue 3 years ago • 1 comments

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

h908714124 avatar Jan 21 '22 13:01 h908714124

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.

cpovirk avatar Jan 21 '22 14:01 cpovirk