Add types to generated RBI files for FrozenRecord

[nickpresta]

Motivation

This change is motivated by some work by team has been doing using FrozenRecord/FrozenApplicationRecord in Core. We noticed that Sorbet wasn't really giving us useful typing information for fields where we expected and looking at the generated RBI file, I saw that each field is typed as T.untyped.

I wanted to see if there was a relatively straightforward change that would improve the typing without a dev having to do a whole lot. Upon further digging and a Slack discussion it seems like another issue is that all the accessor methods are marked as T.untyped, too, which means Record.first (as an example) comes back as T.untyped which means doing things like:

first_name = Record.first.first_name
all_nick_ages = Record.where(first_name: "Nick").order(age: :desc).pluck(:age)

returns in everything being T.untyped, which isn't helpful. Devs are forced to do things like:

first_name = T.let(Record.first.first_name, String)
all_nick_ages = T.let(Record.where(first_name: "Nick").order(age: :desc).pluck(:age), T::Array[Integer])

this becomes a little more frustrating when you're not dealing with an individual field, but with the Record entirely:

# first_record is typed as Record, but the fields are T.untyped
# and defining each field sucks
first_record = T.let(Record.first, Record)
all_nick_records = T.let(Record.where(first_name: "Nick").order(age: :desc), T::Array[Record])
# all_nick_records[0].first_name is T.untyped

So my motivation is to at least make doing T.let(..., Record) the only thing a dev needs to do to get reasonable typing.

If this change is accepted, the next step would be to better type the methods available as part of FrozenRecord (e.g. find, where, order) since we should know (I think?) that calling Record.find(...) returns T.nilable(Record).

Implementation

The implementation depends on the ActiveModel::Attributes functionality that's used in Rails to explicitly define attribute :my_field, :string for each of the fields that we care to type.

Tests

I added a new unit test with a variety of field types to exercise this functionality and have left comments on this PR where I think we could do better, or areas we should consider for further implementation/testing.

[nickpresta]

The potential problem here is that if your first record contains fields that subsequent records don't, the type will be generated without T.nilable, which is wrong. In practice, I'm not sure how much this happens though.

Another consideration is that inferring the type from the first record means that we're using types that are deserialized from whichever backend is used - YAML and JSON primarily. YAML will convert fields to Date and Time whereas JSON wouldn't so the typing may not be consistent for the same values if the data is stored as YAML vs JSON. Again, this may not be a big deal in practice and is the way the library already works.

Some alternatives include:

• Generate all these types as T.nilable(ActualType) and force the nil check on consumers. This accounts for fields that are missing in some records, as well as fields having a null value. This seems like the best option since it requires no real work on the part of devs and while it may be extra caution (e.g. if you know your data is always the same shape and doesn't contain null values), it isn't too big a deal to do record&.thing and move on your life.
• Allow devs to specify on the record the type of each field and leave it up to them to define things correctly. I've been told that "we try to avoid changing the runtime to satisfy Sorbet (such as adding an unnecessary method or attribute, so that Sorbet can understand a signature)" so maybe this isn't a good idea. However, this is a good option to give control to devs so that fields they know won't be nilable aren't typed as such.
• Add some sort of YAML front matter that specifies the type of each field, although this would require a "specification" for what this definition looks like. This seems strictly worse than adding something in code, since we can't use Ruby directly and we have to make up a format to represent type information, but it doesn't influence the runtime in any way. There is also no concept of front matter for JSON, so we would need to either encode the typing information differently or ignore it entirely in JSON.

[nickpresta]

After some discussion in Slack, @paracycle suggested we use ActiveModel::Attributes to declare the attributes and types without doing any runtime discovery.

This covers the cases where the shape inferred from the first record may be incomplete, and makes it explicit which fields are which types.

However, using ActiveModel::Attributes comes with a couple of caveats:

• Arrays, hashes, and symbols cannot be specified, in general. Arrays can be specified if using PostgreSQL (associated test case), but we can't depend on a specific database backend, and hashes don't seem supported at all (although I see some people using ActiveRecord::Type::Json which seems like a workaround). This sucks since arrays and hashes are totally fine in YAML and (ideally) shouldn't require any custom types just to get it to work.
• Typing custom types is "fine", but there's no real way to specific anything more specific than what you can return from the type method on ActiveModel::Type::Value. This results in things like Array and Hash types, which is "fine", but it would still be preferable to be able to specify the element type.
• Defaults specified using attribute :my_field, default: 'hi' don't work in FrozenRecord, since the default_attributes field should be used instead. So we need to look at default_attributes and any dev who specified attribute :my_field, default: 'hi' won't actually have their changes reflected. Perhaps we should throw in this case and suggest using default_attributes?

The only other option to solve these problems is to define something custom that allows for sorbet types specifically. For example:

class Student < FrozenRecord::Base
  extend T::Sig

  self.field_types = {
    first_name: ::String,
    favourite_foods: T::Array[String],
    skills: T::Hash[String, String],
  } 
end

but this isn't ideal either, since we're making it up specifically for Tapioca.

---

With all this said, I think using ActiveModel::Attribute is better than inferring types and for existing code that isn't specifying the attributes, there are no expected changes. I feel confident that this is an improvement, although not perfect, and sets us up for typing of the query/finder/calculation methods.

@paracycle I would appreciate any feedback on whether this approach makes sense and if there are any ways to workaround some of the caveats above -- I'm relatively new to Rails so I'm sure I'm missing some context.

Especially, I would appreciate some help on your last comment:

It should also either raise a runtime error or do a string cast when it encounters the foo: 124 value, since it is not a String.

It wasn't clear where should this check be performed. Thanks for any help!