From a23dfe7d66b2054aa8604b86f60adad10e79da8f Mon Sep 17 00:00:00 2001 From: heppocogne <83043568+heppocogne@users.noreply.github.com> Date: Tue, 2 Aug 2022 17:55:42 +0900 Subject: [PATCH] Warn if a variable is not updated in the setter function Warn if a variable is not updated in the setter function Add `SETTER_MISSING_ASSIGNMENT` warning Fix formatting issues --- modules/gdscript/gdscript_analyzer.cpp | 79 +++++++++++++++++++++++++- modules/gdscript/gdscript_analyzer.h | 1 + modules/gdscript/gdscript_warning.cpp | 5 ++ modules/gdscript/gdscript_warning.h | 1 + 4 files changed, 85 insertions(+), 1 deletion(-) diff --git a/modules/gdscript/gdscript_analyzer.cpp b/modules/gdscript/gdscript_analyzer.cpp index 7ed440929c17..defbc9c3667c 100644 --- a/modules/gdscript/gdscript_analyzer.cpp +++ b/modules/gdscript/gdscript_analyzer.cpp @@ -927,7 +927,7 @@ void GDScriptAnalyzer::resolve_class_body(GDScriptParser::ClassNode *p_class) { member.variable->setter->parameters[0]->set_datatype(member.get_datatype()); } - resolve_function_body(member.variable->setter); + resolve_variable_setter(member.variable); } } } @@ -1255,6 +1255,83 @@ void GDScriptAnalyzer::resolve_function_body(GDScriptParser::FunctionNode *p_fun parser->current_function = previous_function; } +void GDScriptAnalyzer::resolve_variable_setter(GDScriptParser::VariableNode *p_variable) { + GDScriptParser::FunctionNode *p_setter_function = p_variable->setter; + if (p_setter_function->resolved_body) { + return; + } + p_setter_function->resolved_body = true; + + GDScriptParser::FunctionNode *previous_function = parser->current_function; + parser->current_function = p_setter_function; + + resolve_suite(p_setter_function->body); + + bool variable_assigned = false; + Vector body_nodes = p_setter_function->body->statements.duplicate(); + for (int i = 0; i < body_nodes.size() && !variable_assigned; i++) { + if (!body_nodes[i]) { + continue; + } + switch (body_nodes[i]->type) { + case GDScriptParser::Node::Type::ASSIGNMENT: { + auto const *assignment_node = static_cast(body_nodes[i]); + if (assignment_node->assignee && assignment_node->assignee->type == GDScriptParser::Node::Type::IDENTIFIER) { + auto const *identifier_node = static_cast(assignment_node->assignee); + if (identifier_node->source == GDScriptParser::IdentifierNode::Source::MEMBER_VARIABLE && identifier_node->name == p_variable->identifier->name) { + variable_assigned = true; + } + } + } break; + case GDScriptParser::Node::Type::IF: { + auto const *if_node = static_cast(body_nodes[i]); + body_nodes.push_back(if_node->true_block); + body_nodes.push_back(if_node->false_block); + } break; + case GDScriptParser::Node::Type::MATCH: { + auto const *match_node = static_cast(body_nodes[i]); + for (int b = 0; b < match_node->branches.size(); b++) { + body_nodes.push_back(match_node->branches[b]->block); + } + } break; + case GDScriptParser::Node::Type::WHILE: { + auto const *while_node = static_cast(body_nodes[i]); + body_nodes.push_back(while_node->loop); + } break; + case GDScriptParser::Node::Type::FOR: { + auto const *for_node = static_cast(body_nodes[i]); + body_nodes.push_back(for_node->loop); + } break; + case GDScriptParser::Node::Type::SUITE: { + auto const *suite_node = static_cast(body_nodes[i]); + for (int s = 0; s < suite_node->statements.size(); s++) { + body_nodes.push_back(suite_node->statements[s]); + } + } break; + default: + break; + } + } +#ifdef DEBUG_ENABLED + if (!variable_assigned) { + parser->push_warning(p_variable->setter, GDScriptWarning::SETTER_MISSING_ASSIGNMENT, p_variable->identifier->name); + } +#endif // DEBUG_ENABLED + + GDScriptParser::DataType return_type = p_setter_function->body->get_datatype(); + if (!p_setter_function->get_datatype().is_hard_type() && return_type.is_set()) { + // Use the suite inferred type if return isn't explicitly set. + return_type.type_source = GDScriptParser::DataType::INFERRED; + p_setter_function->set_datatype(p_setter_function->body->get_datatype()); + } else if (p_setter_function->get_datatype().is_hard_type() && (p_setter_function->get_datatype().kind != GDScriptParser::DataType::BUILTIN || p_setter_function->get_datatype().builtin_type != Variant::NIL)) { + if (!p_setter_function->body->has_return && p_setter_function->identifier->name != GDScriptLanguage::get_singleton()->strings._init) { + push_error(R"(Not all code paths return a value.)", p_setter_function); + } + } + + parser->current_function = previous_function; +} + void GDScriptAnalyzer::decide_suite_type(GDScriptParser::Node *p_suite, GDScriptParser::Node *p_statement) { if (p_statement == nullptr) { return; diff --git a/modules/gdscript/gdscript_analyzer.h b/modules/gdscript/gdscript_analyzer.h index 3966b81b6ed9..6afe7f89029f 100644 --- a/modules/gdscript/gdscript_analyzer.h +++ b/modules/gdscript/gdscript_analyzer.h @@ -63,6 +63,7 @@ class GDScriptAnalyzer { void resolve_class_body(GDScriptParser::ClassNode *p_class); void resolve_function_signature(GDScriptParser::FunctionNode *p_function); void resolve_function_body(GDScriptParser::FunctionNode *p_function); + void resolve_variable_setter(GDScriptParser::VariableNode *p_variable); void resolve_node(GDScriptParser::Node *p_node); void resolve_suite(GDScriptParser::SuiteNode *p_suite); void resolve_if(GDScriptParser::IfNode *p_if); diff --git a/modules/gdscript/gdscript_warning.cpp b/modules/gdscript/gdscript_warning.cpp index 1cae7bdfac06..f3d0fb343f37 100644 --- a/modules/gdscript/gdscript_warning.cpp +++ b/modules/gdscript/gdscript_warning.cpp @@ -155,6 +155,10 @@ String GDScriptWarning::get_message() const { case INT_ASSIGNED_TO_ENUM: { return "Integer used when an enum value is expected. If this is intended cast the integer to the enum type."; } + case SETTER_MISSING_ASSIGNMENT: { + CHECK_SYMBOLS(1); + return vformat(R"(The member variable '%s' may not be updated within the setter function.)", symbols[0]); + } case WARNING_MAX: break; // Can't happen, but silences warning } @@ -215,6 +219,7 @@ String GDScriptWarning::get_name_from_code(Code p_code) { "EMPTY_FILE", "SHADOWED_GLOBAL_IDENTIFIER", "INT_ASSIGNED_TO_ENUM", + "SETTER_MISSING_ASSIGNMENT", }; static_assert((sizeof(names) / sizeof(*names)) == WARNING_MAX, "Amount of warning types don't match the amount of warning names."); diff --git a/modules/gdscript/gdscript_warning.h b/modules/gdscript/gdscript_warning.h index a639e7b44e57..cbcd71437e6d 100644 --- a/modules/gdscript/gdscript_warning.h +++ b/modules/gdscript/gdscript_warning.h @@ -78,6 +78,7 @@ class GDScriptWarning { EMPTY_FILE, // A script file is empty. SHADOWED_GLOBAL_IDENTIFIER, // A global class or function has the same name as variable. INT_ASSIGNED_TO_ENUM, // An integer value was assigned to an enum-typed variable without casting. + SETTER_MISSING_ASSIGNMENT, // A setter function does not assign any value to the member variable. WARNING_MAX, };