[
https://issues.apache.org/jira/browse/GROOVY-12585?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18136321#comment-18136321
]
ASF GitHub Bot commented on GROOVY-12585:
-----------------------------------------
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.
> Casting a collection to SortedSet, Queue, Deque and similar interfaces fails,
> or with "as" gives a broken proxy
> ---------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12585
> URL: https://issues.apache.org/jira/browse/GROOVY-12585
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Priority: Major
> Fix For: 7.x
>
>
> h3. Problem
> Groovy turns a collection into another collection type when it is assigned or
> cast ({{T x = value}}, {{(T) value}}, through
> {{DefaultTypeTransformation.castToType}}), and when it is coerced with {{as}}
> (through {{DefaultGroovyMethods.asType(Collection, Class)}}). The two paths
> know different default implementations, and neither covers the common
> collection interfaces. So:
> * some assignments fail where the equivalent {{as}} works;
> * some {{as}} coercions return a proxy that does not behave like the
> requested type;
> * an array source and a collection source give different results for the same
> target;
> * a single-element list assigned to some collection classes gives an empty
> collection.
> Results below are the same with indy and classic call sites, on JDK 17 and
> 21. Groups 1 and 2 fail the same way in every release checked, from 2.4.12 to
> 6.0.0.
> h4. 1. Assignment fails where {{as}} works
> ||Target||{{T x = [3, 1, 2, 1]}}||{{[3, 1, 2, 1] as T}}||
> |{{SortedSet}}|{{GroovyCastException}}|{{TreeSet [1, 2, 3]}}|
> |{{Queue}}|{{GroovyCastException}}|{{LinkedList [3, 1, 2, 1]}}|
> |{{Stack}}|{{GroovyCastException}}|{{Stack [3, 1, 2, 1]}}|
> |{{List}} (source {{[3, 1, 2] as
> LinkedHashSet}})|{{GroovyCastException}}|{{ArrayList [3, 1, 2]}}|
> {{asType}} has a table of defaults for {{List}}, {{Set}}, {{SortedSet}},
> {{Queue}} and {{Stack}}. {{castToType}} has none for a {{Collection}} source:
> it gives a {{LinkedHashSet}} when the target accepts one. Otherwise it tries
> the target's constructor with the elements, then with the collection, which
> fails for an interface and for a class without a copy constructor such as
> {{Stack}}.
> An array source goes through a different branch: a no-arg constructor plus
> {{addAll}}. So {{Stack s = new Integer[]\{3, 1, 2\}}} works while {{Stack s =
> [3, 1, 2]}} fails.
> h4. 2. Assignment fails, and {{as}} returns a proxy that does not behave like
> the type
> ||Target||{{T x = value}}||{{value as T}}||
> |{{NavigableSet}}|{{GroovyCastException}}|proxy over the list: {{([3, 1, 2,
> 1] as NavigableSet).size() == 4}}, {{first() == 3}}|
> |{{Deque}}|{{GroovyCastException}}|proxy over the list: {{peekFirst()}}
> throws {{MissingMethodException}}|
> |{{BlockingQueue}}|{{GroovyCastException}}|proxy over the list|
> |{{SortedMap}}, {{NavigableMap}}|{{GroovyCastException}}|dynamic proxy over
> the map: {{firstKey()}} throws {{UnsupportedOperationException}}|
> |{{ConcurrentMap}}|{{GroovyCastException}}|dynamic proxy over the map|
> |{{Iterable}} (array source)|{{GroovyCastException}}|proxy over the array|
> h4. 3. Array and collection sources give different result types
> ||Expression||Result||
> |{{Set s = new Integer[]\{3, 1, 2\}}}|{{LinkedHashSet [3, 1, 2]}}|
> |{{new Integer[]\{3, 1, 2\} as Set}}|{{HashSet [1, 2, 3]}} (order lost)|
> |{{[3, 1, 2] as Set}}|{{LinkedHashSet [3, 1, 2]}}|
> |{{HashSet h = [3, 1, 2]}}|{{LinkedHashSet [3, 1, 2]}}|
> |{{[3, 1, 2] as HashSet}}|{{HashSet [1, 2, 3]}}|
> h4. 4. A single element is passed to a capacity constructor
> Because {{castToType}} tries the target's constructor with the elements
> first, a one-element list picks a capacity constructor:
> ||Expression||Result||
> |{{Vector v = [5]}}|empty {{Vector}} (capacity 5)|
> |{{PriorityQueue q = [5]}}|empty {{PriorityQueue}}|
> |{{ArrayDeque d = [5]}}|empty {{ArrayDeque}}|
> |{{[5] as Vector}}|{{Vector [5]}}|
> h3. Already working (no change intended)
> {{Collection}}, {{Iterable}} (collection source), {{Set}}, {{AbstractSet}},
> {{List}} (list or array source), {{HashSet}}, {{LinkedHashSet}}, {{TreeSet}},
> {{ArrayList}}, {{LinkedList}}, {{Vector}} (more than one element),
> {{ArrayDeque}}, {{PriorityQueue}}, {{ConcurrentLinkedQueue}},
> {{CopyOnWriteArrayList}}, {{Map}}, {{HashMap}}, {{LinkedHashMap}},
> {{AbstractMap}}, {{TreeMap}}, {{ConcurrentHashMap}},
> {{ConcurrentSkipListMap}}. On JDK 21, also {{SequencedCollection}},
> {{SequencedSet}} and {{SequencedMap}}.
> h3. Proposal
> One table of default implementations, used by both {{castToType}} and
> {{asType}}. A value that is already an instance of the target is returned
> unchanged, as now.
> ||Target||Default||
> |{{List}}, {{Collection}}, {{Iterable}}|{{ArrayList}}|
> |{{Set}}|{{LinkedHashSet}}|
> |{{SortedSet}}, {{NavigableSet}}|{{TreeSet}}|
> |{{Queue}}, {{Deque}}|{{LinkedList}} (accepts {{null}} elements, unlike
> {{ArrayDeque}}; matches today's {{as Queue}})|
> |{{BlockingQueue}}|{{LinkedBlockingQueue}}|
> |{{SortedMap}}, {{NavigableMap}}|{{TreeMap}}|
> |{{ConcurrentMap}}|{{ConcurrentHashMap}}|
> For a concrete collection class, assignment and cast try its copy
> constructor, then its no-arg constructor followed by {{addAll}}, and only
> then the elements as constructor arguments. This is the order {{as}} already
> uses, and it fixes group 4. As with {{as}} today (GROOVY-7867), a constructor
> that throws is skipped in favour of the next step, and its exception is kept
> as a suppressed exception if every step fails.
> {{as}} with a concrete class keeps its current path. So the open question is
> answered by keeping today's behaviour: {{HashSet h = list}} still gives a
> {{LinkedHashSet}} (a subclass, so order is kept), and {{list as HashSet}}
> still gives a {{HashSet}}.
> h3. Behaviour changes
> * Assignments in groups 1 and 2 that now fail would succeed.
> * {{as NavigableSet}}, {{as Deque}}, {{as BlockingQueue}}, {{as SortedMap}},
> {{as NavigableMap}} and {{as ConcurrentMap}} would return a real collection
> instead of a proxy, as would {{as Iterable}} and {{as Deque}} with an array
> source. Code relying on the proxy keeping a reference to the original
> collection would see a copy.
> * {{array as Set}} would keep element order ({{LinkedHashSet}} instead of
> {{HashSet}}).
> * A single-element list assigned to a collection class would hold the element
> instead of giving an empty collection (group 4).
> * {{TreeSet}}/{{TreeMap}} targets throw {{ClassCastException}} or
> {{NullPointerException}} for non-{{Comparable}} or {{null}} elements and
> keys, as {{as SortedSet}} and {{TreeSet x = ...}} already do.
> * A map coerced with {{as}} to an interface outside the table, such as
> {{Runnable}} or {{Comparator}}, is still a proxy over the map's closures.
> h3. Out of scope
> The type checker has its own rules for list and map literals, and they are to
> be raised in their own issues:
> * GROOVY-12643: under {{@TypeChecked}} and {{@CompileStatic}}, a literal
> assigned to the interfaces in the table is a compile error, for example
> {{SortedSet s = [3, 1]}}. A literal assigned to a concrete collection class
> is matched against a constructor taking the elements, so {{LinkedList l = [3,
> 4, 5]}} fails with "Cannot find matching constructor
> java.util.LinkedList(int, int, int)". Under {{@CompileStatic}}, {{Vector v =
> [5]}} calls {{Vector(int)}} and stays empty. Until that issue is fixed,
> {{Vector v = [5]}} gives {{[5]}} in dynamic and {{@TypeChecked}} code but
> {{[]}} under {{@CompileStatic}}.
> * GROOVY-12644: under static checking, a literal returned from a method is
> rejected unless its own class is assignable to the return type. {{Set m() \{
> [3, 1] \}}} and {{TreeMap m() \{ [b: 2, a: 1] \}}} fail, although the same
> literal assigned to a {{Set}} or {{TreeMap}} variable compiles.
> * A list or map argument is not converted to a method's parameter type, in
> any mode, by design.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
