Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [RFC] Replace "Missing argument" warning with "Too few arguments" exception »

[RFC] Replace "Missing argument" warning with "Too few arguments" exception

php.internals

Dmitry Stogov

10 years ago
hi, Please take a look into the proposal. https://wiki.php.net/rfc/too_few_args The RFC is extremely simple (both proposal and implementation) and almost completely described by the email subject. I think, this mini-RFC doesn't need 2-weeks discussion period, so I'm going to start the vote on next week. Thanks. Dmitry.

Florian Anderiasch

10 years ago
On 06/01/2016 12:55 PM, Dmitry Stogov wrote:
> hi, > Please take a look into the proposal. > > https://wiki.php.net/rfc/too_few_args
My first thought went into how people use func_get_args() as this is (in a sense) the opposite thereof. That said I couldn't think of a good reason to disagree, but maybe someone has a valid example where this change would have unforeseen consequences. ~Florian

Dmitry Stogov

10 years ago
func_get_args() useful, when we pass more actual parameters than expected. This case is unaffected. Thanks. Dmitry. ________________________________________ From: Florian Anderiasch <ml@anderiasch.de> Sent: Wednesday, June 1, 2016 2:08:09 PM To: Dmitry Stogov; PHP internals Cc: Nikita Popov Subject: Re: [PHP-DEV] [RFC] Replace "Missing argument" warning with "Too few arguments" exception On 06/01/2016 12:55 PM, Dmitry Stogov wrote:
> hi, > Please take a look into the proposal. > > https://wiki.php.net/rfc/too_few_args
My first thought went into how people use func_get_args() as this is (in a sense) the opposite thereof. That said I couldn't think of a good reason to disagree, but maybe someone has a valid example where this change would have unforeseen consequences. ~Florian

Rowan Collins

10 years ago
On 01/06/2016 11:55, Dmitry Stogov wrote:
> hi, > > > Please take a look into the proposal. > > > https://wiki.php.net/rfc/too_few_args > > > The RFC is extremely simple (both proposal and implementation) and almost completely described by the email subject. > > I think, this mini-RFC doesn't need 2-weeks discussion period, so I'm going to start the vote on next week.
I like the concept behind this, but I worry it might have a rather large BC impact. Like removing call-time pass-by-reference, there may be a lot of long-untouched code that needs fixing to work within this constraint. I'm not even sure how easy it would be for static analysis to identify these cases. I guess on the flip-side, people will *probably* have Warnings displayed or logged already... The RFC would also benefit from more discussion of the current behaviour in different cases: function foo($a) {} foo(); // Warning function bar(A $a) {} bar(); // Error function baz(A $a=null) {} baz(); // OK function quux(?A $a) {} quux(); // Not sure. Error? function wibble(int $a) {} wibble(); // Error (regardless of strict_types, I think) function wobble(?int $a) {} wobble(); // Error? Any other cases? Certainly, making these consistent would be nice, but like a lot of consistency, does it come at too high a price? Regards,
-- Rowan Collins [IMSoP]

Fleshgrinder

