godot

godot copied to clipboard

Implements out of order member resolution -- this was allowed in some specific places, but this PR pulls it out into its own function to make the behavior consistent -- and allow more complex ordering

class members are now resolved just before they are needed -- This means class members will always have resolved types before used, else an error is pushed same goes for members of external classes -- This fixes alot of dependency errors.

also fixes a few minor bugs: -- singleton datatypes now get properly resolved in extends -- Object and Variant no longer allow array syntax (Variant[int] doesn't parse) -- pushes an error if a native enum doesn't exist when resolving a type those were all just missing a line or 2

fixes #68069 fixes #69479 fixes #67085

Does this address https://github.com/godotengine/godot/issues/61159?

@MewPurPur i can check in a bit, but my instinct is no

As is, this PR fixes #69479. I just tested it. @rune-scape Can you add it in the description?

@anvilfolk and @adamscott thank you for reviewing!

Technically this could be expanded to allow the analyzer to resolve individual members of other scripts, allowing member dependencies both ways between script interfaces, but i'm not sure anyone has a use for that 🙂

Technically this could be expanded to allow the analyzer to resolve individual members of other scripts, allowing member dependencies both ways between script interfaces, but i'm not sure anyone has a use for that 🙂

Wouldn't this actually be really neat now that cyclic dependencies are possible? Could probably come up with a small interesting case like:

# A.gd

var a := B.b_func()

static func a_func() -> int:
  return 42


# B.gd

static func b_func() -> int:
  return A.a_func()

Something like that?

@anvilfolk exactly! i do kinda wanna see how difficult it is, so i'll put it on my todo list 😄 who knows, maybe its dead simple

i added braces inside resolve_class_member() to make the diff much more readable

@MewPurPur no, this doesn't fix https://github.com/godotengine/godot/issues/61159, but i found the bug. https://github.com/godotengine/godot/pull/69518 fixes that

I cannot NOT underline the great teamwork between you both, @rune-scape and @anvilfolk. <3

Technically this could be expanded to allow the analyzer to resolve individual members of other scripts, allowing member dependencies both ways between script interfaces, but i'm not sure anyone has a use for that slightly_smiling_face

Wouldn't this actually be really neat now that cyclic dependencies are possible? Could probably come up with a small interesting case like:

# A.gd

var a := B.b_func()

static func a_func() -> int:
  return 42


# B.gd

static func b_func() -> int:
  return A.a_func()

Something like that?

Here's what I added in the GDScript tests with #67714:

https://github.com/godotengine/godot/blob/5704055d30499cc63672d44001760a98abfbfc08/modules/gdscript/tests/scripts/analyzer/features/preload_cyclic_reference.gd#L1-L4

https://github.com/godotengine/godot/blob/6f1d4fd8871155efcb29d115a7168879948e1cf3/modules/gdscript/tests/scripts/analyzer/features/preload_cyclic_reference_a.notest.gd#L1-L12

https://github.com/godotengine/godot/blob/6f1d4fd8871155efcb29d115a7168879948e1cf3/modules/gdscript/tests/scripts/analyzer/features/preload_cyclic_reference_b.notest.gd#L1-L10

oh, thats right, the status is set to resolved Before it gets resolved, so that does already work, huh looks like it was dead simple!

On that note, I think it'd be really neat to add a unit test showcasing something that wasn't possible before but is now possible, so that out-of-order member resolution is never lost from this point forward :)

All in all, I think this is an excellent PR! Really appreciate @rune-scape and their patience in letting me pick at their code and generally being an annoying little .... :)

After being stuck in line-by-line code, I just want to double check that I understand the big picture:

Instead of resolving functions first, then class members, and having a specific order in which things are allowed to depend on one another, this PR basically does "on demand" resolution. If a function, variable, subclass, whatever, called member depends on something else, you immediately attempt to resolve it.

If, while resolving one of those dependencies you find that one of the things you are attempting to resolve is RESOLVING, i.e., in the process of being resolved as well, then you have cyclic types and are unable to resolve from then on and you throw an error.

However, if you managed to resolve this big long line of type dependency all the way to the end (probably at builtin/native types or GDScript classes with no further dependencies?), then everything along that line, including the original member, has been resolved. You're likely to eventually try to re-resolve some of those (e.g. the for loop for members in resolve_class_interface/body) but every resolve function has a check on whether something's already been resolved, and you can skip doing that work again. No harm no foul.

Ultimately, it passes every unit test, fixes a couple of issues, and makes GDScript slightly more flexible. The code looks lovely, needing only a couple teeny tiny optional tweaks. With the addition of a unit test showcasing GDScript's new capabilities, I'd say merge merge merge :)

This was a lovely opportunity to learn more GDScript internals - thanks @rune-scape <3

Congrats for your PR, @rune-scape! That's really huge!!

I second the idea to create unit tests though! A great way to make sure we don't mess up your work in the future. 🙃

OH MY. Just saw your differential: +676 −367 *jaw drop*

@anvilfolk Yes! Exactly! i am working on unit tests, and also now that @adamscott pointed out that dependencies between members in scripts can already exist, im working on resolving individual members of other scripts, for better error messages

letting me pick at their code and generally being an annoying little .... :)

thank you for picking at my code! it's better now :)

@rune-scape Don't forget to rebase your branch with master, just to be sure to test your code with the latest merges.

I ran @KoBeWi's huge project as a test for your PR. Here's an error that I got.

extends Control

@onready var total_progress := $TextureRect/TextureProgress as TextureProgressBar
@onready var scene_progress := $TextureRect/TextureProgress2 as TextureProgressBar

Error at (3, 32): Cannot infer the type of "total_progress" variable because the initial value is "null"

Seems that it doesn't like casting of nodes.

I really do think that there ought to be a big project that peeps working on GDScript should test against. I don't know whether @KoBeWi's project is public, but I know @unfa works liblast, which is open-source, so maybe that's the way to go?

I ran @KoBeWi's huge project as a test for your PR. Here's an error that I got.

Seems that it doesn't like casting of nodes.

Here's the MRP: 69471_oomr_infer.zip

@anvilfolk Sure, we can use Liblast too to test!

@adamscott thanks for that info, i'm about to push a fix for that

Wanna test? Be my guest!

Here's instructions for cloning the Liblast repo (some people have trouble because they forget about LFS): https://codeberg.org/Liblast/Liblast#_how_to_edit_the_game

@rune-scape It fixed KoBeWi's project! Runs flawlessly, from what I could tell. Thanks!

there's currently nothing crazy this PR allows, mostly because there's still limitations on resolution order (mostly between scripts), but i'm looking at the changes needed to lift most of them and make dependencies between scripts do what you expect (or at least what i expect) so the tests will mostly just show small bugfixes (when i get around to them)

@rune-scape got another test for you that's giving me trouble.

class_name OuterClass

class InnerClass:
	enum MyEnum { V0, V2, V1 }
	
func test(e : InnerClass.MyEnum) -> void:
	print(e)

Does this work in this PR? The issue is that function parameters call reduce_datatype rather than reduce_identifier or reduce_identifier_from_base.

@anvilfolk yes, that resolves fine in this branch

Needs rebase.

i added a single tiny test, but https://github.com/godotengine/godot/pull/69907 is needed to test dependencies between scripts, and a couple more type resolution bugs are also making my life hard 😢 i also took the opportunity to improve my code :)

Looks like the PR you mentioned is in! :) I take it merging should wait until the bugs are ironed out?