Non-optional belongs_to relationships can still be nil

[francois]

Describe the bug:

ActiveRecord 5.2+ gained the optional option on belongs_to declarations. sorbet-rails infers that belongs_to relationships where optional is false means that the referenced object will always be present.

Steps to reproduce:

class Employee < ActiveRecord::Base
  belongs_to :company # required association because optional is absent
end

T.must(Employee.find(1).company) #=> passes, because validation enforced the presence
T.must(Employee.new.company) #=> fails because company is nil
T.must(Employee.find(1).tap{|e| e.company = nil}.company #=> fails, because company is nil

In our specific case, we discovered the issue with code such as this:

class Sale < ActiveRecord::Base
  belongs_to :payment_term

  def init_with_defaults
    s = Sale.new

    # Sorbet complains about unreachable code
    s.payment_term ||= PaymentTerm.default
    s
  end
end

Expected behavior:

belongs_to relationships should always be T.nilable(Elem), without regards to the optional option.

# employee.rbi
class Employee < ActiveRecord::Base
  sig{ returns(T.nilable(Company)) }
  def company(*) ; end
end

Versions:

• Ruby: ruby 2.6.6p146 (2020-03-31 revision 67876) [x86_64-darwin18]
• Rails: Rails 6.0.3.2
• Sorbet: Sorbet typechecker 0.5.5767 git 14101d244fcc8eb11dceb8289f59db3062b24d95 debug_symbols=true clean=1
• Sorbet-Rails: sorbet-rails (0.7.0)

[francois]

To me, the fix seems to be to change this line:

https://github.com/chanzuckerberg/sorbet-rails/blob/8aa611954e5be727f08eee7c16e7eb1314ea0fbd/lib/sorbet-rails/model_plugins/active_record_assoc.rb#L37

The calls to #belongs_to_and_required? and #has_one_and_required? would need to go, leaving only T.nilable(#{assoc_class}). I can certainly write a PR for that, with specs, but I'm not sure how you want to go about fixing this issue.

[hdoan741]

There is a class of issues with a newly created or unsaved record (Sale.new). I'm thinking about how to deal with it, eg we have 2 type: Sale and UnsavedSale. For UnsavedSale, all attributes would be nilable. It's not implemented yet though :-(

[DanielGilchrist]

Just my 2c on the issue - I think belongs_to associations with optional: false should not be nilable as most business logic will be written for when the records have already been persisted to the DB. I personally prefer and think it's less obtrusive to treat calls where the records aren't persisted yet as an edge case than having to handle nilable values unnecessarily. In saying that, if there were a better solution where we could know when those calls would be nilable that'd be amazing