php / doc-en

English PHP documentation
483 stars 723 forks source link

Document ArgumentCountError for internal functions #2450

Closed enumag closed 1 year ago

enumag commented 1 year ago

Description

The following code doesn't report any issue because PHP generally doesn't care about extra arguments being passed to a method:

class Y {
    public static function from(string $value): self
    {
        return new self();
    }
}

Y::from('name', 1);

However the following code:

enum X: string {
    case NAME = 'name';
}

X::from('name', 1);

causes a fatal error:

Fatal error: Uncaught ArgumentCountError: X::from() expects exactly 1 argument, 2 given in /in/aKdjL:7
Stack trace:
#0 /in/aKdjL(7): X::from('name', 1)
php/php-src#1 {main}
  thrown in /in/aKdjL on line 7

Process exited with code 255.

This is a really nasty behavior, especially because it is neither documented nor detectable with reflection: https://3v4l.org/6XPsW#v8.2.5

It matters especially when using such function as a first-class-callable. I stumbled upon it with this code: https://3v4l.org/YWSEo#v8.2.5

The worst thing about this is that due to this behavior being undocumented and undetectable there is no way for tools like PHPStan to report such problem either.

I don't even know if BackedEnum::from() is the only such function or if there are any others.

PHP Version

8.2.5

Operating System

any

damianwadley commented 1 year ago

I don't even know if BackedEnum::from() is the only such function or if there are any others.

I'm going to assume you didn't even try to look. https://3v4l.org/H0bmY

PHP doesn't check arguments on userland functions because passing "extra" arguments is completely supported. https://www.php.net/manual/en/function.func-get-args.php

There isn't much of a use case for it nowadays since the advent of variable-length argument lists, but it's still a valid feature. If you don't want your code to make use it of then you can do what PHP does: throw an ArgumentCountError if the number of arguments passed is incorrect.

enumag commented 1 year ago

I'm going to assume you didn't even try to look. 3v4l.org/H0bmY

I didn't because I have no clue how to search for them. How did you find these?

My main concern is that PHPStan should be able to detect such bug. How can we tell that a function / method doesn't accept extra arguments? Or is it simply all functions passing the following condition?

ReflectionFunctionAbstract::isInternal() === true && ReflectionFunctionAbstract::isVariadic() === false
MorganLOCode commented 1 year ago

I don't even know if BackedEnum::from() is the only such function or if there are any others.

strlen is an example.

echo strlen("five", "extra");

which triggers warnings as far back as 3v4l goes. In 8.0 it changed to an ArgumentCountError, without any mention on the corresponding Backward incompatible changes page. It ought to be in this list:

A number of warnings have been converted into Error exceptions:

  • Attempting to write to a property of a non-object. Previously this implicitly created an stdClass object for null, false and empty strings.
  • Attempting to append an element to an array for which the PHP_INT_MAX key is already used.
  • Attempting to use an invalid type (array or object) as an array key or string offset.
  • Attempting to write to an array index of a scalar value.
  • Attempting to unpack a non-array/Traversable.
  • Attempting to access unqualified constants which are undefined. Previously, unqualified constant accesses resulted in a warning and were interpreted as strings.
enumag commented 1 year ago

strlen is an example.

Thanks but sadly examples aren't very helpful. I need to know how to tell apart the functions with this behavior from the rest so that PHPStan can detect and report such issue.

MorganLOCode commented 1 year ago

strlen is an example.

Thanks but I don't care about examples. I need to know how to tell apart the functions with this behavior from the rest so that PHPStan can detect and report such issue.

At this point I'm guessing by examining the manual and looking at the declared signatures, to see how many (optional) arguments they take and whether they include ... variadics.

Incidentally, the description of ArgumentCountError doesn't mention that it's thrown on maximum argument counts on inbuilt functions, only minimum counts on user-defined functions.

enumag commented 1 year ago

Are we even sure that all non-variadic internal functions and methods have this behavior? :thinking:

And what about functions that are defined neither in core nor in user-land code but in a pecl extension? :thinking:

Also are there any plans to enforce the same behavior for user-land functions in a future version? For instance along with deprecating func_get_args() in favor of variadic parameters?

iluuu1994 commented 1 year ago

Are we even sure that all non-variadic internal functions and methods have this behavior? thinking

If they use ZEND_PARSE_PARAMETERS_START or zend_parse_parameters then yes.

And what about functions that are defined neither in core nor in user-land code but in a pecl extension? thinking

If they use ZEND_PARSE_PARAMETERS_START or zend_parse_parameters, then they'll behave the same way.

Also are there any plans to enforce the same behavior for user-land functions in a future version?

I don't think there is one. Extra arguments for PHP are heavily used. We could remove this restriction from internal functions, but passing extra arguments will generally be a mistake, so it seems better for PHP to tell you about it.

enumag commented 1 year ago

If they use ZEND_PARSE_PARAMETERS_START or zend_parse_parameters then yes.

So there is no way to tell without studying the C source code of each function?

iluuu1994 commented 1 year ago

Those are used pretty much universally. I can't guarantee there aren't a few functions that don't (e.g. some functions have multiple signatures and so will do multiple calls to the above with the quiet flag), but that's standard behavior for internal functions.

enumag commented 1 year ago

Alright. Thank you! :)

I guess I know what I need now so we can close the issue. There is still the problem with ArgumentCountError documentation being incomplete but that belongs to the documentation repository.

iluuu1994 commented 1 year ago

I also looked through the docs and was surprised that there isn't any mention of this. We can convert this to a documentation issue.