search

google / guava

Google core libraries for Java
Apache License 2.0
50.14k stars 10.89k forks source link

Document that LinkedHashMultimap does not play well with mutable values #357

Closed gissuebot closed 1 year ago

gissuebot commented 9 years ago

Original issue created by vaughnmb on 2010-04-29 at 04:54 PM

What steps will reproduce the problem?

  1. Run the Junit test I provide

What is the expected output? What do you see instead? I expected LinkedHashMultimap.removeAll(key) method to remove everything for the provided key.

What version of the product are you using? On what operating system? google collections 1.0 final on Windows XP

Please provide any additional information below.

I am attaching my Junit Test.

gissuebot commented 9 years ago

Original comment posted by mitchblevins on 2010-04-29 at 11:05 PM

Attached is a slightly different test showing the same problem using a single map.

I would restate the issue as: Values in a LinkedHashMultimap that are mutated and subsequently removed via the removeAll(key) method are retained in the iterators for the values and the entryset.

gissuebot commented 9 years ago

Original comment posted by kevinb@google.com on 2010-04-30 at 01:02 AM

I have bad news for you. I'm afraid that mutating an object that you've added to a collection, in a way that affects its equals, hashCode, or (for sorted collections) comparison order, is something that you just can't do. Whether it's our collections or JDK collections such as TreeSet or HashMap.

I interpret this bug report as saying that we need to make sure the proper disclaimer is added to all our collections. One such as appears on ImmutableSet: "Warning: Like most sets, an ImmutableSet will not function correctly if an element is modified after being placed in the set."

gissuebot commented 9 years ago

Original comment posted by mitchblevins on 2010-04-30 at 01:23 AM

Your example comparing this to the behavior of j.u.c's HashMap doesn't resonate with me, since you can mutate the values of a hashmap without fear of causing a glitch in the Matrix, and the reported bug is for mutations of values.

At the very least, the disclaimer should be explicit.

gissuebot commented 9 years ago

Original comment posted by kevinb9n on 2010-05-01 at 01:06 AM

Sorry for being dismissive -- we'll look into it and if there is a reasonable fix for this that doesn't cost too much we'll make it.

gissuebot commented 9 years ago

Original comment posted by jared.l.levy on 2010-05-12 at 09:02 PM

LinkedHashMultimap.entries() has multiple bugs when values are mutated. Thanks for pointing out this problem.

Fixing your test case isn't difficult. I'll need to study the code to determine all the issues and whether it's feasible to fix them.

gissuebot commented 9 years ago

Original comment posted by jared.l.levy on 2010-05-12 at 10:02 PM

After thinking about this some more, I've change my mind. Instead of changing the behavior, HashSetMultimap and LinkedHashSetMultimap should document that mutating stored values leads to unpredictable behavior.

The following test would fail for either class.

Multimap<K, V> multimap = ... multimap.put(key, value); MutateValueInWayThatChangesHashCode(value); assertTrue(multimap.containsEntry(key, value));

The problem is that the values for a given key are stored in a HashSet or LinkedHashSet, and changing the hash code breaks contains() and remove() logic. In other words, HashMultimap values are analogous to HashMap keys, not HashMap values.

gissuebot commented 9 years ago

Original comment posted by kevinb@google.com on 2010-07-30 at 03:56 AM

(No comment entered for this change.)

Labels: -Priority-Medium

gissuebot commented 9 years ago

Original comment posted by fry@google.com on 2011-01-28 at 04:12 PM

(No comment entered for this change.)

Status: Accepted

gissuebot commented 9 years ago

Original comment posted by kevinb@google.com on 2011-07-13 at 06:18 PM

(No comment entered for this change.)

Status: Triaged

gissuebot commented 9 years ago

Original comment posted by fry@google.com on 2011-12-10 at 03:45 PM

(No comment entered for this change.)

Labels: Package-Collect

gissuebot commented 9 years ago

Original comment posted by wasserman.louis on 2012-02-05 at 09:47 PM

I'm marking this as Type-Documentation, if we still want to do anything with it.

Labels: -Type-Defect, Type-Documentation

gissuebot commented 9 years ago

Original comment posted by wasserman.louis on 2012-02-05 at 09:47 PM

(No comment entered for this change.)

gissuebot commented 9 years ago

Original comment posted by fry@google.com on 2012-02-16 at 07:17 PM

(No comment entered for this change.)

Status: Acknowledged

gissuebot commented 9 years ago

Original comment posted by kevinb@google.com on 2012-05-30 at 07:51 PM

(No comment entered for this change.)

Labels: -Type-Documentation, Type-ApiDocs

gissuebot commented 9 years ago

Original comment posted by kevinb@google.com on 2012-06-22 at 06:16 PM

(No comment entered for this change.)

Status: Research

kevinb9n commented 9 years ago

We think we do need some additional warnings about this.

kevinb9n commented 1 year ago

Well, I lost conviction that this merits a call-out in our docs. Modifying an object in any equals-affecting way while it is still contained in any collection is always risk-seeking behavior.

We plan to start publishing general practices advice and this'll be in there somewhere. Unfortunately I can't tell you now where you will be able to find it (sorry).