Fix GDScript base and outer classes, signals and functions lookup order

[adamscott]

Fix GDScript inner and outer classes lookup order

• Add outer class lookup test
• Add signal lookup test

Fixes #70216 (again)
Fixes #70495 - Accessing keys() dict of an enum defined in a singleton crashes app

[vonagam]

Hah, but unfortunately not right. There is difference in logic for base and outer classes. In base classes they do handle variables, functions and signals, while in outer lookup it is ignored.

So, the next example based on that:

class A:
 var Q := 'right one'

class X:
 const Q := 'wrong one'

class Y extends X:
 class B extends A:
   func check() -> void:
     print(Q)

func _ready() -> void:
 var b := Y.B.new()
 b.check()

[adamscott]

Hah, but unfortunately not right. There is difference in logic for base and outer classes. In base classes they do handle variables, functions and signals, while in outer lookup it is ignored.

So, the next example based on that:

class A:
 var Q := 'right one'

class X:
 const Q := 'wrong one'

class Y extends X:
 class B extends A:
   func check() -> void:
     print(Q)

func _ready() -> void:
 var b := Y.B.new()
 b.check()

Please. I do please you to create a complete example in advance.

[vonagam]

Yeah, sorry for that, my reasoning is that I try to show the simplest example that everyone will see is wrong, not the most convoluted one (because it will be confusing and less in priority).

When you present a solution, I look at it and analyze what would be the next simplest showcase that the solution is wrong.

I also pointed out the root of the problem - as I wrote in the issue opening text you cannot continue using get_class_node_current_scope_classes just for outer logic. So when the known root of the problem is not addressed it is quite easy for me to come up with next example.

[vonagam]

please add some context or a solution yourself

I think I provided the context, but solution is not a one-line change. My thoughts: I think that get_class_node_current_scope_classes has value and to be consistent with other places it should be used to replace whole while (base_class != nullptr) loop and in iteration you need to check if the current script is outer or not, if it is base one then instance things like variables/functions/signals should be accepted, otherwise not.

[adamscott]

@vonagam I replaced the list value of GDScriptParser::ClassNode * with a new list of GDScriptAnalyzer::CurrentScopeClass structs. It adds metadata, as you suggested, to know if the current class node "type" (I'll probably rename it) is BASE or OUTER.

Then, I realized that get_class_node_current_scope_classes() in reduce_identifier_from_base() already loops through the base, so I removed the while (base != nullptr) to introduce a single if (base != nullptr) condition.

@rune-scape @anvilfolk If you could add your input to this complex issue, it would be greatly appreciated.

[anvilfolk]

Been looking at this a little bit, and I'm starting to wonder if maybe we shouldn't make the analyzer a little more flexible. What if we had a bitfield in reduce_identifier_from_base and the like, which defines which types of things are in scope?

I can imagine this bitfield having:

• CLASS_TYPES, such as inner classes,
• CLASS_MEMBER, such as static functions & enums
• INSTANCE_MEMBER such as variables and non-static functions), and ALL for everything.
• ALL all of the above

