Make dry-initializer usable from non-main Ractors after finalize#111
Make dry-initializer usable from non-main Ractors after finalize#111flash-gordon wants to merge 3 commits into
Conversation
81af65d to
390ce64
Compare
|
Note: we may want to rename |
timriley
left a comment
There was a problem hiding this comment.
I'm not a Dry Initializer expect, but upon my first reading, these changes all look sensible. Thank you for putting this together, @flash-gordon!
Aside from my inline comments, a couple of more general thoughts below.
From your PR description:
The public seal entry point is now Dry::Initializer#finalize (no bang). The previous finalize! is gone, and the old internal regen step has been renamed to a private Config#compile.
I think our method name here was always just .finalize (no bang)? I can't find any trace of finalize! in the main branch or git history, at least.
Note: we may want to rename
finalizetofinalize!
Why do you suggest this? Seems like finalize was the API before (as noted above), and I'm not sure if we really have sufficient justification for introducing the bang. I think the method name on its own already conveys its intent, and we don't have "non-mutative" or "somehow less dangerous" equivalent method that needs us to introduce the bang as a differentiator.
(I know that we have Dry::System::Container.finalize! and tbh, I feel like that'd be better without the bang, it doesn't feel necessary there either).
One thing to note about this PR is that the class-level .finalize becomes an important piece of public API, whereas before it was an internal concern only.
At minimum, this means:
- We'll need to update docs over in https://github.com/hanakai-rb/site for this new version
- We should make it clear that finalizing a class freezes the ancestor chain's initializer configs (
Ractor.make_shareablewalking through@parent, as I mentioned via inline). The good news is that adding params to a subclass of a finalized class still works (which is what we actually document at https://hanakai.org/learn/dry/dry-initializer/v3.2/inheritance). But what doesn't work is adding params to a parent after any descendant has been finalized. Feels like this is worth noting both on the method API docs as well as our website docs.
I also wonder whether there's a more specific name we could use for this method so that we avoid potential name collisions. Dry Initializer gets used across all kinds of classes, and "finalize" is generic enough that users might want to use it for their own API. A standalone call to "finalize" inside a class definition is also not clear enough that it actually relates to the initializer params.
What if we called it "finalize_initializer"? It's wordier, but it makes it the connection to the initializer clearer, both in terms of the Ruby method as well as the gem that it came from. What do you think? I'm open to other ideas too, please share your suggestions!
Thanks again for doing this, I love that we're being responsive to user needs even in these more mature gems of ours ❤️
| end | ||
| RUBY | ||
|
|
||
| # Replace the bmethod `klass.dry_initializer` from `DSL#extended` |
There was a problem hiding this comment.
| # Replace the bmethod `klass.dry_initializer` from `DSL#extended` | |
| # Replace the method `klass.dry_initializer` from `DSL#extended` |
typo?
UPDATE: upon seeing "bmethod" repeated further down in this diff, and then googling harder, I realise "bmethod" is shorthand for "a method defined by define_method". Maybe it's fine to leave in, then. But I do think it's possibly confusing to future readers — it doesn't feel like a common Ruby term. What do you think?
There was a problem hiding this comment.
An LLM picked it up from MRI's internals. It can be useful for further LLM-aided work on the sources so I'd change it to the block-based version (aka bmethod in MRI) ...
| # Rebuild the generated initializer on the mixin and recurse into | ||
| # children. Called on every DSL change. Private — it makes no | ||
| # Ractor-readiness guarantees; only {#finalize} does. |
There was a problem hiding this comment.
Can I just say that I really appreciate the level of commenting you've added here. This will be really helpful for our future selves and any other contributors ❤️
There was a problem hiding this comment.
Contrary to before, it now takes time to quench Claude's desire to elaborate on every single thing. When the context grows, an LLM tries to dump all the intermediary steps we took, even though they are mostly irrelevant to the final result
| PrepareDefault, PrepareOptional, | ||
| UnwrapType, CheckType, BuildNestedType, WrapType | ||
| ] | ||
| defined?(Ractor) ? Ractor.make_shareable(list) : list.freeze |
There was a problem hiding this comment.
Wow, this is going to be kind of ungainly if we're going to have to repeat this across large parts of our ecosystem.
There was a problem hiding this comment.
I agree! But for this particular thing, I have a justification. It's a questionable design from the start: modifying the internal pipeline from outside. It should have been provided via a simple plugin API. This design led to such clumsy branching. If we were to release 4.0 of the gem it'd be a good candidate for rework.
| Dry::Initializer::Dispatchers << dispatcher | ||
| end | ||
|
|
||
| # The Dispatchers pipeline is process-global, so reset it after each |
There was a problem hiding this comment.
| # The Dispatchers pipeline is process-global, so reset it after each | |
| # The Dispatcher's pipeline is process-global, so reset it after each |
| end | ||
|
|
||
| # Finalizes config | ||
| # Seal the config. After finalize the class is usable from non-main |
There was a problem hiding this comment.
I notice here we're introducing "seal" as a special term for the first time.
I wonder if we could instead just continue to use "finalize" here (and then explain the consequences for Ractor usage, which you already do) rather than introducing another concept? Please correct me if I'm wrong, but "seal" doesn't seem super widely used (I did see one mention here: https://bugs.ruby-lang.org/issues/21665) and so perhaps it'd be better to avoid introducing a different word.
| RSpec.describe Dry::Initializer, "Ractor compatibility" do | ||
| before do | ||
| skip "Ractor not available" unless defined?(Ractor) | ||
| skip "Ractor tests gated to Ruby 4.0+" if RUBY_VERSION < "4" |
There was a problem hiding this comment.
Didn't we get Ractors before 4.0? Are we gating the tests to 4+ because it's the first "good" version of Ractor? Or do you have other reasons?
There was a problem hiding this comment.
because it's the first "good" version
Mostly this, but I will take a closer look later. I won't be merging this until a PR to dry-validation is sent at least
| @mixin ||= Module.new.tap do |mod| | ||
| initializer = self | ||
| mod.extend(Mixin::Local) | ||
| mod.set_temporary_name("Dry::Initializer::Mixin::Local[#{extended_class&.name}]") |
There was a problem hiding this comment.
extended_class.name may be nil for anonymous classes. Maybe we could have it return the plain inspect in that case, so we get e.g. Dry::Initializer::Mixin::Local[#<Class:0x...>] instead of Dry::Initializer::Mixin::Local[] (which feels a bit confusing).
I know this is a super edge case, but I thought I'd mention it anyway! We may as well make this solid while we're introducing this overhaul.
@timriley I wrote it because the next thing was dry-configurable, which already has |
Introduce `Dry::Initializer#finalize` (and `Config#finalize`) as the single public seal entry point — the previous `finalize!` is gone, and the old internal regen step is now a private `Config#compile`. After `finalize`, the class is callable from any Ractor: * `__dry_initializer_config__` and `klass.dry_initializer` are rewritten from bmethods into `class_eval`'d `def`s that read a shareable `DRY_INITIALIZER_CONFIG` constant. * The Config (with its definitions and default Procs) is deeply frozen via `Ractor.make_shareable`. Further `param`/`option` calls raise `FrozenError`. Pre-finalize, class-level Config storage no longer relies on writing `@dry_initializer` ivars (forbidden in non-main Ractors): both `DSL#extended` and `Dry::Initializer#inherited` install a `define_singleton_method` on the class, which `finalize` later swaps for the Ractor-safe `def`. Other changes pulled along to keep the path clean: * `Config#children` derives live from `Class#subclasses` instead of an owned `Set` — same immediate-only semantics, no mutation. * `Mixin::Local` is removed entirely; `Config#mixin` uses `Module#set_temporary_name` to keep the per-class mixin printing as `Dry::Initializer::Mixin::Local[<Class>]` in `ancestors`/`inspect`. * `Dispatchers.@pipeline` is eagerly initialized at load time and `Ractor.make_shareable`d so the default pipeline is readable from any Ractor. Custom dispatchers still work but opt out of Ractor compatibility unless registered with shareable Procs. * `spec/ractor_spec.rb` and `spec/inspect_spec.rb` cover the new surface end-to-end.
390ce64 to
f305ac1
Compare
Summary
Adds Ractor compatibility to dry-initializer. After
Klass.finalize, a class is usable from any Ractor —.new, subclassing, attribute reads, default Procs that reference other params, all work cross-Ractor.The public seal entry point is now
Dry::Initializer#finalize(no bang). The previousfinalize!is gone, and the old internal regen step has been renamed to a privateConfig#compile.After
finalize:__dry_initializer_config__andklass.dry_initializerare rewritten from bmethods intoclass_eval'ddefs reading a shareableDRY_INITIALIZER_CONFIGconstant.Ractor.make_shareable. Furtherparam/optioncalls raiseFrozenError.Pre-finalize, class-level Config storage no longer relies on writing
@dry_initializerivars (forbidden in non-main Ractors): bothDSL#extendedandDry::Initializer#inheritedinstall adefine_singleton_methodon the class, whichfinalizelater swaps for the Ractor-safedef.Other changes pulled along to keep the path clean:
Config#childrenderives live fromClass#subclassesinstead of an ownedSet— same immediate-only semantics, no mutation.Mixin::Localis removed entirely;Config#mixinusesModule#set_temporary_nameto keep the per-class mixin printing asDry::Initializer::Mixin::Local[<Class>]inancestors/inspect.Dispatchers.@pipelineis eagerly initialized at load andRactor.make_shareabled so the default pipeline is readable from any Ractor. Custom dispatchers still work but opt out of Ractor compatibility unless registered with shareable Procs.spec/ractor_spec.rbandspec/inspect_spec.rbcover the new surface end-to-end.