Skip to content

Commit

Permalink
GDScript: Add static typing for for loop variable
Browse files Browse the repository at this point in the history
  • Loading branch information
dalexeev committed Aug 17, 2023
1 parent 0511f9d commit 6c59ed9
Show file tree
Hide file tree
Showing 20 changed files with 177 additions and 17 deletions.
3 changes: 3 additions & 0 deletions doc/classes/ProjectSettings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,9 @@
<member name="debug/gdscript/warnings/redundant_await" type="int" setter="" getter="" default="1">
When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when a function that is not a coroutine is called with await.
</member>
<member name="debug/gdscript/warnings/redundant_for_variable_type" type="int" setter="" getter="" default="1">
When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when a [code]for[/code] variable type specifier is a supertype of the inferred type.
</member>
<member name="debug/gdscript/warnings/redundant_static_unload" type="int" setter="" getter="" default="1">
When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when the [code]@static_unload[/code] annotation is used in a script without any static variables.
</member>
Expand Down
34 changes: 33 additions & 1 deletion modules/gdscript/gdscript_analyzer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2001,13 +2001,16 @@ void GDScriptAnalyzer::resolve_for(GDScriptParser::ForNode *p_for) {
}

GDScriptParser::DataType variable_type;
String list_visible_type = "<unresolved type>";
if (list_resolved) {
variable_type.type_source = GDScriptParser::DataType::ANNOTATED_INFERRED;
variable_type.kind = GDScriptParser::DataType::BUILTIN;
variable_type.builtin_type = Variant::INT;
list_visible_type = "Array[int]"; // NOTE: `range()` has `Array` return type.
} else if (p_for->list) {
resolve_node(p_for->list, false);
GDScriptParser::DataType list_type = p_for->list->get_datatype();
list_visible_type = list_type.to_string();
if (!list_type.is_hard_type()) {
mark_node_unsafe(p_for->list);
}
Expand Down Expand Up @@ -2051,8 +2054,37 @@ void GDScriptAnalyzer::resolve_for(GDScriptParser::ForNode *p_for) {
push_error(vformat(R"(Unable to iterate on value of type "%s".)", list_type.to_string()), p_for->list);
}
}

if (p_for->variable) {
p_for->variable->set_datatype(variable_type);
if (p_for->datatype_specifier) {
GDScriptParser::DataType specified_type = type_from_metatype(resolve_datatype(p_for->datatype_specifier));
if (!specified_type.is_variant()) {
if (variable_type.is_variant() || !variable_type.is_hard_type()) {
mark_node_unsafe(p_for->variable);
p_for->use_conversion_assign = true;
} else if (!is_type_compatible(specified_type, variable_type, true, p_for->variable)) {
if (is_type_compatible(variable_type, specified_type)) {
mark_node_unsafe(p_for->variable);
p_for->use_conversion_assign = true;
} else {
push_error(vformat(R"(Unable to iterate on value of type "%s" with variable of type "%s".)", list_visible_type, specified_type.to_string()), p_for->datatype_specifier);
}
} else if (!is_type_compatible(specified_type, variable_type)) {
p_for->use_conversion_assign = true;
#ifdef DEBUG_ENABLED
} else {
parser->push_warning(p_for->datatype_specifier, GDScriptWarning::REDUNDANT_FOR_VARIABLE_TYPE, p_for->variable->name, variable_type.to_string(), specified_type.to_string());
#endif
}
#ifdef DEBUG_ENABLED
} else {
parser->push_warning(p_for->datatype_specifier, GDScriptWarning::REDUNDANT_FOR_VARIABLE_TYPE, p_for->variable->name, variable_type.to_string(), specified_type.to_string());
#endif
}
p_for->variable->set_datatype(specified_type);
} else {
p_for->variable->set_datatype(variable_type);
}
}

resolve_suite(p_for->loop);
Expand Down
25 changes: 17 additions & 8 deletions modules/gdscript/gdscript_byte_codegen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1494,19 +1494,16 @@ void GDScriptByteCodeGenerator::start_for(const GDScriptDataType &p_iterator_typ
for_container_variables.push_back(container);
}

void GDScriptByteCodeGenerator::write_for_assignment(const Address &p_variable, const Address &p_list) {
void GDScriptByteCodeGenerator::write_for_assignment(const Address &p_list) {
const Address &container = for_container_variables.back()->get();

// Assign container.
append_opcode(GDScriptFunction::OPCODE_ASSIGN);
append(container);
append(p_list);

for_iterator_variables.push_back(p_variable);
}

void GDScriptByteCodeGenerator::write_for() {
const Address &iterator = for_iterator_variables.back()->get();
void GDScriptByteCodeGenerator::write_for(const Address &p_variable, bool p_use_conversion) {
const Address &counter = for_counter_variables.back()->get();
const Address &container = for_container_variables.back()->get();

Expand Down Expand Up @@ -1599,11 +1596,16 @@ void GDScriptByteCodeGenerator::write_for() {
}
}

Address temp;
if (p_use_conversion) {
temp = Address(Address::LOCAL_VARIABLE, add_local("@iterator_temp", GDScriptDataType()));
}

// Begin loop.
append_opcode(begin_opcode);
append(counter);
append(container);
append(iterator);
append(p_use_conversion ? temp : p_variable);
for_jmp_addrs.push_back(opcodes.size());
append(0); // End of loop address, will be patched.
append_opcode(GDScriptFunction::OPCODE_JUMP);
Expand All @@ -1615,9 +1617,17 @@ void GDScriptByteCodeGenerator::write_for() {
append_opcode(iterate_opcode);
append(counter);
append(container);
append(iterator);
append(p_use_conversion ? temp : p_variable);
for_jmp_addrs.push_back(opcodes.size());
append(0); // Jump destination, will be patched.

if (p_use_conversion) {
write_assign_with_conversion(p_variable, temp);
const GDScriptDataType &type = p_variable.type;
if (type.kind != GDScriptDataType::BUILTIN || type.builtin_type == Variant::ARRAY || type.builtin_type == Variant::DICTIONARY) {
write_assign_false(temp); // Can contain RefCounted, so clear it.
}
}
}

void GDScriptByteCodeGenerator::write_endfor() {
Expand All @@ -1639,7 +1649,6 @@ void GDScriptByteCodeGenerator::write_endfor() {
current_breaks_to_patch.pop_back();

// Pop state.
for_iterator_variables.pop_back();
for_counter_variables.pop_back();
for_container_variables.pop_back();
}
Expand Down
5 changes: 2 additions & 3 deletions modules/gdscript/gdscript_byte_codegen.h
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,6 @@ class GDScriptByteCodeGenerator : public GDScriptCodeGenerator {
// Lists since these can be nested.
List<int> if_jmp_addrs;
List<int> for_jmp_addrs;
List<Address> for_iterator_variables;
List<Address> for_counter_variables;
List<Address> for_container_variables;
List<int> while_jmp_addrs;
Expand Down Expand Up @@ -536,8 +535,8 @@ class GDScriptByteCodeGenerator : public GDScriptCodeGenerator {
virtual void write_jump_if_shared(const Address &p_value) override;
virtual void write_end_jump_if_shared() override;
virtual void start_for(const GDScriptDataType &p_iterator_type, const GDScriptDataType &p_list_type) override;
virtual void write_for_assignment(const Address &p_variable, const Address &p_list) override;
virtual void write_for() override;
virtual void write_for_assignment(const Address &p_list) override;
virtual void write_for(const Address &p_variable, bool p_use_conversion) override;
virtual void write_endfor() override;
virtual void start_while_condition() override;
virtual void write_while(const Address &p_condition) override;
Expand Down
4 changes: 2 additions & 2 deletions modules/gdscript/gdscript_codegen.h
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,8 @@ class GDScriptCodeGenerator {
virtual void write_jump_if_shared(const Address &p_value) = 0;
virtual void write_end_jump_if_shared() = 0;
virtual void start_for(const GDScriptDataType &p_iterator_type, const GDScriptDataType &p_list_type) = 0;
virtual void write_for_assignment(const Address &p_variable, const Address &p_list) = 0;
virtual void write_for() = 0;
virtual void write_for_assignment(const Address &p_list) = 0;
virtual void write_for(const Address &p_variable, bool p_use_conversion) = 0;
virtual void write_endfor() = 0;
virtual void start_while_condition() = 0; // Used to allow a jump to the expression evaluation.
virtual void write_while(const Address &p_condition) = 0;
Expand Down
4 changes: 2 additions & 2 deletions modules/gdscript/gdscript_compiler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1953,13 +1953,13 @@ Error GDScriptCompiler::_parse_block(CodeGen &codegen, const GDScriptParser::Sui
return err;
}

gen->write_for_assignment(iterator, list);
gen->write_for_assignment(list);

if (list.mode == GDScriptCodeGenerator::Address::TEMPORARY) {
codegen.generator->pop_temporary();
}

gen->write_for();
gen->write_for(iterator, for_n->use_conversion_assign);

err = _parse_block(codegen, for_n->loop);
if (err) {
Expand Down
13 changes: 12 additions & 1 deletion modules/gdscript/gdscript_parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1850,7 +1850,18 @@ GDScriptParser::ForNode *GDScriptParser::parse_for() {
n_for->variable = parse_identifier();
}

consume(GDScriptTokenizer::Token::IN, R"(Expected "in" after "for" variable name.)");
if (match(GDScriptTokenizer::Token::COLON)) {
n_for->datatype_specifier = parse_type();
if (n_for->datatype_specifier == nullptr) {
push_error(R"(Expected type specifier after ":".)");
}
}

if (n_for->datatype_specifier == nullptr) {
consume(GDScriptTokenizer::Token::IN, R"(Expected "in" or ":" after "for" variable name.)");
} else {
consume(GDScriptTokenizer::Token::IN, R"(Expected "in" after "for" variable type specifier.)");
}

n_for->list = parse_expression(false);

Expand Down
2 changes: 2 additions & 0 deletions modules/gdscript/gdscript_parser.h
Original file line number Diff line number Diff line change
Expand Up @@ -814,6 +814,8 @@ class GDScriptParser {

struct ForNode : public Node {
IdentifierNode *variable = nullptr;
TypeNode *datatype_specifier = nullptr;
bool use_conversion_assign = false;
ExpressionNode *list = nullptr;
SuiteNode *loop = nullptr;

Expand Down
9 changes: 9 additions & 0 deletions modules/gdscript/gdscript_warning.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,14 @@ String GDScriptWarning::get_message() const {
return R"(The "@static_unload" annotation is redundant because the file does not have a class with static variables.)";
case REDUNDANT_AWAIT:
return R"("await" keyword not needed in this case, because the expression isn't a coroutine nor a signal.)";
case REDUNDANT_FOR_VARIABLE_TYPE:
CHECK_SYMBOLS(3);
if (symbols[1] == symbols[2]) {
return vformat(R"(The for loop iterator "%s" already has inferred type "%s", the specified type is redundant.)", symbols[0], symbols[1]);
} else {
return vformat(R"(The for loop iterator "%s" has inferred type "%s" but its supertype "%s" is specified.)", symbols[0], symbols[1], symbols[2]);
}
break;
case ASSERT_ALWAYS_TRUE:
return "Assert statement is redundant because the expression is always true.";
case ASSERT_ALWAYS_FALSE:
Expand Down Expand Up @@ -209,6 +217,7 @@ String GDScriptWarning::get_name_from_code(Code p_code) {
"STATIC_CALLED_ON_INSTANCE",
"REDUNDANT_STATIC_UNLOAD",
"REDUNDANT_AWAIT",
"REDUNDANT_FOR_VARIABLE_TYPE",
"ASSERT_ALWAYS_TRUE",
"ASSERT_ALWAYS_FALSE",
"INTEGER_DIVISION",
Expand Down
2 changes: 2 additions & 0 deletions modules/gdscript/gdscript_warning.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ class GDScriptWarning {
STATIC_CALLED_ON_INSTANCE, // A static method was called on an instance of a class instead of on the class itself.
REDUNDANT_STATIC_UNLOAD, // The `@static_unload` annotation is used but the class does not have static data.
REDUNDANT_AWAIT, // await is used but expression is synchronous (not a signal nor a coroutine).
REDUNDANT_FOR_VARIABLE_TYPE, // The for variable type specifier is a supertype of the inferred type.
ASSERT_ALWAYS_TRUE, // Expression for assert argument is always true.
ASSERT_ALWAYS_FALSE, // Expression for assert argument is always false.
INTEGER_DIVISION, // Integer divide by integer, decimal part is discarded.
Expand Down Expand Up @@ -120,6 +121,7 @@ class GDScriptWarning {
WARN, // STATIC_CALLED_ON_INSTANCE
WARN, // REDUNDANT_STATIC_UNLOAD
WARN, // REDUNDANT_AWAIT
WARN, // REDUNDANT_FOR_VARIABLE_TYPE
WARN, // ASSERT_ALWAYS_TRUE
WARN, // ASSERT_ALWAYS_FALSE
WARN, // INTEGER_DIVISION
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
func test():
var a: Array[Resource] = []
for node: Node in a:
print(node)
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
GDTEST_ANALYZER_ERROR
Unable to iterate on value of type "Array[Resource]" with variable of type "Node".
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
func test():
var a: Array[Node] = []
for node: Node in a:
print(node)
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
GDTEST_OK
>> WARNING
>> Line: 3
>> REDUNDANT_FOR_VARIABLE_TYPE
>> The for loop iterator "node" already has inferred type "Node", the specified type is redundant.
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
func test():
var a: Array[Node2D] = []
for node: Node in a:
print(node)
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
GDTEST_OK
>> WARNING
>> Line: 3
>> REDUNDANT_FOR_VARIABLE_TYPE
>> The for loop iterator "node" has inferred type "Node2D" but its supertype "Node" is specified.
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
func test():
var a: Array = [Resource.new()]
for node: Node in a:
print(node)
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
GDTEST_RUNTIME_ERROR
>> SCRIPT ERROR
>> on function: test()
>> runtime/errors/for_loop_iterator_type_not_match_specified.gd
>> 3
>> Trying to assign value of type 'Resource' to a variable of type 'Node'.
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
func test():
print("Test range.")
for e: float in range(2, 5):
var elem := e
prints(var_to_str(e), var_to_str(elem))

print("Test int.")
for e: float in 3:
var elem := e
prints(var_to_str(e), var_to_str(elem))

print("Test untyped int array.")
var a1 := [10, 20, 30]
for e: float in a1:
var elem := e
prints(var_to_str(e), var_to_str(elem))

print("Test typed int array.")
var a2: Array[int] = [10, 20, 30]
for e: float in a2:
var elem := e
prints(var_to_str(e), var_to_str(elem))

print("Test String-keys dictionary.")
var d1 := {a = 1, b = 2, c = 3}
for k: StringName in d1:
var key := k
prints(var_to_str(k), var_to_str(key))

print("Test RefCounted-keys dictionary.")
var d2 := {RefCounted.new(): 1, Resource.new(): 2, ConfigFile.new(): 3}
for k: RefCounted in d2:
var key := k
prints(k.get_class(), key.get_class())
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
GDTEST_OK
Test range.
2.0 2.0
3.0 3.0
4.0 4.0
Test int.
0.0 0.0
1.0 1.0
2.0 2.0
Test untyped int array.
10.0 10.0
20.0 20.0
30.0 30.0
Test typed int array.
10.0 10.0
20.0 20.0
30.0 30.0
Test String-keys dictionary.
&"a" &"a"
&"b" &"b"
&"c" &"c"
Test RefCounted-keys dictionary.
RefCounted RefCounted
Resource Resource
ConfigFile ConfigFile

0 comments on commit 6c59ed9

Please sign in to comment.