10 years ago
On 6/1/2016 1:33 PM, Rowan Collins wrote:
> I like the concept behind this, but I worry it might have a rather large > BC impact. Like removing call-time pass-by-reference, there may be a lot > of long-untouched code that needs fixing to work within this constraint. > I'm not even sure how easy it would be for static analysis to identify > these cases. I guess on the flip-side, people will *probably* have > Warnings displayed or logged already... > > The RFC would also benefit from more discussion of the current behaviour > in different cases: > > function foo($a) {} > foo(); // Warning > > function bar(A $a) {} > bar(); // Error > > function baz(A $a=null) {} > baz(); // OK > > function quux(?A $a) {} > quux(); // Not sure. Error? > > function wibble(int $a) {} > wibble(); // Error (regardless of strict_types, I think) > > function wobble(?int $a) {} > wobble(); // Error? > > Any other cases? > > Certainly, making these consistent would be nice, but like a lot of > consistency, does it come at too high a price? >
I fully agree with Rowan here. First we need a proper plan for all these cases and a consistent behavior. Next we need to discuss if something like this can be introduced in a feature release or requires a major release. Note that func_get_args() should die too since we have variadic arguments now. However, this can only be introduced in a major version. (Correct me if I am wrong and didn't thought of a very common use case that requires func_get_args(). I for one never need it.) Whether references need to die completely or not is a more complicated discussion. There are valid use cases for them. However, they should be avoided and other solutions should be preferred if possible.
-- Richard "Fleshgrinder" Fussenegger

Stas Malyshev

10 years ago
Hi!
> Note that func_get_args() should die too since we have variadic > arguments now.
It should not. There is nothing wrong with this function, the fact that some of it's functionality is covered by variadic args notwithstanding.
-- Stas Malyshev smalyshev@gmail.com

Joe Watkins

10 years ago
Evening, Could you expand on the "obvious optimization opportunities" in the document ? They are not obvious to me, and even if they were, this document is part of the history of this change, so a little more detail would be nice. Cheers Joe On Wed, Jun 1, 2016 at 11:55 AM, Dmitry Stogov <dmitry@zend.com> wrote:

Dmitry Stogov

10 years ago
In the SSA optimisation framework (part of opcache), we predict and propagate types of variables. We also may detect situations when some variables may be undefined. If variable can't be "undefined", we may avoid unnecessary run-time checks (e.g .like in ZEND_ADD_LONG handler). Local variables almost always initialized before usage, but with exiting behavior, function arguments always may be uninitialized. $ sapi/cli/php -d opcache.opt_debug_level=0x400000 2>&1 ../Zend/bench.php | less ... fibo_r: ; (lines=14, args=1, vars=1, tmps=3, ssa_vars=9, no_loops) ... #1.CV0($n) [undef, any] = RECV 1 #2.T1 [bool] RANGE[0..1] = IS_SMALLER #1.CV0($n) [undef, any] int(2) JMPZ #2.T1 [bool] RANGE[0..1] BB2 ... With the patch $n won't include "undef" anymore. Thanks. Dmitry. ________________________________ From: Joe Watkins <pthreads@pthreads.org> Sent: Wednesday, June 1, 2016 8:42:49 PM To: Dmitry Stogov Cc: PHP internals; Nikita Popov Subject: Re: [PHP-DEV] [RFC] Replace "Missing argument" warning with "Too few arguments" exception Evening, Could you expand on the "obvious optimization opportunities" in the document ? They are not obvious to me, and even if they were, this document is part of the history of this change, so a little more detail would be nice. Cheers Joe On Wed, Jun 1, 2016 at 11:55 AM, Dmitry Stogov <dmitry@zend.com<mailto:dmitry@zend.com>> wrote: hi, Please take a look into the proposal. https://wiki.php.net/rfc/too_few_args The RFC is extremely simple (both proposal and implementation) and almost completely described by the email subject. I think, this mini-RFC doesn't need 2-weeks discussion period, so I'm going to start the vote on next week. Thanks. Dmitry.

Nikita Popov

10 years ago
On Wed, Jun 1, 2016 at 8:24 PM, Dmitry Stogov <dmitry@zend.com> wrote:
> In the SSA optimisation framework (part of opcache), we predict and > propagate types of variables. > > We also may detect situations when some variables may be undefined. > > If variable can't be "undefined", we may avoid unnecessary run-time checks > (e.g .like in ZEND_ADD_LONG handler). > > Local variables almost always initialized before usage, but with exiting > behavior, function arguments always may be uninitialized. >
To expand on this a bit... if a variable is potentially undefined, we pretty much have to exclude it from optimization entirely, as nearly any transformation we can make is prone to impact the undefined variable notice in some way (remove, duplicate, reorder). The saving grace here is that (untyped) function parameters don't have any type information, so we typically can't apply transformations anyway (e.g. type specialization is obviously not applicable, but many other transformation require guaranteed warning/notice freedom, which usually requires more specific type information than "any"). However, there are some optimizations that would be applicable if not for the fact that the variable is potentially undefined. One such example is copy propagation. Namely, in function test($param) { $var = $param; // ... use($var); } we wouldn't be able to simply replace all uses of $var with $param and save that assignment, because this might duplicate or reorder "undefined variable" notices. Such "useless" variable-to-variable assignments are typically introduced by inlining. (Note: Both copy propagation and inlining are not currently in php-src, but may well be in the future.) Nikita

Aaron Piotrowski

10 years ago
> > On Jun 1, 2016, at 5:55 AM, Dmitry Stogov <dmitry@zend.com> wrote: > > hi, > > > Please take a look into the proposal. > > > https://wiki.php.net/rfc/too_few_args > > > The RFC is extremely simple (both proposal and implementation) and almost completely described by the email subject. > > I think, this mini-RFC doesn't need 2-weeks discussion period, so I'm going to start the vote on next week. > > > Thanks. Dmitry.
+1 on this change. IMO, this is one of the few remaining unusual PHP behaviors that exists for no obvious reason. A function should not be called if too few arguments are provided. While this might be considered a BC break, I can't imagine there's an actual code out there relying on suppressing the warning just to call a function without enough arguments. I see no problem putting this change in 7.1. Aaron Piotrowski

Rowan Collins

10 years ago
On 01/06/2016 19:36, Aaron Piotrowski wrote:
> While this might be considered a BC break, I can't imagine there's an actual code out there relying on suppressing the warning just to call a function without enough arguments. I see no problem putting this change in 7.1.
I think you're overestimating how much people care about their code running without warnings. You don't have to suppress anything, just lazily ignore warnings, or log them to a file you never get round to reading. It *might* be that most users spot and act on the warnings, but I'm not sure how we could know that with any confidence. Regards,
-- Rowan Collins [IMSoP]

Aaron Piotrowski

10 years ago
> On Jun 1, 2016, at 3:56 PM, Rowan Collins <rowan.collins@gmail.com> wrote: > > On 01/06/2016 19:36, Aaron Piotrowski wrote: >> While this might be considered a BC break, I can't imagine there's an actual code out there relying on suppressing the warning just to call a function without enough arguments. I see no problem putting this change in 7.1. > > I think you're overestimating how much people care about their code running without warnings. You don't have to suppress anything, just lazily ignore warnings, or log them to a file you never get round to reading. It *might* be that most users spot and act on the warnings, but I'm not sure how we could know that with any confidence. >
Perhaps I'm overestimating how much people care about eliminating warnings, but I doubt many functions work as intended without being supplied all required arguments. The few situations where code worked as intended when too few arguments were provided to a function are fragile and should be fixed. This change will better alert users to those problems. Aaron Piotrowski

Rowan Collins

10 years ago
On 02/06/2016 00:19, Aaron Piotrowski wrote:
> >> On Jun 1, 2016, at 3:56 PM, Rowan Collins <rowan.collins@gmail.com> wrote: >> >> On 01/06/2016 19:36, Aaron Piotrowski wrote: >>> While this might be considered a BC break, I can't imagine there's an actual code out there relying on suppressing the warning just to call a function without enough arguments. I see no problem putting this change in 7.1. >> >> I think you're overestimating how much people care about their code running without warnings. You don't have to suppress anything, just lazily ignore warnings, or log them to a file you never get round to reading. It *might* be that most users spot and act on the warnings, but I'm not sure how we could know that with any confidence. >> > > Perhaps I'm overestimating how much people care about eliminating warnings, but I doubt many functions work as intended without being supplied all required arguments. The few situations where code worked as intended when too few arguments were provided to a function are fragile and should be fixed. This change will better alert users to those problems.
Sure, but "a good thing in the long run" is not the opposite of "BC break". If people upgrade to PHP 7.1 and get a load of errors, they will just delay adoption of that version. That is not a risk that should be dismissed lightly, IMHO. Regards,
-- Rowan Collins [IMSoP]

Stas Malyshev

10 years ago
Hi!
> Please take a look into the proposal. > > > https://wiki.php.net/rfc/too_few_args > > > The RFC is extremely simple (both proposal and implementation) and almost completely described by the email subject.
Looks fine to me. I don't think having undef function parameters is a very valuable feature that is widely used.
-- Stas Malyshev smalyshev@gmail.com

Christoph Becker

10 years ago
On 02.06.2016 at 01:23, Stanislav Malyshev wrote:
>> Please take a look into the proposal. >> >> >> https://wiki.php.net/rfc/too_few_args >> >> >> The RFC is extremely simple (both proposal and implementation) and almost completely described by the email subject. > > Looks fine to me. I don't think having undef function parameters is a > very valuable feature
ACK
> that is widely used.
I have seen it several times; most likely the authors did it, because it's common in JavaScript where default parameters are still not widely available. I would have been +1 on this RFC if it had targeted 7.0.0, but I tend to be ±0 for PHP 7.x.
-- Christoph M. Becker

Benjamin Eberlei

10 years ago
On Wed, Jun 1, 2016 at 12:55 PM, Dmitry Stogov <dmitry@zend.com> wrote:
> hi, > > > Please take a look into the proposal. > > > https://wiki.php.net/rfc/too_few_args > > > The RFC is extremely simple (both proposal and implementation) and almost > completely described by the email subject. > > I think, this mini-RFC doesn't need 2-weeks discussion period, so I'm > going to start the vote on next week. >
Quick question, I think the error message might be a little confusing asking for "exactly $n" parameters, because it could be a variadic function using func_get_args(), shouldn't the message be? throw Error("Too few arguments to function %s(), 0 passed in %s on line %d and at least %d expected") This only changes the case where the passed arguments are smaller than the required ones or not? This is acceptable BC, but passing more arguments should still not be an error.

Bob Weinand

10 years ago
> Am 01.06.2016 um 12:55 schrieb Dmitry Stogov <dmitry@zend.com>: > > hi, > > > Please take a look into the proposal. > > > https://wiki.php.net/rfc/too_few_args > > > The RFC is extremely simple (both proposal and implementation) and almost completely described by the email subject. > > I think, this mini-RFC doesn't need 2-weeks discussion period, so I'm going to start the vote on next week. > > > Thanks. Dmitry.
Very nice! Just a question: Does the RFC also impact calls to internal functions? (With internal functions we don't have the problem as they're typically immediately aborted, but it would be inconsistent with userland functions to have once a warning, once an exception) Judging from the patch this isn't the case? (at least I see no related changes) Thus I think the scope of the RFC is a bit too small. [and at least it should be explicitly mentioned in the RFC if you decide against that]. Bob

Dmitry Stogov

10 years ago
On 06/02/2016 03:01 PM, Bob Weinand wrote:
>> Am 01.06.2016 um 12:55 schrieb Dmitry Stogov <dmitry@zend.com>: >> >> hi, >> >> >> Please take a look into the proposal. >> >> >> https://wiki.php.net/rfc/too_few_args >> >> >> The RFC is extremely simple (both proposal and implementation) and almost completely described by the email subject. >> >> I think, this mini-RFC doesn't need 2-weeks discussion period, so I'm going to start the vote on next week. >> >> >> Thanks. Dmitry. > Very nice! > > Just a question: Does the RFC also impact calls to internal functions? (With internal functions we don't have the problem as they're typically immediately aborted, but it would be inconsistent with userland functions to have once a warning, once an exception) > > Judging from the patch this isn't the case? (at least I see no related changes) > > Thus I think the scope of the RFC is a bit too small. [and at least it should be explicitly mentioned in the RFC if you decide against that]. > > Bob
The RFC doesn't propose to change behavior of internal functions. In case of wrong number of arguments, they are not executed anyway. It may make sense to change their behavior as well, but I don't see a big value. Thanks. Dmitry.

Bob Weinand

10 years ago
> Am 02.06.2016 um 14:11 schrieb Dmitry Stogov <dmitry@zend.com>: > > > > On 06/02/2016 03:01 PM, Bob Weinand wrote: >>> Am 01.06.2016 um 12:55 schrieb Dmitry Stogov <dmitry@zend.com>: >>> >>> hi, >>> >>> >>> Please take a look into the proposal. >>> >>> >>> https://wiki.php.net/rfc/too_few_args >>> >>> >>> The RFC is extremely simple (both proposal and implementation) and almost completely described by the email subject. >>> >>> I think, this mini-RFC doesn't need 2-weeks discussion period, so I'm going to start the vote on next week. >>> >>> >>> Thanks. Dmitry. >> Very nice! >> >> Just a question: Does the RFC also impact calls to internal functions? (With internal functions we don't have the problem as they're typically immediately aborted, but it would be inconsistent with userland functions to have once a warning, once an exception) >> >> Judging from the patch this isn't the case? (at least I see no related changes) >> >> Thus I think the scope of the RFC is a bit too small. [and at least it should be explicitly mentioned in the RFC if you decide against that]. >> >> Bob > The RFC doesn't propose to change behavior of internal functions. > In case of wrong number of arguments, they are not executed anyway. > It may make sense to change their behavior as well, but I don't see a big value. > > Thanks. Dmitry.
Right, then the RFC should explicitly mention it. It only talks about "function calls" in general right now, but doesn't restrict the scope to userland functions only. The value is basically in having equal and consistent behavior (in failure case) for all functions. I do not see it a must, but it would be a very-nice-to-have in my eyes. Bob