chen870647924 / guava-libraries

Automatically exported from code.google.com/p/guava-libraries
Apache License 2.0
0 stars 0 forks source link

Set returned by ImmutableSet.of() does not behave the same as Collections.emptySet() #866

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Run the following:

Set<Object> a = Collections.emptySet();
a.remove("a");
Set<Object> b = ImmutableSet.of();
b.remove("b");

b.remove() will throw an UnsupportedOperationException.

I assume (though haven't checked) that ImmutableList and ImmutableMap might 
have the same problems?

I don't have any massive issues with this behaviour (it's more 'fail-fast' i 
suppose), but the comment that ImmutableSet.of() behaves comparably to 
Collections.emptySet() is somewhat misleading...it's not a drop in replacement.

Original issue reported on code.google.com by dnd1...@gmail.com on 12 Jan 2012 at 2:26

GoogleCodeExporter commented 9 years ago
It is not well-defined which exception is more correct here.

Do we have really have text that implies *that* exact of an equivalence between 
the two?

Original comment by kevinb@google.com on 13 Jan 2012 at 5:51

GoogleCodeExporter commented 9 years ago
"This set behaves and performs comparably to Collections.emptySet(), and is 
preferable mainly for consistency and maintainability of your code."

TBH, I think this is fine as-is?

Original comment by wasserman.louis on 16 Jan 2012 at 1:11

GoogleCodeExporter commented 9 years ago
IMHO, all Immutable* collections should throw UOE on #remove...having different 
behavior for an empty immutable collection would be confusing.

Original comment by kurt.kluever on 16 Jan 2012 at 7:01

GoogleCodeExporter commented 9 years ago
Just for reference, the Collection documentation on throwing UOE:

"The 'destructive' methods contained in this interface, that is, the methods 
that modify the collection on which they operate, are specified to throw 
UnsupportedOperationException if this collection does not support the 
operation. If this is the case, these methods may, but are not required to, 
throw an UnsupportedOperationException if the invocation would have no effect 
on the collection. For example, invoking the addAll(Collection) method on an 
unmodifiable collection may, but is not required to, throw the exception if the 
collection to be added is empty."

Original comment by wasserman.louis on 16 Jan 2012 at 6:56

GoogleCodeExporter commented 9 years ago
@louis

i respectfully disagree.  ok, it doesn't say 'behaves identically', but it's a 
bit of a nothing statement in that case: 'it sort of behaves like 
Collections.emptySet(), except that it throws exceptions sometimes.'  it's not 
preferable if it breaks your code!

i don't think the current behaviour is inconsistent with the Collection 
documentation, but neither is Collections.emptySet().  My personal preference 
is that it behave identically to Collections.emptySet() and be a true drop-in 
replacement, but if not i think the documentation should be more explicit about 
what the differences in behaviour are.

Original comment by dnd1...@gmail.com on 18 Jan 2012 at 7:59

GoogleCodeExporter commented 9 years ago
I have to rule out Option 1 completely.  ImmutableSet.of() has to behave 
consistently with nonempty ImmutableSets.

I do have to say, though: if it breaks your code, you're _already_ doing 
something wrong.

Original comment by wasserman.louis on 18 Jan 2012 at 6:39

GoogleCodeExporter commented 9 years ago
Re Option 1, I can see your point, I have no problem with leaving the behaviour 
as-is.  Nevertheless, I believe it should be documented that the behaviour is 
different to Collections.emptySet().  The (incorrect) impression I got was that 
it was a drop-in replacement for Collections.emptySet(), and it is not.

Original comment by dnd1...@gmail.com on 18 Jan 2012 at 7:57

GoogleCodeExporter commented 9 years ago
We feel this is covered by the statement "behaves *comparably*" as opposed to 
"behaves exactly as", and don't feel it's worth spending extra text on this 
minor issue.

Original comment by kevinb@google.com on 30 Jan 2012 at 6:12

GoogleCodeExporter commented 9 years ago
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:14

GoogleCodeExporter commented 9 years ago

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