Merge pull request #57151 from cdemirer/fix-match-array-dict-pattern-logic-error

Fix logic errors in match-statement Array & Dictionary patterns

5 files changed, 137 insertions, 49 deletions

diff --git a/modules/gdscript/gdscript_compiler.cpp b/modules/gdscript/gdscript_compiler.cpp
index 910f94a936..b2cce9d8ee 100644
--- a/modules/gdscript/gdscript_compiler.cpp
+++ b/modules/gdscript/gdscript_compiler.cpp
@@ -1389,25 +1389,9 @@ GDScriptCodeGenerator::Address GDScriptCompiler::_parse_match_pattern(CodeGen &c
 			codegen.generator->pop_temporary();
 			codegen.generator->pop_temporary();
 
-			// If this isn't the first, we need to OR with the previous pattern. If it's nested, we use AND instead.
-			if (p_is_nested) {
-				// Use the previous value as target, since we only need one temporary variable.
-				codegen.generator->write_and_right_operand(result_addr);
-				codegen.generator->write_end_and(p_previous_test);
-			} else if (!p_is_first) {
-				// Use the previous value as target, since we only need one temporary variable.
-				codegen.generator->write_or_right_operand(result_addr);
-				codegen.generator->write_end_or(p_previous_test);
-			} else {
-				// Just assign this value to the accumulator temporary.
-				codegen.generator->write_assign(p_previous_test, result_addr);
-			}
-			codegen.generator->pop_temporary(); // Remove temp result addr.
-
 			// Create temporaries outside the loop so they can be reused.
 			GDScriptCodeGenerator::Address element_addr = codegen.add_temporary();
 			GDScriptCodeGenerator::Address element_type_addr = codegen.add_temporary();
-			GDScriptCodeGenerator::Address test_addr = p_previous_test;
 
 			// Evaluate element by element.
 			for (int i = 0; i < p_pattern->array.size(); i++) {
@@ -1417,7 +1401,7 @@ GDScriptCodeGenerator::Address GDScriptCompiler::_parse_match_pattern(CodeGen &c
 				}
 
 				// Use AND here too, as we don't want to be checking elements if previous test failed (which means this might be an invalid get).
-				codegen.generator->write_and_left_operand(test_addr);
+				codegen.generator->write_and_left_operand(result_addr);
 
 				// Add index to constant map.
 				GDScriptCodeGenerator::Address index_addr = codegen.add_constant(i);
@@ -1431,19 +1415,34 @@ GDScriptCodeGenerator::Address GDScriptCompiler::_parse_match_pattern(CodeGen &c
 				codegen.generator->write_call_utility(element_type_addr, "typeof", typeof_args);
 
 				// Try the pattern inside the element.
-				test_addr = _parse_match_pattern(codegen, r_error, p_pattern->array[i], element_addr, element_type_addr, p_previous_test, false, true);
+				result_addr = _parse_match_pattern(codegen, r_error, p_pattern->array[i], element_addr, element_type_addr, result_addr, false, true);
 				if (r_error != OK) {
 					return GDScriptCodeGenerator::Address();
 				}
 
-				codegen.generator->write_and_right_operand(test_addr);
-				codegen.generator->write_end_and(test_addr);
+				codegen.generator->write_and_right_operand(result_addr);
+				codegen.generator->write_end_and(result_addr);
 			}
 			// Remove element temporaries.
 			codegen.generator->pop_temporary();
 			codegen.generator->pop_temporary();
 
-			return test_addr;
+			// If this isn't the first, we need to OR with the previous pattern. If it's nested, we use AND instead.
+			if (p_is_nested) {
+				// Use the previous value as target, since we only need one temporary variable.
+				codegen.generator->write_and_right_operand(result_addr);
+				codegen.generator->write_end_and(p_previous_test);
+			} else if (!p_is_first) {
+				// Use the previous value as target, since we only need one temporary variable.
+				codegen.generator->write_or_right_operand(result_addr);
+				codegen.generator->write_end_or(p_previous_test);
+			} else {
+				// Just assign this value to the accumulator temporary.
+				codegen.generator->write_assign(p_previous_test, result_addr);
+			}
+			codegen.generator->pop_temporary(); // Remove temp result addr.
+
+			return p_previous_test;
 		} break;
 		case GDScriptParser::PatternNode::PT_DICTIONARY: {
 			if (p_is_nested) {
@@ -1488,27 +1487,9 @@ GDScriptCodeGenerator::Address GDScriptCompiler::_parse_match_pattern(CodeGen &c
 			codegen.generator->pop_temporary();
 			codegen.generator->pop_temporary();
 
-			// If this isn't the first, we need to OR with the previous pattern. If it's nested, we use AND instead.
-			if (p_is_nested) {
-				// Use the previous value as target, since we only need one temporary variable.
-				codegen.generator->write_and_right_operand(result_addr);
-				codegen.generator->write_end_and(p_previous_test);
-			} else if (!p_is_first) {
-				// Use the previous value as target, since we only need one temporary variable.
-				codegen.generator->write_or_right_operand(result_addr);
-				codegen.generator->write_end_or(p_previous_test);
-			} else {
-				// Just assign this value to the accumulator temporary.
-				codegen.generator->write_assign(p_previous_test, result_addr);
-			}
-			codegen.generator->pop_temporary(); // Remove temp result addr.
-
 			// Create temporaries outside the loop so they can be reused.
-			temp_type.builtin_type = Variant::BOOL;
-			GDScriptCodeGenerator::Address test_result = codegen.add_temporary(temp_type);
 			GDScriptCodeGenerator::Address element_addr = codegen.add_temporary();
 			GDScriptCodeGenerator::Address element_type_addr = codegen.add_temporary();
-			GDScriptCodeGenerator::Address test_addr = p_previous_test;
 
 			// Evaluate element by element.
 			for (int i = 0; i < p_pattern->dictionary.size(); i++) {
@@ -1519,7 +1500,7 @@ GDScriptCodeGenerator::Address GDScriptCompiler::_parse_match_pattern(CodeGen &c
 				}
 
 				// Use AND here too, as we don't want to be checking elements if previous test failed (which means this might be an invalid get).
-				codegen.generator->write_and_left_operand(test_addr);
+				codegen.generator->write_and_left_operand(result_addr);
 
 				// Get the pattern key.
 				GDScriptCodeGenerator::Address pattern_key_addr = _parse_expression(codegen, r_error, element.key);
@@ -1530,11 +1511,11 @@ GDScriptCodeGenerator::Address GDScriptCompiler::_parse_match_pattern(CodeGen &c
 				// Check if pattern key exists in user's dictionary. This will be AND-ed with next result.
 				func_args.clear();
 				func_args.push_back(pattern_key_addr);
-				codegen.generator->write_call(test_result, p_value_addr, "has", func_args);
+				codegen.generator->write_call(result_addr, p_value_addr, "has", func_args);
 
 				if (element.value_pattern != nullptr) {
 					// Use AND here too, as we don't want to be checking elements if previous test failed (which means this might be an invalid get).
-					codegen.generator->write_and_left_operand(test_result);
+					codegen.generator->write_and_left_operand(result_addr);
 
 					// Get actual value from user dictionary.
 					codegen.generator->write_get(element_addr, pattern_key_addr, p_value_addr);
@@ -1545,16 +1526,16 @@ GDScriptCodeGenerator::Address GDScriptCompiler::_parse_match_pattern(CodeGen &c
 					codegen.generator->write_call_utility(element_type_addr, "typeof", func_args);
 
 					// Try the pattern inside the value.
-					test_addr = _parse_match_pattern(codegen, r_error, element.value_pattern, element_addr, element_type_addr, test_addr, false, true);
+					result_addr = _parse_match_pattern(codegen, r_error, element.value_pattern, element_addr, element_type_addr, result_addr, false, true);
 					if (r_error != OK) {
 						return GDScriptCodeGenerator::Address();
 					}
-					codegen.generator->write_and_right_operand(test_addr);
-					codegen.generator->write_end_and(test_addr);
+					codegen.generator->write_and_right_operand(result_addr);
+					codegen.generator->write_end_and(result_addr);
 				}
 
-				codegen.generator->write_and_right_operand(test_addr);
-				codegen.generator->write_end_and(test_addr);
+				codegen.generator->write_and_right_operand(result_addr);
+				codegen.generator->write_end_and(result_addr);
 
 				// Remove pattern key temporary.
 				if (pattern_key_addr.mode == GDScriptCodeGenerator::Address::TEMPORARY) {
@@ -1565,9 +1546,23 @@ GDScriptCodeGenerator::Address GDScriptCompiler::_parse_match_pattern(CodeGen &c
 			// Remove element temporaries.
 			codegen.generator->pop_temporary();
 			codegen.generator->pop_temporary();
-			codegen.generator->pop_temporary();
 
-			return test_addr;
+			// If this isn't the first, we need to OR with the previous pattern. If it's nested, we use AND instead.
+			if (p_is_nested) {
+				// Use the previous value as target, since we only need one temporary variable.
+				codegen.generator->write_and_right_operand(result_addr);
+				codegen.generator->write_end_and(p_previous_test);
+			} else if (!p_is_first) {
+				// Use the previous value as target, since we only need one temporary variable.
+				codegen.generator->write_or_right_operand(result_addr);
+				codegen.generator->write_end_or(p_previous_test);
+			} else {
+				// Just assign this value to the accumulator temporary.
+				codegen.generator->write_assign(p_previous_test, result_addr);
+			}
+			codegen.generator->pop_temporary(); // Remove temp result addr.
+
+			return p_previous_test;
 		} break;
 		case GDScriptParser::PatternNode::PT_REST:
 			// Do nothing.
diff --git a/modules/gdscript/tests/scripts/parser/features/match_dictionary.gd b/modules/gdscript/tests/scripts/parser/features/match_dictionary.gd
new file mode 100644
index 0000000000..377dd25e9e
--- /dev/null
+++ b/modules/gdscript/tests/scripts/parser/features/match_dictionary.gd
@@ -0,0 +1,43 @@
+func foo(x):
+    match x:
+        {"key1": "value1", "key2": "value2"}:
+            print('{"key1": "value1", "key2": "value2"}')
+        {"key1": "value1", "key2"}:
+            print('{"key1": "value1", "key2"}')
+        {"key1", "key2": "value2"}:
+            print('{"key1", "key2": "value2"}')
+        {"key1", "key2"}:
+            print('{"key1", "key2"}')
+        {"key1": "value1"}:
+            print('{"key1": "value1"}')
+        {"key1"}:
+            print('{"key1"}')
+        _:
+            print("wildcard")
+
+func bar(x):
+    match x:
+        {0}:
+            print("0")
+        {1}:
+            print("1")
+        {2}:
+            print("2")
+        _:
+            print("wildcard")
+
+func test():
+    foo({"key1": "value1", "key2": "value2"})
+    foo({"key1": "value1", "key2": ""})
+    foo({"key1": "", "key2": "value2"})
+    foo({"key1": "", "key2": ""})
+    foo({"key1": "value1"})
+    foo({"key1": ""})
+    foo({"key1": "value1", "key2": "value2", "key3": "value3"})
+    foo({"key1": "value1", "key3": ""})
+    foo({"key2": "value2"})
+    foo({"key3": ""})
+    bar({0: "0"})
+    bar({1: "1"})
+    bar({2: "2"})
+    bar({3: "3"})
diff --git a/modules/gdscript/tests/scripts/parser/features/match_dictionary.out b/modules/gdscript/tests/scripts/parser/features/match_dictionary.out
new file mode 100644
index 0000000000..4dee886927
--- /dev/null
+++ b/modules/gdscript/tests/scripts/parser/features/match_dictionary.out
@@ -0,0 +1,15 @@
+GDTEST_OK
+{"key1": "value1", "key2": "value2"}
+{"key1": "value1", "key2"}
+{"key1", "key2": "value2"}
+{"key1", "key2"}
+{"key1": "value1"}
+{"key1"}
+wildcard
+wildcard
+wildcard
+wildcard
+0
+1
+2
+wildcard
diff --git a/modules/gdscript/tests/scripts/parser/features/match_multiple_patterns_with_array.gd b/modules/gdscript/tests/scripts/parser/features/match_multiple_patterns_with_array.gd
new file mode 100644
index 0000000000..dbe223f5f5
--- /dev/null
+++ b/modules/gdscript/tests/scripts/parser/features/match_multiple_patterns_with_array.gd
@@ -0,0 +1,26 @@
+func foo(x):
+    match x:
+        1, [2]:
+            print('1, [2]')
+        _:
+            print('wildcard')
+
+func bar(x):
+    match x:
+        [1], [2], [3]:
+            print('[1], [2], [3]')
+        [4]:
+            print('[4]')
+        _:
+            print('wildcard')
+
+func test():
+    foo(1)
+    foo([2])
+    foo(2)
+    bar([1])
+    bar([2])
+    bar([3])
+    bar([4])
+    bar([5])
+
diff --git a/modules/gdscript/tests/scripts/parser/features/match_multiple_patterns_with_array.out b/modules/gdscript/tests/scripts/parser/features/match_multiple_patterns_with_array.out
new file mode 100644
index 0000000000..a12b934d67
--- /dev/null
+++ b/modules/gdscript/tests/scripts/parser/features/match_multiple_patterns_with_array.out
@@ -0,0 +1,9 @@
+GDTEST_OK
+1, [2]
+1, [2]
+wildcard
+[1], [2], [3]
+[1], [2], [3]
+[1], [2], [3]
+[4]
+wildcard