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

Add support for overloaded signatures and add overloaded ActiveRecordRelations sigs #1799

Merged
merged 4 commits into from
Mar 7, 2024

Conversation

bdewater
Copy link
Contributor

Motivation

Close #1789

Implementation

Based on the patch in aforementioned issue.

Tests

Updated tests.

@bdewater bdewater requested a review from a team as a code owner February 20, 2024 04:19
@bdewater
Copy link
Contributor Author

Getting an unrelated test failure on Rails 7 for Rubies 3.0-3.3, but not Ruby head for
cli check-shims it detects duplicates from Sorbet's payload

@KaanOzkan
Copy link
Contributor

@bdewater it's resolved on main, you can rebase.

@bdewater bdewater force-pushed the active-record-relations-overloads branch 2 times, most recently from ac18693 to 43f8e1a Compare February 22, 2024 17:11
@bdewater
Copy link
Contributor Author

@KaanOzkan thanks, only failing check now is for missing labels which I can't add :)

@paracycle
Copy link
Member

@bdewater This will need to be rebased on main again, since we just merged a mandatory minimum dependency on the version of sorbet that includes support for multiple signatures in #1809.

Otherwise, we would need to do feature checking to detect if the version of Sorbet that we are operating against has support for that or not.

lib/tapioca/rbi_ext/model.rb Outdated Show resolved Hide resolved
lib/tapioca/rbi_ext/model.rb Outdated Show resolved Hide resolved
lib/tapioca/rbi_ext/model.rb Outdated Show resolved Hide resolved
lib/tapioca/rbi_ext/model.rb Outdated Show resolved Hide resolved
lib/tapioca/dsl/compilers/active_record_relations.rb Outdated Show resolved Hide resolved
@bdewater bdewater force-pushed the active-record-relations-overloads branch from 43f8e1a to 5af5e29 Compare February 29, 2024 02:00
@bdewater
Copy link
Contributor Author

Thanks for the feedback @paracycle! Rebased and incorporated your suggestions.

Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

Thank you!

@bdewater bdewater force-pushed the active-record-relations-overloads branch 2 times, most recently from 8e689eb to 46e6ae7 Compare March 1, 2024 03:46
@bdewater bdewater force-pushed the active-record-relations-overloads branch from 46e6ae7 to 4457b30 Compare March 1, 2024 15:33
Comment on lines 559 to 566
common_relation_methods_module.create_sig(
parameters: [create_param("args", type: args_type)],
return_type: "T::Enumerable[#{constant_name}]",
),
common_relation_methods_module.create_sig(
parameters: [create_param("args", type: "T.untyped")],
return_type: constant_name,
),
Copy link
Contributor

Choose a reason for hiding this comment

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

This pattern doesn't seem to work

# test.rbi
class Foo
  sig { returns(T.untyped) }
  def foo; end

  sig { returns(T::Array[T.untyped]) }
  def bar; end
end

sig { params(args: T::Array[T.untyped]).returns(T::Enumerable[Foo]) }
sig { params(args: T.untyped).returns(Foo) }
def find(args); end
# test.rb
T.reveal_type(find(Foo.new.foo)) # T::Enumerable[Foo]
T.reveal_type(find(Foo.new.bar)) # T::Enumerable[Foo]

First sig always takes precedence. I'm not sure why, maybe it's a sorbet bug. We could open a ticket and revert this change until its fixed.

Copy link
Contributor Author

@bdewater bdewater Mar 5, 2024

Choose a reason for hiding this comment

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

Interesting, that does feel like a bug. T::Array[T.untyped] has to be first since T.untyped otherwise would match everything, except in this particular case when it doesn't match T.untyped 🙈

We could also generates a bit more strict sigs and flip the order so that the common case is first in case Sorbet cannot resolve the correct sig:

sig { params(args: T.any(String, Integer)).returns(Foo) }
sig { params(args: T::Array[T.any(String, Integer)]).returns(T::Enumerable[Foo]) }
def find(args); end

I think this would cover ~all cases (UUID and other string PKs, (big)int PKs)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think that's good. I assume bug doesn't happen then. I feel like String and Integer is enough but I don't have much context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

T.untyped argument would be matched against the first signature since it's a subtype of everything, as Jez pointed out in the linked Sorbet issue.

Also noticed this particular issue was documented on https://sorbet.org/docs/overloads 😅

In the presence of untyped arguments, chances are high that the first overload is selected, which might not be desired.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some experimenting with the Rails console - find(nil) errors here, find({}) errors here which gives a pretty good list of types that can be passed.

The problem is that T.untyped is both the superclass and the subclass of all types in Sorbet, so that when given an untyped parameter the first signature (returning an array) would be selected whereas probably a single model instance would be intended (see the removed note in a previous commit about find being untyped in the first place).

Integer and String (think UUID) should cover the most common cases, but it is technically possible to pass anything that can be quoted by Active Record except nil (which is disallowed by ActiveRecord::FinderMethods#find_with_ids).
@bdewater bdewater force-pushed the active-record-relations-overloads branch from 455239b to d357096 Compare March 6, 2024 21:39
Copy link
Contributor

@KaanOzkan KaanOzkan left a comment

Choose a reason for hiding this comment

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

Retried CI, strange failure.

@KaanOzkan
Copy link
Contributor

KaanOzkan commented Mar 7, 2024

CI is green but there are actually some failures. Looks like in main too. Let's hold off until we can trust CI
It's only the experimental ones so we're good.
Screenshot 2024-03-07 at 11 38 16 AM

@KaanOzkan KaanOzkan merged commit defcac7 into Shopify:main Mar 7, 2024
18 checks passed
Comment on lines +562 to +571
sigs = [
common_relation_methods_module.create_sig(
parameters: [create_param("args", type: id_types)],
return_type: constant_name,
),
common_relation_methods_module.create_sig(
parameters: [create_param("args", type: array_type)],
return_type: "T::Enumerable[#{constant_name}]",
),
]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing one overload:

find can be passed a block, in which case it delegates to Enumerable#find, which returns a nilable result.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this covers it #1844

Copy link
Contributor

Choose a reason for hiding this comment

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

That's spot on. Thanks @KaanOzkan!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is there a way to write a compiler that defines multiple sig for a single method?
4 participants