Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GROOVY-11294: Make generated toList() and toMap() methods on records return immutable collections #2044

Conversation

PaintNinja
Copy link
Contributor

@PaintNinja PaintNinja changed the title GROOVY-11294 record immutable tolist GROOVY-11294: Make generated toList() and toMap() methods on records return immutable collections Jan 26, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: 25 lines in your changes are missing coverage. Please review.

Comparison is base (6b8915f) 68.5474% compared to head (3f46926) 68.5163%.
Report is 15 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##               master      #2044        +/-   ##
==================================================
- Coverage     68.5474%   68.5163%   -0.0311%     
- Complexity      29119      29174        +55     
==================================================
  Files            1422       1422                
  Lines          113482     113535        +53     
  Branches        19521      19555        +34     
==================================================
+ Hits            77789      77790         +1     
- Misses          29156      29201        +45     
- Partials         6537       6544         +7     
Files Coverage Δ
...ava/org/codehaus/groovy/runtime/InvokerHelper.java 70.3125% <100.0000%> (+0.1867%) ⬆️
...in/java/org/codehaus/groovy/vmplugin/VMPlugin.java 6.6667% <ø> (ø)
.../groovy/transform/RecordTypeASTTransformation.java 85.9649% <90.4762%> (-0.0130%) ⬇️
.../java/org/codehaus/groovy/vmplugin/v16/Java16.java 51.5152% <16.6667%> (-7.7441%) ⬇️
...in/java/org/codehaus/groovy/vmplugin/v8/Java8.java 76.9006% <0.0000%> (-2.2039%) ⬇️
...in/java/org/codehaus/groovy/vmplugin/v9/Java9.java 59.7633% <8.3333%> (-3.9310%) ⬇️

... and 41 files with indirect coverage changes

@eric-milles
Copy link
Member

Adding reliance on the runtime library (InvokerHelper and VMPlugin) means a record can no longer be tagged @POJO and used in a vanilla java environment. And though an immutable retval could be cached, it isn't here. So what is the primary benefit? It looks like a pound of cure for an ounce of problem.

@PaintNinja
Copy link
Contributor Author

PaintNinja commented Jan 29, 2024

Groovy records already rely on the runtime library - the generated toList() and toMap() use ScriptBytecodeAdapter#createList/createMap in Groovy 4 and Groovy 5 snapshots (pre this PR) - even when tagged with @POJO.

As explained on the mailing list, using the runtime library allows for using Java's faster immutable List.of()/Map.of() when possible without needing many changes to the AST transform or bytecode gen.

After reading some of the feedback here, on the mailing list and jira, I better understand the importance of preferring simpler approaches and will be replying to the comments and re-evaluating my PR accordingly when I have time.

@PaintNinja PaintNinja marked this pull request as draft January 29, 2024 22:00
@paulk-asert
Copy link
Contributor

As per comment in the issue, I'll close this PR. There is plenty of scope for further improvements - as noted, even with POJO, records still make calls into runtime. This is why @pojo is marked @Incubating and one of the things we still plan to improve. But it is the kind of thing we'd like to improve for more than just records if we can, so we'd target a broader change if we can make such a change.

@paulk-asert paulk-asert closed this Feb 5, 2024
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.

4 participants