Skip to content

Add warnings for shadowed @GDScriptand @GlobalScope functions #106987

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

Open
EdwardChanCH wants to merge 1 commit into
base: master
Choose a base branch
from
+92 −1
Open

Add warnings for shadowed @GDScriptand @GlobalScope functions #106987

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
33 changes: 32 additions & 1 deletion modules/gdscript/gdscript_analyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1119,6 +1119,9 @@ void GDScriptAnalyzer::resolve_class_member(GDScriptParser::ClassNode *p_class,
} break;
case GDScriptParser::ClassNode::Member::SIGNAL: {
check_class_member_name_conflict(p_class, member.signal->identifier->name, member.signal);
#ifdef DEBUG_ENABLED
is_shadowing(member.signal->identifier, "signal", false);
#endif // DEBUG_ENABLED

member.signal->set_datatype(resolving_datatype);

Expand Down Expand Up @@ -1149,6 +1152,9 @@ void GDScriptAnalyzer::resolve_class_member(GDScriptParser::ClassNode *p_class,
} break;
case GDScriptParser::ClassNode::Member::ENUM: {
check_class_member_name_conflict(p_class, member.m_enum->identifier->name, member.m_enum);
#ifdef DEBUG_ENABLED
is_shadowing(member.m_enum->identifier, "named enum", false);
#endif // DEBUG_ENABLED

member.m_enum->set_datatype(resolving_datatype);
GDScriptParser::DataType enum_type = make_class_enum_type(member.m_enum->identifier->name, p_class, parser->script_path, true);
Expand Down Expand Up @@ -1183,8 +1189,9 @@ void GDScriptAnalyzer::resolve_class_member(GDScriptParser::ClassNode *p_class,
dictionary[String(element.identifier->name)] = element.value;

#ifdef DEBUG_ENABLED
// Named enum identifiers do not shadow anything since you can only access them with `NamedEnum.ENUM_VALUE`.
// Named enum values do not shadow anything since you can only access them with `NamedEnum.VALUE`.
if (member.m_enum->identifier->name == StringName()) {
// TODO: Unreachable code? Unnamed enum values are checked in case GDScriptParser::ClassNode::Member::ENUM_VALUE.
is_shadowing(element.identifier, "enum member", false);
}
#endif // DEBUG_ENABLED
Expand Down Expand Up @@ -1214,6 +1221,9 @@ void GDScriptAnalyzer::resolve_class_member(GDScriptParser::ClassNode *p_class,

if (member.enum_value.custom_value) {
check_class_member_name_conflict(p_class, member.enum_value.identifier->name, member.enum_value.custom_value);
#ifdef DEBUG_ENABLED
is_shadowing(member.enum_value.identifier, "enum value", false);
#endif // DEBUG_ENABLED

const GDScriptParser::EnumNode *prev_enum = current_enum;
current_enum = member.enum_value.parent_enum;
Expand All @@ -1230,6 +1240,9 @@ void GDScriptAnalyzer::resolve_class_member(GDScriptParser::ClassNode *p_class,
}
} else {
check_class_member_name_conflict(p_class, member.enum_value.identifier->name, member.enum_value.parent_enum);
#ifdef DEBUG_ENABLED
is_shadowing(member.enum_value.identifier, "enum value", false);
#endif // DEBUG_ENABLED

if (member.enum_value.index > 0) {
const GDScriptParser::EnumNode::Value &prev_value = member.enum_value.parent_enum->values[member.enum_value.index - 1];
Expand All @@ -1247,6 +1260,7 @@ void GDScriptAnalyzer::resolve_class_member(GDScriptParser::ClassNode *p_class,
member.enum_value.identifier->set_datatype(make_class_enum_type(UNNAMED_ENUM, p_class, parser->script_path, false));
} break;
case GDScriptParser::ClassNode::Member::CLASS:
// TODO: Unreachanble code? Inner classes do not trigger this.
check_class_member_name_conflict(p_class, member.m_class->identifier->name, member.m_class);
// If it's already resolving, that's ok.
if (!member.m_class->base_type.is_resolving()) {
Expand Down Expand Up @@ -1314,6 +1328,15 @@ void GDScriptAnalyzer::resolve_class_interface(GDScriptParser::ClassNode *p_clas
resolve_class_interface(base_class, p_class);
}

#ifdef DEBUG_ENABLED
// TODO: This only checks if any inner class name shadows global identifiers,
// because global class names are also considered as global identifiers.
// Checking global class names would require changing is_shadowing() to ignore
// global class names as global identifiers when checking for shadowed global identifiers.
if (p_class->identifier && !ScriptServer::is_global_class(p_class->identifier->name)) {
is_shadowing(p_class->identifier, "class", false);
}
#endif // DEBUG_ENABLED
for (int i = 0; i < p_class->members.size(); i++) {
resolve_class_member(p_class, i);

Expand Down Expand Up @@ -1968,6 +1991,14 @@ void GDScriptAnalyzer::resolve_function_signature(GDScriptParser::FunctionNode *
}

#ifdef DEBUG_ENABLED
// Named lambdas do not override anything because they cannot be accessed by name.
// Native methods have a special push_warning() instead of is_shadowing(),
// because the latter also throws an unnecessary SHADOWED_VARIABLE_BASE_CLASS for native method overrides.
MethodBind *native_method = ClassDB::get_method(parser->current_class->base_type.native_type, function_name);
if (!p_is_lambda && !native_method && p_function->identifier) {
is_shadowing(p_function->identifier, "method", false);
}

if (p_function->return_type == nullptr) {
parser->push_warning(p_function, GDScriptWarning::UNTYPED_DECLARATION, "Function", function_visible_name);
}
Expand Down
48 changes: 48 additions & 0 deletions modules/gdscript/tests/scripts/analyzer/warnings/shadowning.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,51 @@ func test():
var base_const_member

print('warn')

func char(_code :int) -> Variant:
return null

func log(_x: float) -> Variant:
return null

func method1(_arg: float) -> Variant:
return null

func method2(..._arg: Array) -> Variant:
return null

func method3() -> void:
var sinh: int = 1

for char in []:
pass

var k = 1
match k:
var abs:
pass

var named_lambda = func log(): # Note: Named lambdas do not override anything.
return null

class floor extends Node:
var property1: Variant = null

var sin: Variant = null

const cos: Variant = null

enum tan {
min = 333 # Note: Named enum key do not shadow anything, because it can only be accessed as NamedEnum.KEY.
}

enum {
clamp,
clampi = 1
}

@warning_ignore("unused_signal")
signal sqrt

@warning_ignore("unused_signal")
signal s(pow: Variant) # Note: Signal parameter do not shadow anything.
12 changes: 12 additions & 0 deletions modules/gdscript/tests/scripts/analyzer/warnings/shadowning.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,16 @@ GDTEST_OK
~~ WARNING at line 19: (SHADOWED_VARIABLE_BASE_CLASS) The local variable "base_variable_member" is shadowing an already-declared variable at line 4 in the base class "ShadowingBase".
~~ WARNING at line 20: (SHADOWED_VARIABLE_BASE_CLASS) The local constant "base_function_member" is shadowing an already-declared function at line 6 in the base class "ShadowingBase".
~~ WARNING at line 21: (SHADOWED_VARIABLE_BASE_CLASS) The local variable "base_const_member" is shadowing an already-declared constant at line 3 in the base class "ShadowingBase".
~~ WARNING at line 25: (SHADOWED_GLOBAL_IDENTIFIER) The method "char" has the same name as a built-in function.
~~ WARNING at line 28: (SHADOWED_GLOBAL_IDENTIFIER) The method "log" has the same name as a built-in function.
~~ WARNING at line 38: (SHADOWED_GLOBAL_IDENTIFIER) The variable "sinh" has the same name as a built-in function.
~~ WARNING at line 40: (SHADOWED_GLOBAL_IDENTIFIER) The "for" iterator variable "char" has the same name as a built-in function.
~~ WARNING at line 45: (SHADOWED_GLOBAL_IDENTIFIER) The pattern bind "abs" has the same name as a built-in function.
~~ WARNING at line 51: (SHADOWED_GLOBAL_IDENTIFIER) The class "floor" has the same name as a built-in function.
~~ WARNING at line 54: (SHADOWED_GLOBAL_IDENTIFIER) The variable "sin" has the same name as a built-in function.
~~ WARNING at line 56: (SHADOWED_GLOBAL_IDENTIFIER) The constant "cos" has the same name as a built-in function.
~~ WARNING at line 58: (SHADOWED_GLOBAL_IDENTIFIER) The named enum "tan" has the same name as a built-in function.
~~ WARNING at line 63: (SHADOWED_GLOBAL_IDENTIFIER) The enum value "clamp" has the same name as a built-in function.
~~ WARNING at line 64: (SHADOWED_GLOBAL_IDENTIFIER) The enum value "clampi" has the same name as a built-in function.
~~ WARNING at line 68: (SHADOWED_GLOBAL_IDENTIFIER) The signal "sqrt" has the same name as a built-in function.
warn
Loading