This way, depending on what we are searching for (e.g. whether we're looking into base classes or outer classes), we could pass the method some information stating that it should ignore instance members and class types, and only look for class members.

Would this make sense at all?

[vonagam]

Alternative solution to separate bases from outer:
get_class_node_current_scope_classes currently returns base classes first then outer ones. So if you get a number of base classes either by returning or passing int by reference then by comparing if current index is less than a number of base classes or not you will know it's type. That's an option that requires no additional types/structures, but just an option.

[adamscott]

get_class_node_current_scope_classes currently returns base classes first then outer ones.

I think you misunderstand the method.

It returns, yes, base classes in priority over outer classes. But the recursive nature of the method makes it so that outer classes of the first base class have priority over the second base class.

[adamscott]

It returns, yes, base classes in priority over outer classes. But the recursive nature of the method makes it so that outer classes of the first base class have priority over the second base class.

Whoa, I'm totally wrong. You're right!

[adamscott]

Just found out how to add support for signals. It's in the PR.

[adamscott]

@vnen @vonagam I rebased the PR with the merge of #70613.

I think the PR is ready.

[vnen]

There's a bit of an issue about loading the script in the middle of the analyzer because this will not work if there are cyclic dependencies between the classes (that is, the script being loaded depends on the one being parsed, this will fail).

I'm not sure why this needs to be made a compile-time constant. I can see some benefits but I don't know if it can work properly in all cases so it might be better for the compiler to sort this out in the next phase.

[adamscott]

There's a bit of an issue about loading the script in the middle of the analyzer because this will not work if there are cyclic dependencies between the classes (that is, the script being loaded depends on the one being parsed, this will fail).

For now, the tests pass and no issue about this has been found yet. Do you have an idea of a test to add where the issue would happen?

I'm not sure why this needs to be made a compile-time constant.

I added the inlining as it's just a set of instructions to execute, there's no logic beyond this function, like there's no need for this function other than reduce duplicated code. But I'm kinda a noob at C++, so I can remove the inline part.

[vnen]

Looking at the code deeper now I understand why this "works". get_full_script() actually returns the shallow script here, since this is not asking to update from disk. The current script being loaded is already on the full_gdscript_cache because it 's added to the map before it's actually compiled, so the get_full_script() will never fail, but sometimes return a shallow (i.e. not compiled) script instead.

So technically this will always work because even if it can't compile due to a reference cycle, it will still get the script that will be compiled later.

The main problem now is that you add a reference to the script in the function's list of constants, which will create a reference cycle that can never be cleared. Especially since this is also done for the very script being compiled now, so any reference to the same class will create this ref cycle (noted, it's not often that one references the same class by name, but there's no reason to keep a new reference to same script if that happens).

My question is still the same though: why does this need to be a compile-time constant? Even if you are accessing members from it, I believe the analyzer is able to retrieve the members directly without having to store the class. For instance: var v = ClassName.InnerClass.SOME_CONST can just grab SOME_CONST and store it directly as a constant. With this PR the InnerClass would also be stored in this case, which is unnecessary.

Unless I'm missing something else, making this constant does not serve any particular purpose and it's not needed in any case.

[vonagam]

It started with need to make const X := preload(...).X work. If a subscript for an inner class is not constant then it cannot be assigned to a constant right now. And if it is a constant then it is expected to have reduced_value. #70055 is where it was fixed like that.

[vnen]

My issue is that with a script like this:

class_name Foo
const x = 0
func bar():
	print(Foo.x)

You now store a reference to the Foo script inside the constants of its own function bar. I understand storing references to other scripts (even if they make a cycle) but I don't see how this is necessary.

At the very least this should avoid storing the very same script being compiled.

[vnen]

The overall approach is good, I just have the one concerned I commented about loading scripts during parsing.

[adamscott]

The build fails, but it's Windows that failed to build the artifacts, not that it had errors elsewhere.

[vonagam]

I'm coauthoring (for part inside reduce_identifier_from_base), so do approve.

Though, it is bugging me that get_full_script is used for getting a script for a class and not get_shallow_script with raise_status as it is done for the same purpose in preload. But at this point it is already a preexisting thing, this PR extends what this logic applies to but does not change it. I do hope that in future shallow script can be made to work.

Otherwise everything is perfect.

[adamscott]

and not get_shallow_script with raise_status as it is done for the same purpose in preload.

I did not try that, I'll try!

[adamscott]

As just discussed with @vonagam, get_shallow_script() returns errors and doesn't pass the tests. But it works in GDScriptAnalyzer::reduce_preload();.

The raise_status part that he underline is more complex than I anticipated.

So, as he suggested, another PR should maybe take a look at why and fix it.

[vnen]

I believe we can merge as it is since it works and fixes the issues. I still have concerns about the lifetime of the scripts since I believe there are some cases creating reference cycles, but we can solve those problems (if they do exist indeed) properly later on.

[akien-mga]

Thanks to both of you, huge work!