On Thu, Feb 20, 2025, at 16:54, Volker Dusch wrote:
On Wed, Feb 12, 2025 at 9:57 PM Rob Landers rob@bottled.codes wrote:
Hey Rob,
Sorry for writing the mail again, I just noticed I forgot to include the list on my first reply to you, also corrected a mistake in the second paragraph.
There will eventually be a php 9, where BC changes will be possible.
I don’t assume PHP will change the behavior of widely used core functions to throw exceptions in PHP 9 as the BC impact will be colossal, hurting adoption. Or am I misunderstanding you here?
The point that there might be arguments over usage in internal code is why we went with an “only if it is a clear problem, especially when the function has important side effects” policy to avoid this. There are not a too many examples in core, more in userland.
I was merely pointing out that if you were to want to make BC breaks, you can. You just won’t get it
I have to stop you here. It is absolutely NOT safe and reasonable to ignore the output of array_pop. You popped it for a reason. If you just care about removing the last element, then you should use unset. Unset gives the intention. If I review code with array_pop, I’ll spend quite a bit of time checking that it was intentionally ignoring the return value (and leave a comment of the same, asking to use unset instead).
There are plenty of codebases where array_pop is used for this reason. Identifying the last element of a php array. unset($foo[array_key_last($foo)]); is only possible since 7.3 and not widely used. array_pop is also shorter and faster for the same effect (when used on unknown array shapes ofc). The examples are plenty https://github.com/search?q=repo%3AWordPress%2FWordPress%20array_pop&type=code
Regards,
Volker
This feels like a rationalization, and not a reason. It has a return value, and there are more expressive ways to remove a value from an array that makes the intention clearer. If you aren’t using the return value, you should make it clear that is the intention. That’s why I said it should be included in this, for exactly the rationale you gave. It IS faster, but it clearly has a side effect that should be used or intentionally discarded.
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.
Thank you to everyone who responded to our call for comments on the list here, and to Ondřej who replied on Mastodon to the question if the property would be useful for Static Analysis tools: https://phpc.social/@OndrejMirtes/114040888791921128
Given that the thread has died down, and all points have been discussed, we plan to start the voting later this week unless any important point comes up.
We have also updated the RFC to note that, if (void) is not voted for in the secondary vote, we’d special case $_ as a discard outlet such that OPcache will not optimize the unused variable.
In this thread, I only found the information that currently OPcache does not discard such unused assignments. It would be good to know if such optimization could be considered to not end up in a situation that such optimization would be useful but can’t be applied as of this feature.
Thank you to everyone who responded to our call for comments on the list here, and to Ondřej who replied on Mastodon to the question if the property would be useful for Static Analysis tools: https://phpc.social/@OndrejMirtes/114040888791921128
Given that the thread has died down, and all points have been discussed, we plan to start the voting later this week unless any important point comes up.
We have also updated the RFC to note that, if (void) is not voted for in the secondary vote, we’d special case $_ as a discard outlet such that OPcache will not optimize the unused variable.
In this thread, I only found the information that currently OPcache does not
discard such unused assignments. It would be good to know if such optimization
could be considered to not end up in a situation that such optimization would be
useful but can't be applied as of this feature.
Unless it knows for sure that the function is unable to return an object (this includes arrays, which could contain objects), OPcache is already disallowed to elide the dead store. Otherwise this would result in an observable behavioral change, since destructors are called immediately rather than when the variable goes out of scope / is overwritten. For practically all modern code this means that this optimization only affects internal functions, since OPcache only looks at a single file. Thus it does not know about the return type of userland functions defined in a different file.
In fact, if you call `get_defined_vars()` or similar functions the dead store elimination is also disabled, since that would result in an observable behavioral change.
Should the `(void)` cast not be accepted, we will only special case the assignment to `$_` to not be elided, even if OPcache knows that the function will never return an object. The behavior for other variables will remain unchanged.
On Tue, Mar 4, 2025, 22:16 Tim Düsterhus <tim@bastelstu.be> wrote:
Hi Marc
Should the (void) cast not be accepted, we will only special case the
assignment to $_ to not be elided, even if OPcache knows that the
function will never return an object. The behavior for other variables
will remain unchanged.
X
Just asking, as my engine knowledge is a bit limited,
Wouldn’t it be possible that when OPcache would optimize away the unused variable assigned to a function, it could detect that that function have the NoDiscard attribute and also remove the attribute effect (warning triggering).
Or identify that the function has the NoDiscard attribute and based on that do not optimize away the variable?
Just asking, as my engine knowledge is a bit limited,
Wouldn't it be possible that when OPcache would optimize away the unused
variable assigned to a function, it could detect that that function have
the NoDiscard attribute and also remove the attribute effect (warning
triggering).
Not with how the implementation currently works: It's checking whether or not the FCALL opcode’s result is used or not. If the assignment is removed, the FCALL result will be unused.
Or identify that the function has the NoDiscard attribute and based on that
do not optimize away the variable?
First things first: It's not a super important optimization to have, the assignment to a variable is one of the cheaper operations - and given that OPcache needs to know about the function it's also pretty limited in application. Of course you can also always just remove the assignment yourself (or ask your favorite IDE or static analyzer to point it out for you).
That said: Checking for the attribute and performing the optimization is likely possible, though likely of limited cost/benefit. In any case that would be an implementation detail that does not change observable behavior. The RFC defines that `(void)` or alternatively `$_` will be the solution that guarantees that the warning will be suppressed (which will then also be part of the documentation), how exactly that will work internally is something that can and will change when refactoring and evolving the engine and OPcache.
On Tue, Mar 4, 2025, 23:22 Tim Düsterhus <tim@bastelstu.be> wrote:
Or identify that the function has the NoDiscard attribute and based on that
do not optimize away the variable?
First things first: It’s not a super important optimization to have, the
assignment to a variable is one of the cheaper operations - and given
that OPcache needs to know about the function it’s also pretty limited
in application. Of course you can also always just remove the assignment
yourself (or ask your favorite IDE or static analyzer to point it out
for you).
That said: Checking for the attribute and performing the optimization is
likely possible, though likely of limited cost/benefit. In any case that
would be an implementation detail that does not change observable
behavior. The RFC defines that (void) or alternatively $_ will be
the solution that guarantees that the warning will be suppressed (which
will then also be part of the documentation), how exactly that will work
internally is something that can and will change when refactoring and
evolving the engine and OPcache.
Thank you for explaining.
My motivation for asking is because I believe that special casing $_ variable might not be a desirable solution, and I personally find it worse than (void) casting.
And so, people might vote “No” for the first vote because they are not happy with either results of the second option. And maybe we can find a technical solution that doesn’t require a behavior/documentation change if second vote fails.
I agree that this optimization is not really important, so finding a way to avoid it would be nice.
