paulk-asert commented on PR #3154:
URL: https://github.com/apache/groovy/pull/3154#issuecomment-6646349211
### What this PR achieves
With this PR, under `@TypeChecked` and `@CompileStatic`, a value written to
a `Map<K,V>` is converted to `V` whenever `V` is a concrete type. The rules are
the same ones an assignment uses, so `String s = "$n"` gives a String and `Long
l = 1` gives a Long. The conversion goes through `putAt(Map<K,V>, K, Object,
@ClassTag(preempt=true) Class<V>)`, with the compiler supplying the token. That
is the one sanctioned way for statically checked code to behave differently
from dynamic code, and `classTagPreemptionDisabled` turns it off. Dynamic code
is unchanged.
In practice:
- **Every write form behaves the same.** Before, `m.k = "$n"` and `m['k'] =
"$n"` gave different results. 5.1.3 stored the GString for both. 6.0.0 stored
it for the property write and rejected the subscript write. Master converts the
property write and still rejects the subscript. Now all of these convert:
- property writes, including inside `with { }` and with `?.`;
- subscript writes and `putAt`;
- `?=` and `??=`;
- nested maps;
- non-String keys (Integer, enum).
- **Writes rejected since 6.0.0 compile again.** They now store a value of
the declared type, rather than the wrong type 5.1.3 stored.
- **Conversion isn't limited to GString.** It applies wherever assignment
would convert: `Map<String,Long>`: `m.a = 1` stores a Long,
`Map<String,BigDecimal>`: `m.a = 1` stores a BigDecimal, and
`Map<String,String>`: `m.k = 123` stores `"123"`.
- **The value read back now matches the declared type.** A leftover GString
in a `Map<String,String>` gives false from `equals`, `containsValue` and hash
lookups against a String. It also throws ClassCastException in Java code
reading the map, and in 6.0.0 `@CompileStatic` String method calls.
Unchanged, by design:
- **Map methods:** `put`, `putAll`, `merge`, `<<`, map literals and the like
are still checked like any other call, so a GString argument is still a compile
error.
- **Value types with no single target:** wildcards, raw `Map`, `Object` and
`CharSequence` get no `@ClassTag` token, so their values are stored unchanged.
Two differences from master, both deliberate:
- master converts a property write to a `? extends String` map, and this PR
doesn't, because no token is supplied;
- master ignores `classTagPreemptionDisabled`, and this PR honours it.
Master's conversion predates the `@ClassTag` approach.
### What a write stores under static type checking
Each cell is the class of the value stored, with `def n = 'ab'`, under
`@TypeChecked` and `@CompileStatic`. The two modes agree in every cell except
where a cell shows *TC / CS*. ❌ is a compile error. Dynamic code stores the
value unchanged in every version, so it is shown once.
- † marks rows that `Groovy12535` didn't cover before this PR.
- ‡ marks writes that type check but still store a value that isn't of the
declared type. These are outside this PR; see the end of this comment.
| Write |
Dynamic (all) | 5.1.3 | 6.0.0 |
master | this PR |
| ------------------------------------------------------------------------ |
------------- | ----------------------------- | ----------------------------- |
------- | ---------- |
| **`Map<String,String>`, property writes** |
| | |
| |
| `m.k = "$n"` |
GString | GString | GString |
String | String |
| `m.with { k = "$n" }` |
GString | GString | GString |
String | String |
| `m.with { it.k = "$n" }` |
GString | GString | GString |
String | String |
| `m?.k = "$n"` |
GString | GString | GString |
String | String |
| `m.a = m.b = "$n"` |
GString | GString | GString |
String | String |
| `m.k = flag ? "$n" : 'cd'` |
GString | GString | GString |
String | String |
| `GString g = "$n"; m.k = g` |
GString | GString | GString |
String | String |
| `def v = (m.k = "$n")`: stored, `v` |
GString | GString, `v` GString / String | GString, `v` GString / String |
String | String |
| `this.a = "$n"` in a `HashMap<String,String>` subclass |
GString | GString | GString |
String | String |
| `m.k = a` where `def (a, b) = ["$n", "$n"]` † |
GString | GString | GString |
String | String |
| static field `fm.k = "$n"` † |
GString | GString | GString |
String | String |
| closure parameter `m.k = "$n"` † |
GString | GString | GString |
String | String |
| `TreeMap<String,String>`: `m.k = "$n"` † |
GString | GString | GString |
String | String |
| nested `mm.x.k = "$n"` (`Map<String,Map<String,String>>`) † |
GString | GString | GString |
String | String |
| **`Map<String,String>`, subscript and compound writes** |
| | |
| |
| `m['k'] = "$n"` |
GString | GString | ❌ |
❌ | String |
| nested `mm['x']['k'] = "$n"` † |
GString | GString | ❌ |
❌ | String |
| `m.k ?= "$n"` |
GString | GString | GString |
GString | String |
| `m['k'] ?= "$n"` † |
GString | GString | ❌ |
❌ | String |
| `m.k ??= "$n"` † |
GString | n/a (no `??=`) | n/a (no `??=`) |
GString | String |
| `m.with { k ?= "$n" }` † |
GString | GString | GString |
GString | String |
| `m.putAt('k', "$n")` |
GString | GString | ❌ |
❌ | String |
| **Other key types** |
| | |
| |
| `Map<Integer,String>`: `m[1] = "$n"` † |
GString | ❌ | ❌ | ❌
| String |
| `Map<E,String>` (enum key): `m[E.A] = "$n"` † |
GString | ❌ | ❌ | ❌
| String |
| **Other value conversions** |
| | |
| |
| `Map<String,String>`: `m.k = 123` † |
Integer | Integer | Integer |
Integer | String |
| `Map<String,String>`: `m['k'] = 123` † |
Integer | Integer | ❌ |
❌ | String |
| `Map<String,Long>`: `m.a = 1` |
Integer | Integer | Integer |
Integer | Long |
| `Map<String,Long>`: `m['b'] = 2` |
Integer | Integer | ❌ |
❌ | Long |
| `Map<String,Long>`: `m.putAt('c', 3)` † |
Integer | Integer | ❌ |
❌ | Long |
| `Map<String,BigDecimal>`: `m.a = 1` † |
Integer | Integer | Integer |
Integer | BigDecimal |
| `Map<String,Double>`: `m['a'] = 1` † |
Integer | Integer | ❌ |
❌ | Double |
| `Map<String,Integer>`: `m.a = (short) 1` † |
Short | Short | Short |
Short | Integer |
| `Map<String,Long>`: `m.a += 1` † |
Long | Long | Long |
Long | Long |
| `Map<String,Long>`: `m.a++` † |
Long | Long | Long |
Long | Long |
| **Wildcard value types: no `@ClassTag` token, so no conversion** |
| | |
| |
| `? super String`: `m.k = "$n"` † ‡ |
GString | GString | GString |
GString | GString |
| `? super String`: `m['k'] = "$n"` † ‡ |
GString | GString | GString |
GString | GString |
| `? super Long`: `m.k = 1` † ‡ |
Integer | Integer | Integer |
Integer | Integer |
| `? super Long`: `m['k'] = 1` † ‡ |
Integer | Integer | Integer |
Integer | Integer |
| `? extends String`: `m.k = "$n"` ‡ |
GString | GString | GString |
String | GString |
| `? extends String`: `m['k'] = "$n"` † |
GString | GString | ❌ |
❌ | ❌ |
| `? extends Long`: `m.k = 1` † ‡ |
Integer | Integer | Integer |
Integer | Integer |
| `? extends Number`: `m.k = 1` † ‡ |
Integer | Integer | Integer |
Integer | Integer |
| `? extends Number`: `m['k'] = 1` † |
Integer | Integer | ❌ |
❌ | ❌ |
| `?`: `m.k = "$n"` † ‡ |
GString | GString | GString |
GString | GString |
| `?`: `m['k'] = "$n"` † ‡ |
GString | GString | GString |
GString | GString |
| **Value type that a `GString` already is, or not known** |
| | |
| |
| `Map<String,Object>`: `m.k = "$n"` |
GString | GString | GString |
GString | GString |
| `Map<String,CharSequence>`: `m.k = "$n"` |
GString | GString | GString |
GString | GString |
| raw `Map`: `m.k = "$n"` |
GString | GString | GString |
GString | GString |
| raw `Map`: `m['k'] = "$n"` † |
GString | GString | GString |
GString | GString |
| **Other map methods: the arguments are checked as for any call** |
| | |
| |
| `m.put('k', "$n")` |
GString | ❌ | ❌ | ❌
| ❌ |
| `m.putIfAbsent('k', "$n")` † |
GString | ❌ | ❌ | ❌
| ❌ |
| `m.merge('k', "$n") { a, b -> a + b }` † |
GString | ❌ | ❌ | ❌
| ❌ |
| `m.putAll([k: "$n"])` † |
GString | ❌ | ❌ | ❌
| ❌ |
| `m << [k: "$n"]` † |
GString | ❌ | ❌ | ❌
| ❌ |
| `m += [k: "$n"]` † |
GString | ❌ | ❌ | ❌
| ❌ |
| `Map<String,String> m = [k: "$n"]` † |
GString | ❌ | ❌ | ❌
| ❌ |
| `new HashMap<String,String>([k: "$n"])` † |
GString | ❌ | ❌ | ❌
| ❌ |
| `m.withDefault { "$n" }` † |
GString | ❌ | ❌ | ❌
| ❌ |
| `m.computeIfAbsent('k') { "$n" }` † |
GString | String | String |
String | String |
| `Map<String,List<String>>`: `m.k = ["$n"]` † |
GString | ❌ | ❌ | ❌
| ❌ |
| **Properties declared with a type variable (not maps, outside this PR)** |
| | |
| |
| `Box<String>`: `b.v = "$n"` † ‡ |
GString | GString | GString |
GString | GString |
| `Box<String>`: `b.@v = "$n"` † ‡ |
GString | GString | GString |
GString | GString |
| `Box<String>`: `b.with { v = "$n" }` † ‡ |
GString | GString | GString |
GString | GString |
| `StrBox extends Box<String>`: `s.v = "$n"` † ‡ |
GString | GString | GString |
GString | GString |
| `Box<Long>`: `b.v = 1` † ‡ |
Integer | Integer | Integer |
Integer | Integer |
| `Map.Entry<String,String>`: `e.value = "$n"` † ‡ |
GString | GString | GString |
GString | GString |
| `Box<String>`: `b.setV("$n")` † |
GString | ❌ | ❌ | ❌
| ❌ |
| control, `String p`: `c.p = "$n"` † |
String | String | String |
String | String |
| **Already of the value type** |
| | |
| |
| `m.k = 'ab'` |
String | String | String |
String | String |
| `m.k = null` |
null | null | null |
null | null |
| **Explicit type arguments** |
| | |
| |
| `Optional.<String>of("$n")` † ‡ |
GString | GString | GString |
GString | GString |
| `Collections.<String>singletonList("$n")` † ‡ |
GString | GString | GString |
GString | GString |
| **Compiled with `classTagPreemptionDisabled`** |
| | |
| |
| `m.k = "$n"` ‡ |
GString | n/a | GString |
String | GString |
| `m['k'] = "$n"` |
GString | n/a | ❌ |
❌ | ❌ |
5.1.3 and 6.0.0 are the released jars, master is 9d20996988, and "this PR"
is 5a2df03fb9. `classTagPreemptionDisabled` doesn't exist in 5.1.3.
### Not addressed here
None of the ‡ rows is a regression; each stores what it stored in 5.1.3 and
6.0.0. They can't be fixed by conversion without making `@TypeChecked` change
runtime behaviour outside the `@ClassTag` mechanism. The sound alternative is
to reject them, the way the equivalent setter or `put` call is already
rejected. That is a separate, breaking change, so it belongs in its own issue:
- **Properties declared with a type variable** (`Box<String>.v = "$n"`,
`Map.Entry.value`): It would deserve a JIRA proposing that the write be checked
as the setter call would be.
- **Wildcard value types and explicit type arguments:** left as they are for
now.
--
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]
