From 70bf6ab57d6c1a3096eb298bf1d8474ed4c1d82f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Wed, 8 Oct 2025 11:16:48 +0200 Subject: [PATCH 1/4] Fix double evaluation --- .../TestFramework/Assertions/Assert.That.cs | 352 ++++++++++++++---- .../Assertions/AssertTests.That.cs | 25 +- 2 files changed, 312 insertions(+), 65 deletions(-) diff --git a/src/TestFramework/TestFramework/Assertions/Assert.That.cs b/src/TestFramework/TestFramework/Assertions/Assert.That.cs index 99d1e3a882..06a4ecf1ed 100644 --- a/src/TestFramework/TestFramework/Assertions/Assert.That.cs +++ b/src/TestFramework/TestFramework/Assertions/Assert.That.cs @@ -24,6 +24,7 @@ public static partial class AssertExtensions /// The source code of the condition expression. This parameter is automatically populated by the compiler. /// Thrown if is . /// Thrown if the evaluated condition is . + [RequiresDynamicCode("Calls Microsoft.VisualStudio.TestTools.UnitTesting.AssertExtensions.EvaluateExpression(Expression, Dictionary)")] public static void That(Expression> condition, string? message = null, [CallerArgumentExpression(nameof(condition))] string? conditionExpression = null) { if (condition == null) @@ -31,7 +32,12 @@ public static void That(Expression> condition, string? message = null throw new ArgumentNullException(nameof(condition)); } - if (condition.Compile()()) + // Cache to store evaluated expression values to avoid re-evaluation + var evaluationCache = new Dictionary(); + + bool result = EvaluateExpression(condition.Body, evaluationCache); + + if (result) { return; } @@ -45,7 +51,7 @@ public static void That(Expression> condition, string? message = null sb.AppendLine(string.Format(CultureInfo.InvariantCulture, FrameworkMessages.AssertThatMessageFormat, message)); } - string details = ExtractDetails(condition.Body); + string details = ExtractDetails(condition.Body, evaluationCache); if (!string.IsNullOrWhiteSpace(details)) { sb.AppendLine(FrameworkMessages.AssertThatDetailsPrefix); @@ -56,10 +62,191 @@ public static void That(Expression> condition, string? message = null } } - private static string ExtractDetails(Expression expr) + [RequiresDynamicCode("Calls Microsoft.VisualStudio.TestTools.UnitTesting.AssertExtensions.EvaluateAllSubExpressions(Expression, Dictionary)")] + private static bool EvaluateExpression(Expression expr, Dictionary cache) + { + // Use a single-pass evaluation that only evaluates each expression once + EvaluateAllSubExpressions(expr, cache); + + // The root expression should now be cached + if (cache.TryGetValue(expr, out object? result)) + { + return (bool)result!; + } + + // Fallback - this should not happen if EvaluateAllSubExpressions works correctly + return false; + } + + [RequiresDynamicCode("Calls System.Linq.Expressions.Expression.Lambda(Expression, params ParameterExpression[])")] + private static void EvaluateAllSubExpressions(Expression expr, Dictionary cache) + { + // If already evaluated, skip + if (cache.ContainsKey(expr)) + { + return; + } + + try + { + // First, recursively evaluate all sub-expressions + switch (expr) + { + case BinaryExpression binaryExpr: + EvaluateAllSubExpressions(binaryExpr.Left, cache); + EvaluateAllSubExpressions(binaryExpr.Right, cache); + break; + + case UnaryExpression unaryExpr: + EvaluateAllSubExpressions(unaryExpr.Operand, cache); + break; + + case MemberExpression memberExpr: + if (memberExpr.Expression is not null) + { + EvaluateAllSubExpressions(memberExpr.Expression, cache); + } + + break; + + case MethodCallExpression callExpr: + if (callExpr.Object is not null) + { + EvaluateAllSubExpressions(callExpr.Object, cache); + } + + foreach (Expression argument in callExpr.Arguments) + { + EvaluateAllSubExpressions(argument, cache); + } + + break; + + case ConditionalExpression conditionalExpr: + EvaluateAllSubExpressions(conditionalExpr.Test, cache); + EvaluateAllSubExpressions(conditionalExpr.IfTrue, cache); + EvaluateAllSubExpressions(conditionalExpr.IfFalse, cache); + break; + + case InvocationExpression invocationExpr: + EvaluateAllSubExpressions(invocationExpr.Expression, cache); + foreach (Expression argument in invocationExpr.Arguments) + { + EvaluateAllSubExpressions(argument, cache); + } + + break; + + case NewExpression newExpr: + foreach (Expression argument in newExpr.Arguments) + { + EvaluateAllSubExpressions(argument, cache); + } + + break; + + case ListInitExpression listInitExpr: + EvaluateAllSubExpressions(listInitExpr.NewExpression, cache); + foreach (ElementInit initializer in listInitExpr.Initializers) + { + foreach (Expression argument in initializer.Arguments) + { + EvaluateAllSubExpressions(argument, cache); + } + } + + break; + + case NewArrayExpression newArrayExpr: + foreach (Expression expression in newArrayExpr.Expressions) + { + EvaluateAllSubExpressions(expression, cache); + } + + break; + + case TypeBinaryExpression typeBinaryExpr: + EvaluateAllSubExpressions(typeBinaryExpr.Expression, cache); + break; + } + + // Now build a new expression that replaces sub-expressions with their cached values + // This prevents re-execution of side effects + Expression replacedExpr = ReplaceSubExpressionsWithConstants(expr, cache); + + // Evaluate the replaced expression - this won't cause side effects since + // all sub-expressions are now constants + object? result = Expression.Lambda(replacedExpr).Compile().DynamicInvoke(); + cache[expr] = result; + } + catch + { + cache[expr] = ""; + } + } + + private static Expression ReplaceSubExpressionsWithConstants(Expression expr, Dictionary cache) + { + // If this expression's direct sub-expressions are cached, replace them with constants + switch (expr) + { + case BinaryExpression binaryExpr: + Expression left = cache.TryGetValue(binaryExpr.Left, out object? leftValue) + ? Expression.Constant(leftValue, binaryExpr.Left.Type) + : binaryExpr.Left; + Expression right = cache.TryGetValue(binaryExpr.Right, out object? rightValue) + ? Expression.Constant(rightValue, binaryExpr.Right.Type) + : binaryExpr.Right; + return Expression.MakeBinary(binaryExpr.NodeType, left, right); + + case UnaryExpression unaryExpr: + Expression operand = cache.TryGetValue(unaryExpr.Operand, out object? value) + ? Expression.Constant(value, unaryExpr.Operand.Type) + : unaryExpr.Operand; + return Expression.MakeUnary(unaryExpr.NodeType, operand, unaryExpr.Type); + + case MemberExpression memberExpr when memberExpr.Expression is not null && cache.ContainsKey(memberExpr.Expression): + Expression instance = Expression.Constant(cache[memberExpr.Expression], memberExpr.Expression.Type); + return Expression.MakeMemberAccess(instance, memberExpr.Member); + + case MethodCallExpression callExpr: + Expression? obj = callExpr.Object is not null && cache.TryGetValue(callExpr.Object, out object? callExprValue) + ? Expression.Constant(callExprValue, callExpr.Object.Type) + : callExpr.Object; + + Expression[] args = [.. callExpr.Arguments + .Select(arg => cache.TryGetValue(arg, out object? value) + ? Expression.Constant(value, arg.Type) + : arg)]; + + return Expression.Call(obj, callExpr.Method, args); + + case ConditionalExpression conditionalExpr: + Expression test = cache.TryGetValue(conditionalExpr.Test, out object? testValue) + ? Expression.Constant(testValue, conditionalExpr.Test.Type) + : conditionalExpr.Test; + Expression ifTrue = cache.TryGetValue(conditionalExpr.IfTrue, out object? ifTrueValue) + ? Expression.Constant(ifTrueValue, conditionalExpr.IfTrue.Type) + : conditionalExpr.IfTrue; + Expression ifFalse = cache.TryGetValue(conditionalExpr.IfFalse, out object? ifFalseValue) + ? Expression.Constant(ifFalseValue, conditionalExpr.IfFalse.Type) + : conditionalExpr.IfFalse; + return Expression.Condition(test, ifTrue, ifFalse); + + case TypeBinaryExpression typeBinaryExpr when cache.ContainsKey(typeBinaryExpr.Expression): + Expression typeExpr = Expression.Constant(cache[typeBinaryExpr.Expression], typeBinaryExpr.Expression.Type); + return Expression.TypeIs(typeExpr, typeBinaryExpr.TypeOperand); + + default: + // For other expressions or leaf nodes, return as-is + return expr; + } + } + + private static string ExtractDetails(Expression expr, Dictionary evaluationCache) { var details = new Dictionary(); - ExtractVariablesFromExpression(expr, details); + ExtractVariablesFromExpression(expr, details, evaluationCache); if (details.Count == 0) { @@ -82,7 +269,7 @@ private static string ExtractDetails(Expression expr) return sb.ToString(); } - private static void ExtractVariablesFromExpression(Expression? expr, Dictionary details, bool suppressIntermediateValues = false) + private static void ExtractVariablesFromExpression(Expression? expr, Dictionary details, Dictionary evaluationCache, bool suppressIntermediateValues = false) { if (expr is null) { @@ -93,55 +280,55 @@ private static void ExtractVariablesFromExpression(Expression? expr, Dictionary< { // Special handling for array indexing (myArray[index]) case BinaryExpression binaryExpr when binaryExpr.NodeType == ExpressionType.ArrayIndex: - HandleArrayIndexExpression(binaryExpr, details); + HandleArrayIndexExpression(binaryExpr, details, evaluationCache); break; case BinaryExpression binaryExpr: - ExtractVariablesFromExpression(binaryExpr.Left, details, suppressIntermediateValues); - ExtractVariablesFromExpression(binaryExpr.Right, details, suppressIntermediateValues); + ExtractVariablesFromExpression(binaryExpr.Left, details, evaluationCache, suppressIntermediateValues); + ExtractVariablesFromExpression(binaryExpr.Right, details, evaluationCache, suppressIntermediateValues); break; case TypeBinaryExpression typeBinaryExpr: // Extract variables from the expression being tested (e.g., 'obj' in 'obj is int') - ExtractVariablesFromExpression(typeBinaryExpr.Expression, details, suppressIntermediateValues); + ExtractVariablesFromExpression(typeBinaryExpr.Expression, details, evaluationCache, suppressIntermediateValues); break; // Special handling for ArrayLength expressions case UnaryExpression unaryExpr when unaryExpr.NodeType == ExpressionType.ArrayLength: string arrayName = GetCleanMemberName(unaryExpr.Operand); string lengthDisplayName = $"{arrayName}.Length"; - TryAddExpressionValue(unaryExpr, lengthDisplayName, details); + TryAddExpressionValue(unaryExpr, lengthDisplayName, details, evaluationCache); if (unaryExpr.Operand is not MemberExpression) { - ExtractVariablesFromExpression(unaryExpr.Operand, details, suppressIntermediateValues); + ExtractVariablesFromExpression(unaryExpr.Operand, details, evaluationCache, suppressIntermediateValues); } break; case UnaryExpression unaryExpr: - ExtractVariablesFromExpression(unaryExpr.Operand, details, suppressIntermediateValues); + ExtractVariablesFromExpression(unaryExpr.Operand, details, evaluationCache, suppressIntermediateValues); break; case MemberExpression memberExpr: - AddMemberExpressionToDetails(memberExpr, details); + AddMemberExpressionToDetails(memberExpr, details, evaluationCache); break; case MethodCallExpression callExpr: - HandleMethodCallExpression(callExpr, details, suppressIntermediateValues); + HandleMethodCallExpression(callExpr, details, evaluationCache, suppressIntermediateValues); break; case ConditionalExpression conditionalExpr: - ExtractVariablesFromExpression(conditionalExpr.Test, details, suppressIntermediateValues); - ExtractVariablesFromExpression(conditionalExpr.IfTrue, details, suppressIntermediateValues); - ExtractVariablesFromExpression(conditionalExpr.IfFalse, details, suppressIntermediateValues); + ExtractVariablesFromExpression(conditionalExpr.Test, details, evaluationCache, suppressIntermediateValues); + ExtractVariablesFromExpression(conditionalExpr.IfTrue, details, evaluationCache, suppressIntermediateValues); + ExtractVariablesFromExpression(conditionalExpr.IfFalse, details, evaluationCache, suppressIntermediateValues); break; case InvocationExpression invocationExpr: - ExtractVariablesFromExpression(invocationExpr.Expression, details, suppressIntermediateValues); + ExtractVariablesFromExpression(invocationExpr.Expression, details, evaluationCache, suppressIntermediateValues); foreach (Expression argument in invocationExpr.Arguments) { - ExtractVariablesFromExpression(argument, details, suppressIntermediateValues); + ExtractVariablesFromExpression(argument, details, evaluationCache, suppressIntermediateValues); } break; @@ -149,7 +336,7 @@ private static void ExtractVariablesFromExpression(Expression? expr, Dictionary< case NewExpression newExpr: foreach (Expression argument in newExpr.Arguments) { - ExtractVariablesFromExpression(argument, details, suppressIntermediateValues); + ExtractVariablesFromExpression(argument, details, evaluationCache, suppressIntermediateValues); } // Don't display the new object value if we're suppressing intermediate values @@ -157,18 +344,18 @@ private static void ExtractVariablesFromExpression(Expression? expr, Dictionary< if (!suppressIntermediateValues) { string newExprDisplay = GetCleanMemberName(newExpr); - TryAddExpressionValue(newExpr, newExprDisplay, details); + TryAddExpressionValue(newExpr, newExprDisplay, details, evaluationCache); } break; case ListInitExpression listInitExpr: - ExtractVariablesFromExpression(listInitExpr.NewExpression, details, suppressIntermediateValues: true); + ExtractVariablesFromExpression(listInitExpr.NewExpression, details, evaluationCache, suppressIntermediateValues: true); foreach (ElementInit initializer in listInitExpr.Initializers) { foreach (Expression argument in initializer.Arguments) { - ExtractVariablesFromExpression(argument, details, suppressIntermediateValues); + ExtractVariablesFromExpression(argument, details, evaluationCache, suppressIntermediateValues); } } @@ -177,25 +364,25 @@ private static void ExtractVariablesFromExpression(Expression? expr, Dictionary< case NewArrayExpression newArrayExpr: foreach (Expression expression in newArrayExpr.Expressions) { - ExtractVariablesFromExpression(expression, details, suppressIntermediateValues); + ExtractVariablesFromExpression(expression, details, evaluationCache, suppressIntermediateValues); } break; } } - private static void HandleArrayIndexExpression(BinaryExpression arrayIndexExpr, Dictionary details) + private static void HandleArrayIndexExpression(BinaryExpression arrayIndexExpr, Dictionary details, Dictionary evaluationCache) { string arrayName = GetCleanMemberName(arrayIndexExpr.Left); - string indexValue = GetIndexArgumentDisplay(arrayIndexExpr.Right); + string indexValue = GetIndexArgumentDisplay(arrayIndexExpr.Right, evaluationCache); string indexerDisplay = $"{arrayName}[{indexValue}]"; - TryAddExpressionValue(arrayIndexExpr, indexerDisplay, details); + TryAddExpressionValue(arrayIndexExpr, indexerDisplay, details, evaluationCache); // Extract variables from the index argument - ExtractVariablesFromExpression(arrayIndexExpr.Right, details); + ExtractVariablesFromExpression(arrayIndexExpr.Right, details, evaluationCache); } - private static void AddMemberExpressionToDetails(MemberExpression memberExpr, Dictionary details) + private static void AddMemberExpressionToDetails(MemberExpression memberExpr, Dictionary details, Dictionary evaluationCache) { string displayName = GetCleanMemberName(memberExpr); @@ -204,54 +391,49 @@ private static void AddMemberExpressionToDetails(MemberExpression memberExpr, Di return; } - try - { - object? value = Expression.Lambda(memberExpr).Compile().DynamicInvoke(); + // Use cached value if available, otherwise try to get it from the cache or mark as failed + details[displayName] = evaluationCache.TryGetValue(memberExpr, out object? cachedValue) + ? cachedValue + : ""; - // Skip Func and Action delegates as they don't provide useful information in assertion failures - if (IsFuncOrActionType(value?.GetType())) - { - return; - } - - details[displayName] = value; - } - catch + // Skip Func and Action delegates as they don't provide useful information in assertion failures + if (IsFuncOrActionType(cachedValue?.GetType())) { - details[displayName] = ""; + details.Remove(displayName); + return; } // Only extract variables from the object being accessed if it's not a member expression or indexer (which would show the full collection) if (memberExpr.Expression is not null and not MemberExpression) { - ExtractVariablesFromExpression(memberExpr.Expression, details, suppressIntermediateValues: true); + ExtractVariablesFromExpression(memberExpr.Expression, details, evaluationCache, suppressIntermediateValues: true); } } - private static void HandleMethodCallExpression(MethodCallExpression callExpr, Dictionary details, bool suppressIntermediateValues = false) + private static void HandleMethodCallExpression(MethodCallExpression callExpr, Dictionary details, Dictionary evaluationCache, bool suppressIntermediateValues = false) { // Special handling for indexers (get_Item calls) if (callExpr.Method.Name == "get_Item" && callExpr.Object is not null && callExpr.Arguments.Count == 1) { string objectName = GetCleanMemberName(callExpr.Object); - string indexValue = GetIndexArgumentDisplay(callExpr.Arguments[0]); + string indexValue = GetIndexArgumentDisplay(callExpr.Arguments[0], evaluationCache); string indexerDisplay = $"{objectName}[{indexValue}]"; - TryAddExpressionValue(callExpr, indexerDisplay, details); + TryAddExpressionValue(callExpr, indexerDisplay, details, evaluationCache); // Extract variables from the index argument but not from the object. - ExtractVariablesFromExpression(callExpr.Arguments[0], details, suppressIntermediateValues); + ExtractVariablesFromExpression(callExpr.Arguments[0], details, evaluationCache, suppressIntermediateValues); } else if (callExpr.Method.Name == "Get" && callExpr.Object is not null && callExpr.Arguments.Count > 0) { string objectName = GetCleanMemberName(callExpr.Object); - string indexDisplay = string.Join(", ", callExpr.Arguments.Select(GetIndexArgumentDisplay)); + string indexDisplay = string.Join(", ", callExpr.Arguments.Select(arg => GetIndexArgumentDisplay(arg, evaluationCache))); string indexerDisplay = $"{objectName}[{indexDisplay}]"; - TryAddExpressionValue(callExpr, indexerDisplay, details); + TryAddExpressionValue(callExpr, indexerDisplay, details, evaluationCache); // Extract variables from the index arguments but not from the object foreach (Expression argument in callExpr.Arguments) { - ExtractVariablesFromExpression(argument, details, suppressIntermediateValues); + ExtractVariablesFromExpression(argument, details, evaluationCache, suppressIntermediateValues); } } else @@ -263,14 +445,14 @@ private static void HandleMethodCallExpression(MethodCallExpression callExpr, Di { // For boolean-returning methods, extract details from the object being called // This captures the last non-boolean method call in a chain - ExtractVariablesFromExpression(callExpr.Object, details, suppressIntermediateValues); + ExtractVariablesFromExpression(callExpr.Object, details, evaluationCache, suppressIntermediateValues); } } else { // For non-boolean methods, capture the method call itself string methodCallDisplay = GetCleanMemberName(callExpr); - TryAddExpressionValue(callExpr, methodCallDisplay, details); + TryAddExpressionValue(callExpr, methodCallDisplay, details, evaluationCache); // Don't extract from the object to avoid duplication } @@ -278,7 +460,7 @@ private static void HandleMethodCallExpression(MethodCallExpression callExpr, Di // Always extract variables from the arguments foreach (Expression argument in callExpr.Arguments) { - ExtractVariablesFromExpression(argument, details, suppressIntermediateValues); + ExtractVariablesFromExpression(argument, details, evaluationCache, suppressIntermediateValues); } } } @@ -306,7 +488,7 @@ private static string GetCleanMemberName(Expression? expr) ? "" : CleanExpressionText(expr.ToString()); - private static string GetIndexArgumentDisplay(Expression indexArg) + private static string GetIndexArgumentDisplay(Expression indexArg, Dictionary evaluationCache) { try { @@ -315,7 +497,32 @@ private static string GetIndexArgumentDisplay(Expression indexArg) return FormatValue(constExpr.Value); } - // For complex index expressions, just use the expression string + // For member expressions that are fields or simple variable references, + // preserve the variable name to help with readability + if (indexArg is MemberExpression memberExpr && IsVariableReference(memberExpr)) + { + return CleanExpressionText(indexArg.ToString()); + } + + // For parameter expressions (method parameters), preserve the parameter name + if (indexArg is ParameterExpression) + { + return CleanExpressionText(indexArg.ToString()); + } + + // For complex expressions, preserve the original expression text for better readability + if (!IsSimpleExpression(indexArg)) + { + return CleanExpressionText(indexArg.ToString()); + } + + // For other simple expressions, try to use cached value + if (evaluationCache.TryGetValue(indexArg, out object? cachedValue)) + { + return FormatValue(cachedValue); + } + + // Fallback to expression text return CleanExpressionText(indexArg.ToString()); } catch @@ -324,11 +531,32 @@ private static string GetIndexArgumentDisplay(Expression indexArg) } } + private static bool IsVariableReference(MemberExpression memberExpr) + // Check if this is a field or local variable reference (not a property access on an object) + // Fields typically have Expression as null (static) or ConstantExpression (instance field on captured variable) + => memberExpr.Expression is null or ConstantExpression; + + private static bool IsSimpleExpression(Expression expr) + => expr switch + { + // Constants are simple + ConstantExpression => true, + // Parameter references are simple but should preserve names for indices + ParameterExpression => false, // Changed: preserve parameter names in indices + // Member expressions should be evaluated case by case + MemberExpression => false, // Changed: evaluate member expressions individually + // Simple unary operations on members like "!flag" + UnaryExpression unary when unary.Operand is MemberExpression or ParameterExpression => false, + // Everything else is considered complex (binary operations, method calls, etc.) + _ => false, + }; + private static string FormatValue(object? value) => value switch { null => "null", string s => $"\"{s}\"", + IFormattable formattable => formattable.ToString(null, CultureInfo.InvariantCulture), IEnumerable e => $"[{string.Join(", ", e.Select(FormatValue))}]", IEnumerable e and not string => $"[{string.Join(", ", e.Cast().Select(FormatValue))}]", _ => value.ToString() ?? "", @@ -765,23 +993,21 @@ private static bool TryRemoveWrapper(string input, ref int index, string pattern return false; } - private static bool TryAddExpressionValue(Expression expr, string displayName, Dictionary details) + private static bool TryAddExpressionValue(Expression expr, string displayName, Dictionary details, Dictionary evaluationCache) { if (details.ContainsKey(displayName)) { return false; } - try - { - object? value = Expression.Lambda(expr).Compile().DynamicInvoke(); - details[displayName] = value; - } - catch + // Use cached value if available + if (evaluationCache.TryGetValue(expr, out object? cachedValue)) { - details[displayName] = ""; + details[displayName] = cachedValue; + return true; } + details[displayName] = ""; return true; } diff --git a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs index 5b0d058e6d..f06a2f9b11 100644 --- a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs +++ b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs @@ -479,10 +479,10 @@ public void That_NewExpression_ExtractsVariablesCorrectly() .WithMessage(""" Assert.That(() => new DateTime(year, month, day) == DateTime.MinValue) failed. Details: - DateTime.MinValue = 1/1/0001 12:00:00 AM + DateTime.MinValue = 01/01/0001 00:00:00 day = 25 month = 12 - new DateTime(year, month, day) = 12/25/2023 12:00:00 AM + new DateTime(year, month, day) = 12/25/2023 00:00:00 year = 2023 """); } @@ -1042,4 +1042,25 @@ public void That_WithNullConstantAndVariable_OnlyIncludesVariable() nullVariable = null """); } + + public void That_DoesNotEvaluateTwice() + { + var box = new Box(); + int expected = 2; + + Action act = () => Assert.That(() => box.GetNumber() == expected); + + act.Should().Throw(); + } + + private class Box + { + private int _c; + + public int GetNumber() + { + _c++; + return _c; + } + } } From d6f4d06c09cf9471b7bea033b445b8f68abf63cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Wed, 8 Oct 2025 13:12:53 +0200 Subject: [PATCH 2/4] Cleanup --- .../TestFramework/Assertions/Assert.That.cs | 263 ++++++++++++++---- 1 file changed, 209 insertions(+), 54 deletions(-) diff --git a/src/TestFramework/TestFramework/Assertions/Assert.That.cs b/src/TestFramework/TestFramework/Assertions/Assert.That.cs index 06a4ecf1ed..968301b953 100644 --- a/src/TestFramework/TestFramework/Assertions/Assert.That.cs +++ b/src/TestFramework/TestFramework/Assertions/Assert.That.cs @@ -10,6 +10,29 @@ namespace Microsoft.VisualStudio.TestTools.UnitTesting; /// public static partial class AssertExtensions { + // Constants for standardized display values + private const string FailedToEvaluate = ""; + private const string NullDisplay = "null"; + private const string NullAngleBrackets = ""; + + // Constants for indexer method names + private const string GetItemMethodName = "get_Item"; + private const string GetMethodName = "Get"; + + // Constants for compiler-generated patterns + private const string AnonymousTypePrefix = "<>f__AnonymousType"; + private const string ValueWrapperPattern = "value("; + private const string ArrayLengthWrapperPattern = "ArrayLength("; + private const string NewKeyword = "new "; + private const string ActionTypePrefix = "Action`"; + private const string FuncTypePrefix = "Func`"; + + // Constants for collection type patterns + private const string ListInitPattern = "`1()"; + + // Constants for parenthesis limits + private const int MaxConsecutiveParentheses = 2; + /// /// Provides That extension to Assert class. /// @@ -32,9 +55,12 @@ public static void That(Expression> condition, string? message = null throw new ArgumentNullException(nameof(condition)); } - // Cache to store evaluated expression values to avoid re-evaluation + // Cache to store evaluated expression values to avoid re-evaluation. + // This is critical for expressions with side effects - we evaluate each sub-expression + // only once and reuse the cached result throughout the assertion process. var evaluationCache = new Dictionary(); + // Evaluate the condition expression and cache all sub-expression values bool result = EvaluateExpression(condition.Body, evaluationCache); if (result) @@ -62,6 +88,10 @@ public static void That(Expression> condition, string? message = null } } + /// + /// Evaluates an expression tree and caches all sub-expression values to avoid re-evaluation. + /// This ensures expressions with side effects (like method calls) are only executed once. + /// [RequiresDynamicCode("Calls Microsoft.VisualStudio.TestTools.UnitTesting.AssertExtensions.EvaluateAllSubExpressions(Expression, Dictionary)")] private static bool EvaluateExpression(Expression expr, Dictionary cache) { @@ -78,10 +108,15 @@ private static bool EvaluateExpression(Expression expr, Dictionary + /// Recursively evaluates all sub-expressions in the tree and caches their values. + /// Uses a bottom-up approach: evaluate children first, then replace them with constants + /// before evaluating the parent. This prevents side effects from being executed multiple times. + /// [RequiresDynamicCode("Calls System.Linq.Expressions.Expression.Lambda(Expression, params ParameterExpression[])")] private static void EvaluateAllSubExpressions(Expression expr, Dictionary cache) { - // If already evaluated, skip + // If already evaluated, skip to avoid duplicate work if (cache.ContainsKey(expr)) { return; @@ -89,19 +124,24 @@ private static void EvaluateAllSubExpressions(Expression expr, Dictionary + /// Replaces sub-expressions in an expression tree with constant values from the cache. + /// This prevents re-execution of side effects when the parent expression is compiled and invoked. + /// private static Expression ReplaceSubExpressionsWithConstants(Expression expr, Dictionary cache) { // If this expression's direct sub-expressions are cached, replace them with constants switch (expr) { case BinaryExpression binaryExpr: + // Replace left and right operands with constants if they're cached Expression left = cache.TryGetValue(binaryExpr.Left, out object? leftValue) ? Expression.Constant(leftValue, binaryExpr.Left.Type) : binaryExpr.Left; @@ -200,16 +255,19 @@ private static Expression ReplaceSubExpressionsWithConstants(Expression expr, Di return Expression.MakeBinary(binaryExpr.NodeType, left, right); case UnaryExpression unaryExpr: + // Replace the operand with a constant if it's cached Expression operand = cache.TryGetValue(unaryExpr.Operand, out object? value) ? Expression.Constant(value, unaryExpr.Operand.Type) : unaryExpr.Operand; return Expression.MakeUnary(unaryExpr.NodeType, operand, unaryExpr.Type); case MemberExpression memberExpr when memberExpr.Expression is not null && cache.ContainsKey(memberExpr.Expression): + // Replace the object being accessed with a constant Expression instance = Expression.Constant(cache[memberExpr.Expression], memberExpr.Expression.Type); return Expression.MakeMemberAccess(instance, memberExpr.Member); case MethodCallExpression callExpr: + // Replace the target object and all arguments with constants if they're cached Expression? obj = callExpr.Object is not null && cache.TryGetValue(callExpr.Object, out object? callExprValue) ? Expression.Constant(callExprValue, callExpr.Object.Type) : callExpr.Object; @@ -222,6 +280,7 @@ private static Expression ReplaceSubExpressionsWithConstants(Expression expr, Di return Expression.Call(obj, callExpr.Method, args); case ConditionalExpression conditionalExpr: + // Replace all three parts of the ternary expression with constants if they're cached Expression test = cache.TryGetValue(conditionalExpr.Test, out object? testValue) ? Expression.Constant(testValue, conditionalExpr.Test.Type) : conditionalExpr.Test; @@ -234,17 +293,24 @@ private static Expression ReplaceSubExpressionsWithConstants(Expression expr, Di return Expression.Condition(test, ifTrue, ifFalse); case TypeBinaryExpression typeBinaryExpr when cache.ContainsKey(typeBinaryExpr.Expression): + // Replace the object being type-checked with a constant Expression typeExpr = Expression.Constant(cache[typeBinaryExpr.Expression], typeBinaryExpr.Expression.Type); return Expression.TypeIs(typeExpr, typeBinaryExpr.TypeOperand); default: - // For other expressions or leaf nodes, return as-is + // For other expressions or leaf nodes (constants, parameters), return as-is return expr; } } + /// + /// Extracts diagnostic details from the failed assertion by identifying variables + /// and their values in the expression tree. Returns a formatted string showing + /// variable names and their evaluated values. + /// private static string ExtractDetails(Expression expr, Dictionary evaluationCache) { + // Dictionary to store variable names and their values var details = new Dictionary(); ExtractVariablesFromExpression(expr, details, evaluationCache); @@ -253,7 +319,7 @@ private static string ExtractDetails(Expression expr, Dictionary> sortedDetails = details.OrderBy(kvp => kvp.Key, StringComparer.Ordinal); var sb = new StringBuilder(); @@ -269,6 +335,11 @@ private static string ExtractDetails(Expression expr, Dictionary + /// Recursively walks the expression tree to extract meaningful variable references and their values. + /// The suppressIntermediateValues parameter controls whether to display the value of intermediate + /// expressions (like 'new List()' when it's part of 'new List().Count'). + /// private static void ExtractVariablesFromExpression(Expression? expr, Dictionary details, Dictionary evaluationCache, bool suppressIntermediateValues = false) { if (expr is null) @@ -284,6 +355,7 @@ private static void ExtractVariablesFromExpression(Expression? expr, Dictionary< break; case BinaryExpression binaryExpr: + // For binary operations (e.g., x > y), extract variables from both sides ExtractVariablesFromExpression(binaryExpr.Left, details, evaluationCache, suppressIntermediateValues); ExtractVariablesFromExpression(binaryExpr.Right, details, evaluationCache, suppressIntermediateValues); break; @@ -293,12 +365,15 @@ private static void ExtractVariablesFromExpression(Expression? expr, Dictionary< ExtractVariablesFromExpression(typeBinaryExpr.Expression, details, evaluationCache, suppressIntermediateValues); break; - // Special handling for ArrayLength expressions + // Special handling for ArrayLength expressions (e.g., myArray.Length) case UnaryExpression unaryExpr when unaryExpr.NodeType == ExpressionType.ArrayLength: + // Display as "arrayName.Length" for better readability string arrayName = GetCleanMemberName(unaryExpr.Operand); string lengthDisplayName = $"{arrayName}.Length"; TryAddExpressionValue(unaryExpr, lengthDisplayName, details, evaluationCache); + // Only extract the array variable if it's not already a member expression + // (to avoid duplicate entries like "myArray" and "myArray.Length") if (unaryExpr.Operand is not MemberExpression) { ExtractVariablesFromExpression(unaryExpr.Operand, details, evaluationCache, suppressIntermediateValues); @@ -307,24 +382,29 @@ private static void ExtractVariablesFromExpression(Expression? expr, Dictionary< break; case UnaryExpression unaryExpr: + // For other unary operations (e.g., !flag), extract the operand ExtractVariablesFromExpression(unaryExpr.Operand, details, evaluationCache, suppressIntermediateValues); break; case MemberExpression memberExpr: + // For member access (e.g., obj.Property), add to details AddMemberExpressionToDetails(memberExpr, details, evaluationCache); break; case MethodCallExpression callExpr: + // Handle method calls with special logic for indexers and boolean methods HandleMethodCallExpression(callExpr, details, evaluationCache, suppressIntermediateValues); break; case ConditionalExpression conditionalExpr: + // For ternary expressions, extract from all three parts ExtractVariablesFromExpression(conditionalExpr.Test, details, evaluationCache, suppressIntermediateValues); ExtractVariablesFromExpression(conditionalExpr.IfTrue, details, evaluationCache, suppressIntermediateValues); ExtractVariablesFromExpression(conditionalExpr.IfFalse, details, evaluationCache, suppressIntermediateValues); break; case InvocationExpression invocationExpr: + // For delegate invocations, extract from the delegate and all arguments ExtractVariablesFromExpression(invocationExpr.Expression, details, evaluationCache, suppressIntermediateValues); foreach (Expression argument in invocationExpr.Arguments) { @@ -334,13 +414,14 @@ private static void ExtractVariablesFromExpression(Expression? expr, Dictionary< break; case NewExpression newExpr: + // For object creation, extract from constructor arguments foreach (Expression argument in newExpr.Arguments) { ExtractVariablesFromExpression(argument, details, evaluationCache, suppressIntermediateValues); } - // Don't display the new object value if we're suppressing intermediate values - // (which happens when it's part of a member access chain) + // Don't display the new object value if it's part of a member access chain + // (e.g., don't show "new Person()" when displaying "new Person().Name") if (!suppressIntermediateValues) { string newExprDisplay = GetCleanMemberName(newExpr); @@ -350,6 +431,7 @@ private static void ExtractVariablesFromExpression(Expression? expr, Dictionary< break; case ListInitExpression listInitExpr: + // For collection initializers, suppress the intermediate 'new List()' but show the elements ExtractVariablesFromExpression(listInitExpr.NewExpression, details, evaluationCache, suppressIntermediateValues: true); foreach (ElementInit initializer in listInitExpr.Initializers) { @@ -362,6 +444,7 @@ private static void ExtractVariablesFromExpression(Expression? expr, Dictionary< break; case NewArrayExpression newArrayExpr: + // For array creation, extract from all element expressions foreach (Expression expression in newArrayExpr.Expressions) { ExtractVariablesFromExpression(expression, details, evaluationCache, suppressIntermediateValues); @@ -371,6 +454,10 @@ private static void ExtractVariablesFromExpression(Expression? expr, Dictionary< } } + /// + /// Handles array indexing expressions (e.g., myArray[i]) by displaying them in indexer notation + /// and extracting the index variable. + /// private static void HandleArrayIndexExpression(BinaryExpression arrayIndexExpr, Dictionary details, Dictionary evaluationCache) { string arrayName = GetCleanMemberName(arrayIndexExpr.Left); @@ -382,8 +469,13 @@ private static void HandleArrayIndexExpression(BinaryExpression arrayIndexExpr, ExtractVariablesFromExpression(arrayIndexExpr.Right, details, evaluationCache); } + /// + /// Adds a member expression (e.g., obj.Property) to the details dictionary with its cached value. + /// Filters out Func and Action delegates as they don't provide useful diagnostic information. + /// private static void AddMemberExpressionToDetails(MemberExpression memberExpr, Dictionary details, Dictionary evaluationCache) { + // Get a clean, readable name for the member (e.g., "person.Name" instead of compiler-generated text) string displayName = GetCleanMemberName(memberExpr); if (details.ContainsKey(displayName)) @@ -391,10 +483,10 @@ private static void AddMemberExpressionToDetails(MemberExpression memberExpr, Di return; } - // Use cached value if available, otherwise try to get it from the cache or mark as failed + // Use cached value if available, otherwise mark as failed details[displayName] = evaluationCache.TryGetValue(memberExpr, out object? cachedValue) ? cachedValue - : ""; + : FailedToEvaluate; // Skip Func and Action delegates as they don't provide useful information in assertion failures if (IsFuncOrActionType(cachedValue?.GetType())) @@ -403,28 +495,38 @@ private static void AddMemberExpressionToDetails(MemberExpression memberExpr, Di return; } - // Only extract variables from the object being accessed if it's not a member expression or indexer (which would show the full collection) + // Only extract variables from the object being accessed if it's not a member expression + // (to avoid showing both "person" and "person.Name" when "person.Name" is sufficient) if (memberExpr.Expression is not null and not MemberExpression) { ExtractVariablesFromExpression(memberExpr.Expression, details, evaluationCache, suppressIntermediateValues: true); } } + /// + /// Handles method call expressions with special logic for: + /// - Indexers (get_Item and Get methods): displayed as object[index] + /// - Boolean-returning methods: extract from the target object for better diagnostics + /// - Other methods: capture the method call itself and extract from arguments. + /// private static void HandleMethodCallExpression(MethodCallExpression callExpr, Dictionary details, Dictionary evaluationCache, bool suppressIntermediateValues = false) { - // Special handling for indexers (get_Item calls) - if (callExpr.Method.Name == "get_Item" && callExpr.Object is not null && callExpr.Arguments.Count == 1) + // Special handling for indexers (e.g., list[0] calls get_Item(0)) + if (callExpr.Method.Name == GetItemMethodName && callExpr.Object is not null && callExpr.Arguments.Count == 1) { + // Display as "listName[indexValue]" for readability string objectName = GetCleanMemberName(callExpr.Object); string indexValue = GetIndexArgumentDisplay(callExpr.Arguments[0], evaluationCache); string indexerDisplay = $"{objectName}[{indexValue}]"; TryAddExpressionValue(callExpr, indexerDisplay, details, evaluationCache); - // Extract variables from the index argument but not from the object. + // Extract variables from the index argument but not from the object + // (to avoid showing both "list" and "list[0]") ExtractVariablesFromExpression(callExpr.Arguments[0], details, evaluationCache, suppressIntermediateValues); } - else if (callExpr.Method.Name == "Get" && callExpr.Object is not null && callExpr.Arguments.Count > 0) + else if (callExpr.Method.Name == GetMethodName && callExpr.Object is not null && callExpr.Arguments.Count > 0) { + // Handle multi-dimensional array indexers (e.g., array.Get(i, j) displayed as array[i, j]) string objectName = GetCleanMemberName(callExpr.Object); string indexDisplay = string.Join(", ", callExpr.Arguments.Select(arg => GetIndexArgumentDisplay(arg, evaluationCache))); string indexerDisplay = $"{objectName}[{indexDisplay}]"; @@ -438,19 +540,20 @@ private static void HandleMethodCallExpression(MethodCallExpression callExpr, Di } else { - // Check if the method returns a boolean + // Check if the method returns a boolean (e.g., list.Any(), string.Contains()) if (callExpr.Method.ReturnType == typeof(bool)) { if (callExpr.Object is not null) { - // For boolean-returning methods, extract details from the object being called - // This captures the last non-boolean method call in a chain + // For boolean-returning methods, extract details from the object being called. + // This provides more useful context (e.g., show "list" and "list.Count" rather than "list.Any()") ExtractVariablesFromExpression(callExpr.Object, details, evaluationCache, suppressIntermediateValues); } } else { // For non-boolean methods, capture the method call itself + // (e.g., "list.Count" when used in a comparison) string methodCallDisplay = GetCleanMemberName(callExpr); TryAddExpressionValue(callExpr, methodCallDisplay, details, evaluationCache); @@ -474,31 +577,36 @@ private static bool IsFuncOrActionType(Type? type) // Check for Action types if (type == typeof(Action) || - (type.IsGenericType && type.GetGenericTypeDefinition().Name.StartsWith("Action`", StringComparison.Ordinal))) + (type.IsGenericType && type.GetGenericTypeDefinition().Name.StartsWith(ActionTypePrefix, StringComparison.Ordinal))) { return true; } // Check for Func types - return type.IsGenericType && type.GetGenericTypeDefinition().Name.StartsWith("Func`", StringComparison.Ordinal); + return type.IsGenericType && type.GetGenericTypeDefinition().Name.StartsWith(FuncTypePrefix, StringComparison.Ordinal); } private static string GetCleanMemberName(Expression? expr) => expr is null - ? "" + ? NullAngleBrackets : CleanExpressionText(expr.ToString()); + /// + /// Gets a display string for an index argument in an indexer expression. + /// Preserves variable names for readability (e.g., "i" instead of "0" if i is a variable). + /// private static string GetIndexArgumentDisplay(Expression indexArg, Dictionary evaluationCache) { try { + // For constant values, just format the value if (indexArg is ConstantExpression constExpr) { return FormatValue(constExpr.Value); } // For member expressions that are fields or simple variable references, - // preserve the variable name to help with readability + // preserve the variable name to help with readability (e.g., "myIndex" instead of "5") if (indexArg is MemberExpression memberExpr && IsVariableReference(memberExpr)) { return CleanExpressionText(indexArg.ToString()); @@ -510,7 +618,7 @@ private static string GetIndexArgumentDisplay(Expression indexArg, Dictionary + /// Determines if a member expression is a simple variable reference (field or captured variable) + /// rather than a property access on an object. + /// private static bool IsVariableReference(MemberExpression memberExpr) - // Check if this is a field or local variable reference (not a property access on an object) // Fields typically have Expression as null (static) or ConstantExpression (instance field on captured variable) => memberExpr.Expression is null or ConstantExpression; + /// + /// Determines if an expression is simple enough to evaluate directly or if its + /// textual representation should be preserved for better diagnostic messages. + /// private static bool IsSimpleExpression(Expression expr) => expr switch { - // Constants are simple + // Constants are simple and can be evaluated ConstantExpression => true, - // Parameter references are simple but should preserve names for indices - ParameterExpression => false, // Changed: preserve parameter names in indices + // Parameter references should preserve their names for indices (e.g., "i" not "0") + ParameterExpression => false, // Member expressions should be evaluated case by case - MemberExpression => false, // Changed: evaluate member expressions individually - // Simple unary operations on members like "!flag" + MemberExpression => false, + // Simple unary operations on members like "!flag" should preserve the expression text UnaryExpression unary when unary.Operand is MemberExpression or ParameterExpression => false, // Everything else is considered complex (binary operations, method calls, etc.) _ => false, @@ -554,30 +669,34 @@ private static bool IsSimpleExpression(Expression expr) private static string FormatValue(object? value) => value switch { - null => "null", + null => NullDisplay, string s => $"\"{s}\"", IFormattable formattable => formattable.ToString(null, CultureInfo.InvariantCulture), IEnumerable e => $"[{string.Join(", ", e.Select(FormatValue))}]", IEnumerable e and not string => $"[{string.Join(", ", e.Cast().Select(FormatValue))}]", - _ => value.ToString() ?? "", + _ => value.ToString() ?? NullAngleBrackets, }; + /// + /// Cleans up compiler-generated artifacts from expression text to make it more readable. + /// Removes display classes, compiler wrappers, and formats anonymous types and collections properly. + /// private static string CleanExpressionText(string raw) { - // Remove display class names and generated compiler prefixes string cleaned = raw; - // Remove compiler-generated wrappers FIRST, before display class cleanup + // Remove compiler-generated wrappers FIRST (e.g., "value()", "ArrayLength()") + // This must happen before display class cleanup to avoid breaking patterns cleaned = RemoveCompilerGeneratedWrappers(cleaned); - // Handle anonymous types - remove the compiler-generated type wrapper + // Handle anonymous types - convert compiler-generated type names to C# syntax (e.g., "new { ... }") cleaned = RemoveAnonymousTypeWrappers(cleaned); // Handle list initialization expressions - convert from Add method calls to collection initializer syntax cleaned = CleanListInitializers(cleaned); // Handle compiler-generated display classes more comprehensively - // Updated pattern to handle cases with and without parentheses around the display class + // Removes patterns like "DisplayClass0_1.myVariable" to just "myVariable" cleaned = CompilerGeneratedDisplayClassRegex().Replace(cleaned, "$1"); // Remove unnecessary outer parentheses and excessive consecutive parentheses @@ -586,6 +705,10 @@ private static string CleanExpressionText(string raw) return cleaned; } + /// + /// Removes anonymous type wrappers (e.g., "new <>f__AnonymousType0(x = 1)") + /// and replaces them with C# anonymous type syntax (e.g., "new { x = 1 }"). + /// private static string RemoveAnonymousTypeWrappers(string input) { if (string.IsNullOrEmpty(input)) @@ -599,11 +722,11 @@ private static string RemoveAnonymousTypeWrappers(string input) while (i < input.Length) { // Look for anonymous type pattern: new <>f__AnonymousType followed by generic parameters - if (i <= input.Length - 4 && input.Substring(i, 4) == "new " && - i + 4 < input.Length && input.Substring(i + 4).StartsWith("<>f__AnonymousType", StringComparison.Ordinal)) + if (i <= input.Length - NewKeyword.Length && input.Substring(i, NewKeyword.Length) == NewKeyword && + i + NewKeyword.Length < input.Length && input.Substring(i + NewKeyword.Length).StartsWith(AnonymousTypePrefix, StringComparison.Ordinal)) { // Find the start of the constructor parameters - int constructorStart = input.IndexOf('(', i + 4); + int constructorStart = input.IndexOf('(', i + NewKeyword.Length); if (constructorStart == -1) { result.Append(input[i]); @@ -649,6 +772,11 @@ private static string RemoveAnonymousTypeWrappers(string input) return result.ToString(); } + /// + /// Cleans up collection initializer expressions by converting verbose Add method calls + /// (e.g., "new List`1() { Void Add(Int32)(1), Void Add(Int32)(2) }") + /// to standard C# syntax (e.g., "new List<int> { 1, 2 }"). + /// private static string CleanListInitializers(string input) { if (string.IsNullOrEmpty(input)) @@ -741,12 +869,12 @@ private static bool TryMatchListInitPattern(string input, int startIndex, out st patternEnd = startIndex; // Check for "new " at the start - if (startIndex + 4 >= input.Length || !input.Substring(startIndex, 4).Equals("new ", StringComparison.Ordinal)) + if (startIndex + NewKeyword.Length >= input.Length || !input.Substring(startIndex, NewKeyword.Length).Equals(NewKeyword, StringComparison.Ordinal)) { return false; } - int pos = startIndex + 4; + int pos = startIndex + NewKeyword.Length; // Skip whitespace while (pos < input.Length && char.IsWhiteSpace(input[pos])) @@ -775,12 +903,12 @@ private static bool TryMatchListInitPattern(string input, int startIndex, out st } // Check for "`1()" pattern - if (pos + 4 >= input.Length || !input.Substring(pos, 4).Equals("`1()", StringComparison.Ordinal)) + if (pos + ListInitPattern.Length >= input.Length || !input.Substring(pos, ListInitPattern.Length).Equals(ListInitPattern, StringComparison.Ordinal)) { return false; } - pos += 4; + pos += ListInitPattern.Length; // Skip whitespace while (pos < input.Length && char.IsWhiteSpace(input[pos])) @@ -838,6 +966,10 @@ private static string CleanTypeName(string typeName) _ => typeName, }; + /// + /// Removes parentheses that wrap the entire expression and cleans up excessive + /// consecutive parentheses (e.g., "(((x)))" becomes "x", "((x) && (y))" becomes "(x) && (y)"). + /// private static string CleanParentheses(string input) { if (string.IsNullOrEmpty(input)) @@ -864,6 +996,10 @@ private static string CleanParentheses(string input) return input; } + /// + /// Removes outer parentheses if they wrap the entire expression without serving + /// a syntactic purpose (e.g., "(x > 5)" becomes "x > 5"). + /// private static string RemoveOuterParentheses(string input) { if (input.Length < 2 || !input.StartsWith('(') || !input.EndsWith(')')) @@ -894,6 +1030,10 @@ private static string RemoveOuterParentheses(string input) return parenCount == 0 ? input.Substring(1, input.Length - 2).Trim() : input; } + /// + /// Reduces excessive consecutive identical parentheses to at most 2. + /// This handles cases where multiple layers of compilation create redundant nesting. + /// private static string CleanExcessiveParentheses(string input) { if (string.IsNullOrEmpty(input)) @@ -918,7 +1058,7 @@ private static string CleanExcessiveParentheses(string input) } // Keep at most 2 consecutive parentheses - int keepCount = Math.Min(count, 2); + int keepCount = Math.Min(count, MaxConsecutiveParentheses); result.Append(new string(currentChar, keepCount)); i += count; } @@ -932,6 +1072,10 @@ private static string CleanExcessiveParentheses(string input) return result.ToString(); } + /// + /// Removes compiler-generated wrapper functions like "value(...)" and "ArrayLength(...)" + /// that appear in expression tree string representations. + /// private static string RemoveCompilerGeneratedWrappers(string input) { var result = new StringBuilder(); @@ -939,8 +1083,8 @@ private static string RemoveCompilerGeneratedWrappers(string input) while (i < input.Length) { - if (TryRemoveWrapper(input, ref i, "value(", RemoveCompilerGeneratedWrappers, result) || - TryRemoveWrapper(input, ref i, "ArrayLength(", content => RemoveCompilerGeneratedWrappers(content) + ".Length", result)) + if (TryRemoveWrapper(input, ref i, ValueWrapperPattern, RemoveCompilerGeneratedWrappers, result) || + TryRemoveWrapper(input, ref i, ArrayLengthWrapperPattern, content => RemoveCompilerGeneratedWrappers(content) + ".Length", result)) { continue; } @@ -952,9 +1096,14 @@ private static string RemoveCompilerGeneratedWrappers(string input) return result.ToString(); } + /// + /// Generic helper method to try removing a specific wrapper pattern from the input string. + /// Uses a transformation function to convert the unwrapped content as needed. + /// private static bool TryRemoveWrapper(string input, ref int index, string pattern, Func transform, StringBuilder result) { + // Check if the pattern exists at the current index if (index > input.Length - pattern.Length || !string.Equals(input.Substring(index, pattern.Length), pattern, StringComparison.Ordinal)) { @@ -965,7 +1114,7 @@ private static bool TryRemoveWrapper(string input, ref int index, string pattern int parenCount = 1; int i = start; - // Find matching closing parenthesis + // Find matching closing parenthesis by counting nesting levels while (i < input.Length && parenCount > 0) { if (input[i] == '(') @@ -980,21 +1129,26 @@ private static bool TryRemoveWrapper(string input, ref int index, string pattern i++; } + // If we found a complete wrapper, extract and transform the content if (parenCount == 0) { - // Extract content and apply transformation string content = input.Substring(start, i - start - 1); result.Append(transform(content)); index = i; return true; } - // Malformed, don't consume the pattern + // Malformed wrapper, don't consume the pattern return false; } + /// + /// Attempts to add an expression's value to the details dictionary using the cached value. + /// Returns true if the value was added, false if the key already exists. + /// private static bool TryAddExpressionValue(Expression expr, string displayName, Dictionary details, Dictionary evaluationCache) { + // Don't overwrite existing entries if (details.ContainsKey(displayName)) { return false; @@ -1007,7 +1161,8 @@ private static bool TryAddExpressionValue(Expression expr, string displayName, D return true; } - details[displayName] = ""; + // Mark as failed if we couldn't evaluate it + details[displayName] = FailedToEvaluate; return true; } From 7c7b517d8690ecc57167902509de013bb2aefd44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Wed, 8 Oct 2025 13:47:34 +0200 Subject: [PATCH 3/4] Apply suggestion from @Evangelink --- src/TestFramework/TestFramework/Assertions/Assert.That.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/TestFramework/TestFramework/Assertions/Assert.That.cs b/src/TestFramework/TestFramework/Assertions/Assert.That.cs index 968301b953..0dfb10c7f7 100644 --- a/src/TestFramework/TestFramework/Assertions/Assert.That.cs +++ b/src/TestFramework/TestFramework/Assertions/Assert.That.cs @@ -968,7 +968,8 @@ private static string CleanTypeName(string typeName) /// /// Removes parentheses that wrap the entire expression and cleans up excessive - /// consecutive parentheses (e.g., "(((x)))" becomes "x", "((x) && (y))" becomes "(x) && (y)"). + /// consecutive parentheses (e.g., "(((x)))" becomes "x", "((x) && (y))" becomes "(x) && (y)"). + /// private static string CleanParentheses(string input) { From 811a90ad3f6f60dac904215aaa0646deb510d7b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Wed, 8 Oct 2025 13:56:01 +0200 Subject: [PATCH 4/4] tests --- .../Assertions/AssertTests.That.cs | 33 ++++++++++++++++--- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs index f06a2f9b11..e26f8ec8d7 100644 --- a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs +++ b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs @@ -1043,14 +1043,39 @@ public void That_WithNullConstantAndVariable_OnlyIncludesVariable() """); } - public void That_DoesNotEvaluateTwice() + public void That_DoesNotEvaluateTwice_WhenAssertionFails() { var box = new Box(); - int expected = 2; - Action act = () => Assert.That(() => box.GetNumber() == expected); + // If we evaluate twice, box.GetNumber() is called once on comparison, and once when message for assertion is built. + // We compare to 0 to force failure. + Action act = () => Assert.That(() => box.GetNumber() == 0); - act.Should().Throw(); + // GetNumber() should report 1, which is the value when we evaluate only once. + act.Should().Throw() + .WithMessage(""" + Assert.That(() => box.GetNumber() == 0) failed. + Details: + box.GetNumber() = 1 + """); + + // We call again, this should be second call now. + box.GetNumber().Should().Be(2); + } + + public void That_DoesEvaluateTwice() + { + var box = new Box(); + + // Compare to 0 to force failure. + Action act = () => Assert.That(() => box.GetNumber() + box.GetNumber() == 0); + + act.Should().Throw() + .WithMessage(""" + Assert.That(() => box.GetNumber() + box.GetNumber() == 0) failed. + Details: + box.GetNumber() = 1 + """); } private class Box