On Wed, 24 Feb 2021 01:58:48 GMT, Stuart Marks <sma...@openjdk.org> wrote:
>>> Is there any behavior change here that merits a CSR review? >> >> Yes. See my comments in the bug report: >> >> https://bugs.openjdk.java.net/browse/JDK-6323374?focusedCommentId=14296330&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14296330 >> >> There is not only the issue of the identity of the object returned, but the >> change is also observable in the serialized form. Most people would consider >> the change (less nesting) to be an improvement, but the change is >> observable, and as we know any observable behavior can become depended upon >> by applications. > > Code changes all look good. I'm thinking that we should add `@implNote` > clauses to all the docs of the affected methods, saying something like "This > method may return its argument if it is already unmodifiable." Usually it's > reasonable to leave these kinds of behaviors unspecified (and we do so > elsewhere) but since this is a change in long-standing behavior, it seems > reasonable to highlight it explicitly. I don't think we want to specify it > though, because of the issue with ImmutableCollections (as discussed > previously) and possible future tuning of behavior regarding the various Set > and Map subinterfaces. (For example, C.unmodifiableSet(arg) could return arg > if it's an UnmodifiableNavigableSet.) > > The test seems to have a lot of uncomfortable dependencies, both explicit and > implicit, on the various ImmutableCollection and UnmodifiableX implementation > classes. Would it be sufficient to test various instances for reference > equality and inequality instead? For example, something like > > var list0 = List.of(); > var list1 = Collections.unmodifiableList(list0); > var list2 = Collections.unmodifiableList(list1); > assertNotSame(list0, list1); > assertSame(list1, list2); > > This would avoid having to write test cases that cover various internal > classes. The ImmutableCollections classes have been reorganized in the past, > and while we don't have any plans to do so again at the moment, there is > always the possibility of it happening again. > > One could write out all the different cases "by hand" but there are rather a > lot of them. It might be fruitful to extract the "wrap once, wrap again, > assertNotSame, assertSame" logic into a generic test and drive it somehow > with a data provider that provides the base instance and a wrapper function. Per @stuart-marks I rewrote the tests using some of his suggestions, which substantially reduced dependencies and test size. ------------- PR: https://git.openjdk.java.net/jdk/pull/2596
