Make ImmutableEntry a public type

[dimo414]

Because ImmutableEntry is a package-private type Maps.immutableEntry() has to declare its return type as Map.Entry, which (because it's an interface) can't be guaranteed to be immutable. This means that users of Maps.immutableEntry() can't express the immutability of the entry objects they're constructing to anyone they pass them off to. Since the immutable types are intended to be "'Interfaces', not implementations", I think it would make sense to expose this type publicly.

[ben-manes]

AbstractMap.SimpleImmutableEntry is built in, so I don't think this is necessary.

[cpovirk]

In fact, we've talked about changing immutableEntry to return SimpleImmutableEntry (especially on Android to save a class there). We were thwarted by some people who rely on immutableEntry to return a GWT-serializable object (though in principle we could do it anyway for Android, since we don't support GWT there). (We made some baby steps, deleting subclass ImmutableMapEntry in the Android branch in CL 150124044, and you can see where we got stuck removing ImmutableEntry itself in CL 146917938.)

At a higher level, I don't feel too bad about lacking a way to express Entry immutability in the type system: Entry objects rarely occur outside maps/multimaps, and the way Java generics work, entrySet is always declared as a set of plain Entry objects, never ImmutableEntry. (Possibly this is an argument for being willing to break the people who depend on immutableEntry to return a GWT-serializable type: How many people are serializing bare Entry objects, rather than serializing maps/multimaps? Of course I'm sure the answer is "more than zero," since presumably that's why we permitted serialization in the first place.)

[dimo414]

SimpleImmutableMap is actually still problematic since it's not final (and hence subclasses could potentially still not be immutable).

There are cases where you want to represent a key-value mapping but can't use a Map structure directly (e.g. this is how proto3 supports maps) and providing a properly immutable Entry implementation would make such patterns safer, rare as they may be.

[cowwoc]

@dimo414 I thought you had a point about SimpleImmutableMap not being immutable because it is not being final, but upon digging into the source-code I question whether this is true.

Both "key" and "value" fields are private and final. Even if a subclass were to override setValue() it could not mutate the state.

The only other attack vectors that come to mind are Reflection and Serialization, but you have as much ability to attack the superclass using Reflection as you do with the subclass. And serialization won't help you modify already-instantiated instances.

Do you agree, or have I missed an attack vector?

[Maaartinus]

@cowwoc I guess, you both mean AbstractMap.SimpleImmutableEntry. The fields being immutable doesn't help when you can override methods using them. As no single method is final, you can do pretty everything (e.g., add mutable key2 and value2 and redirect all methods to them, and as bonus randomize hashCode).

[cowwoc]

@Maaartinus true. I filed a bug report against the JDK. I'll post the link here once it becomes public.

[cowwoc]

Here is the JDK bug report: https://bugs.openjdk.java.net/browse/JDK-8199318

[dimo414]

As Martin said on the bug, it's doubtful the JDK can retrofit this class to be properly immutable at this point. Hence why it would be useful for Guava to provide such a type :)