Using other scripts by their class_name in an import plugin causes import to fail

[tcoxon]

Godot version

4.1.1-stable

System information

Godot v4.1.1.stable (fabd7c89d) - Arch Linux #1 SMP PREEMPT_DYNAMIC Sat, 10 Jun 2023 00:35:35 +0000 - Vulkan (Forward+) - dedicated NVIDIA GeForce GTX 970 (nvidia; 530.41.03) - AMD Ryzen 5 2600 Six-Core Processor (12 Threads)

Issue description

I have been encountering a strange issue where builds of my project created and exported from a fresh clone of the repo are missing their translations. I've narrowed this down to a weird quirk of import plugin scripts (my project uses a custom editor plugin to import translations). It seems as though global script class names don't exist during the initial import.

The reproducer project contains a CSV file translations/example.csv and its corresponding .import set up to be imported using the "Translation Plus" plugin.

If the two lines at the top of addons/translation_plus/import_plugin.gd that shadow the global class names TranslationSet and TranslationPlus are commented out, and the project's hidden .godot directory is removed, then the next time the project is opened, importing example.csv fails.

Steps to reproduce

1. Download the attached reproducer.
2. Open the project in Godot and notice translation/example.csv is imported as Translation Plus.
3. Open the script addons/translation_plus/import_plugin.gd
4. Comment the two lines (these lines should not be necessary as TranslationSet and TranslationPlus are global script class names):

const TranslationSet = preload("translation_set.gd")
const TranslationPlus = preload("translation_plus.gd")

5. Close the project
6. Erase the .godot directory so that the editor does a clean reimport next time it opens the project.
7. Open the project and observe that example.csv fails to import and instead Godot imports it as CSV Translation. Errors are logged:

res://addons/translation_plus/import_plugin.gd:84 - Parse Error: Identifier "TranslationSet" not declared in the current scope.
  res://addons/translation_plus/import_plugin.gd:84 - Parse Error: Cannot infer the type of "xset" variable because the value doesn't have a set type.
  res://addons/translation_plus/import_plugin.gd:92 - Parse Error: Identifier "TranslationPlus" not declared in the current scope.
  res://addons/translation_plus/import_plugin.gd:92 - Parse Error: Cannot infer the type of "t" variable because the value doesn't have a set type.
  res://addons/translation_plus/import_plugin.gd:96 - Parse Error: Could not find type "TranslationPlus" in the current scope.
  res://addons/translation_plus/import_plugin.gd:98 - Parse Error: Identifier "TranslationPlus" not declared in the current scope.
  res://addons/translation_plus/plugin.gd:7 - Invalid call. Nonexistent function 'new' in base 'GDScript'.
  editor/editor_file_system.cpp:2280 - Condition "!importer.is_valid()" is true. Continuing.

Minimal reproduction project

csv-import-plugin.zip

[dalexeev]

6. Erase the .godot directory so that the editor does a clean reimport next time it opens the project.

After restarting the editor (with filled .godot/global_script_class_cache.cfg), does the bug disappear or not?

As far as I remember, @KoBeWi previously fixed a similar bug with incorrect initialization order.

[tcoxon]

After restarting the editor (with filled .godot/global_script_class_cache.cfg), does the bug disappear or not?

Yes. If instead of completely wiping .godot/ I wipe everything but .godot/global_script_class_cache.cfg, then the csv file imports successfully.

In my case, import is failing in CICD where it builds from clean every time, so it's not like I can just avoid deleting that file.

[donn-xx]

I am on v4.2.beta3.official [e8d57af] and developing plugins and this is still happening. I now regularly 'rm -fr .godot' in order to see what will happen.

The only way I have found to stop the errors is to resort to the const preload thing. Using class_name is fragile.

Even starting the project twice does not negate all the parse errors caused by class_name references.

[isaaccp]

Looking at this briefly (but be aware I have little experience with the Godot source), it looks like the problem is that _enter_tree() is being invoked before the global class cache is populated.

This is logging from a run with extra debug print_line():

Trying to load res://addons/translation_plus/loc.gd
SCRIPT ERROR: Parse Error: Identifier "TranslationSet" not declared in the current scope.
          at: GDScript::reload (res://addons/translation_plus/import_plugin.gd:84)
res://addons/translation_plus/import_plugin.gd:84 - Parse Error: Identifier "TranslationSet" not declared in the current scope.
SCRIPT ERROR: Parse Error: Cannot infer the type of "xset" variable because the value doesn't have a set type.
          at: GDScript::reload (res://addons/translation_plus/import_plugin.gd:84)
...
SCRIPT ERROR: Compile Error: 
          at: GDScript::reload (res://addons/translation_plus/plugin.gd:-1)
res://addons/translation_plus/plugin.gd:-1 - Compile Error: 
Trying to load res://addons/translation_plus/plugin.gd
ERROR: Failed to load script "res://addons/translation_plus/plugin.gd" with error "Compilation failed".
   at: ResourceFormatLoaderGDScript::load (modules\gdscript\gdscript.cpp:2786)
modules\gdscript\gdscript.cpp:2786 - Failed to load script "res://addons/translation_plus/plugin.gd" with error "Compilation failed". (User)
SCRIPT ERROR: Invalid call. Nonexistent function 'new' in base 'GDScript'.
          at: _enter_tree (res://addons/translation_plus/plugin.gd:7)
res://addons/translation_plus/plugin.gd:7 - Invalid call. Nonexistent function 'new' in base 'GDScript'.
Adding global class TranslationPlus
Adding global class TranslationSet
Trying to load res://addons/translation_plus/translation_plus.gd
Trying to load res://addons/translation_plus/translation_set.gd
Invoking save_global_classes

I'll try to see why the ordering is like this and whether it can be changed without breaking something else.

[isaaccp]

Comparing the stack traces for each of the two relevant places. The plugin is loaded from SceneTree::initialize() whereas the global class list is saved for the first time from SceneTree::process(), which is called afterwards.

See attached stack trace screenshots.

[KoBeWi]

Import plugins needs to be loaded early to import assets during the first filesystem scan. If you want to change order, the global class list loading should be moved earlier.

[isaaccp]

Makes sense. From the stack trace, it looks like as of now there is nothing explicitly initializing the global class cache. It just happens to be written the first time a resource is saved as a side-effect.

I'll see if it's possible to trigger the write explicitly before SceneTree::initialize() runs or at least before it invokes _set_tree().

[isaaccp]

Looking at editor_node.cpp there is already an attempt to retry loading addons that fail to load after waiting for the initial file scan, but it doesn't seem to be working for this case. (Note that this still would cause issues for import plugins even if it was working, so probably still would be worth fixing some other way).

Going through it with the debugger, it looks like despite all the errors thrown out, this check passes:

		// Errors in the script cause the base_type to be an empty StringName.
		if (scr->get_instance_base_type() == StringName()) {
			if (_initializing_plugins) {
				// However, if it happens during initialization, waiting for file scan might help.
				pending_addons.push_back(p_addon);
				return;
			}

with get_instance_base_type() returning EditorPlugin as expected. "Errors in the script cause the base_type to be an empty StringName." seems to not be true in this case, likely because the errors are coming from the attempt to preload("import_plugin.gd"), not from "plugin.gd" itself.

As a result, the plugin is considered loaded and not added to pending_addons.

When later, in add_editor_plugin, we try to add this to the tree using add_child(), it fails with:

SCRIPT ERROR: Invalid call. Nonexistent function 'new' in base 'GDScript'.
          at: _enter_tree (res://addons/translation_plus/plugin.gd:7)
res://addons/translation_plus/plugin.gd:7 - Invalid call. Nonexistent function 'new' in base 'GDScript'.

because preloading import_plugin.gd fails.

I am not sure if there is some way to detect those errors thrown by the file being preloaded.

As mentioned above, even if we fixed this, this would still cause problems for import plugins as far as I can tell, so maybe the proper order would be:
a) Attempt to scan everything (this creates global class cache), some resource types may fail if they depend on an import plugin that is not loaded yet
b) Try to enable plugins (this should work now for all plugins)
c) If we enabled any import plugins, retry to import resources that failed to load

I am new to the godot editor so I am not sure if that order has other problems :)

(some screenshots for extra info)

[isaaccp]

It looks like when we load a script, we still consider it loaded even if there are parsing errors:

godot/modules/gdscript/gdscript.cpp

Line 2790 in 80de898

// Don't fail loading because of parsing error.

I think in order to work around that, the check in

godot/editor/editor_node.cpp

Line 3434 in 80de898

// Errors in the script cause the base_type to be an empty StringName.

checks for whether get_instance_base_type() is not empty, assuming that if there is a parse error in the plugin script, it won't be set. The problem is that in this case the error is not in the plugin itself but in the script that it preloads, and thus despite the error (which shows up as a COMPILATION_ERROR within the gdscript loader), get_instance_base_type() is set to EditorPlugin, causing this problem.