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 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.
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 added
> for 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: 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.
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_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
>
— Rob
