Skip to content

Conversation

Andremoniy
Copy link
Contributor

I have fully refactored the code, added new checks and addressed issues related to the sealed java.* classes in JDK 17

andremoniy2 added 4 commits February 14, 2022 00:42
…o Issue249-jdk17

� Conflicts:
�	hamcrest/src/main/java/org/hamcrest/collection/IsUnmodifiableCollection.java
�	hamcrest/src/test/java/org/hamcrest/collection/IsUnmodifiableCollectionTest.java
@rmcdouga
Copy link

rmcdouga commented Sep 1, 2025

I've reviewed this PR and I have some comments.

First a couple of small points:

  1. There are no JavaDocs
  2. Does not handle Collections.emptyList() and Collections.emptySet() (fails to recognize these as unmodifiable)

These two things are easy to fix.

Now I am getting into more of a design discussion. It seems there's a lot of reflection code around creating a new instance of a non-JDK class that implements Collection. The code seems fragile as it needs to discover how to create a new instance of a class and populate it. It may need to bypass the access specified by the class designer. It does not (and cannot) understand static factories.

I am wondering if all this code and fragility is really worth it? Could we rename the method isModifiableJdkCollection() and just check against known JDK unmodifiable collections? Do we really need to support testing of custom Collection implementations?

If we do need to support custom Collection implementations, then could we just try modifying the existing collection rather thn trying to create a new instance (and note the behaviour in the JavaDocs)? This would let us avoid the reflection code.

I'm worried that as more "Integrity by Default" JEPs get implemented, this code is going to start to require command line args for it to continue functioning and I'm not sure if the use case of testing a custom Collection implementation is really worth the weight of the reflection code it requires.

Thoughts?

@rmcdouga rmcdouga mentioned this pull request Sep 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants