GROOVY-7774: Collection addAll fails CompileStatic type checking when… #376
Conversation
… adding a collection of subtypes
|
+1 |
|
I added in some Map methods which suffered the same problem |
| final Map<G, Map<K, V>> answer = new LinkedHashMap<G, Map<K, V>>(); | ||
| for (Map.Entry<G, List<Map.Entry<K, V>>> outer : initial.entrySet()) { | ||
| public static <G, K, V> Map<G, Map<? extends K, ? extends V>> groupBy( | ||
| Map<K, V> self, @ClosureParams(MapEntryOrKeyValue.class) Closure<G> closure) { |
There was a problem hiding this comment.
Should the wildcards be on the self parameter instead? Just not used to seeing them on return types, but nested wildcards often confuse me 😄
Would the closure parameter benefit from having ? extends G?
There was a problem hiding this comment.
Nevermind my comments on wildcards on self and closure params, the more I think about it that doesn't make sense.
However, wont changing the return type to have wildcards extends mean that nothing can be put in the map that is mapped to the groupBy key which some may rely on being able to do?
There was a problem hiding this comment.
It isn't exactly what I would like but groupBy calls putAll and the other changes are to make Java happy. We might have to do some casting and suppress some warnings instead.
There was a problem hiding this comment.
we should use the following guideline: no wildcards on self or returns, only on parameters we use as input. In mutating calls, even the (in/)out parameters should not have wildcards. Because wildcards make it very very difficult to use the container for any adding.
… adding a collection of subtypes (same treatment for some Map methods)
|
I removed the groupBy changes and inlined a few lines instead. |
| @@ -5176,7 +5176,9 @@ public static Map groupBy(Object[] self, List<Closure> closures) { | |||
| G key = outer.getKey(); | |||
| List<Map.Entry<K, V>> entries = outer.getValue(); | |||
| Map<K, V> target = createSimilarMap(self); | |||
| putAll(target, entries); | |||
There was a problem hiding this comment.
you removed the putAll here because of generics?
There was a problem hiding this comment.
Yes, but feel free to play around with other options if you have a strong preference.
There was a problem hiding this comment.
my suggestion would be to change putAll to public static <K, V> Map<K, V> putAll(Map<K, V> self, Collection<? extends Map.Entry<? extends K, ? extends V>> entries)
which more or less adds ? extends before the Map.Entry part and to change back to use the putAll instead of the loop
There was a problem hiding this comment.
Nice suggestion. Done.
… adding a collection of subtypes (same treatment for some Map methods) (closes #376)
… adding a collection of subtypes