Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:126381
Feedback-ID: ifab94697:Fastmail
Precedence: bulk
MIME-Version: 1.0
Date: Wed, 12 Feb 2025 21:57:34 +0100
To: =?UTF-8?Q?Tim_D=C3=BCsterhus?= <tim@bastelstu.be>,
 "Kamil Tekiela" <tekiela246@gmail.com>
Cc: "Volker Dusch" <volker@tideways-gmbh.com>,
 "php internals" <internals@lists.php.net>
Message-ID: <46f5a4ab-ae85-4a15-9781-27a94eedc512@app.fastmail.com>
In-Reply-To: <95a69ca549b246c3645f8c6db519f669@bastelstu.be>
References: <a995ac24905030e168e7f6006b996cd5@bastelstu.be>
 <CAGBsUrfFvWBcncpNorb3dmXJ_=cZLhmdaVkFHR6L5SkrhtPLcw@mail.gmail.com>
 <95a69ca549b246c3645f8c6db519f669@bastelstu.be>
Subject: Re: [PHP-DEV] RFC: Marking return values as important (#[\NoDiscard])
Content-Type: multipart/alternative;
 boundary=d766a4c28ecd45a19c0b938045a8dcdf
From: rob@bottled.codes ("Rob Landers")

--d766a4c28ecd45a19c0b938045a8dcdf
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable



On Wed, Feb 12, 2025, at 17:43, Tim D=C3=BCsterhus wrote:
> Hi
>=20
> 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.
>=20
> Given that even Rust with a modern stdlib and ergonomic language=20
> features has such an attribute, we do not believe it's an invented=20
> problem. With the `bulk_process()` example, we also tried to provide=20
> coherent user-story with a use-case that the readers might've already=20
> encountered themselves in the same or a similar fashion. Personally I=20
> encountered situations where I wished such an attribute would've warne=
d=20
> me both in PHP and JavaScript/TypeScript. Also in C of course, but giv=
en=20
> that C doesn't have Exceptions, the situation is somewhat different.
>=20
> For an additional example use-case, consider a function returning a=20
> resource object with side-effects which will automatically clean up th=
e=20
> resource when being destructed. Specifically a function to spawn a new=20
> process or thread:
>=20
>      class Process {
>          public function __destruct() { /* kill the process */ }
>      }
>=20
>      #[NoDiscard("the process will be terminated when the Process obje=
ct=20
> goes out of scope")]
>      function start_process(string $executable): Process {
>          return new Process($executable);
>      }
>=20
>      start_process('/my/background/process');
>=20
> Depending on timing, the process might or might not run to completion=20
> before PHP gets around to kill it, for example when a step debugger is=20
> 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 i=
n dev (where opcache and xdebug are not usually compatible) only to show=
 up in production, potentially causing a worse situation for those throw=
ing warnings as exceptions?

>=20
> > A return value is always supposed to be used. If some API is returni=
ng
> > a value that can be safely ignored, it's a badly designed API. If a
>=20
> We agree that in a greenfield ecosystem, we would make ignoring the=20
> return value of a function a warning by default (with an opt-out=20
> mechanism), as done in Swift. However in PHP we do not have a greenfie=
ld=20
> situation, there a number of functions where the return value is usele=
ss=20
> and only exists for historical reasons. This includes all functions=20
> which nowadays have a return-type of just `true`, which was only added=20
> 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,=20
> without being a badly designed API. `array_pop()` would come to mind: =
If=20
> one is only interested in the side-effect of removing the last element=20
> in the array, one does not need the return value without necessarily=20
> doing something unsafe.

I have to stop you here. It is absolutely NOT safe and reasonable to ign=
ore the output of array_pop. You popped it for a reason. If you just car=
e about removing the last element, then you should use unset. Unset give=
s the intention. If I review code with array_pop, I=E2=80=99ll spend qui=
te 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).

