# RFC: Marking return values as important (#[\NoDiscard]) - Thread: https://externals.io/message/126232 - Original archive: https://news-web.php.net/php.internals/126232 - Started by tim@bastelstu.be on 2025-01-29T15:12:35+00:00 - Messages: 48 - Page 1 of 2: messages 1 to 25 (25 per page, add `?page=N` to the URL) - Next page: https://externals.io/message/126232.md?page=2 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · RFCs at https://externals.io/rfc.md · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #126232 — tim@bastelstu.be — 2025-01-29T15:12:35+00:00 URL: https://externals.io/message/126232#126232 Hi Volker and I would like to start discussion on our RFC to allow "Marking return values as important (#[\NoDiscard])". Please find the following resources for your reference: - RFC: https://wiki.php.net/rfc/marking_return_value_as_important - Implementation: https://github.com/php/php-src/pull/17599 Best regards Tim Düsterhus --- ## #126233 — Rob Landers — 2025-01-29T19:31:38+00:00 URL: https://externals.io/message/126232#126233 · In reply to: #126232 > Hi > > Volker and I would like to start discussion on our RFC to allow "Marking > return values as important (#[\NoDiscard])". > > Please find the following resources for your reference: > > - RFC: https://wiki.php.net/rfc/marking_return_value_as_important > - Implementation: https://github.com/php/php-src/pull/17599 > > Best regards > Tim Düsterhus > Hi Tim, This looks promising! Why do we need the (void) cast? Wouldn't "@" work and avoid a BC issue? — Rob --- ## #126234 — tim@bastelstu.be — 2025-01-29T19:50:19+00:00 URL: https://externals.io/message/126232#126234 · In reply to: #126233 Hi > This looks promising! > > Why do we need the (void) cast? Wouldn't "@" work and avoid a BC issue? The `@` operator also suppresses any other warning or notice within the call chain. That is likely undesirable because it might hide issues one is interested in. See this example (https://3v4l.org/NDtR7): Hi > > > > […] > > The `@` operator also suppresses any other warning or notice within the > call chain. That is likely undesirable because it might hide issues one > is interested in. See this example (https://3v4l.org/NDtR7): > > function inner() { > var_dump($undefined); > } > > function outer() { > inner(); > } > > outer(); > outer(); > @outer(); > > The `(void)` cast makes the intent of not being interested in the return > value clear. In C casting to `(void)` is an established pattern of > "using" a value that the compiler would otherwise consider unused. > Likewise will the `void` operator suppress the TypeScript-ESLint warning > of not using returned `Promise`: > https://typescript-eslint.io/rules/no-floating-promises/#ignorevoid > > Best regards > Tim Düsterhus > Hi Tim, I understand what you are saying, but I can also just remove the warning via: $_ = outer; Which, to me, is clearer on the intent than (void)outer(); It also happens to make the diffs more natural looking when and if the return value gets used during a code review. I'll also note that having (void) doesn't preclude me from using $_ either. I was mostly just curious as to the reasoning; I don't like it, but it makes sense. Best of luck! — Rob --- ## #126236 — Christian Schneider — 2025-01-29T22:48:03+00:00 URL: https://externals.io/message/126232#126236 · In reply to: #126235 Am 29.01.2025 um 21:16 schrieb Rob Landers : > > > […] > > I understand what you are saying, but I can also just remove the warning via: > > $_ = outer(); > > Which, to me, is clearer on the intent than (void)outer(); It also happens to make the diffs more natural looking when and if the return value gets used during a code review. I'll also note that having (void) doesn't preclude me from using $_ either. I think the (void) cast is familiar for people from C or the like, the only caveat is that it is not backward compatible, i.e. code using it won't work on older versions of PHP so migrating to it is not seamless, so it probably should not be used for a while in public libraries / packages. I guess another option would be to define a function nop($_) {} and then use nop(outer()); but my question is whether $_ = outer() and nop(outer()) will be guaranteed to not being optimized in a way which causes the warning to reappear. I don't know much about the optimizations being done but this could be an issue in the future, no? Regards, - Chris --- ## #126237 — Rowan Tommins [IMSoP] — 2025-01-30T09:02:21+00:00 URL: https://externals.io/message/126232#126237 · In reply to: #126236 >I think the (void) cast is familiar for people from C or the like, the only caveat is that it is not backward compatible, i.e. code using it won't work on older versions of PHP so migrating to it is not seamless, so it probably should not be used for a while in public libraries / packages. I suspect that a majority of PHP users will probably have had no experience in C, which is easy to forget on this list where we're working on a C codebase. In C, "void" has various uses in the type system, but in PHP this would effectively just be a specific keyword for this case. I think it would be good to explore alternatives - for instance, I think C# has a reserved _ variable to assign discarded results to, but may be misremembering. >but my question is whether $_ = outer() and nop(outer()) will be guaranteed to not being optimized in a way which causes the warning to reappear. I don't know much about the optimizations being done but this could be an issue in the future, no? I second this concern: having code that you think has suppressed the warning later be optimized and start warning could be very confusing, and given the backwards compatibility issue, users will need something other than "(void)" which is reliable. My other thought reading the proposal is that if this can be done efficiently, can we use the same approach to warn when a "void" function's return *is* used? Regards, Rowan Tommins [IMSoP] --- ## #126238 — Matteo Beccati — 2025-01-30T12:06:34+00:00 URL: https://externals.io/message/126232#126238 · In reply to: #126232 Hi Tim, > Hi > > Volker and I would like to start discussion on our RFC to allow "Marking > return values as important (#[\NoDiscard])". > > Please find the following resources for your reference: > > - RFC: https://wiki.php.net/rfc/marking_return_value_as_important > - Implementation: https://github.com/php/php-src/pull/17599 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. Cheers -- Matteo Beccati Development & Consulting - http://www.beccati.com/ --- ## #126240 — tim@bastelstu.be — 2025-01-30T15:50:30+00:00 URL: https://externals.io/message/126232#126240 · In reply to: #126235 Hi Am 2025-01-29 21:16, schrieb Rob Landers: > I understand what you are saying, but I can also just remove the > warning via: > > $_ = outer; Please note that `$_` is a regular variable and thus will increase the lifetimes of returned objects (and arrays that can contain objects), which might or might not be observable when implementing `__destruct()`. Also note that OPcache will actually optimize away dead stores if it can guarantee that the difference is not observable (i.e. if it knows that the function never returns an object and if `get_defined_vars()` is not called). Of course it would be possible to exclude `$_` from this dead-store optimization with PHP 8.5+ to allow for a smoother migration for libraries that require cross-version compatibility. We nevertheless wanted to offer an alternative that might be less confusing than a “special variable” that might also conflict with diagnostics like unused variable detection. > […] It also happens to make the diffs more natural looking when and if > the return value gets used during a code review. […] Can you clarify what you mean by “more natural looking”? Best regards Tim Düsterhus --- ## #126241 — tim@bastelstu.be — 2025-01-30T15:50:59+00:00 URL: https://externals.io/message/126232#126241 · In reply to: #126236 Hi Am 2025-01-29 23:48, schrieb Christian Schneider: > I guess another option would be to define a > function nop($_) {} > and then use > nop(outer()); > but my question is whether $_ = outer() and nop(outer()) will be > guaranteed to not being optimized in a way which causes the warning to > reappear. I don't know much about the optimizations being done but this > could be an issue in the future, no? For `$_` see my reply to Rob. For the function: OPcache will attempt to inline functions that only return a (implicit) return statement. However this only works if the function is defined in the same file, as OPcache currently cannot perform cross-file optimizations. I'd say that defining the `nop()` function in a dedicated file is currently the most reliable solution to prevent OPcache from optimizing away the usage in a backwards-compatible way. Of course the best solution is actually doing something useful with the return value, as the attribute is intended to point out when not using the return value is very likely to be wrong. We would expect it to be rarely necessary to just suppress the warning. Best regards Tim Düsterhus --- ## #126242 — tim@bastelstu.be — 2025-01-30T15:51:09+00:00 URL: https://externals.io/message/126232#126242 · In reply to: #126237 Hi Am 2025-01-30 10:02, schrieb Rowan Tommins [IMSoP]: > I think it would be good to explore alternatives - for instance, I > think C# has a reserved _ variable to assign discarded results to, but > may be misremembering. A quick search confirms that C# supports the `_` variable. It's also supported with Rust. We are open to possible alternatives to the `(void)` cast, this was also something we discussed while drafting the RFC. However we opted for the `(void)` with out proposal for the following reasons: - `$_ = func();` is already working as a regular variable (see my reply to Rob regarding lifetimes), thus repurposing it as a “discard pile” would cause backwards compatibility issues. - `_ = func();` would introduce new syntax (just like `(void)`), but does not look and feel like PHP to us. - `(void)func();` has precedent in other languages (just like `_ =`) and looks and feels like PHP. In fact in earlier PHP versions there already was an `(unset)` cast to unconditionally turn an expression into `null`. We opted for `(void)` instead of bringing back `(unset)` for the following reasons: (1) Possible confusion about a deprecated + removed feature coming back (e.g. avoiding old blog articles). (2) More cleanly enforcing that `(void)` is a statement, rather than an expression. (3) The similarity with the `void` return type to indicate that “nothing” is returned. > > […] > > I second this concern: having code that you think has suppressed the > warning later be optimized and start warning could be very confusing, > and given the backwards compatibility issue, users will need something > other than "(void)" which is reliable. See my replies to Rob and Christian. > My other thought reading the proposal is that if this can be done > efficiently, can we use the same approach to warn when a "void" > function's return *is* used? Performing this check would be possible, but it would likely be less efficient. The current implementation of `#[\NoDiscard]` optimizes based on the assumption that the attribute is going to be comparatively rarely used (i.e. only when it actually matters, as per the “Recommended Usage” section of the RFC). The “Implementation Details” section already mentions this, but basically what's happening is that the checks for `#[\Deprecated]` and `#[\NoDiscard]` are happening at the same time (it's a bitflag check) and you are only paying the cost of the more detailed checks if the function is Deprecated or NoDiscard (or both). Then there's also the “OPcode specialization” to remove `#[\NoDiscard]` from the bitflag check when the return value is known to be used. And finally functions that are known to neither be `#[\Deprecated]` nor `#[\NoDiscard]` at compile time (this is mostly *fully-qualified* native functions) are going through an even faster OPcode that completely omits several safety checks. You can check this for yourself by passing a script through `php -d opcache.enable_cli=1 -d opcache.opt_debug_level=0x20000 script.php`. If you see `DO_ICALL` (native functions) or `DO_UCALL` (userland functions) then that's the optimized version omitting the checks. Here's an example: > 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 “return value matters” would also allow library and tool authors like Psalm and PHPStan (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 “important”, I think, would be an improvement 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 -- Volker Dusch Head of Engineering Tideways GmbH Königswinterer Str. 116 53227 Bonn https://tideways.io/imprint Sitz der Gesellschaft: Bonn Geschäftsführer: Benjamin Außenhofer (geb. Eberlei) Registergericht: Amtsgericht Bonn, HRB 22127 --- ## #126246 — Claude Pache — 2025-01-30T17:48:39+00:00 URL: https://externals.io/message/126232#126246 · In reply to: #126242 > Le 30 janv. 2025 à 16:51, Tim Düsterhus a écrit : > > Hi > > Am 2025-01-30 10:02, schrieb Rowan Tommins [IMSoP]: > > […] > > A quick search confirms that C# supports the `_` variable. It's also supported with Rust. > > We are open to possible alternatives to the `(void)` cast, this was also something we discussed while drafting the RFC. However we opted for the `(void)` with out proposal for the following reasons: > > - `$_ = func();` is already working as a regular variable (see my reply to Rob regarding lifetimes), thus repurposing it as a “discard pile” would cause backwards compatibility issues. > - `_ = func();` would introduce new syntax (just like `(void)`), but does not look and feel like PHP to us. > - `(void)func();` has precedent in other languages (just like `_ =`) and looks and feels like PHP. In fact in earlier PHP versions there already was an `(unset)` cast to unconditionally turn an expression into `null`. We opted for `(void)` instead of bringing back `(unset)` for the following reasons: (1) Possible confusion about a deprecated + removed feature coming back (e.g. avoiding old blog articles). (2) More cleanly enforcing that `(void)` is a statement, rather than an expression. (3) The similarity with the `void` return type to indicate that “nothing” is returned. A possible alternative is: `! func();`, that already “works” as of today, modulo possible optimisation. In any case, I think that `(void) func();` is not self-explanatory, and in the event I want to ignore a result I am not supposed to ignore, I will probably write something more explicit like, `$unused = func();`(possibly decorated with an annotation telling static analysers not to complain that $unused is unused), or `forget(func())` BTW, the RFC says that introducing the `(void)` cast is a backward compatible change, which is incorrect; see: https://3v4l.org/iN7OY —Claude --- ## #126249 — Rob Landers — 2025-01-30T19:59:10+00:00 URL: https://externals.io/message/126232#126249 · In reply to: #126240 > Hi > > Am 2025-01-29 21:16, schrieb Rob Landers: > > […] > > Please note that `$_` is a regular variable and thus will increase the > lifetimes of returned objects (and arrays that can contain objects), > which might or might not be observable when implementing `__destruct()`. > Also note that OPcache will actually optimize away dead stores if it can > guarantee that the difference is not observable (i.e. if it knows that > the function never returns an object and if `get_defined_vars()` is not > called). If $_ gets optimized away and causes the warning, then this seems like a (future) bug with opcache (it optimized something away and it was observable). It also shouldn't matter if you use $_, $unused, or $whatever; I just chose that as an example as someone who came from C# + Scala + Go lineage. Some might say that is a feature, though... and the inconsistency (whether opcache is used or not) may not be ideal. For example: $lock = flock($fp); -if ($lock) { +if(true) { // do stuff This diff is rather obvious something went wrong here; but use your imagination to imagine a diff where this is not so clear. In this case $lock accidentally becomes unused. If opcache is enabled and optimizes this code, we get the warning we probably desire, but without opcache, we do not get the warning (tests usually have opcache disabled)! > > Of course it would be possible to exclude `$_` from this dead-store > optimization with PHP 8.5+ to allow for a smoother migration for > libraries that require cross-version compatibility. I believe the draft pattern matching RFC uses this variable name. It is probably good to align there. > > We nevertheless wanted to offer an alternative that might be less > confusing than a “special variable” that might also conflict with > diagnostics like unused variable detection. > > > […] > > Can you clarify what you mean by “more natural looking”? Here are two diffs: -(void)doSomething(); +$var = doSomething(); +somethingElse($var); or -$_ = doSomething(); +$var = doSomething(); +somethingElse($var); At the expense of adding some html... I've added syntax highlighting for those using html readers to show how it might look in something like github. It is immediately clear what has changed: only the variable name is highlighted as a change on that line, letting me know what to look for. Though, as I mentioned, I come from a lineage of languages where _ is a thing, so it looks more natural to me (other languages using _ are Rust, Swift and Zig). For someone coming from more C/Typescript backgrounds, they may disagree. — Rob --- ## #126257 — tim@bastelstu.be — 2025-01-31T11:07:33+00:00 URL: https://externals.io/message/126232#126257 · In reply to: #126246 Hi Am 2025-01-30 18:48, schrieb Claude Pache: > BTW, the RFC says that introducing the `(void)` cast is a backward > compatible change, which is incorrect; see: https://3v4l.org/iN7OY :-| Thank you for pointing that out. We've adjusted the “Backward Incompatible Changes” section accordingly and also changed threshold for the separate vote for the inclusion of `(void)` from 50%+1 to 2/3 to more accurately reflect the possible impact. Best regards Tim Düsterhus --- ## #126283 — Derick Rethans — 2025-02-04T11:10:51+00:00 URL: https://externals.io/message/126232#126283 · In reply to: #126232 > Hi > > Volker and I would like to start discussion on our RFC to allow "Marking > return values as important (#[\NoDiscard])". > > Please find the following resources for your reference: > > - RFC: https://wiki.php.net/rfc/marking_return_value_as_important > - Implementation: https://github.com/php/php-src/pull/17599 Looks reasonable to me. What's the reason why this emits a warning though, instead of throwing an Exception? I'm sure you thought of it, but it would be nice to have that consideration mentioned in the RFC. cheers, Derick --- ## #126292 — Volker Dusch — 2025-02-05T10:58:19+00:00 URL: https://externals.io/message/126232#126292 · In reply to: #126283 > What's the reason why this emits a warning though, instead of throwing > an Exception? I'm sure you thought of it, but it would be nice to have > that consideration mentioned in the RFC. > Thank you for the question, I now added the missing reasoning to the RFC text. > A E_(USER_)WARNING was chosen over an exception to avoid BC concerns, as throwing would potentially break existing code that "works as implemented" and because this is only a warning in the literal sense of the word, given that the function executes successfully. Given that PHP uses Exceptions only in hard failure cases, we found it to be more prudent to go this way. If users really want, they can suppress/ignore this warning without adapting existing code, e.g., using the proposed (void) cast. But the main reason was to not cause hard failures through exceptions. Kind Regards, Volker -- Volker Dusch Head of Engineering Tideways GmbH Königswinterer Str. 116 53227 Bonn https://tideways.io/imprint Sitz der Gesellschaft: Bonn Geschäftsführer: Benjamin Außenhofer (geb. Eberlei) Registergericht: Amtsgericht Bonn, HRB 22127 --- ## #126377 — Volker Dusch — 2025-02-12T11:52:15+00:00 URL: https://externals.io/message/126232#126377 · In reply to: #126232 > Volker and I would like to start discussion on our RFC to allow "Marking > return values as important (#[\NoDiscard])". > > Please find the following resources for your reference: > > - RFC: https://wiki.php.net/rfc/marking_return_value_as_important > - Implementation: https://github.com/php/php-src/pull/17599 Hello everyone, it's been two weeks, but given the feedback we received, we don't feel the discussion didn't reach a conclusive resolution yet. So we wanted to solicit additional opinions. In an attempt to summarize the discussion, how we see it, there are two main points of contention we'd like to discuss further. a) Intentionally discarding the return value The `(void)flock($fp);` syntax was the main thing discussed on the list. With `$_ = flock($fp);` as the most suggested alternative. The issue with using `$_` as it currently stands, is that the unused variable would be cleaned up by OPcache. Should we instead have a special case in OPcache ensuring it does not apply its optimization to a variable if it's named `$_` instead? The semantic difference would be that $_ would keep returned values (and objects) alive and in memory, whereas (void) would discard them immediately. Leading to different times when, for example, destructors would be called. The consequence of this would be that 3rd party tools like IDEs and Static and Dynamic Code Analyzers would also have to build that special case in to not produce "unused variable" warnings. We're also happy to change the secondary vote to "(void) vs $_" as we feel there needs to be a way to discard the return value consistently for this to be a complete and usable feature. b) Naming of the attribute Nobody mentioned this on the list, but before opening a vote we'd like to heard if the attribute name makes sense to you. We've chosen #[NoDiscard] as it's also used in C and C++. See [1] for the full list of references. If you feel this doesn't fit with PHP, we welcome other suggestions. [1] https://wiki.php.net/rfc/marking_return_value_as_important#precedent Kind Regards, Volker -- Volker Dusch Head of Engineering Tideways GmbH Königswinterer Str. 116 53227 Bonn https://tideways.io/imprint Sitz der Gesellschaft: Bonn Geschäftsführer: Benjamin Außenhofer (geb. Eberlei) Registergericht: Amtsgericht Bonn, HRB 22127 --- ## #126378 — Bilge — 2025-02-12T13:17:55+00:00 URL: https://externals.io/message/126232#126378 · In reply to: #126232 > Hi > > Volker and I would like to start discussion on our RFC to allow > "Marking return values as important (#[\NoDiscard])". > > Please find the following resources for your reference: > > - RFC: https://wiki.php.net/rfc/marking_return_value_as_important > - Implementation: https://github.com/php/php-src/pull/17599 > > Best regards > Tim Düsterhus Apologies if this has already been brought up; I haven't read the entire thread, but isn't the entire premise of this RFC based on a falsehood? >This kind of “partial success” can only be reliably communicated by means of a return value Exceptions are objects, so you can attach whatever additional information you wish to that object. Perhaps the word "reliably" is doing a lot of heavy lifting in that sentence, but if you would refute me (which presumably you will), then I think this needs to be explained in better detail in the RFC, because it looks to me that exploiting object properties is just as viable as returning something, and probably preferred, since this solves your problem of the user not handling the failure. Cheers, Bilge --- ## #126379 — Kamil Tekiela — 2025-02-12T14:07:51+00:00 URL: https://externals.io/message/126232#126379 · In reply to: #126232 Hi Tim and Volker, 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. 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 developer forgets to use a return value, a static analysis tool should inform the developer of the mistake. I understand that not all functions are pure like in your example, but adding an attribute 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. My second gripe is the use of `E_WARNING` and E_USER_WARNING. I am of the opinion that warnings in PHP are a really bad idea. PHP isn't a language like C or C++ where the warnings would show up only during the compilation phase. In PHP warnings are runtime errors. The code should emit an exception instead of a warning. It would also make it 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 the PHP engine generating `E_USER_WARNING` which should be reserved only for warnings triggered by trigger_error. 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? This problem should be solved using static analysers, IDE, and proper code refactoring. Regards, Kamil --- ## #126380 — tim@bastelstu.be — 2025-02-12T16:43:03+00:00 URL: https://externals.io/message/126232#126380 · In reply to: #126379 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 --- ## #126381 — Rob Landers — 2025-02-12T20:57:34+00:00 URL: https://externals.io/message/126232#126381 · In reply to: #126380 > Hi > > Am 2025-02-12 15:07, schrieb Kamil Tekiela: > > […] > > 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? > > > […] > > 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). > > > […] > > 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. > > > […] > > See Volker's reply to Derick. > > > […] > > 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. > > > […] > > 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. > > > […] > > `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”. > > > […] > > See Volker's reply to Matteo. > > Best regards > Tim Düsterhus > — Rob --- ## #126382 — Larry Garfield — 2025-02-12T21:31:44+00:00 URL: https://externals.io/message/126232#126382 · In reply to: #126377 > > > […] > > Hello everyone, > > it's been two weeks, but given the feedback we received, we don't feel > the discussion didn't reach a conclusive resolution yet. > > So we wanted to solicit additional opinions. In an attempt to summarize > the discussion, how we see it, there are two main points of contention > we'd like to discuss further. I'm still undecided on the RFC overall, but one thing that is problematic is the phrasing of the messages. Currently, the messages in the attribute are fragments of an English sentence, seemingly designed to fit grammatically with a sentence fragment that is coded into the engine somewhere but not readily available to developers. From my phrasing I think you can guess my opinion of that. That is impossible to document cleanly for English speakers. It will not translate at all for anyone who is writing in a non-English language (which people do). People are going to get it wrong more than they get it right, in any language. Instead, the wording should be structured to be a complete sentence, and the built-in message updated to make that logical. That gives the developer much more freedom to write a meaningful, contextually-relevant message in the language of their choice. --Larry Garfield --- ## #126384 — Jorg Sowa — 2025-02-12T21:44:24+00:00 URL: https://externals.io/message/126232#126384 · In reply to: #126232 I must agree with Kamil. I don't see practical benefits of this feature that would surpass the implications it has for the language. We already have static analysis handling such cases and it can be extended even to non-pure functions. Moreover, syntax (void) adds additional complexity to beginners' understanding of the typing system. The second option with $_ is BC on the other hand, so both options are unsatisfying. And honestly, I don't have good ideas for it. Kind regards, Jorg Sowa --- ## #126385 — Morgan — 2025-02-13T00:31:18+00:00 URL: https://externals.io/message/126232#126385 · In reply to: #126377 > > b) Naming of the attribute > > Nobody mentioned this on the list, but before opening a vote we'd like > to heard if the attribute name makes sense to you. On this, it could be spelled #[\MustUse], rather than phrase it as a negative. --- ## #126386 — Jakob Givoni — 2025-02-13T08:16:49+00:00 URL: https://externals.io/message/126232#126386 · In reply to: #126385 My thoughts overall on this: 0. I'm not against introducing the attribute, only how it's used/enforced etc. (And, incidentally, what it actually MEANS, see if you can spot that from the rest of my comments....) 1. Static analysers and IDEs: I agree with everyone who's said that whether or not the return value is "used" should be a concern for tooling like static analysers and the IDE, and ideally not for PHP runtime. Attributes were added as a structured replacement for docblock props and I don't like it when they affect how a program actually runs (as long as you're not using reflection). 2. If it must be a concern for PHP at runtime, a warning is preferred over an exception. It only makes sense – it's in the name: Exceptions are for exceptional situations, warnings are for warning you. 3. Naming of the attribute: I think the most precise name (if I understand the purpose correctly) is: #[ReturnValueMayContainCriticalInformation] 4. Should all internal functions, such as `fopen()` (which may return false on error instead of throwing an exception), have this attribute? I understand that ideally we only want to add it where the failure to inspect the return value could result in something outright dangerous, but I think that's a really hard call to make. If we scale it back to mean "Look, this function may not behave the way you'd expect it to, - the return value may actually contain some information you might be interested in, so you better check it man", then I think it could be a small but useful addition. Best, Jakob --- Next page: https://externals.io/message/126232.md?page=2