Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:123249
Message-ID: <41ad56d9-8b27-420e-ba85-a3e9a6122bcf@wcflabs.de>
Date: Wed, 1 May 2024 23:26:08 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PHP-DEV] RFC [Discussion]: array_find
To: PHP internals <internals@lists.php.net>
References: <24e4529d-0b75-44de-90ef-34de5dfb1c99@wcflabs.de>
 <278889be-82ab-4827-a9e7-801b5ba2d8f8@app.fastmail.com>
 <CAAKU0ukwfF+D5L+23BEqL053ktCp5AQOjRGdha-8pXN-yf1HBQ@mail.gmail.com>
 <06373f2b-5de0-4582-96c5-29c3b474c01d@wcflabs.de>
 <45e6365b-4963-4969-8cc1-80bae6922fc8@wcflabs.de>
 <60bb55a5-16c9-4df0-8521-75f82d3dbf5b@app.fastmail.com>
Content-Language: de-DE
In-Reply-To: <60bb55a5-16c9-4df0-8521-75f82d3dbf5b@app.fastmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
From: josh@wcflabs.de ("=?UTF-8?Q?Joshua_R=C3=BCsweg?=")

Hi

On 01.05.24 12:26, Larry Garfield wrote:
> This looks good to me, with one remaining exception, which isn't specific to this function but should still be discussed: Always passing the value and key to the callback is unsafe, for two reasons.
> 
> 1. If the callback is an internal function rather than a user-land one, and it has only one argument, it will error confusingly.  That makes the current implementation incompatible with unary built-in functions.  See, for instance, https://www.php.net/is_string (and friends)

I think, that this problem can easily be detected with static analysers. 
Currently neither PHPStan [1] nor psalm [2] does detect this issue, but 
as the tools already validate the signature (e.g. str_contains is 
rejected) this can probably be integrated and might even be considered a 
bug.

The proper fix from PHP's side would be something like the proposal in 
https://externals.io/message/122928 (RFC idea: using the void type to 
control maximum arity of user-defined functions).


> 2. If the callback takes two arguments but the second is optional, it's highly unlikely that the key is the value expected as the second argument.  This could lead to confusingly hilarious errors.  See, for instance, https://www.php.net/intval.
> 
> These won't come up in the typical case of passing an inline closure (either short or long form), but are still hidden landmines for anyone using functions not tailor made for these functions.

I see the problem, but don't think, that this is a problem, that we can 
solve. As a note: Such problems exists in JavaScript, too [3] and is 
handled in the same way.

> I'm not sure of a good solution here, honestly, so I don't know what to recommend.  In Crell/fp, I ended up just making two different versions of the function that pass one or two arguments.  I don't think that's a good answer for this RFC, but I'm not sure what is.  At the very least, it should be mentioned as a known-limitation that gets documented., unless we can come up with something better.

I have added this problem in the section "Open Issues" in the RFC to 
document this behavior.


[1] https://phpstan.org/r/bd0866cd-6a76-4c18-8eb3-0f3848de7f4a
[2] https://psalm.dev/r/1baa3f8e0d
[3] https://wirfs-brock.com/allen/posts/166