-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[wasm][debugger] Fix debugger behavior when the type has ToString method overridden #76780
Conversation
…the result of the ToString and not the type name.
Tagging subscribers to this area: @thaystg Issue DetailsThe description of an object that has ToString implemented should bethe result of the ToString and not the type name. Fixes #76748
|
src/mono/wasm/debugger/BrowserDebugProxy/JObjectValueCreator.cs
Outdated
Show resolved
Hide resolved
Implement support for value types with ToString overriden. Fix Debugger* is more relevant than ToString
if (typeInfo.Name == "object") | ||
return null; | ||
Microsoft.WebAssembly.Diagnostics.MethodInfo methodInfo = typeInfo.Info.Methods.FirstOrDefault(m => m.Name == "ToString"); | ||
if (!IsEnum && methodInfo == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be ||
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this method exit early in case of IsEnum==true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No should be &&
, if it's a enum we want to call ToString even if we cannot find the method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't get the method in case of an enum? Is that the case for other valuetypes? record struct
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only for enum, I added tests for record also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are no methods returned for enums at all? Is that a debugger-agent thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No methods returned for enums at all, not sure if it's a debugger-agent thing or if they are really not implemented in the enum, they are somewhere like in a "base class".
src/mono/wasm/debugger/BrowserDebugProxy/JObjectValueCreator.cs
Outdated
Show resolved
Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/JObjectValueCreator.cs
Outdated
Show resolved
Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/JObjectValueCreator.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Ankit Jain <radical@gmail.com>
…t on objects and valuetypes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the minor comments, LGTM 👍
src/mono/wasm/debugger/BrowserDebugProxy/JObjectValueCreator.cs
Outdated
Show resolved
Hide resolved
@@ -1839,6 +1839,43 @@ public Task<JObject> InvokeMethod(DotnetObjectId dotnetObjectId, CancellationTok | |||
: throw new ArgumentException($"Cannot invoke method with id {methodId} on {dotnetObjectId}", nameof(dotnetObjectId)); | |||
} | |||
|
|||
public async Task<string> InvokeToStringAsync(List<int> typeIds, bool isValueType, bool isEnum, int objectId, BindingFlags extraFlags, CancellationToken token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extraFlags
could have BindingFlags.Default
as the default value.
Co-authored-by: Ankit Jain <radical@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, good job👍
foreach (var methodId in methodIds) | ||
{ | ||
var methodInfoFromRuntime = await GetMethodInfo(methodId, token); | ||
if (methodInfoFromRuntime.Info.GetParametersInfo().Length > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MethodInfoWithDebugInformation
implements GetParametersInfo()
, so we could shorten:
if (methodInfoFromRuntime.Info.GetParametersInfo().Length > 0) | |
if (methodInfoFromRuntime.GetParametersInfo().Length > 0) |
The description of an object that has ToString implemented should bethe result of the ToString and not the type name.
Fixes #76748
Using this PR: