RFC: Marking return values as important (#[\NoDiscard]) View as Markdown

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 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

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

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

Hello everyone,

it's been two weeks, but given the feedback we received, we don't feel
the discussion didn't reach a conclusive resolution yet.

So we wanted to solicit additional opinions. In an attempt to summarize
the discussion, how we see it, there are two main points of contention
we'd like to discuss further.

I'm still undecided on the RFC overall, but one thing that is problematic is the phrasing of the messages. Currently, the messages in the attribute are fragments of an English sentence, seemingly designed to fit grammatically with a sentence fragment that is coded into the engine somewhere but not readily available to developers.

From my phrasing I think you can guess my opinion of that.

That is impossible to document cleanly for English speakers. It will not translate at all for anyone who is writing in a non-English language (which people do). People are going to get it wrong more than they get it right, in any language.

Instead, the wording should be structured to be a complete sentence, and the built-in message updated to make that logical. That gives the developer much more freedom to write a meaningful, contextually-relevant message in the language of their choice.

--Larry Garfield

b) Naming of the attribute

Nobody mentioned this on the list, but before opening a vote we'd like
to heard if the attribute name makes sense to you.

On this, it could be spelled #[\MustUse], rather than phrase it as a
negative.

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
  Hello everyone,

it's been two weeks, but given the feedback we received, we don't feel
the discussion didn't reach a conclusive resolution yet.

So we wanted to solicit additional opinions. In an attempt to
summarize the discussion, how we see it, there are two main points of
contention we'd like to discuss further.

a) Intentionally discarding the return value

The (void)flock($fp); syntax was the main thing discussed on the list.
With $_ = flock($fp); as the most suggested alternative.

The issue with using $_ as it currently stands, is that the unused
variable would be cleaned up by OPcache. Should we instead have a
special case in OPcache ensuring it does not apply its optimization to
a variable if it's named $_ instead? The semantic difference would
be that $_ would keep returned values (and objects) alive and in
memory, whereas (void) would discard them immediately. Leading to
different times when, for example, destructors would be called.

The consequence of this would be that 3rd party tools like IDEs and
Static and Dynamic Code Analyzers would also have to build that
special case in to not produce "unused variable" warnings.

We're also happy to change the secondary vote to "(void) vs $_" as we
feel there needs to be a way to discard the return value consistently
for this to be a complete and usable feature.

I would not be in favour of special casing this, and henceforth the
(void) cast makes more sense to me. Mostly, because the special casing
has to be done in many places, and in many tools.

b) Naming of the attribute

Nobody mentioned this on the list, but before opening a vote we'd like
to heard if the attribute name makes sense to you.

We've chosen #[NoDiscard] as it's also used in C and C++. See [1] for
the full list of references. If you feel this doesn't fit with PHP, we
welcome other suggestions.

I think it fits, and there is already precedence.

cheers,
Derick

--
https://derickrethans.nl | https://xdebug.org | https://dram.io

Author of Xdebug. Like it? Consider supporting me: https://xdebug.org/support

mastodon: @derickr@phpc.social @xdebug@phpc.social

Hi

Apologies for the delay in getting back to you. Volker was out of office
and I try to coordinate the replies with him to not misrepresent his
opinion.

Am 2025-02-12 14:17, schrieb Bilge:

Apologies if this has already been brought up; I haven't read the
entire thread, but isn't the entire premise of this RFC based on a
falsehood?

We do not believe so.

This kind of “partial success” can only be reliably communicated by
means of a return value

Exceptions are objects, so you can attach whatever additional
information you wish to that object.

Perhaps the word "reliably" is doing a lot of heavy lifting in that
sentence, but if you would refute me (which presumably you will), then
I think this needs to be explained in better detail in the RFC, because
it looks to me that exploiting object properties is just as viable as
returning something, and probably preferred, since this solves your
problem of the user not handling the failure.

Perhaps one can find a better word than “reliably” there, nevertheless:
We disagree that throwing an Exception is an appropriate solution to
communicate partial success. While it certainly would ensure that
developers can't forget to handle the (partial) failure case, it would
also be a very non-idiomatic use of Exceptions. Instead of being able to
check ->getMessage() or ->getCode() to find out the cause, which is
automatically supported by generic Exception-handling mechanisms as
provided by every framework, one needs to specifically call a
->getResults() methods to find out the individual results.

And when it's a generic function where depending on the type of item
processed, one is also interested in the “return value” of the
successful cases, instead of the binary success/failure option, it would
become very unwieldy, requiring the use of a single-statement try-block
to handle both the “all successful” and the “at least one failure” case,
ending up in a “return value with extra steps” situation.

 try {
     $results = bulk_process($items);
     /* All successful. */
 } catch (BulkProcessingFailures $e) {
     $results = $e->getResults();
     /* At least one failed. */
 }

 foreach ($results as $item => $result) {
     if ($result instanceof SuccessResult) {
         echo "Created item ", $item, " with ID: ", $result->getId(), 

PHP_EOL;
} else {
echo "Failed to create item ", $item, PHP_EOL;
}
}

Best regards
Tim Düsterhus

Hi

Am 2025-02-12 15:07, schrieb Kamil Tekiela:

I'd be honest, I have a very negative attitude towards this proposal
and I'd be voting against it. It seems to me like it's creating a
problem and then trying to find a solution for it.

