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

Memoize default values in Schema, not Field #1651

Merged
merged 1 commit into from
Mar 12, 2025

Conversation

kubukoz
Copy link
Member

@kubukoz kubukoz commented Feb 19, 2025

Surely this won't break anything...

PR Checklist (not all items are relevant to all PRs)

  • Added unit-tests (for runtime code)
  • Added bootstrapped code + smoke tests (when the rendering logic is modified)
  • Added build-plugins integration tests (when reflection loading is required at codegen-time)
  • Added alloy compliance tests (when simpleRestJson protocol behaviour is expanded/updated)
  • Updated dynamic module to match generated-code behaviour
  • Added documentation
  • Updated changelog

@Baccata
Copy link
Contributor

Baccata commented Feb 20, 2025

can you elaborate on the why ?

@plokhotnyuk
Copy link
Contributor

plokhotnyuk commented Feb 20, 2025

I would prefer to have Option[() => A] instead (kind of evaluator function without any memorization) because instantiation of default values can be side effecting or the result value could be just mutable and should not be shared.

@kubukoz
Copy link
Member Author

kubukoz commented Feb 20, 2025

can you elaborate on the why ?

@Baccata I think it could be surprising that default values would get memoized in one place but not in another. My assumption is that the current behavior may not have been intentional, but maybe that's mistaken

I would prefer to have Option[() => A] instead (kind of evaluator function without any memorization) because instantiation of default values can be side effecting or the result value could be just mutable and should not be shared.

there should be no side effects in instantiation of defaults from generated code. The only way I see it happening would be if you use a refinement to a mutable type, and that's probably already broken in several ways...

@Baccata Baccata marked this pull request as ready for review March 12, 2025 10:02
@Baccata Baccata merged commit 58cf08a into series/0.18 Mar 12, 2025
11 checks passed
@kubukoz kubukoz deleted the memoize-defaultvalue branch March 12, 2025 13:56
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