Fix ActiveRecordRelations Model.new typing #1983
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
As pointed out in this issue: #1981
The typing for
Model.new
accepts anT::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:
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 beT::Array
. This then implicitly means that any arguments toSomeModel.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 inCommonRelationMethods
and avoid theT::Array
issue.Tests
Updated tests to reflect change.