>=20
> > 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.
>=20
> Ignoring the return value of functions having the attribute should be =
a=20
> rare occurrence given the intended use-case of pointing out unsafe=20
> situations. But as with static analyzers there needs to solution to=20
> suppress warnings, after carefully verifying that the warning is not=20
> applicable in a given situation.

And what if it isn=E2=80=99t? There=E2=80=99s a non-zero possibility we =
will be seeing RFCs for years arguing over whether one function or anoth=
er should have this attribute (like array_pop), not to mention libraries=
 using it as well.

>=20
> > the compilation phase. In PHP warnings are runtime errors. The code
> > should emit an exception instead of a warning. It would also make it
>=20
> See Volker's reply to Derick.
>=20
> > 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
>=20
> Even if an Exception would be thrown, there would be a mechanism to=20
> suppress the Exception. A catch-block wouldn't work, because if the=20
> Exception is thrown, the function is not executed / control flow is=20
> interrupted.

Many people turn warnings into exceptions. I=E2=80=99m not a fan, person=
ally, but the codebase I work in daily does this.

>=20
> > the PHP engine generating E_USER_WARNING which should be reserved on=
ly
> > for warnings triggered by trigger_error.
>=20
> This follows the lead of the `#[\Deprecated]` attribute, which emits=20
> `E_USER_DEPRECATED` for userland functions and `E_DEPRECATED` for nati=
ve=20
> functions, despite being triggered by the engine.
>=20
> > 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?
>=20
> `DateTimeImmutable` is not part of the =E2=80=9CUse Cases=E2=80=9D sec=
tion: Our intended=20
> use-case is the kind of `bulk_process()` functionality that is used fo=
r=20
> all the code snippets. But given that the attribute is also useful for=20
> `DateTimeImmutable`, we made it part of the proposal, without it being=20
> part of the intended =E2=80=9Cuser-story=E2=80=9D.
>=20
> > This problem should be solved using static analysers, IDE, and proper
> > code refactoring.
>=20
> See Volker's reply to Matteo.
>=20
> Best regards
> Tim D=C3=BCsterhus
>=20

=E2=80=94 Rob
--d766a4c28ecd45a19c0b938045a8dcdf
Content-Type: text/html; charset=utf-8
Content-Transfer-Encoding: quoted-printable

<!DOCTYPE html><html><head><title></title><style type=3D"text/css">p.Mso=
Normal,p.MsoNoSpacing{margin:0}</style></head><body><div><br></div><div>=
<br></div><div>On Wed, Feb 12, 2025, at 17:43, Tim D=C3=BCsterhus wrote:=
<br></div><blockquote type=3D"cite" id=3D"qt" style=3D""><div>Hi<br></di=
v><div><br></div><div>Am 2025-02-12 15:07, schrieb Kamil Tekiela:<br></d=
iv><div>&gt; I'd be honest, I have a very negative attitude towards this=
 proposal<br></div><div>&gt; and I'd be voting against it. It seems to m=
e like it's creating a<br></div><div>&gt; problem and then trying to fin=
d a solution for it.<br></div><div><br></div><div>Given that even Rust w=
ith a modern stdlib and ergonomic language&nbsp;<br></div><div>features =
has such an attribute, we do not believe it's an invented&nbsp;<br></div=
><div>problem. With the `bulk_process()` example, we also tried to provi=
de&nbsp;<br></div><div>coherent user-story with a use-case that the read=
ers might've already&nbsp;<br></div><div>encountered themselves in the s=
ame or a similar fashion. Personally I&nbsp;<br></div><div>encountered s=
ituations where I wished such an attribute would've warned&nbsp;<br></di=
v><div>me both in PHP and JavaScript/TypeScript. Also in C of course, bu=
t given&nbsp;<br></div><div>that C doesn't have Exceptions, the situatio=
n is somewhat different.<br></div><div><br></div><div>For an additional =
example use-case, consider a function returning a&nbsp;<br></div><div>re=
source object with side-effects which will automatically clean up the&nb=
sp;<br></div><div>resource when being destructed. Specifically a functio=
n to spawn a new&nbsp;<br></div><div>process or thread:<br></div><div><b=
r></div><div>&nbsp;&nbsp;&nbsp;&nbsp; class Process {<br></div><div>&nbs=
p;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; public function __destruct(=
) { /* kill the process */ }<br></div><div>&nbsp;&nbsp;&nbsp;&nbsp; }<br=
></div><div><br></div><div>&nbsp;&nbsp;&nbsp;&nbsp; #[NoDiscard("the pro=
cess will be terminated when the Process object&nbsp;<br></div><div>goes=
 out of scope")]<br></div><div>&nbsp;&nbsp;&nbsp;&nbsp; function start_p=
