Update argument forwarding syntax (#4247)

* Update imported tests for forward arguments

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Co-authored-by: Vinicius Stock <vinicius.stock@shopify.com>

* Update parser for new forward arguments syntax

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Co-authored-by: Vinicius Stock <vinicius.stock@shopify.com>

* Update desugar for new forward arguments syntax

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Co-authored-by: Vinicius Stock <vinicius.stock@shopify.com>

* Update resolver for new forward argument syntax

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Co-authored-by: Vinicius Stock <vinicius.stock@shopify.com>

Co-authored-by: Vinicius Stock <vinicius.stock@shopify.com>

ast/desugar/Desugar.cc

@@ -96,6 +96,18 @@ pair<MethodDef::ARGS_store, InsSeq::STATS_store> desugarArgs(DesugarContext dctx
                 destructures.emplace_back(node2TreeImpl(dctx, std::move(destructure)));
             } else if (auto *lhs = parser::cast_node<parser::Kwnilarg>(arg.get())) {
                 // TODO implement logic for `**nil` args
+            } else if (auto *fargs = parser::cast_node<parser::ForwardArg>(arg.get())) {
+                // we desugar (m, n, ...) into (m, n, *<fwd-args>, **<fwd-kwargs>, &<fwd-block>)
+                // add `*<fwd-args>`
+                unique_ptr<parser::Node> rest =
+                    make_unique<parser::Restarg>(fargs->loc, core::Names::fwdArgs(), fargs->loc);
+                args.emplace_back(node2TreeImpl(dctx, std::move(rest)));
+                // add `**<fwd-kwargs>`
+                unique_ptr<parser::Node> kwrest = make_unique<parser::Kwrestarg>(fargs->loc, core::Names::fwdKwargs());
+                args.emplace_back(node2TreeImpl(dctx, std::move(kwrest)));
+                // add `&<fwd-block>`
+                unique_ptr<parser::Node> block = make_unique<parser::Blockarg>(fargs->loc, core::Names::fwdBlock());
+                args.emplace_back(node2TreeImpl(dctx, std::move(block)));
             } else {
                 args.emplace_back(node2TreeImpl(dctx, std::move(arg)));
             }
@@ -106,18 +118,6 @@ pair<MethodDef::ARGS_store, InsSeq::STATS_store> desugarArgs(DesugarContext dctx
         for (int i = 1; i <= numparamMax(dctx, &numparams->decls); i++) {
             args.emplace_back(numparamTree(dctx, i, &numparams->decls));
         }
-    } else if (auto *fargs = parser::cast_node<parser::ForwardArgs>(argnode.get())) {
-        // we desugar (...) into (*<fwd-args>, **<fwd-kwargs>, &<fwd-block>)
-        args.reserve(3);
-        // add `*<fwd-args>`
-        unique_ptr<parser::Node> rest = make_unique<parser::Restarg>(fargs->loc, core::Names::fwdArgs(), fargs->loc);
-        args.emplace_back(node2TreeImpl(dctx, std::move(rest)));
-        // add `**<fwd-kwargs>`
-        unique_ptr<parser::Node> kwrest = make_unique<parser::Kwrestarg>(fargs->loc, core::Names::fwdKwargs());
-        args.emplace_back(node2TreeImpl(dctx, std::move(kwrest)));
-        // add `&<fwd-block>`
-        unique_ptr<parser::Node> block = make_unique<parser::Blockarg>(fargs->loc, core::Names::fwdBlock());
-        args.emplace_back(node2TreeImpl(dctx, std::move(block)));
     } else if (argnode.get() == nullptr) {
         // do nothing
     } else {
@@ -629,7 +629,10 @@ ExpressionPtr node2TreeImpl(DesugarContext dctx, unique_ptr<parser::Node> what)
                     flags.isPrivateOk = true;
                 }
 
-                if (absl::c_any_of(send->args, [](auto &arg) { return parser::isa_node<parser::Splat>(arg.get()); })) {
+                if (absl::c_any_of(send->args, [](auto &arg) {
+                        return parser::isa_node<parser::Splat>(arg.get()) ||
+                               parser::isa_node<parser::ForwardedArgs>(arg.get());
+                    })) {
                     // Build up an array that represents the keyword args for the send. When there is a Kwsplat, treat
                     // all keyword arguments as a single argument.
                     unique_ptr<parser::Node> kwArray;
@@ -693,9 +696,37 @@ ExpressionPtr node2TreeImpl(DesugarContext dctx, unique_ptr<parser::Node> what)
                         argnodes.erase(it);
                     }
 
-                    auto array = make_unique<parser::Array>(loc, std::move(argnodes));
+                    auto hasFwdArgs = false;
+                    auto fwdIt = absl::c_find_if(
+                        argnodes, [](auto &arg) { return parser::isa_node<parser::ForwardedArgs>(arg.get()); });
+                    if (fwdIt != argnodes.end()) {
+                        block = make_unique<parser::LVar>(loc, core::Names::fwdBlock());
+                        hasFwdArgs = true;
+                        argnodes.erase(fwdIt);
+                    }
 
+                    auto array = make_unique<parser::Array>(loc, std::move(argnodes));
                     auto args = node2TreeImpl(dctx, std::move(array));
+
+                    if (hasFwdArgs) {
+                        auto fwdArgs = MK::Local(loc, core::Names::fwdArgs());
+                        auto argsSplat = MK::Send0(loc, std::move(fwdArgs), core::Names::toA());
+                        auto argsConcat = MK::Send1(loc, std::move(args), core::Names::concat(), std::move(argsSplat));
+
+                        auto fwdKwargs = MK::Local(loc, core::Names::fwdKwargs());
+                        auto kwargsSplat = MK::Send1(loc, MK::Constant(loc, core::Symbols::Magic()),
+                                                     core::Names::toHashDup(), std::move(fwdKwargs));
+
+                        Array::ENTRY_store kwargsEntries;
+                        kwargsEntries.emplace_back(std::move(kwargsSplat));
+                        auto kwargsArray = MK::Array(loc, std::move(kwargsEntries));
+
+                        argsConcat =
+                            MK::Send1(loc, std::move(argsConcat), core::Names::concat(), std::move(kwargsArray));
+
+                        args = std::move(argsConcat);
+                    }
+
                     auto kwargs = node2TreeImpl(dctx, std::move(kwArray));
                     auto method =
                         MK::Literal(loc, core::make_type<core::LiteralType>(core::Symbols::Symbol(), send->method));
@@ -726,42 +757,6 @@ ExpressionPtr node2TreeImpl(DesugarContext dctx, unique_ptr<parser::Node> what)
                         }
                     }
                     result = std::move(res);
