paulk-asert commented on PR #3154:
URL: https://github.com/apache/groovy/pull/3154#issuecomment-6683658257
Thanks Daniel. ACK on both deltas, both deliberate, plus one proposal for
6.0.x that follows from the first.
**1. `? extends String` property writes.** No `@ClassTag` token is supplied
for a wildcard value type, so the write stores the value as it did in 5.1.3 and
6.0.0, like every other wildcard row. Master's #3057 conversion predates the
`@ClassTag` approach.
Looking at this more closely, I'd like to revert #3057 on `GROOVY_6_0_X`
before 6.0.1 rather than keep it there:
- It makes `@TypeChecked` code store a different value from the same dynamic
code (String vs GString) outside the `@ClassTag` mechanism, which is meant to
be the one sanctioned way for that to happen.
- It ignores `classTagPreemptionDisabled`, which 6.0.0 already ships, so the
documented opt-out doesn't opt out.
- With this PR on master, some writes would flip twice across releases:
| Write (under `@TypeChecked` / `@CompileStatic`) | 6.0.0 | 6.0.1 with #3057
| 7 with this PR |
|---|---|---|---|
| `Map<String,? extends String>`: `m.k = "$n"` | GString | String | GString |
| `Map<String,String>`: `m.k = "$n"`, preemption disabled | GString | String
| GString |
- The `ClassCastException` reads from 6.0.0 are already fixed on
`GROOVY_6_0_X` by other changes. With #3057 reverted locally on the current
6.0.x head, `@CompileStatic` reads such as `m.k.toUpperCase()` and
`m.get('k').length()` on a GString-holding `Map<String,String>` work. That
includes a GString put there by dynamic code, which #3057 never covered. Every
write in my matrix then stores exactly what 6.0.0 stored.
So 6.0.x would keep 6.0.0's write behaviour with the crash fixed, and the
conversion would arrive once, in 7, through `@ClassTag` with the opt-out
honoured. If you'd rather keep #3057 on 6.0.x, I'll add an upgrade note for the
two rows above instead but it will be strange to flip and flip back.
**2. All assignment conversions, not just GString to String.**
`Map<String,Long>`: `m.a = 1` stores a `Long`, and `Map<String,String>`: `m.k =
123` stores `"123"`. That matches `Long l = 1` and `String s = 123`, and it is
what makes the map hold values of the declared type. It only changes the type
of a value that was already accepted. No type-checked write that compiled
before now fails. The GROOVY-12535 section of COMPATIBILITY.md now states the
rule as "converted as an assignment to a variable of that type is", gives the
number-to-String example, and says code relying on a stored `Integer` or number
keeping its own type is affected. The release notes will say the same.
**Nits.** I've fixed the `ClassTagSupport` comment, which now names
`withDefault` and `putAt`, and rebased onto current master. I left out
`Objects.requireNonNull(valueType)`, because `valueType.cast` already throws an
NPE on a null token.
Agreed on no backport of this shape to 6.0.x.
--
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]
