On Tue, 23 Feb 2021 23:32:01 GMT, Stuart Marks <sma...@openjdk.org> wrote:
>>> > Is there any behavior change here that merits a CSR review? >>> >>> Maybe. The one observable change is that calling `Collections.bar(foo)` >>> with a `foo` that is already a `bar` will return the instance rather than >>> unnecessarily wrap it. This could change semantics in applications >>> inadvertently or deliberately relying on identity. >> >> Yes. The CSR was to consider primarily this case. Probably out of an >> abundance of caution here. @stuart-marks may have another case to consider. > >> 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. ------------- PR: https://git.openjdk.java.net/jdk/pull/2596
