-
-
Notifications
You must be signed in to change notification settings - Fork 42
Make dry-initializer usable from non-main Ractors after finalize #111
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
base: main
Are you sure you want to change the base?
Changes from all commits
0237448
cd41b09
f305ac1
82c9d0d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -6,6 +6,10 @@ module Initializer | |||||
| # Gem-related configuration of some class | ||||||
| # | ||||||
| class Config | ||||||
| # Name of the constant {#finalize} sets on the mixin to hold this | ||||||
| # Config. Long enough to not collide with user-defined constants. | ||||||
| CONFIG_CONST = :DRY_INITIALIZER_CONFIG | ||||||
|
|
||||||
| # @!attribute [r] null | ||||||
| # @return [Dry::Initializer::UNDEFINED, nil] value of unassigned variable | ||||||
|
|
||||||
|
|
@@ -26,18 +30,33 @@ class Config | |||||
| def mixin | ||||||
| @mixin ||= Module.new.tap do |mod| | ||||||
| initializer = self | ||||||
| mod.extend(Mixin::Local) | ||||||
| mod.set_temporary_name("Dry::Initializer::Mixin::Local[#{extended_class&.inspect}]") | ||||||
| mod.define_method(:__dry_initializer_config__) do | ||||||
| initializer | ||||||
| end | ||||||
| mod.send :private, :__dry_initializer_config__ | ||||||
| end | ||||||
| end | ||||||
|
|
||||||
| # List of configs of all subclasses of the [#extended_class] | ||||||
| # Configs of {#extended_class}'s direct (one-level) subclasses. | ||||||
| # Deeper descendants are reached transitively by recursing into each | ||||||
| # child's own `#children` (as {#compile} does). | ||||||
| # | ||||||
| # Skips subclasses that don't have their own `dry_initializer` | ||||||
| # singleton method yet — they'd inherit ours and we'd recurse into | ||||||
| # ourselves. `DSL#extended` and `Dry::Initializer#inherited` install | ||||||
| # those singletons; this guard keeps `#children` correct in between | ||||||
| # (e.g. while the parent's own Config is still being built). | ||||||
| # | ||||||
| # @return [Array<Dry::Initializer::Config>] | ||||||
| def children | ||||||
| @children ||= Set.new | ||||||
| return [] unless extended_class | ||||||
|
|
||||||
| extended_class.subclasses.filter_map do |klass| | ||||||
| next unless klass.singleton_class.method_defined?(:dry_initializer, false) | ||||||
|
|
||||||
| klass.dry_initializer | ||||||
| end | ||||||
| end | ||||||
|
|
||||||
| # List of definitions for initializer params | ||||||
|
|
@@ -104,13 +123,40 @@ def code | |||||
| Builders::Initializer[self] | ||||||
| end | ||||||
|
|
||||||
| # Finalizes config | ||||||
| # Seal the config. After finalize the class is usable from non-main | ||||||
| # Ractors and the Config (with its definitions) is deeply frozen — | ||||||
| # further `param`/`option` calls raise `FrozenError`. | ||||||
| # | ||||||
| # @return [self] | ||||||
| def finalize | ||||||
| @definitions = final_definitions | ||||||
| check_order_of_params | ||||||
| mixin.class_eval(code, "#{__FILE__}:#{__LINE__} class_eval") | ||||||
| children.each(&:finalize) | ||||||
| return self if @finalized | ||||||
|
|
||||||
| compile | ||||||
|
|
||||||
| mixin.const_set(CONFIG_CONST, self) unless mixin.const_defined?(CONFIG_CONST, false) | ||||||
| mixin.send(:undef_method, :__dry_initializer_config__) \ | ||||||
| if mixin.private_method_defined?(:__dry_initializer_config__) | ||||||
| mixin.module_eval(<<~RUBY, __FILE__, __LINE__ + 1) | ||||||
| private def __dry_initializer_config__ | ||||||
| #{CONFIG_CONST} | ||||||
| end | ||||||
| RUBY | ||||||
|
|
||||||
| # Replace the bmethod `klass.dry_initializer` from `DSL#extended` | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||
| # / `Dry::Initializer#inherited` with a `def` reading the mixin | ||||||
| # constant, so it's callable from any Ractor. | ||||||
| if extended_class | ||||||
| extended_class.singleton_class.send(:undef_method, :dry_initializer) \ | ||||||
| if extended_class.singleton_class.method_defined?(:dry_initializer, false) | ||||||
| extended_class.class_eval(<<~RUBY, __FILE__, __LINE__ + 1) | ||||||
| def self.dry_initializer | ||||||
| #{CONFIG_CONST} | ||||||
| end | ||||||
| RUBY | ||||||
| end | ||||||
|
|
||||||
| @finalized = true | ||||||
| Ractor.make_shareable(self) if defined?(Ractor) | ||||||
| self | ||||||
| end | ||||||
|
|
||||||
|
|
@@ -126,32 +172,48 @@ def inch | |||||
| lines.join("\n") | ||||||
| end | ||||||
|
|
||||||
| protected | ||||||
|
|
||||||
| # Rebuild the generated initializer on the mixin and recurse into | ||||||
| # children. Called on every DSL change. Protected — it makes no | ||||||
| # Ractor-readiness guarantees (only {#finalize} does), but the | ||||||
| # recompile chain crosses sibling Config instances. | ||||||
| # @return [self] | ||||||
| def compile | ||||||
| @definitions = final_definitions | ||||||
| check_order_of_params | ||||||
| mixin.class_eval(code, "#{__FILE__}:#{__LINE__} class_eval") | ||||||
| children.each { |child| child.compile } | ||||||
| self | ||||||
| end | ||||||
|
|
||||||
| private | ||||||
|
|
||||||
| def initialize(extended_class = nil, null: UNDEFINED) | ||||||
| @extended_class = extended_class.tap { |klass| klass&.include mixin } | ||||||
| @extended_class = extended_class | ||||||
| sklass = extended_class&.superclass | ||||||
| @parent = sklass.dry_initializer if sklass.is_a? Dry::Initializer | ||||||
| @null = null || parent&.null | ||||||
| @definitions = {} | ||||||
| finalize | ||||||
| extended_class&.include(mixin) | ||||||
| compile | ||||||
| end | ||||||
|
|
||||||
| def add_definition(option, name, type, block, **opts) | ||||||
| opts = { | ||||||
| parent: extended_class, | ||||||
| option: option, | ||||||
| null: null, | ||||||
| option:, | ||||||
| null:, | ||||||
| source: name, | ||||||
| type: type, | ||||||
| block: block, | ||||||
| type:, | ||||||
| block:, | ||||||
| **opts | ||||||
| } | ||||||
|
|
||||||
| options = Dispatchers.call(**opts) | ||||||
| definition = Definition.new(**options) | ||||||
| definitions[definition.source] = definition | ||||||
| finalize | ||||||
| compile | ||||||
| mixin.class_eval definition.code | ||||||
| end | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,12 +106,20 @@ def call(**options) | |
| require_relative "dispatchers/wrap_type" | ||
|
|
||
| def pipeline | ||
| @pipeline ||= [ | ||
| PrepareSource, PrepareTarget, PrepareIvar, PrepareReader, | ||
| PrepareDefault, PrepareOptional, | ||
| UnwrapType, CheckType, BuildNestedType, WrapType | ||
| ] | ||
| @pipeline ||= begin | ||
| list = [ | ||
| PrepareSource, PrepareTarget, PrepareIvar, PrepareReader, | ||
| PrepareDefault, PrepareOptional, | ||
| UnwrapType, CheckType, BuildNestedType, WrapType | ||
| ] | ||
| defined?(Ractor) ? Ractor.make_shareable(list) : list.freeze | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow, this is going to be kind of ungainly if we're going to have to repeat this across large parts of our ecosystem.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| end | ||
| end | ||
|
|
||
| # Eagerly initialize so that the first read from a non-main Ractor | ||
| # doesn't trip the class-ivar-set restriction. Done at load time so | ||
| # the assignment happens in the main Ractor. | ||
| pipeline | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,6 @@ def self.extended(klass) | |
| end | ||
|
|
||
| require_relative "mixin/root" | ||
| require_relative "mixin/local" | ||
| end | ||
| end | ||
| end | ||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,6 @@ | |
|
|
||
| module Dry | ||
| module Initializer | ||
| VERSION = "3.2.0" | ||
| VERSION = "3.3.0" | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| # The per-class mixin that `extend Dry::Initializer` installs is an | ||
| # anonymous module. By default it would print as `#<Module:0x000...>` in | ||
| # ancestors output, backtraces, and `inspect` — noisy and uninformative. | ||
| # These tests pin down the user-visible effect: that mixin renders as | ||
| # `Dry::Initializer::Mixin::Local[<class>]` instead. | ||
|
|
||
| RSpec.describe "Dry::Initializer mixin inspection" do | ||
| before do | ||
| class Test::Foo | ||
| extend Dry::Initializer | ||
| param :foo | ||
| end | ||
| end | ||
|
|
||
| let(:mixin) { Test::Foo.dry_initializer.mixin } | ||
|
|
||
| it "shows up in the class's ancestors with a class-named label" do | ||
| expect(Test::Foo.ancestors.map(&:to_s)) | ||
| .to include("Dry::Initializer::Mixin::Local[Test::Foo]") | ||
| end | ||
|
|
||
| it "renders the same string from inspect, to_s, and string interpolation" do | ||
| expected = "Dry::Initializer::Mixin::Local[Test::Foo]" | ||
|
|
||
| expect(mixin.inspect).to eq(expected) | ||
| expect(mixin.to_s).to eq(expected) | ||
| expect("includes #{mixin}").to eq("includes #{expected}") | ||
| end | ||
|
|
||
| it "uses each class's own name when more than one class extends Dry::Initializer" do | ||
| class Test::Bar | ||
| extend Dry::Initializer | ||
| param :bar | ||
| end | ||
|
|
||
| foo_mixin = Test::Foo.dry_initializer.mixin | ||
| bar_mixin = Test::Bar.dry_initializer.mixin | ||
|
|
||
| expect(foo_mixin.to_s).to eq("Dry::Initializer::Mixin::Local[Test::Foo]") | ||
| expect(bar_mixin.to_s).to eq("Dry::Initializer::Mixin::Local[Test::Bar]") | ||
| end | ||
|
|
||
| it "falls back to the class's inspect for anonymous classes" do | ||
| klass = Class.new { extend Dry::Initializer; param :foo } | ||
|
|
||
| expect(klass.dry_initializer.mixin.to_s) | ||
| .to match(/\ADry::Initializer::Mixin::Local\[#<Class:0x\h+>\]\z/) | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.