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

Zend/Optimizer/block_pass.c

@@ -274,7 +274,9 @@ static void zend_optimize_block(zend_basic_block *block, zend_op_array *op_array
 				 * If it's not local, then the other blocks successors must also eventually either FREE or consume the temporary,
 				 * hence removing the temporary is not safe in the general case, especially when other consumers are not FREE.
 				 * A FREE may not be removed without also removing the source's result, because otherwise that would cause a memory leak. */
-				if (opline->op1_type == IS_TMP_VAR) {
+				if (opline->extended_value == ZEND_FREE_VOID_CAST) {
+					/* Keep the ZEND_FREE opcode alive. */
+				} else if (opline->op1_type == IS_TMP_VAR) {
 					src = VAR_SOURCE(opline->op1);
 					if (src) {
 						switch (src->opcode) {

Zend/Optimizer/dce.c

@@ -80,7 +80,6 @@ static inline bool may_have_side_effects(
 		case ZEND_IS_IDENTICAL:
 		case ZEND_IS_NOT_IDENTICAL:
 		case ZEND_QM_ASSIGN:
-		case ZEND_FREE:
 		case ZEND_FE_FREE:
 		case ZEND_TYPE_CHECK:
 		case ZEND_DEFINED:
@@ -127,6 +126,8 @@ static inline bool may_have_side_effects(
 		case ZEND_ARRAY_KEY_EXISTS:
 			/* No side effects */
 			return 0;
+		case ZEND_FREE:
+			return opline->extended_value == ZEND_FREE_VOID_CAST;
 		case ZEND_ADD_ARRAY_ELEMENT:
 			/* TODO: We can't free two vars. Keep instruction alive. <?php [0, "$a" => "$b"]; */
 			if ((opline->op1_type & (IS_VAR|IS_TMP_VAR)) && (opline->op2_type & (IS_VAR|IS_TMP_VAR))) {

Zend/Optimizer/optimize_func_calls.c

@@ -79,7 +79,7 @@ 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)
 {
 	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|ZEND_ACC_NODISCARD))
 		/* 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

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() is expected to be consumed in %s on line %d
+
+Warning: The return value of function test2() is expected to be consumed, this is important in %s on line %d
+
+Warning: The return value of function test3() is expected to be consumed in %s on line %d
+
+Warning: The return value of function test() is expected to be consumed in %s on line %d
+
+Warning: The return value of function test() is expected to be consumed in %s on line %d
+
+Warning: The return value of method Clazz::test() is expected to be consumed in %s on line %d
+
+Warning: The return value of method Clazz::test2() is expected to be consumed, this is important in %s on line %d
+
+Warning: The return value of method Clazz::test3() is expected to be consumed in %s on line %d
+
+Warning: The return value of method Clazz::test() is expected to be consumed in %s on line %d
+
+Warning: The return value of function {closure:%s:%d}() is expected to be consumed in %s on line %d
+
+Warning: The return value of function {closure:%s:%d}() is expected to be consumed 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() is expected to be consumed in %s on line %d
+__call(test)
+
+Warning: The return value of method Clazz::test() is expected to be consumed in %s on line %d
+__callStatic(test)
+
+Warning: The return value of method Clazz::__invoke() is expected to be consumed 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() is expected to be consumed 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() is expected to be consumed, as locking the stream might have failed in %s on line %d
+
+Warning: The return value of method DateTimeImmutable::setTimestamp() is expected to be consumed, 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() is expected to be consumed 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() is expected to be consumed, 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() is expected to be consumed 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() is expected to be consumed, custom message 2 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)

Zend/tests/attributes/nodiscard/property_readonly_001.phpt

@@ -0,0 +1,14 @@
+--TEST--
+#[\NoDiscard]: NoDiscard::$message is readonly.
+--FILE--
+<?php
+
+$d = new \NoDiscard("foo");
+$d->message = 'bar';
+
+?>
+--EXPECTF--
+Fatal error: Uncaught Error: Cannot modify readonly property NoDiscard::$message in %s:%d
+Stack trace:
+#0 {main}
+  thrown in %s on line %d

Zend/tests/attributes/nodiscard/property_readonly_002.phpt

@@ -0,0 +1,15 @@
+--TEST--
+#[\NoDiscard]: __construct() respects that properties are readonly.
+--FILE--
+<?php
+
+$d = new \NoDiscard("foo");
+$d->__construct("bar");
+
+?>
+--EXPECTF--
+Fatal error: Uncaught Error: Cannot modify readonly property NoDiscard::$message in %s:%d
+Stack trace:
+#0 %s(%d): NoDiscard->__construct('bar')
+#1 {main}
+  thrown in %s on line %d