paulk-asert opened a new pull request, #3165: URL: https://github.com/apache/groovy/pull/3165
## Summary Make collection and map conversions consistent between dynamic code, `@TypeChecked` and `@CompileStatic`. Three commits, one per JIRA, each with its own tests and COMPATIBILITY entry: | Commit | Issue | Scope | |---|---|---| | 1 | [GROOVY-12585](https://issues.apache.org/jira/browse/GROOVY-12585) | Runtime: one table of default implementations for casting and `as` | | 2 | [GROOVY-12643](https://issues.apache.org/jira/browse/GROOVY-12643) | Type checker: list and map literals assigned to collection types | | 3 | [GROOVY-12644](https://issues.apache.org/jira/browse/GROOVY-12644) | Type checker: list and map literals returned from methods | ### 1. GROOVY-12585: runtime Assignment and cast (`castToType`) and coercion with `as` (`asType`) now share one table of default implementations: | Target | Default | |---|---| | `List`, `Collection`, `Iterable` | `ArrayList` | | `Set` | `LinkedHashSet` | | `SortedSet`, `NavigableSet` | `TreeSet` | | `Queue`, `Deque` | `LinkedList` | | `BlockingQueue` | `LinkedBlockingQueue` | | `SortedMap`, `NavigableMap` | `TreeMap` | | `ConcurrentMap` | `ConcurrentHashMap` | - `SortedSet s = list`, `Deque d = list`, `Stack s = list`, `List l = aSet`, `SortedMap m = map` and `Iterable i = anArray` now succeed; they threw `GroovyCastException`. - `as NavigableSet`, `as Deque`, `as BlockingQueue`, `as SortedMap`, `as NavigableMap` and `as ConcurrentMap` return a real collection (a copy) instead of a proxy that did not behave like the type. - `anArray as Set` keeps element order (`LinkedHashSet`, not `HashSet`). - On assignment, a concrete collection class tries its copy constructor, then its no-arg constructor plus `addAll`, before passing the elements as constructor arguments. So `Vector v = [5]`, `PriorityQueue q = [5]` and `ArrayDeque d = [5]` hold the element instead of being empty with that capacity. - Unchanged: `as` with a concrete class keeps its own path (`list as HashSet` is a `HashSet`; `HashSet h = list` is still a `LinkedHashSet`). The GROOVY-7867 fallback order and its suppressed exception are also unchanged. ### 2. GROOVY-12643: type checker, assigned literals - A list literal assigned to `SortedSet`, `NavigableSet`, `Queue`, `Deque` or `BlockingQueue`, and a map literal assigned to `SortedMap`, `NavigableMap` or `ConcurrentMap`, now compile, as `Set` already did. These have been compile errors since at least 2.4. The inferred type is the default implementation, and the elements are checked against the declared type arguments. - A list literal assigned to a concrete collection class with a collection or no-arg constructor copies the elements instead of passing them as constructor arguments. So `LinkedList l = [3, 1]` and `Stack s = [3, 1]` compile, and under `@CompileStatic`, `Vector v = [5]` holds the element, as in dynamic code. Other classes keep the list-constructor rule (`Point p = [1, 2]`, `Dimension d = [100, 200]`). - `@CompileStatic` needs no new code generation: the existing indy cast already goes through `castToType`. ### 3. GROOVY-12644: type checker, returned literals A list or map literal returned from a method was accepted only if the literal's own class was assignable to the return type, so `Set m() { [3, 1] }` failed although `Set s = [3, 1]` compiles. A returned literal is now checked as `ReturnType x = literal` is. This only accepts code that did not compile before. Array return types keep their existing check. Closure and lambda results are unchanged, because they are not converted to the expected type at run time. ## Compatibility Most of this only turns failures into results. The changes in behaviour that reviewers should look at: - **Proxies become copies:** `as` for the interfaces above returns a copy instead of a proxy over the original collection or map. - **Element order:** `anArray as Set` keeps it. - **The capacity idiom (commit 2), the main one to decide:** statically compiled code that used a one-element literal as a capacity changes meaning. `Vector v = [10]` now holds `10`, and `ArrayDeque<String> d = [16]` is a compile error; write `new ArrayDeque(16)` for a capacity. `ArraysAndCollectionsSTCTest` pinned `ArrayDeque<String> deque = [123] // ArrayDeque(int numElements)` from GROOVY-10002, and it now asserts the new behaviour. @eric-milles, does that look right to you? Each change has an entry in `COMPATIBILITY.md`. The list and map constructor section of `core-semantics.adoc` is updated. ## Tests - **New:** `bugs.Groovy12585`, `bugs.Groovy12643` and `bugs.Groovy12644`, 10 tests each. They cover dynamic code, `@TypeChecked` and `@CompileStatic`, generics, and negative and unchanged-behaviour cases. Each fails without its commit's production change, except the guards for unchanged behaviour. - **Updated:** `ConstructorsSTCTest.testMapLiteral` (`SortedMap m = [:]`), and `ArraysAndCollectionsSTCTest` (the `Deque`/`Queue` literal rejections and the capacity idiom). - The full `:test` suite passes on each commit. ## Related - #3111 (GROOVY-12584) removes the indy cast selector's collection fast path, so every indy collection cast goes through `castToType`. It is independent of this PR and can land in either order. It closes the one indy-only case where assignment and `as` still differ: an abstract subclass of `HashSet` or `ArrayList`. - Out of scope: a list or map argument is not converted to a method's parameter type, in any mode, by design. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
