Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:126380
Precedence: bulk
MIME-Version: 1.0
Date: Wed, 12 Feb 2025 17:43:03 +0100
To: Kamil Tekiela <tekiela246@gmail.com>
Cc: Volker Dusch <volker@tideways-gmbh.com>, php internals
 <internals@lists.php.net>
Subject: Re: [PHP-DEV] RFC: Marking return values as important (#[\NoDiscard])
In-Reply-To: <CAGBsUrfFvWBcncpNorb3dmXJ_=cZLhmdaVkFHR6L5SkrhtPLcw@mail.gmail.com>
References: <a995ac24905030e168e7f6006b996cd5@bastelstu.be>
 <CAGBsUrfFvWBcncpNorb3dmXJ_=cZLhmdaVkFHR6L5SkrhtPLcw@mail.gmail.com>
Message-ID: <95a69ca549b246c3645f8c6db519f669@bastelstu.be>
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
From: tim@bastelstu.be (=?UTF-8?Q?Tim_D=C3=BCsterhus?=)

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.

Related to the above example and also to the `flock()` function: A 
developer might want to write a locking function that returns a lock 
resource that will automatically unlock when it goes out of scope, 
preventing the unlock from being forgotten. It might not be immediately 
obvious that the locking function returns a lock resource, especially 
when it's associated with another object:

     $cacheDriver->lock('someCacheEntry');

Is there a `$cacheDriver->unlock('someCacheEntry')` or will `->lock()` 
return a lock resource? Having the attribute on the `lock()` method can 
help.

Searching for `language:rust "must_use ="` on GitHub reveals that a 
common use-case in Rust is lazy iterators, where you can call 
higher-order functions (e.g. map() or filter()), but they will not do 
anything unless you explicitly “poll” them at least once. As far as I 
know, Java has lazy streams that behave identically.

> 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'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.

> 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.

> 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.

> 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