Codegen the T::Struct initializer

[jeffcarbs]

Similar to #2683, we codegen the T::Struct initializer so that instead of initializing variables by looping through props at runtime, we generate code at load time (or earlier if using eager loading). This PR was extracted from #6684 since that PR was trying to do a little too much.

Motivation

Speeding up T::Struct initialization. This produces a 1.5-2x speedup in the benchmarks.

Before

Plain Ruby (μs/iter):
  0.447608   0.003099   0.450707 (  0.451505)
T::Props.new, mostly nil input (μs/iter):
  3.288567   0.020158   3.308725 (  3.314212)
T::Props.new, all props set (μs/iter):
  6.767186   0.018295   6.785481 (  6.788665)

After

Plain Ruby (μs/iter):
  0.441517   0.000999   0.442516 (  0.443651)
T::Props.new, mostly nil input (μs/iter):
  2.006774   0.003380   2.010154 (  2.010666)
T::Props.new, all props set (μs/iter):
  5.240457   0.029039   5.269496 (  5.303440)

NOTE: There were slightly faster benchmarks before the following two commits:

• Make generate_missing_handler more like deser helper
  • Reason: Less custom code, rely more on the default setters defined when props are added
  • Effect: Small since it only matters with mostly nil inputs and defaults are present.
• Remove custom typecheck handler
  • Reason: Less custom code, rely on value_validate_proc which is the same validation logic that the setters use
  • Effect: ~0.5 μs/iter (so around 4.8 instead of 5.3).

The safety in reusing that code is probably worth the small performance hit to ensure no logic drifts, although just wanted to call it out to show there's more juice to squeeze.

Test plan

See included automated tests. Similar to the serde PR, I added T::Props::GeneratedCodeValidation.validate_initialize to verify the code in the initialize method is what we expect.

Additional info

Here's an example of the generated code from the ComplexStruct:

Code

