Getting typecheck failures after #313 was merged

[jshirley]

Hi folks!

We started to get typechecking failures after this commit: 53bbc23 in #313

The code failing typecheck is a pretty standard Rails pattern:

    data = params.require(:magic_links).permit(...)
    data[:other_field] = other_value
    @magic_link = organization.magic_links.create(data)

The typecheck error is:

app/controllers/controller.rb:90: Method valid? does not exist on T::Array[MagicLink] https://srb.help/7003
    90 |      if @magic_link.valid?

If we revert the change to actionpack.rbi, we get a clean typecheck:

$ bundle exec srb tc
No errors! Great job.

I think that the new signatures fight with the Rails interface where create can accept an array to create multiple records.

If I change the code to .create(*data), it passes typechecking (but .to_h doesn't, it still seems to think it is an array).

Happy to keep testing more on this, it's puzzling that this is the only method that is getting mixed up -- we use this pattern everywhere, it's such a common one.

Thanks!

[KaanOzkan]

Without seeing the exact types for all the calls it's hard to say. Are you able to simplify the code and sprinkle it with T.reveal_type calls to track the type? Looking at the diff this should change T.reveal_type(params.require(:magic_links)) to return T.untyped instead of ActionController::Parameters. This could be causing different types to propagate down the line and trigger the error.

This particular commit looks correct but this investigation can point to another sig that's inaccurate and can be fixed.

[jshirley]

I broke out the code to check each thing, it's a bit wonky now, but here's the code and the typecheck output.

     data = params.require(:magic_link).permit(:purpose, :redirect_to)
      T.reveal_type(data)
      data[:redirect_to] = URI.parse(data[:redirect_to]).path if data[:redirect_to].present?
      T.reveal_type(data)

      p = data.merge(
        expires_at: 15.minutes.from_now,
        user_id: @organization_user.user_id,
      )
      T.reveal_type(p) # T.untyped
      @magic_link = T.must(@organization_user.organization).magic_links.create(p)
      T.reveal_type(@magic_link) # T::Array[MagicLink]

Clearing output directory
app/controllers/users_controller.rb:81: Revealed type: T.untyped https://srb.help/7014
    81 |      T.reveal_type(data)
              ^^^^^^^^^^^^^^^^^^^
  Got T.untyped originating from:
    app/controllers/users_controller.rb:80:
    80 |      data = params.require(:magic_link).permit(:purpose, :redirect_to)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

app/controllers/users_controller.rb:83: Revealed type: T.untyped https://srb.help/7014
    83 |      T.reveal_type(data)
              ^^^^^^^^^^^^^^^^^^^
  Got T.untyped originating from:
    app/controllers/users_controller.rb:80:
    80 |      data = params.require(:magic_link).permit(:purpose, :redirect_to)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

app/controllers/users_controller.rb:89: Revealed type: T.untyped https://srb.help/7014
    89 |      T.reveal_type(p)
              ^^^^^^^^^^^^^^^^
  Got T.untyped originating from:
    app/controllers/users_controller.rb:85:
    85 |      p = data.merge(
    86 |        expires_at: 15.minutes.from_now,
    87 |        user_id: @organization_user.user_id,
    88 |      )

app/controllers/users_controller.rb:91: Revealed type: T::Array[MagicLink] https://srb.help/7014
    91 |      T.reveal_type(@magic_link)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^
  Got T::Array[MagicLink] originating from:
    app/controllers/admin/organization_users_controller.rb:90:
    90 |      @magic_link = T.must(@organization_user.organization).magic_links.create(p)
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

app/controllers/users_controller.rb:93: Method valid? does not exist on T::Array[MagicLink] https://srb.help/7003
    93 |      if @magic_link.valid?
                             ^^^^^^
[snip]
Errors: 5

It's very weird (to me) that this instance is the only one that is causing the T::Array[...]. I'm pretty new into Sorbet, but happy to dig further into anything else!

[jshirley]

Also here is the rbi for the MagicLink class, but the create method looks the same as any other ActiveRecord model.

    sig { params(block: T.nilable(T.proc.params(object: ::MagicLink).void)).returns(::MagicLink) }
    sig do
      params(
        attributes: T::Array[T.untyped],
        block: T.nilable(T.proc.params(object: ::MagicLink).void)
      ).returns(T::Array[::MagicLink])
    end
    sig do
      params(
        attributes: T.untyped,
        block: T.nilable(T.proc.params(object: ::MagicLink).void)
      ).returns(::MagicLink)
    end
    def create(attributes = nil, &block); end

(like a User rbi):

    sig { params(block: T.nilable(T.proc.params(object: ::User).void)).returns(::User) }
    sig do
      params(
        attributes: T::Array[T.untyped],
        block: T.nilable(T.proc.params(object: ::User).void)
      ).returns(T::Array[::User])
    end
    sig { params(attributes: T.untyped, block: T.nilable(T.proc.params(object: ::User).void)).returns(::User) }
    def create(attributes = nil, &block); end

[KaanOzkan]

I think the issue is that T.untyped matches T::Array[T.untyped] and now sorbet selects the second create signature instead of the 3rd. Returning T::Array[::MagicLink] instead of MagicLink

[jshirley]

To test that, I manually edited the magic_link.rbi to swap the positions of those sigs and ... yup!

Now it's what I expect:

Revealed type: MagicLink https://srb.help/7014
    91 |      T.reveal_type(@magic_link)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^

[KaanOzkan]

Unfortunately the order seems to be intentional Shopify/tapioca@00d9b03#diff-ccd1794e49448cc1badb48d2a32af60d5fcd47336d00a484fbf474ebf79b6789R994-R1007. And I don't think Sorbet will support differentiating between T.untyped and T::Array[T.untyped].

For now you can use T.unsafe(organization.magic_links).create(data) in the call site.

[egiurleo]

@jshirley did this resolve your issue? Any other questions? If not, I'll close the issue.

[jshirley]

Hi Emily, I would say not really, but I don't know if there is actually a fix here that will safely apply. We've worked around forcing avoiding the array rather than using T.unsafe.

It pops up in our code base from time to time, but we've shared this ticket and the workaround for it. I know that there are reasons for the #313 PR but it's still breaking! 😢