Skip to content

Commit 413490c

Browse files
girdenis-pgirdenis-p
committed
Fix analyzer pushing SHADOWED_VARIABLE warning for members shadowed in subclasses
This fixes a bug in the analyzer where it did not push the SHADOWED_VARIABLE_BASE_CLASS warning for members shadowed by variable in subclass. It does this by comparing the class which contains the shadowed member with the class containing the variable, and pushing SHADOWED_VARIABLE only if the classes are the same. Additionally, SHADOWED_VARIABLE_BASE_CLASS can take an extra symbol which helps to specify the line for non native base class.
1 parent 87318a2 commit 413490c

15 files changed

+83
-42
lines changed

‎doc/classes/ProjectSettings.xml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -561,10 +561,10 @@
561561
When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when defining a local or member variable, signal, or enum that would have the same name as a built-in function or global class name, thus shadowing it.
562562
</member>
563563
<member name="debug/gdscript/warnings/shadowed_variable" type="int" setter="" getter="" default="1">
564-
When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when defining a local or member variable that would shadow a member variable that the class defines.
564+
When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when a local variable or local constant shadows a member declared in the current class.
565565
</member>
566566
<member name="debug/gdscript/warnings/shadowed_variable_base_class" type="int" setter="" getter="" default="1">
567-
When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when defining a local or subclass member variable that would shadow a variable that is inherited from a parent class.
567+
When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when a local variable or local constant shadows a member declared in a base class.
568568
</member>
569569
<member name="debug/gdscript/warnings/standalone_expression" type="int" setter="" getter="" default="1">
570570
When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when calling an expression that may have no effect on the surrounding code, such as writing [code]2 + 2[/code] as a statement.

‎modules/gdscript/gdscript_analyzer.cpp‎

Lines changed: 31 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5809,8 +5809,6 @@ void GDScriptAnalyzer::validate_call_arg(const List<GDScriptParser::DataType> &p
58095809
#ifdef DEBUG_ENABLED
58105810
void GDScriptAnalyzer::is_shadowing(GDScriptParser::IdentifierNode *p_identifier, const String &p_context, const bool p_in_local_scope) {
58115811
const StringName &name = p_identifier->name;
5812-
GDScriptParser::DataType base = parser->current_class->get_datatype();
5813-
GDScriptParser::ClassNode *base_class = base.class_type;
58145812

58155813
{
58165814
List<MethodInfo> gdscript_funcs;
@@ -5838,37 +5836,53 @@ void GDScriptAnalyzer::is_shadowing(GDScriptParser::IdentifierNode *p_identifier
58385836
}
58395837
}
58405838

5839+
const GDScriptParser::DataType current_class_type = parser->current_class->get_datatype();
58415840
if (p_in_local_scope) {
5842-
while (base_class != nullptr) {
5841+
GDScriptParser::ClassNode *base_class = current_class_type.class_type;
5842+
5843+
if (base_class != nullptr) {
58435844
if (base_class->has_member(name)) {
58445845
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()));
58455846
return;
58465847
}
58475848
base_class = base_class->base_type.class_type;
58485849
}
5850+
5851+
while (base_class != nullptr) {
5852+
if (base_class->has_member(name)) {
5853+
String base_class_name = base_class->get_global_name();
5854+
if (base_class_name.is_empty()) {
5855+
base_class_name = base_class->fqcn;
5856+
}
5857+
5858+
parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_VARIABLE_BASE_CLASS, p_context, p_identifier->name, base_class->get_member(name).get_type_name(), itos(base_class->get_member(name).get_line()), base_class_name);
5859+
return;
5860+
}
5861+
base_class = base_class->base_type.class_type;
5862+
}
58495863
}
58505864

