diff --git a/src/TestFramework/TestFramework/Assertions/Assert.That.cs b/src/TestFramework/TestFramework/Assertions/Assert.That.cs index 08f08673c6..dadb59c4db 100644 --- a/src/TestFramework/TestFramework/Assertions/Assert.That.cs +++ b/src/TestFramework/TestFramework/Assertions/Assert.That.cs @@ -10,6 +10,34 @@ namespace Microsoft.VisualStudio.TestTools.UnitTesting; /// public static partial class AssertExtensions { + // Constants for standardized display values + 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; + + // Sentinel placed in the evaluation cache when a sub-expression cannot be evaluated. + // Using a reference-identity object (rather than a string) prevents accidentally + // substituting it for a same-typed operand when rebuilding parent expressions, and + // lets diagnostic extraction translate it to a localized "" display. + private static readonly object FailedToEvaluateSentinel = new(); + /// /// Provides That extension to Assert class. /// @@ -24,6 +52,9 @@ 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 . +#if NET7_0_OR_GREATER + [RequiresDynamicCode("Calls Microsoft.VisualStudio.TestTools.UnitTesting.AssertExtensions.EvaluateExpression(Expression, Dictionary)")] +#endif public static void That(Expression> condition, string? message = null, [CallerArgumentExpression(nameof(condition))] string? conditionExpression = null) { TelemetryCollector.TrackAssertionCall("Assert.That"); @@ -33,8 +64,27 @@ public static void That(Expression> condition, string? message = null throw new ArgumentNullException(nameof(condition)); } - var details = new Dictionary(); - bool result = EvaluateAndCollectDetails(condition.Body, details); + Dictionary? evaluationCache = null; + bool result; + + if (RequiresSinglePassEvaluation(condition.Body)) + { + // Potentially side-effecting expressions must be evaluated once while caching values. + evaluationCache = CreateEvaluationCache(); + result = EvaluateExpression(condition.Body, evaluationCache); + } + else + { + // For side-effect-free expressions, keep the fast path and only compute details on failures. + result = condition.Compile().Invoke(); + if (result) + { + return; + } + + evaluationCache = CreateEvaluationCache(); + EvaluateAllSubExpressions(condition.Body, evaluationCache); + } if (result) { @@ -50,8 +100,8 @@ public static void That(Expression> condition, string? message = null sb.AppendLine(string.Format(CultureInfo.InvariantCulture, FrameworkMessages.AssertThatMessageFormat, message)); } - string detailsString = BuildDetailsString(details); - if (!string.IsNullOrWhiteSpace(detailsString)) + string details = ExtractDetails(condition.Body, evaluationCache!); + if (!string.IsNullOrWhiteSpace(details)) { if (sb.Length == 0) { @@ -59,130 +109,507 @@ public static void That(Expression> condition, string? message = null } sb.AppendLine(FrameworkMessages.AssertThatDetailsPrefix); - sb.AppendLine(detailsString); + sb.AppendLine(details); } Assert.ReportAssertFailed($"Assert.That({expressionText})", sb.ToString().TrimEnd()); } } - private static string BuildDetailsString(Dictionary details) + /// + /// 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. + /// +#if NET7_0_OR_GREATER + [RequiresDynamicCode("Calls Microsoft.VisualStudio.TestTools.UnitTesting.AssertExtensions.EvaluateAllSubExpressions(Expression, Dictionary)")] +#endif + private static bool EvaluateExpression(Expression expr, Dictionary cache) { - if (details.Count == 0) + // Use a single-pass evaluation that only evaluates each expression once + EvaluateAllSubExpressions(expr, cache); + + // The root expression should now be cached with a bool value when the walk succeeded. + if (cache.TryGetValue(expr, out object? result) && result is bool boolResult) { - return string.Empty; + return boolResult; } - // Sort details alphabetically by variable name for consistent ordering - IOrderedEnumerable> sortedDetails = details.OrderBy(kvp => kvp.Key, StringComparer.Ordinal); + // The walk did not produce a bool for the root (e.g., a sub-expression threw and was + // cached as the failure sentinel, then the rebuilt parent could not be evaluated either). + // Re-invoke the original lambda so the user sees the actual exception thrown by their + // assertion code rather than an unrelated InvalidCastException from the sentinel. + return Expression.Lambda>(expr).Compile().Invoke(); + } - var sb = new StringBuilder(); - foreach (KeyValuePair kvp in sortedDetails) +#pragma warning disable IDE0028 // Keep explicit constructor for broader compiler compatibility when source-building. + private static Dictionary CreateEvaluationCache() => new Dictionary(); +#pragma warning restore IDE0028 + + private static bool RequiresSinglePassEvaluation(Expression expr) + => expr switch { -#if NET - sb.AppendLine(CultureInfo.InvariantCulture, $" {kvp.Key} = {FormatValue(kvp.Value)}"); -#else - sb.AppendLine($" {kvp.Key} = {FormatValue(kvp.Value)}"); -#endif - } + BinaryExpression binaryExpr => binaryExpr.Method is not null + || RequiresSinglePassEvaluation(binaryExpr.Left) + || RequiresSinglePassEvaluation(binaryExpr.Right), - return sb.ToString(); - } + UnaryExpression unaryExpr => unaryExpr.Method is not null + || RequiresSinglePassEvaluation(unaryExpr.Operand), + + MemberExpression memberExpr => (memberExpr.Expression is not null && RequiresSinglePassEvaluation(memberExpr.Expression)) + || memberExpr.Member.MemberType == MemberTypes.Property, + + ConditionalExpression conditionalExpr => RequiresSinglePassEvaluation(conditionalExpr.Test) + || RequiresSinglePassEvaluation(conditionalExpr.IfTrue) + || RequiresSinglePassEvaluation(conditionalExpr.IfFalse), + + TypeBinaryExpression typeBinaryExpr => RequiresSinglePassEvaluation(typeBinaryExpr.Expression), + + MethodCallExpression or InvocationExpression or NewExpression or ListInitExpression or MemberInitExpression + or NewArrayExpression or IndexExpression => true, - private static readonly object UnsetCapture = new(); + _ => false, + }; /// - /// Evaluates exactly once while capturing the values of selected sub-expressions - /// so the assertion failure message can describe what each named operand evaluated to. - /// Sub-expressions inside short-circuited / unreached branches are evaluated lazily as a fallback - /// (one evaluation total — the root never ran them). Fixes issue #6690. + /// 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. + /// Short-circuit operators (&&, ||, ??) and conditional expressions + /// honor C# evaluation order so unevaluated branches do not run side effects or throw. /// - /// if the condition evaluated to . - private static bool EvaluateAndCollectDetails(Expression body, Dictionary details) +#if NET7_0_OR_GREATER + [RequiresDynamicCode("Calls System.Linq.Expressions.Expression.Lambda(Expression, params ParameterExpression[])")] +#endif + private static void EvaluateAllSubExpressions(Expression expr, Dictionary cache) { - // Pass 1: Walk the tree to identify capture points and their display names. - var context = new AnalysisContext(); - AnalyzeExpression(body, context); - - // Pass 2: Rewrite the tree so that each captured sub-expression's value is written - // to a captures array as a side effect of the single evaluation of the root. - ParameterExpression arrayParam = Expression.Parameter(typeof(object?[]), "captures"); - var rewriter = new CaptureRewriter(context.CaptureMap, arrayParam); - Expression rewrittenBody = rewriter.Visit(body)!; - - // Compile and invoke ONCE. - // This intentionally pays the rewrite+compile cost on every Assert.That call (including passing ones) - // to guarantee single-pass evaluation for correctness (#6690). If this ever shows up as a hotspot, - // we can consider caching the compiled delegate by expression-tree instance. - var lambda = Expression.Lambda>(rewrittenBody, arrayParam); - object?[] values = new object?[context.CaptureNames.Count]; - - // Pre-fill with a sentinel so we can distinguish "not captured" (because the branch was - // short-circuited / not evaluated) from "captured null". - for (int i = 0; i < values.Length; i++) + // If already evaluated, skip to avoid duplicate work + if (cache.ContainsKey(expr)) { - values[i] = UnsetCapture; + return; } - bool result = lambda.Compile()(values); - - if (result) + try { - return true; - } + bool hasChildren = false; - // Build details using first-occurrence-per-name semantics, filtering out Func/Action values - // (matching the historical behavior, using runtime type as the existing code did). - for (int i = 0; i < context.CaptureNames.Count; i++) - { - string name = context.CaptureNames[i]; - if (details.ContainsKey(name)) + // First, recursively evaluate all sub-expressions (depth-first traversal). + // This ensures that when we evaluate a parent expression, all its children + // are already cached and can be replaced with constant values. + switch (expr) { - continue; - } + // Short-circuit binary operators: evaluate Left first; only walk Right + // when the original C# semantics would have executed it. This preserves + // assertions like `s != null && s.Length > 0` (no NRE) and `flag || Throw()`. + case BinaryExpression binaryExpr when binaryExpr.NodeType is ExpressionType.AndAlso or ExpressionType.OrElse: + if (TryEvaluateShortCircuitBinary(binaryExpr, cache)) + { + return; + } - object? value = values[i]; - if (ReferenceEquals(value, UnsetCapture)) - { - // The capture slot was never written, meaning the sub-expression was not evaluated - // (e.g., a short-circuited && / || branch or an unreached ternary branch). - // Only fall back to evaluating expressions that are conventionally pure operand - // reads (variable/property access, array indexers and lengths). For arbitrary - // method calls we must NOT re-evaluate, otherwise we'd silently violate - // short-circuit semantics for the user's code (issue #6690). - Expression expr = context.CaptureExpressions[i]; - if (IsSafeToReevaluate(expr)) - { - try + break; + + // Null-coalescing: evaluate Left; only walk Right when Left is null. + case BinaryExpression binaryExpr when binaryExpr.NodeType == ExpressionType.Coalesce: + if (TryEvaluateCoalesce(binaryExpr, cache)) { - value = Expression - .Lambda>(Expression.Convert(expr, typeof(object))) - .Compile()(); + return; } - catch + + break; + + case BinaryExpression binaryExpr: + // Evaluate both operands before evaluating the binary operation + EvaluateAllSubExpressions(binaryExpr.Left, cache); + EvaluateAllSubExpressions(binaryExpr.Right, cache); + hasChildren = true; + break; + + // Quote wraps an unevaluated expression tree (typically the lambda argument + // to IQueryable methods like Where/Any). Walking into the operand would + // compile it into a delegate and prevent rebuilding the Quote, which would + // break Queryable scenarios. Treat it as a leaf instead. + case UnaryExpression unaryExpr when unaryExpr.NodeType == ExpressionType.Quote: + break; + + case UnaryExpression unaryExpr: + // Evaluate the operand before evaluating the unary operation + EvaluateAllSubExpressions(unaryExpr.Operand, cache); + hasChildren = true; + break; + + case MemberExpression memberExpr: + // For member access (e.g., obj.Property), evaluate the object first + if (memberExpr.Expression is not null) { - value = ""; + EvaluateAllSubExpressions(memberExpr.Expression, cache); + hasChildren = true; } - } - else - { - // Skip potentially side-effecting captures that were short-circuited. - continue; - } + + break; + + case MethodCallExpression callExpr: + // For method calls, evaluate the target object and all arguments first + if (callExpr.Object is not null) + { + EvaluateAllSubExpressions(callExpr.Object, cache); + hasChildren = true; + } + + foreach (Expression argument in callExpr.Arguments) + { + EvaluateAllSubExpressions(argument, cache); + hasChildren = true; + } + + break; + + case ConditionalExpression conditionalExpr: + // Ternary expressions evaluate Test first and only the chosen branch. + if (TryEvaluateConditional(conditionalExpr, cache)) + { + return; + } + + break; + + case InvocationExpression invocationExpr: + // For delegate invocations, evaluate the delegate and all arguments + EvaluateAllSubExpressions(invocationExpr.Expression, cache); + foreach (Expression argument in invocationExpr.Arguments) + { + EvaluateAllSubExpressions(argument, cache); + } + + hasChildren = true; + break; + + case NewExpression newExpr: + // For object creation, evaluate all constructor arguments + foreach (Expression argument in newExpr.Arguments) + { + EvaluateAllSubExpressions(argument, cache); + hasChildren = true; + } + + break; + + case ListInitExpression listInitExpr: + // For collection initializers, evaluate the inner constructor arguments + // and all initializer arguments individually. We intentionally do not + // evaluate the wrapping NewExpression as a whole: List initializers + // require an unrealized NewExpression at rebuild time, and reusing a + // pre-built instance would force the constructor to run a second time. + foreach (Expression argument in listInitExpr.NewExpression.Arguments) + { + EvaluateAllSubExpressions(argument, cache); + } + + foreach (ElementInit initializer in listInitExpr.Initializers) + { + foreach (Expression argument in initializer.Arguments) + { + EvaluateAllSubExpressions(argument, cache); + } + } + + hasChildren = true; + break; + + case NewArrayExpression newArrayExpr: + // For array creation, evaluate all element expressions + foreach (Expression expression in newArrayExpr.Expressions) + { + EvaluateAllSubExpressions(expression, cache); + hasChildren = true; + } + + break; + + case IndexExpression indexExpr: + // Indexer access (e.g., list[i] expressed as IndexExpression) — evaluate + // the indexed object and all argument expressions. + if (indexExpr.Object is not null) + { + EvaluateAllSubExpressions(indexExpr.Object, cache); + hasChildren = true; + } + + foreach (Expression argument in indexExpr.Arguments) + { + EvaluateAllSubExpressions(argument, cache); + hasChildren = true; + } + + break; + + case TypeBinaryExpression typeBinaryExpr: + // For type checks (e.g., obj is Type), evaluate the object being tested + EvaluateAllSubExpressions(typeBinaryExpr.Expression, cache); + hasChildren = true; + break; } - if (IsFuncOrActionType(value?.GetType())) + // Evaluate the current expression and cache the result. + // For non-leaf expressions (hasChildren == true), we replace sub-expressions with their cached values first. + // For leaf expressions (hasChildren == false), we evaluate them directly. + if (hasChildren) { - continue; + // Now build a new expression that replaces sub-expressions with their cached values. + // This is crucial: by replacing evaluated sub-expressions with constants, we ensure + // that compiling and invoking this expression won't cause side effects to re-execute. + Expression replacedExpr = ReplaceSubExpressionsWithConstants(expr, cache); + + // Evaluate the replaced expression - this is now safe because all sub-expressions + // that could have side effects have been replaced with their constant values. + object? result = Expression.Lambda(replacedExpr).Compile().DynamicInvoke(); + cache[expr] = result; + } + else + { + // This is a leaf expression (no children to evaluate). + // Evaluate it directly and cache the result. + object? result = Expression.Lambda(expr).Compile().DynamicInvoke(); + cache[expr] = result; } + } + catch + { + // If evaluation fails (e.g., null reference, division by zero), cache the failure + // sentinel so other branches can still be diagnosed. The root caller (EvaluateExpression) + // detects a non-bool root result and re-invokes the original lambda to surface the + // user's real exception instead of masking it as an InvalidCastException. + cache[expr] = FailedToEvaluateSentinel; + } + } + + /// + /// Evaluates an && or || binary expression with short-circuit semantics + /// and caches both the operand values (when evaluated) and the resulting bool. + /// +#if NET7_0_OR_GREATER + [RequiresDynamicCode("Calls System.Linq.Expressions.Expression.Lambda(Expression, params ParameterExpression[])")] +#endif + private static bool TryEvaluateShortCircuitBinary(BinaryExpression binaryExpr, Dictionary cache) + { + EvaluateAllSubExpressions(binaryExpr.Left, cache); + if (!cache.TryGetValue(binaryExpr.Left, out object? leftValue) || leftValue is not bool leftBool) + { + // Left couldn't be evaluated to a bool — let the default leaf evaluation path + // attempt to compile the whole node so we surface a coherent failure. + return false; + } + + bool isAndAlso = binaryExpr.NodeType == ExpressionType.AndAlso; + bool shouldEvaluateRight = isAndAlso ? leftBool : !leftBool; + + if (!shouldEvaluateRight) + { + cache[binaryExpr] = leftBool; + return true; + } + + EvaluateAllSubExpressions(binaryExpr.Right, cache); + if (!cache.TryGetValue(binaryExpr.Right, out object? rightValue) || rightValue is not bool rightBool) + { + return false; + } + + cache[binaryExpr] = isAndAlso ? leftBool && rightBool : leftBool || rightBool; + return true; + } + + /// + /// Evaluates a ?? binary expression with short-circuit semantics, walking Right + /// only when Left is null. The actual result is computed by rebuilding the expression so + /// any user-supplied is preserved. + /// +#if NET7_0_OR_GREATER + [RequiresDynamicCode("Calls System.Linq.Expressions.Expression.Lambda(Expression, params ParameterExpression[])")] +#endif + private static bool TryEvaluateCoalesce(BinaryExpression binaryExpr, Dictionary cache) + { + EvaluateAllSubExpressions(binaryExpr.Left, cache); + if (!cache.TryGetValue(binaryExpr.Left, out object? leftValue) || ReferenceEquals(leftValue, FailedToEvaluateSentinel)) + { + return false; + } + + if (leftValue is not null) + { + // Rebuild with the cached Left so a user-supplied Conversion still runs. + Expression rebuilt = ReplaceSubExpressionsWithConstants(binaryExpr, cache); + cache[binaryExpr] = Expression.Lambda(rebuilt).Compile().DynamicInvoke(); + return true; + } + + EvaluateAllSubExpressions(binaryExpr.Right, cache); + Expression rebuiltWithRight = ReplaceSubExpressionsWithConstants(binaryExpr, cache); + cache[binaryExpr] = Expression.Lambda(rebuiltWithRight).Compile().DynamicInvoke(); + return true; + } + + /// + /// Evaluates a ternary expression by walking first + /// and only the selected branch, then caching the branch's result as the conditional's value. + /// +#if NET7_0_OR_GREATER + [RequiresDynamicCode("Calls System.Linq.Expressions.Expression.Lambda(Expression, params ParameterExpression[])")] +#endif + private static bool TryEvaluateConditional(ConditionalExpression conditionalExpr, Dictionary cache) + { + EvaluateAllSubExpressions(conditionalExpr.Test, cache); + if (!cache.TryGetValue(conditionalExpr.Test, out object? testValue) || testValue is not bool testBool) + { + return false; + } - details[name] = value; + Expression chosenBranch = testBool ? conditionalExpr.IfTrue : conditionalExpr.IfFalse; + EvaluateAllSubExpressions(chosenBranch, cache); + if (cache.TryGetValue(chosenBranch, out object? branchValue)) + { + cache[conditionalExpr] = branchValue; + return true; } return false; } - private static void AnalyzeExpression(Expression? expr, AnalysisContext context, bool suppressIntermediateValues = false) + /// + /// 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. + /// Uses the Update helpers on each node type so node metadata such as + /// and are preserved. + /// + private static Expression ReplaceSubExpressionsWithConstants(Expression expr, Dictionary cache) + { + switch (expr) + { + case BinaryExpression binaryExpr: + return binaryExpr.Update( + ReplaceChildWithConstant(binaryExpr.Left, cache), + binaryExpr.Conversion, + ReplaceChildWithConstant(binaryExpr.Right, cache)); + + case UnaryExpression unaryExpr: + return unaryExpr.Update(ReplaceChildWithConstant(unaryExpr.Operand, cache)); + + case MemberExpression memberExpr when memberExpr.Expression is not null: + return memberExpr.Update(ReplaceChildWithConstant(memberExpr.Expression, cache)); + + case MethodCallExpression callExpr: + Expression? obj = callExpr.Object is not null + ? ReplaceChildWithConstant(callExpr.Object, cache) + : null; + IEnumerable callArgs = callExpr.Arguments.Select(arg => ReplaceChildWithConstant(arg, cache)); + return callExpr.Update(obj, callArgs); + + case ConditionalExpression conditionalExpr: + return conditionalExpr.Update( + ReplaceChildWithConstant(conditionalExpr.Test, cache), + ReplaceChildWithConstant(conditionalExpr.IfTrue, cache), + ReplaceChildWithConstant(conditionalExpr.IfFalse, cache)); + + case TypeBinaryExpression typeBinaryExpr: + return typeBinaryExpr.Update(ReplaceChildWithConstant(typeBinaryExpr.Expression, cache)); + + case InvocationExpression invocationExpr: + return invocationExpr.Update( + ReplaceChildWithConstant(invocationExpr.Expression, cache), + invocationExpr.Arguments.Select(arg => ReplaceChildWithConstant(arg, cache))); + + case NewExpression newExpr: + return newExpr.Update(newExpr.Arguments.Select(arg => ReplaceChildWithConstant(arg, cache))); + + case NewArrayExpression newArrayExpr: + return newArrayExpr.Update(newArrayExpr.Expressions.Select(e => ReplaceChildWithConstant(e, cache))); + + case ListInitExpression listInitExpr: + NewExpression rebuiltNew = listInitExpr.NewExpression.Update( + listInitExpr.NewExpression.Arguments.Select(arg => ReplaceChildWithConstant(arg, cache))); + IEnumerable rebuiltInits = listInitExpr.Initializers.Select(init => + init.Update(init.Arguments.Select(arg => ReplaceChildWithConstant(arg, cache)))); + return listInitExpr.Update(rebuiltNew, rebuiltInits); + + case IndexExpression indexExpr when indexExpr.Object is not null: + Expression indexObj = ReplaceChildWithConstant(indexExpr.Object, cache); + return indexExpr.Update(indexObj, indexExpr.Arguments.Select(arg => ReplaceChildWithConstant(arg, cache))); + + case IndexExpression indexExpr: + // Object is null: leave as-is (static indexer scenario — not expressible in C# trees today). + return indexExpr; + + default: + // For other expressions or leaf nodes (constants, parameters), return as-is + return expr; + } + } + + /// + /// Returns a for when its value + /// has been successfully cached and is type-compatible with the child's declared type; + /// otherwise returns the original child expression unchanged. + /// + private static Expression ReplaceChildWithConstant(Expression child, Dictionary cache) + { + if (!cache.TryGetValue(child, out object? value)) + { + return child; + } + + if (ReferenceEquals(value, FailedToEvaluateSentinel)) + { + return child; + } + + if (value is null) + { + // null is assignable to any reference / nullable type — guard against value-type slots. + return !child.Type.IsValueType || Nullable.GetUnderlyingType(child.Type) is not null + ? Expression.Constant(null, child.Type) + : child; + } + + return !child.Type.IsInstanceOfType(value) ? child : Expression.Constant(value, child.Type); + } + + /// + /// 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); + + if (details.Count == 0) + { + return string.Empty; + } + + // Sort details alphabetically by variable name for consistent, readable output + IOrderedEnumerable> sortedDetails = details.OrderBy(kvp => kvp.Key, StringComparer.Ordinal); + + var sb = new StringBuilder(); + foreach (KeyValuePair kvp in sortedDetails) + { +#if NET + sb.AppendLine(CultureInfo.InvariantCulture, $" {kvp.Key} = {FormatValue(kvp.Value)}"); +#else + sb.AppendLine($" {kvp.Key} = {FormatValue(kvp.Value)}"); +#endif + } + + return sb.ToString(); + } + + /// + /// 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) { @@ -193,251 +620,258 @@ private static void AnalyzeExpression(Expression? expr, AnalysisContext context, { // Special handling for array indexing (myArray[index]) case BinaryExpression binaryExpr when binaryExpr.NodeType == ExpressionType.ArrayIndex: - AnalyzeArrayIndexExpression(binaryExpr, context); + HandleArrayIndexExpression(binaryExpr, details, evaluationCache); + break; + + case BinaryExpression binaryExpr when binaryExpr.NodeType is ExpressionType.AndAlso or ExpressionType.OrElse: + // Always walk both sides for diagnostics so users see every captured variable. + // We populate the cache for Right with a safe evaluation pass; the catch inside + // EvaluateAllSubExpressions stores a sentinel for sub-expressions that would + // throw (e.g., `s.Length` when s is null) and the detail helpers omit those. + // Side effects on Right run at most once total — only here, not during the + // single-pass assertion evaluation that already honored short-circuit. + ExtractVariablesFromExpression(binaryExpr.Left, details, evaluationCache, suppressIntermediateValues); + EvaluateAllSubExpressions(binaryExpr.Right, evaluationCache); + ExtractVariablesFromExpression(binaryExpr.Right, details, evaluationCache, suppressIntermediateValues); break; - // Assignment-style binary nodes (=, +=, -=, …) require writable left-hand operands. - // CaptureRewriter would replace the LHS with a Block, making it non-writable and - // causing the rewritten lambda to fail to compile. Skip these entirely. - case BinaryExpression binaryExpr when IsAssignmentNodeType(binaryExpr.NodeType): + case BinaryExpression binaryExpr when binaryExpr.NodeType == ExpressionType.Coalesce: + // Same rationale as AndAlso/OrElse: walk both sides for diagnostic completeness. + ExtractVariablesFromExpression(binaryExpr.Left, details, evaluationCache, suppressIntermediateValues); + EvaluateAllSubExpressions(binaryExpr.Right, evaluationCache); + ExtractVariablesFromExpression(binaryExpr.Right, details, evaluationCache, suppressIntermediateValues); break; case BinaryExpression binaryExpr: - AnalyzeExpression(binaryExpr.Left, context, suppressIntermediateValues); - AnalyzeExpression(binaryExpr.Right, context, suppressIntermediateValues); + // 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; case TypeBinaryExpression typeBinaryExpr: - AnalyzeExpression(typeBinaryExpr.Expression, context, suppressIntermediateValues); + // Extract variables from the expression being tested (e.g., 'obj' in 'obj is int') + 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"; - context.AddCapture(unaryExpr, lengthDisplayName); + 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) { - AnalyzeExpression(unaryExpr.Operand, context, suppressIntermediateValues); + ExtractVariablesFromExpression(unaryExpr.Operand, details, evaluationCache, suppressIntermediateValues); } break; - // Unary update nodes (++x, x++, --x, x--) require writable operands. CaptureRewriter - // would replace the operand with a Block, making it non-writable and causing compilation - // of the rewritten lambda to fail. Skip these entirely. - case UnaryExpression unaryExpr when IsUnaryUpdateNodeType(unaryExpr.NodeType): - break; - case UnaryExpression unaryExpr: - AnalyzeExpression(unaryExpr.Operand, context, suppressIntermediateValues); + // For other unary operations (e.g., !flag), extract the operand + ExtractVariablesFromExpression(unaryExpr.Operand, details, evaluationCache, suppressIntermediateValues); break; case MemberExpression memberExpr: - AnalyzeMemberExpression(memberExpr, context); + // For member access (e.g., obj.Property), add to details + AddMemberExpressionToDetails(memberExpr, details, evaluationCache); break; case MethodCallExpression callExpr: - AnalyzeMethodCallExpression(callExpr, context, suppressIntermediateValues); + // Handle method calls with special logic for indexers and boolean methods + HandleMethodCallExpression(callExpr, details, evaluationCache, suppressIntermediateValues); break; case ConditionalExpression conditionalExpr: - AnalyzeExpression(conditionalExpr.Test, context, suppressIntermediateValues); - AnalyzeExpression(conditionalExpr.IfTrue, context, suppressIntermediateValues); - AnalyzeExpression(conditionalExpr.IfFalse, context, suppressIntermediateValues); + // Walk all three parts for diagnostic completeness. The unselected branch is + // populated via a safe evaluation; sub-expressions that would throw are + // omitted (the sentinel handling in the detail helpers skips them). + ExtractVariablesFromExpression(conditionalExpr.Test, details, evaluationCache, suppressIntermediateValues); + EvaluateAllSubExpressions(conditionalExpr.IfTrue, evaluationCache); + ExtractVariablesFromExpression(conditionalExpr.IfTrue, details, evaluationCache, suppressIntermediateValues); + EvaluateAllSubExpressions(conditionalExpr.IfFalse, evaluationCache); + ExtractVariablesFromExpression(conditionalExpr.IfFalse, details, evaluationCache, suppressIntermediateValues); break; case InvocationExpression invocationExpr: - AnalyzeExpression(invocationExpr.Expression, context, suppressIntermediateValues); + // For delegate invocations, extract from the delegate and all arguments + ExtractVariablesFromExpression(invocationExpr.Expression, details, evaluationCache, suppressIntermediateValues); foreach (Expression argument in invocationExpr.Arguments) { - AnalyzeExpression(argument, context, suppressIntermediateValues); + ExtractVariablesFromExpression(argument, details, evaluationCache, suppressIntermediateValues); } break; case NewExpression newExpr: + // For object creation, extract from constructor arguments foreach (Expression argument in newExpr.Arguments) { - AnalyzeExpression(argument, context, suppressIntermediateValues); + ExtractVariablesFromExpression(argument, details, evaluationCache, suppressIntermediateValues); } + // 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); - context.AddCapture(newExpr, newExprDisplay); + TryAddExpressionValue(newExpr, newExprDisplay, details, evaluationCache); } break; case ListInitExpression listInitExpr: - AnalyzeExpression(listInitExpr.NewExpression, context, suppressIntermediateValues: true); + // 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) { foreach (Expression argument in initializer.Arguments) { - AnalyzeExpression(argument, context, suppressIntermediateValues); + ExtractVariablesFromExpression(argument, details, evaluationCache, suppressIntermediateValues); } } break; case NewArrayExpression newArrayExpr: + // For array creation, extract from all element expressions foreach (Expression expression in newArrayExpr.Expressions) { - AnalyzeExpression(expression, context, suppressIntermediateValues); + ExtractVariablesFromExpression(expression, details, evaluationCache, suppressIntermediateValues); } break; } } - private static void AnalyzeArrayIndexExpression(BinaryExpression arrayIndexExpr, AnalysisContext context) + /// + /// 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); - string indexValue = GetIndexArgumentDisplay(arrayIndexExpr.Right); + string indexValue = GetIndexArgumentDisplay(arrayIndexExpr.Right, evaluationCache); string indexerDisplay = $"{arrayName}[{indexValue}]"; - context.AddCapture(arrayIndexExpr, indexerDisplay); + TryAddExpressionValue(arrayIndexExpr, indexerDisplay, details, evaluationCache); - AnalyzeExpression(arrayIndexExpr.Right, context); + // Extract variables from the index argument + ExtractVariablesFromExpression(arrayIndexExpr.Right, details, evaluationCache); } - private static void AnalyzeMemberExpression(MemberExpression memberExpr, AnalysisContext context) + /// + /// 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) { - // Skip Func and Action delegates as they don't provide useful information in assertion failures. - // Use the static type so we don't have to evaluate the expression at analysis time. - if (IsFuncOrActionType(memberExpr.Type)) + // 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)) { return; } - string displayName = GetCleanMemberName(memberExpr); - context.AddCapture(memberExpr, displayName); + bool hasCachedValue = evaluationCache.TryGetValue(memberExpr, out object? cachedValue); + + // Skip sub-expressions that failed safe evaluation (e.g., NRE when walking the unreached + // branch of `s != null && s.Length > 0`). Surfacing them as "" would + // confuse users with a benign short-circuit. + if (hasCachedValue && ReferenceEquals(cachedValue, FailedToEvaluateSentinel)) + { + return; + } + + // Use cached value if available, otherwise mark as failed + details[displayName] = hasCachedValue + ? cachedValue + : FrameworkMessages.AssertThatFailedToEvaluate; - // 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) + // Skip Func and Action delegates as they don't provide useful information in assertion failures + if (IsFuncOrActionType(cachedValue?.GetType())) { - AnalyzeExpression(memberExpr.Expression, context, suppressIntermediateValues: true); + details.Remove(displayName); + return; + } + + // Only extract variables from the object being accessed if it's not a member expression + // or a method call (to avoid showing both "person" and "person.Name" or both + // "provider.GetBox()" and "provider.GetBox().Value" when the leaf access is sufficient + // — and to avoid surfacing intermediate side-effecting calls in failure details). + if (memberExpr.Expression is not null and not MemberExpression and not MethodCallExpression) + { + ExtractVariablesFromExpression(memberExpr.Expression, details, evaluationCache, suppressIntermediateValues: true); } } - private static void AnalyzeMethodCallExpression(MethodCallExpression callExpr, AnalysisContext context, bool suppressIntermediateValues = false) + /// + /// 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]); + string indexValue = GetIndexArgumentDisplay(callExpr.Arguments[0], evaluationCache); string indexerDisplay = $"{objectName}[{indexValue}]"; - context.AddCapture(callExpr, indexerDisplay); + TryAddExpressionValue(callExpr, indexerDisplay, details, evaluationCache); - AnalyzeExpression(callExpr.Arguments[0], context, suppressIntermediateValues); + // 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 (IsArrayGetMethod(callExpr)) + 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(GetIndexArgumentDisplay)); + string indexDisplay = string.Join(", ", callExpr.Arguments.Select(arg => GetIndexArgumentDisplay(arg, evaluationCache))); string indexerDisplay = $"{objectName}[{indexDisplay}]"; - context.AddCapture(callExpr, indexerDisplay); + TryAddExpressionValue(callExpr, indexerDisplay, details, evaluationCache); + // Extract variables from the index arguments but not from the object foreach (Expression argument in callExpr.Arguments) { - AnalyzeExpression(argument, context, suppressIntermediateValues); + ExtractVariablesFromExpression(argument, details, evaluationCache, suppressIntermediateValues); } } else { + // 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) { - AnalyzeExpression(callExpr.Object, context, suppressIntermediateValues); + // 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 { - string methodCallDisplay = GetMethodCallDisplayName(callExpr); - context.AddCapture(callExpr, methodCallDisplay); + // 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); + + // Don't extract from the object to avoid duplication } + // Always extract variables from the arguments foreach (Expression argument in callExpr.Arguments) { - AnalyzeExpression(argument, context, suppressIntermediateValues); + ExtractVariablesFromExpression(argument, details, evaluationCache, suppressIntermediateValues); } } } - /// - /// Builds a friendly display name for a method-call expression so the details message uses the same - /// syntax the user wrote. Static methods get prefixed with their declaring type's name; instance methods on - /// captured this render as this.Method(...); extension methods use the first argument as the - /// receiver. Fixes issue #6691. - /// - private static string GetMethodCallDisplayName(MethodCallExpression callExpr) - { - string methodName = callExpr.Method.Name; - - // Extension methods are static methods on a static class marked [Extension]; the receiver is the - // first argument. Render like the user wrote: receiver.Method(rest). - if (callExpr.Object is null - && callExpr.Method.IsDefined(typeof(ExtensionAttribute), inherit: false) - && callExpr.Arguments.Count > 0) - { - // Use IsCapturedThis with the first parameter's declared type so that - // Assert.That(() => this.SomeExtension() == x) renders as "this.SomeExtension(x)" - // rather than "<>4__this.SomeExtension(x)" or "TypeName.SomeExtension(x)". - Expression firstArg = callExpr.Arguments[0]; - Type receiverParamType = callExpr.Method.GetParameters()[0].ParameterType; - string receiver = IsCapturedThis(firstArg, receiverParamType) - ? "this" - : GetCleanMemberName(firstArg); - string extArgs = string.Join(", ", callExpr.Arguments.Skip(1).Select(static a => CleanExpressionText(a.ToString()))); - return $"{receiver}.{methodName}({extArgs})"; - } - - string argsStr = string.Join(", ", callExpr.Arguments.Select(static a => CleanExpressionText(a.ToString()))); - - if (callExpr.Object is null) - { - // Regular static method: use the declaring type's short name as the receiver display. - string typeName = callExpr.Method.DeclaringType is { } dt - ? CleanTypeName(dt.FullName ?? dt.Name) - : ""; - return $"{typeName}.{methodName}({argsStr})"; - } - - if (IsCapturedThis(callExpr.Object, callExpr.Method.DeclaringType)) - { - return $"this.{methodName}({argsStr})"; - } - - string objectDisplay = GetCleanMemberName(callExpr.Object); - return $"{objectDisplay}.{methodName}({argsStr})"; - } - - /// - /// Returns if is a reference to the enclosing - /// instance (this) — either accessed via the compiler-synthesized display-class field - /// (named like <>4__this) or as a representing - /// the enclosing instance (no closure case). For the constant form we require the expression's - /// static type to match its runtime type and be assignable to , - /// so inherited methods on this still render as this.Method(...) without - /// mis-labeling base-typed locals. - /// - private static bool IsCapturedThis(Expression objectExpr, Type? declaringType) - // Display-class field for captured this is named like "<>4__this". - => (objectExpr is MemberExpression me - && me.Member.Name.StartsWith("<>", StringComparison.Ordinal) - && me.Member.Name.EndsWith("__this", StringComparison.Ordinal)) - // No-closure case: the object is a ConstantExpression for the enclosing instance. - // We still guard against base-typed values by requiring ce.Type == runtime type. - || (declaringType is not null - && objectExpr is ConstantExpression ce - && ce.Value is not null - && ce.Type == ce.Value.GetType() - && declaringType.IsAssignableFrom(ce.Type)); - private static bool IsFuncOrActionType(Type? type) { if (type is null) @@ -447,205 +881,60 @@ 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); } - /// - /// Returns for expression kinds that are conventionally pure operand - /// reads (variable/property access, array indexers and lengths, collection indexers). These - /// can safely be re-evaluated on their own when they were skipped by short-circuit evaluation - /// of the root expression. Method calls and constructors are excluded because they may have - /// side effects (see issue #6690). - /// - /// - /// This heuristic intentionally treats (which covers both - /// fields and properties) and the well-known indexer-style method calls - /// (get_Item/Array.Get) as pure, even though property getters and user-defined - /// indexers can technically have side effects. This matches the pre-fix behavior — which - /// always re-evaluated those expressions — and ensures backward compatibility with existing - /// failure-detail output for short-circuited conditions like - /// name == "x" && obj.Property == y. Method calls with arbitrary names are - /// excluded because they are the common case of side-effecting code (the original motivation - /// for issue #6690). Safety is recursive: each child expression must also be safe. - /// - private static bool IsSafeToReevaluate(Expression expr) - => expr switch - { - ConstantExpression => true, - ParameterExpression => true, - MemberExpression memberExpr => memberExpr.Expression is null || IsSafeToReevaluate(memberExpr.Expression), - BinaryExpression { NodeType: ExpressionType.ArrayIndex } arrayIndexExpr - => IsSafeToReevaluate(arrayIndexExpr.Left) && IsSafeToReevaluate(arrayIndexExpr.Right), - UnaryExpression { NodeType: ExpressionType.ArrayLength } arrayLengthExpr - => IsSafeToReevaluate(arrayLengthExpr.Operand), - // Indexer-style method calls are conventionally pure reads; the previous implementation - // evaluated them eagerly too. Keep this limited to actual indexers and multidimensional - // array reads so arbitrary user-defined `Get(...)` methods are not re-invoked. - MethodCallExpression methodCallExpr when IsCollectionIndexerRead(methodCallExpr) - => IsMethodCallSafe(methodCallExpr), - _ => false, - }; - - private static bool IsMethodCallSafe(MethodCallExpression callExpr) - => (callExpr.Object is null || IsSafeToReevaluate(callExpr.Object)) - && callExpr.Arguments.All(IsSafeToReevaluate); - - private static bool IsCollectionIndexerRead(MethodCallExpression callExpr) - => callExpr.Method.Name == "get_Item" || IsArrayGetMethod(callExpr); - - private static bool IsArrayGetMethod(MethodCallExpression callExpr) - => callExpr.Method.Name == "Get" - && callExpr.Object is not null - // Multidimensional arrays (e.g. int[,]) expose runtime-synthesized `Get`/`Set`/`Address` - // methods on the array type itself — not on `System.Array` — so check the receiver type - // rather than the method's declaring type to catch them. - && callExpr.Object.Type.IsArray - && callExpr.Arguments.Count > 0; - - /// - /// Returns for binary values that represent - /// an assignment or compound-assignment operation (=, +=, -=, …). These require a writable - /// left-hand operand and must not be rewritten by . - /// - private static bool IsAssignmentNodeType(ExpressionType nodeType) - => nodeType is ExpressionType.Assign - or ExpressionType.AddAssign - or ExpressionType.AndAssign - or ExpressionType.DivideAssign - or ExpressionType.ExclusiveOrAssign - or ExpressionType.LeftShiftAssign - or ExpressionType.ModuloAssign - or ExpressionType.MultiplyAssign - or ExpressionType.OrAssign - or ExpressionType.PowerAssign - or ExpressionType.RightShiftAssign - or ExpressionType.SubtractAssign - or ExpressionType.AddAssignChecked - or ExpressionType.MultiplyAssignChecked - or ExpressionType.SubtractAssignChecked; + private static string GetCleanMemberName(Expression? expr) + => expr is null + ? NullAngleBrackets + : CleanExpressionText(expr.ToString()); /// - /// Returns for unary values that represent - /// an increment or decrement-assign operation (++x, x++, --x, x--). These require a writable - /// operand and must not be rewritten by . + /// 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 bool IsUnaryUpdateNodeType(ExpressionType nodeType) - => nodeType is ExpressionType.PreIncrementAssign - or ExpressionType.PreDecrementAssign - or ExpressionType.PostIncrementAssign - or ExpressionType.PostDecrementAssign; - - private sealed class AnalysisContext + private static string GetIndexArgumentDisplay(Expression indexArg, Dictionary evaluationCache) { -#pragma warning disable IDE0028 // Collection initialization can be simplified - Dictionary needs ReferenceEqualityComparer - public Dictionary CaptureMap { get; } = new(ReferenceEqualityComparer.Instance); -#pragma warning restore IDE0028 - - public List CaptureNames { get; } = []; - - public List CaptureExpressions { get; } = []; - - public void AddCapture(Expression expr, string name) + try { - // One slot per Expression instance, so duplicates of the same display name (e.g. `x + x`) - // and side-effecting calls that appear multiple times each get their own slot. - // First-occurrence-by-name wins at display-build time. - if (CaptureMap.ContainsKey(expr)) + // For constant values, just format the value + if (indexArg is ConstantExpression constExpr) { - return; + return FormatValue(constExpr.Value); } - // Cannot box void-typed values into the captures array; nothing useful to display anyway. - if (expr.Type == typeof(void)) + // For member expressions that are fields or simple variable references, + // preserve the variable name to help with readability (e.g., "myIndex" instead of "5") + if (indexArg is MemberExpression memberExpr && IsVariableReference(memberExpr)) { - return; + return CleanExpressionText(indexArg.ToString()); } - int index = CaptureNames.Count; - CaptureNames.Add(name); - CaptureExpressions.Add(expr); - CaptureMap[expr] = index; - } - } - - /// - /// Rewrites the lambda body so every captured sub-expression's value is stored into a captures array - /// as a side effect of the single root evaluation. Each captured node e at slot i is - /// replaced by { var t = e; captures[i] = (object)t; t }, so e is evaluated exactly once. - /// - private sealed class CaptureRewriter : ExpressionVisitor - { - private readonly Dictionary _captureMap; - private readonly ParameterExpression _arrayParam; - - public CaptureRewriter(Dictionary captureMap, ParameterExpression arrayParam) - { - _captureMap = captureMap; - _arrayParam = arrayParam; - } - - [return: NotNullIfNotNull(nameof(node))] - public override Expression? Visit(Expression? node) - { - if (node is not null && _captureMap.TryGetValue(node, out int index)) + // For parameter expressions (method parameters), preserve the parameter name + if (indexArg is ParameterExpression) { - // Visit children first so nested captures inside `node` are also rewritten and evaluated once. - // base.Visit dispatches to VisitX (Member/MethodCall/...), which recurses into children via this.Visit, - // so our override is consulted for every descendant. - Expression visited = base.Visit(node)!; - - ParameterExpression temp = Expression.Variable(visited.Type); - return Expression.Block( - visited.Type, - new[] { temp }, - Expression.Assign(temp, visited), - Expression.Assign( - Expression.ArrayAccess(_arrayParam, Expression.Constant(index)), - Expression.Convert(temp, typeof(object))), - temp); + return CleanExpressionText(indexArg.ToString()); } - return base.Visit(node); - } - } - -#if !NET - /// - /// Minimal stand-in for System.Collections.Generic.ReferenceEqualityComparer - /// (which is .NET 5+ only) so we can key dictionaries by reference - /// from netstandard2.0. - /// - private sealed class ReferenceEqualityComparer : IEqualityComparer - { - public static readonly ReferenceEqualityComparer Instance = new(); - - public bool Equals(Expression? x, Expression? y) => ReferenceEquals(x, y); - - public int GetHashCode(Expression obj) => RuntimeHelpers.GetHashCode(obj); - } -#endif - - private static string GetCleanMemberName(Expression? expr) - => expr is null - ? "" - : CleanExpressionText(expr.ToString()); + // For complex expressions (e.g., "i + 1"), preserve the expression text for clarity + if (!IsSimpleExpression(indexArg)) + { + return CleanExpressionText(indexArg.ToString()); + } - private static string GetIndexArgumentDisplay(Expression indexArg) - { - try - { - if (indexArg is ConstantExpression constExpr) + // For other simple expressions, try to use cached value + if (evaluationCache.TryGetValue(indexArg, out object? cachedValue)) { - return FormatValue(constExpr.Value); + return FormatValue(TranslateFailureSentinel(cachedValue)); } - // For complex index expressions, just use the expression string + // Fallback to expression text return CleanExpressionText(indexArg.ToString()); } catch @@ -654,32 +943,64 @@ private static string GetIndexArgumentDisplay(Expression indexArg) } } + /// + /// 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) + // 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 and can be evaluated + ConstantExpression => true, + // 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, + // 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, + }; + 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 @@ -688,6 +1009,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)) @@ -701,11 +1026,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]); @@ -751,6 +1076,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)) @@ -843,12 +1173,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])) @@ -877,12 +1207,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])) @@ -902,8 +1232,7 @@ private static bool TryMatchListInitPattern(string input, int startIndex, out st } private static string CleanTypeName(string typeName) - { - string cleanedTypeName = typeName switch + => typeName switch { "Int32" => "int", "Int64" => "long", @@ -941,22 +1270,10 @@ private static string CleanTypeName(string typeName) _ => typeName, }; - if (cleanedTypeName != typeName) - { - return cleanedTypeName; - } - - string[] nestedSegments = typeName.Split('+'); - for (int i = 0; i < nestedSegments.Length; i++) - { - string segment = nestedSegments[i]; - int lastDot = segment.LastIndexOf('.'); - nestedSegments[i] = lastDot >= 0 ? segment.Substring(lastDot + 1) : segment; - } - - return string.Join(".", nestedSegments); - } - + /// + /// 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)) @@ -983,6 +1300,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("(", StringComparison.Ordinal) || !input.EndsWith(")", StringComparison.Ordinal)) @@ -1013,6 +1334,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)) @@ -1037,7 +1362,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(currentChar, keepCount); i += count; } @@ -1051,6 +1376,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(); @@ -1058,8 +1387,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; } @@ -1071,9 +1400,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)) { @@ -1084,7 +1418,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] == '(') @@ -1099,24 +1433,90 @@ 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 or the cached value + /// is the failure sentinel. + /// + private static bool TryAddExpressionValue(Expression expr, string displayName, Dictionary details, Dictionary evaluationCache) + { + // Use cached value if available + if (evaluationCache.TryGetValue(expr, out object? cachedValue)) + { + // Skip sub-expressions that failed safe evaluation (e.g., NRE when walking the + // unreached branch of a short-circuited operator). Surfacing them as + // "" would be misleading for benign short-circuits. + if (ReferenceEquals(cachedValue, FailedToEvaluateSentinel)) + { + return false; + } + + // If the key already exists, check if it has the same value + if (details.TryGetValue(displayName, out object? existingValue)) + { + // If values are different, add with a suffix to show multiple evaluations + if (!Equals(existingValue, cachedValue)) + { + int counter = 2; + string numberedName; + do + { + numberedName = $"{displayName} ({counter})"; + counter++; + } + while (details.ContainsKey(numberedName)); + + details[numberedName] = cachedValue; + return true; + } + + // Same value, don't add duplicate + return false; + } + + details[displayName] = cachedValue; + return true; + } + + // Don't add if key already exists and we don't have a cached value + if (details.ContainsKey(displayName)) + { + return false; + } + + // Mark as failed if we couldn't evaluate it + details[displayName] = FrameworkMessages.AssertThatFailedToEvaluate; + return true; + } + + /// + /// Converts the internal failure sentinel into the localized + /// display string. Any other + /// value (including ) is returned unchanged. + /// + private static object? TranslateFailureSentinel(object? value) + => ReferenceEquals(value, FailedToEvaluateSentinel) + ? FrameworkMessages.AssertThatFailedToEvaluate + : value; + #if NET - [GeneratedRegex(@"[A-Za-z0-9_\.]+(?:\+[A-Za-z0-9_\.]+)*\+<>c__DisplayClass\d+_\d+\.(\w+(?:\.\w+)*(?:\[[^\]]+\])?)")] + [GeneratedRegex(@"[A-Za-z0-9_\.]+\+<>c__DisplayClass\d+_\d+\.(\w+(?:\.\w+)*(?:\[[^\]]+\])?)")] private static partial Regex CompilerGeneratedDisplayClassRegex(); #else private static Regex CompilerGeneratedDisplayClassRegex() - => new(@"[A-Za-z0-9_\.]+(?:\+[A-Za-z0-9_\.]+)*\+<>c__DisplayClass\d+_\d+\.(\w+(?:\.\w+)*(?:\[[^\]]+\])?)", RegexOptions.Compiled); + => new(@"[A-Za-z0-9_\.]+\+<>c__DisplayClass\d+_\d+\.(\w+(?:\.\w+)*(?:\[[^\]]+\])?)", RegexOptions.Compiled); #endif } diff --git a/src/TestFramework/TestFramework/Resources/FrameworkMessages.resx b/src/TestFramework/TestFramework/Resources/FrameworkMessages.resx index 2b2cdee2f4..266be57571 100644 --- a/src/TestFramework/TestFramework/Resources/FrameworkMessages.resx +++ b/src/TestFramework/TestFramework/Resources/FrameworkMessages.resx @@ -1,17 +1,17 @@ - @@ -454,6 +454,9 @@ Actual: {2} The maximum value must be greater than or equal to the minimum value. + + <Failed to evaluate> + {0} assertion(s) failed within the assert scope. {0} is the number of assertion failures collected in the scope. diff --git a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.cs.xlf b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.cs.xlf index a4e00e24cf..57b7bc65c3 100644 --- a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.cs.xlf +++ b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.cs.xlf @@ -268,6 +268,11 @@ Podrobnosti: + + <Failed to evaluate> + <Failed to evaluate> + + Message: {0} Zpráva: {0} diff --git a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.de.xlf b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.de.xlf index d1d1f553cd..cfaf34a42e 100644 --- a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.de.xlf +++ b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.de.xlf @@ -268,6 +268,11 @@ Details: + + <Failed to evaluate> + <Failed to evaluate> + + Message: {0} Meldung: {0} diff --git a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.es.xlf b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.es.xlf index afec17d9d4..fc28fe7090 100644 --- a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.es.xlf +++ b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.es.xlf @@ -268,6 +268,11 @@ Detalles: + + <Failed to evaluate> + <Failed to evaluate> + + Message: {0} Mensaje: {0} diff --git a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.fr.xlf b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.fr.xlf index 86add49667..f698792467 100644 --- a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.fr.xlf +++ b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.fr.xlf @@ -268,6 +268,11 @@ Détails : + + <Failed to evaluate> + <Failed to evaluate> + + Message: {0} Message : {0} diff --git a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.it.xlf b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.it.xlf index 4032c7d044..f825d12ef4 100644 --- a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.it.xlf +++ b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.it.xlf @@ -268,6 +268,11 @@ Dettagli: + + <Failed to evaluate> + <Failed to evaluate> + + Message: {0} Messaggio: {0} diff --git a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.ja.xlf b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.ja.xlf index 7859becd1d..e45235428c 100644 --- a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.ja.xlf +++ b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.ja.xlf @@ -268,6 +268,11 @@ 詳細: + + <Failed to evaluate> + <Failed to evaluate> + + Message: {0} メッセージ: {0} diff --git a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.ko.xlf b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.ko.xlf index 8508782d7a..ca3823b703 100644 --- a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.ko.xlf +++ b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.ko.xlf @@ -268,6 +268,11 @@ 세부 정보: + + <Failed to evaluate> + <Failed to evaluate> + + Message: {0} 메시지: {0} diff --git a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.pl.xlf b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.pl.xlf index cbd31f9879..a404aafff7 100644 --- a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.pl.xlf +++ b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.pl.xlf @@ -268,6 +268,11 @@ Szczegóły: + + <Failed to evaluate> + <Failed to evaluate> + + Message: {0} Komunikat: {0} diff --git a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.pt-BR.xlf b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.pt-BR.xlf index ef782f8883..101d97ed86 100644 --- a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.pt-BR.xlf +++ b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.pt-BR.xlf @@ -268,6 +268,11 @@ Detalhes: + + <Failed to evaluate> + <Failed to evaluate> + + Message: {0} Mensagem: {0} diff --git a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.ru.xlf b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.ru.xlf index a3551e964a..4965fb5de6 100644 --- a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.ru.xlf +++ b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.ru.xlf @@ -268,6 +268,11 @@ Подробности: + + <Failed to evaluate> + <Failed to evaluate> + + Message: {0} Сообщение: {0} diff --git a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.tr.xlf b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.tr.xlf index 58c1f882ac..3b7b927a75 100644 --- a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.tr.xlf +++ b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.tr.xlf @@ -268,6 +268,11 @@ Ayrıntılar: + + <Failed to evaluate> + <Failed to evaluate> + + Message: {0} İleti: {0} diff --git a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.zh-Hans.xlf b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.zh-Hans.xlf index 847cbe4319..2a98a35fe8 100644 --- a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.zh-Hans.xlf +++ b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.zh-Hans.xlf @@ -268,6 +268,11 @@ 详细信息: + + <Failed to evaluate> + <Failed to evaluate> + + Message: {0} 消息: {0} diff --git a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.zh-Hant.xlf b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.zh-Hant.xlf index b1c4045fbb..23be632390 100644 --- a/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.zh-Hant.xlf +++ b/src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.zh-Hant.xlf @@ -268,6 +268,11 @@ 詳細資料: + + <Failed to evaluate> + <Failed to evaluate> + + Message: {0} 訊息: {0} diff --git a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs index b3a5256c6b..d0a9d9e004 100644 --- a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs +++ b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs @@ -1,9 +1,6 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. -using System.Linq.Expressions; -using System.Reflection; - using AwesomeAssertions; using TestFramework.ForTestingMSTest; @@ -482,10 +479,10 @@ public void That_NewExpression_ExtractsVariablesCorrectly() .WithMessage($""" Assert.That(() => new DateTime(year, month, day) == DateTime.MinValue) failed. Details: - DateTime.MinValue = {DateTime.MinValue.ToString(CultureInfo.CurrentCulture)} + DateTime.MinValue = {DateTime.MinValue.ToString(CultureInfo.InvariantCulture)} day = 25 month = 12 - new DateTime(year, month, day) = {new DateTime(year, month, day).ToString(CultureInfo.CurrentCulture)} + new DateTime(year, month, day) = {new DateTime(year, month, day).ToString(CultureInfo.InvariantCulture)} year = 2023 """); } @@ -1046,447 +1043,238 @@ public void That_WithNullConstantAndVariable_OnlyIncludesVariable() """); } - // ---- Tests for issue #6690 (single-pass evaluation) ------------------------------------ - private sealed class Counter - { - public int CallCount { get; private set; } - - public int GetNumber() - { - CallCount++; - return CallCount; - } - - public int Get(int value) - { - CallCount++; - return value; - } - } - - public void That_ExpressionWithSideEffect_EvaluatesOnlyOnce() + public void That_DoesNotEvaluateTwice_WhenAssertionFails() { - // Reproduces issue #6690: prior to the fix, Assert.That evaluated the expression once to - // determine the boolean result, then re-compiled and re-invoked each sub-expression for - // reporting. With a stateful method like Counter.GetNumber(), this produced a misleading - // details string where the reported value differed from the value actually compared. - var box = new Counter(); - int expected = 2; + var box = new Box(); - Action act = () => Assert.That(() => box.GetNumber() == expected); + // If we evaluate twice, box.GetValueWithSideEffect() 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.GetValueWithSideEffect() == 0); - act.Should().Throw(); + // GetValueWithSideEffect() should report 1, which is the value when we evaluate only once. + act.Should().Throw() + .WithMessage(""" + Assert.That(() => box.GetValueWithSideEffect() == 0) failed. + Details: + box.GetValueWithSideEffect() = 1 + """); - // Each textual occurrence of GetNumber() should have been evaluated exactly once. - box.CallCount.Should().Be(1); + // We call again, this should be second call now. + box.GetValueWithSideEffect().Should().Be(2); } - public void That_ExpressionWithSideEffect_FailureMessageReflectsFirstEvaluation() + public void That_DoesNotEvaluateTwice_WhenAssertionFails_NoSideEffect() { - var box = new Counter(); - int expected = 2; - - Action act = () => Assert.That(() => box.GetNumber() == expected); + int i = 1; + Action act = () => Assert.That(() => i + i == 0); act.Should().Throw() - .WithMessage( - """ - Assert.That(() => box.GetNumber() == expected) failed. - Details: - box.GetNumber() = 1 - expected = 2 - """); + .WithMessage(""" + Assert.That(() => i + i == 0) failed. + Details: + i = 1 + """); } - public void That_ExpressionWithSideEffect_AppearingTwice_EvaluatesEachOccurrenceOnce() + public void That_DoesEvaluateTwice_WhenMethodIsLeaf() { - // Each textual occurrence of `box.GetNumber()` is evaluated naturally (once each), - // matching what the user wrote. There must be no extra evaluations from the framework. - // The reported value uses first-occurrence-wins-by-name semantics. - var box = new Counter(); + var box = new Box(); - Action act = () => Assert.That(() => box.GetNumber() == box.GetNumber()); + // Compare to 0 to force failure. + Action act = () => Assert.That(() => box.GetValueWithSideEffect() + box.GetValueWithSideEffect() == 0); act.Should().Throw() - .WithMessage( - """ - Assert.That(() => box.GetNumber() == box.GetNumber()) failed. - Details: - box.GetNumber() = 1 - """); - - // Two textual occurrences in the expression -> exactly two invocations. - box.CallCount.Should().Be(2); + .WithMessage(""" + Assert.That(() => box.GetValueWithSideEffect() + box.GetValueWithSideEffect() == 0) failed. + Details: + box.GetValueWithSideEffect() = 1 + box.GetValueWithSideEffect() (2) = 2 + """); } - public void That_ShortCircuitedExpression_StillReportsBothOperands() + public void That_FastPathFailure_StillExtractsDetails() { - // When `&&` short-circuits, the right-hand side isn't evaluated by the rewritten lambda. - // For reporting we still want to show pure operand values, so the implementation falls back - // to evaluating just the un-captured *variable* sub-expression on its own. - bool a = false; - int b = 5; + int x = 3; - Action act = () => Assert.That(() => a && b > 10); + Action act = () => Assert.That(() => x == 5); act.Should().Throw() - .WithMessage( - """ - Assert.That(() => a && b > 10) failed. - Details: - a = False - b = 5 - """); + .WithMessage(""" + Assert.That(() => x == 5) failed. + Details: + x = 3 + """); } - public void That_ShortCircuitedExpression_DoesNotEvaluateUnreachedSideEffect() + public void That_FieldAccessWithSideEffectingParent_DoesNotEvaluateTwice() { - // The right-hand side of `&&` must not be invoked when the left side is false, even after - // our rewriting. The framework must not "fall back" to evaluating side-effecting calls - // that the user's expression intentionally skipped via short-circuit. - var box = new Counter(); - bool a = false; + var provider = new BoxProvider(); - Action act = () => Assert.That(() => a && box.GetNumber() > 0); + Action act = () => Assert.That(() => provider.GetBox().Value == 0); - act.Should().Throw(); + act.Should().Throw() + .WithMessage(""" + Assert.That(() => provider.GetBox().Value == 0) failed. + Details: + provider.GetBox().Value = 1 + """); - // Counter must not have been called. - box.CallCount.Should().Be(0); + provider.Calls.Should().Be(1); } - public void That_ShortCircuitedExpression_DoesNotReevaluateArbitraryGetMethod() + public void That_AndAlso_ShortCircuits_DoesNotEvaluateRightWhenLeftIsFalse() { - var box = new Counter(); - bool a = false; - int expected = 2; + string? s = null; - Action act = () => Assert.That(() => a && box.Get(expected) == expected); + // s != null is false; the right operand s.Length > 0 must NOT execute on the assertion + // path (it would NRE) and the diagnostic walk must not surface "" + // for the benign short-circuit. + Action act = () => Assert.That(() => s != null && s.Length > 0); act.Should().Throw() - .WithMessage( - """ - Assert.That(() => a && box.Get(expected) == expected) failed. - Details: - a = False - expected = 2 - """); - box.CallCount.Should().Be(0); + .WithMessage(""" + Assert.That(() => s != null && s.Length > 0) failed. + Details: + s = null + """); } - private sealed class SideEffectingReceiverFactory + public void That_OrElse_ShortCircuit_PassingAssertion_DoesNotInvokeRight() { - public int GetObjectCallCount { get; private set; } + var counter = new SideEffectCounter(); + bool leftTrue = true; - public int GetListCallCount { get; private set; } + // Assertion passes because leftTrue is true; counter.Increment() must NOT run. + Action act = () => Assert.That(() => leftTrue || counter.Increment() > 0); - public SideEffectingReceiverObject GetObject() - { - GetObjectCallCount++; - return new SideEffectingReceiverObject(); - } - - public List GetList() - { - GetListCallCount++; - return [42]; - } - } - - private sealed class SideEffectingReceiverObject - { - public int Property => 42; + act.Should().NotThrow(); + counter.Count.Should().Be(0); } - public void That_ShortCircuitedExpression_DoesNotReevaluateMemberExpressionWithSideEffectingReceiver() + public void That_Coalesce_ShortCircuit_PassingAssertion_DoesNotInvokeRight() { - var factory = new SideEffectingReceiverFactory(); + var counter = new SideEffectCounter(); + string? value = "hello"; - Action act = () => Assert.That(() => false && factory.GetObject().Property == 0); + // Non-null value short-circuits the null-coalescing operator; counter.Increment() must NOT run. + Action act = () => Assert.That(() => (value ?? counter.Increment().ToString()) == "hello"); - act.Should().Throw(); - factory.GetObjectCallCount.Should().Be(0); + act.Should().NotThrow(); + counter.Count.Should().Be(0); } - public void That_ShortCircuitedExpression_DoesNotReevaluateIndexerWithSideEffectingReceiver() + public void That_Conditional_PassingAssertion_DoesNotInvokeUnselectedBranch() { - var factory = new SideEffectingReceiverFactory(); + var counter = new SideEffectCounter(); + bool useTrueBranch = true; - Action act = () => Assert.That(() => false && factory.GetList()[0] == 0); + // useTrueBranch picks the IfTrue side; counter.Increment() in IfFalse must NOT run. + Action act = () => Assert.That(() => (useTrueBranch ? 1 : counter.Increment()) == 1); - act.Should().Throw(); - factory.GetListCallCount.Should().Be(0); + act.Should().NotThrow(); + counter.Count.Should().Be(0); } - public void That_ArbitraryGetMethod_RendersAsMethodCall_NotIndexer() + public void That_PropagatesUserExceptions_OnSinglePassPath() { - var box = new Counter(); - int expected = 2; - - Action act = () => Assert.That(() => box.Get(expected) == 3); - - act.Should().Throw() - .WithMessage( - """ - Assert.That(() => box.Get(expected) == 3) failed. - Details: - box.Get(expected) = 2 - expected = 2 - """); - } - - // ---- Tests for issue #6691 (method call display names) --------------------------------- - public string SameClassInstanceMethod() => "Giraffe"; + var thrower = new Thrower(); - public static string SameClassStaticMethod() => "Giraffe"; - - private sealed class Zoo - { - public string GetAnimal() => "Giraffe"; + // The lambda throws InvalidOperationException; previously the single-pass evaluator + // would swallow it and turn it into a confusing InvalidCastException ("" -> bool). + // Now the original exception must surface unchanged. + Action act = () => Assert.That(() => thrower.Throw()); - public static string GetAnimalStatic() => "Giraffe"; + act.Should().Throw() + .WithMessage("boom"); } - public void That_InstanceMethodOnSameType_RendersAsThis() + public void That_NewExpressionWithSideEffectingArgument_DoesNotEvaluateTwice() { - // Reproduces issue #6691: previously the instance method on the enclosing test class was - // shown with its full type name (e.g. "Namespace.AssertTests.SameClassInstanceMethod()"). - // It should render as "this.SameClassInstanceMethod()" instead. - string animal = string.Empty; + var counter = new SideEffectCounter(); - Action act = () => Assert.That(() => SameClassInstanceMethod() == animal); + Action act = () => Assert.That(() => new Wrapper(counter.Increment()).Value == 99); - act.Should().Throw() - .WithMessage( - """ - Assert.That(() => SameClassInstanceMethod() == animal) failed. - Details: - animal = "" - this.SameClassInstanceMethod() = "Giraffe" - """); + act.Should().Throw(); + counter.Count.Should().Be(1); } - public void That_StaticMethodOnSameType_RendersWithTypeName() + public void That_NewArrayWithSideEffectingElement_DoesNotEvaluateTwice() { - // Static method on the enclosing test class used to render without any type qualifier - // (just "SameClassStaticMethod()"). It should include the declaring type name. - string animal = string.Empty; + var counter = new SideEffectCounter(); - Action act = () => Assert.That(() => SameClassStaticMethod() == animal); + Action act = () => Assert.That(() => new[] { counter.Increment() }.Length == 99); - act.Should().Throw() - .WithMessage( - """ - Assert.That(() => SameClassStaticMethod() == animal) failed. - Details: - AssertTests.SameClassStaticMethod() = "Giraffe" - animal = "" - """); + act.Should().Throw(); + counter.Count.Should().Be(1); } - public void That_InstanceMethodOnOtherType_RendersWithObjectName() + public void That_InvocationWithSideEffectingArgument_DoesNotEvaluateTwice() { - // The "happy path" that was already correct: instance method on a local variable. - string animal = string.Empty; - var zoo = new Zoo(); + var counter = new SideEffectCounter(); + Func identity = x => x; - Action act = () => Assert.That(() => zoo.GetAnimal() == animal); + Action act = () => Assert.That(() => identity(counter.Increment()) == 99); - act.Should().Throw() - .WithMessage( - """ - Assert.That(() => zoo.GetAnimal() == animal) failed. - Details: - animal = "" - zoo.GetAnimal() = "Giraffe" - """); + act.Should().Throw(); + counter.Count.Should().Be(1); } - public void That_StaticMethodOnOtherType_RendersWithTypeName() + public void That_QueryableWithQuotedLambda_FailsAsAssertionFailure() { - // Static method on a non-enclosing class should include its type name. - string animal = string.Empty; + int[] items = [1, 2, 3]; - Action act = () => Assert.That(() => Zoo.GetAnimalStatic() == animal); + // The Where/Any call uses a quoted lambda; the previous walker tried to unwrap the Quote + // and produced an InvalidCastException instead of an AssertFailedException. + Action act = () => Assert.That(() => items.AsQueryable().Any(i => i > 99)); - act.Should().Throw() - .WithMessage( - """ - Assert.That(() => Zoo.GetAnimalStatic() == animal) failed. - Details: - AssertTests.Zoo.GetAnimalStatic() = "Giraffe" - animal = "" - """); - } - - private class AnimalBase - { - public string Whisper() => "Whisper"; + act.Should().Throw(); } - private class InheritedMethodBase + private sealed class SideEffectCounter { - public string InheritedWhisper() => "Whisper"; - } + public int Count { get; private set; } - private sealed class InheritedMethodProbe : InheritedMethodBase - { - public void AssertInheritedMethodRendersAsThis() + public int Increment() { - string expected = "Roar"; - - Action act = () => Assert.That(() => InheritedWhisper() == expected); - - act.Should().Throw() - .WithMessage( - """ - Assert.That(() => InheritedWhisper() == expected) failed. - Details: - expected = "Roar" - this.InheritedWhisper() = "Whisper" - """); + Count++; + return Count; } } - private sealed class Cat : AnimalBase + private sealed class Thrower { + public bool Throw() => throw new InvalidOperationException("boom"); } - public void That_InstanceMethodOnLocalOfBaseType_DoesNotRenderAsThis() - { - // Regression guard for IsCapturedThis precision: when the asserted method is declared on - // a base type AND the receiver is a local variable of that base type, we must render the - // local's name (not "this"), even if the local happens to be assignable to the method's - // declaring type. Previously a loose `IsInstanceOfType` check could mislabel this case. - AnimalBase pet = new Cat(); - string expected = "Roar"; - - Action act = () => Assert.That(() => pet.Whisper() == expected); - - act.Should().Throw() - .WithMessage( - """ - Assert.That(() => pet.Whisper() == expected) failed. - Details: - expected = "Roar" - pet.Whisper() = "Whisper" - """); - } - - public void That_InheritedInstanceMethodOnThis_RendersAsThis() + private sealed class Wrapper(int value) { - var probe = new InheritedMethodProbe(); - - probe.AssertInheritedMethodRendersAsThis(); + public int Value { get; } = value; } - // ---- Documented-trade-off test: properties in short-circuited branches are re-evaluated ---- - private sealed class PropertyCounter + private class Box { - public int GetCount { get; private set; } + private int _c; - public int Value + public int GetValueWithSideEffect() { - get - { - GetCount++; - return 42; - } + _c++; + return _c; } } - public void That_ShortCircuitedPropertyAccess_IsReevaluatedForReporting() + private sealed class BoxProvider { - // This locks down the documented trade-off in IsSafeToReevaluate: when a property - // appears in a short-circuited branch, the failure path re-evaluates it so it appears - // in the details message. Property getters CAN have side effects, so this is a - // deliberate compromise — it matches the pre-fix behavior and keeps the failure message - // informative for the common case of pure getters. - var counter = new PropertyCounter(); - - Action act = () => Assert.That(() => false && counter.Value == 0); - - act.Should().Throw() - .WithMessage( - """ - Assert.That(() => false && counter.Value == 0) failed. - Details: - counter.Value = 42 - """); + public int Calls { get; private set; } - // The property was invoked once by the fallback path (the root short-circuited). - counter.GetCount.Should().Be(1); - } - - // ---- Extension method on `this` renders as this.Method(...) (issue #6691) ---------------- - public void That_ExtensionMethodOnThis_RendersAsThis() - { - // Regression guard for IsCapturedThis in the extension method path: - // an extension method called on `this` must render as "this.GetDisplayName()" - // rather than the raw display-class field ("<>4__this.GetDisplayName()") or - // a type name prefix ("AssertTests.GetDisplayName()"). - string expected = "WrongName"; - - Action act = () => Assert.That(() => this.GetDisplayName() == expected); - - act.Should().Throw() - .WithMessage( - """ - Assert.That(() => this.GetDisplayName() == expected) failed. - Details: - expected = "WrongName" - this.GetDisplayName() = "MyTestClass" - """); - } - - // ---- Assignment / update expression trees must not cause compilation failures ----------- - // C# expression-tree lambdas do not allow assignment operators (the compiler emits CS0832), - // so these nodes can only appear in manually-constructed expression trees. The fix ensures - // CaptureRewriter skips them rather than wrapping a writable LHS in a non-writable Block. - private sealed class MutableBox - { -#pragma warning disable SA1401 // Fields should be private - intentional: this type tests Expression.Assign on a field. - public int Value; -#pragma warning restore SA1401 - } - - public void That_ManuallyConstructedAssignExpression_DoesNotThrow() - { - // Construct: (box.Value = 5) > 0 — passes, so no AssertFailedException. - var box = new MutableBox(); - FieldInfo fi = typeof(MutableBox).GetField(nameof(MutableBox.Value))!; - MemberExpression field = Expression.Field(Expression.Constant(box), fi); - BinaryExpression assign = Expression.Assign(field, Expression.Constant(5)); - BinaryExpression body = Expression.GreaterThan(assign, Expression.Constant(0)); - var lambda = Expression.Lambda>(body); - - Action act = () => Assert.That(lambda); - - // Must not throw a compilation error (InvalidOperationException / InvalidProgramException). - act.Should().NotThrow(); - box.Value.Should().Be(5); + public ValueBox GetBox() + { + Calls++; + return new ValueBox(Calls); + } } - public void That_ManuallyConstructedPreIncrementAssignExpression_DoesNotThrow() + private readonly struct ValueBox(int value) { - // Construct: ++box.Value > 0 — passes, so no AssertFailedException. - var box = new MutableBox { Value = 5 }; - FieldInfo fi = typeof(MutableBox).GetField(nameof(MutableBox.Value))!; - MemberExpression field = Expression.Field(Expression.Constant(box), fi); - UnaryExpression preInc = Expression.PreIncrementAssign(field); - BinaryExpression body = Expression.GreaterThan(preInc, Expression.Constant(0)); - var lambda = Expression.Lambda>(body); - - Action act = () => Assert.That(lambda); - - // Must not throw a compilation error. - act.Should().NotThrow(); - box.Value.Should().Be(6); + public int Value { get; } = value; } } - -internal static class AssertTestsExtensions -{ - /// Helper for the That_ExtensionMethodOnThis_RendersAsThis test. - public static string GetDisplayName(this AssertTests _) => "MyTestClass"; -}