[dotnet] [bidi] Expand usability of CallFunctionAsync<TResult> and EvaluateAsync<TResult>

[RenderMichael]

Description

Greatly expands usability of two methods which de-serialize BiDi types to .NET types

Test suite demonstrates the full extent, here is an example

var response = await context.Script.CallFunctionAsync<dynamic>("() => { return { objKey: 'objValue' }; }", false);

Assert.That(response.objKey, Is.EqualTo("objValue"));

DateTime before = DateTime.Now;
var response = await context.Script.CallFunctionAsync<DateTime?>("() => { return { new Date() }; }", false);
DateTime after = DateTime.Now;

Assert.That(response, Is.InRange(before, after));

Motivation and Context

The two methods can be much more usable

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Type Conversion Redundancy The ConvertTo method contains redundant type checking conditions. For example, typeof(TResult) == typeof(object) appears multiple times in different type conversion blocks, which could lead to unexpected behavior depending on execution order. public TResult? ConvertTo<TResult>() { if (this is UndefinedRemoteValue) { throw new BiDiException($"Unable to convert undefined to {typeof(TResult)}"); } if (this is NullRemoteValue) { if (default(TResult) == null) { return default; } throw new BiDiException($"Unable to convert null to non-nullable value type {typeof(TResult)}"); } if (this is BooleanRemoteValue b) { if (typeof(TResult) == typeof(bool) || typeof(TResult) == typeof(bool?) || typeof(TResult) == typeof(object)) { return (TResult)(object)b.Value; } } if (this is DateRemoteValue date) { if (typeof(TResult) == typeof(DateTime) || typeof(TResult) == typeof(DateTime?) || typeof(TResult) == typeof(object)) { return (TResult)(object)DateTime.Parse(date.Value); } if (typeof(TResult) == typeof(DateTimeOffset) || typeof(TResult) == typeof(DateTimeOffset?)) { return (TResult)(object)DateTimeOffset.Parse(date.Value); } if (typeof(TResult) == typeof(string)) { return (TResult)(object)date.Value; } } if (this is NumberRemoteValue number) { if (typeof(TResult) == typeof(double) || typeof(TResult) == typeof(double?) || typeof(TResult) == typeof(object)) { return (TResult)(object)number.Value; } if (typeof(TResult) == typeof(int) || typeof(TResult) == typeof(int?)) { if (double.IsPositiveInfinity(number.Value) || double.IsNegativeInfinity(number.Value) || double.IsNaN(number.Value)) { throw new BiDiException($"Cannot represent {number.Value} as an {nameof(Int32)}"); } return (TResult)(object)(int)number.Value; } if (typeof(TResult) == typeof(float) || typeof(TResult) == typeof(float?)) { return (TResult)(object)(float)number.Value; } if (typeof(TResult) == typeof(BigInteger) || typeof(TResult) == typeof(BigInteger?)) { return (TResult)(object)new BigInteger(number.Value); } } if (this is BigIntRemoteValue bigInt) { var bigNumber = BigInteger.Parse(bigInt.Value); if (typeof(TResult) == typeof(BigInteger) || typeof(TResult) == typeof(BigInteger?) || typeof(TResult) == typeof(object)) { return (TResult)(object)bigNumber; } if (typeof(TResult) == typeof(double) || typeof(TResult) == typeof(double?) || typeof(TResult) == typeof(object)) { return (TResult)(object)(double)bigNumber; } if (typeof(TResult) == typeof(int) || typeof(TResult) == typeof(int?)) { return (TResult)(object)(int)bigNumber; } if (typeof(TResult) == typeof(float) || typeof(TResult) == typeof(float?)) { return (TResult)(object)(float)bigNumber; } } if (this is StringRemoteValue str) { if (typeof(TResult) == typeof(string) || typeof(TResult) == typeof(object)) { return (TResult)(object)str.Value; } } if (this is RegExpRemoteValue regex) { if (typeof(TResult) == typeof(string) || typeof(TResult) == typeof(object)) { if (string.IsNullOrEmpty(regex.Value.Pattern)) { return (TResult)(object)$"/{regex.Value.Pattern}"; } return (TResult)(object)$"/{regex.Value.Pattern}/{regex.Value.Flags}"; } if (typeof(TResult) == typeof(System.Text.RegularExpressions.Regex)) { return (TResult)(object)new System.Text.RegularExpressions.Regex(regex.Value.Pattern, RegExpValue.JavaScriptFlagsToRegexOptions(regex.Value.Flags)); } } if (this is ArrayRemoteValue array) { if (typeof(TResult) == typeof(IReadOnlyList<RemoteValue>) || typeof(TResult) == typeof(IEnumerable<RemoteValue>) || typeof(TResult) == typeof(object)) { return (TResult)(object)array.Value; } if (typeof(TResult) == typeof(RemoteValue[])) { return (TResult)(object)array.Value.ToArray(); } if (typeof(TResult) == typeof(List<RemoteValue>)) { return (TResult)(object)array.Value.ToList(); } } if (this is ObjectRemoteValue obj) { if (typeof(TResult) == typeof(IDictionary<string, RemoteValue>) || typeof(TResult) == typeof(Dictionary<string, RemoteValue>)) { return (TResult)(object)obj.Value.ToDictionary(value => ((StringRemoteValue)value[0]).Value, value => value[1]); } if (typeof(TResult) == typeof(IDictionary<RemoteValue, RemoteValue>) || typeof(TResult) == typeof(Dictionary<RemoteValue, RemoteValue>)) { return (TResult)(object)obj.Value.ToDictionary(value => value[0], value => value[1]); } if (typeof(TResult) == typeof(object)) { // Handle dynamic here IDictionary<string, object?> values = new ExpandoObject(); foreach (IReadOnlyList<RemoteValue> property in obj.Value) { string propertyName = ((StringRemoteValue)property[0]).Value; values[propertyName] = property[1].ConvertTo<dynamic>(); } return (TResult)(object)values; } } if (this is MapRemoteValue map) { if (typeof(TResult) == typeof(IDictionary<RemoteValue, RemoteValue>) || typeof(TResult) == typeof(Dictionary<RemoteValue, RemoteValue>)) { return (TResult)(object)map.Value.ToDictionary(value => value[0], value => value[1]); } if (typeof(TResult) == typeof(IDictionary<string, RemoteValue>) || typeof(TResult) == typeof(Dictionary<string, RemoteValue>)) { return (TResult)(object)map.Value.ToDictionary(value => ((StringRemoteValue)value[0]).Value, value => value[1]); } } if (this is SetRemoteValue set) { if (typeof(TResult) == typeof(IReadOnlyList<RemoteValue>) || typeof(TResult) == typeof(IEnumerable<RemoteValue>) || typeof(TResult) == typeof(object)) { return (TResult)(object)set.Value; } if (typeof(TResult) == typeof(RemoteValue[])) { return (TResult)(object)set.Value.ToArray(); } if (typeof(TResult) == typeof(List<RemoteValue>)) { return (TResult)(object)set.Value.ToList(); } if (typeof(TResult) == typeof(ISet<RemoteValue>) || #if NET8_0_OR_GREATER typeof(TResult) == typeof(IReadOnlySet<RemoteValue>) || #endif typeof(TResult) == typeof(HashSet<RemoteValue>)) { return (TResult)(object)new HashSet<RemoteValue>(set.Value); } } throw new BiDiException($"Cannot convert to type {typeof(TResult)} from {this}"); Exception Handling The error messages in BiDiException throws could be more specific. For example, when converting undefined or handling unsupported conversions, the error messages could include more context about the specific conversion that failed. { throw new BiDiException($"Unable to convert undefined to {typeof(TResult)}"); } if (this is NullRemoteValue) { if (default(TResult) == null) { return default; } throw new BiDiException($"Unable to convert null to non-nullable value type {typeof(TResult)}");

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add numeric overflow checks Add overflow checks when converting BigInteger to smaller numeric types like int or float. Currently, the code directly casts BigInteger to int/float without checking if the value exceeds the target type's range, which could lead to data loss or incorrect values. dotnet/src/webdriver/BiDi/Modules/Script/RemoteValue.cs [159-186] if (this is BigIntRemoteValue bigInt) { var bigNumber = BigInteger.Parse(bigInt.Value); if (typeof(TResult) == typeof(BigInteger) || typeof(TResult) == typeof(BigInteger?) || typeof(TResult) == typeof(object)) { return (TResult)(object)bigNumber; } if (typeof(TResult) == typeof(double) || typeof(TResult) == typeof(double?) || typeof(TResult) == typeof(object)) { return (TResult)(object)(double)bigNumber; } if (typeof(TResult) == typeof(int) || typeof(TResult) == typeof(int?)) { + if (bigNumber > int.MaxValue || bigNumber < int.MinValue) + { + throw new BiDiException($"BigInteger value {bigNumber} is outside the range of Int32"); + } return (TResult)(object)(int)bigNumber; } if (typeof(TResult) == typeof(float) || typeof(TResult) == typeof(float?)) { + if (bigNumber > float.MaxValue || bigNumber < float.MinValue) + { + throw new BiDiException($"BigInteger value {bigNumber} is outside the range of Single"); + } return (TResult)(object)(float)bigNumber; } } Apply this suggestion Suggestion importance[1-10]: 8 __ Why: This suggestion adds important overflow checks when converting BigInteger values to smaller numeric types, preventing potential data loss and runtime errors. This is a significant improvement to the code's robustness and error handling.	Medium
Learned best practice	Remove unnecessary null-forgiving operator after null check to improve code safety The code uses a null-forgiving operator (!) on flags after checking if it's null or empty. This is unnecessary and potentially unsafe. Since you've already verified that flags is not null or empty, you don't need the null-forgiving operator. Simply use flags directly. dotnet/src/webdriver/BiDi/Modules/Script/RegExpValue.cs [37] -foreach (char flag in flags!) +foreach (char flag in flags) Apply this suggestion Suggestion importance[1-10]: 6	Low
General	Log unsupported regex flags Add a warning or log message when encountering unsupported JavaScript regex flags. This would help developers understand why certain regex behaviors might differ between JavaScript and .NET. dotnet/src/webdriver/BiDi/Modules/Script/RegExpValue.cs [28-58] internal static RegexOptions JavaScriptFlagsToRegexOptions(string? flags) { if (string.IsNullOrEmpty(flags)) { return RegexOptions.None; } RegexOptions value = default; foreach (char flag in flags!) { if (flag == 'i') { value |= RegexOptions.IgnoreCase; } else if (flag == 'm') { value |= RegexOptions.Multiline; } else if (flag == 's') { value |= RegexOptions.Singleline; } else { - // Unsupported flag + // Log or trace unsupported flag for debugging + System.Diagnostics.Debug.WriteLine($"Unsupported JavaScript RegExp flag: {flag}"); } } return value; } Apply this suggestion Suggestion importance[1-10]: 4 __ Why: This suggestion adds debug logging for unsupported regex flags, which improves diagnostics and developer experience. While helpful for troubleshooting, it's a minor enhancement that doesn't address a critical issue or bug.	Low
Update

[RenderMichael]

Per conversation on Slack:

AOT-safe?

Yes, this is AOT-safe. It doesn't root any types that wouldn't certainly be rooted in else where, and patterns like typeof(T) == typeof(int) are special-cased in the compiler to trim away. So ConvertTo<int> will be very optimized.