isUnmodifiable assertion for Iterable instances

[scordio]

Feature summary

Similarly to #2102, we could have an isUnmodifiable() assertion for Iterable types.

We might consider moving AbstractCollectionAssert::isUnmodifiable to AbstractIterableAssert, as long as it's binary compatible. Otherwise, we might move the inner implementation and keep the existing public method, to be removed in version 4.

Example

Iterable<Integer> iterable = Collections.unmodifiableList(Arrays.asList(1, 2, 3));

assertThat(iterable).isUnmodifiable();

[etellman]

I can take a look at this one, if nobody else is yet.

[scordio]

Sure, go for it, @etellman!

[etellman]

I think I got a little carried away with tests and moving things around, but here's a PR: #3160

Since AbstractIterableAssert is already 4000 lines long and there's some overlap with AbstractMapAssert, I extracted mutating method detection into a new MutatingMethodFinder class. MutatingMethodFinder looks for a mutating method, and returns it in an Optional if it finds one. The assertion classes throw the appropriate exception if there is a mutating method. This also simplifies testing, since MutatingMethodFinder can be tested separately.

I added tests that make all of the methods but one unsupported and verify that the class is still identified as mutable. All the common mutable collections implement add() so just testing with real collections like ArrayList, never gets past the add() check.

The only slight change to the behavior is that I treated throwing a documented exceptions other than UnsupportedOperationException as meaning the method is implemented. If your collection doesn't allow null, it's expected that trying to add null to it will throw a NPE, and this just means the method is supported. The other case where this comes up is trying to set index 0 in an empty list. If the method is supported, this will throw and IndexOutOfBoundsException. If a method throws a RuntimeException for something other than these two cases, MutatingMethodFinder passes it on to the caller to decide what to do. Let me know what you think about this.

Since the concrete classes know what kind of collection they have, I pushed this decision down to the concrete classes. If an assertion class knows that it has a list, it doesn't need to check whether actual is an instance of a list. This is avoids a little unnecessary runtime type checking.

This also forces the author of a concrete assertion class to think about what type of collection the assertion assertions are for and perhaps write a test. I added a test for checking whether unmodifiable Guava Multisets are really unmodifiabele and found, I think, a bug. I didn't get the expected UnsupportedOperationException for removeIf() unless it finds something to remove. I'll file an issue with the Guava project.

A disadvantage of this approach is that the concrete classes have to think about what kind of collection they have. An alternative would be to get rid of the visitor idea and have AbstractIterableAssert always call MutatingMethodFinder.visitCollection() (probably renamed). Let me know if you prefer this approach. With this way of doing it, the only public methods in MutatingMethodFinder might be something like checkCollection and checkMap.

It looks like there's a binary compatibility issue. I ran mvnw verify but apparently this doesn't run the binary compatibility check. This probably means isUnmodifiable needs to live in both AbstractCollectionAssert and AbstractIterableAssert for now. I'll make this change and see if the error goes away.

[etellman]

Here's issue in google/guava for changing the behavior of UnmodifiableMultiset: google/guava#6702

It looks like it's technically OK to either throw UnsupportedOperationException or to do nothing if nothing changes in the collection. But the consensus seems to be that it's less confusing to always throw the exception, even if the collection wouldn't have changed, based on the arguments. Then you know right away that that call will never work.

[scordio]

Hey @etellman, thanks a lot for your effort!

I haven't managed to look at your changes yet but I'll do it as soon as I can.

[etellman]

No problem.

google/guava#6702 has generated a surprisingly high amount of discussion with the Guava people. I had assumed it would be uncontroversial, but they're a bit reluctant to change anything, for fear of somebody counting on removeIf() silently doing nothing if there's nothing to be removed. There's a related issue in Guava for a different collection type, also still open.

[etellman]

The Guava people approved google/guava#6702. It turns out that this class is rarely used and they're confident either way will be fine.

I thought about this a bit more, however, and the unmodifiability checks so could be changed so it wouldn't matter....

The checks now are trying to verify that every potentially mutating method can be made to throw an UnsupportedOperationException. This isn't necessary if the test just trying to determine if a particular collection instance will always have the same contents it has when the test is run. If a collection is empty and you can't add anything to it, it doesn't matter what methods like clear() and sort() do.

For example, it makes sense for a class that can only represent an empty list to implement sort() as a no-op, since an empty list is already sorted. And it's valid to have this class throw UnsupportedOperationException for add but have a no-op for things like sort() and clear().

The distinction is whether "is unmodifiable" means "this particular instance will never change" or "any instance of the class that this is an instance of will never change". The second thing isn't possible to automatically determine, which is why there is a need for lists of pre-approved collection types now.

I did a little POC of this approach, and it seems to work. I can open a new issue/PR to discuss it, however, as this one is already pretty large.

[etellman]

Since nobody has looked at this change yet anyway, I just added the above idea to the existing PR (#3160). This means that the special cases for known unmodifiable classes aren't needed.

Sorry, this change is pretty big and more than the originally-requested "move a method to a different class". Much of it is new tests, however, and not needing lists of pre-approved known unmodifiable classes seems easier to maintain and more general.

I'm going to stop messing with this now---feel free to do what you want with it.