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.

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

Warn if a variable is not updated in the setter function #63818

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
79 changes: 78 additions & 1 deletion modules/gdscript/gdscript_analyzer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -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<GDScriptParser::Node *> 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<GDScriptParser::AssignmentNode *>(body_nodes[i]);
if (assignment_node->assignee && assignment_node->assignee->type == GDScriptParser::Node::Type::IDENTIFIER) {
auto const *identifier_node = static_cast<GDScriptParser::IdentifierNode *>(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<GDScriptParser::IfNode *>(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<GDScriptParser::MatchNode *>(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<GDScriptParser::WhileNode *>(body_nodes[i]);
body_nodes.push_back(while_node->loop);
} break;
case GDScriptParser::Node::Type::FOR: {
auto const *for_node = static_cast<GDScriptParser::ForNode *>(body_nodes[i]);
body_nodes.push_back(for_node->loop);
} break;
case GDScriptParser::Node::Type::SUITE: {
auto const *suite_node = static_cast<GDScriptParser::SuiteNode *>(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;
Expand Down
1 change: 1 addition & 0 deletions modules/gdscript/gdscript_analyzer.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
5 changes: 5 additions & 0 deletions modules/gdscript/gdscript_warning.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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.");
Expand Down
1 change: 1 addition & 0 deletions modules/gdscript/gdscript_warning.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down