def __t_props_generated_initialize(hash)
  found = 25
  decorator = self.class.decorator

  val = hash[:primitive]
  if val.nil? && !hash.key?(:primitive)
    found -= 1
    @primitive = raise ArgumentError, "Missing required prop `primitive` for class `#{self.class.name}`"

  else
    @primitive = val
    decorator.props.fetch(:primitive).fetch(:value_validate_proc).call(@primitive)
  end

  val = hash[:nilable]
  if val.nil? && !hash.key?(:nilable)
    found -= 1
    @nilable = nil
    decorator.props.fetch(:nilable).fetch(:value_validate_proc).call(@nilable)
  else
    @nilable = val
    decorator.props.fetch(:nilable).fetch(:value_validate_proc).call(@nilable)
  end

  val = hash[:nilable_on_read]
  if val.nil? && !hash.key?(:nilable_on_read)
    found -= 1
    @nilable_on_read = raise ArgumentError, "Missing required prop `nilable_on_read` for class `#{self.class.name}`"

  else
    @nilable_on_read = val
    decorator.props.fetch(:nilable_on_read).fetch(:value_validate_proc).call(@nilable_on_read)
  end

  val = hash[:primitive_default]
  if val.nil? && !hash.key?(:primitive_default)
    found -= 1
    @primitive_default = 0

  else
    @primitive_default = val
    decorator.props.fetch(:primitive_default).fetch(:value_validate_proc).call(@primitive_default)
  end

  val = hash[:primitive_nilable_default]
  if val.nil? && !hash.key?(:primitive_nilable_default)
    found -= 1
    @primitive_nilable_default = 0

  else
    @primitive_nilable_default = val
    decorator.props.fetch(:primitive_nilable_default).fetch(:value_validate_proc).call(@primitive_nilable_default)
  end

  val = hash[:factory]
  if val.nil? && !hash.key?(:factory)
    found -= 1
    @factory = decorator.props_with_defaults.fetch(:factory).default
    decorator.props.fetch(:factory).fetch(:value_validate_proc).call(@factory)
  else
    @factory = val
    decorator.props.fetch(:factory).fetch(:value_validate_proc).call(@factory)
  end

  val = hash[:primitive_array]
  if val.nil? && !hash.key?(:primitive_array)
    found -= 1
    @primitive_array = raise ArgumentError, "Missing required prop `primitive_array` for class `#{self.class.name}`"

  else
    @primitive_array = val
    decorator.props.fetch(:primitive_array).fetch(:value_validate_proc).call(@primitive_array)
  end

  val = hash[:array_default]
  if val.nil? && !hash.key?(:array_default)
    found -= 1
    @array_default = []

  else
    @array_default = val
    decorator.props.fetch(:array_default).fetch(:value_validate_proc).call(@array_default)
  end

  val = hash[:primitive_hash]
  if val.nil? && !hash.key?(:primitive_hash)
    found -= 1
    @primitive_hash = raise ArgumentError, "Missing required prop `primitive_hash` for class `#{self.class.name}`"

  else
    @primitive_hash = val
    decorator.props.fetch(:primitive_hash).fetch(:value_validate_proc).call(@primitive_hash)
  end

  val = hash[:array_of_nilable]
  if val.nil? && !hash.key?(:array_of_nilable)
    found -= 1
    @array_of_nilable = raise ArgumentError, "Missing required prop `array_of_nilable` for class `#{self.class.name}`"

  else
    @array_of_nilable = val
    decorator.props.fetch(:array_of_nilable).fetch(:value_validate_proc).call(@array_of_nilable)
  end

  val = hash[:nilable_array]
  if val.nil? && !hash.key?(:nilable_array)
    found -= 1
    @nilable_array = nil
    decorator.props.fetch(:nilable_array).fetch(:value_validate_proc).call(@nilable_array)
  else
    @nilable_array = val
    decorator.props.fetch(:nilable_array).fetch(:value_validate_proc).call(@nilable_array)
  end

  val = hash[:substruct]
  if val.nil? && !hash.key?(:substruct)
    found -= 1
    @substruct = raise ArgumentError, "Missing required prop `substruct` for class `#{self.class.name}`"

  else
    @substruct = val
    decorator.props.fetch(:substruct).fetch(:value_validate_proc).call(@substruct)
  end

  val = hash[:nilable_substract]
  if val.nil? && !hash.key?(:nilable_substract)
    found -= 1
    @nilable_substract = nil
    decorator.props.fetch(:nilable_substract).fetch(:value_validate_proc).call(@nilable_substract)
  else
    @nilable_substract = val
    decorator.props.fetch(:nilable_substract).fetch(:value_validate_proc).call(@nilable_substract)
  end

  val = hash[:default_substruct]
  if val.nil? && !hash.key?(:default_substruct)
    found -= 1
    @default_substruct = decorator.props_with_defaults.fetch(:default_substruct).default

  else
    @default_substruct = val
    decorator.props.fetch(:default_substruct).fetch(:value_validate_proc).call(@default_substruct)
  end

  val = hash[:array_of_substruct]
  if val.nil? && !hash.key?(:array_of_substruct)
    found -= 1
    @array_of_substruct = raise ArgumentError, "Missing required prop `array_of_substruct` for class `#{self.class.name}`"

  else
    @array_of_substruct = val
    decorator.props.fetch(:array_of_substruct).fetch(:value_validate_proc).call(@array_of_substruct)
  end

  val = hash[:hash_of_substruct]
  if val.nil? && !hash.key?(:hash_of_substruct)
    found -= 1
    @hash_of_substruct = raise ArgumentError, "Missing required prop `hash_of_substruct` for class `#{self.class.name}`"

  else
    @hash_of_substruct = val
    decorator.props.fetch(:hash_of_substruct).fetch(:value_validate_proc).call(@hash_of_substruct)
  end

  val = hash[:infinity_float]
  if val.nil? && !hash.key?(:infinity_float)
    found -= 1
    @infinity_float = decorator.props_with_defaults.fetch(:infinity_float).default

  else
    @infinity_float = val
    decorator.props.fetch(:infinity_float).fetch(:value_validate_proc).call(@infinity_float)
  end

  val = hash[:negative_infinity_float]
  if val.nil? && !hash.key?(:negative_infinity_float)
    found -= 1
    @negative_infinity_float = decorator.props_with_defaults.fetch(:negative_infinity_float).default

  else
    @negative_infinity_float = val
    decorator.props.fetch(:negative_infinity_float).fetch(:value_validate_proc).call(@negative_infinity_float)
  end

  val = hash[:nan_float]
  if val.nil? && !hash.key?(:nan_float)
    found -= 1
    @nan_float = decorator.props_with_defaults.fetch(:nan_float).default

  else
    @nan_float = val
    decorator.props.fetch(:nan_float).fetch(:value_validate_proc).call(@nan_float)
  end

  val = hash[:enum]
  if val.nil? && !hash.key?(:enum)
    found -= 1
    @enum = raise ArgumentError, "Missing required prop `enum` for class `#{self.class.name}`"

  else
    @enum = val
    decorator.props.fetch(:enum).fetch(:value_validate_proc).call(@enum)
  end

  val = hash[:nilable_enum]
  if val.nil? && !hash.key?(:nilable_enum)
    found -= 1
    @nilable_enum = nil
    decorator.props.fetch(:nilable_enum).fetch(:value_validate_proc).call(@nilable_enum)
  else
    @nilable_enum = val
    decorator.props.fetch(:nilable_enum).fetch(:value_validate_proc).call(@nilable_enum)
  end

  val = hash[:default_enum]
  if val.nil? && !hash.key?(:default_enum)
    found -= 1
    @default_enum = decorator.props_with_defaults.fetch(:default_enum).default

  else
    @default_enum = val
    decorator.props.fetch(:default_enum).fetch(:value_validate_proc).call(@default_enum)
  end

  val = hash[:deprecated_enum]
  if val.nil? && !hash.key?(:deprecated_enum)
    found -= 1
    @deprecated_enum = raise ArgumentError, "Missing required prop `deprecated_enum` for class `#{self.class.name}`"

  else
    @deprecated_enum = val
    decorator.props.fetch(:deprecated_enum).fetch(:value_validate_proc).call(@deprecated_enum)
  end

  val = hash[:nilable_deprecated_enum]
  if val.nil? && !hash.key?(:nilable_deprecated_enum)
    found -= 1
    @nilable_deprecated_enum = nil
    decorator.props.fetch(:nilable_deprecated_enum).fetch(:value_validate_proc).call(@nilable_deprecated_enum)
  else
    @nilable_deprecated_enum = val
    decorator.props.fetch(:nilable_deprecated_enum).fetch(:value_validate_proc).call(@nilable_deprecated_enum)
  end

  val = hash[:default_deprecated_enum]
  if val.nil? && !hash.key?(:default_deprecated_enum)
    found -= 1
    @default_deprecated_enum = :foo_one

  else
    @default_deprecated_enum = val
    decorator.props.fetch(:default_deprecated_enum).fetch(:value_validate_proc).call(@default_deprecated_enum)
  end

  if found < hash.size
    raise ArgumentError, "#{self.class}: Unrecognized properties: #{(hash.keys - decorator.props.keys).join(', ')}"
  end
end

[jeffcarbs]

@froydnj - I think this is ready for a look. I rebased onto the latest master to pull in those specs that were backfilled locking in the weak/strong constructor behavior. I also refactored some things a bit to make it look more like the deserialize generator.

[froydnj]

Failing testcase from the compiler testsuite:

# frozen_string_literal: true
# typed: true

class A < T::Struct
  prop :int, Integer
  prop :str, String
  prop :optint, Integer, default: 5
  prop :optarray, T::Array[Integer], default: [6]
  prop :optstr, String, default: "shared string"
end

# Verify default arguments don't share structure.
x = A.new(int: 5, str: "foo")
y = A.new(int: 6, str: "bar")
p x.optarray.object_id != y.optarray.object_id
# ...but it's OK if the strings do, given that we're using frozen_string_literal.
p x.optstr.object_id == y.optstr.object_id

Before:

st-froydnj2:sorbet froydnj$ ruby -r ./gems/sorbet-runtime/lib/sorbet-runtime.rb ../struct-sharing.rb
true
true

After:

st-froydnj2:sorbet froydnj$ ruby -r ./gems/sorbet-runtime/lib/sorbet-runtime.rb ../struct-sharing.rb
true
false

[froydnj]

This also fails on many tests in Stripe's codebase (we only report the first 100 errors, so I don't have a good sense of how far it made it..but it's also failing in the abstractions around credit cards, so I imagine that it didn't make it very far).

I'll try to reduce a testcase, but I probably won't have it until next week.

[froydnj]

http://go/builds/bui_NKLtkyWuMKi8E0 for any interested Stripe.

[jeffcarbs]

@froydnj - Pushed up a commit to address the object equality issue (and added a test case for it in sorbet-runtime to confirm). The issue was that for any string defaults we were just putting the string literal in the source. Now we check if the string is frozen and use "#{string.inspect}.freeze" if so. The code around defaults was adapted from the deserialize generator and I found a test that actually locks in the opposite behavior, i.e. string defaults, even frozen, do not "share structure". Seems like we may want to fix that behavior (in a separate PR, of course), since if your default is an immutable string but the struct is initialized with a mutable string that could be dangerous: https://github.com/sorbet/sorbet/blob/4dd7dee1208155cb5b1ead8bca7abc58d2e4863d/gems/sorbet-runtime/test/types/props/serializable.rb#L1224-L1231.