5851-
StringName parent = base.native_type;
5852-
while (parent != StringName()) {
5853-
ERR_FAIL_COND_MSG(!class_exists(parent), "Non-existent native base class.");
5865+
StringName native_base_class = current_class_type.native_type;
5866+
while (native_base_class != StringName()) {
5867+
ERR_FAIL_COND_MSG(!class_exists(native_base_class), "Non-existent native base class.");
58545868

5855-
if (ClassDB::has_method(parent, name, true)) {
5856-
parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_VARIABLE_BASE_CLASS, p_context, p_identifier->name, "method", parent);
5869+
if (ClassDB::has_method(native_base_class, name, true)) {
5870+
parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_VARIABLE_BASE_CLASS, p_context, p_identifier->name, "method", native_base_class);
58575871
return;
5858-
} else if (ClassDB::has_signal(parent, name, true)) {
5859-
parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_VARIABLE_BASE_CLASS, p_context, p_identifier->name, "signal", parent);
5872+
} else if (ClassDB::has_signal(native_base_class, name, true)) {
5873+
parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_VARIABLE_BASE_CLASS, p_context, p_identifier->name, "signal", native_base_class);
58605874
return;
5861-
} else if (ClassDB::has_property(parent, name, true)) {
5862-
parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_VARIABLE_BASE_CLASS, p_context, p_identifier->name, "property", parent);
5875+
} else if (ClassDB::has_property(native_base_class, name, true)) {
5876+
parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_VARIABLE_BASE_CLASS, p_context, p_identifier->name, "property", native_base_class);
58635877
return;
5864-
} else if (ClassDB::has_integer_constant(parent, name, true)) {
5865-
parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_VARIABLE_BASE_CLASS, p_context, p_identifier->name, "constant", parent);
5878+
} else if (ClassDB::has_integer_constant(native_base_class, name, true)) {
5879+
parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_VARIABLE_BASE_CLASS, p_context, p_identifier->name, "constant", native_base_class);
58665880
return;
5867-
} else if (ClassDB::has_enum(parent, name, true)) {
5868-
parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_VARIABLE_BASE_CLASS, p_context, p_identifier->name, "enum", parent);
5881+
} else if (ClassDB::has_enum(native_base_class, name, true)) {
5882+
parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_VARIABLE_BASE_CLASS, p_context, p_identifier->name, "enum", native_base_class);
58695883
return;
58705884
}
5871-
parent = ClassDB::get_parent_class(parent);
5885+
native_base_class = ClassDB::get_parent_class(native_base_class);
58725886
}
58735887
}
58745888
#endif // DEBUG_ENABLED

