RFC: Marking return values as important (#[\NoDiscard]) #17599
Conversation
TimWolla
force-pushed
the
return-value-unused
branch
from
d7c712e to
296b7a4
Compare
TimWolla
force-pushed
the
return-value-unused
branch
2 times, most recently
from
59254ee to
7f8ad60
Compare
TimWolla
force-pushed
the
return-value-unused
branch
from
7f8ad60 to
6f8d462
Compare
TimWolla
force-pushed
the
return-value-unused
branch
from
6f8d462 to
7a5a475
Compare
| ir_GUARD(ret, jit_STUB_ADDR(jit, jit_stub_exception_handler)); | ||
| } | ||
|
|
||
| if ((func->common.fn_flags & ZEND_ACC_NODISCARD) && !RETURN_VALUE_USED(opline)) { |
There was a problem hiding this comment.
You should probably combine these two checks, and handle them both in the helper to avoid assembly-bloat.
There was a problem hiding this comment.
I tried that initially, but failed to access RETURN_VALUE_USED(opline) within the helper. It's also doing some shenanigans with getting OPLINE_D as parameter and then treating it as zend_execute_data*. I would need an explanation of how to make this change.
But independent of that, I expect the warning to rarely be triggered in practice, because it is meant to point out unsafe code. Thus I would also expect this branch not to be taken, thus not generating the corresponding assembly. I would expect this to be more efficient than merging the two checks in a single helper.
There was a problem hiding this comment.
Sorry, I meant to comment this for the function JIT above. The tracing part is fine.
I tried that initially, but failed to access RETURN_VALUE_USED(opline) within the helper
You shouldn't need to. You can create two versions, one that checks for deprecated only, and one that checks for deprecated and deprecated & nodiscard that is only emitted when the opline result is known to be unused at JIT-time.
| @@ -3927,15 +3927,15 @@ ZEND_API uint8_t zend_get_call_op(const zend_op *init_op, zend_function *fbc) /* | |||
| ZEND_ASSERT(!(fbc->common.fn_flags & ZEND_ACC_CALL_VIA_TRAMPOLINE)); | |||
| if (fbc->type == ZEND_INTERNAL_FUNCTION && !(CG(compiler_options) & ZEND_COMPILE_IGNORE_INTERNAL_FUNCTIONS)) { | |||
| if (init_op->opcode == ZEND_INIT_FCALL && !zend_execute_internal) { | |||
| if (!(fbc->common.fn_flags & ZEND_ACC_DEPRECATED)) { | |||
| if (!(fbc->common.fn_flags & (ZEND_ACC_DEPRECATED|ZEND_ACC_NODISCARD))) { | |||
There was a problem hiding this comment.
Similarly, we can allow ZEND_DO_ICALL if we know the return value is used. Annoyingly, the return value is only removed after the call has compiled:
Line 813 in e1f3249
Maybe this could be improved by handlingZEND_AST_CALL in zend_compile_stmt() and passing a NULL result.
| return result; | ||
| } | ||
|
|
||
| ZEND_API ZEND_COLD void ZEND_FASTCALL zend_nodiscard_function(const zend_function *fbc) |
There was a problem hiding this comment.
Nit: Name should include error or throw.
There was a problem hiding this comment.
This naming is consistent with zend_deprecated_function(), zend_deprecated_class_constant(), zend_false_to_array_deprecated() and probably others. I can rename the function (should I then rename all of them)?
There was a problem hiding this comment.
Since they are ZEND_API, renaming them seems unnecessary. I'd prefer just properly naming the new ones, but I understand your reasoning to keep it consistent.
| zend_deprecated_function(fbc); | ||
| const uint32_t no_discard = (!RETURN_VALUE_USED(opline)) * ZEND_ACC_NODISCARD; | ||
|
|
||
| if (UNEXPECTED((fbc->common.fn_flags & (ZEND_ACC_DEPRECATED|no_discard)) != 0)) { |
There was a problem hiding this comment.
Nit: The vast majority of code omits != 0. I'd prefer removing it, it's only noise.
Should not warn, but the RFC explicitly acknowledges OPcache optimizations (which is part of the reason for the explicit |
RFC: https://wiki.php.net/rfc/marking_return_value_as_important
Open Issues: