RFC: Marking return values as important (#[\NoDiscard])

Zend/Optimizer/optimize_func_calls.c

@@ -78,8 +78,10 @@ static void zend_delete_call_instructions(zend_op_array *op_array, zend_op *opli
 
 static void zend_try_inline_call(zend_op_array *op_array, zend_op *fcall, zend_op *opline, zend_function *func)
 {
+	const uint32_t no_discard = RETURN_VALUE_USED(opline) ? 0 : ZEND_ACC_NODISCARD;
+
 	if (func->type == ZEND_USER_FUNCTION
-	 && !(func->op_array.fn_flags & (ZEND_ACC_ABSTRACT|ZEND_ACC_HAS_TYPE_HINTS|ZEND_ACC_DEPRECATED))
+	 && !(func->op_array.fn_flags & (ZEND_ACC_ABSTRACT|ZEND_ACC_HAS_TYPE_HINTS|ZEND_ACC_DEPRECATED|no_discard))
 		/* TODO: function copied from trait may be inconsistent ??? */
 	 && !(func->op_array.fn_flags & (ZEND_ACC_TRAIT_CLONE))
 	 && fcall->extended_value >= func->op_array.required_num_args
@@ -202,18 +204,12 @@ void zend_optimize_func_calls(zend_op_array *op_array, zend_optimizer_ctx *ctx)
 						fcall->op1.num = zend_vm_calc_used_stack(fcall->extended_value, call_stack[call].func);
 						literal_dtor(&ZEND_OP2_LITERAL(fcall));
 						fcall->op2.constant = fcall->op2.constant + 1;
-						if (opline->opcode != ZEND_CALLABLE_CONVERT) {
-							opline->opcode = zend_get_call_op(fcall, call_stack[call].func);
-						}
 					} else if (fcall->opcode == ZEND_INIT_NS_FCALL_BY_NAME) {
 						fcall->opcode = ZEND_INIT_FCALL;
 						fcall->op1.num = zend_vm_calc_used_stack(fcall->extended_value, call_stack[call].func);
 						literal_dtor(&op_array->literals[fcall->op2.constant]);
 						literal_dtor(&op_array->literals[fcall->op2.constant + 2]);
 						fcall->op2.constant = fcall->op2.constant + 1;
-						if (opline->opcode != ZEND_CALLABLE_CONVERT) {
-							opline->opcode = zend_get_call_op(fcall, call_stack[call].func);
-						}
 					} else if (fcall->opcode == ZEND_INIT_STATIC_METHOD_CALL
 							|| fcall->opcode == ZEND_INIT_METHOD_CALL
 							|| fcall->opcode == ZEND_INIT_PARENT_PROPERTY_HOOK_CALL
@@ -223,6 +219,16 @@ void zend_optimize_func_calls(zend_op_array *op_array, zend_optimizer_ctx *ctx)
 						ZEND_UNREACHABLE();
 					}
 
+					/* If the INIT opcode changed the DO opcode can also change to
+					 * a more optimized one.
+					 *
+					 * At this point we also know whether or not the result of
+					 * the DO opcode is used, allowing to optimize calls to
+					 * ZEND_ACC_NODISCARD functions. */
+					if (opline->opcode != ZEND_CALLABLE_CONVERT) {
+						opline->opcode = zend_get_call_op(fcall, call_stack[call].func, !RESULT_UNUSED(opline));
+					}
+
 					if ((ZEND_OPTIMIZER_PASS_16 & ctx->optimization_level)
 							&& call_stack[call].try_inline
 							&& opline->opcode != ZEND_CALLABLE_CONVERT) {

Zend/tests/attributes/nodiscard/001.phpt

@@ -0,0 +1,86 @@
+--TEST--
+#[\NoDiscard]: Basic test.
+--FILE--
+<?php
+
+#[\NoDiscard]
+function test(): int {
+	return 0;
+}
+
+#[\NoDiscard("this is important")]
+function test2(): int {
+	return 0;
+}
+
+#[\NoDiscard]
+function test3(...$args): int {
+	return 0;
+}
+
+class Clazz {
+	#[\NoDiscard]
+	function test(): int {
+		return 0;
+	}
+
+	#[\NoDiscard("this is important")]
+	function test2(): int {
+		return 0;
+	}
+
+	#[\NoDiscard]
+	static function test3(): int {
+		return 0;
+	}
+}
+
+$closure = #[\NoDiscard] function(): int {
+	return 0;
+};
+
+$closure2 = #[\NoDiscard] function(): int {
+	return 0;
+};
+
+test();
+test2();
+test3(1, 2, named: 3);
+call_user_func("test");
+$fcc = test(...);
+$fcc();
+
+$cls = new Clazz();
+$cls->test();
+$cls->test2();
+Clazz::test3();
+
+call_user_func([$cls, "test"]);
+
+$closure();
+
+$closure2();
+
+?>
+--EXPECTF--
+Warning: The return value of function test() should either be used or intentionally ignored by casting it as (void) in %s on line %d
+
+Warning: The return value of function test2() should either be used or intentionally ignored by casting it as (void), this is important in %s on line %d
+
+Warning: The return value of function test3() should either be used or intentionally ignored by casting it as (void) in %s on line %d
+
+Warning: The return value of function test() should either be used or intentionally ignored by casting it as (void) in %s on line %d
+
+Warning: The return value of function test() should either be used or intentionally ignored by casting it as (void) in %s on line %d
+
+Warning: The return value of method Clazz::test() should either be used or intentionally ignored by casting it as (void) in %s on line %d
+
+Warning: The return value of method Clazz::test2() should either be used or intentionally ignored by casting it as (void), this is important in %s on line %d
+
+Warning: The return value of method Clazz::test3() should either be used or intentionally ignored by casting it as (void) in %s on line %d
+
+Warning: The return value of method Clazz::test() should either be used or intentionally ignored by casting it as (void) in %s on line %d
+
+Warning: The return value of function {closure:%s:%d}() should either be used or intentionally ignored by casting it as (void) in %s on line %d
+
+Warning: The return value of function {closure:%s:%d}() should either be used or intentionally ignored by casting it as (void) in %s on line %d

Zend/tests/attributes/nodiscard/002.phpt

@@ -0,0 +1,44 @@
+--TEST--
+#[\NoDiscard]: __call(), __callStatic(), and __invoke().
+--FILE--
+<?php
+
+class Clazz {
+	#[\NoDiscard]
+	public function __call(string $name, array $args): int {
+		echo "__call({$name})", PHP_EOL;
+
+		return strlen($name);
+	}
+
+	#[\NoDiscard]
+	public static function __callStatic(string $name, array $args): int {
+		echo "__callStatic({$name})", PHP_EOL;
+
+		return strlen($name);
+	}
+
+	#[\NoDiscard]
+	public function __invoke(string $param): int {
+		echo "__invoke({$param})", PHP_EOL;
+
+		return strlen($param);
+	}
+}
+
+$cls = new Clazz();
+$cls->test();
+Clazz::test();
+$cls('foo');
+
+?>
+--EXPECTF--
+Warning: The return value of method Clazz::test() should either be used or intentionally ignored by casting it as (void) in %s on line %d
+__call(test)
+
+Warning: The return value of method Clazz::test() should either be used or intentionally ignored by casting it as (void) in %s on line %d
+__callStatic(test)
+
+Warning: The return value of method Clazz::__invoke() should either be used or intentionally ignored by casting it as (void) in %s on line %d
+__invoke(foo)
+

Zend/tests/attributes/nodiscard/003.phpt

@@ -0,0 +1,22 @@
+--TEST--
+#[\NoDiscard]: Taken from trait.
+--FILE--
+<?php
+
+trait T {
+	#[\NoDiscard]
+	function test(): int {
+		return 0;
+	}
+}
+
+class Clazz {
+	use T;
+}
+
+$cls = new Clazz();
+$cls->test();
+
+?>
+--EXPECTF--
+Warning: The return value of method Clazz::test() should either be used or intentionally ignored by casting it as (void) in %s on line %d

Zend/tests/attributes/nodiscard/005.phpt

@@ -0,0 +1,17 @@
+--TEST--
+#[\NoDiscard]: Native function and method.
+--FILE--
+<?php
+
+$f = tmpfile();
+flock($f, LOCK_SH | LOCK_NB);
+fclose($f);
+
+$date = new DateTimeImmutable('now');
+$date->setTimestamp(0);
+
+?>
+--EXPECTF--
+Warning: The return value of function flock() should either be used or intentionally ignored by casting it as (void), as locking the stream might have failed in %s on line %d
+
+Warning: The return value of method DateTimeImmutable::setTimestamp() should either be used or intentionally ignored by casting it as (void), as DateTimeImmutable::setTimestamp() does not modify the object itself in %s on line %d

Zend/tests/attributes/nodiscard/006.phpt

@@ -0,0 +1,20 @@
+--TEST--
+#[\NoDiscard]: execute_ex overwritten
+--EXTENSIONS--
+zend_test
+--INI--
+zend_test.replace_zend_execute_ex=1
+opcache.jit=disable
+--FILE--
+<?php
+
+#[\NoDiscard]
+function test(): int {
+	return 0;
+}
+
+test();
+
+?>
+--EXPECTF--
+Warning: The return value of function test() should either be used or intentionally ignored by casting it as (void) in %s on line %d

Zend/tests/attributes/nodiscard/007.phpt

@@ -0,0 +1,21 @@
+--TEST--
+#[\NoDiscard]: execute_internal overwritten
+--EXTENSIONS--
+zend_test
+--INI--
+zend_test.observer.execute_internal=1
+--FILE--
+<?php
+
+$f = tmpfile();
+flock($f, LOCK_SH | LOCK_NB);
+fclose($f);
+
+?>
+--EXPECTF--
+<!-- internal enter tmpfile() -->
+<!-- internal enter NoDiscard::__construct() -->
+
+Warning: The return value of function flock() should either be used or intentionally ignored by casting it as (void), as locking the stream might have failed in %s on line %d
+<!-- internal enter flock() -->
+<!-- internal enter fclose() -->

Zend/tests/attributes/nodiscard/008.phpt

@@ -0,0 +1,18 @@
+--TEST--
+#[\NoDiscard]: Combining with #[\Deprecated].
+--FILE--
+<?php
+
+#[\NoDiscard]
+#[\Deprecated]
+function test(): int {
+	return 0;
+}
+
+test();
+
+?>
+--EXPECTF--
+Deprecated: Function test() is deprecated in %s on line %d
+
+Warning: The return value of function test() should either be used or intentionally ignored by casting it as (void) in %s on line %d

Zend/tests/attributes/nodiscard/009.phpt

@@ -0,0 +1,14 @@
+--TEST--
+#[\NoDiscard]: Combining with #[\Deprecated] (Internal).
+--EXTENSIONS--
+zend_test
+--FILE--
+<?php
+
+zend_test_deprecated_nodiscard();
+
+?>
+--EXPECTF--
+Deprecated: Function zend_test_deprecated_nodiscard() is deprecated, custom message in %s on line %d
+
+Warning: The return value of function zend_test_deprecated_nodiscard() should either be used or intentionally ignored by casting it as (void), custom message 2 in %s on line %d

Zend/tests/attributes/nodiscard/010.phpt

@@ -0,0 +1,76 @@
+--TEST--
+#[\NoDiscard]: Functions with known-used result use DO_[IU]CALL opcodes
+--INI--
+opcache.enable=1
+opcache.enable_cli=1
+opcache.optimization_level=-1
+opcache.opt_debug_level=0x20000
+--EXTENSIONS--
+opcache
+--FILE--
+<?php
+
+$f = tmpfile();
+flock($f, LOCK_SH | LOCK_NB);
+(void)flock($f, LOCK_SH | LOCK_NB);
+$success = flock($f, LOCK_SH | LOCK_NB);
+fclose($f);
+
+#[\NoDiscard]
+function test() {
+	return new stdClass();
+}
+
+test();
+(void)test();
+$obj = test();
+
+?>
+--EXPECTF--
+$_main:
+     ; (lines=29, args=0, vars=3, tmps=1)
+     ; (after optimizer)
+     ; %s
+0000 INIT_FCALL 0 %d string("tmpfile")
+0001 V3 = DO_ICALL
+0002 ASSIGN CV0($f) V3
+0003 INIT_FCALL 2 %d string("flock")
+0004 SEND_VAR CV0($f) 1
+0005 SEND_VAL int(5) 2
+0006 DO_FCALL_BY_NAME
+0007 INIT_FCALL 2 %d string("flock")
+0008 SEND_VAR CV0($f) 1
+0009 SEND_VAL int(5) 2
+0010 V3 = DO_ICALL
+0011 FREE V3
+0012 INIT_FCALL 2 %d string("flock")
+0013 SEND_VAR CV0($f) 1
+0014 SEND_VAL int(5) 2
+0015 V3 = DO_ICALL
+0016 ASSIGN CV1($success) V3
+0017 INIT_FCALL 1 %d string("fclose")
+0018 SEND_VAR CV0($f) 1
+0019 DO_ICALL
+0020 INIT_FCALL 0 %d string("test")
+0021 DO_FCALL_BY_NAME
+0022 INIT_FCALL 0 %d string("test")
+0023 V3 = DO_UCALL
+0024 FREE V3
+0025 INIT_FCALL 0 %d string("test")
+0026 V3 = DO_UCALL
+0027 ASSIGN CV2($obj) V3
+0028 RETURN int(1)
+
+test:
+     ; (lines=3, args=0, vars=0, tmps=1)
+     ; (after optimizer)
+     ; %s
+0000 V0 = NEW 0 string("stdClass")
+0001 DO_FCALL
+0002 RETURN V0
+LIVE RANGES:
+     0: 0001 - 0002 (new)
+
+Warning: The return value of function flock() should either be used or intentionally ignored by casting it as (void), as locking the stream might have failed in %s on line %d
+
+Warning: The return value of function test() should either be used or intentionally ignored by casting it as (void) in %s on line %d

Zend/tests/attributes/nodiscard/error_code_001.phpt

@@ -0,0 +1,21 @@
+--TEST--
+#[\NoDiscard]: Code is E_USER_WARNING.
+--FILE--
+<?php
+
+set_error_handler(function (int $errno, string $errstr, ?string $errfile = null, ?int $errline = null) {
+	var_dump($errno, E_USER_WARNING, $errno === E_USER_WARNING);
+});
+
+#[\NoDiscard]
+function test(): int {
+	return 0;
+}
+
+test();
+
+?>
+--EXPECT--
+int(512)
+int(512)
+bool(true)