[
https://issues.apache.org/jira/browse/GROOVY-12535?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18139572#comment-18139572
]
ASF GitHub Bot commented on GROOVY-12535:
-----------------------------------------
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.
> STC: property-style assignment to a Map<String,String> accepts a GString
> value and stores it unconverted
> --------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12535
> URL: https://issues.apache.org/jira/browse/GROOVY-12535
> Project: Groovy
> Issue Type: Bug
> Reporter: Paul King
> Assignee: Daniel Sun
> Priority: Major
> Fix For: 7.0.0-alpha-1, 6.0.1
>
>
> Under static type checking, a {{GString}} can't be put into a
> {{Map<String,String>}} through {{put}} or the subscript operator; both are
> rejected. Assigning through property notation is accepted, though, and the
> {{GString}} is stored as is:
> {code:groovy}
> @groovy.transform.CompileStatic
> def go() {
> def n = 'ab'
> Map<String,String> m = [:]
> m.k = "$n"
> ((Map) m).values()*.getClass()*.simpleName
> }
> assert go() == ['GStringImpl'] // expected ['String']
> {code}
> ||Form||Result (4.0.29, 6.0.0, master)||
> |{{m.k = "$n"}}|compiles, stores a {{GStringImpl}}|
> |m.with { k = "$n" }|compiles, stores a {{GStringImpl}}|
> |{{m['k'] = "$n"}}|STC error: Cannot call ... putAt ...|
> |{{m.put('k', "$n")}}|STC error: Cannot call ... put(String, String) ...|
> Statically compiled code can't otherwise add a {{GString}} to a collection
> typed for Strings ({{<<}}, {{add}}, {{set}}, {{addAll}}, {{+=}}, {{putAt}}
> and {{put}} are all rejected), so this is the one hole on the write side.
> Later reads that expect a {{String}} can then fail. For example, Java code
> doing {{String s = m.get('k')}} throws {{ClassCastException}}, and
> {{m.get('k').equals('ab')}} is false.
> Expected: either convert the value to a {{String}} at the assignment, as
> Groovy does for {{String s = "$n"}}, or report the same error as {{put}} and
> {{putAt}}. Converting matches the general rule that a {{GString}} becomes a
> {{String}} where one is required, and wouldn't break existing code that
> compiles today.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
