Fix constants scope in extended or inner GDScript classes by adamscott · Pull Request #69587 · godotengine/godot

adamscott · 2022-12-04T21:59:45Z

~~This PR depends on #69865~~ Merged, thanks

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 · 2022-12-04T22:00:15Z

CC @anvilfolk @rune-scape

anvilfolk · 2022-12-04T22:17:43Z

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 · 2022-12-04T22:19:31Z

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

Gonna test this at once.

rune-scape · 2022-12-04T22:58:25Z

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 · 2022-12-04T23:20:46Z

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 #...>

modules/gdscript/gdscript_analyzer.cpp

rune-scape · 2022-12-05T00:04:37Z

modules/gdscript/gdscript_analyzer.cpp

i think ideally this would just call reduce_identifier_from_base recursively on outer classes, and check if the passed base is part of the current classes hierarchy to see if it can access its self, but maybe thats just ideally
it would make the logic of searching a tree much simpler

I don't really visualize how to use reduce_identifier_from_base there as you do, if you could show an example.

i think it wouldn't have the information to avoid inf recursion, so looping is probably a better idea

rune-scape · 2022-12-05T00:14:21Z

modules/gdscript/gdscript_analyzer.cpp

i don't know how to request changes, but i'm pretty sure this should be a loop inside the next if statement, collecting this outer class's bases outer classes (the terminology is so confusing), same in resolve_datatype

anvilfolk · 2022-12-05T00:31:55Z

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 · 2022-12-05T00:41:37Z

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

rune-scape · 2022-12-05T01:03:14Z

modules/gdscript/gdscript_analyzer.cpp

this should be moved outside of the base class loop and also collect the base classes of the base

rune-scape · 2022-12-05T01:15:29Z

modules/gdscript/gdscript_analyzer.cpp

i think maybe this and the next loop should be instead something like this:

GDScriptParser::ClassNode *outer_base_class = script_class->base_type.class_type; if (outer_base_class != nullptr) { script_classes.append(outer_base_class); GDScriptParser::ClassNode *outer_base_outer_class = outer_base_class->outer; while (outer_base_outer_class != nullptr) { script_classes.append(outer_base_outer_class ); outer = outer_base_outer_class ->outer; } }

each run is one of the blue circles:

wait no, the bases of each outer need to be searched first...
i think i'm back to thinking recursion is the least complicated way to do this, and just have checked_script_classes as a member of the analyzer or passed into reduce_identifier_from_base

@rune-scape If you want to make a PR with your recursive changes and add me as a co-author, go for it! You seem to have an idea to fix this.

adamscott · 2022-12-05T05:05:43Z

modules/gdscript/gdscript_cache.cpp

For some reason, without this, the tests will result in a heap-use-after-free error with the address sanitizer.

That is so confusing! I don't see why these changes would do that.

Could it be a regression from elsewhere that shows up for you if you've rebased from master?

i'm suspicious of this part, but the rest looks great!

I'll create a new PR for that part, it doesn't belong in this PR.

trollodel · 2022-12-05T07:06:01Z

Does this supersedes #57510?

adamscott · 2022-12-05T09:42:08Z

@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 · 2022-12-05T10:07:23Z

Does this supersedes #57510?

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

modules/gdscript/gdscript_analyzer.cpp

anvilfolk · 2022-12-05T13:52:47Z

modules/gdscript/gdscript_analyzer.cpp

The name implies you are getting all classes that are defined in scripts, but nothing else. Something about the name script_classes makes it a little hard for me to understand what it does. Would it make more sense to say something like get_context_classes or get_outer_and_base_classes or something more in line with its intent? Those names are terrible too :P

If nothing else, perhaps some comments explaining exactly what the function does would be nice for future readers :)

It also seems like it doesn't really check for native/builtin classes? Should it?

I'll name the function get_class_node_current_scope_classes().

And no, no need for native/builtin classes, as checks are already made for those.

I love the name!!!! That's perfect.

And just making sure: native/builtin class checks are already made going all the way up outer-class & extends hierarchy? Because maybe we're looking for an enum, say, somewhere in Object, and you might need to follow up a few GDScript classes before you get there?

trollodel · 2022-12-05T17:00:34Z

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 · 2022-12-05T17:10:22Z

@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 · 2022-12-05T17:37:32Z

@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 · 2022-12-05T19:01:21Z

Updated the PR to use lists instead of vectors

anvilfolk · 2022-12-07T00:20:17Z

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 · 2022-12-08T19:58:44Z

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. 🙃

adamscott · 2022-12-08T20:00:15Z

modules/gdscript/gdscript_analyzer.cpp

Just to be sure @anvilfolk, this prioritizes the node base type over its outer class

Yes, you are right! And it should prioritize all base classes before even the first outer class.

And then it prioritizes the first outer class's base classes (all the way up) before moving on to first outer class's outer class, and so forth.

It gets pretty confusing, but honestly if somebody is looking this deep into the class hierarchy for names that are reused everywhere, they should just refactor their code, hehehe :)

It should also add native/builtin classes and everything from what I can tell! :)

rune-scape · 2022-12-10T00:34:20Z

modules/gdscript/gdscript_cache.cpp

i'm suspicious of this part, but the rest looks great!

adamscott · 2022-12-10T17:59:05Z

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

adamscott · 2022-12-10T18:15:55Z

@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 · 2022-12-10T20:58:57Z

Thanks!

