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

Block arity isn't considered for overloads #7743

Open
bdewater opened this issue Mar 4, 2024 · 2 comments · May be fixed by #7859
Open

Block arity isn't considered for overloads #7743

bdewater opened this issue Mar 4, 2024 · 2 comments · May be fixed by #7859
Labels
bug Something isn't working unconfirmed This issue has not yet been confirmed by the Sorbet team

Comments

@bdewater
Copy link
Contributor

bdewater commented Mar 4, 2024

I'm trying to add types for ActiveSupport::Notifications but noticed that block arity isn't considered for overloaded methods, but it is for procs (as can be seen by hovering over subscribe in VS Code).

Depending on the arity given to either the framework will either pass a single ActiveSupport::Notifications::Event object or five positional arguments.

Input

Since overloads don't work on Sorbet.run, omitting the link.

test.rbi:

class ActiveSupport::Notifications::Event
  sig { returns(T.untyped) }
  def duration; end
end

class ActiveSupport::Notifications
  sig do
    params(
      pattern: T.nilable(T.any(String, Regexp)),
      block: T.proc.params(
        name: String,
        start: Time,
        finish: Time,
        id: String,
        payload: T::Hash[T.untyped, T.untyped],
      ).void,
    ).returns(T.untyped)
  end
  sig do
    params(
      pattern: T.nilable(T.any(String, Regexp)),
      block: T.proc.params(event: ActiveSupport::Notifications::Event).void,
    ).returns(T.untyped)
  end
  sig do
    params(
      pattern: T.nilable(T.any(String, Regexp)),
      callback: T.proc.params(
        name: String,
        start: Time,
        finish: Time,
        id: String,
        payload: T::Hash[T.untyped, T.untyped],
      ).void,
    ).returns(T.untyped)
  end
  sig do
    params(
      pattern: T.nilable(T.any(String, Regexp)),
      callback: T.proc.params(event: ActiveSupport::Notifications::Event).void,
    ).returns(T.untyped)
  end
  def self.subscribe(pattern = T.unsafe(nil), callback = T.unsafe(nil), &block); end
end

test.rb:

# uses first sig - correct
return1 = ActiveSupport::Notifications.subscribe("some_event.gem_name") do |_name, start, finish, _id, _payload|
  puts finish - start
end

# uses first sig - not correct
return2 = ActiveSupport::Notifications.subscribe("some_event.gem_name") do |event|
  puts event.duration
end

# uses third sig - correct
callback1 = ->(_name, start, finish, _id, _payload) { puts finish - start }
return3 = ActiveSupport::Notifications.subscribe("some_event.gem_name", callback1)

# uses fourth sig - correct
callback2 = ->(event) { puts event.duration }
return4 = ActiveSupport::Notifications.subscribe("some_event.gem_name", callback2)

Observed output

# uses first sig - not correct
return2 = ActiveSupport::Notifications.subscribe("some_event.gem_name") do |event|
  # event is typed as String from the first sig, but should be
  # ActiveSupport::Notifications::Event from the second sig
  puts event.duration
end

Expected behavior

See above snippet comments.

EDIT: I understand support for overloaded methods is limited, but I didn't see this particular limitation mentioned on https://sorbet.org/docs/overloads#restrictions-on-overloaded-methods

@bdewater bdewater added bug Something isn't working unconfirmed This issue has not yet been confirmed by the Sorbet team labels Mar 4, 2024
@jez
Copy link
Collaborator

jez commented Mar 4, 2024

I have a prototype open for this in #7741

I can probably split the changes to overload resolution out into their own change, because inferring types for lambda return values causes a few thousand new type errors on Stripe's codebase and will need some time before we can fix them.

@bdewater
Copy link
Contributor Author

bdewater commented Mar 5, 2024

That would be great, thanks @jez !

@jez jez linked a pull request Apr 25, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working unconfirmed This issue has not yet been confirmed by the Sorbet team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants