C#: Improve GD.PushError and GD.PushWarning

[raulsntos]

• Use the name, file path and line number of the caller that invokes GD.PushError and GD.PushWarning instead of the location in the C++ runtime_interop.cpp file.
  • See Better error display in debugger panel #71943 (comment).
• Deprecate GD.PushError and GD.PushWarning overloads that take a params parameter, since those can't be used with [Caller] attributes and the reported error location will be the GD.cs file.
• Remove GD.PushError and GD.PushWarning overloads that take a single string parameter. This breaks compatibility but otherwise overload resolution will prefer them over the new overloads, which defeats the purpose of this PR.
  • Interestingly, I'm the one that added those overloads as performance optimization in C#: Sync GD with Core #71946.
  • This breaks binary compatibility but not source compatibility, meaning that it only breaks for users that are calling these methods from a library that was built for 4.0 - 4.1. Users calling these methods directly in their game code will be unaffected.
• Improvements to getting the C# stack trace.
  • Use C# type keywords for built-in types in method declarations.
  • Remove extra space before each parameter in method declarations.
  • Skip one more frame to avoid NativeInterop.NativeFuncs.
    • Fixes In C#, Polling for InputMap action that does not exist produces a difficult to debug error. #79449.
  • Skip methods annotated with the [StackTraceHidden] attribute.
• Improvements to ScriptEditorDebugger when source is in project.
  • Avoid overriding error metadata when the source is inside the project file.
    • Fixes [.NET] Double clicking (unnecessarily long) debug log entry doesn't jumpt to source but to source-generator code #72662.
    • May fix C# Selecting an error makes a "Save Resource As" menu appears when running the project #76415.
  • Use the source function in the title when the source is inside the project file.

Users that use these methods would expect the reported location printed by these methods to correspond to a location in their project source files. Specifically, they'd expect to see the file path and line number at which they call these methods, and not the location of the C++ code (which is always the same). Now, these methods are a lot more useful since users can know which line in their source code printed the error/warning.

4.1.stable	This PR

[raulsntos]

Removed the breaks compat label, since I changed this to avoid breaking compatibility.

[RedworkDE]

If the a stack from is from source generated code attempting to open its source will fail because the source doesn't exist:

ERROR: Cannot open file 'C:/dir/Net7/Godot.SourceGenerators/Godot.SourceGenerators.ScriptMethodsGenerator/Test_Node_ScriptMethods.generated.cs'.
   at: (modules\mono\utils\string_utils.cpp:152)
ERROR: Failed to read file: 'C:/dir/Net7/Godot.SourceGenerators/Godot.SourceGenerators.ScriptMethodsGenerator/Test_Node_ScriptMethods.generated.cs'.
   at: (modules\mono\csharp_script.cpp:2754)
ERROR: Cannot load C# script file 'C:/dir/Net7/Godot.SourceGenerators/Godot.SourceGenerators.ScriptMethodsGenerator/Test_Node_ScriptMethods.generated.cs'.
   at: (modules\mono\csharp_script.cpp:2820)
ERROR: Failed loading resource: C:/dir/Net7/Godot.SourceGenerators/Godot.SourceGenerators.ScriptMethodsGenerator/Test_Node_ScriptMethods.generated.cs. Make sure resources have been imported by opening the project in the editor at least once.
   at: (C:\dir\godot_4.0.1\core/io/resource_loader.cpp:274)

(Same probably happens for libraries where the sources are not available, but paths are embedded in the pdb)
Can we pretend that we don't know about the source file if it doesn't exist and avoid these errors?
Also If you open a frame from a file that does exist but is not part of the project, the editor tries to convince you to save it as part of the project, if you save any script.

Also about the formatting of method signatures: if we want to format them in C# style, reference parameters are formatted as Type& instead of ref Type and inner types as Outer+Inner instead of Outer.Inner

[RedworkDE]

Does this always reliably work or can inlining mean that these frames can potentially not exist? (Idk if such frames are available in C#)

[raulsntos]

That's a really good point, I don't know.

[raulsntos]

Is this still a concern or is this not a problem since it's only going to execute in Debug builds which hopefully disable inlining optimizations?

[raulsntos]

If the a stack from is from source generated code attempting to open its source will fail because the source doesn't exist

There is a check to avoid the source function if it's not part of the project. It only checks the path, is that check not enough?

Can we pretend that we don't know about the source file if it doesn't exist and avoid these errors?

That sounds reasonable, I'm a little worried about how this would affect the script debugger since I think it assumes the entire stack trace contains methods defined in the user project. Maybe we should filter everything that is not part of the user project from the stack trace, but that feels wrong since it may be useful information.

[RedworkDE]

There is a check to avoid the source function if it's not part of the project. It only checks the path, is that check not enough?

What check do you mean?

godot/core/io/resource_loader.cpp

Lines 276 to 279 in 202e4b2

	#ifdef TOOLS_ENABLED
	Ref<FileAccess> file_check = FileAccess::create(FileAccess::ACCESS_RESOURCES);
	ERR_FAIL_COND_V_MSG(!file_check->file_exists(p_path), Ref<Resource>(), "Resource file not found: " + p_path + ".");
	#endif

Could only trigger if the previous errors didn't happen.

I would add a single error to this function: (tho I suppose we could do that in a different PR as well)

godot/editor/debugger/editor_debugger_node.cpp

Lines 142 to 145 in 202e4b2

	void EditorDebuggerNode::_error_selected(const String &p_file, int p_line, int p_debugger) {
	Ref<Script> s = ResourceLoader::load(p_file);
	emit_signal(SNAME("goto_script_line"), s, p_line - 1);
	}

[raulsntos]

What check do you mean?

This one:

godot/editor/debugger/script_editor_debugger.cpp

Line 495 in 13ab2b6

bool source_is_project_file = oe.source_file.begins_with("res://");

godot/editor/debugger/script_editor_debugger.cpp

Lines 608 to 611 in 13ab2b6

	if (!source_is_project_file) {
	// Only override metadata if the source is not inside the project.
	error->set_metadata(0, meta);
	}

The metadata is then used to determine which file to open:

godot/editor/debugger/script_editor_debugger.cpp

Lines 1486 to 1491 in 13ab2b6

	Array meta = selected->get_metadata(0);
	if (meta.size() == 0) {
	return;
	}
	emit_signal(SNAME("error_selected"), String(meta[0]), int(meta[1]));

I would add a single error to this function: (tho I suppose we could do that in a different PR as well)

I'd prefer to do it in a different PR.

[RedworkDE]

Needs #79342 to work correctly on windows, but is still a significant improvement without.

[akien-mga]

Thanks!