Fix missing "Optional parameter before required" deprecation on union null type

iluuu1994 · iluuu1994 · commit 68ef3938f42a · 2023-06-28T20:45:43.000+02:00
The check would only work for the ?type syntax, but not type|null. Switch to a check during type compilation instead. Fixes GH-11488 Closes GH-11497
diff --git a/NEWS b/NEWS
@@ -7,6 +7,8 @@ PHP                                                                        NEWS
     (nielsdos)
   . Fixed oss-fuzz #60011 (Mis-compilation of by-reference nullsafe operator).
     (ilutov)
+  . Fixed GH-11488 (Missing "Optional parameter before required" deprecation on
+    union null type). (ilutov)
 
 - DOM:
   . Fixed bug GH-11500 (Namespace reuse in createElementNS() generates wrong
diff --git a/Zend/tests/gh11488.phpt b/Zend/tests/gh11488.phpt
@@ -0,0 +1,21 @@
+--TEST--
+GH-11488: "Optional parameter before required" warning for union nullable type
+--FILE--
+<?php
+function a(
+    string|null $a = null,
+    $b,
+) {}
+function b(
+    Foo&Bar $c = null,
+    $d,
+) {}
+function c(
+    (Foo&Bar)|null $e = null,
+    $f,
+) {}
+?>
+--EXPECTF--
+Deprecated: Optional parameter $a declared before required parameter $b is implicitly treated as a required parameter in %s on line %d
+
+Deprecated: Optional parameter $e declared before required parameter $f is implicitly treated as a required parameter in %s on line %d
diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c
@@ -6503,8 +6503,10 @@ static void zend_is_type_list_redundant_by_single_type(zend_type_list *type_list
 	}
 }
 
-static zend_type zend_compile_typename(
-		zend_ast *ast, bool force_allow_null) /* {{{ */
+static zend_type zend_compile_typename(zend_ast *ast, bool force_allow_null);
+
+static zend_type zend_compile_typename_ex(
+		zend_ast *ast, bool force_allow_null, bool *forced_allow_null) /* {{{ */
 {
 	bool is_marked_nullable = ast->attr & ZEND_TYPE_NULLABLE;
 	zend_ast_attr orig_ast_attr = ast->attr;
@@ -6707,6 +6709,10 @@ static zend_type zend_compile_typename(
 		zend_error_noreturn(E_COMPILE_ERROR, "null cannot be marked as nullable");
 	}
 
+	if (force_allow_null && !is_marked_nullable && !(type_mask & MAY_BE_NULL)) {
+		*forced_allow_null = true;
+	}
+
 	if (is_marked_nullable || force_allow_null) {
 		ZEND_TYPE_FULL_MASK(type) |= MAY_BE_NULL;
 		type_mask = ZEND_TYPE_PURE_MASK(type);
@@ -6725,6 +6731,12 @@ static zend_type zend_compile_typename(
 }
 /* }}} */
 
+static zend_type zend_compile_typename(zend_ast *ast, bool force_allow_null)
+{
+	bool forced_allow_null;
+	return zend_compile_typename_ex(ast, force_allow_null, &forced_allow_null);
+}
+
 /* May convert value from int to float. */
 static bool zend_is_valid_default_value(zend_type type, zval *value)
 {
@@ -6954,28 +6966,6 @@ static void zend_compile_params(zend_ast *ast, zend_ast *return_type_ast, uint32
 			zend_const_expr_to_zval(
 				&default_node.u.constant, default_ast_ptr, /* allow_dynamic */ true);
 			CG(compiler_options) = cops;
-
-			if (last_required_param != (uint32_t) -1 && i < last_required_param) {
-				/* Ignore parameters of the form "Type $param = null".
-				 * This is the PHP 5 style way of writing "?Type $param", so allow it for now. */
-				bool is_implicit_nullable =
-					type_ast && !(type_ast->attr & ZEND_TYPE_NULLABLE)
-					&& Z_TYPE(default_node.u.constant) == IS_NULL;
-				if (!is_implicit_nullable) {
-					zend_ast *required_param_ast = list->child[last_required_param];
-					zend_error(E_DEPRECATED,
-						"Optional parameter $%s declared before required parameter $%s "
-						"is implicitly treated as a required parameter",
-						ZSTR_VAL(name), ZSTR_VAL(zend_ast_get_str(required_param_ast->child[1])));
-				}
-
-				/* Regardless of whether we issue a deprecation, convert this parameter into
-				 * a required parameter without a default value. This ensures that it cannot be
-				 * used as an optional parameter even with named parameters. */
-				opcode = ZEND_RECV;
-				default_node.op_type = IS_UNUSED;
-				zval_ptr_dtor(&default_node.u.constant);
-			}
 		} else {
 			opcode = ZEND_RECV;
 			default_node.op_type = IS_UNUSED;
@@ -6993,12 +6983,13 @@ static void zend_compile_params(zend_ast *ast, zend_ast *return_type_ast, uint32
 			);
 		}
 
+		bool forced_allow_nullable = false;
 		if (type_ast) {
 			uint32_t default_type = *default_ast_ptr ? Z_TYPE(default_node.u.constant) : IS_UNDEF;
 			bool force_nullable = default_type == IS_NULL && !property_flags;
 
 			op_array->fn_flags |= ZEND_ACC_HAS_TYPE_HINTS;
-			arg_info->type = zend_compile_typename(type_ast, force_nullable);
+			arg_info->type = zend_compile_typename_ex(type_ast, force_nullable, &forced_allow_nullable);
 
 			if (ZEND_TYPE_FULL_MASK(arg_info->type) & MAY_BE_VOID) {
 				zend_error_noreturn(E_COMPILE_ERROR, "void cannot be used as a parameter type");
@@ -7017,6 +7008,26 @@ static void zend_compile_params(zend_ast *ast, zend_ast *return_type_ast, uint32
 					ZSTR_VAL(name), ZSTR_VAL(type_str));
 			}
 		}
+		if (last_required_param != (uint32_t) -1
+		 && i < last_required_param
+		 && default_node.op_type == IS_CONST) {
+			/* Ignore parameters of the form "Type $param = null".
+			 * This is the PHP 5 style way of writing "?Type $param", so allow it for now. */
+			if (!forced_allow_nullable) {
+				zend_ast *required_param_ast = list->child[last_required_param];
+				zend_error(E_DEPRECATED,
+					"Optional parameter $%s declared before required parameter $%s "
+					"is implicitly treated as a required parameter",
+					ZSTR_VAL(name), ZSTR_VAL(zend_ast_get_str(required_param_ast->child[1])));
+			}
+
+			/* Regardless of whether we issue a deprecation, convert this parameter into
+			 * a required parameter without a default value. This ensures that it cannot be
+			 * used as an optional parameter even with named parameters. */
+			opcode = ZEND_RECV;
+			default_node.op_type = IS_UNUSED;
+			zval_ptr_dtor(&default_node.u.constant);
+		}
 
 		opline = zend_emit_op(NULL, opcode, NULL, &default_node);
 		SET_NODE(opline->result, &var_node);
