Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:117506
Message-ID: <fd9ffa13-20dd-8a50-96c6-a092cf146179@bastelstu.be>
Date: Sat, 9 Apr 2022 13:05:37 +0200
MIME-Version: 1.0
Content-Language: en-US
To: "G. P. B." <george.banyard@gmail.com>,
 PHP internals <internals@lists.php.net>
References: <CAFPFaM+3gxkRbNdza0T_69Nma7d3Y5G5pTG-Ow_8grW_cbHJTw@mail.gmail.com>
In-Reply-To: <CAFPFaM+3gxkRbNdza0T_69Nma7d3Y5G5pTG-Ow_8grW_cbHJTw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] [RFC] Add true as type
From: tim@bastelstu.be (=?UTF-8?Q?Tim_D=c3=bcsterhus?=)

Hi

On 4/8/22 18:47, G. P. B. wrote:
> Now that null and false can be used as standalone types, I think it makes a
> lot of sense to add true as a value type for the sake of completeness with
> false.

I agree that it would make sense to have 'true' as the counterpart of 
`false` for completeness, but …

> It also has many use cases within internals and userland to declare proper
> type signatures to functions and methods.
> 
… seeing the examples of functions that might return `true`, but won't 
return false, I feel like the correct solution is not adding `true` as a 
standalone type, but instead making those function return something more 
obvious.

- The `sort()` family should rather be be ': void' which would make it 
more obvious to static analysis that '$sorted = sort($unsorted)' is not 
a correct usage.
- `DiagnoseCommand::checkComposerSchema()` in composer would benefit 
from having support for tagged unions, as currently both the error case 
(`string`) and the success case (`true`) are truthy. With the current 
PHP 8.1 feature set I can imagine that returning a proper bool and 
putting the reason in an `string &$reason` reference parameter would 
make it more obvious to use this function in an `if ()`.
- `JsonFile::validateSchema()` in composer should be ': void' as errors 
appear to be communicated using Exceptions.
- The closure in `ComposerRepository::asyncFetchFile()` would benefit 
from having support for tagged unions, but even with the current PHP 8.1 
feature set I can imagine that:

     enum CacheStatus { case Hit; }

and then making the function `mixed[]|CacheStatus` instead of 
`mixed[]|true` would make it more obvious what the meaning of the second 
option in the union is.

Best regards
Tim Düsterhus