ImmutableSetMultimap.Builder.put should not throw exceptions on duplicate KV pairs

[GoogleCodeExporter]

Currently, if you try to add the same key/value pair to an
ImmutableSetMultimap.Builder, an IllegalArgumentException is thrown.  This
should be changed to be consistent with the SetMultimap contract, which
specifies "Adding a key-value pair that's already in the multimap has no
effect."

Even though the ImmutableSetMultimap.Builder does not technically implement
SetMultimap (although ImmutableSetMultimap does), it is highly unintuitive
that its put method works differently.

Original issue reported on code.google.com by darren%b...@gtempaccount.com on 18 Jun 2009 at 10:33

[GoogleCodeExporter]

I believe you're right, ImmutableSetMultimap.Builder.put() is inconsistent.

ImmutableSet.of(a, a) silently collapses duplicates, and 
ImmutableSet.Builder.add() 
ignores them as well.

The reason ImmutableMap.put() throws on a duplicate key is that it's much more 
likely 
to indicate a probable bug -- in theory, we would only throw if you are 
actually 
reassigning the key to a different (not equals()) value, but in practice we 
handled 
it more simply/conservatively.

The ImmutableSetMultimap case is analogous to ImmutableSet; puts should 
logically be 
idempotent.

Original comment by kevin...@gmail.com on 1 Jul 2009 at 5:37

• Changed state: Accepted

[GoogleCodeExporter]

There is a decent argument for rejecting duplicates all around:

I write this:

static final ImmutableSet<Project> PUBLIC_PROJECTS = ImmutableSet.of(
    GMAIL,
    WAVE,
    PROJECT_CONQUER_THE_WORLD,
    COLLECTIONS,
    PAGE_SPEED,
    PROJECT_CONQUER_THE_WORLD); // harmless duplicate

Oh, no!  We haven't announced Project Conquer the World!  No matter:

static final ImmutableSet<Project> PUBLIC_PROJECTS = ImmutableSet.of(
    GMAIL,
    WAVE,
    // PROJECT_CONQUER_THE_WORLD,
    COLLECTIONS,
    PAGE_SPEED,
    PROJECT_CONQUER_THE_WORLD);

Whew!  We're safe.

Original comment by cpov...@google.com on 1 Jul 2009 at 8:58

[GoogleCodeExporter]

Yes, there are a few bug-prone situations like that.  Another is "did I get all 
50 
states here? let me count the lines... yep, 50!"

We did take this into account back when we first created these, and while I 
can't 
recollect the entire rationale, we decided on what you see now.  Part of the 
problem 
is that it's perfectly legitimate to use 
ImmutableSet.copyOf(listThatMightHaveDups) 
to de-dup a list, and it would be incredibly weird for of and copyOf to act 
differently. 

I suppose we could still consider some kind of option like 
ImmutableSet.Builder.setStrict().  Dunno how I feel about that.

Original comment by kevin...@gmail.com on 1 Jul 2009 at 9:04

[GoogleCodeExporter]

A typical use case I have is sharing an ImmutableMap instance many times, and 
occasionally creating a new immutable map which is a copy of the old one with 
the value 
for 1 key changed.

The current throwing behaviour makes this somewhat un-elegant.

Original comment by jvdne...@gmail.com on 2 Jul 2009 at 7:42

[GoogleCodeExporter]

Fixed for RC3!  Thanks.

Original comment by kevin...@gmail.com on 12 Aug 2009 at 12:10

• Changed state: Fixed