On Wed, Feb 12, 2025, at 17:43, Tim Düsterhus wrote:
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 providecoherent 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.
You yourself has said this will behave differently depending on whether or not opcache is available. Would that not make the warning disappear in dev (where opcache and xdebug are not usually compatible) only to show up in production, potentially causing a worse situation for those throwing warnings as exceptions?
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 addedfor this singular purpose.
There will eventually be a php 9, where BC changes will be possible.
There’s cases where ignoring the return value is safe and reasonable,
without being a badly designed API.
array_pop()would come to mind: Ifone 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.
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).
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.
And what if it isn’t? There’s a non-zero possibility we will be seeing RFCs for years arguing over whether one function or another should have this attribute (like array_pop), not to mention libraries using it as well.
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.
Many people turn warnings into exceptions. I’m not a fan, personally, but the codebase I work in daily does this.
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_DEPRECATEDfor userland functions andE_DEPRECATEDfor nativefunctions, 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?
DateTimeImmutableis not part of the “Use Cases” section: Our intendeduse-case is the kind of
bulk_process()functionality that is used forall the code snippets. But given that the attribute is also useful for
DateTimeImmutable, we made it part of the proposal, without it beingpart 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
— Rob
