UnmodifiableMultiset.removeIf() should throw UnsupportedOperationException

[etellman]

UnmodifiableMultiset.removeIf() should throw UnsupportedOperationException, even if nothing would be removed. This would make it match the other mutating methods in UnmodifiableMultiset.

Currently, if the predicate doesn't accept any of the elements in the set, removeIf succeeds and does nothing. The set isn't modified, but it's surprising that removeIf doesn't throw UnsupportedOperationException in this case.

I'll attach a PR which will fix this, assuming you agree it should be changed.

[perceptron8]

IMO it's somewhat similar to #3909.

[etellman]

Thanks for the very quick response. You're right, it is the same idea as #3909. As @kevinb9n said in that one:

With that said, I do think that the behavior you're asking for is slightly better. It's better to always fail on a "structurally" invalid request even though it "just happened to be harmless" in these particular circumstances. Not doing so can create a "time bomb" in the code.

That was my thought for this one as well.

[etellman]

Here's a simple PR to fix it, if desired: #6703

[eamonnmcmanus]

The downside, of course, is that this may break code that was implicitly relying on the current behaviour. Even if it might have been better to throw from the outset, it's less clear that changing it now is worth the small risk of breakage.

[etellman]

Makes sense.

For context, this came up as part of adding an isImmutable() assertion to AssertJ. If a collection throws UnsupportedOperationException for all potentially mutating operations, it's considered immutable. This works for any of the Guava collections that extend ImmutableCollection and almost works for UnmodifiableMultiset, except for removeIf() which always succeeds when the set is empty.

AssertJ also keeps a list of known immutable collection types, to handle cases like this. If you decide to keep the current behavior, we can add UnmodifiableMultiset to this list, since it actually is unmodifiable.

[eamonnmcmanus]

I tried running all of Google's tests (from nearly all of Google's products and services) with an equivalent change in place, and there were no failures. There are actually only a few dozen uses of Multisets.unmodifiableMultiset in our millions of source files. So I think it would actually be OK to make this change. In the very unlikely event that anyone is broken by it, they were already skating on the edge of brokenness anyway.

[etellman]

Sounds good. Thanks!