8000 [RFC] Do not error on `**kwargs` parameters name mismatch by Morriar · Pull Request #5142 · sorbet/sorbet · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[RFC] Do not error on **kwargs parameters name mismatch #5142

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 17, 2022
Merged

[RFC] Do not error on **kwargs parameters name mismatch #5142

merged 2 commits into from
Mar 17, 2022

Conversation

Morriar
Copy link
Collaborator
@Morriar Morriar commented Jan 21, 2022

Motivation

When Sorbet is checking methods redefinitions, it errors on mismatching names for keyword arguments and **kwargs:

# typed: true
extend T::Sig

sig { params(arg: T.untyped).void}
def m1(arg); end

sig { params(argX: T.untyped).void}
def m1(argX); end # ok

sig { params(arg: T.untyped).void}
def m2(*arg); end

sig { params(argX: T.untyped).void}
def m2(*argX); end # ok

sig { params(block: T.untyped).void}
def m3(&block); end

sig { params(blockX: T.untyped).void}
def m3(&blockX); end # ok

sig { params(kwarg: T.untyped).void}
def m4(kwarg:); end

sig { params(kwarg1: T.untyped).void}
def m4(kwarg1:); end # error: Method `Object#m4` redefined with mismatched keyword argument name. Expected: `kwarg`, got: `kwarg1`

sig { params(kwargs: T.untyped).void}
def m5(**kwargs); end

sig { params(kwargs1: T.untyped).void}
def m5(**kwargs1); end # error: Method `Object#m5` redefined with mismatched keyword argument name. Expected: `kwargs`, got: `kwargs1`

While this verification makes sense for keyword arguments (they should be the same in all the definitions of the same methods), I'm not sure it applies to **kwargs.

Consider the following case:

# sorbet/rbi/gems/foo.rbi (generated)

class View
  sig { params(kwargs: T.untyped).void }
  def render(**kwargs); end
end
# sorbet/rbi/shims/foo.rbi (manually written shim)

class View
  sig { params(options: T::Hash[Symbol, String]).void }
  def render(**options); end
end

The renaming in the shim file should be allowed as it still is compatible with the generated definition for View#render.

Implementation

Added a possible implementation for this change to see what else would break.

Test plan

See included automated tests.

@Morriar Morriar requested a review from a team as a code owner January 21, 2022 18:17
@Morriar Morriar requested review from elliottt and removed request for a team January 21, 2022 18:17
@Morriar Morriar changed the title [RFC] Do not error **kwargs parameters name mismatch [RFC] Do not error on **kwargs parameters name mismatch Jan 21, 2022
@jez
Copy link
Collaborator
jez commented Jan 25, 2022

At first I misread this PR, thinking that you were attempting to make

def foo(x:, y:); end
def foo(**kwargs); end

not report an error. That doesn't seem to be the case, and I'm generally on board with this idea now knowing what it really does.

There's a problem with your implementation:

❯ sorbet -p symbol-table-raw foo.rb
class <C <U <root>>> < <C <U Object>> ()
  class <S <C <U <root>>> $1>[<C <U <AttachedClass>>>] < <S <C <U Object>> $1> ()
    method <S <C <U <root>>> $1>#<N <U <static-init>> $152> (<blk>) @ Loc {file=foo.rb start=3:1 end=6:4}
      argument <blk><block> @ Loc {file=foo.rb start=??? end=???}
  class <C <U A>> < <C <U Object>> () @ Loc {file=foo.rb start=3:1 end=3:8}
    method <C <U A>>#<M <U foo> $1> (args, <blk>) @ Loc {file=foo.rb start=4:3 end=4:18}
      argument args<keyword, repeated> @ Loc {file=foo.rb start=4:11 end=4:17}
      argument <blk><block> @ Loc {file=foo.rb start=??? end=???}
    method <C <U A>>#<U foo> (argv, <blk>) @ Loc {file=foo.rb start=5:3 end=5:18}
      argument argv<keyword, repeated> @ Loc {file=foo.rb start=5:11 end=5:17}
      argument <blk><block> @ Loc {file=foo.rb start=??? end=???}

You're just papering over the symptoms, but not actually addressing the root cause.

What we see in this symbol-table output is that there are two foo methods defined on A:

  • method <C <U A>>#<M <U foo> $1>
  • method <C <U A>>#<U foo>

The <M ...>-style name is a mangle-rename name kind. That happens any time that mangleRenameSymbol is called. Despite not reporting an error, we're still mangling the old symbol out of the way so that we can re-enter a new method, with different argument names.

We have solved a similar problem in the past for positional args: you'll notice that

def foo(x); end
def foo(y); end

reports no error. The way that this is done is that all positional args are given a name of the form arg$<n> where <n> is the index of the positional arg:

sorbet/namer/namer.cc

Line 851 in 3cd1ae1

name = ctx.state.freshNameUnique(core::UniqueNameKind::PositionalArg, core::Names::arg(), pos + 1);

The original argument name is not actually stored in in the symbol table—the only time it's ever needed is for error messages, and for that, we use a clever trick: we record the Loc for the arg, and define ArgInfo::argumentName to read the source file under that Loc when printing argument names:

sorbet/core/Symbols.cc

Lines 1571 to 1573 in 3cd1ae1

// positional arg
if (auto source = loc.source(gs)) {
return source.value();

We should do something similar with **kwargs—canonicalize their name in the symbol table to something like <kwargs> (with a well-known name like core::Names::kwargs()), and use the Loc on the ArgInfo when printing the name. That way, the code path that you're modifying here wouldn't even get hit.

10000
jez
jez requested changes Jan 25, 2022
View reviewed changes
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.

generally on board.

@jez jez removed the request for review from elliottt January 25, 2022 01:19
@Morriar Morriar force-pushed the at-params-names branch 2 times, most recently from ef07189 to 54ae15f Compare January 25, 2022 21:16
@Morriar
Do not error on **kwargs parameters name mismatch
26de81c 
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@froydnj froydnj requested a review from jez March 16, 2022 14:22
jez
jez approved these changes Mar 17, 2022
View reviewed changes
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.

beautiful, thanks!

parser/Builder.cc
@@ -1085,7 +1085,7 @@ class Builder::Impl {
core::NameRef nm;

if (name != nullptr) {
loc = loc.join(tokLoc(name));
loc = tokLoc(name);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a minor thing, but instead of overwriting loc in just this case, do you mind changing it so that loc is not given an initial value, and instead it's written to once in each of the if branches?

Copy link
Collaborator

Choose a reason for hiding this comment

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

i went ahead and did this. it'll merge when the tests pass.

@jez
Copy link
Collaborator
jez commented Mar 17, 2022

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_LKnzZEzNmX3HKm
→ https://go/builds/bui_LKnzzMKJ5GjIhf

@jez jez enabled auto-merge (squash) March 17, 2022 02:05
@jez jez merged commit f72aeb0 into sorbet:master Mar 17, 2022
@Morriar Morriar deleted the at-params-names branch March 17, 2022 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jez jez jez approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Morriar @jez
0