Add T.bind for changing bindings inside procs and lambdas #4170

vinistock · 2021-04-22T18:21:44Z

This PR introduces T.bind outside of the context of method signatures to allow users to bind procs and lambdas to different types. This is especially useful in the Rails world, where a lot of procs are executed with instance_exec.

For example,

class Post < ApplicationRecord
  # In callbacks from ActiveRecord, ActiveModel and Actionpack, `if` and `unless` options
  # can be passed as lambdas to determine whether it should run or not. These keys are part
  # of keyword arguments and the block is captured and then executed in the context of the instance.
  #
  # Sorbet doesn't know that it will be executed in an instance context and errors with
  # "method `should_do_it?` is not defined for T.class_of(Post)
  before_create :do_something, if: -> { should_do_it? }

  private

  def should_do_it?
    true
  end
end

With the changes in this PR, users can indicate the correct binding inside each proc, guaranteeing that type checking is verifying against the right types. Below is an example usage and more examples can be found in the tests included.

class Post < ApplicationRecord
  # Proc is now bound to `Post` and not `T.class_of(Post)`. No errors!
  before_create :do_something, if: -> { T.bind(self, Post); should_do_it? }

  private

  def should_do_it?
    true
  end
end

Motivation

Sorbet currently does not allow indicating the context in which a captured block will run, which limits typing in common Rails use cases, such as the previously mentioned callbacks, but also active support concerns.

Test plan

The included tests display examples for concerns and callbacks.

infer/environment.cc

test/testdata/bound_proc.rb

gems/sorbet-runtime/test/types/configuration.rb

gems/sorbet-runtime/lib/types/_types.rb

infer/environment.cc

test/testdata/bound_proc.rb

Morriar · 2021-04-22T21:22:01Z

I think this PR is also providing a solution for #230?

vinistock · 2021-04-28T13:43:42Z

Updated the PR description to also close #230, which indeed will be fixed by this.

@jez should I add documentation about T.bind to the website code in this PR or in a subsequent one?

jez · 2021-04-28T18:49:22Z

@jez should I add documentation about T.bind to the website code in this PR or in a subsequent one?

I prefer in this PR, but if you wanted to do it in a follow up PR I wouldn't block this PR on it.

cfg/builder/builder_walk.cc

gems/sorbet-runtime/lib/types/_types.rb

gems/sorbet-runtime/test/types/configuration.rb

jez

This looks great, thanks for those changes!

Some small suggested changes, and then waiting to hear back w.r.t. whether you want to do docs in this PR or another. I can merge as is or wait for the docs, just let me know.

vinistock · 2021-04-28T20:19:58Z

Cool! I'll add the documentation to this PR and address the remaining comments, so that we can ship it all in one go.

Co-authored-by: Jake Zimmerman <zimmerman.jake@gmail.com>

vinistock · 2021-04-29T18:52:18Z

Alright, added website documentation. Let me know if the copy looks good!

jez · 2021-04-29T21:11:00Z

@vinistock Does this actually close #4095?

That issue requests a static error when using T.proc.bind on a non-block argument.

This PR implements an alternative for people who think that they want the behavior they imagine would be possible with T.proc.bind on a non-block argument.

I don't think we should expand the scope of this PR, but I am going to mark this PR as not closing that issue.

jez · 2021-04-29T22:02:22Z

We have a policy of testing changes to Sorbet against Stripe's codebase before
merging them. I've kicked off a test run for the current PR. When the build
finishes, I'll share with you whether or how it failed. Thanks!

Stripe employees can see the build results here:

→ https://go/builds/bui_JOUwqSiKKgp6Fd
→ https://go/builds/bui_JOUwYlCyr4Mq8r

jez · 2021-04-29T23:34:31Z

Thanks!

vinistock requested a review from a team as a code owner April 22, 2021 18:21

vinistock requested review from jez and removed request for a team April 22, 2021 18:21