C#: Print error when MethodBind/Callable call fails

[raulsntos]

• Implement same behavior for handling CallError as in GDScript VM (except it doesn't do debug_break, just printing the error for now since the debugger functions are not implemented in C# yet).
  • Fixes [Mono] Node.Call() silently fails #59830.
• Add missing member to CallError enum.
  • Member was added to Core in Add a const call mode to Object, Variant and Script. #62468.

[RedworkDE]

Additionally there seem to be a whole bunch of issues related to the error itself, but they are probably largely resolved by #79280

[raulsntos]

Additionally there seem to be a whole bunch of issues related to the error itself, but they are probably largely resolved by #79280

I'm not sure what issues you are referring to. If they are existing issues then yeah that would be unrelated to this PR. Or are you saying that this PR introduces new issues?

[RedworkDE]

I was referring to the errors that pop up when clicking a from of source generated code, which are really visible with this PR as the top stack frame of the error is such a frame, and when you click it you are immediately greeted with a bunch of error popup. So no new issue from this PR but actually not already fixed either.

[RedworkDE]

Even with #79280 and #79342 the error location seems a bit confused:

Works great then otherwise, with no more errors by the editor.

[raulsntos]

Even with #79280 and #79342 the error location seems a bit confused

We should probably add [StackTraceHidden] to the DebugCheckCallError methods, like I do in #79280 for ErrPrintError. I'll do it here even though it's weird because in this PR it won't do anything.

[RedworkDE]

We should probably add [StackTraceHidden] to the DebugCheckCallError methods, like I do in #79280 for ErrPrintError. I'll do it here even though it's weird because in this PR it won't do anything.

This doesn't really help (at least for me), unless the stack trace hidden is added pretty much everywhere. Instead I think it is easier to add a PushError overload that allows skipping a few stack frames:

diff --git a/modules/mono/glue/GodotSharp/GodotSharp/Core/GD.cs b/modules/mono/glue/GodotSharp/GodotSharp/Core/GD.cs
index 33ebb8171e..83f674989e 100644
--- a/modules/mono/glue/GodotSharp/GodotSharp/Core/GD.cs
+++ b/modules/mono/glue/GodotSharp/GodotSharp/Core/GD.cs
@@ -336,10 +336,10 @@ namespace Godot
         }
 
         [StackTraceHidden]
-        private static void ErrPrintError(string message, godot_error_handler_type type = godot_error_handler_type.ERR_HANDLER_ERROR)
+        private static void ErrPrintError(string message, godot_error_handler_type type = godot_error_handler_type.ERR_HANDLER_ERROR, int skipFrames = 0)
         {
             // Skip 1 frame to avoid current method.
-            var stackFrame = DebuggingUtils.GetCurrentStackFrame(skipFrames: 1);
+            var stackFrame = DebuggingUtils.GetCurrentStackFrame(skipFrames: skipFrames + 1);
             string callerFilePath = ProjectSettings.LocalizePath(stackFrame.GetFileName());
             DebuggingUtils.GetStackFrameMethodDecl(stackFrame, out string callerName);
             int callerLineNumber = stackFrame.GetFileLineNumber();
@@ -366,6 +366,11 @@ namespace Godot
             ErrPrintError(message);
         }
 
+        internal static void PushErrorWithSkippedFrames(string message, int skipFrames)
+        {
+            ErrPrintError(message, skipFrames: skipFrames);
+        }
+
         /// <summary>
         /// Pushes an error message to Godot's built-in debugger and to the OS terminal.
         ///
diff --git a/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/ExceptionUtils.cs b/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/ExceptionUtils.cs
index 28bdc25524..b9e49c9285 100644
--- a/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/ExceptionUtils.cs
+++ b/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/ExceptionUtils.cs
@@ -136,7 +136,7 @@ namespace Godot.NativeInterop
             }
         }
 
-        [Conditional("DEBUG"), StackTraceHidden]
+        [Conditional("DEBUG")]
         public unsafe static void DebugCheckCallError(godot_string_name method, IntPtr instance, godot_variant** args, int argCount, godot_variant_call_error error)
         {
             if (error.Error != godot_variant_call_error_error.GODOT_CALL_ERROR_CALL_OK)
@@ -144,11 +144,11 @@ namespace Godot.NativeInterop
                 using godot_variant instanceVariant = VariantUtils.CreateFromGodotObjectPtr(instance);
                 string where = GetCallErrorWhere(method, &instanceVariant, args, argCount);
                 string errorText = GetCallErrorMessage(error, where, args);
-                GD.PushError(errorText);
+                GD.PushErrorWithSkippedFrames(errorText, 3);
             }
         }
 
-        [Conditional("DEBUG"), StackTraceHidden]
+        [Conditional("DEBUG")]
         public unsafe static void DebugCheckCallError(in godot_callable callable, godot_variant** args, int argCount, godot_variant_call_error error)
         {
             if (error.Error != godot_variant_call_error_error.GODOT_CALL_ERROR_CALL_OK)
@@ -156,7 +156,7 @@ namespace Godot.NativeInterop
                 using godot_variant callableVariant = VariantUtils.CreateFromCallableTakingOwnershipOfDisposableValue(callable);
                 string where = $"callable '{VariantUtils.ConvertToString(callableVariant)}'";
                 string errorText = GetCallErrorMessage(error, where, args);
-                GD.PushError(errorText);
+                GD.PushErrorWithSkippedFrames(errorText, 3);
             }
         }

(while this somewhat reintroduces the stack frame count & inlining concerns, after some thinking about it this doesn't apply to the editor as such optimizations are disabled for assemblies built in debug mode)

[raulsntos]

This doesn't really help (at least for me), unless the stack trace hidden is added pretty much everywhere.

Yes, it feels like we'd end up having to add it all throughout GodotSharp at this point. Which is why I suggested in the other PR (#79280) that maybe we should just filter every frame until we reach the first method defined in the game project assembly.

Anyway, this is kind of unrelated to the purpose of this PR which was only to make sure the Call methods would not fail silently, so I decided to undo that change and leave that issue to the other PR (#79280).

[RedworkDE]

Does what it supposed to, there are some minor issues that are largely resolved by other pending PRs.

[akien-mga]

Thanks!