Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ActiveRecordRelations Model.new typing #1983

Merged
merged 1 commit into from
Aug 9, 2024
Merged

Conversation

bitwise-aiden
Copy link
Contributor

Motivation

As pointed out in this issue: #1981

The typing for Model.new accepts an T::Array which it never actually does. The original discussion was around this being acceptable, but after trying to fix issues from this change to the compiler I'm now thinking that it is better to type this appropriately.

Consider:

def some_untyped_method
  {
    a: 1,
    b: 1,
    c: 1,
  }
end

SomeModel.new(some_untyped_method)
#             ^^^^^^^^^^^^^^^^^^^ T.untyped = return type becomes `T::Array`

Because of how Sorbet selects the signature in the case of T.untyped and overloaded signatures, it will tend to pick the first viable, which in the case of #new will be T::Array. This then implicitly means that any arguments to SomeModel.new will have to be typed otherwise it picks a type that is never possible for this method.

Implementation

I'm aware this may not be the correct approach, but I want to stat a conversation so we can best solve it. As for what I have here I've added a new with the removal of the T::Array signature to the model class after the extend. This will make it so that it overrides the signatures in CommonRelationMethods and avoid the T::Array issue.

Tests

Updated tests to reflect change.

Comment on lines 1036 to 1039
method.add_sig do |sig|
sig.add_param("block", "T.nilable(T.proc.params(object: #{constant_name}).void)")
sig.return_type = constant_name
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is needed, since the attributes param is already optional.

Copy link
Contributor Author

@bitwise-aiden bitwise-aiden Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right! It was originally a guard for the case where we had T::Array since it would default to that one in the case of SomeModel.new, but that doesn't apply here.

@bitwise-aiden bitwise-aiden merged commit 2634126 into main Aug 9, 2024
29 checks passed
@bitwise-aiden bitwise-aiden deleted the ba-fix-model-new branch August 9, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants