Hi
Volker and I would like to start discussion on our RFC to allow "Marking
return values as important (#[\NoDiscard])".
Please find the following resources for your reference:
- RFC: https://wiki.php.net/rfc/marking_return_value_as_important
- Implementation: https://github.com/php/php-src/pull/17599
Best regards
Tim Düsterhus
Hi
Volker and I would like to start discussion on our RFC to allow "Marking
return values as important (#[\NoDiscard])".Please find the following resources for your reference:
- RFC: https://wiki.php.net/rfc/marking_return_value_as_important
- Implementation: https://github.com/php/php-src/pull/17599
Best regards
Tim Düsterhus
Hi Tim,
This looks promising!
Why do we need the (void) cast? Wouldn't "@" work and avoid a BC issue?
— Rob
Hi
This looks promising!
Why do we need the (void) cast? Wouldn't "@" work and avoid a BC issue?
The @
operator also suppresses any other warning or notice within the
call chain. That is likely undesirable because it might hide issues one
is interested in. See this example (https://3v4l.org/NDtR7):
<?php
function inner() {
var_dump($undefined);
}
function outer() {
inner();
}
outer();
outer();
@outer();
The (void)
cast makes the intent of not being interested in the return
value clear. In C casting to (void)
is an established pattern of
"using" a value that the compiler would otherwise consider unused.
Likewise will the void
operator suppress the TypeScript-ESLint warning
of not using returned Promise
:
https://typescript-eslint.io/rules/no-floating-promises/#ignorevoid
Best regards
Tim Düsterhus
Hi
This looks promising!
Why do we need the (void) cast? Wouldn't "@" work and avoid a BC issue?
The
@
operator also suppresses any other warning or notice within the
call chain. That is likely undesirable because it might hide issues one
is interested in. See this example (https://3v4l.org/NDtR7):<?php function inner() { var_dump($undefined); } function outer() { inner(); } outer(); outer(); @outer();
The
(void)
cast makes the intent of not being interested in the return
value clear. In C casting to(void)
is an established pattern of
"using" a value that the compiler would otherwise consider unused.
Likewise will thevoid
operator suppress the TypeScript-ESLint warning
of not using returnedPromise
:
https://typescript-eslint.io/rules/no-floating-promises/#ignorevoidBest regards
Tim Düsterhus
Hi Tim,
I understand what you are saying, but I can also just remove the warning via:
$_ = outer;
Which, to me, is clearer on the intent than (void)outer(); It also happens to make the diffs more natural looking when and if the return value gets used during a code review. I'll also note that having (void) doesn't preclude me from using $_ either.
I was mostly just curious as to the reasoning; I don't like it, but it makes sense.
Best of luck!
— Rob
Am 29.01.2025 um 21:16 schrieb Rob Landers rob@bottled.codes:
The
(void)
cast makes the intent of not being interested in the return
value clear. In C casting to(void)
is an established pattern of
"using" a value that the compiler would otherwise consider unused.I understand what you are saying, but I can also just remove the warning via:
$_ = outer();
Which, to me, is clearer on the intent than (void)outer(); It also happens to make the diffs more natural looking when and if the return value gets used during a code review. I'll also note that having (void) doesn't preclude me from using $_ either.
I think the (void) cast is familiar for people from C or the like, the only caveat is that it is not backward compatible, i.e. code using it won't work on older versions of PHP so migrating to it is not seamless, so it probably should not be used for a while in public libraries / packages.
I guess another option would be to define a
function nop($) {}
and then use
nop(outer());
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?
Regards,
- Chris
I think the (void) cast is familiar for people from C or the like, the only caveat is that it is not backward compatible, i.e. code using it won't work on older versions of PHP so migrating to it is not seamless, so it probably should not be used for a while in public libraries / packages.
I suspect that a majority of PHP users will probably have had no experience in C, which is easy to forget on this list where we're working on a C codebase. In C, "void" has various uses in the type system, but in PHP this would effectively just be a specific keyword for this case.
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.
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.
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?
Regards,
Rowan Tommins
[IMSoP]
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 intonull
.
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 thevoid
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
Le 30 janv. 2025 à 16:51, Tim Düsterhus tim@bastelstu.be a écrit :
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 intonull
. 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 thevoid
return type to indicate that “nothing” is returned.
A possible alternative is: ! func();
, that already “works” as of today, modulo possible optimisation.
In any case, I think that (void) func();
is not self-explanatory, and in the event I want to ignore a result I am not supposed to ignore, I will probably write something more explicit like, $unused = func();
(possibly decorated with an annotation telling static analysers not to complain that $unused is unused), or forget(func())
BTW, the RFC says that introducing the (void)
cast is a backward compatible change, which is incorrect; see: https://3v4l.org/iN7OY
—Claude
Hi
Am 2025-01-30 18:48, schrieb Claude Pache:
BTW, the RFC says that introducing the
(void)
cast is a backward
compatible change, which is incorrect; see: https://3v4l.org/iN7OY
:-| Thank you for pointing that out. We've adjusted the “Backward
Incompatible Changes” section accordingly and also changed threshold for
the separate vote for the inclusion of (void)
from 50%+1 to 2/3 to
more accurately reflect the possible impact.
Best regards
Tim Düsterhus
Hi
Am 2025-01-29 23:48, schrieb Christian Schneider:
I guess another option would be to define a
function nop($) {}
and then use
nop(outer());
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?
For $_
see my reply to Rob. For the function: OPcache will attempt to
inline functions that only return a (implicit) return statement. However
this only works if the function is defined in the same file, as OPcache
currently cannot perform cross-file optimizations. I'd say that defining
the nop()
function in a dedicated file is currently the most reliable
solution to prevent OPcache from optimizing away the usage in a
backwards-compatible way.
Of course the best solution is actually doing something useful with the
return value, as the attribute is intended to point out when not using
the return value is very likely to be wrong. We would expect it to be
rarely necessary to just suppress the warning.
Best regards
Tim Düsterhus
Hi
Am 2025-01-29 21:16, schrieb Rob Landers:
I understand what you are saying, but I can also just remove the
warning via:$_ = outer;
Please note that $_
is a regular variable and thus will increase the
lifetimes of returned objects (and arrays that can contain objects),
which might or might not be observable when implementing __destruct()
.
Also note that OPcache will actually optimize away dead stores if it can
guarantee that the difference is not observable (i.e. if it knows that
the function never returns an object and if get_defined_vars()
is not
called).
Of course it would be possible to exclude $_
from this dead-store
optimization with PHP 8.5+ to allow for a smoother migration for
libraries that require cross-version compatibility.
We nevertheless wanted to offer an alternative that might be less
confusing than a “special variable” that might also conflict with
diagnostics like unused variable detection.
[…] It also happens to make the diffs more natural looking when and if
the return value gets used during a code review. […]
Can you clarify what you mean by “more natural looking”?
Best regards
Tim Düsterhus
Hi
Am 2025-01-29 21:16, schrieb Rob Landers:
I understand what you are saying, but I can also just remove the
warning via:$_ = outer;
Please note that
$_
is a regular variable and thus will increase the
lifetimes of returned objects (and arrays that can contain objects),
which might or might not be observable when implementing__destruct()
.
Also note that OPcache will actually optimize away dead stores if it can
guarantee that the difference is not observable (i.e. if it knows that
the function never returns an object and ifget_defined_vars()
is not
called).
If $_ gets optimized away and causes the warning, then this seems like a (future) bug with opcache (it optimized something away and it was observable). It also shouldn't matter if you use $_, $unused, or $whatever; I just chose that as an example as someone who came from C# + Scala + Go lineage.
Some might say that is a feature, though... and the inconsistency (whether opcache is used or not) may not be ideal.
For example:
$lock = flock($fp);
-if ($lock) {
+if(true) {
// do stuff
This diff is rather obvious something went wrong here; but use your imagination to imagine a diff where this is not so clear. In this case $lock accidentally becomes unused. If opcache is enabled and optimizes this code, we get the warning we probably desire, but without opcache, we do not get the warning (tests usually have opcache disabled)!
Of course it would be possible to exclude
$_
from this dead-store
optimization with PHP 8.5+ to allow for a smoother migration for
libraries that require cross-version compatibility.
I believe the draft pattern matching RFC uses this variable name. It is probably good to align there.
We nevertheless wanted to offer an alternative that might be less
confusing than a “special variable” that might also conflict with
diagnostics like unused variable detection.[…] It also happens to make the diffs more natural looking when and if
the return value gets used during a code review. […]Can you clarify what you mean by “more natural looking”?
Here are two diffs:
-(void)doSomething();
+$var = doSomething();
+somethingElse($var);
or
-$_ = doSomething();
+$var = doSomething();
+somethingElse($var);
At the expense of adding some html... I've added syntax highlighting for those using html readers to show how it might look in something like github. It is immediately clear what has changed: only the variable name is highlighted as a change on that line, letting me know what to look for. Though, as I mentioned, I come from a lineage of languages where _ is a thing, so it looks more natural to me (other languages using _ are Rust, Swift and Zig). For someone coming from more C/Typescript backgrounds, they may disagree.
— Rob
Hi Tim,
Hi
Volker and I would like to start discussion on our RFC to allow "Marking
return values as important (#[\NoDiscard])".Please find the following resources for your reference:
I reckon it's a nice thing to have, but to be honest I think handling it
just during static analysis would be the best approach. Also there would
be no need for a (void) cast to suppress the warning.
Cheers
Matteo Beccati
Development & Consulting - http://www.beccati.com/
Hi Matteo,
I reckon it's a nice thing to have, but to be honest I think handling it
just during static analysis would be the best approach. Also there would
be no need for a (void) cast to suppress the warning.
We also discussed if this should be part of the core language or something
that is only supported by tooling. During the discussion we looked for
existing context and at what was said around #[Override] [1] at the time.
I won't repeat Tim's point from last year here for brevity, but I think the
same ideas apply.
Having the core language offer a way to tag a function as “return value
matters” would also allow library and tool authors like Psalm and PHPStan
(free & pro versions) to add and work with these tags.
Not having each tool to curate its lists of functions where the return
value is considered “important”, I think, would be an improvement overall.
It also enables library authors to suggest it to these tools in a
standardized way.
PHPStorm (and EA-Extended inspections plugin) also ship with lists of
functions they warn about that are more extensive than what we propose
here, but they also only work for core as there is so no standardized way
yet, that I'm aware of, to tell PHPStorm (or any LSP) about this type of
requirement.
The void
cast or the $_
discussed currently would be a /** @phpstan-suppress UnusedReturnValue */
(or something like that) in
PHPStan. So there are various ways of expressing f this concept in syntax,
and we tried to find something that doesn't add BC issues to PHP in any
way.
There has always been overlap between what SA is doing for its users and
what PHP is doing for all users of the language. From type checking (static
vs. runtime) to warnings and deprecations, things are both checked at
runtime and at CI time. PHP its self is more conservative and has more BC
focus than the tools, of course, but I think this is a case where the
attribute would help with creating a common language.
Kind Regards,
Volker
[1] https://externals.io/message/120233#120432
--
Volker Dusch
Head of Engineering
Tideways GmbH
Königswinterer Str. 116
53227 Bonn
https://tideways.io/imprint
Sitz der Gesellschaft: Bonn
Geschäftsführer: Benjamin Außenhofer (geb. Eberlei)
Registergericht: Amtsgericht Bonn, HRB 22127
Hi
Volker and I would like to start discussion on our RFC to allow "Marking
return values as important (#[\NoDiscard])".Please find the following resources for your reference:
Looks reasonable to me.
What's the reason why this emits a warning though, instead of throwing
an Exception? I'm sure you thought of it, but it would be nice to have
that consideration mentioned in the RFC.
cheers,
Derick
What's the reason why this emits a warning though, instead of throwing
an Exception? I'm sure you thought of it, but it would be nice to have
that consideration mentioned in the RFC.
Thank you for the question, I now added the missing reasoning to the RFC
text.
A <php>E_(USER_)WARNING</php> was chosen over an exception to avoid BC
concerns, as throwing would potentially break existing code that "works as
implemented" and because this is only a warning in the literal sense of the
word, given that the function executes successfully.
Given that PHP uses Exceptions only in hard failure cases, we found it to
be more prudent to go this way. If users really want, they can
suppress/ignore this warning without adapting existing code, e.g., using
the proposed (void) cast. But the main reason was to not cause hard
failures through exceptions.
Kind Regards,
Volker
--
Volker Dusch
Head of Engineering
Tideways GmbH
Königswinterer Str. 116
53227 Bonn
https://tideways.io/imprint
Sitz der Gesellschaft: Bonn
Geschäftsführer: Benjamin Außenhofer (geb. Eberlei)
Registergericht: Amtsgericht Bonn, HRB 22127