Skip to content

Commit

Permalink
[wasm][debugger] Implement support for null-conditional operators in …
Browse files Browse the repository at this point in the history 
…simple expressions (#69307)

* Basic testcase.

* Fixed non-null access to proxy-hidden members.

* Fixing the issue and adding more tests.

* Fixed a failing test: let CompileAndRunTheExpression throw.

* Blocked failing tests on firefox.

* Applied @radical's suggestions.

* Reverted indexing to fix tests.

* Reverted LastOrDefault instead of indexing + corrected names.

* Changed to throw when root name empty.

* Removing null suppression that does not change the evaluation result but fails without this change.

* Applied @radical's comments.

* Change exception on string property evaluation.

* Added @radical's suggestions.

Co-authored-by: Ankit Jain <radical@gmail.com>
  • Loading branch information
@ilonatommy @radical
ilonatommy and radical authored Jun 8, 2022
1 parent fc8ce9b commit 2c110b6
Show file tree
Hide file tree
Showing 4 changed files with 158 additions and 50 deletions.
101 changes: 64 additions & 37 deletions src/mono/wasm/debugger/BrowserDebugProxy/MemberReferenceResolver.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,18 +77,17 @@ public async Task<JObject> GetValueFromObject(JToken objRet, CancellationToken t
return null;
}

public async Task<(JObject containerObject, string remaining)> ResolveStaticMembersInStaticTypes(string varName, CancellationToken token)
public async Task<(JObject containerObject, ArraySegment<string> remaining)> ResolveStaticMembersInStaticTypes(ArraySegment<string> parts, CancellationToken token)
{
string classNameToFind = "";
string[] parts = varName.Split(".", StringSplitOptions.TrimEntries);
var store = await proxy.LoadStore(sessionId, token);
var methodInfo = context.CallStack.FirstOrDefault(s => s.Id == scopeId)?.Method?.Info;

if (methodInfo == null)
return (null, null);

int typeId = -1;
for (int i = 0; i < parts.Length; i++)
for (int i = 0; i < parts.Count; i++)
{
string part = parts[i];

Expand All @@ -97,9 +96,9 @@ public async Task<JObject> GetValueFromObject(JToken objRet, CancellationToken t
JObject memberObject = await FindStaticMemberInType(classNameToFind, part, typeId);
if (memberObject != null)
{
string remaining = null;
if (i < parts.Length - 1)
remaining = string.Join('.', parts[(i + 1)..]);
ArraySegment<string> remaining = null;
if (i < parts.Count - 1)
remaining = parts[i..];

return (memberObject, remaining);
}
Expand Down Expand Up @@ -177,6 +176,10 @@ async Task<int> FindStaticTypeId(string typeName)
// Checks Locals, followed by `this`
public async Task<JObject> Resolve(string varName, CancellationToken token)
{
// question mark at the end of expression is invalid
if (varName[^1] == '?')
throw new ReturnAsErrorException($"Expected expression.", "ReferenceError");

//has method calls
if (varName.Contains('('))
return null;
Expand All @@ -187,18 +190,19 @@ public async Task<JObject> Resolve(string varName, CancellationToken token)
if (scopeCache.ObjectFields.TryGetValue(varName, out JObject valueRet))
return await GetValueFromObject(valueRet, token);

string[] parts = varName.Split(".");
if (parts.Length == 0)
return null;
string[] parts = varName.Split(".", StringSplitOptions.TrimEntries);
if (parts.Length == 0 || string.IsNullOrEmpty(parts[0]))
throw new ReturnAsErrorException($"Failed to resolve expression: {varName}", "ReferenceError");

JObject retObject = await ResolveAsLocalOrThisMember(parts[0]);
bool throwOnNullReference = parts[0][^1] != '?';
if (retObject != null && parts.Length > 1)
retObject = await ResolveAsInstanceMember(string.Join('.', parts[1..]), retObject);
retObject = await ResolveAsInstanceMember(parts, retObject, throwOnNullReference);

if (retObject == null)
{
(retObject, string remaining) = await ResolveStaticMembersInStaticTypes(varName, token);
if (!string.IsNullOrEmpty(remaining))
(retObject, ArraySegment<string> remaining) = await ResolveStaticMembersInStaticTypes(parts, token);
if (remaining != null && remaining.Count != 0)
{
if (retObject.IsNullValuedObject())
{
Expand All @@ -207,7 +211,7 @@ public async Task<JObject> Resolve(string varName, CancellationToken token)
}
else
{
retObject = await ResolveAsInstanceMember(remaining, retObject);
retObject = await ResolveAsInstanceMember(remaining, retObject, throwOnNullReference);
}
}
}
Expand All @@ -217,7 +221,6 @@ public async Task<JObject> Resolve(string varName, CancellationToken token)

async Task<JObject> ResolveAsLocalOrThisMember(string name)
{
var nameTrimmed = name.Trim();
if (scopeCache.Locals.Count == 0 && !localsFetched)
{
Result scope_res = await proxy.GetScopeProperties(sessionId, scopeId, token);
Expand All @@ -226,7 +229,11 @@ async Task<JObject> ResolveAsLocalOrThisMember(string name)
localsFetched = true;
}

if (scopeCache.Locals.TryGetValue(nameTrimmed, out JObject obj))
// remove null-condition, otherwise TryGet by name fails
if (name[^1] == '?' || name[^1] == '!')
name = name.Remove(name.Length - 1);

if (scopeCache.Locals.TryGetValue(name, out JObject obj))
return obj["value"]?.Value<JObject>();

if (!scopeCache.Locals.TryGetValue("this", out JObject objThis))
Expand All @@ -242,54 +249,74 @@ async Task<JObject> ResolveAsLocalOrThisMember(string name)
return null;
}

JToken objRet = valueOrError.Value.FirstOrDefault(objPropAttr => objPropAttr["name"].Value<string>() == nameTrimmed);
JToken objRet = valueOrError.Value.FirstOrDefault(objPropAttr => objPropAttr["name"].Value<string>() == name);
if (objRet != null)
return await GetValueFromObject(objRet, token);

return null;
}

async Task<JObject> ResolveAsInstanceMember(string expr, JObject baseObject)
async Task<JObject> ResolveAsInstanceMember(ArraySegment<string> parts, JObject baseObject, bool throwOnNullReference)
{
JObject resolvedObject = baseObject;
string[] parts = expr.Split('.');
for (int i = 0; i < parts.Length; i++)
// parts[0] - name of baseObject
for (int i = 1; i < parts.Count; i++)
{
string partTrimmed = parts[i].Trim();
if (partTrimmed.Length == 0)
string part = parts[i];
if (part.Length == 0)
return null;

bool hasCurrentPartNullCondition = part[^1] == '?';

// current value of resolvedObject is on parts[i - 1]
if (resolvedObject.IsNullValuedObject())
{
// trying null.$member
if (throwOnNullReference)
throw new ReturnAsErrorException($"Expression threw NullReferenceException trying to access \"{part}\" on a null-valued object.", "ReferenceError");

if (i == parts.Count - 1)
{
// this is not ideal, it returns the last object
// that had objectId and was null-valued,
// so the class/description of object are not of the last part
return resolvedObject;
}

// check if null condition is correctly applied: should we throw or return null-object
throwOnNullReference = !hasCurrentPartNullCondition;
continue;
}

if (!DotnetObjectId.TryParse(resolvedObject?["objectId"]?.Value<string>(), out DotnetObjectId objectId))
return null;
{
if (resolvedObject["type"].Value<string>() == "string")
throw new ReturnAsErrorException($"String properties evaluation is not supported yet.", "ReferenceError"); // Issue #66823
if (!throwOnNullReference)
throw new ReturnAsErrorException($"Operation '?' not allowed on primitive type - '{parts[i - 1]}'", "ReferenceError");
throw new ReturnAsErrorException($"Cannot find member '{part}' on a primitive type", "ReferenceError");
}

ValueOrError<GetMembersResult> valueOrError = await proxy.RuntimeGetObjectMembers(sessionId, objectId, null, token);
var args = JObject.FromObject(new { forDebuggerDisplayAttribute = true });
ValueOrError<GetMembersResult> valueOrError = await proxy.RuntimeGetObjectMembers(sessionId, objectId, args, token);
if (valueOrError.IsError)
{
logger.LogDebug($"ResolveAsInstanceMember failed with : {valueOrError.Error}");
return null;
}

JToken objRet = valueOrError.Value.FirstOrDefault(objPropAttr => objPropAttr["name"]?.Value<string>() == partTrimmed);
if (part[^1] == '!' || part[^1] == '?')
part = part.Remove(part.Length - 1);

JToken objRet = valueOrError.Value.FirstOrDefault(objPropAttr => objPropAttr["name"]?.Value<string>() == part);
if (objRet == null)
return null;

resolvedObject = await GetValueFromObject(objRet, token);
if (resolvedObject == null)
return null;

if (resolvedObject.IsNullValuedObject())
{
if (i < parts.Length - 1)
{
// there is some parts remaining, and can't
// do null.$remaining
return null;
}

return resolvedObject;
}
throwOnNullReference = !hasCurrentPartNullCondition;
}

return resolvedObject;
}
}
Expand Down
12 changes: 6 additions & 6 deletions src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -756,14 +756,14 @@ internal async Task<ValueOrError<GetMembersResult>> RuntimeGetObjectMembers(Sess
GetObjectCommandOptions getObjectOptions = GetObjectCommandOptions.WithProperties;
if (args != null)
{
if (args["accessorPropertiesOnly"] != null && args["accessorPropertiesOnly"].Value<bool>())
{
if (args["accessorPropertiesOnly"]?.Value<bool>() == true)
getObjectOptions |= GetObjectCommandOptions.AccessorPropertiesOnly;
}
if (args["ownProperties"] != null && args["ownProperties"].Value<bool>())
{

if (args["ownProperties"]?.Value<bool>() == true)
getObjectOptions |= GetObjectCommandOptions.OwnProperties;
}

if (args["forDebuggerDisplayAttribute"]?.Value<bool>() == true)
getObjectOptions |= GetObjectCommandOptions.ForDebuggerDisplayAttribute;
}
try
{
Expand Down
69 changes: 63 additions & 6 deletions src/mono/wasm/debugger/DebuggerTestSuite/EvaluateOnCallFrameTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -507,13 +507,13 @@ public async Task EvaluateSimpleMethodCallsError() => await CheckInspectLocalsAt
var (_, res) = await EvaluateOnCallFrame(id, "this.objToTest.MyMethodWrong()", expect_ok: false );
Assert.Equal(
$"Method 'MyMethodWrong' not found in type 'DebuggerTests.EvaluateMethodTestsClass.ParmToTest'",
$"Method 'MyMethodWrong' not found in type 'DebuggerTests.EvaluateMethodTestsClass.ParmToTest'",
res.Error["result"]?["description"]?.Value<string>());
(_, res) = await EvaluateOnCallFrame(id, "this.objToTest.MyMethod(1)", expect_ok: false);
Assert.Equal(
"Unable to evaluate method 'MyMethod'. Too many arguments passed.",
res.Error["result"]?["description"]?.Value<string>());
Assert.Equal(
"Unable to evaluate method 'MyMethod'. Too many arguments passed.",
res.Error["result"]?["description"]?.Value<string>());
(_, res) = await EvaluateOnCallFrame(id, "this.CallMethodWithParm(\"1\")", expect_ok: false );
Assert.Contains("Unable to evaluate method 'this.CallMethodWithParm(\"1\")'", res.Error["message"]?.Value<string>());
Expand All @@ -523,7 +523,7 @@ public async Task EvaluateSimpleMethodCallsError() => await CheckInspectLocalsAt
(_, res) = await EvaluateOnCallFrame(id, "this.ParmToTestObjException.MyMethod()", expect_ok: false );
Assert.Contains(
"Cannot evaluate '(this.ParmToTestObjException.MyMethod()\n)'",
"Cannot evaluate '(this.ParmToTestObjException.MyMethod()\n)'",
res.Error["result"]?["description"]?.Value<string>());
});

Expand Down Expand Up @@ -1147,8 +1147,65 @@ await EvaluateOnCallFrameAndCheck(id,
);
});

[ConditionalFact(nameof(RunningOnChrome))]
public async Task EvaluateNullObjectPropertiesPositive() => await CheckInspectLocalsAtBreakpointSite(
$"DebuggerTests.EvaluateNullableProperties", "Evaluate", 6, "Evaluate",
$"window.setTimeout(function() {{ invoke_static_method ('[debugger-test] DebuggerTests.EvaluateNullableProperties:Evaluate'); 1 }})",
wait_for_event_fn: async (pause_location) =>
{
var id = pause_location["callFrames"][0]["callFrameId"].Value<string>();
// we have no way of returning int? for null values,
// so we return the last non-null class name
await EvaluateOnCallFrameAndCheck(id,
("list.Count", TNumber(1)),
("list!.Count", TNumber(1)),
("list?.Count", TNumber(1)),
("listNull", TObject("System.Collections.Generic.List<int>", is_null: true)),
("listNull?.Count", TObject("System.Collections.Generic.List<int>", is_null: true)),
("tc?.MemberList?.Count", TNumber(2)),
("tc!.MemberList?.Count", TNumber(2)),
("tc!.MemberList!.Count", TNumber(2)),
("tc?.MemberListNull?.Count", TObject("System.Collections.Generic.List<int>", is_null: true)),
("tc.MemberListNull?.Count", TObject("System.Collections.Generic.List<int>", is_null: true)),
("tcNull?.MemberListNull?.Count", TObject("DebuggerTests.EvaluateNullableProperties.TestClass", is_null: true)));
});

[ConditionalFact(nameof(RunningOnChrome))]
public async Task EvaluateNullObjectPropertiesNegative() => await CheckInspectLocalsAtBreakpointSite(
$"DebuggerTests.EvaluateNullableProperties", "Evaluate", 6, "Evaluate",
$"window.setTimeout(function() {{ invoke_static_method ('[debugger-test] DebuggerTests.EvaluateNullableProperties:Evaluate'); 1 }})",
wait_for_event_fn: async (pause_location) =>
{
var id = pause_location["callFrames"][0]["callFrameId"].Value<string>();
await CheckEvaluateFail("list.Count.x", "Cannot find member 'x' on a primitive type");
await CheckEvaluateFail("listNull.Count", GetNullReferenceErrorOn("\"Count\""));
await CheckEvaluateFail("listNull!.Count", GetNullReferenceErrorOn("\"Count\""));
await CheckEvaluateFail("tcNull.MemberListNull.Count", GetNullReferenceErrorOn("\"MemberListNull\""));
await CheckEvaluateFail("tc.MemberListNull.Count", GetNullReferenceErrorOn("\"Count\""));
await CheckEvaluateFail("tcNull?.MemberListNull.Count", GetNullReferenceErrorOn("\"Count\""));
await CheckEvaluateFail("listNull?.Count.NonExistingProperty", GetNullReferenceErrorOn("\"NonExistingProperty\""));
await CheckEvaluateFail("tc?.MemberListNull! .Count", GetNullReferenceErrorOn("\"Count\""));
await CheckEvaluateFail("tc?. MemberListNull!.Count", GetNullReferenceErrorOn("\"Count\""));
await CheckEvaluateFail("tc?.MemberListNull.Count", GetNullReferenceErrorOn("\"Count\""));
await CheckEvaluateFail("tc! .MemberListNull!.Count", GetNullReferenceErrorOn("\"Count\""));
await CheckEvaluateFail("tc!.MemberListNull. Count", GetNullReferenceErrorOn("\"Count\""));
await CheckEvaluateFail("tcNull?.Sibling.MemberListNull?.Count", GetNullReferenceErrorOn("\"MemberListNull?\""));
await CheckEvaluateFail("listNull?", "Expected expression.");
await CheckEvaluateFail("listNull!.Count", GetNullReferenceErrorOn("\"Count\""));
await CheckEvaluateFail("x?.p", "Operation '?' not allowed on primitive type - 'x?'");
string GetNullReferenceErrorOn(string name) => $"Expression threw NullReferenceException trying to access {name} on a null-valued object.";
async Task CheckEvaluateFail(string expr, string message)
{
(_, Result _res) = await EvaluateOnCallFrame(id, expr, expect_ok: false);
AssertEqual(message, _res.Error["result"]?["description"]?.Value<string>(), $"Expression '{expr}' - wrong error message");
}
});

[Fact]
public async Task EvaluateMethodsOnPrimitiveTypesReturningPrimitives() => await CheckInspectLocalsAtBreakpointSite(
public async Task EvaluateMethodsOnPrimitiveTypesReturningPrimitives() => await CheckInspectLocalsAtBreakpointSite(
"DebuggerTests.PrimitiveTypeMethods", "Evaluate", 11, "Evaluate",
"window.setTimeout(function() { invoke_static_method ('[debugger-test] DebuggerTests.PrimitiveTypeMethods:Evaluate'); })",
wait_for_event_fn: async (pause_location) =>
Expand Down
26 changes: 25 additions & 1 deletion src/mono/wasm/debugger/tests/debugger-test/debugger-evaluate-test.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1371,7 +1371,7 @@ public static void Evaluate()
var test = new TestClass();
}
}

public static class PrimitiveTypeMethods
{
public class TestClass
Expand Down Expand Up @@ -1401,6 +1401,30 @@ public static void Evaluate()
string localString = "S*T*R";
}
}

public static class EvaluateNullableProperties
{
class TestClass
{
public List<int> MemberListNull = null;
public List<int> MemberList = new List<int>() {1, 2};
public TestClass Sibling { get; set; }
}
static void Evaluate()
{
#nullable enable
List<int>? listNull = null;
#nullable disable
List<int> list = new List<int>() {1};
TestClass tc = new TestClass();
TestClass tcNull = null;
string str = "str#value";
string str_null = null;
int x = 5;
int? x_null = null;
int? x_val = x;
}
}
}

namespace DebuggerTestsV2
Expand Down

0 comments on commit 2c110b6

Please sign in to comment.