Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:126242
Precedence: bulk
MIME-Version: 1.0
Date: Thu, 30 Jan 2025 16:51:09 +0100
To: php internals <internals@lists.php.net>
Subject: Re: [PHP-DEV] RFC: Marking return values as important (#[\NoDiscard])
In-Reply-To: <FC48217C-7934-4957-9DC9-6C5576B78DA8@rwec.co.uk>
References: <a995ac24905030e168e7f6006b996cd5@bastelstu.be>
 <a3d14e31-53eb-4a04-8fb2-01f58809fec1@app.fastmail.com>
 <3f170549-cfd6-441e-b892-cf51d726dbe8@bastelstu.be>
 <3b4d9be8-9255-44ab-95c5-ea3045212cf4@app.fastmail.com>
 <B698F6D0-7EB8-49B8-9638-072B8BE40AC1@cschneid.com>
 <FC48217C-7934-4957-9DC9-6C5576B78DA8@rwec.co.uk>
Message-ID: <da1854112a3acb434e43868b431fe9e9@bastelstu.be>
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
From: tim@bastelstu.be (=?UTF-8?Q?Tim_D=C3=BCsterhus?=)

Hi

Am 2025-01-30 10:02, schrieb Rowan Tommins [IMSoP]:
> I think it would be good to explore alternatives - for instance, I 
> think C# has a reserved _ variable to assign discarded results to, but 
> may be misremembering.

A quick search confirms that C# supports the `_` variable. It's also 
supported with Rust.

We are open to possible alternatives to the `(void)` cast, this was also 
something we discussed while drafting the RFC. However we opted for the 
`(void)` with out proposal for the following reasons:

- `$_ = func();` is already working as a regular variable (see my reply 
to Rob regarding lifetimes), thus repurposing it as a “discard pile” 
would cause backwards compatibility issues.
- `_ = func();` would introduce new syntax (just like `(void)`), but 
does not look and feel like PHP to us.
- `(void)func();` has precedent in other languages (just like `_ =`) and 
looks and feels like PHP. In fact in earlier PHP versions there already 
was an `(unset)` cast to unconditionally turn an expression into `null`. 
We opted for `(void)` instead of bringing back `(unset)` for the 
following reasons: (1) Possible confusion about a deprecated + removed 
feature coming back (e.g. avoiding old blog articles). (2) More cleanly 
enforcing that `(void)` is a statement, rather than an expression. (3) 
The similarity with the `void` return type to indicate that “nothing” is 
returned.

>> but my question is whether $_ = outer() and nop(outer()) will be 
>> guaranteed to not being optimized in a way which causes the warning to 
>> reappear. I don't know much about the optimizations being done but 
>> this could be an issue in the future, no?
> 
> I second this concern: having code that you think has suppressed the 
> warning later be optimized and start warning could be very confusing, 
> and given the backwards compatibility issue, users will need something 
> other than "(void)" which is reliable.

See my replies to Rob and Christian.

> My other thought reading the proposal is that if this can be done 
> efficiently, can we use the same approach to warn when a "void" 
> function's return *is* used?

Performing this check would be possible, but it would likely be less 
efficient.

The current implementation of `#[\NoDiscard]` optimizes based on the 
assumption that the attribute is going to be comparatively rarely used 
(i.e. only when it actually matters, as per the “Recommended Usage” 
section of the RFC). The “Implementation Details” section already 
mentions this, but basically what's happening is that the checks for 
`#[\Deprecated]` and `#[\NoDiscard]` are happening at the same time 
(it's a bitflag check) and you are only paying the cost of the more 
detailed checks if the function is Deprecated or NoDiscard (or both).

Then there's also the “OPcode specialization” to remove `#[\NoDiscard]` 
from the bitflag check when the return value is known to be used.

And finally functions that are known to neither be `#[\Deprecated]` nor 
`#[\NoDiscard]` at compile time (this is mostly *fully-qualified* native 
functions) are going through an even faster OPcode that completely omits 
several safety checks. You can check this for yourself by passing a 
script through `php -d opcache.enable_cli=1 -d 
opcache.opt_debug_level=0x20000 script.php`. If you see `DO_ICALL` 
(native functions) or `DO_UCALL` (userland functions) then that's the 
optimized version omitting the checks.

Here's an example:

     <?php

     namespace Foo;

     strrev('test');

results in:

     $_main:
          ; (lines=4, args=0, vars=0, tmps=0)
          ; (after optimizer)
          ; test.php:1-6
     0000 INIT_NS_FCALL_BY_NAME 1 string("Foo\\strrev")
     0001 SEND_VAL_EX string("test") 1
     0002 DO_FCALL_BY_NAME
     0003 RETURN int(1)

whereas

     <?php

     namespace Foo;

     \strrev('test');

results in

     $_main:
          ; (lines=4, args=0, vars=0, tmps=0)
          ; (after optimizer)
          ; test.php:1-6
     0000 INIT_FCALL 1 96 string("strrev")
     0001 SEND_VAL string("test") 1
     0002 DO_ICALL
     0003 RETURN int(1)

Note the difference for 0000 and 0002.

Best regards
Tim Düsterhus