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

ruby27: Support forwarding arguments syntax #3420

Merged
merged 5 commits into from
Dec 16, 2020

Conversation

Morriar
Copy link
Collaborator

@Morriar Morriar commented Sep 14, 2020

Motivation

Ruby 2.7 introduced the argument forwarding syntax:

def foo(...)
  target(...)
end

And if I'm following this documentation correctly, it would be comparable to...

def foo(*args, **kwargs, &block)
  target(*args, **kwargs, &block)
end

This PR imports the whitequark tests, updates the parser and introduces desugaring/resolving support for the forwarding syntax.

Approach

  1. During parsing we store ... calls as a FORWARD(ED)_ARG node in the AST:

      DefMethod {
        name = <U foo>
        args = ForwardArgs {
        }
        body = Send {
          receiver = NULL
          method = <U bar>
          args = [
            ForwardedArgs {
            }
          ]
        }
      }
    
  2. During desugar the ... node are transformed into *args, **kwargs, &block:

    def foo<<C <todo sym>>>(*<args>, **<kargs>:, &<blk>)
      ::<Magic>.<call-with-splat-and-block>(<self>, :"bar", ::T.unsafe(<args>).to_a().concat([<kargs>.to_hash()]), <blk>)
    end
    
  3. During resolve, nothing change regarding local variables and arguments resolution:

        MethodDef{
          flags = {}
          name = <U foo><<U foo>>
          args = [Local{
              localVariable = <U <args>>
            }, Local{
              localVariable = <U <kargs>>
            }, Local{
              localVariable = <U <blk>>
            }]
    

    But if a signature if provided we check that it contains the right parameters:

    sig do
     params(
       x: T.untyped
     ).returns(String)
    end # error: Malformed `sig` for argument forwarding. Signature must declare exactly `3` parameters named `args`, `kargs` and `block`
    def foo(...)
    end

    Then rename the args to <args> to match the desugared signature (and avoid someone using the args when it's not really declared:

    sig do
      params(
        args: Integer,
        kargs: T::Hash[Symbol, String],
        block: T.proc.returns(Integer)
      ).void
    end
    def foo(...)
      puts args # error: Method `args` does not exist on `Foo`
      puts kargs # error: Method `kargs` does not exist on `Foo`
      puts block # error: Method `block` does not exist on `Foo`
      target(...)
    end

Test plan

See included automated tests.

@Morriar Morriar requested a review from a team as a code owner September 14, 2020 20:08
@Morriar Morriar requested review from elliottt and removed request for a team September 14, 2020 20:08
@jez jez self-requested a review September 14, 2020 20:11
@Morriar Morriar mentioned this pull request Sep 14, 2020
@jez jez removed their request for review November 13, 2020 06:11
@Morriar
Copy link
Collaborator Author

Morriar commented Nov 27, 2020

Rebased on master

core/tools/generate_names.cc Outdated Show resolved Hide resolved
ast/desugar/Desugar.cc Outdated Show resolved Hide resolved
resolver/resolver.cc Outdated Show resolved Hide resolved
Copy link
Collaborator

@jez jez left a comment

Choose a reason for hiding this comment

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

The parser and desugar changes look great. I think we'd like to omit the resolver & sig changes for now.

@Morriar
Copy link
Collaborator Author

Morriar commented Dec 7, 2020

Removed the resolver part but left the test to show the current behaviour.

As you can see in test/testdata/resolver/forward_args.rb test, not handling the signature makes things even more confusing.

@Morriar Morriar requested a review from jez December 7, 2020 22:06
@jez
Copy link
Collaborator

jez commented Dec 8, 2020

Removed the resolver part but left the test to show the current behaviour.

As you can see in test/testdata/resolver/forward_args.rb test, not handling the signature makes things even more confusing.

Sorry for not being clear. My suggestion was that we keep desugaring to *<args>, **<kwargs>, &<blk> (i.e., with those unwritable names) so that it isn't possible to reference those arguments as local variables in the body of a method, and then we add some code to prevent people from writing a signature when we detect that the method definition came from a forward_args node.

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
This commit tracks upstream commit ruby/ruby@95f7992.

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@Morriar
Copy link
Collaborator Author

Morriar commented Dec 14, 2020

Oh sorry for the confusion!

I added an error if you try to provide a signature for a method declared with argument forwarding syntax:

sig { void } # error:  Unsupported `sig` for argument forwarding syntax. Rewrite the method as `def A#foo(*args, **kwargs, &blk)` to use a signature
def foo(...); end

It's less confusing that way 👍

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)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice i like these names!

resolver/resolver.cc Outdated Show resolved Hide resolved
@jez
Copy link
Collaborator

jez commented Dec 16, 2020

Thanks so much for being patient through all the back and forth!

@jez jez merged commit 007e64f into sorbet:master Dec 16, 2020
@Morriar Morriar deleted the at-ruby27-forward-args branch March 3, 2022 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants