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

[TimWolla]

RFC: https://wiki.php.net/rfc/marking_return_value_as_important

[iluuu1994]

There could be some edge-cases with the optimizer. For example:

if (foo());

Should or should this not error? It will not without opcache, but will with.

[TimWolla]

Should or should this not error? It will not without opcache, but will with.

Should not warn, but the RFC explicitly acknowledges OPcache optimizations (which is part of the reason for the explicit (void) cast): https://wiki.php.net/rfc/marking_return_value_as_important#void_cast_to_suppress_the_warning

[dstogov]

I don't see big problems.

Run-time performance should be the same (thanks to RETVAL specialization).
JIT code increase will affect only functions with NoDiscard attribute.
Right?

For optimizer, we may introduce an additional option that ignores NoDiscard attribute (disabled by default). We already have few similar "unsafe" bits in opcache.optimization_level.

[TimWolla]

Thank you for the review.

JIT code increase will affect only functions with NoDiscard attribute.
Right?

Yes, this is my understanding. The tracing JIT should only emit additional code if both the attribute is set and if the result in unused. The function jit has the same code size and will just call a different helper, as suggested by Ilija in https://github.com/php/php-src/pull/17599/files#r1934186865.

For optimizer, we may introduce an additional option that ignores NoDiscard attribute (disabled by default). We already have few similar "unsafe" bits in opcache.optimization_level.

I tried to optimize the implementation for the “no warning happens” case, since users are expected to either use the value or to suppress it with (void), so I hope that this is not necessary in practice. For example the JIT will not emit any additional code.

Thus I would defer this to a later point when we have more data.

[dstogov]

I tried to optimize the implementation for the “no warning happens” case, since users are expected to either use the value or to suppress it with (void), so I hope that this is not necessary in practice. For example the JIT will not emit any additional code.

You are right. It's not a big problem if we avoid inlining of functions with NoDisacard and with missing return value or (void) prefix.

[lubosdz]

IMO - this attribute can be easily replaced with proper programming, e.g. returning array [$cntSuccess, $cntFailed] or whatever.

Aside remark - (sorry for interferring, I am not C-programmer and neither understand PHP-core stuff):
I do not really understand this craziness about the attributes - it's not programming anymore, it's configuring and with quite ugly very specific syntax. Almost everything what the attributes do can be done in a classic programming. So it seems to me like a shortcut for lazy programmers. One understandable argument might be "less boilerplate code", but that also means higher informational density, ergo steeper learning curve, thus possibly easier to miss errors. I always try to avoid attributes if possible and rather implement own solution. But that's me - other developers may say different .. Some research would be beneficial how much are attributes favourable amongst common developers - but pls do not ask only those 0.1% of framework or PHP core developers, ask other common userland developers.

[iluuu1994]

LGTM otherwise.

[nielsdos]

I find code like this noisy, this just checks the contract of RETURN_THROWS. I would prefer to just keep the comment and remove the check

[TimWolla]

I'm not sure I follow. Do you mean “remove the check” (thus keeping the RETURN_THROWS())? This would be incorrect if the assignment doesn't fail.

Or do you mean “remove the entire block, including the RETURN_THROWS()”? In that case the comment no longer makes sense.

In any case, this mirrors the constructor of Deprecated::__construct(), which has 2 readonly properties and where the RETURN_THROWS() is actually doing something, so I find it helpful for clarity. In production builds this should hopefully not result in any emitted assembly either.

see also #11293 (comment) and #11293 (comment).

[nielsdos]

I meant the check + the return, i.e. the entire block. I think the comment may still make sense to remind the reader that the property update can fail (but then the commit should probably placed a few lines up to just prior to the call).

[nielsdos]

Ok, see nit