Skip to content

GROOVY-12024: add getAt(Map,String) and putAt(Map,String,V)#2580

Merged
paulk-asert merged 1 commit into
masterfrom
GROOVY-12024
Jun 9, 2026
Merged

GROOVY-12024: add getAt(Map,String) and putAt(Map,String,V)#2580
paulk-asert merged 1 commit into
masterfrom
GROOVY-12024

Conversation

@eric-milles

@eric-milles eric-milles commented May 31, 2026

Copy link
Copy Markdown
Member

Should map['key'] work like map.key or map.get('key')? In older Groovy versions, getAt(Map,Object) was considered closer than getAt(Object,String). Adding getAt(Map,String) and putAt(Map,String,V) restores alignment between getAt and get at the expense of the public property handling for the subscript syntax.

STC of map['key'] = value would indicate "Cannot assign value of type ..." but now reports "Cannot call putAt ... with arguments ..."

@eric-milles eric-milles requested a review from paulk-asert May 31, 2026 16:39

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.

Benchmark suite Current: dc01f8c Previous: 7bfdeea Ratio
org.apache.groovy.perf.grails.DynamicDispatchBench.invokeMethodInterception 10.069979808420793 ms/op 6.492504280080586 ms/op 1.55

This comment was automatically generated by workflow using github-action-benchmark.

@codecov-commenter

codecov-commenter commented May 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.1832%. Comparing base (a3b47f2) to head (dc01f8c).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##               master      #2580        +/-   ##
==================================================
+ Coverage     68.1808%   68.1832%   +0.0023%     
  Complexity      33164      33164                
==================================================
  Files            1511       1511                
  Lines          126232     126235         +3     
  Branches        22897      22897                
==================================================
+ Hits            86066      86071         +5     
+ Misses          32527      32524         -3     
- Partials         7639       7640         +1     
Files with missing lines Coverage Δ
.../codehaus/groovy/runtime/DefaultGroovyMethods.java 75.1044% <100.0000%> (+0.0208%) ⬆️

... and 8 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sonarqubecloud

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown
Contributor

JMH summary — classic (commit 84922ba)

Speedup vs trailing 90-day baseline on gh-pages. Higher = faster.
1.00 = in line with history. Per-benchmark ratio, geomean within group.
Time-per-op units inverted so direction is consistent.

Group Speedup n
bench 0.997 × 26
core 1.141 × 77
grails 0.977 × 80

Baseline: dev/bench/jmh/<part>/classic/data.js on gh-pages, trailing 90 days. Daily dashboard · Per-suite raw data

@github-actions

Copy link
Copy Markdown
Contributor

JMH summary — indy (commit 84922ba)

Speedup vs trailing 90-day baseline on gh-pages. Higher = faster.
1.00 = in line with history. Per-benchmark ratio, geomean within group.
Time-per-op units inverted so direction is consistent.

Group Speedup n
bench 0.974 × 26
core 0.993 × 77
grails 1.039 × 80

Baseline: dev/bench/jmh/<part>/indy/data.js on gh-pages, trailing 90 days. Daily dashboard · Per-suite raw data

@testlens-app

testlens-app Bot commented May 31, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: dc01f8c
▶️ Tests: 100169 executed
⚪️ Checks: 30/30 completed


Learn more about TestLens at testlens.app.

@paulk-asert

Copy link
Copy Markdown
Contributor

I think this is an improvement, I will try to do an assessment of all the cases tomorrow.

@paulk-asert

Copy link
Copy Markdown
Contributor

The key reframing: under the dynamic runtime, m['k'] already means m.get('k') for every key — [:].class, [:].properties, [:].metaClass all return null (the absent entry), not the bean. The surprising behavior was confined to static resolution, where subscript was binding to the property-access overload. So this PR isn't trading one consistency for another so much as realigning STC subscript with the long-standing dynamic semantics (and with get/put).

Verified values (dynamic against the runtime; static column = intended 6.0 / the STC test assertions in this PR):

Plain map[foo:'bar'] / [class:'C'] / [:]

key m.k dynamic m.k @CompileStatic m['k'] (6.0) m.get('k')
ordinary ('foo') 'bar' 'bar' 'bar' 'bar'
meta-name present ('class''C') 'C' (entry, not the Class) 'C' 'C' 'C'
meta-name absent ('class') null null null null

Map subclassclass C extends HashMap { def foo = 1; def bar = 2 }, then put('foo',11), put('baz',33)

key m.k dynamic m.k @CompileStatic m['k'] (6.0) m.get('k')
declared prop and entry ('foo': prop=1, entry=11) 11 (entry wins) 1 (property wins) 11 11
declared prop, no entry ('bar': prop=2) null (property ignored) 2 (property) null null
entry, no declared prop ('baz': entry=33) 33 33 33 33

Two takeaways:

  1. After this PR, m['k']m.get('k') in every row and both modes — one clean column, no asterisks. That's the win worth stating explicitly as the invariant.
  2. The genuinely treacherous cell is m.k on a Map subclass, which disagrees dynamic-vs-static (entry wins dynamically, declared property wins under STC). This PR doesn't touch dot access, so that divergence remains — subscript just sidesteps it now. Might be worth a follow-up issue, or at least a doc note steering entry access to []/get.

Suggestion for locking it in: pin the invariant as a paired test that runs the same asserts under dynamic and @CompileStatic, so the static/dynamic gap that caused this can't silently reopen via overload-resolution drift. Happy to put that together if helpful.

@paulk-asert paulk-asert left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should take this but then work on the table to close anomolies

@paulk-asert paulk-asert merged commit 4da7301 into master Jun 9, 2026
63 checks passed
@paulk-asert paulk-asert deleted the GROOVY-12024 branch June 9, 2026 23:02
@paulk-asert

Copy link
Copy Markdown
Contributor

The above table was wrong. Here is the revised one:

For reference, here's the full m.k vs m['k'] vs m.get('k') matrix on 6.0 after this change, across dynamic and @CompileStatic access. Every cell is now pinned by the live tables in _working-with-collections.adoc / WorkingWithCollectionsTest.groovy.

Plain map[foo:'bar'] / [class:'C'] / [:]

key m.k m.k (@CompileStatic) m['k'] m.get('k')
ordinary ('foo') 'bar' 'bar' 'bar' 'bar'
meta-name present ('class''C') 'C' (entry, not the Class) 'C' 'C' 'C'
meta-name absent ('class') null null null null

Map subclassclass C extends HashMap { def foo = 1; def bar = 2 }, then put('foo',11), put('baz',33)

key m.k m.k (@CompileStatic) m['k'] m.get('k')
'foo' (property=1, entry=11) 1 1 11 11
'bar' (property=2, no entry) 2 2 null null
'baz' (no property, entry=33) 33 33 33 33

Takeaways:

  1. m['k']m.get('k') in every row and in both modes — that's the invariant this PR restores (subscript was previously diverging under static resolution).
  2. m.k and m['k'] agree on a plain map but legitimately differ on a subclass: m.k reads the declared property when one exists (falling back to the entry), while m['k']/get always read the entry. Dynamic and @CompileStatic columns now match throughout, so the only axis that matters is property-vs-entry, not dynamic-vs-static.
  3. Practical guidance: use ['k']/get whenever you mean "the entry"; reserve m.k for property semantics.

@paulk-asert

Copy link
Copy Markdown
Contributor

To keep this up -to-date, I added a "live table" of these results in:
src/spec/doc/_working-with-collections.adoc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants