GDScript: Fix out of date errors in depended scripts

[rune-scape]

fixes the extremely annoying bug where a depended scripts interface wont update in the editor, leading to errors that can only be cleared up by restarting the editor
-- removes GDScriptParserRef from the cache when reloading a script to let other scripts that depend on it use that new stuff
-- and many other small changes to fix the weird issues that come with making and breaking circular references

fixes #81292

[vnen]

I feel like this is working around a more fundamental problem with the cache.

Ideally, after the script that is being loaded is compiled, the parser cache should be empty. The only reason the cache exists is to avoid parsing the same script more than once in the same dependency tree, it is not supposed to keep stuff across different scripts being parsed. If it were working as intended, then there would be no need to swap any parser, because it would always have to re-parse the dependencies.

So, I'd like to focus on why the cache is not being cleared properly.

[rune-scape]

Ideally, after the script that is being loaded is compiled, the parser cache should be empty.

one script may trigger a reload on another script, while still needing the parser ref, making this tricky...

but if

it is not supposed to keep stuff across different scripts being parsed.

then each dependancy tree should have its own cache, this would really increase the number of times a script would need to be parsed and analysed

So, I'd like to focus on why the cache is not being cleared properly.

i agree the cache is not in the best state, but it was simply never programmed to do what ur saying it was only programmed to clear the parser ref when a script is removed

ive actually already rewritten the gdscript parser ref cache for a different thing i never made a PR for, i could do it in a few days

id also like to add that a bandaid can go with a structural change,

[vnen]

then each dependancy tree should have its own cache, this would really increase the number of times a script would need to be parsed and analysed

Assuming no parallel loading, only a single script would be loaded at a time. At the end of this script loading, the cache should be empty. That's the ideal scenario, it shouldn't increase the number of parses. With parallel loading it should work too, assuming there's no script being changed in the middle of it.

The bug itself manifest only when changing things in the editor, which should only be (re)loading a single script: the one being edited. Even if it's not, it should be starting with an empty cache anyway, so no outdated information would be available.

I understand that the current cache is quite subpar and needs to be refactored. My worry is that this PR is a bit complex and I don't know if it's going to create other bugs. For instance, all type refences to GDScript types point to a GDScriptParser::ClassNode, so if something suddenly deletes the parser, those references will be dangling. Especially if the cache is not being cleared properly, there might be some old parse tree referring to those stale pointers. I'm not sure if that would actually happen in practice, but it makes me hesitate to approve this change as it is.

[rune-scape]

For instance, all type refences to GDScript types point to a GDScriptParser::ClassNode, so if something suddenly deletes the parser, those references will be dangling

youre right, this is the exact problem i was trying to solve. its why this PR isnt only 5 lines of code

• i moved the GDScriptParserRef references to a member of GDScriptParser, so their lifetimes are tied together, so when the last ref goes out of scope, it really is safe
• there are only 2 times the GDScriptParserRefs are cleared and that is when a GDScript is removed, meaning all references to it are invalid and its ok to delete the cached parser tree, and when the entire cache is being cleared.

there isnt any other object lifetime the GDScriptParser class nodes are tied to
the bug then becomes: if a GDScriptParserRef gets removed from the cache due to a script reload, and another script tries to access the parser of a class node it has and it get the wrong parser (because reload was called somewhere inbetween) and errors out here:
https://github.com/godotengine/godot/blob/0030938098f220e2563c86a1c9f7f6fdcb697644/modules/gdscript/gdscript_analyzer.cpp#L340
and the solution i made was to only remove the parser from the cache when the hash of the source changes, which is very unlikely during dependant script reload
i think even if this isnt approved some of the changes in this PR solve the potential bug ur talking about.
as hard as it was to track down the issues, this was a simple one to solve, theres much more unnecessarily convoluted circular ref counting nightmares in the godot codebase, hopefully those get simplified too

If it were working as intended, then there would be no need to swap any parser

also the parser.swap() is not used for swapping parsers,, its just a little trick to reset the parser and let a copy of it go out of scope and get the tree cleaned up, i was annoyed with how the clear() method seemed completely out of date and wouldn't actually support calling parse() again (which could happen with this PR)

Assuming no parallel loading, only a single script would be loaded at a time. At the end of this script loading, the cache should be empty. That's the ideal scenario, it shouldn't increase the number of parses

when loading an actual game, it would need possibly dozens of scripts, each would have a parser ref cache of its entire dependancy tree, some of the dep trees would be very similar... leading to multiple of the same script being parsed and analyzed for different scripts

it seems like the parser dependancy cache your're talking about would be geared toward multithreaded loading of scripts

[rune-scape]

removed the swap method as it was a confusing way to clear the parser

[vnen]

Okay, I guess I did get confused by the swap method. I'll do a proper review now, perhaps we can have this as a bug fix while we refactor the semantics of the cache.

when loading an actual game, it would need possibly dozens of scripts, each would have a parser ref cache of its entire dependancy tree, some of the dep trees would be very similar... leading to multiple of the same script being parsed and analyzed for different scripts

I see what you mean, but for me that's not really an issue. It might be a "problem" if you are loading many scripts in a short amount of time (I say "problem" in quotes, because I don't think parsing is slow enough to benefit greatly of this). If you load one now and the next one only, say, five minutes later, is it worth keeping all the parse trees in the cache? I do understand that during runtime the scripts don't change the source code, so keeping the parse tree is somewhat beneficial in terms of load time, but I'm not sure if it's worth the extra memory. In the end is another hard problem with cache all over again.

[vnen]

LGTM

[rune-scape]

It might be a "problem" if you are loading many scripts in a short amount of time

i do want to point out that that games will do this on startup .. ive profiled the engine startup on the game im working on and parsing and compiling gdscript is a not insignificant chunk of time already

[akien-mga]

Thanks!

[adamscott]

I bisected a regression when trying to port Dialogic from 4.2 to 4.3 and this commit seems to be the culprit.

Here's the MRP:
reprobug.zip