Debugger stops when loading scripts with invalid syntax

[MikeSchulze]

Godot version

v4.2.stable.official [46dc277]

System information

Windows 10

Issue description

I'm the developer of the GdUnit4 plugin and run into a regression issue since update to Godot 4.2.
My plugin provides a tool to mock scripts for testing purpose.
When i load a given script to mock by using GDScript.load(<path>) it produces now runtime errors when the script has some syntax errors.
This breaks now my implementation and the Debugger stops during load on the script error. Before, it was just load the script without checking for syntax/script errors.

The load is now checking in addition for script errors, this is a major regression!
Before I could just load any script also "broken" scripts.

Please roll this change back or provide a flag to enable/disable script syntax checking on load.

Steps to reproduce

res://tests/invaldScript.gd

extends Node

func ready(

var resource = load("res://tests/invaldScript.gd")

The debugger now stops on the invalid script

Minimal reproduction project

n/a

[bitbrain]

We addon developers rely on the old behaviour, as we want to e.g. test via unit tests that loading a broken script does not break anything. However, currently, this just fails the CI build (this did not happen in 4.1 and prior). If it is not possible any longer to revert back to the old behaviour due to architectural reasons, a suggested workaround would be greatly appreciated.

[winston-yallow]

I'm not entirely sure why you would load syntactically broken scripts. Wouldn't just providing an empty GDScript.new() work for mocking?

But as a possible workaround, you could load the source code via the FileAccess API and then create the script object from that:

var script := GDScript.new()
script.source_code = code_from_file
var err := script.reload()

[dalexeev]

The load is now checking in addition for script errors, this is a major regression!

This is not a regression, this is a fix for incorrect behavior. See #78540 and #83540.

Please roll this change back or provide a flag to enable/disable script syntax checking on load.

• See Add a GDScript error checker class #64065.

[KoBeWi]

Related: godotengine/godot-proposals#8548

[AThousandShips]

Note that this is in 4.1 as well, if those PRs are indeed the source of those changes (this also shows the importance of declaring what versions you are talking about when suggesting something is a regression, since unless this is caused by some other PR this behavior is in 4.1.3)

[MikeSchulze]

I'm not entirely sure why you would load syntactically broken scripts. Wouldn't just providing an empty GDScript.new() work for mocking?

But as a possible workaround, you could load the source code via the FileAccess API and then create the script object from that:

var script := GDScript.new()
script.source_code = code_from_file
var err := script.reload()

This now work, I already tried it results in the same problem

[MikeSchulze]

Note that this is in 4.1 as well, if those PRs are indeed the source of those changes (this also shows the importance of declaring what versions you are talking about when suggesting something is a regression, since unless this is caused by some other PR this behavior is in 4.1.3)

There a no issues when using v4.1.3.stable.official [f06b6836a] the load works without the debugger stoped.
This is behavior change in Godot 4.2.

[MikeSchulze]

To bring more light into the issue here, an example project to show the problem.
load_issue.zip

Godot_v4.1.3-stable_win64.exe -d res://TestScene.tscn

D:\develop\workspace\issues\load_issue>Godot Engine v4.1.3.stable.official.f06b6836a - https://godotengine.org
Vulkan API 1.3.242 - Forward+ - Using Vulkan Device #0: NVIDIA - NVIDIA GeForce GTX 980

ERROR: Failed to load script "res://invalidScript.gd" with error "Parse error".
   at: load (modules/gdscript/gdscript.cpp:2705)
script: <GDScript#-9223372010078337895> true

vs

Godot_v4.2-stable_win64.exe -d res://TestScene.tscn

D:\develop\workspace\issues\load_issue>Godot Engine v4.2.stable.official.46dc27791 - https://godotengine.org
Vulkan API 1.3.242 - Forward+ - Using Vulkan Device #0: NVIDIA - NVIDIA GeForce GTX 980


Debugger Break, Reason: 'Parser Error: Expected indented block after function declaration.'
*Frame 0 - res://invalidScript.gd:10 in function ''
Enter "help" for assistance.
debug>


Debugger Break, Reason: 'Parser Error: Expected indented block after function declaration.'
*Frame 0 - res://invalidScript.gd:10 in function ''
Enter "help" for assistance.
debug>

This breaks now the workflow, on Godot 4.2 the program is on hold via debugger break.
As @bitbrain already mentioned, if you're running a test in a CI workflow, this will now break it and the WF will run out of time.
Before the script was loading without a debugger break.

[YuriSizov]

I don't quite understand why you would need to load broken scripts on CI. Unit tests for your project should be testing for issues in your project's logic and behavior. Broken scripts are not that, they won't work because Godot itself won't be able to do anything with them. They will always trigger the debugger at runtime during test runs.

What exactly is it that you're trying to test here?

[bitbrain]

@YuriSizov you can write addons where users specify custom scripts. These scripts can be broken, though. In case they are, the plugin should not crash etc. but handle that gracefully. For this, e.g. I have written tests in my addon and the setup worked fine until 4.2 - I found a workaround though, which is to move the broken scripts outside the folder scanning of the unit test execution scope. While this is fine, other addon devs may stumble upon the same issue.

Having a mechanism to at least toggle off the 'error throwing' of load() somehow would improve this.

[MikeSchulze]

@YuriSizov as @bitbrain already wrote, I have no control what the user is testing.
I only provide a tool to test your implementations, this could also mean test of a corrupted script.
GdUnit4 has his own error handler to scan the log for possible script errors and will notify about.
But with this change it breaks the workflow and I have no chance to continue the processing without manual interactions.

To report script errors during loading are very fine, but it should not end up in a debugger on hold.

[bitbrain]

To add to my previous comment, as I forgot to mention it: Godot's design philosophy usually is to avoid throwing errors whenever possible (this is why e.g. GDScript does not support exceptions as control flow) - instead of crashing, we should always aim to 'continue' whenever possible. The behaviour in Godot 4.1.3 was okay, but looking at the issues linked above I understand why the contract on load was changed.

An alternative to this could be to have a new method called load_or_null that does return null in case it was unable to load something (without errors) - that being said, with 4.2 going forward, tools like gdUnit will be unable to 'carry on' when there are script errors (and they should carry on, as it's a unit testing suite, not the game itself)

[akien-mga]

Godot_v4.2-stable_win64.exe -d res://TestScene.tscn

D:\develop\workspace\issues\load_issue>Godot Engine v4.2.stable.official.46dc27791 - https://godotengine.org
Vulkan API 1.3.242 - Forward+ - Using Vulkan Device #0: NVIDIA - NVIDIA GeForce GTX 980


Debugger Break, Reason: 'Parser Error: Expected indented block after function declaration.'
*Frame 0 - res://invalidScript.gd:10 in function ''
Enter "help" for assistance.
debug>

This breaks now the workflow, on Godot 4.2 the program is on hold via debugger break. As @bitbrain already mentioned, if you're running a test in a CI workflow, this will now break it and the WF will run out of time. Before the script was loading without a debugger break.

Just to be clear, the -d option that you're using stands for:

  -d, --debug                       Debug (local stdout debugger).

So this seems to me like it behaves as intended. There's an error in the project, you asked for a stdout debugger, and you get that.

It seems to me that you don't actually want a stdout debugger to interrupt the CI flow, so you maybe you shouldn't use -d?

I tested and without -d in 4.2 there's no interruption.

$ Godot_v4.2-stable_linux.x86_64 res://TestScene.tscn
Godot Engine v4.2.stable.official.46dc27791 - https://godotengine.org
Vulkan API 1.3.246 - Forward+ - Using Vulkan Device #1: AMD - AMD Radeon RX Vega M GL Graphics (RADV VEGAM)
 
SCRIPT ERROR: Parse Error: Expected indented block after function declaration.
          at: GDScript::reload (res://invalidScript.gd:10)
ERROR: Failed to load script "res://invalidScript.gd" with error "Parse error".
   at: load (modules/gdscript/gdscript.cpp:2775)
script: <GDScript#-9223372010195778363> true

[MikeSchulze]

@akien-mga i now -d is debug.
I need to use debug mode on tests run to get the stack trace to parse error line for failure reporting.
And as I already say it works on all versions before without any problems.

[YuriSizov]

Well, the problem here is that the debug mode is specifically to launch your scripts with a debugger attached. So with the bugs fixed in 4.2 this is now operating as expected and as designed.

What you really need is some form of godotengine/godot-proposals#8548 with a way to retrieve errors after the fact. Something that #85544 is trying to accomplish. You made it work with the debug mode and the old behavior, but your workflow is based on something that wasn't working properly, so now that it is fixed, it doesn't work anymore.

It's unfortunate, but it's always risky to commit to an implementation based on something unreliable. I would suggest opening a proposal for a proper solution in this situation. Not sure much can be done as a workaround for now without compromising on error reporting where it's supposed to work, i.e. under most other circumstances.

[MikeSchulze]

@YuriSizov I never saw in the documentation that the debugger should hold the script while loading if it is invalid.
What bug fix do you mean that should fix this?

Again, I have no problems with the errors showing up on load. That is a great improvement to see such errors.
But why is the debugger now jumping into the script that I just want to load?
Please understand that this is only loading and not executing a script.

In my opinion, this is not correct, a debugger should only intervene in code that is currently being executed.

[adamscott]

Discussing of this issue with the GDScript team in our weekly meeting.

A bisection of the issue would be appreciated, to know if the change that introduced this issue was intentional or not.

[MikeSchulze]

I tested against all 4.2 builds, this issue was introduced in build v4.2.beta3.official.e8d57afae

Godot Engine v4.2.beta2.official.f8818f85e - https://godotengine.org
Vulkan API 1.3.242 - Forward+ - Using Vulkan Device #0: NVIDIA - NVIDIA GeForce GTX 980
 
script: <GDScript#-9223372010128669493> true

v4.2.beta2 the test scene runs and loads the corrupted script without debugger stop.

Godot Engine v4.2.beta3.official.e8d57afae - https://godotengine.org
Vulkan API 1.3.242 - Forward+ - Using Vulkan Device #0: NVIDIA - NVIDIA GeForce GTX 980
 
  res://invalidScript.gd:10 - Parse Error: Expected indented block after function declaration.
  modules/gdscript/gdscript.cpp:2703 - Failed to load script "res://invalidScript.gd" with error "Parse error". (User)
  scene/gui/code_edit.cpp:1704 - Index p_line = 9 is out of bounds (get_line_count() = 9).
  scene/gui/code_edit.cpp:1704 - Index p_line = 9 is out of bounds (get_line_count() = 9).
  scene/gui/text_edit.cpp:5841 - Index p_line = 9 is out of bounds (text.size() = 9).
  scene/gui/text_edit.cpp:5835 - Index p_line = 9 is out of bounds (text.size() = 9).
script: <GDScript#-9223372009138813749> true
--- Debugging process stopped ---

On v4.2.beta3 the test scene runs and stops in the debugger at loading the corrupted script.

[AThousandShips]

That commit is unrelated to this change, it is a particle commit, so must be some other commit

[MikeSchulze]

That commit is unrelated to this change, it is a particle commit, so must be some other commit

I just tested against official builds.

[AThousandShips]

Would need an actual bisect to find it specifically, but this is a good starting point

[MikeSchulze]

Would need an actual bisect to find it specifically, but this is a good starting point

I know, but I never setup a build environment to build the Godot editor from a specific commit.

[akien-mga]

Well in beta 3 it does sound very likely to be #83540 as @dalexeev pointed out earlier. This was presumably cherry-picked in 4.1.3, but apparently doesn't introduce the intended behavior, maybe it relied on earlier changes which weren't cherry-picked.

[adamscott]

There a no issues when using v4.1.3.stable.official [f06b6836a] the load works without the debugger stoped.
This is behavior change in Godot 4.2.

As much I don't want to add salt to the wound, I would highly encourage plugin developers to test their plugins against upcoming beta releases (or at minimum against release candidate releases) in order to prevent such situations.

Nobody benefits from incompatibilities like this one.

[bitbrain]

@adamscott I think it actually might be worth to have a dedicated 'future' branch building against specific godot nightly builds to verify regressions dynamically in addon repos. That is obviously more effort but worth it in the long run. I will discuss this privately with others. Offtopic over.

[MikeSchulze]

There a no issues when using v4.1.3.stable.official [f06b6836a] the load works without the debugger stoped.
This is behavior change in Godot 4.2.

As much I don't want to add salt to the wound, I would highly encourage plugin developers to test their plugins against upcoming beta releases (or at minimum against release candidate releases) in order to prevent such situations.

Nobody benefits from incompatibilities like this one.

I am testing the upcoming versions, as you can see here, it had unfortunately turned out in the past that quite often things are broken. I feel like I have to install a workaround in every major release because things have changed unpredictably. In this case, it's a side issue that I hadn't covered before. Especially the massive problems with the newly introduced caching and the .godot folder speak volumes.