Fix constants scope in extended or inner GDScript classes

[adamscott]

Especially when checking the typing, the gdscript parser doesn't seem to pass through the code to set the right script_class->outer value for a given GDScriptParser::ClassNode. It is available though inside script_class->base_type.class_type in GDScriptAnalyzer::resolve_datatype().

~~So this PR uses script_class->base_type.class_type if the script_class->outer is a nullptr.~~

Edit: I updated the logic! It now checks for outer classes and their extended classes, without the need of adding one more loop. I prioritized the outer classes first, as this was the logic as for now.

This code now works:

# `base.gd` declares `A`
extends "base.gd"

const OuterDeclared: = "OuterDeclared"

static func test(a: A) -> void:
	print(A)  # prints <GDScript #...>

# `base_inner.gd` declares `B`
class InnerBaseClass extends "base_inner.gd":
	static func test_a(a: A) -> void:
		print(OuterDeclared)  # prints "OuterDeclared"
		print(A)  # prints <GDScript #...>

	static func test_b(b: B) -> void:
		print(B)  # prints <GDScript #...>

It makes the constants of base class available as typing inside the extended classes.

Fixes #69586

[adamscott]

CC @anvilfolk @rune-scape

[anvilfolk]

Generally makes sense - haven't be able to dig deeply.

Is it possible to both have an outer class as well as a base class? If so, both ought to be checked right? Right now it's prioritizing the outer class over the base class.

So something like:

class_name OuterClass

class A:
  const myvar = preload(...)

class B extends A:
  func my_func(p : myvar):
    print(myVar)
    print(A.myVar)

Would this work, or would class B look straight to BaseClass?

[adamscott]

Would this work, or would class B look straight to BaseClass?

Gonna test this at once.

[rune-scape]

i think you'll need 2 loops, one for checking base class, and one for checking each bases outer classes (because you don't want the outer-most script's base, you want the bases base (thats confusing, sorry)) look at reduce_identifier_from_base, the same code exists there, i think this whole search current class thing should really use reduce_identifier_from_base, instead of duplicated code, but thats much more work

[adamscott]

I updated the logic! It now checks for outer classes and their extended classes, without the need of adding one more loop. I prioritized the outer classes first, as this was the logic as for now.

This code now works:

# `base.gd` declares `A`
extends "base.gd"

const OuterDeclared: = "OuterDeclared"

static func test(a: A) -> void:
	print(A)  # prints <GDScript #...>

# `base_inner.gd` declares `B`
class InnerBaseClass extends "base_inner.gd":
	static func test_a(a: A) -> void:
		print(OuterDeclared)  # prints "OuterDeclared"
		print(A)  # prints <GDScript #...>

	static func test_b(b: B) -> void:
		print(B)  # prints <GDScript #...>

[anvilfolk]

Look at these amazing MSPaint skills!

But in all seriousness, inner classes need to check outer classes (blue) as well as base classes (red). I was thinking about this and it isn't really a tree - I think @adamscott is right that we need to worry about checking things twice, and about the possibility of cyclic dependencies.

In that sense, having a has_been_checked set of some sort makes a lot of sense to me.

[adamscott]

I updated the branch. Replaced RBSet with Vector. Added a Vector<GDScriptParser::ClassNode *> checked_script_classes as @anvilfolk suggested.

[trollodel]

Does this supersedes #57510?

[adamscott]

@trollodel Didn't know that there was an existing PR about the same issue. I searched the issue tracker before starting, didn't think about searching the pull requests too. Sorry.

[adamscott]

Does this supersedes #57510?

I would say yes. I just tested your PR and it doesn't pass, unfortunately, my added unit test.

[trollodel]

I would say yes. I just tested your PR and it doesn't pass, unfortunately, my added unit test.

@adamscott ok. I want to test this PR in my personal project, if I'll find some time for that. But your tests are bigger than mine, so it's probably good.

[anvilfolk]

@trollodel it's always frustrating when this happens - so I personally really appreciate everyone's graciousness here <3

There's a couple active GDScript contributors right now, which didn't use to be the case and explains why your PR wasn't reviewed at the time. Please feel free to hop on RocketChat or tag any of us for GDScript PRs.

We've been trying to review each other's PRs and make sure GDScript changes move along, and you'd be very welcome if you wanted to join these efforts again <3

[trollodel]

@anvilfolk Thanks for your words, but I don't feel frustrated (quite the contrary TBH). I use #57510 in my personal fork anyway, so I don't really care when it would be reviewed.

There's a couple active GDScript contributors right now, which didn't use to be the case and explains why your PR wasn't reviewed at the time. Please feel free to hop on RocketChat or tag any of us for GDScript PRs.

We've been trying to review each other's PRs and make sure GDScript changes move along, and you'd be very welcome if you wanted to join these efforts again <3

I'm already in the GDScript channel on RC, but I don't have anything to say or PR stalled now.

[adamscott]

Updated the PR to use lists instead of vectors

[anvilfolk]

I just realized something that I hadn't realized before, so I thought I would make it clear here - especially since I think the PR doesn't fall into this trap! :)

The treatment of outer classes and base classes is meant to be different. You should look for instance members in the base class only. Searching for types, constants, class members, etc, should be done on both base class and outer class, recursively in some smart order.

So here's my understanding of how things are working from a bird's eye view:

resolve_identifier() {
	check_local_scope()
	if (found)
		return;
	else
		resolve_identifier_from_base()
}

resolve_identifier_from_base() {
	while(base_class) {
		check_instance_and_class_members()
		if (found)
			return;
		base_class = base_class.superclass
	}

	while (base_class and/or outer_class) {
		check_class_members_only();
		cleverly_move_to_outer_class_first_then_base_class_recursively()
	}
}

I was worried that this was was doing base&outer class recursion for check_instance_and_class_members(), but nope! Smart PR author is smart! Everything checks out still!!

I feel like it's likely that there is some amount of duplication of work between check_instance_and_class_members() and check_class_members_only(). Right? But if not too many, it's not too worrysome, and can be tackled in a PR further down the line.

Last comment is that I just realized how deeply important the order of cleverly_move_to_outer_class_first_then_base_class_recursively() is if identifiers are reused a lot. The main thing, I think, is for it to be consistent. And for people to know that they can use identifiers that are more fully qualified, e.g. instead of using SOME_ENUM_CONSTANT, they can absolutely use OuterClassOfBaseClass.SOME_ENUM_CONSTANT, which really helps disambiguate things!

[adamscott]

I was worried that this was was doing base&outer class recursion for check_instance_and_class_members(), but nope! Smart PR author is smart! Everything checks out still!!

I'm doing things that I didn't even knew I was doing. :upside_down_face:

[adamscott]

Removed all the GDScriptCache additions to create instead PR #69865.

[adamscott]

@akien-mga The PR doesn't build because I split off the code that made this PR build to #69865.

Once #69865 merged, it should fix the current sanitizers issues.

[akien-mga]

Thanks!

[anvilfolk]

Congraaaats!