Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix logic errors in match-statement Array & Dictionary patterns #57151

Merged
merged 1 commit into from
Jun 15, 2022
Merged

Fix logic errors in match-statement Array & Dictionary patterns #57151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Fix logic errors in match-statement Array & Dictionary Patterns
  • Loading branch information
@cdemirer
cdemirer committed Mar 2, 2022
commit 3afe50c2fa8281d8fb8563cf477841c31cf0f0e2
93 changes: 44 additions & 49 deletions modules/gdscript/gdscript_compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1359,25 +1359,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++) {
Expand All @@ -1387,7 +1371,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);
Expand All @@ -1401,19 +1385,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) {
Expand Down Expand Up @@ -1458,27 +1457,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++) {
Expand All @@ -1489,7 +1470,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);
Expand All @@ -1500,11 +1481,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);
Expand All @@ -1515,16 +1496,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) {
Expand All @@ -1535,9 +1516,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.
Expand Down
43 changes: 43 additions & 0 deletions modules/gdscript/tests/scripts/parser/features/match_dictionary.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"})
15 changes: 15 additions & 0 deletions modules/gdscript/tests/scripts/parser/features/match_dictionary.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
26 changes: 26 additions & 0 deletions modules/gdscript/tests/scripts/parser/features/match_multiple_patterns_with_array.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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])

9 changes: 9 additions & 0 deletions modules/gdscript/tests/scripts/parser/features/match_multiple_patterns_with_array.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
GDTEST_OK
1, [2]
1, [2]
wildcard
[1], [2], [3]
[1], [2], [3]
[1], [2], [3]
[4]
wildcard