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
Named params implementation #5357
Conversation
a9da175
to
cd8ed72
Compare
This comment has been minimized.
This comment has been minimized.
902339e
to
c457127
Compare
338535f
to
8a2b596
Compare
63357de
to
5d37c34
Compare
ee0d1c7
to
298806e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments
df1f4e0
to
92f330c
Compare
Rather than checking the argument count
If named parameters are used, we never jump over these opcodes.
This opcode is only needed if there are undef args and may be dropped otherwise (even though there are named args).
Separately track the concept of "undef args" and "extra args", to make this more amenable to future optimization.
Use a helper and release the array rather than destroying it, as it may have rc>1 in the future.
Most places now need a custom call_info check, this is not worthwhile.
The frame is freed separately here.
} | ||
|
||
try { | ||
test2(a: 42); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test indicates to me that in test2(...$a)
when we pass in test2(a: 42)
that a is known, but if you var_dump $a
you don't get [42]
you get ['a' => 42]
. That part doesn't make sense to me; it is both known and unknown. In my opinion it's known, so it should just be [42]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current behavior is intended. The variadic is not a proper parameter, just the variable where trailing parameters are collected into. Any other behavior would make forwarding variadics impossible, as there would always be one parameter name that does not get forwarded correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you show me an example of why this is necessary?
Any other behavior would make forwarding variadics impossible, as there would always be one parameter name that does not get forwarded correctly.
Not knowing if $a = [42]
or $a = ['a' => 42]
matters. In all existing code, the first element was guaranteed to be $a[0]
and now it's not. I suspect most people (including me me), used $a[0]
, not [first] = $a
or something. Imagine we are writing a variadic map
like array_map
; part of it would look something like:
if (\count($a) === 1) {
// preserve keys!
foreach ($a[0] as $key => $value) {
yield $key => $transform($value);
}
}
I think this circles back to my raised concern during the RFC process, and I hope there's some piece I'm missing because at the moment this looks very bad to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you show me an example of why this is necessary?
function foo($x = 1, $a = 2) {}
function call($f, ...$a) { $f(...$a); }
call('foo', a: 42);
This would not work if a:
placed 42
inside the variadic parameter... In fact, I'm not even sure how you suggest that to work, do you mean that $a = 42
or that $a = [42]
? Or that the above code is illegal and it must be a: [42]
?
I don't see how parameter forwarding could work if the variadic is treated as a proper parameter label.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect most people (including me me), used $a[0], not [first] = $a or something. Imagine we are writing a variadic map like array_map; part of it would look something like
You'd want $first = reset($a);
[first] = $a
is equivalent to$first = $a[0]
do you mean that $a = 42 or that $a = [42]? Or that the above code is illegal and it must be a: [42]?
I don't see how parameter forwarding could work if the variadic is treated as a proper parameter label.
The forwarding behavior described by nikic here and in the RFC seems reasonable to me.
EDIT: Otherwise, you'd be in a situation where ...$args
is no longer guaranteed to be an array ($a = 42)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: why should forwarding variadics take place over the function itself?
In your example call
has a parameter $a
. In my opinion call('foo', a: 42)
should be wrong, yes. It should be call('foo', a: [42])
because $a
does exist as a parameter in call
. It's absolutely a parameter. You can reflect on it, you can use it directly and its type is an array
. It's a parameter.
Consider a slightly different invocation call('foo', a: [42], x: 13)
. I think on the inside of call
the variables should be $f = 'foo'
and $a = [42, 'x' => 13]
, maybe? The extra args would get merged into the end of the parameter pack if the parameter pack is explicitly given in the named arguments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider a slightly different invocation call('foo', a: [42], x: 13). I think on the inside of call the variables should be $f = 'foo' and
$a = [42, 'x' => 13]
, maybe? The extra args would get merged into the end of the parameter pack if the parameter pack is explicitly given in the named arguments?
For function call($f, ...$a) { $f(...$a); }
What about call('foo', 11, x: 13, a: [0 => 42, 'x' => 14])
- With the implementation that was voted on and merged into 8.0.0beta2, this would have a consistent behavior of $a = [0 => 11, 'x' => 13, 'a' => [0 => 42, 'x' => 14]]
. With alternatives, you'd have to throw or overwrite keys, even if those key names were only specified once as named parameters (x and a) at the call site
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of interest, I checked how two other scripting languages did that. It's the same as what the RFC did, apparently - the parameter names of variadic (named) arguments aren't special cases
python3
>>> my_function(1, 2, 3, myargs = 'v1', myvarargs = 'v2')
(1, 2, 3) {'myargs': 'v1', 'myvarargs': 'v2'}
irb
2.6.3 :001 > def bar(**kwargs) print(kwargs) end
=> :bar
2.6.3 :002 > bar(first: 'a', kwargs: 123)
{:first=>"a", :kwargs=>123} => nil
Wouldn't it be great if we had numbered arguments? So we could assign a value to variable by it's number/index, for example:
|
This is a port of https://wiki.php.net/rfc/named_params to PHP 7. This is just the basic userland functionality, there's still some parts missing: