diff --git a/src/TestFramework/TestFramework/Assertions/Assert.That.cs b/src/TestFramework/TestFramework/Assertions/Assert.That.cs index 382a53f397..08f08673c6 100644 --- a/src/TestFramework/TestFramework/Assertions/Assert.That.cs +++ b/src/TestFramework/TestFramework/Assertions/Assert.That.cs @@ -33,7 +33,10 @@ public static void That(Expression> condition, string? message = null throw new ArgumentNullException(nameof(condition)); } - if (condition.Compile()()) + var details = new Dictionary(); + bool result = EvaluateAndCollectDetails(condition.Body, details); + + if (result) { return; } @@ -47,8 +50,8 @@ public static void That(Expression> condition, string? message = null sb.AppendLine(string.Format(CultureInfo.InvariantCulture, FrameworkMessages.AssertThatMessageFormat, message)); } - string details = ExtractDetails(condition.Body); - if (!string.IsNullOrWhiteSpace(details)) + string detailsString = BuildDetailsString(details); + if (!string.IsNullOrWhiteSpace(detailsString)) { if (sb.Length == 0) { @@ -56,20 +59,15 @@ public static void That(Expression> condition, string? message = null } sb.AppendLine(FrameworkMessages.AssertThatDetailsPrefix); - sb.AppendLine(details); + sb.AppendLine(detailsString); } Assert.ReportAssertFailed($"Assert.That({expressionText})", sb.ToString().TrimEnd()); } } -#pragma warning disable IDE0051 // Remove unused private members - false positive - private static string ExtractDetails(Expression expr) -#pragma warning restore IDE0051 // Remove unused private members + private static string BuildDetailsString(Dictionary details) { - var details = new Dictionary(); - ExtractVariablesFromExpression(expr, details); - if (details.Count == 0) { return string.Empty; @@ -91,7 +89,100 @@ private static string ExtractDetails(Expression expr) return sb.ToString(); } - private static void ExtractVariablesFromExpression(Expression? expr, Dictionary details, bool suppressIntermediateValues = false) + private static readonly object UnsetCapture = new(); + + /// + /// 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. + /// + /// if the condition evaluated to . + private static bool EvaluateAndCollectDetails(Expression body, Dictionary details) + { + // 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++) + { + values[i] = UnsetCapture; + } + + bool result = lambda.Compile()(values); + + if (result) + { + return true; + } + + // 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)) + { + continue; + } + + 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 + { + value = Expression + .Lambda>(Expression.Convert(expr, typeof(object))) + .Compile()(); + } + catch + { + value = ""; + } + } + else + { + // Skip potentially side-effecting captures that were short-circuited. + continue; + } + } + + if (IsFuncOrActionType(value?.GetType())) + { + continue; + } + + details[name] = value; + } + + return false; + } + + private static void AnalyzeExpression(Expression? expr, AnalysisContext context, bool suppressIntermediateValues = false) { if (expr is null) { @@ -102,55 +193,66 @@ private static void ExtractVariablesFromExpression(Expression? expr, Dictionary< { // Special handling for array indexing (myArray[index]) case BinaryExpression binaryExpr when binaryExpr.NodeType == ExpressionType.ArrayIndex: - HandleArrayIndexExpression(binaryExpr, details); + AnalyzeArrayIndexExpression(binaryExpr, context); + 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): break; case BinaryExpression binaryExpr: - ExtractVariablesFromExpression(binaryExpr.Left, details, suppressIntermediateValues); - ExtractVariablesFromExpression(binaryExpr.Right, details, suppressIntermediateValues); + AnalyzeExpression(binaryExpr.Left, context, suppressIntermediateValues); + AnalyzeExpression(binaryExpr.Right, context, suppressIntermediateValues); break; case TypeBinaryExpression typeBinaryExpr: - // Extract variables from the expression being tested (e.g., 'obj' in 'obj is int') - ExtractVariablesFromExpression(typeBinaryExpr.Expression, details, suppressIntermediateValues); + AnalyzeExpression(typeBinaryExpr.Expression, context, suppressIntermediateValues); break; // Special handling for ArrayLength expressions case UnaryExpression unaryExpr when unaryExpr.NodeType == ExpressionType.ArrayLength: string arrayName = GetCleanMemberName(unaryExpr.Operand); string lengthDisplayName = $"{arrayName}.Length"; - TryAddExpressionValue(unaryExpr, lengthDisplayName, details); + context.AddCapture(unaryExpr, lengthDisplayName); if (unaryExpr.Operand is not MemberExpression) { - ExtractVariablesFromExpression(unaryExpr.Operand, details, suppressIntermediateValues); + AnalyzeExpression(unaryExpr.Operand, context, 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: - ExtractVariablesFromExpression(unaryExpr.Operand, details, suppressIntermediateValues); + AnalyzeExpression(unaryExpr.Operand, context, suppressIntermediateValues); break; case MemberExpression memberExpr: - AddMemberExpressionToDetails(memberExpr, details); + AnalyzeMemberExpression(memberExpr, context); break; case MethodCallExpression callExpr: - HandleMethodCallExpression(callExpr, details, suppressIntermediateValues); + AnalyzeMethodCallExpression(callExpr, context, suppressIntermediateValues); break; case ConditionalExpression conditionalExpr: - ExtractVariablesFromExpression(conditionalExpr.Test, details, suppressIntermediateValues); - ExtractVariablesFromExpression(conditionalExpr.IfTrue, details, suppressIntermediateValues); - ExtractVariablesFromExpression(conditionalExpr.IfFalse, details, suppressIntermediateValues); + AnalyzeExpression(conditionalExpr.Test, context, suppressIntermediateValues); + AnalyzeExpression(conditionalExpr.IfTrue, context, suppressIntermediateValues); + AnalyzeExpression(conditionalExpr.IfFalse, context, suppressIntermediateValues); break; case InvocationExpression invocationExpr: - ExtractVariablesFromExpression(invocationExpr.Expression, details, suppressIntermediateValues); + AnalyzeExpression(invocationExpr.Expression, context, suppressIntermediateValues); foreach (Expression argument in invocationExpr.Arguments) { - ExtractVariablesFromExpression(argument, details, suppressIntermediateValues); + AnalyzeExpression(argument, context, suppressIntermediateValues); } break; @@ -158,26 +260,24 @@ private static void ExtractVariablesFromExpression(Expression? expr, Dictionary< case NewExpression newExpr: foreach (Expression argument in newExpr.Arguments) { - ExtractVariablesFromExpression(argument, details, suppressIntermediateValues); + AnalyzeExpression(argument, context, suppressIntermediateValues); } - // Don't display the new object value if we're suppressing intermediate values - // (which happens when it's part of a member access chain) if (!suppressIntermediateValues) { string newExprDisplay = GetCleanMemberName(newExpr); - TryAddExpressionValue(newExpr, newExprDisplay, details); + context.AddCapture(newExpr, newExprDisplay); } break; case ListInitExpression listInitExpr: - ExtractVariablesFromExpression(listInitExpr.NewExpression, details, suppressIntermediateValues: true); + AnalyzeExpression(listInitExpr.NewExpression, context, suppressIntermediateValues: true); foreach (ElementInit initializer in listInitExpr.Initializers) { foreach (Expression argument in initializer.Arguments) { - ExtractVariablesFromExpression(argument, details, suppressIntermediateValues); + AnalyzeExpression(argument, context, suppressIntermediateValues); } } @@ -186,58 +286,43 @@ private static void ExtractVariablesFromExpression(Expression? expr, Dictionary< case NewArrayExpression newArrayExpr: foreach (Expression expression in newArrayExpr.Expressions) { - ExtractVariablesFromExpression(expression, details, suppressIntermediateValues); + AnalyzeExpression(expression, context, suppressIntermediateValues); } break; } } - private static void HandleArrayIndexExpression(BinaryExpression arrayIndexExpr, Dictionary details) + private static void AnalyzeArrayIndexExpression(BinaryExpression arrayIndexExpr, AnalysisContext context) { string arrayName = GetCleanMemberName(arrayIndexExpr.Left); string indexValue = GetIndexArgumentDisplay(arrayIndexExpr.Right); string indexerDisplay = $"{arrayName}[{indexValue}]"; - TryAddExpressionValue(arrayIndexExpr, indexerDisplay, details); + context.AddCapture(arrayIndexExpr, indexerDisplay); - // Extract variables from the index argument - ExtractVariablesFromExpression(arrayIndexExpr.Right, details); + AnalyzeExpression(arrayIndexExpr.Right, context); } - private static void AddMemberExpressionToDetails(MemberExpression memberExpr, Dictionary details) + private static void AnalyzeMemberExpression(MemberExpression memberExpr, AnalysisContext context) { - string displayName = GetCleanMemberName(memberExpr); - - if (details.ContainsKey(displayName)) + // 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)) { return; } - try - { - object? value = Expression.Lambda(memberExpr).Compile().DynamicInvoke(); - - // Skip Func and Action delegates as they don't provide useful information in assertion failures - if (IsFuncOrActionType(value?.GetType())) - { - return; - } - - details[displayName] = value; - } - catch - { - details[displayName] = ""; - } + string displayName = GetCleanMemberName(memberExpr); + context.AddCapture(memberExpr, displayName); // Only extract variables from the object being accessed if it's not a member expression or indexer (which would show the full collection) if (memberExpr.Expression is not null and not MemberExpression) { - ExtractVariablesFromExpression(memberExpr.Expression, details, suppressIntermediateValues: true); + AnalyzeExpression(memberExpr.Expression, context, suppressIntermediateValues: true); } } - private static void HandleMethodCallExpression(MethodCallExpression callExpr, Dictionary details, bool suppressIntermediateValues = false) + private static void AnalyzeMethodCallExpression(MethodCallExpression callExpr, AnalysisContext context, 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) @@ -245,53 +330,114 @@ private static void HandleMethodCallExpression(MethodCallExpression callExpr, Di string objectName = GetCleanMemberName(callExpr.Object); string indexValue = GetIndexArgumentDisplay(callExpr.Arguments[0]); string indexerDisplay = $"{objectName}[{indexValue}]"; - TryAddExpressionValue(callExpr, indexerDisplay, details); + context.AddCapture(callExpr, indexerDisplay); - // Extract variables from the index argument but not from the object. - ExtractVariablesFromExpression(callExpr.Arguments[0], details, suppressIntermediateValues); + AnalyzeExpression(callExpr.Arguments[0], context, suppressIntermediateValues); } - else if (callExpr.Method.Name == "Get" && callExpr.Object is not null && callExpr.Arguments.Count > 0) + else if (IsArrayGetMethod(callExpr)) { string objectName = GetCleanMemberName(callExpr.Object); string indexDisplay = string.Join(", ", callExpr.Arguments.Select(GetIndexArgumentDisplay)); string indexerDisplay = $"{objectName}[{indexDisplay}]"; - TryAddExpressionValue(callExpr, indexerDisplay, details); + context.AddCapture(callExpr, indexerDisplay); - // Extract variables from the index arguments but not from the object foreach (Expression argument in callExpr.Arguments) { - ExtractVariablesFromExpression(argument, details, suppressIntermediateValues); + AnalyzeExpression(argument, context, suppressIntermediateValues); } } else { - // Check if the method returns a boolean if (callExpr.Method.ReturnType == typeof(bool)) { if (callExpr.Object is not null) { - // For boolean-returning methods, extract details from the object being called - // This captures the last non-boolean method call in a chain - ExtractVariablesFromExpression(callExpr.Object, details, suppressIntermediateValues); + AnalyzeExpression(callExpr.Object, context, suppressIntermediateValues); } } else { - // For non-boolean methods, capture the method call itself - string methodCallDisplay = GetCleanMemberName(callExpr); - TryAddExpressionValue(callExpr, methodCallDisplay, details); - - // Don't extract from the object to avoid duplication + string methodCallDisplay = GetMethodCallDisplayName(callExpr); + context.AddCapture(callExpr, methodCallDisplay); } - // Always extract variables from the arguments foreach (Expression argument in callExpr.Arguments) { - ExtractVariablesFromExpression(argument, details, suppressIntermediateValues); + AnalyzeExpression(argument, context, 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) @@ -310,6 +456,181 @@ private static bool IsFuncOrActionType(Type? type) return type.IsGenericType && type.GetGenericTypeDefinition().Name.StartsWith("Func`", 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; + + /// + /// 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 . + /// + private static bool IsUnaryUpdateNodeType(ExpressionType nodeType) + => nodeType is ExpressionType.PreIncrementAssign + or ExpressionType.PreDecrementAssign + or ExpressionType.PostIncrementAssign + or ExpressionType.PostDecrementAssign; + + private sealed class AnalysisContext + { +#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) + { + // 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)) + { + return; + } + + // Cannot box void-typed values into the captures array; nothing useful to display anyway. + if (expr.Type == typeof(void)) + { + return; + } + + 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)) + { + // 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 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 ? "" @@ -581,7 +902,8 @@ private static bool TryMatchListInitPattern(string input, int startIndex, out st } private static string CleanTypeName(string typeName) - => typeName switch + { + string cleanedTypeName = typeName switch { "Int32" => "int", "Int64" => "long", @@ -619,6 +941,22 @@ 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); + } + private static string CleanParentheses(string input) { if (string.IsNullOrEmpty(input)) @@ -774,31 +1112,11 @@ private static bool TryRemoveWrapper(string input, ref int index, string pattern return false; } - private static bool TryAddExpressionValue(Expression expr, string displayName, Dictionary details) - { - if (details.ContainsKey(displayName)) - { - return false; - } - - try - { - object? value = Expression.Lambda(expr).Compile().DynamicInvoke(); - details[displayName] = value; - } - catch - { - details[displayName] = ""; - } - - return true; - } - #if NET - [GeneratedRegex(@"[A-Za-z0-9_\.]+\+<>c__DisplayClass\d+_\d+\.(\w+(?:\.\w+)*(?:\[[^\]]+\])?)")] + [GeneratedRegex(@"[A-Za-z0-9_\.]+(?:\+[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_\.]+\+<>c__DisplayClass\d+_\d+\.(\w+(?:\.\w+)*(?:\[[^\]]+\])?)", RegexOptions.Compiled); + => new(@"[A-Za-z0-9_\.]+(?:\+[A-Za-z0-9_\.]+)*\+<>c__DisplayClass\d+_\d+\.(\w+(?:\.\w+)*(?:\[[^\]]+\])?)", RegexOptions.Compiled); #endif } diff --git a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs index c090c24e4c..b3a5256c6b 100644 --- a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs +++ b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs @@ -1,6 +1,9 @@ // 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; @@ -1042,4 +1045,448 @@ public void That_WithNullConstantAndVariable_OnlyIncludesVariable() nullVariable = null """); } + + // ---- 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() + { + // 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; + + Action act = () => Assert.That(() => box.GetNumber() == expected); + + act.Should().Throw(); + + // Each textual occurrence of GetNumber() should have been evaluated exactly once. + box.CallCount.Should().Be(1); + } + + public void That_ExpressionWithSideEffect_FailureMessageReflectsFirstEvaluation() + { + var box = new Counter(); + int expected = 2; + + Action act = () => Assert.That(() => box.GetNumber() == expected); + + act.Should().Throw() + .WithMessage( + """ + Assert.That(() => box.GetNumber() == expected) failed. + Details: + box.GetNumber() = 1 + expected = 2 + """); + } + + public void That_ExpressionWithSideEffect_AppearingTwice_EvaluatesEachOccurrenceOnce() + { + // 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(); + + Action act = () => Assert.That(() => box.GetNumber() == box.GetNumber()); + + 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); + } + + public void That_ShortCircuitedExpression_StillReportsBothOperands() + { + // 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; + + Action act = () => Assert.That(() => a && b > 10); + + act.Should().Throw() + .WithMessage( + """ + Assert.That(() => a && b > 10) failed. + Details: + a = False + b = 5 + """); + } + + public void That_ShortCircuitedExpression_DoesNotEvaluateUnreachedSideEffect() + { + // 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; + + Action act = () => Assert.That(() => a && box.GetNumber() > 0); + + act.Should().Throw(); + + // Counter must not have been called. + box.CallCount.Should().Be(0); + } + + public void That_ShortCircuitedExpression_DoesNotReevaluateArbitraryGetMethod() + { + var box = new Counter(); + bool a = false; + int expected = 2; + + Action act = () => Assert.That(() => a && box.Get(expected) == expected); + + act.Should().Throw() + .WithMessage( + """ + Assert.That(() => a && box.Get(expected) == expected) failed. + Details: + a = False + expected = 2 + """); + box.CallCount.Should().Be(0); + } + + private sealed class SideEffectingReceiverFactory + { + public int GetObjectCallCount { get; private set; } + + public int GetListCallCount { get; private set; } + + public SideEffectingReceiverObject GetObject() + { + GetObjectCallCount++; + return new SideEffectingReceiverObject(); + } + + public List GetList() + { + GetListCallCount++; + return [42]; + } + } + + private sealed class SideEffectingReceiverObject + { + public int Property => 42; + } + + public void That_ShortCircuitedExpression_DoesNotReevaluateMemberExpressionWithSideEffectingReceiver() + { + var factory = new SideEffectingReceiverFactory(); + + Action act = () => Assert.That(() => false && factory.GetObject().Property == 0); + + act.Should().Throw(); + factory.GetObjectCallCount.Should().Be(0); + } + + public void That_ShortCircuitedExpression_DoesNotReevaluateIndexerWithSideEffectingReceiver() + { + var factory = new SideEffectingReceiverFactory(); + + Action act = () => Assert.That(() => false && factory.GetList()[0] == 0); + + act.Should().Throw(); + factory.GetListCallCount.Should().Be(0); + } + + public void That_ArbitraryGetMethod_RendersAsMethodCall_NotIndexer() + { + 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"; + + public static string SameClassStaticMethod() => "Giraffe"; + + private sealed class Zoo + { + public string GetAnimal() => "Giraffe"; + + public static string GetAnimalStatic() => "Giraffe"; + } + + public void That_InstanceMethodOnSameType_RendersAsThis() + { + // 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; + + Action act = () => Assert.That(() => SameClassInstanceMethod() == animal); + + act.Should().Throw() + .WithMessage( + """ + Assert.That(() => SameClassInstanceMethod() == animal) failed. + Details: + animal = "" + this.SameClassInstanceMethod() = "Giraffe" + """); + } + + public void That_StaticMethodOnSameType_RendersWithTypeName() + { + // 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; + + Action act = () => Assert.That(() => SameClassStaticMethod() == animal); + + act.Should().Throw() + .WithMessage( + """ + Assert.That(() => SameClassStaticMethod() == animal) failed. + Details: + AssertTests.SameClassStaticMethod() = "Giraffe" + animal = "" + """); + } + + public void That_InstanceMethodOnOtherType_RendersWithObjectName() + { + // The "happy path" that was already correct: instance method on a local variable. + string animal = string.Empty; + var zoo = new Zoo(); + + Action act = () => Assert.That(() => zoo.GetAnimal() == animal); + + act.Should().Throw() + .WithMessage( + """ + Assert.That(() => zoo.GetAnimal() == animal) failed. + Details: + animal = "" + zoo.GetAnimal() = "Giraffe" + """); + } + + public void That_StaticMethodOnOtherType_RendersWithTypeName() + { + // Static method on a non-enclosing class should include its type name. + string animal = string.Empty; + + Action act = () => Assert.That(() => Zoo.GetAnimalStatic() == animal); + + act.Should().Throw() + .WithMessage( + """ + Assert.That(() => Zoo.GetAnimalStatic() == animal) failed. + Details: + AssertTests.Zoo.GetAnimalStatic() = "Giraffe" + animal = "" + """); + } + + private class AnimalBase + { + public string Whisper() => "Whisper"; + } + + private class InheritedMethodBase + { + public string InheritedWhisper() => "Whisper"; + } + + private sealed class InheritedMethodProbe : InheritedMethodBase + { + public void AssertInheritedMethodRendersAsThis() + { + string expected = "Roar"; + + Action act = () => Assert.That(() => InheritedWhisper() == expected); + + act.Should().Throw() + .WithMessage( + """ + Assert.That(() => InheritedWhisper() == expected) failed. + Details: + expected = "Roar" + this.InheritedWhisper() = "Whisper" + """); + } + } + + private sealed class Cat : AnimalBase + { + } + + 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() + { + var probe = new InheritedMethodProbe(); + + probe.AssertInheritedMethodRendersAsThis(); + } + + // ---- Documented-trade-off test: properties in short-circuited branches are re-evaluated ---- + private sealed class PropertyCounter + { + public int GetCount { get; private set; } + + public int Value + { + get + { + GetCount++; + return 42; + } + } + } + + public void That_ShortCircuitedPropertyAccess_IsReevaluatedForReporting() + { + // 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 + """); + + // 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 void That_ManuallyConstructedPreIncrementAssignExpression_DoesNotThrow() + { + // 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); + } +} + +internal static class AssertTestsExtensions +{ + /// Helper for the That_ExtensionMethodOnThis_RendersAsThis test. + public static string GetDisplayName(this AssertTests _) => "MyTestClass"; }