‎modules/gdscript/gdscript_warning.cpp‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,10 +61,13 @@ String GDScriptWarning::get_message() const {
6161
return vformat(R"(The signal "%s" is declared but never explicitly used in the class.)", symbols[0]);
6262
case SHADOWED_VARIABLE:
6363
CHECK_SYMBOLS(4);
64-
return vformat(R"(The local %s "%s" is shadowing an already-declared %s at line %s.)", symbols[0], symbols[1], symbols[2], symbols[3]);
64+
return vformat(R"(The local %s "%s" is shadowing an already-declared %s at line %s in the current class.)", symbols[0], symbols[1], symbols[2], symbols[3]);
6565
case SHADOWED_VARIABLE_BASE_CLASS:
6666
CHECK_SYMBOLS(4);
67-
return vformat(R"(The local %s "%s" is shadowing an already-declared %s at the base class "%s".)", symbols[0], symbols[1], symbols[2], symbols[3]);
67+
if (symbols.size() > 4) {
68+
return vformat(R"(The local %s "%s" is shadowing an already-declared %s at line %s in the base class "%s".)", symbols[0], symbols[1], symbols[2], symbols[3], symbols[4]);
69+
}
70+
return vformat(R"(The local %s "%s" is shadowing an already-declared %s in the base class "%s".)", symbols[0], symbols[1], symbols[2], symbols[3]);
6871
case SHADOWED_GLOBAL_IDENTIFIER:
6972
CHECK_SYMBOLS(3);
7073
return vformat(R"(The %s "%s" has the same name as a %s.)", symbols[0], symbols[1], symbols[2]);

‎modules/gdscript/gdscript_warning.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,8 @@ class GDScriptWarning {
5353
UNUSED_PRIVATE_CLASS_VARIABLE, // Class variable is declared private ("_" prefix) but never used in the class.
5454
UNUSED_PARAMETER, // Function parameter is never used.
5555
UNUSED_SIGNAL, // Signal is defined but never explicitly used in the class.
56-
SHADOWED_VARIABLE, // Variable name shadowed by other variable in same class.
57-
SHADOWED_VARIABLE_BASE_CLASS, // Variable name shadowed by other variable in some base class.
56+
SHADOWED_VARIABLE, // A local variable/constant shadows a current class member.
57+
SHADOWED_VARIABLE_BASE_CLASS, // A local variable/constant shadows a base class member.
5858
SHADOWED_GLOBAL_IDENTIFIER, // A global class or function has the same name as variable.
5959
UNREACHABLE_CODE, // Code after a return statement.
6060
UNREACHABLE_PATTERN, // Pattern in a match statement after a catch all pattern (wildcard or bind).

‎modules/gdscript/tests/scripts/analyzer/warnings/confusable_local_usage.out‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,6 @@ GDTEST_OK
66
>> WARNING
77
>> Line: 5
88
>> SHADOWED_VARIABLE
9-
>> The local variable "a" is shadowing an already-declared variable at line 1.
9+
>> The local variable "a" is shadowing an already-declared variable at line 1 in the current class.
1010
1
1111
2

‎modules/gdscript/tests/scripts/analyzer/warnings/confusable_local_usage_initializer.out‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,6 @@ GDTEST_OK
1010
>> WARNING
1111
>> Line: 5
1212
>> SHADOWED_VARIABLE
13-
>> The local variable "a" is shadowing an already-declared variable at line 1.
13+
>> The local variable "a" is shadowing an already-declared variable at line 1 in the current class.
1414
1
1515
2

‎modules/gdscript/tests/scripts/analyzer/warnings/confusable_local_usage_loop.out‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ GDTEST_OK
66
>> WARNING
77
>> Line: 6
88
>> SHADOWED_VARIABLE
9-
>> The local variable "a" is shadowing an already-declared variable at line 1.
9+
>> The local variable "a" is shadowing an already-declared variable at line 1 in the current class.
1010
1
1111
2
1212
1

‎modules/gdscript/tests/scripts/analyzer/warnings/lambda_shadowing_arg.out‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,5 +2,5 @@ GDTEST_OK
22
>> WARNING
33
>> Line: 4
44
>> SHADOWED_VARIABLE
5-
>> The local function parameter "shadow" is shadowing an already-declared variable at line 1.
5+
>> The local function parameter "shadow" is shadowing an already-declared variable at line 1 in the current class.
66
shadow

‎modules/gdscript/tests/scripts/analyzer/warnings/shadowing_base.notest.gd‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
class_name ShadowingBase
2+
3+
const base_const_member = 1
4+
var base_variable_member
5+
6+
func base_function_member():
7+
pass

‎modules/gdscript/tests/scripts/analyzer/warnings/shadowning.gd‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
class_name ShadowedClass
2+
extends ShadowingBase
23

34
var member: int = 0
45

@@ -7,6 +8,7 @@ var print_debug := 'print_debug'
78
var print := 'print'
89

910
@warning_ignore("unused_variable")
11+
@warning_ignore("unused_local_constant")
1012
func test():
1113
var Array := 'Array'
1214
var Node := 'Node'
@@ -15,5 +17,8 @@ func test():
1517
var member := 'member'
1618
var reference := 'reference'
1719
var ShadowedClass := 'ShadowedClass'
20+
var base_variable_member
21+
const base_function_member = 1
22+
var base_const_member
1823

1924
print('warn')

0 commit comments

Comments
 (0)