Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:126245
Precedence: bulk
MIME-Version: 1.0
References: <a995ac24905030e168e7f6006b996cd5@bastelstu.be> <78eb6c1b-ad20-4b74-b60b-f79623f436db@beccati.com>
In-Reply-To: <78eb6c1b-ad20-4b74-b60b-f79623f436db@beccati.com>
Date: Thu, 30 Jan 2025 17:42:15 +0100
Message-ID: <CAMzRQTcar8N6kAh1oEfMtEjuWBejfGds7jsiW5OZTKn-_3e5rA@mail.gmail.com>
Subject: Re: [PHP-DEV] RFC: Marking return values as important (#[\NoDiscard])
To: Matteo Beccati <php@beccati.com>
Cc: php internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000007fda1c062cef1c7e"
From: volker@tideways-gmbh.com (Volker Dusch)

--0000000000007fda1c062cef1c7e
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Matteo,

On Thu, Jan 30, 2025 at 1:07=E2=80=AFPM Matteo Beccati <php@beccati.com> wr=
ote:

>
> I reckon it's a nice thing to have, but to be honest I think handling it
> just during static analysis would be the best approach. Also there would
> be no need for a (void) cast to suppress the warning.
>
>
We also discussed if this should be part of the core language or something
that is only supported by tooling. During the discussion we looked for
existing context and at what was said around #[Override] [1]  at the time.
I won't repeat Tim's point from last year here for brevity, but I think the
same ideas apply.

Having the core language offer a way to tag a function as =E2=80=9Creturn v=
alue
matters=E2=80=9D would also allow library and tool authors like Psalm and P=
HPStan
(free & pro versions) to add and work with these tags.
Not having each tool to curate its lists of functions where the return
value is considered =E2=80=9Cimportant=E2=80=9D, I think, would be an impro=
vement overall.
It also enables library authors to suggest it to these tools in a
standardized way.

PHPStorm (and EA-Extended inspections plugin) also ship with lists of
functions they warn about that are more extensive than what we propose
here, but they also only work for core as there is so no standardized way
yet, that I'm aware of, to tell PHPStorm (or any LSP) about this type of
requirement.

The `void` cast or the `$_`  discussed currently would be a `/**
@phpstan-suppress UnusedReturnValue */` (or something like that) in
PHPStan. So there are various ways of expressing f this concept in syntax,
and we tried to find something that doesn't add BC issues to PHP in any
way.

There has always been overlap between what SA is doing for its users and
what PHP is doing for all users of the language. From type checking (static
vs. runtime) to warnings and deprecations, things are both checked at
runtime and at CI time. PHP its self is more conservative and has more BC
focus than the tools, of course, but I think this is a case where the
attribute would help with creating a common language.

Kind Regards,
Volker

[1] https://externals.io/message/120233#120432


--=20
Volker Dusch
Head of Engineering
Tideways GmbH
K=C3=B6nigswinterer Str. 116
53227 Bonn
https://tideways.io/imprint

Sitz der Gesellschaft: Bonn
Gesch=C3=A4ftsf=C3=BChrer: Benjamin Au=C3=9Fenhofer (geb. Eberlei)
Registergericht: Amtsgericht Bonn, HRB 22127

--0000000000007fda1c062cef1c7e
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div>Hi Matteo,</div><div dir=3D"ltr"><br></div><div dir=
=3D"ltr">On Thu, Jan 30, 2025 at 1:07=E2=80=AFPM Matteo Beccati &lt;<a href=
=3D"mailto:php@beccati.com">php@beccati.com</a>&gt; wrote:</div><div class=
=3D"gmail_quote gmail_quote_container"><blockquote class=3D"gmail_quote" st=
yle=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padd=
ing-left:1ex"><br>
I reckon it&#39;s a nice thing to have, but to be honest I think handling i=
t <br>
just during static analysis would be the best approach. Also there would <b=
r>
be no need for a (void) cast to suppress the warning.<br><br>
</blockquote></div><div><br></div><div>We also discussed if this should be =
part of the core language or something that is only supported by tooling.=
=C2=A0During the discussion we looked for existing context and at what was =
said around #[Override] [1]=C2=A0 at the time. I won&#39;t repeat Tim&#39;s=
 point from last year here for brevity, but I think the same ideas apply.</=
div><div><br></div><div>Having the core language offer a way to tag a funct=
ion as =E2=80=9Creturn value matters=E2=80=9D would also allow library and =
tool authors like Psalm and PHPStan (free &amp; pro versions) to add and wo=
rk with these tags.</div><div>Not=C2=A0having each tool to curate its lists=
 of functions where the=C2=A0return value is=C2=A0considered =E2=80=9Cimpor=
tant=E2=80=9D, I think, would be an improvement=C2=A0overall. It also enabl=
es library authors to suggest it to these tools in=C2=A0a standardized way.=
</div><div><br></div><div>PHPStorm (and EA-Extended inspections plugin) als=
o ship with lists of functions they warn about that are more extensive than=
 what we propose here, but they also only work for core as there is so no s=
tandardized way yet, that I&#39;m aware of, to tell PHPStorm (or any LSP) a=
bout this type of requirement.=C2=A0</div><div><br></div><div>The `void` ca=
st or the `$_`=C2=A0 discussed currently would be a `<span style=3D"backgro=
und-color:rgba(232,232,232,0.04);color:rgb(232,145,45);font-family:Monaco,M=
enlo,Consolas,&quot;Courier New&quot;,monospace;font-size:12px;font-variant=
-ligatures:none">/** @phpstan-suppress UnusedReturnValue */</span>` (or som=
ething like that) in PHPStan. So there are various ways of expressing f thi=
s concept in syntax, and we tried to find something that doesn&#39;t add BC=
 issues to PHP in any way.=C2=A0=C2=A0</div><div><br></div><div>There has a=
lways been overlap between what SA is doing for its users and what PHP is d=
oing for all users of the language. From type checking (static vs. runtime)=
 to warnings and deprecations, things are both checked at runtime and at CI=
 time. PHP its self is more conservative and has more BC focus than the too=
ls, of course, but I think this is a case where the attribute would help wi=
th=C2=A0creating a common language.</div><div><br></div><div>Kind Regards,<=
/div><div>Volker</div><div><br></div><div>[1]=C2=A0<a href=3D"https://exter=
nals.io/message/120233#120432">https://externals.io/message/120233#120432</=
a></div><div><br></div><div><br></div><span class=3D"gmail_signature_prefix=
">-- </span><br><div dir=3D"ltr" class=3D"gmail_signature"><div dir=3D"ltr"=
><div style=3D"color:rgb(34,34,34);font-family:arial,sans-serif;font-size:1=
2.8px">Volker Dusch</div><div style=3D"color:rgb(34,34,34);font-family:aria=
l,sans-serif;font-size:12.8px">Head of Engineering</div><div style=3D"color=
:rgb(34,34,34);font-family:arial,sans-serif;font-size:12.8px"><div>Tideways=
 GmbH</div><div>K=C3=B6nigswinterer Str. 116</div><div>53227 Bonn</div><div=
><a href=3D"https://tideways.io/imprint" style=3D"color:rgb(17,85,204)" tar=
get=3D"_blank">https://tideways.io/imprint</a><br></div><div><br></div><div=
>Sitz der Gesellschaft: Bonn</div><div>Gesch=C3=A4ftsf=C3=BChrer: Benjamin =
Au=C3=9Fenhofer (geb. Eberlei)</div><div>Registergericht: Amtsgericht Bonn,=
 HRB 22127</div></div></div></div></div>

--0000000000007fda1c062cef1c7e--