AbstractBiMap breaks the Map contract for put(key, value)

[GoogleCodeExporter]

The JavaDoc for java.util.Map.put(K,V), which is copied by Guava's BiMap 
without any comment, says the following:

"If the map previously contained a mapping for the key, the old value is 
replaced by the specified value."

See 
http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/collect
/BiMap.html#put(K, V) for example

Here is that method's implementation in AbstractBiMap:

**********************************
  @Override public V put(@Nullable K key, @Nullable V value) {
    return putInBothMaps(key, value, false);
  }

  private V putInBothMaps(@Nullable K key, @Nullable V value, boolean force) {
    checkKey(key);
    checkValue(value);
    boolean containedKey = containsKey(key);
    if (containedKey && Objects.equal(value, get(key))) {
      return value;
    }
    if (force) {
      inverse().remove(value);
    } else {
      checkArgument(!containsValue(value), "value already present: %s", value);
    }
    V oldValue = delegate.put(key, value);
    updateInverseMap(key, containedKey, oldValue, value);
    return oldValue;
  }

**********************************

Observe that, if the old value is .equal() to the new one, the method does 
*not* replace the value, and it returns the *new* instead of the old value:

if (containedKey && Objects.equal(value, get(key))) {
  return value;
}

Note that if you pass it a distinct (in the == sense) value that is equal (in 
the Object.equals sense), it does pretty much the opposite of what the spec 
says: it keeps the old value, and returns the new one.

(I noticed that because I have code that depends on the specified Map behavior, 
which is passed a BiMap from outside code.)

I believe the BiMap should respect the Map.put contract; it's probably silently 
breaking other code as well right now.

By the way, the inverse map has the same problem. That is, after doing

bimap.put(b, a); bimap.inverse().put(a, B);

(where b!=B && b.equals(B)) bimap should be {a:B} rather than {a:b}.

The contract for Map doesn't say anything about the key in put, but for 
consistency with the inverse I think BiMap should do the same thing, i.e.:

{a:b}.put(A,b) -> {A:b}
{a:b}.put(a,B) -> {a:B}
{a:b}.put(A,B) -> {A:B}

Original issue reported on code.google.com by bogd...@gmail.com on 13 Mar 2014 at 5:37

[GoogleCodeExporter]

I forgot to mention: AFAIK AbstractBiMap is used for all but the Immutable 
Guava implementations, so this affects all maps for which it is relevant

Original comment by bogd...@gmail.com on 13 Mar 2014 at 5:40

[GoogleCodeExporter]

I believe we've historically interpreted the Collections Framework's APIs as 
treating .equals() objects as interchangeable in every way, and not making any 
guarantees about reference equality.  To give another example, 
Ints.asList(int[]) does not guarantee that list.get(i) == list.get(i), because 
it needs to rebox the Integer on every get() call.

Original comment by lowas...@google.com on 13 Mar 2014 at 6:16

[GoogleCodeExporter]

Guava decided early on that "equals means interchangeable", and we would have 
to make rather a lot of changes if if we were to start treating reference 
identity as significant. (Think what would happen to HashMultiset! It seems 
almost unimplementable.)

We believe that 99% of the time that you care about the distinction between 
equal instances, it implies a bug in equals(). Equal means interchangeable. We 
realize that the valid need for identity-based structures exist, but in our 
experience it's much rarer than users think.

Original comment by kevinb@google.com on 13 Mar 2014 at 7:46

[GoogleCodeExporter]

All that said, it's possible there's a simple and harmless fix here; I don't 
know yet. Also there might be doc improvements that could help.

Original comment by kevinb@google.com on 13 Mar 2014 at 7:47

[GoogleCodeExporter]

Yes, well, I can understand the practical argument for generally basing 
collections on  equals. But my problem is that BiMap extends Map, and yet acts 
differently from it without even documenting the incompatibility anywhere. 

Note that the Map.put contract says “old value is replaced by the specified 
value”, it says nothing about identity, equality, etc. (Technically speaking, 
a map as document should perform every step of the put even if the new value is 
== to the old.)

Now, “equals means interchangeable” is a defensible decision for the 
library — though given the diversity of Equivalences, Comparators, and the 
links to memory from weak-reference–based collections, one might hope for a 
bit more subtlety — but at the very least it should be mentioned in the 
documentation of each class. IMO at least the methods that are declared from 
the standard Java collections should document thoroughly every such detail.

Note that HashMultiset only extends Collection and Iterable; the first has a 
very 'loose' contract (one shouldn't have many expectations about a 
Collection<X> without looking at the details), and the last a very simple one. 
Even more, Ints.asList is free to do (and guarantee) whatever it wants, since 
it doesn't extend any standard class. The problem here is that BiMap extends 
Map but (at least arguably) doesn't follow its contract. Even if you had that 
argument and decided that the BiMap behavior is allowed, it should at least be 
clearly documented as a difference from most maps.

Original comment by bogd...@gmail.com on 14 Mar 2014 at 7:46

[GoogleCodeExporter]

This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:09

• Added labels: MigratedToGitHub

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:17

• Changed state: Migrated

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 3 Nov 2014 at 9:07

• Added labels: Restrict-AddIssueComment-Commit