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

Fix for not being able to ignore shadowing warnings on class scope #75620

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 23 additions & 24 deletions modules/gdscript/gdscript_analyzer.cpp
Expand Up @@ -1553,7 +1553,7 @@ void GDScriptAnalyzer::resolve_function_signature(GDScriptParser::FunctionNode *
}
parser->push_warning(p_function->parameters[i]->identifier, GDScriptWarning::UNUSED_PARAMETER, visible_name, p_function->parameters[i]->identifier->name);
}
is_shadowing(p_function->parameters[i]->identifier, "function parameter");
is_shadowing(p_function->parameters[i]->identifier, "function parameter", true);
#endif // DEBUG_ENABLED
#ifdef TOOLS_ENABLED
if (p_function->parameters[i]->initializer) {
Expand Down Expand Up @@ -1874,9 +1874,8 @@ void GDScriptAnalyzer::resolve_variable(GDScriptParser::VariableNode *p_variable
} else if (p_variable->assignments == 0) {
parser->push_warning(p_variable, GDScriptWarning::UNASSIGNED_VARIABLE, p_variable->identifier->name);
}

is_shadowing(p_variable->identifier, kind);
}
is_shadowing(p_variable->identifier, kind, p_is_local);
#endif
}

Expand All @@ -1889,9 +1888,8 @@ void GDScriptAnalyzer::resolve_constant(GDScriptParser::ConstantNode *p_constant
if (p_constant->usages == 0) {
parser->push_warning(p_constant, GDScriptWarning::UNUSED_LOCAL_CONSTANT, p_constant->identifier->name);
}

is_shadowing(p_constant->identifier, kind);
}
is_shadowing(p_constant->identifier, kind, p_is_local);
#endif
}

Expand Down Expand Up @@ -2052,7 +2050,7 @@ void GDScriptAnalyzer::resolve_for(GDScriptParser::ForNode *p_for) {
p_for->set_datatype(p_for->loop->get_datatype());
#ifdef DEBUG_ENABLED
if (p_for->variable) {
is_shadowing(p_for->variable, R"("for" iterator variable)");
is_shadowing(p_for->variable, R"("for" iterator variable)", true);
}
#endif
}
Expand Down Expand Up @@ -2148,7 +2146,7 @@ void GDScriptAnalyzer::resolve_match_pattern(GDScriptParser::PatternNode *p_matc
}
p_match_pattern->bind->set_datatype(result);
#ifdef DEBUG_ENABLED
is_shadowing(p_match_pattern->bind, "pattern bind");
is_shadowing(p_match_pattern->bind, "pattern bind", true);
if (p_match_pattern->bind->usages == 0 && !String(p_match_pattern->bind->name).begins_with("_")) {
parser->push_warning(p_match_pattern->bind, GDScriptWarning::UNUSED_VARIABLE, p_match_pattern->bind->name);
}
Expand Down Expand Up @@ -4890,8 +4888,8 @@ void GDScriptAnalyzer::validate_call_arg(const List<GDScriptParser::DataType> &p
}

#ifdef DEBUG_ENABLED
void GDScriptAnalyzer::is_shadowing(GDScriptParser::IdentifierNode *p_local, const String &p_context) {
const StringName &name = p_local->name;
void GDScriptAnalyzer::is_shadowing(GDScriptParser::IdentifierNode *p_identifier, const String &p_context, const bool p_in_local_scope) {
const StringName &name = p_identifier->name;
GDScriptParser::DataType base = parser->current_class->get_datatype();
GDScriptParser::ClassNode *base_class = base.class_type;

Expand All @@ -4901,49 +4899,50 @@ void GDScriptAnalyzer::is_shadowing(GDScriptParser::IdentifierNode *p_local, con

for (MethodInfo &info : gdscript_funcs) {
if (info.name == name) {
parser->push_warning(p_local, GDScriptWarning::SHADOWED_GLOBAL_IDENTIFIER, p_context, name, "built-in function");
parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_GLOBAL_IDENTIFIER, p_context, name, "built-in function");
return;
}
}

if (Variant::has_utility_function(name)) {
parser->push_warning(p_local, GDScriptWarning::SHADOWED_GLOBAL_IDENTIFIER, p_context, name, "built-in function");
parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_GLOBAL_IDENTIFIER, p_context, name, "built-in function");
return;
} else if (ClassDB::class_exists(name)) {
parser->push_warning(p_local, GDScriptWarning::SHADOWED_GLOBAL_IDENTIFIER, p_context, name, "global class");
parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_GLOBAL_IDENTIFIER, p_context, name, "global class");
return;
} else if (GDScriptParser::get_builtin_type(name) != Variant::VARIANT_MAX) {
parser->push_warning(p_local, GDScriptWarning::SHADOWED_GLOBAL_IDENTIFIER, p_context, name, "built-in type");
parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_GLOBAL_IDENTIFIER, p_context, name, "built-in type");
return;
}
}

