Shopify / tapioca

The swiss army knife of RBI generation
MIT License
735 stars 122 forks source link

Sorbet generates unreachable assignment for AR default attributes #1370

Open lake-effect opened 1 year ago

lake-effect commented 1 year ago

Referred from https://github.com/sorbet/sorbet/issues/6621

Input

Model:

# typed: strict

class Shortcode < ApplicationRecord
  after_initialize :set_defaults, if: :new_record?

  private

  def set_defaults
    self.code ||= SecureRandom.urlsafe_base64(10)
  end
end

# == Schema Information
#
# Table name: shortcodes
#
#  id            :bigint           not null, primary key
#  code          :string           not null
#  created_at    :datetime         not null
#  updated_at    :datetime         not null
#
# Indexes
#
#  index_shortcodes_on_code  (code) UNIQUE

Migration:

class CreateShortcodes < ActiveRecord::Migration[7.0]
  def change
    create_table :shortcodes do |t|
      t.string :code, null: false
      t.index :code, unique: true
      t.timestamps
    end
  end
end

Annotation RBI (generated by tapioca):

class Shortcode
  include GeneratedAttributeMethods
  extend GeneratedRelationMethods

  private

  # cut for length

  module GeneratedAttributeMethods

  # cut for length

    sig { returns(::String) }
    def code; end

    sig { params(value: ::String).returns(::String) }
    def code=(value); end

    sig { returns(T::Boolean) }
    def code?; end

    sig { returns(T.nilable(::String)) }
    def code_before_last_save; end

    sig { returns(T.untyped) }
    def code_before_type_cast; end

    sig { returns(T::Boolean) }
    def code_came_from_user?; end

    sig { returns(T.nilable([::String, ::String])) }
    def code_change; end

    sig { returns(T.nilable([::String, ::String])) }
    def code_change_to_be_saved; end

    sig { returns(T::Boolean) }
    def code_changed?; end

    sig { returns(T.nilable(::String)) }
    def code_in_database; end

    sig { returns(T.nilable([::String, ::String])) }
    def code_previous_change; end

    sig { returns(T::Boolean) }
    def code_previously_changed?; end

    sig { returns(T.nilable(::String)) }
    def code_previously_was; end

    sig { returns(T.nilable(::String)) }
    def code_was; end

    sig { void }
    def code_will_change!; end
  end
end

Observed output

$ bundle exec srb tc
Indexing |=======================================================| ETA: 0h00m00s
Resolving |======================================================| ETA: 0h00m01s
app/models/shortcode.rb:7: This code is unreachable https://srb.help/700600m00s
    7 |    self.code ||= SecureRandom.urlsafe_base64(10)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    app/models/micropage.rb:7: This condition was always truthy (String)
    7 |    self.code ||= SecureRandom.urlsafe_base64(10)
            ^^^^^^^^^^^^^^^
  Got String originating from:
    app/models/shortcode.rb:7:
    7 |    self.short_code ||= SecureRandom.urlsafe_base64(10)
            ^^^^^^^^^^^^^^^
CFG+Inference |==================================================| ETA: 0h00m00s
Errors: 1
error Command failed with exit code 1.

Expected behavior

The check should have passed, because it is possible for self.short_code to be either nil or defined depending on how Shortcode.create! is called. You can call Shortcode.create!(code: "my_code") and create a record with code defined, and you can also call Shortcode.create! at which point line 7 above will assign a random code. The code is not unreachable.


Sorbet version: 0.5.10160 Tapioca version: 0.10.3

Morriar commented 1 year ago

@paracycle, regarding your comment in https://github.com/sorbet/sorbet/issues/6621#issuecomment-1351577985:

I am surprised that Tapioca is generating a non-nilable column type here.

It seems like we're generating this as a non-type on purpose: https://github.com/Shopify/tapioca/blob/main/spec/tapioca/dsl/compilers/active_record_columns_spec.rb#L169-L212. If the code presented here makes sense, we should change the DSL compiler to produce a nilable type.