Given that even Rust with a modern stdlib and ergonomic language
features has such an attribute, we do not believe it's an invented
problem. With the bulk_process() example, we also tried to provide
coherent user-story with a use-case that the readers might've already
encountered themselves in the same or a similar fashion. Personally I
encountered situations where I wished such an attribute would've warned
me both in PHP and JavaScript/TypeScript. Also in C of course, but given
that C doesn't have Exceptions, the situation is somewhat different.

For an additional example use-case, consider a function returning a
resource object with side-effects which will automatically clean up the
resource when being destructed. Specifically a function to spawn a new
process or thread:

 class Process {
     public function __destruct() { /* kill the process */ }
 }

 #[NoDiscard("the process will be terminated when the Process object 

goes out of scope")]
function start_process(string $executable): Process {
return new Process($executable);
}

 start_process('/my/background/process');

Depending on timing, the process might or might not run to completion
before PHP gets around to kill it, for example when a step debugger is
attached to PHP, making the bug disappear when debugging.

Related to the above example and also to the flock() function: A
developer might want to write a locking function that returns a lock
resource that will automatically unlock when it goes out of scope,
preventing the unlock from being forgotten. It might not be immediately
obvious that the locking function returns a lock resource, especially
when it's associated with another object:

 $cacheDriver->lock('someCacheEntry');

Is there a $cacheDriver->unlock('someCacheEntry') or will ->lock()
return a lock resource? Having the attribute on the lock() method can
help.

Searching for language:rust "must_use =" on GitHub reveals that a
common use-case in Rust is lazy iterators, where you can call
higher-order functions (e.g. map() or filter()), but they will not do
anything unless you explicitly “poll” them at least once. As far as I
know, Java has lazy streams that behave identically.

A return value is always supposed to be used. If some API is returning
a value that can be safely ignored, it's a badly designed API. If a

We agree that in a greenfield ecosystem, we would make ignoring the
return value of a function a warning by default (with an opt-out
mechanism), as done in Swift. However in PHP we do not have a greenfield
situation, there a number of functions where the return value is useless
and only exists for historical reasons. This includes all functions
which nowadays have a return-type of just true, which was only added
for this singular purpose.

There's cases where ignoring the return value is safe and reasonable,
without being a badly designed API. array_pop() would come to mind: If
one is only interested in the side-effect of removing the last element
in the array, one does not need the return value without necessarily
doing something unsafe.

doesn't feel like the solution. In fact, it's creating a problem for
users who want to ignore the value, which you then propose to solve
with (void) cast.

Ignoring the return value of functions having the attribute should be a
rare occurrence given the intended use-case of pointing out unsafe
situations. But as with static analyzers there needs to solution to
suppress warnings, after carefully verifying that the warning is not
applicable in a given situation.

the compilation phase. In PHP warnings are runtime errors. The code
should emit an exception instead of a warning. It would also make it

See Volker's reply to Derick.

much easier to handle and you wouldn't need any special construct to
allow users to ignore the new attribute. And I am really not a fan of

Even if an Exception would be thrown, there would be a mechanism to
suppress the Exception. A catch-block wouldn't work, because if the
Exception is thrown, the function is not executed / control flow is
interrupted.

the PHP engine generating E_USER_WARNING which should be reserved only
for warnings triggered by trigger_error.

This follows the lead of the #[\Deprecated] attribute, which emits
E_USER_DEPRECATED for userland functions and E_DEPRECATED for native
functions, despite being triggered by the engine.

The examples you used don't support the need for the new attribute.
Regarding the DateTimeImmutable methods, you said yourself: "The
methods are badly named, because they do not actually set the updated
value". So your proposal suggests adding a brand new thing to PHP to
deal with bad method names?

DateTimeImmutable is not part of the “Use Cases” section: Our intended
use-case is the kind of bulk_process() functionality that is used for
all the code snippets. But given that the attribute is also useful for
DateTimeImmutable, we made it part of the proposal, without it being
part of the intended “user-story”.

This problem should be solved using static analysers, IDE, and proper
code refactoring.

See Volker's reply to Matteo.

Best regards
Tim Düsterhus

I must agree with Kamil. I don't see practical benefits of this feature
that would surpass the implications it has for the language. We already
have static analysis handling such cases and it can be extended even to
non-pure functions. Moreover, syntax (void) adds additional complexity to
beginners' understanding of the typing system. The second option with $_ is
BC on the other hand, so both options are unsatisfying. And honestly, I
don't have good ideas for it.

Kind regards,
Jorg Sowa

Hi Jorg,

To be clear: There is no BC break with $_. Opcache doesn't discard
variables containing objects, so there is no observable change in behavior
from what we're proposing. It doesn't change the variable semantics at all.
Or am I misunderstanding what you're referring to?

In addition, I reached out to see if Ondřej (Creator of PHPStan) had an
opinion: https://phpc.social/@OndrejMirtes/114040888791921128 - So on top
of our previous arguments that PHP shouldn't depend on 3rd party tools for
everything, there is also at least interest in having this in core from the
tool creators.

@edorian It already exists for PHPStan as a 3rd party extension:
https://github.com/DaveLiddament/phpstan-php-language-extensions
(MustUseResult)
Additionally, PHPStan itself reports this if you call a @phpstan-pure
method or function and not use its result.
This attribute would be valuable for functions that aren’t pure but
you’re still supposed to read the returned value.

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