while (base_class != nullptr) {
if (base_class->has_member(name)) {
parser->push_warning(p_local, GDScriptWarning::SHADOWED_VARIABLE, p_context, p_local->name, base_class->get_member(name).get_type_name(), itos(base_class->get_member(name).get_line()));
return;
if (p_in_local_scope) {
while (base_class != nullptr) {
if (base_class->has_member(name)) {
parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_VARIABLE, p_context, p_identifier->name, base_class->get_member(name).get_type_name(), itos(base_class->get_member(name).get_line()));
return;
}
base_class = base_class->base_type.class_type;
}
base_class = base_class->base_type.class_type;
}

StringName parent = base.native_type;
while (parent != StringName()) {
ERR_FAIL_COND_MSG(!class_exists(parent), "Non-existent native base class.");

if (ClassDB::has_method(parent, name, true)) {
parser->push_warning(p_local, GDScriptWarning::SHADOWED_VARIABLE_BASE_CLASS, p_context, p_local->name, "method", parent);
parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_VARIABLE_BASE_CLASS, p_context, p_identifier->name, "method", parent);
return;
} else if (ClassDB::has_signal(parent, name, true)) {
parser->push_warning(p_local, GDScriptWarning::SHADOWED_VARIABLE_BASE_CLASS, p_context, p_local->name, "signal", parent);
parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_VARIABLE_BASE_CLASS, p_context, p_identifier->name, "signal", parent);
return;
} else if (ClassDB::has_property(parent, name, true)) {
parser->push_warning(p_local, GDScriptWarning::SHADOWED_VARIABLE_BASE_CLASS, p_context, p_local->name, "property", parent);
parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_VARIABLE_BASE_CLASS, p_context, p_identifier->name, "property", parent);
return;
} else if (ClassDB::has_integer_constant(parent, name, true)) {
parser->push_warning(p_local, GDScriptWarning::SHADOWED_VARIABLE_BASE_CLASS, p_context, p_local->name, "constant", parent);
parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_VARIABLE_BASE_CLASS, p_context, p_identifier->name, "constant", parent);
return;
} else if (ClassDB::has_enum(parent, name, true)) {
parser->push_warning(p_local, GDScriptWarning::SHADOWED_VARIABLE_BASE_CLASS, p_context, p_local->name, "enum", parent);
parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_VARIABLE_BASE_CLASS, p_context, p_identifier->name, "enum", parent);
return;
}
parent = ClassDB::get_parent_class(parent);
Expand Down
2 changes: 1 addition & 1 deletion modules/gdscript/gdscript_analyzer.h
Expand Up @@ -133,7 +133,7 @@ class GDScriptAnalyzer {
Ref<GDScriptParserRef> get_parser_for(const String &p_path);
void reduce_identifier_from_base_set_class(GDScriptParser::IdentifierNode *p_identifier, GDScriptParser::DataType p_identifier_datatype);
#ifdef DEBUG_ENABLED
void is_shadowing(GDScriptParser::IdentifierNode *p_local, const String &p_context);
void is_shadowing(GDScriptParser::IdentifierNode *p_identifier, const String &p_context, const bool p_in_local_scope);
#endif

public:
Expand Down
14 changes: 0 additions & 14 deletions modules/gdscript/gdscript_parser.cpp
Expand Up @@ -783,20 +783,6 @@ void GDScriptParser::parse_class_member(T *(GDScriptParser::*p_parse_function)(b

if (member->identifier != nullptr) {
if (!((String)member->identifier->name).is_empty()) { // Enums may be unnamed.

#ifdef DEBUG_ENABLED
List<MethodInfo> gdscript_funcs;
GDScriptLanguage::get_singleton()->get_public_functions(&gdscript_funcs);
for (MethodInfo &info : gdscript_funcs) {
if (info.name == member->identifier->name) {
push_warning(member->identifier, GDScriptWarning::SHADOWED_GLOBAL_IDENTIFIER, p_member_kind, member->identifier->name, "built-in function");
}
}
if (Variant::has_utility_function(member->identifier->name)) {
push_warning(member->identifier, GDScriptWarning::SHADOWED_GLOBAL_IDENTIFIER, p_member_kind, member->identifier->name, "built-in function");
}
#endif

if (current_class->members_indices.has(member->identifier->name)) {
push_error(vformat(R"(%s "%s" has the same name as a previously declared %s.)", p_member_kind.capitalize(), member->identifier->name, current_class->get_member(member->identifier->name).get_type_name()), member->identifier);
} else {
Expand Down
@@ -1,7 +1,7 @@
extends Node

@onready var shorthand = $Node
@onready var call = get_node(^"Node")
@onready var call_no_cast = get_node(^"Node")
@onready var shorthand_with_cast = $Node as Node
@onready var call_with_cast = get_node(^"Node") as Node

Expand All @@ -13,6 +13,6 @@ func _init():
func test():
# Those are expected to be `null` since `_ready()` is never called on tests.
prints("shorthand", shorthand)
prints("call", call)
prints("call_no_cast", call_no_cast)
prints("shorthand_with_cast", shorthand_with_cast)
prints("call_with_cast", call_with_cast)
@@ -1,5 +1,5 @@
GDTEST_OK
shorthand <null>
call <null>
call_no_cast <null>
shorthand_with_cast <null>
call_with_cast <null>
@@ -1,5 +1,9 @@
var member: int = 0

var print_debug := 'print_debug'
@warning_ignore("shadowed_global_identifier")
var print := 'print'

@warning_ignore("unused_variable")
func test():
var Array := 'Array'
Expand Down
16 changes: 10 additions & 6 deletions modules/gdscript/tests/scripts/analyzer/warnings/shadowning.out
@@ -1,26 +1,30 @@
GDTEST_OK
>> WARNING
>> Line: 5
>> Line: 3
>> SHADOWED_GLOBAL_IDENTIFIER
>> The variable "print_debug" has the same name as a built-in function.
>> WARNING
>> Line: 9
>> SHADOWED_GLOBAL_IDENTIFIER
>> The variable "Array" has the same name as a built-in type.
>> WARNING
>> Line: 6
>> Line: 10
>> SHADOWED_GLOBAL_IDENTIFIER
>> The variable "Node" has the same name as a global class.
>> WARNING
>> Line: 7
>> Line: 11
>> SHADOWED_GLOBAL_IDENTIFIER
>> The variable "is_same" has the same name as a built-in function.
>> WARNING
>> Line: 8
>> Line: 12
>> SHADOWED_GLOBAL_IDENTIFIER
>> The variable "sqrt" has the same name as a built-in function.
>> WARNING
>> Line: 9
>> Line: 13
>> SHADOWED_VARIABLE
>> The local variable "member" is shadowing an already-declared variable at line 1.
>> WARNING
>> Line: 10
>> Line: 14
>> SHADOWED_VARIABLE_BASE_CLASS
>> The local variable "reference" is shadowing an already-declared method at the base class "RefCounted".
warn