rocess(string $executable): Process {<br></div><div>&nbsp;&nbsp;&nbsp;&n=
bsp;&nbsp;&nbsp;&nbsp;&nbsp; return new Process($executable);<br></div><=
div>&nbsp;&nbsp;&nbsp;&nbsp; }<br></div><div><br></div><div>&nbsp;&nbsp;=
&nbsp;&nbsp; start_process('/my/background/process');<br></div><div><br>=
</div><div>Depending on timing, the process might or might not run to co=
mpletion&nbsp;<br></div><div>before PHP gets around to kill it, for exam=
ple when a step debugger is&nbsp;<br></div><div>attached to PHP, making =
the bug disappear when debugging.<br></div></blockquote><div><br></div><=
div>You yourself has said this will behave differently depending on whet=
her or not opcache is available. Would that not make the warning disappe=
ar in dev (where opcache and xdebug are not usually compatible) only to =
show up in production, potentially causing a worse situation for those t=
hrowing warnings as exceptions?</div><div><br></div><blockquote type=3D"=
cite" id=3D"qt" style=3D""><div><br></div><div>&gt; A return value is al=
ways supposed to be used. If some API is returning<br></div><div>&gt; a =
value that can be safely ignored, it's a badly designed API. If a<br></d=
iv><div><br></div><div>We agree that in a greenfield ecosystem, we would=
 make ignoring the&nbsp;<br></div><div>return value of a function a warn=
ing by default (with an opt-out&nbsp;<br></div><div>mechanism), as done =
in Swift. However in PHP we do not have a greenfield&nbsp;<br></div><div=
>situation, there a number of functions where the return value is useles=
s&nbsp;<br></div><div>and only exists for historical reasons. This inclu=
des all functions&nbsp;<br></div><div>which nowadays have a return-type =
of just `true`, which was only added&nbsp;<br></div><div>for this singul=
ar purpose.<br></div></blockquote><div><br></div><div>There will eventua=
lly be a php 9, where BC changes will be possible.</div><div><br></div><=
blockquote type=3D"cite" id=3D"qt" style=3D""><div>There's cases where i=
gnoring the return value is safe and reasonable,&nbsp;<br></div><div>wit=
hout being a badly designed API. `array_pop()` would come to mind: If&nb=
sp;<br></div><div>one is only interested in the side-effect of removing =
the last element&nbsp;<br></div><div>in the array, one does not need the=
 return value without necessarily&nbsp;<br></div><div>doing something un=
safe.<br></div></blockquote><div><br></div><div>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 las=
t element, then you should use unset. Unset gives the intention. If I re=
view code with array_pop, I=E2=80=99ll spend quite a bit of time checkin=
g that it was intentionally ignoring the return value (and leave a comme=
nt of the same, asking to use unset instead).<br></div><div><br></div><b=
lockquote type=3D"cite" id=3D"qt" style=3D""><div><br></div><div>&gt; do=
esn't feel like the solution. In fact, it's creating a problem for<br></=
div><div>&gt; users who want to ignore the value, which you then propose=
 to solve<br></div><div>&gt; with (void) cast.<br></div><div><br></div><=
