From 13ab2b6f4f61dbfb4f90c6602f126c247d4c38c5 Mon Sep 17 00:00:00 2001 From: Raul Santos Date: Sun, 9 Jul 2023 23:25:20 +0200 Subject: [PATCH] C#: Improve `GD.PushError` and `GD.PushWarning` - 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. - 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`. - 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. - 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. --- editor/debugger/script_editor_debugger.cpp | 12 +++- .../GodotSharp/Core/DebuggingUtils.cs | 71 +++++++++++++++++-- .../glue/GodotSharp/GodotSharp/Core/GD.cs | 26 +++++-- .../Core/NativeInterop/ExceptionUtils.cs | 2 +- .../Core/NativeInterop/InteropStructs.cs | 9 +++ .../Core/NativeInterop/NativeFuncs.cs | 6 +- modules/mono/glue/runtime_interop.cpp | 21 +++--- 7 files changed, 117 insertions(+), 30 deletions(-) diff --git a/editor/debugger/script_editor_debugger.cpp b/editor/debugger/script_editor_debugger.cpp index 8985387043e8..2e41cd709dd2 100644 --- a/editor/debugger/script_editor_debugger.cpp +++ b/editor/debugger/script_editor_debugger.cpp @@ -526,8 +526,11 @@ void ScriptEditorDebugger::_parse_message(const String &p_msg, const Array &p_da error->set_custom_color(1, color); String error_title; - if (oe.callstack.size() > 0) { - // If available, use the script's stack in the error title. + if (!oe.source_func.is_empty() && source_is_project_file) { + // If source function is inside the project file. + error_title += oe.source_func + ": "; + } else if (oe.callstack.size() > 0) { + // Otherwise, if available, use the script's stack in the error title. error_title = _format_frame_text(&oe.callstack[0]) + ": "; } else if (!oe.source_func.is_empty()) { // Otherwise try to use the C++ source function. @@ -602,7 +605,10 @@ void ScriptEditorDebugger::_parse_message(const String &p_msg, const Array &p_da if (i == 0) { stack_trace->set_text(0, "<" + TTR("Stack Trace") + ">"); stack_trace->set_text_alignment(0, HORIZONTAL_ALIGNMENT_LEFT); - error->set_metadata(0, meta); + if (!source_is_project_file) { + // Only override metadata if the source is not inside the project. + error->set_metadata(0, meta); + } tooltip += TTR("Stack Trace:") + "\n"; } diff --git a/modules/mono/glue/GodotSharp/GodotSharp/Core/DebuggingUtils.cs b/modules/mono/glue/GodotSharp/GodotSharp/Core/DebuggingUtils.cs index c4161d2dedad..57b292793aa1 100644 --- a/modules/mono/glue/GodotSharp/GodotSharp/Core/DebuggingUtils.cs +++ b/modules/mono/glue/GodotSharp/GodotSharp/Core/DebuggingUtils.cs @@ -14,14 +14,46 @@ namespace Godot { private static void AppendTypeName(this StringBuilder sb, Type type) { - if (type.IsPrimitive) - sb.Append(type.Name); - else if (type == typeof(void)) + // Use the C# type keyword for built-in types. + // https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/built-in-types + if (type == typeof(void)) sb.Append("void"); + else if (type == typeof(bool)) + sb.Append("bool"); + else if (type == typeof(byte)) + sb.Append("byte"); + else if (type == typeof(sbyte)) + sb.Append("sbyte"); + else if (type == typeof(char)) + sb.Append("char"); + else if (type == typeof(decimal)) + sb.Append("decimal"); + else if (type == typeof(double)) + sb.Append("double"); + else if (type == typeof(float)) + sb.Append("float"); + else if (type == typeof(int)) + sb.Append("int"); + else if (type == typeof(uint)) + sb.Append("uint"); + else if (type == typeof(nint)) + sb.Append("nint"); + else if (type == typeof(nuint)) + sb.Append("nuint"); + else if (type == typeof(long)) + sb.Append("long"); + else if (type == typeof(ulong)) + sb.Append("ulong"); + else if (type == typeof(short)) + sb.Append("short"); + else if (type == typeof(ushort)) + sb.Append("ushort"); + else if (type == typeof(object)) + sb.Append("object"); + else if (type == typeof(string)) + sb.Append("string"); else sb.Append(type); - - sb.Append(' '); } internal static void InstallTraceListener() @@ -70,13 +102,26 @@ namespace Godot } } + internal static unsafe StackFrame? GetCurrentStackFrame(int skipFrames = 0) + { + // We skip 2 frames: + // The first skipped frame is the current method. + // The second skipped frame is a method in NativeInterop.NativeFuncs. + var stackTrace = new StackTrace(skipFrames: 2 + skipFrames, fNeedFileInfo: true); + return stackTrace.GetFrame(0); + } + [UnmanagedCallersOnly] internal static unsafe void GetCurrentStackInfo(void* destVector) { try { var vector = (godot_stack_info_vector*)destVector; - var stackTrace = new StackTrace(skipFrames: 1, fNeedFileInfo: true); + + // We skip 2 frames: + // The first skipped frame is the current method. + // The second skipped frame is a method in NativeInterop.NativeFuncs. + var stackTrace = new StackTrace(skipFrames: 2, fNeedFileInfo: true); int frameCount = stackTrace.FrameCount; if (frameCount == 0) @@ -87,6 +132,14 @@ namespace Godot int i = 0; foreach (StackFrame frame in stackTrace.GetFrames()) { + var method = frame.GetMethod(); + + if (method is MethodInfo methodInfo && methodInfo.IsDefined(typeof(StackTraceHiddenAttribute))) + { + // Skip methods marked hidden from the stack trace. + continue; + } + string? fileName = frame.GetFileName(); int fileLineNumber = frame.GetFileLineNumber(); @@ -102,6 +155,9 @@ namespace Godot i++; } + + // Resize the vector again in case we skipped some frames. + vector->Resize(i); } catch (Exception e) { @@ -122,7 +178,10 @@ namespace Godot var sb = new StringBuilder(); if (methodBase is MethodInfo methodInfo) + { sb.AppendTypeName(methodInfo.ReturnType); + sb.Append(' '); + } sb.Append(methodBase.DeclaringType?.FullName ?? ""); sb.Append('.'); diff --git a/modules/mono/glue/GodotSharp/GodotSharp/Core/GD.cs b/modules/mono/glue/GodotSharp/GodotSharp/Core/GD.cs index 9425b7424c37..33ebb8171e8a 100644 --- a/modules/mono/glue/GodotSharp/GodotSharp/Core/GD.cs +++ b/modules/mono/glue/GodotSharp/GodotSharp/Core/GD.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Text; using Godot.NativeInterop; @@ -334,6 +335,21 @@ namespace Godot NativeFuncs.godotsharp_printt(godotStr); } + [StackTraceHidden] + private static void ErrPrintError(string message, godot_error_handler_type type = godot_error_handler_type.ERR_HANDLER_ERROR) + { + // Skip 1 frame to avoid current method. + var stackFrame = DebuggingUtils.GetCurrentStackFrame(skipFrames: 1); + string callerFilePath = ProjectSettings.LocalizePath(stackFrame.GetFileName()); + DebuggingUtils.GetStackFrameMethodDecl(stackFrame, out string callerName); + int callerLineNumber = stackFrame.GetFileLineNumber(); + + using godot_string messageStr = Marshaling.ConvertStringToNative(message); + using godot_string callerNameStr = Marshaling.ConvertStringToNative(callerName); + using godot_string callerFilePathStr = Marshaling.ConvertStringToNative(callerFilePath); + NativeFuncs.godotsharp_err_print_error(callerNameStr, callerFilePathStr, callerLineNumber, messageStr, p_type: type); + } + /// /// Pushes an error message to Godot's built-in debugger and to the OS terminal. /// @@ -347,8 +363,7 @@ namespace Godot /// Error message. public static void PushError(string message) { - using var godotStr = Marshaling.ConvertStringToNative(message); - NativeFuncs.godotsharp_pusherror(godotStr); + ErrPrintError(message); } /// @@ -364,7 +379,7 @@ namespace Godot /// Arguments that form the error message. public static void PushError(params object[] what) { - PushError(AppendPrintParams(what)); + ErrPrintError(AppendPrintParams(what)); } /// @@ -378,8 +393,7 @@ namespace Godot /// Warning message. public static void PushWarning(string message) { - using var godotStr = Marshaling.ConvertStringToNative(message); - NativeFuncs.godotsharp_pushwarning(godotStr); + ErrPrintError(message, type: godot_error_handler_type.ERR_HANDLER_WARNING); } /// @@ -393,7 +407,7 @@ namespace Godot /// Arguments that form the warning message. public static void PushWarning(params object[] what) { - PushWarning(AppendPrintParams(what)); + ErrPrintError(AppendPrintParams(what), type: godot_error_handler_type.ERR_HANDLER_WARNING); } /// diff --git a/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/ExceptionUtils.cs b/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/ExceptionUtils.cs index 2d8067d3006a..bb510cee7e60 100644 --- a/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/ExceptionUtils.cs +++ b/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/ExceptionUtils.cs @@ -95,7 +95,7 @@ namespace Godot.NativeInterop } NativeFuncs.godotsharp_internal_script_debugger_send_error(nFunc, nFile, line, - nErrorMsg, nExcMsg, p_warning: godot_bool.False, stackInfoVector); + nErrorMsg, nExcMsg, godot_error_handler_type.ERR_HANDLER_ERROR, stackInfoVector); } } diff --git a/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/InteropStructs.cs b/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/InteropStructs.cs index 43e7c7eb9ac3..23b9228a93b9 100644 --- a/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/InteropStructs.cs +++ b/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/InteropStructs.cs @@ -1133,4 +1133,13 @@ namespace Godot.NativeInterop get => _ptr != null ? *((int*)_ptr - 1) : 0; } } + + [SuppressMessage("ReSharper", "InconsistentNaming")] + public enum godot_error_handler_type + { + ERR_HANDLER_ERROR = 0, + ERR_HANDLER_WARNING, + ERR_HANDLER_SCRIPT, + ERR_HANDLER_SHADER, + } } diff --git a/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/NativeFuncs.cs b/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/NativeFuncs.cs index 3ec3d1e53067..d42ee156577e 100644 --- a/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/NativeFuncs.cs +++ b/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/NativeFuncs.cs @@ -58,7 +58,7 @@ namespace Godot.NativeInterop internal static partial void godotsharp_internal_script_debugger_send_error(in godot_string p_func, in godot_string p_file, int p_line, in godot_string p_err, in godot_string p_descr, - godot_bool p_warning, in DebuggingUtils.godot_stack_info_vector p_stack_info_vector); + godot_error_handler_type p_type, in DebuggingUtils.godot_stack_info_vector p_stack_info_vector); internal static partial godot_bool godotsharp_internal_script_debugger_is_active(); @@ -540,9 +540,7 @@ namespace Godot.NativeInterop internal static partial void godotsharp_var_to_str(in godot_variant p_var, out godot_string r_ret); - internal static partial void godotsharp_pusherror(in godot_string p_str); - - internal static partial void godotsharp_pushwarning(in godot_string p_str); + internal static partial void godotsharp_err_print_error(in godot_string p_function, in godot_string p_file, int p_line, in godot_string p_error, in godot_string p_message = default, godot_bool p_editor_notify = godot_bool.False, godot_error_handler_type p_type = godot_error_handler_type.ERR_HANDLER_ERROR); // Object diff --git a/modules/mono/glue/runtime_interop.cpp b/modules/mono/glue/runtime_interop.cpp index ee4de4e9f5a7..24a9d4030aeb 100644 --- a/modules/mono/glue/runtime_interop.cpp +++ b/modules/mono/glue/runtime_interop.cpp @@ -92,10 +92,10 @@ void godotsharp_stack_info_vector_destroy( void godotsharp_internal_script_debugger_send_error(const String *p_func, const String *p_file, int32_t p_line, const String *p_err, const String *p_descr, - bool p_warning, const Vector *p_stack_info_vector) { + ErrorHandlerType p_type, const Vector *p_stack_info_vector) { const String file = ProjectSettings::get_singleton()->localize_path(p_file->simplify_path()); EngineDebugger::get_script_debugger()->send_error(*p_func, file, p_line, *p_err, *p_descr, - true, p_warning ? ERR_HANDLER_WARNING : ERR_HANDLER_ERROR, *p_stack_info_vector); + true, p_type, *p_stack_info_vector); } bool godotsharp_internal_script_debugger_is_active() { @@ -1320,12 +1320,14 @@ void godotsharp_printraw(const godot_string *p_what) { OS::get_singleton()->print("%s", reinterpret_cast(p_what)->utf8().get_data()); } -void godotsharp_pusherror(const godot_string *p_str) { - ERR_PRINT(*reinterpret_cast(p_str)); -} - -void godotsharp_pushwarning(const godot_string *p_str) { - WARN_PRINT(*reinterpret_cast(p_str)); +void godotsharp_err_print_error(const godot_string *p_function, const godot_string *p_file, int32_t p_line, const godot_string *p_error, const godot_string *p_message, bool p_editor_notify, ErrorHandlerType p_type) { + _err_print_error( + reinterpret_cast(p_function)->utf8().get_data(), + reinterpret_cast(p_file)->utf8().get_data(), + p_line, + reinterpret_cast(p_error)->utf8().get_data(), + reinterpret_cast(p_message)->utf8().get_data(), + p_editor_notify, p_type); } void godotsharp_var_to_str(const godot_variant *p_var, godot_string *r_ret) { @@ -1611,8 +1613,7 @@ static const void *unmanaged_callbacks[]{ (void *)godotsharp_str_to_var, (void *)godotsharp_var_to_bytes, (void *)godotsharp_var_to_str, - (void *)godotsharp_pusherror, - (void *)godotsharp_pushwarning, + (void *)godotsharp_err_print_error, (void *)godotsharp_object_to_string, };