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

Don't use named argument key names as parameters for method_missing calls #10388

Merged

Conversation

HertzDevil
Copy link
Contributor

Resolves #10381 through #10381 (comment).

method_missing is used in the standard library only by forward_missing_to, which in turn is used only by HTTP::Headers. The specs for it don't exercise this capability beyond calling HTTP::Headers#same?, which can be rewritten without any implicit delegation, and which doesn't have named arguments anyway (it uses #object_id). So the standard library most certainly won't break, but external libraries that refer to call arguments using {{ call.named_args[...].name }} will.

@asterite
Copy link
Member

Thank you! ❤️

If it isn't too much work... could you briefly explain what does this change and why? I didn't have time to follow the entire conversation in #10381 which covers a lot of topics.

I guess this is also a breaking change?

From the description I still don't understand why one would need to change name to value when name was working fine before.

@HertzDevil
Copy link
Contributor Author

The PR makes the named parameters of method_missing's generated defs use _named_arg#{i} instead of copying them from the key names.

The main reasons are:

  • A named parameter's external name doesn't need to be a valid identifier, but its internal one must be:
    def foo(*, "a b" a b); end         # unusable inside `method_missing`
    def foo(*, "a b" _named_arg0); end # okay
    
    foo("a b": 0)
  • Allowing implicit access to the named arguments by name can potentially shadow other calls:
    class Foo
      def to
        1
      end
    
      forward_missing_to to
    end
    
    Foo.new.upto(to: 3).to_a # => [3]
    Foo.new.upto(3).to_a     # => [1, 2, 3]
    If the internal parameter names for the generated def start with an underscore, as the positional and block parameters already do, then the chances of collision are greatly reduced because these names are reserved by Crystal.
  • Ruby's method_missing also accesses the call's named arguments through a double splat parameter, and that parameter's values, not keys, are the actual arguments.

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic topic:lang:macros labels Feb 14, 2021
@asterite asterite added this to the 1.0.0 milestone Feb 17, 2021
@asterite asterite merged commit a65a905 into crystal-lang:master Feb 17, 2021
@asterite
Copy link
Member

@HertzDevil Thank you again! 💙

@HertzDevil HertzDevil deleted the bug/method-missing-named-params branch February 18, 2021 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic topic:lang:macros
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NamedArgument.name == NamedArgument.value in method_missing
3 participants