ruby27: Introduce numbered parameters support #3344

Morriar · 2020-08-04T16:54:38Z

Motivation

In block without explicitly specified parameters, variables _1 through _9 can be used to reference parameters.

With numbered parameters, this snippet:

filenames.each { |f| File.read(f) }

Is equivalent to

filenames.each { File.read(_1) }

This pull request introduces full support for numbered parameters in Sorbet:

[1, 2, 3].map { T.reveal_type(_1) } # Revealed type: `Integer`

["albatross", "dog", "horse"].max(2) do
  T.reveal_type(_1) # Revealed type: `String`
  T.reveal_type(_2) # Revealed type: `String`
  _1.length <=> _2.length
end

See the tests for more usage examples.

Implementation

During parsing blocks containing identifiers _1 to _9 are created as NumBlock instead of Block and we keep a stack of the numbers used.

During desugar, the NumBlock is translated into an ordinary block. Implicit parameters are added to the block args from _1 to the max number used.

At this point, Sorbet can work its magic on the block and perform the typing.

Test plan

See included automated tests.

test/whitequark/test_numbered_args_after_27_0.parse-tree-whitequark.exp

third_party/parser/cc/lexer.rl

third_party/parser/include/ruby_parser/driver.hh

parser/Builder.cc

third_party/parser/cc/grammars/typedruby.ypp

parser/Builder.cc

test/whitequark/test_assignment_to_numparams_0.rb

third_party/parser/codegen/generate_diagnostics.cc

parser/Builder.cc

ast/desugar/Desugar.cc

test/testdata/desugar/numbered_parameters.rb.desugar-tree.exp

jez · 2020-09-10T00:35:17Z

This looks really good. I'm done leaving comments for now. Feel free to re-request review when you want another look.

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

…cks and lambdas Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

jez · 2020-09-10T22:31:01Z

third_party/parser/include/ruby_parser/driver.hh

+// For each scope we save the highest numparam used (or 0) and all the nodes in the
+// scope referencing a numparam.
+//
+// The stack as 3 states:


Suggested change

// The stack as 3 states:

// The stack has 3 states:

jez · 2020-09-10T22:33:54Z

ast/desugar/Desugar.cc

+        if (auto *lvar = parser::cast_node<parser::LVar>(decl.get())) {
+            if (numparamNum(dctx, decl.get()) == num) {
+                return MK::Local(lvar->loc, lvar->name);
+            }
+        }


Should this be an ENFORCE if this cast ever fails?

(i.e., it's fine for it to fallback to the dummy MK::Local if this search fails to find something so that sorbet doesn't crash, but would that be an expected situation or a bug in sorbet?)

oh i see, we might be making locals for like _1 and _2 only because we saw _3 in the body. makes sense, i see that the comment is getting at that now.

jez

Thanks!

jez · 2020-09-10T22:38:30Z

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 result here:

→ https://go/builds/bui_HzyMorejlXmxQ4

Morriar requested a review from a team as a code owner August 4, 2020 16:54

Morriar requested review from DarkDimius and removed request for a team August 4, 2020 16:54

Morriar mentioned this pull request Aug 4, 2020

Support 2.7 syntax #2900

Closed

Morriar force-pushed the at-ruby27-numparams branch 2 times, most recently from 5d5c331 to 6644684 Compare August 4, 2020 17:39

Morriar force-pushed the at-ruby27-numparams branch from 6644684 to 4aed149 Compare August 20, 2020 18:38