div>Ignoring the return value of functions having the attribute should b=
e a&nbsp;<br></div><div>rare occurrence given the intended use-case of p=
ointing out unsafe&nbsp;<br></div><div>situations. But as with static an=
alyzers there needs to solution to&nbsp;<br></div><div>suppress warnings=
, after carefully verifying that the warning is not&nbsp;<br></div><div>=
applicable in a given situation.<br></div></blockquote><div><br></div><d=
iv>And what if it isn=E2=80=99t? There=E2=80=99s a non-zero possibility =
we will be seeing RFCs for years arguing over whether one function or an=
other should have this attribute (like array_pop), not to mention librar=
ies using it as well.<br></div><div><br></div><blockquote type=3D"cite" =
id=3D"qt" style=3D""><div><br></div><div>&gt; the compilation phase. In =
PHP warnings are runtime errors. The code<br></div><div>&gt; should emit=
 an exception instead of a warning. It would also make it<br></div><div>=
<br></div><div>See Volker's reply to Derick.<br></div><div><br></div><di=
v>&gt; much easier to handle and you wouldn't need any special construct=
 to<br></div><div>&gt; allow users to ignore the new attribute. And I am=
 really not a fan of<br></div><div><br></div><div>Even if an Exception w=
ould be thrown, there would be a mechanism to&nbsp;<br></div><div>suppre=
ss the Exception. A catch-block wouldn't work, because if the&nbsp;<br><=
/div><div>Exception is thrown, the function is not executed / control fl=
ow is&nbsp;<br></div><div>interrupted.<br></div></blockquote><div><br></=
div><div>Many people turn warnings into exceptions. I=E2=80=99m not a fa=
n, personally, but the codebase I work in daily does this.</div><div><br=
></div><blockquote type=3D"cite" id=3D"qt" style=3D""><div><br></div><di=
v>&gt; the PHP engine generating E_USER_WARNING which should be reserved=
 only<br></div><div>&gt; for warnings triggered by trigger_error.<br></d=
iv><div><br></div><div>This follows the lead of the `#[\Deprecated]` att=
ribute, which emits&nbsp;<br></div><div>`E_USER_DEPRECATED` for userland=
 functions and `E_DEPRECATED` for native&nbsp;<br></div><div>functions, =
despite being triggered by the engine.<br></div><div><br></div><div>&gt;=
 The examples you used don't support the need for the new attribute.<br>=
</div><div>&gt; Regarding the DateTimeImmutable methods, you said yourse=
lf: "The<br></div><div>&gt; methods are badly named, because they do not=
 actually set the updated<br></div><div>&gt; value". So your proposal su=
ggests adding a brand new thing to PHP to<br></div><div>&gt; deal with b=
ad method names?<br></div><div><br></div><div>`DateTimeImmutable` is not=
 part of the =E2=80=9CUse Cases=E2=80=9D section: Our intended&nbsp;<br>=
</div><div>use-case is the kind of `bulk_process()` functionality that i=
s used for&nbsp;<br></div><div>all the code snippets. But given that the=
 attribute is also useful for&nbsp;<br></div><div>`DateTimeImmutable`, w=
e made it part of the proposal, without it being&nbsp;<br></div><div>par=
t of the intended =E2=80=9Cuser-story=E2=80=9D.<br></div><div><br></div>=
<div>&gt; This problem should be solved using static analysers, IDE, and=
 proper<br></div><div>&gt; code refactoring.<br></div><div><br></div><di=
v>See Volker's reply to Matteo.<br></div><div><br></div><div>Best regard=
s<br></div><div>Tim D=C3=BCsterhus<br></div><div><br></div></blockquote>=
<div><br></div><div id=3D"sig121229152">=E2=80=94 Rob<br></div></body></=
html>
--d766a4c28ecd45a19c0b938045a8dcdf--