anvilfolk · 2022-12-10T21:07:12Z

Congraaaats!

adamscott requested a review from a team as a code owner December 4, 2022 21:59

Calinou added bug topic:gdscript labels Dec 4, 2022

Calinou added this to the 4.0 milestone Dec 4, 2022

adamscott force-pushed the fix-constant-base-typing-in-extended-class branch from b2ac34a to d09600d Compare December 4, 2022 23:17

adamscott changed the title ~~Fix constant base typing in extended GDScript class~~ Fix constants scope in extended or inner GDScript classes Dec 4, 2022

adamscott force-pushed the fix-constant-base-typing-in-extended-class branch from d09600d to c1c8cb2 Compare December 4, 2022 23:42

rune-scape suggested changes Dec 5, 2022

View reviewed changes

adamscott force-pushed the fix-constant-base-typing-in-extended-class branch from c1c8cb2 to 35eca64 Compare December 5, 2022 00:39

rune-scape reviewed Dec 5, 2022

View reviewed changes

adamscott force-pushed the fix-constant-base-typing-in-extended-class branch 2 times, most recently from 333b5a7 to c112c6b Compare December 5, 2022 05:04

adamscott commented Dec 5, 2022

View reviewed changes

adamscott force-pushed the fix-constant-base-typing-in-extended-class branch 2 times, most recently from 7ae904e to 60ca361 Compare December 5, 2022 05:19

anvilfolk reviewed Dec 5, 2022

View reviewed changes

modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved

anvilfolk reviewed Dec 5, 2022

View reviewed changes

adamscott force-pushed the fix-constant-base-typing-in-extended-class branch from 60ca361 to d41655a Compare December 5, 2022 14:14

trollodel mentioned this pull request Dec 5, 2022

Fix GDScript error when using an inherited const as annotation #57510

Closed

adamscott force-pushed the fix-constant-base-typing-in-extended-class branch 2 times, most recently from 82b2454 to 8a0dd2e Compare December 5, 2022 19:00

adamscott mentioned this pull request Dec 6, 2022

Fix cyclic reference base being loaded but not valid (which is ok) #69259

Merged

anvilfolk mentioned this pull request Dec 6, 2022

GDScript enum fixes & refactor #69590

Merged

adamscott commented Dec 8, 2022

View reviewed changes

adamscott force-pushed the fix-constant-base-typing-in-extended-class branch 2 times, most recently from 8154042 to 35cce21 Compare December 9, 2022 01:30

anvilfolk approved these changes Dec 9, 2022

View reviewed changes

akien-mga requested a review from rune-scape December 9, 2022 15:47

rune-scape approved these changes Dec 10, 2022

View reviewed changes

modules/gdscript/gdscript_cache.cpp Outdated

Copy link
Copy Markdown

Contributor

rune-scape Dec 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm suspicious of this part, but the rest looks great!

adamscott mentioned this pull request Dec 10, 2022

Fix GDScriptCache to not remove scripts/scenes individually when clearing #69865

Merged

adamscott force-pushed the fix-constant-base-typing-in-extended-class branch from 35cce21 to d20b3b1 Compare December 10, 2022 17:57

adamscott force-pushed the fix-constant-base-typing-in-extended-class branch from d20b3b1 to fc5cabb Compare December 10, 2022 18:22

Fix constant base typing in extended GDScript class

65a49ba

adamscott force-pushed the fix-constant-base-typing-in-extended-class branch from fc5cabb to 65a49ba Compare December 10, 2022 18:39

akien-mga approved these changes Dec 10, 2022

View reviewed changes

akien-mga merged commit 8f6f244 into godotengine:master Dec 10, 2022

anvilfolk mentioned this pull request Dec 11, 2022

Inner classes are not recognized as type in extended classes. #69050

Closed

vonagam mentioned this pull request Dec 17, 2022

GDScript 2.0: Wrong lookup for outer things #70216

Closed

Uh oh!

Conversation

adamscott commented Dec 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamscott commented Dec 4, 2022

Uh oh!

anvilfolk commented Dec 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamscott commented Dec 4, 2022

Uh oh!

rune-scape commented Dec 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamscott commented Dec 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamscott Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rune-scape Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anvilfolk commented Dec 5, 2022

Uh oh!

adamscott commented Dec 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rune-scape Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rune-scape Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trollodel commented Dec 5, 2022

Uh oh!

adamscott commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamscott commented Dec 5, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamscott Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trollodel commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anvilfolk commented Dec 5, 2022

Uh oh!

trollodel commented Dec 5, 2022

Uh oh!

adamscott commented Dec 5, 2022

Uh oh!

adamscott commented Dec 4, 2022 •

edited

Loading

anvilfolk commented Dec 4, 2022 •

edited

Loading

rune-scape commented Dec 4, 2022 •

edited

Loading

adamscott commented Dec 4, 2022 •

edited

Loading

adamscott Dec 5, 2022 •

edited

Loading

rune-scape Dec 5, 2022 •

edited

Loading

rune-scape Dec 5, 2022 •

edited

Loading

rune-scape Dec 5, 2022 •

edited

Loading

adamscott commented Dec 5, 2022 •

edited

Loading

adamscott Dec 5, 2022 •

edited

Loading

trollodel commented Dec 5, 2022 •

edited

Loading

anvilfolk commented Dec 7, 2022 •

edited

Loading

adamscott commented Dec 8, 2022 •

edited

Loading