-                } else if (send->args.size() == 1 && parser::isa_node<parser::ForwardedArgs>(send->args.back().get())) {
-                    // If the call is forwarding arguments like `foo(...)`
-                    // we desugar it as `foo(<fwd-args>, **<fwd-kwargs>, &<fwd-block>)`
-
-                    auto method =
-                        MK::Literal(loc, core::make_type<core::LiteralType>(core::Symbols::Symbol(), send->method));
-
-                    Send::ARGS_store sendArgs;
-                    sendArgs.emplace_back(std::move(rec));
-                    sendArgs.emplace_back(std::move(method));
-
-                    auto args = MK::Local(loc, core::Names::fwdArgs());
-                    auto argsSplat = MK::Send0(loc, std::move(args), core::Names::toA());
-
-                    auto kwargsSplat = MK::Send1(loc, MK::Constant(loc, core::Symbols::Magic()),
-                                                 core::Names::toHashDup(), MK::Local(loc, core::Names::fwdKwargs()));
-
-                    Array::ENTRY_store kwargsEntries;
-                    kwargsEntries.emplace_back(std::move(kwargsSplat));
-                    auto kwargsArray = MK::Array(loc, std::move(kwargsEntries));
-
-                    auto argsConcat =
-                        MK::Send1(loc, std::move(argsSplat), core::Names::concat(), std::move(kwargsArray));
-                    sendArgs.emplace_back(std::move(argsConcat));
-
-                    auto kwArray = make_unique<parser::Nil>(loc);
-                    auto kwArgs = node2TreeImpl(dctx, std::move(kwArray));
-                    sendArgs.emplace_back(std::move(kwArgs));
-
-                    auto blk = MK::Local(loc, core::Names::fwdBlock());
-                    sendArgs.emplace_back(std::move(blk));
-
-                    auto send = MK::Send(loc, MK::Constant(loc, core::Symbols::Magic()),
-                                         core::Names::callWithSplatAndBlock(), 4, std::move(sendArgs), {});
-
-                    result = std::move(send);
                 } else {
                     int numPosArgs = send->args.size();
                     if (numPosArgs > 0) {

parser/Builder.cc

@@ -754,8 +754,8 @@ class Builder::Impl {
                                 std::move(body));
     }
 
-    unique_ptr<Node> forward_args(const token *begin, const token *dots, const token *end) {
-        return make_unique<ForwardArgs>(tokLoc(begin).join(tokLoc(dots)).join(tokLoc(end)));
+    unique_ptr<Node> forward_arg(const token *begin, const token *dots, const token *end) {
+        return make_unique<ForwardArg>(tokLoc(dots));
     }
 
     unique_ptr<Node> forwarded_args(const token *dots) {
@@ -1797,9 +1797,9 @@ ForeignPtr for_(SelfPtr builder, const token *for_, ForeignPtr iterator, const t
                                         build->cast_node(body), end));
 }
 
-ForeignPtr forward_args(SelfPtr builder, const token *begin, const token *dots, const token *end) {
+ForeignPtr forward_arg(SelfPtr builder, const token *begin, const token *dots, const token *end) {
     auto build = cast_builder(builder);
-    return build->toForeign(build->forward_args(begin, dots, end));
+    return build->toForeign(build->forward_arg(begin, dots, end));
 }
 
 ForeignPtr forwarded_args(SelfPtr builder, const token *dots) {
@@ -2312,7 +2312,7 @@ struct ruby_parser::builder Builder::interface = {
     float_,
     floatComplex,
     for_,
-    forward_args,
+    forward_arg,
     forwarded_args,
     gvar,
     hash_pattern,

parser/tools/generate_ast.cc

@@ -273,8 +273,8 @@ NodeDef nodes[] = {
     },
     // "..." argument forwarding in definition site
     {
-        "ForwardArgs",
-        "forward_args",
+        "ForwardArg",
+        "forward_arg",
         vector<FieldDef>(),
     },
     // "..." argument forwarding in call site

resolver/resolver.cc

@@ -2542,12 +2542,13 @@ class ResolveSignaturesWalk {
         auto methodInfo = method.data(ctx);
 
         // Is this a signature for a method defined with argument forwarding syntax?
-        if (methodInfo->arguments().size() == 3) {
-            // To match, the definition must have been desugared in exaclty 3 parameters named
+        if (methodInfo->arguments().size() >= 3) {
+            // To match, the definition must have been desugared with at least 3 parameters named
             // `<fwd-args>`, `<fwd-kwargs>` and `<fwd-block>`
-            auto l1 = getArgLocal(ctx, methodInfo->arguments()[0], mdef, 0, isOverloaded)->localVariable;
-            auto l2 = getArgLocal(ctx, methodInfo->arguments()[1], mdef, 1, isOverloaded)->localVariable;
-            auto l3 = getArgLocal(ctx, methodInfo->arguments()[2], mdef, 2, isOverloaded)->localVariable;
+            auto len = methodInfo->arguments().size();
+            auto l1 = getArgLocal(ctx, methodInfo->arguments()[len - 3], mdef, len - 3, isOverloaded)->localVariable;
+            auto l2 = getArgLocal(ctx, methodInfo->arguments()[len - 2], mdef, len - 2, isOverloaded)->localVariable;
+            auto l3 = getArgLocal(ctx, methodInfo->arguments()[len - 1], mdef, len - 1, isOverloaded)->localVariable;
             if (l1._name == core::Names::fwdArgs() && l2._name == core::Names::fwdKwargs() &&
                 l3._name == core::Names::fwdBlock()) {
                 if (auto e = ctx.beginError(exprLoc, core::errors::Resolver::InvalidMethodSignature)) {

test/testdata/desugar/forward_args.rb

@@ -10,6 +10,14 @@ def foo(...)
   def foo2(*args, **kwargs, &block)
     T.unsafe(self).bar(*args, **kwargs, &block)
   end
+
+  def foo3(...)
+    T.unsafe(self).bar(*[1, 2, 3], ...)
+  end
+
+  def foo4(*args, **kwargs, &block)
+    T.unsafe(self).bar(*[1, 2, 3], *args, **kwargs, &block)
+  end
 end
 
 Foo.new.foo

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

@@ -5,7 +5,7 @@ class <emptyTree><<C <root>>> < (::<todo sym>)
     end
 
     def foo<<todo method>>(*<fwd-args>, *<fwd-kwargs>:, &<fwd-block>)
-      ::<Magic>.<call-with-splat-and-block>(<emptyTree>::<C T>.unsafe(<self>), :bar, <fwd-args>.to_a().concat([::<Magic>.<to-hash-dup>(<fwd-kwargs>)]), nil, <fwd-block>)
+      ::<Magic>.<call-with-splat-and-block>(<emptyTree>::<C T>.unsafe(<self>), :bar, [].concat(<fwd-args>.to_a()).concat([::<Magic>.<to-hash-dup>(<fwd-kwargs>)]), nil, <fwd-block>)
     end
 
     def foo2<<todo method>>(*args, *kwargs:, &block)
@@ -14,6 +14,17 @@ class <emptyTree><<C <root>>> < (::<todo sym>)
               <hashTemp>$2
             end]), nil, block)
     end
+
+    def foo3<<todo method>>(*<fwd-args>, *<fwd-kwargs>:, &<fwd-block>)
+      ::<Magic>.<call-with-splat-and-block>(<emptyTree>::<C T>.unsafe(<self>), :bar, ::<Magic>.<splat>([1, 2, 3]).concat(<fwd-args>.to_a()).concat([::<Magic>.<to-hash-dup>(<fwd-kwargs>)]), nil, <fwd-block>)
+    end
+
+    def foo4<<todo method>>(*args, *kwargs:, &block)
+      ::<Magic>.<call-with-splat-and-block>(<emptyTree>::<C T>.unsafe(<self>), :bar, ::<Magic>.<splat>([1, 2, 3]).concat(::<Magic>.<splat>(args)).concat([begin
+              <hashTemp>$2 = ::<Magic>.<to-hash-dup>(kwargs)
+              <hashTemp>$2
+            end]), nil, block)
+    end
   end
 
   <emptyTree>::<C Foo>.new().foo()

test/testdata/resolver/forward_args.rb

@@ -71,6 +71,24 @@ def foo_fwd3(...)
     puts kargs # error: Method `kargs` does not exist on `Foo`
     puts block # error: Method `block` does not exist on `Foo`
   end
+
+  sig do
+    params(
+      a: T.untyped,
+      b: T::Hash[T.untyped, T.untyped],
+      c: T.untyped
+    ).void
+  end # error: Unsupported `sig` for argument forwarding syntax
+  def foo_fwd4(a, b, c, ...)
+    puts a, b, c
+  end
+
+  def foo_fwd5(a, b, c, ...)
+    T.reveal_type(a) # error: Revealed type: `T.untyped`
+    T.reveal_type(b) # error: Revealed type: `T.untyped`
+    T.reveal_type(c) # error: Revealed type: `T.untyped`
+    d = T.unsafe(self).bar(a, b, c, ...)
+  end
 end
 
 Foo.new.foo_fwd

test/whitequark/test_forward_args_0.parse-tree-whitequark.exp → test/whitequark/test_forward_arg_0.parse-tree-whitequark.exp

@@ -1,4 +1,5 @@
 s(:def, :foo,
-  s(:forward_args),
+  s(:args,
+    s(:forward_arg)),
   s(:send, nil, :bar,
     s(:forwarded_args)))

test/whitequark/test_forward_args_invalid_4.rb

@@ -0,0 +1,3 @@
+# typed: true
+
+def foo(x,y,z); bar(x, y, z, ...); end # error: unexpected token "..."

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

@@ -0,0 +1,5 @@
+s(:def, :foo,
+  s(:args,
+    s(:forward_arg)),
+  s(:send, nil, :bar,
+    s(:forwarded_args)))

test/whitequark/test_forward_args_legacy_0.rb

@@ -0,0 +1,3 @@
+# typed: true
+
+def foo(...); bar(...); end

test/whitequark/test_forward_args_1.parse-tree-whitequark.exp → test/whitequark/test_forward_args_legacy_1.parse-tree-whitequark.exp

@@ -1,4 +1,5 @@
 s(:def, :foo,
-  s(:forward_args),
+  s(:args,
+    s(:forward_arg)),
   s(:super,
     s(:forwarded_args)))

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

@@ -0,0 +1,3 @@
+s(:def, :foo,
+  s(:args,
+    s(:forward_arg)), nil)

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

@@ -0,0 +1,9 @@
+s(:def, :foo,
+  s(:args,
+    s(:arg, :a),
+    s(:arg, :b),
+    s(:forward_arg)),
+  s(:send, nil, :bar,
+    s(:lvar, :a),
+    s(:int, "42"),
+    s(:forwarded_args)))

test/whitequark/test_trailing_forward_arg_0.rb

@@ -0,0 +1,3 @@
+# typed: true
+
+def foo(a, b, ...); bar(a, 42, ...); end

third_party/parser/cc/grammars/typedruby.ypp

@@ -1289,6 +1289,12 @@ int yylex(parser::semantic_type *lval, ruby_parser::typedruby27 &driver) {
                     {
                       $$ = driver.alloc.delimited_node_list($1, $2, $3);
                     }
+                | tLPAREN2 args tCOMMA args_forward rparen
+                    {
+                      auto forwarded_args = driver.build.forwarded_args(self, $4);
+                      $2->emplace_back(forwarded_args);
+                      $$ = driver.alloc.delimited_node_list($1, $2, $5);
+                    }
                 | tLPAREN2 args_forward rparen
                     {
                       auto forwarded_args = driver.build.forwarded_args(self, $2);
@@ -3075,11 +3081,20 @@ keyword_variable: kNIL
                       driver.lex.set_state_expr_value();
                       $$ = driver.build.args(self, $1, $2, $3, true);
                     }
+                | tLPAREN2 f_arg tCOMMA args_forward rparen
+                    {
+                      driver.lex.declare_forward_args();
+                      auto forward_arg = driver.build.forward_arg(self, nullptr, $4, nullptr);
+                      $2->emplace_back(forward_arg);
+                      $$ = driver.build.args(self, $1, $2, $5, true);
+                    }
                 | tLPAREN2 args_forward rparen
                     {
                       driver.lex.set_state_expr_value();
                       driver.lex.declare_forward_args();
-                      $$ = driver.build.forward_args(self, $1, $2, $3);
+                      auto forward_arg = driver.build.forward_arg(self, $1, $2, $3);
+                      auto node_list = driver.alloc.node_list(forward_arg);
+                      $$ = driver.build.args(self, $1, node_list, $3, true);
                     }
                 |   {
                       $<boolean>$ = driver.lex.in_kwarg;

third_party/parser/include/ruby_parser/builder.hh

@@ -57,7 +57,7 @@ struct builder {
 	ForeignPtr(*float_)(SelfPtr builder, const token* tok);
 	ForeignPtr(*floatComplex)(SelfPtr builder, const token* tok);
 	ForeignPtr(*for_)(SelfPtr builder, const token* for_, ForeignPtr iterator, const token* in_, ForeignPtr iteratee, const token* do_, ForeignPtr body, const token* end);
-	ForeignPtr(*forward_args)(SelfPtr builder, const token* begin, const token* dots, const token* end);
+	ForeignPtr(*forward_arg)(SelfPtr builder, const token* begin, const token* dots, const token* end);
 	ForeignPtr(*forwarded_args)(SelfPtr builder, const token* dots);
 	ForeignPtr(*gvar)(SelfPtr builder, const token* tok);
 	ForeignPtr(*hash_pattern)(SelfPtr builder, const token* begin, const node_list* kwargs, const token* end);