Missing T::Struct options that impact type checking

[jeffcarbs]

We started using the factory option for a field in one of our T::Structs and noticed it didn't come through the RBI that tapioca generated. factory is basically the same as default but takes a proc so the value can be set dynamically. Like default, if a prop/const has a factory set you don't have to pass a value in the initializer so its absence from the RBI meaningfully impacts type-checking. Given that tapioca uses the RBI::TStructConst/Prop models, support needs to be added here first.

I started down the path of a fix, adding handling for factory where there's handling for default, when I realized that there's at least one more option that affects type checking: immutable. If you set prop :x, Integer, immutable: true it basically acts like const where it will show a type error if you try to use the setter. I also realized that the current parsing logic expects default (if present at all) to be the first option so if you have immutable: true, default: 1 the parser will currently drop both on the floor.

Here's a sorbet.run to showing these examples.

There are many more options (although many seem to be internal to stripe) and technically you can pass arbitrary options.

Before I went further I wanted to get your thoughts. I think we at least need to handle factory and immutable since those impact type checking, so there are a few options:

• add factory and immutable attrs to TStructField (and const/prop) and handle them whenever we currently handle default.
  • Slightly more verbose but very explicit
  • We'll drop other options on the floor. This is probably fine but it would mean that input == (parse(input).out) is no longer necessarily true.
• remove default from TStructField and replace it with options which would capture all options
  • More flexible but less explicit
• just add factory, and when we see “immutable: true” parse it as a Const instead of a Prop.

There's also a middle ground where the constructor for TStructField and friends still only take default (and maybe factory and immutable as well?) as an explicit option since it's the most common but also take an options hash. The initializer would join the two so you'd only ever operate on the hash. This would be similar to how Attr takes (name, names) and then joins them together in the initializer.

I'm happy to take this on here and in tapioca so let me know what you think.

[jeffcarbs]

I made an attempt at the second approach (using an “options” hash). The one spec failing is because of how we output procs not matching the source proc, which I can look into.

[Morriar]

Thanks for taking a look at this 🙏

I like the approach, my only concern is the change in the constructor signature for TStructField which will be breaking compatibility.

[Morriar]

Let's rename this:

Suggested change

	name, type, options = parse_tstruct_prop(node)
	name, type, options = parse_tstruct_field(node)

[Morriar]

And merge the two cases:

when :prop, :const
  parse_tstruct_field(method_name, node)
else
  # ...

[Morriar]

Maybe we can return the TStructField instance directly:

Suggested change

	sig { params(node: AST::Node).returns([String, String, T::Hash[Symbol, String]]) }
	sig { params(node: AST::Node).returns(TStructField) }

[Morriar]

We'll need to add some magic here to properly print the value when it's a proc.

[Morriar]

I would rather add the new options to the constructor to avoid a breaking change that would require updating all the Tapioca's custom compilers.

[jeffcarbs]

I wound up fixing this with a lighter touch directly in tapioca (Shopify/tapioca#833) and don't have time to pick this PR back up so going to close it out.