GDScript: Fix common mismatched external parser errors (second try)

[rune-scape]

retry of #94617

i rewrote the part that crashed, as it wasn't very readable or safe

collection of every mrp that exposes an edge case so far: external-parser-edgecases.zip

with these changes, on my machine, the project loads with no errors (after letting godot import everything)

added tests too

fixes #94244

[vnen]

Looks okay to me. Tested with the repro project and it works. Also nice to have some tests.

[mihe]

I'm seeing project-breaking regressions with this PR applied, unfortunately, in the form of Parse Error: Parser bug: Could not find external parser. (Trying to resolve class member "..." of "...").

I'll see if I can reduce it down to an MRP, but these ones are likely going to be a lot tougher to untangle than the regression from #94617.

[mihe]

Thankfully it wasn't as difficult as I thought to extract this, so here's an MRP that reproduces the error I mentioned above: external-parser-error.zip

The MRP can probably be reduced further, but this mimics the relationships in the actual project. You'll find the error in script_a.gd.

Given the new MRP that shows the bug still exists

[vnen]

The error does make sense, as this code is looking for depended parsers, but there's nothing actually adding those scripts to the list.

An inclusion like this on top of this PR would solve this issue in particular:

diff --git a/modules/gdscript/gdscript_analyzer.cpp b/modules/gdscript/gdscript_analyzer.cpp
index f46e392632..1ca326a891 100644
--- a/modules/gdscript/gdscript_analyzer.cpp
+++ b/modules/gdscript/gdscript_analyzer.cpp
@@ -3704,6 +3704,15 @@ Ref<GDScriptParserRef> GDScriptAnalyzer::ensure_cached_parser_for_class(const GD
                external_class_parser_cache.insert(p_class, E->value);
                parser_ref = E->value;
            }
+       } else {
+           Ref<GDScriptParserRef> temp = parser->get_depended_parser_for(script_path);
+           if (temp.is_valid()) {
+               temp->raise_status(GDScriptParserRef::PARSED);
+               if (temp->get_parser()->has_class(p_class)) {
+                   external_class_parser_cache.insert(p_class, temp);
+                   parser_ref = temp;
+               }
+           }
        }
    }

Although a proper fix might want to load it the depended parser earlier.

Also, I'm not sure how this would interact with with parser refs being invalidated on reload.

[mihe]

I'm not seeing any more errors after applying the patch that @vnen provided above.

[akien-mga]

So I'd like to release 4.3.rc2 tomorrow, ideally including this fix. So we have two options:

1. Amend in @vnen's workaround
2. Wait for @rune-scape to amend the PR with their own fix if they'd take a different approach

The second option is obviously preferred but I can't presume of rune's availability on a short notice. If you see this let us know which option you prefer, otherwise I'd go with adding vnen's workaround later today (8-9pm UTC) before starting a build.

[adamscott]

So I'd like to release 4.3.rc2 tomorrow, ideally including this fix. So we have two options:

1. Amend in @vnen's workaround

2. Wait for @rune-scape to amend the PR with their own fix if they'd take a different approach

The second option is obviously preferred but I can't presume of rune's availability on a short notice. If you see this let us know which option you prefer, otherwise I'd go with adding vnen's workaround later today (8-9pm UTC) before starting a build.

What about option 3: Another PR that includes @rune-scape's work as co-author?

[akien-mga]

What about option 3: Another PR that includes @rune-scape's work as co-author?

That's the same as (1) with just a formal difference, not really what I'm interested in at this stage. I can force push the PR to amend it with vnen's change and merge in a minute. If we merged another PR we'd close this one so it's the exact same outcome.

[vnen]

If we go for (1) let's add this instead, which is essentially the same thing but less verbose:

diff --git a/modules/gdscript/gdscript_analyzer.cpp b/modules/gdscript/gdscript_analyzer.cpp
index f46e392632..2412d877a8 100644
--- a/modules/gdscript/gdscript_analyzer.cpp
+++ b/modules/gdscript/gdscript_analyzer.cpp
@@ -3697,6 +3697,7 @@ Ref<GDScriptParserRef> GDScriptAnalyzer::ensure_cached_parser_for_class(const GD
            }
        }
    } else {
+       parser->get_depended_parser_for(script_path);
        if (HashMap<String, Ref<GDScriptParserRef>>::Iterator E = parser->depended_parsers.find(script_path)) {
            // Silently ensure it's parsed.
            E->value->raise_status(GDScriptParserRef::PARSED);

To add to what I said before about doing this earlier in the code path, for what I see it should be done right before calling the ensure_cached_parser_for_class() function, so we might as well do it inside.

[rune-scape]

i have a fix that seems to work

the issue in my code was that it was not calling ensure_cached_parser_for_class in get_class_node_current_scope_classes

that method should ideally be dynamically calling resolve_class_inheritance as the classes are searched through which would cache the class parsers, but it doesn't right now

[rune-scape]

i also fixed a bug where it only pretended to look at base class, which may have also solved it but i didn't test
+added a search through the external dependant parser's class parser cache,
+made the local search for the external parser a fallback option
+added a test case for this

interestingly the test case seems to trigger a runtime error when 1 line is uncommented, but its only testing the analyzer, so it should be fine for now

edit: also updated mrp with the code from #94871 (comment) and my test cases

[mihe]

@rune-scape This latest iteration (rune-scape/godot@f8a066b0c) results in:

Parse Error: Parser bug: Could not find external parser. (Trying to fetch classes in the current scope: base class of "...")

Do you want me to try to reduce that down to an MRP as well?

[rune-scape]

ach! yes please, thank you so much for the MRPs ❤️

[mihe]

@rune-scape Here's an MRP for this latest error: external-parser-error-f8a066b0c.zip

[rune-scape]

@mihe thank you, that MRP pointed out that my function wasn't searching everywhere it needed to

+updated collection of mrp, and updated tests

[mihe]

@rune-scape The latest iteration (rune-scape/godot@6e8fa6dd5) seems to work without errors, at least in this particular (large) project that I'm working with.

[akien-mga]

Thanks @rune-scape for the quick turnaround and everyone involved in testing!

Let's give this another spin for 4.3.rc2 :)

[akien-mga]

Thanks!

[riazey]

Just a heads up the plugin Kanban Tasks -Todo Manager 2.0 by HolonProductions also throws a parser mismatch error (available on the assetLib) as of 4.3.beta3 & RC1- Was gonna wait until rc2 to check but wanted to let ya know in case!

[rune-scape]

@riazey :3 i no longer get that error after this pr when downloading from the asset library and enabling it then restarting the editor :]