GDScript: Fix bug with identifier shadowed below in current scope

[dalexeev]

• This PR implements the behavior we found desirable as a result of GDScript: Fix local can be used before declared #73427 discussion [comment].
• This behavior is consistent with 3.x, except for Test 5 [comment] (var a = a + 1).
  • Test 5 can be seen as a 3.x bug.
  • So the behavior in master can be seen as a regression from 3.x.
• This is Option 1 ("intuitive") [comment].
• Closes Accessing a member variable before declaring a variable that would overshadow returns null #54944.

[dalexeev]

Ready for review.

Test project: gdscript-test-scopes-4.x.zip

Notes:

1. I'm not sure about the warning identifiers CONFUSABLE_LOCAL_DECLARATION and CONFUSABLE_LOCAL_USAGE, especially the last one. If you have any suggestions, let me know.

2. The word "below" in the warning text is ok? In the case of var a = a + 1, the shadowing declaration is on the same line, and before. I would appreciate suggestions on how to improve the warning text.

• Should I add a line number to the warning? This will require some more code so that the IdentifierNode has a union pointer for all source types. [code]

3. The CONFUSABLE_LOCAL_USAGE warning is only generated for the current block:

var a = 1
func test():
    print(a) # Warning CONFUSABLE_LOCAL_USAGE.
    var a = 2
    print(a)

If you nest this inside another block, there will be no warning:

var a = 1
func test():
    print(a) # No warning.
    if true:
        var a = 2
        print(a)

Is this OK or should we also check all nested blocks recursively? I don't think it's worth it. Also, a naive implementation can have a negative performance impact for large scripts.

4. I think the for iterator variables and match binds don't need the warnings. The fact is that the locals are declared at the very beginning of the for/match branch block, so the same name identifier cannot be declared below them. But I'm not sure if the following code should generate the CONFUSABLE_LOCAL_DECLARATION warning:

for i in 3: # <-- CONFUSABLE_LOCAL_DECLARATION?
    print(i)
var i = 2

5. I didn't find any errors, even this case now give correct autocompletion:

var a := Color.RED

func f(a: String = a.to_html(), b = a.begins_with("#")):
    prints(a, b)

But please test too. Don't forget about possible regressions elsewhere.

[dalexeev]

Tech note

IdentifierNode [code] has name and source properties. This allows us to distinguish identifiers of the same name (for example, a member variable and a local variable) in the same scope. The bug is that the compiler and editor did not take this possibility into account, but always checked for the presence of an identifier in the following order: locals, class members, globals. Changes to gdscript_compiler.cpp in this PR are just moving the code inside the switch(identifier->source).

The order in which the identifier source is resolved:

1. GDScriptParser::parse_identifier() [code]. If there is a local identifier in the current block, then source is set, otherwise it will be resolved in the analyzer. Since the parser works in one pass, identifiers that are located before the declaration of a local variable/constant are also left with an unspecified source.

2. GDScriptAnalyzer::reduce_identifier(). If the identifier has no source is set, then the analyzer checks if there is a class member with the name [code]. If so, reduce_identifier_from_base() sets source [code]. Otherwise, the analyzer checks if there is a global (class, autoload, etc.) with the name.

[adamscott]

Discussed in the GDScript meeting. It has been approved! Thanks @dalexeev for your PR and your work.

[YuriSizov]

Thanks!

[Cykyrios]

One issue I have with this is lambda functions: any variable declared inside the lambda function can raise a warning about confusable local declaration if code below the function declares a variable with the same name.

var my_func := func my_func() -> int:
	var problem := 0
	return problem
var problem := my_func.call() as int

The variable problem is declared in a nested block, sure, but it is also a function, so I would not expect a warning to be raised.
As a side note, any parameter passed to my_func also causes the same warning (and @warning_ignore does not actually silence the warning).

The second example below is a bit different and probably has to do with bad practice: I assume it is intended behavior to raise a warning here for another_problem?

for i in 10:
	var another_problem := i
	print(another_problem)
@warning_ignore("unused_variable")
var another_problem := 0

[dalexeev]

The variable problem is declared in a nested block, sure, but it is also a function, so I would not expect a warning to be raised.

Lambdas capture identities from outer scope, so I think the warning is valid.

@warning_ignore does not actually silence the warning

This is a bug.