[jeffcarbs]

@froydnj - thanks again for the review! I pulled most of those test changes into those separate PRs that have already been merged to ensure we're locking in existing behavior. The only new test changes here are adding the validation for the generated code.

I got all the tests and rubocop passing. I also updated the description with the latest benchmarks, some info about some commits where I backed out some custom code, and an example of the generated input. Would you be able to trigger the tests and maybe run a build on the stripe codebase to see what issues surface?

[froydnj]

We have a policy of testing changes to Sorbet against Stripe's codebase before merging them. I've kicked off a test run for the current PR. When the build finishes, I'll share with you whether or how it failed. Thanks!

Stripe employees can see the build results here:

→ https://go/builds/bui_PRnETGCnZQU4K5 → https://go/builds/bui_PRnEuUPTdDRplM → https://go/builds/bui_PRnEF9hBiODQAM

This change also has sorbet-runtime changes. Those tests:

→ https://go/builds/bui_PRnM5RpL8Y2rMk

[froydnj]

I asked around and it looks like we might not be freezing those classes @jeffcarbs . So you may want to hold off fixing that for just a bit.

[froydnj]

We have a policy of testing changes to Sorbet against Stripe's codebase before merging them. I've kicked off a test run for the current PR. When the build finishes, I'll share with you whether or how it failed. Thanks!

Stripe employees can see the build results here:

→ https://go/builds/bui_PTEnQnRhPSyM5e → https://go/builds/bui_PTEnKL8bFwAJxb → https://go/builds/bui_PTEnMFpFOkznqy

This change also has sorbet-runtime changes. Those tests:

→ https://go/builds/bui_PTEuPqzIj5Oi9i

[froydnj]

It looks like there's a test failure where a constructor is not raising a RuntimeError. It will take me a little while to track this one down.

[jeffcarbs]

OK sounds good. Let me know if there's anything I can do to help.

[froydnj]

We have a policy of testing changes to Sorbet against Stripe's codebase before merging them. I've kicked off a test run for the current PR. When the build finishes, I'll share with you whether or how it failed. Thanks!

Stripe employees can see the build results here:

→ https://go/builds/bui_PU36OeSoXEM87q → https://go/builds/bui_PU36P2Y1Nm5He2 → https://go/builds/bui_PU36KZ7VW93UWd

This change also has sorbet-runtime changes. Those tests:

→ https://go/builds/bui_PU3GaujPwh7k7c

[froydnj]

I thought of something else to test, which actually passed without issues (which is a good thing!). Would you mind adding something like this:

    describe 'does not overwrite user-declared initialize' do
      it 'works' do
        m = Class.new do
          include T::Props::WeakConstructor

          prop :foo, T.nilable(String)

          def initialize(foo)
            raise "raising with #{foo}"
          end
        end

        m.decorator.eagerly_define_lazy_methods!

        ex = assert_raises(StandardError) do
          m.new('secret value')
        end

        assert_equal('raising with secret value', ex.message)
      end
    end

The PR seems to be performance-neutral on Stripe's test suite (not that this is bad, just that the lazy initialization bits are not blocking anything important, and we would force evaluation of the lazy bits in production anyway).

[jeffcarbs]

Just pushed up a commit to add that test and rebased onto the latest upstream to pull in the change from #7643.

The performance bump here is definitely pretty minor, but this is a precursor to support #6667. Currently the runtime type checks are the mechanism by which missing required props are identified, so disabling those type checks would break that behavior. There's no great way to change that in the current world without introducing more performance overhead.

After this PR, if you stop checking types for all props, the performance is close to the PORO:

Plain Ruby (μs/iter):
  0.435867   0.000729   0.436596 (  0.436767)
T::Props.new, mostly nil input (μs/iter):
  2.058915   0.010521   2.069436 (  2.073485)
T::Props.new, mostly nil input, checked(:never) (μs/iter): <-- [NEW]
  1.002338   0.002045   1.004383 (  1.005056)
T::Props.new, all props set (μs/iter):
  5.290807   0.029587   5.320394 (  5.348496)
T::Props.new, all props set, checked(:never) (μs/iter):    <-- [NEW]
  0.605002   0.001626   0.606628 (  0.607012)