From 35cfd56924e535e8bccbadd8f15987180a8b7581 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Sun, 17 May 2026 21:25:44 +0200 Subject: [PATCH 1/8] Fix Assert.That: single-pass evaluation and consistent method-call display Fixes two related issues in MSTest's Assert.That(Expression>): * Issue #6690: the expression was evaluated twice -- once to determine the boolean result and once per sub-expression for the failure details -- producing misleading values for stateful/side-effecting calls. Replaced the two-pass eval-then-walk approach with a single ExpressionVisitor-based rewriter that records each captured sub-expression's value as a side effect of the single root evaluation. Pure operand reads (variables/properties, array indexers, get_Item/Get) are re-evaluated lazily when short-circuited so the failure message stays informative; arbitrary method calls are intentionally NOT re-evaluated to preserve short-circuit semantics for side-effecting code. * Issue #6691: method-call display names were inconsistent (static methods missed the type prefix; instance methods on \ his\ showed the full namespace). Replaced the ToString-based path with GetMethodCallDisplayName, which builds the name directly from the MethodCallExpression so static methods show \TypeName.Method(...)\, instance methods on captured \ his\`n show \ his.Method(...)\, extension methods show \ eceiver.Method(rest)\, and other instance methods show \object.Method(args)\. Adds 11 new unit tests covering both issues plus regression guards for short-circuit semantics, inherited-method receiver disambiguation, and all four method-call display permutations from #6691. All 750 AssertTests pass and build.cmd succeeds with 0 warnings, 0 errors across netstandard2.0, net462, net8.0, and net9.0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../TestFramework/Assertions/Assert.That.cs | 412 ++++++++++++++---- .../Assertions/AssertTests.That.cs | 272 ++++++++++++ 2 files changed, 588 insertions(+), 96 deletions(-) diff --git a/src/TestFramework/TestFramework/Assertions/Assert.That.cs b/src/TestFramework/TestFramework/Assertions/Assert.That.cs index 382a53f397..b7ec17f51f 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,95 @@ 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. + 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(expr).Compile().DynamicInvoke(); + } + 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 +188,54 @@ 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; 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; 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 +243,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 +269,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 +313,105 @@ 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) { 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) + { + string receiver = GetCleanMemberName(callExpr.Arguments[0]); + 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?.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 whose value is of + /// exactly (which Roslyn may emit when only this is + /// captured). The exact-type check (rather than IsInstanceOfType) prevents mis-labeling a + /// non-this receiver as this when the asserted method is declared on a base type + /// and the receiver happens to be a local of that base type. + /// + 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 whose runtime type is exactly + // the declaring type. Exact-type (rather than IsInstanceOfType) avoids mis-labeling + // a base-typed local as "this" when the asserted method is inherited from a base. + || (declaringType is not null + && objectExpr is ConstantExpression ce + && ce.Value is not null + && ce.Value.GetType() == declaringType); + private static bool IsFuncOrActionType(Type? type) { if (type is null) @@ -310,6 +430,126 @@ 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/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). + /// + private static bool IsSafeToReevaluate(Expression expr) + => expr switch + { + MemberExpression => true, + BinaryExpression { NodeType: ExpressionType.ArrayIndex } => true, + UnaryExpression { NodeType: ExpressionType.ArrayLength } => true, + // Indexer-style method calls (auto-properties get_Item / multi-dim array Get) are + // conventionally pure reads; the previous implementation evaluated them eagerly too. + MethodCallExpression { Method.Name: "get_Item" or "Get" } => true, + _ => false, + }; + + 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 ? "" @@ -774,26 +1014,6 @@ 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+)*(?:\[[^\]]+\])?)")] private static partial Regex CompilerGeneratedDisplayClassRegex(); diff --git a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs index c090c24e4c..cf91aea389 100644 --- a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs +++ b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs @@ -1042,4 +1042,276 @@ 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 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); + + try + { + act(); + } + catch (AssertFailedException) + { + // Expected. + } + + // 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); + + try + { + act(); + } + catch (AssertFailedException) + { + // Expected. + } + + // Counter must not have been called. + box.CallCount.Should().Be(0); + } + + // ---- 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 = ""; + + 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 = ""; + + 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 = ""; + 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 = ""; + + Action act = () => Assert.That(() => Zoo.GetAnimalStatic() == animal); + + act.Should().Throw() + .WithMessage( + """ + Assert.That(() => Zoo.GetAnimalStatic() == animal) failed. + Details: + Zoo.GetAnimalStatic() = "Giraffe" + animal = "" + """); + } + + private class AnimalBase + { + public string Whisper() => "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" + """); + } + + // ---- 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); + } } From 901f64a110421878e278382c4d3102eb7f8b922c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 18 May 2026 07:13:34 +0000 Subject: [PATCH 2/8] Address Assert.That review feedback Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com> --- .../TestFramework/Assertions/Assert.That.cs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/TestFramework/TestFramework/Assertions/Assert.That.cs b/src/TestFramework/TestFramework/Assertions/Assert.That.cs index b7ec17f51f..27b697381d 100644 --- a/src/TestFramework/TestFramework/Assertions/Assert.That.cs +++ b/src/TestFramework/TestFramework/Assertions/Assert.That.cs @@ -111,6 +111,9 @@ private static bool EvaluateAndCollectDetails(Expression body, Dictionary>(rewrittenBody, arrayParam); object?[] values = new object?[context.CaptureNames.Count]; @@ -152,7 +155,9 @@ private static bool EvaluateAndCollectDetails(Expression body, Dictionary>(Expression.Convert(expr, typeof(object))) + .Compile()(); } catch { @@ -377,7 +382,9 @@ private static string GetMethodCallDisplayName(MethodCallExpression callExpr) if (callExpr.Object is null) { // Regular static method: use the declaring type's short name as the receiver display. - string typeName = callExpr.Method.DeclaringType?.Name ?? ""; + string typeName = callExpr.Method.DeclaringType is { } dt + ? CleanTypeName(dt.FullName ?? dt.Name) + : ""; return $"{typeName}.{methodName}({argsStr})"; } From 25ced7d010734cbbfaad712130106f92c9cf48d4 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Mon, 18 May 2026 12:07:46 +0200 Subject: [PATCH 3/8] Address Assert.That review feedback Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../TestFramework/Assertions/Assert.That.cs | 27 +++++++-- .../Assertions/AssertTests.That.cs | 56 +++++++++++-------- 2 files changed, 55 insertions(+), 28 deletions(-) diff --git a/src/TestFramework/TestFramework/Assertions/Assert.That.cs b/src/TestFramework/TestFramework/Assertions/Assert.That.cs index 27b697381d..abe2fb49d5 100644 --- a/src/TestFramework/TestFramework/Assertions/Assert.That.cs +++ b/src/TestFramework/TestFramework/Assertions/Assert.That.cs @@ -461,9 +461,11 @@ private static bool IsSafeToReevaluate(Expression expr) MemberExpression => true, BinaryExpression { NodeType: ExpressionType.ArrayIndex } => true, UnaryExpression { NodeType: ExpressionType.ArrayLength } => true, - // Indexer-style method calls (auto-properties get_Item / multi-dim array Get) are - // conventionally pure reads; the previous implementation evaluated them eagerly too. - MethodCallExpression { Method.Name: "get_Item" or "Get" } => true, + // 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 { Method.Name: "get_Item" } => true, + MethodCallExpression { Method.Name: "Get", Method.DeclaringType: { } declaringType } when declaringType == typeof(Array) => true, _ => false, }; @@ -828,7 +830,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", @@ -866,6 +869,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)) diff --git a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs index cf91aea389..3504802180 100644 --- a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs +++ b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs @@ -1044,7 +1044,6 @@ public void That_WithNullConstantAndVariable_OnlyIncludesVariable() } // ---- Tests for issue #6690 (single-pass evaluation) ------------------------------------ - private sealed class Counter { public int CallCount { get; private set; } @@ -1054,6 +1053,12 @@ public int GetNumber() CallCount++; return CallCount; } + + public int Get(int value) + { + CallCount++; + return value; + } } public void That_ExpressionWithSideEffect_EvaluatesOnlyOnce() @@ -1067,14 +1072,7 @@ public void That_ExpressionWithSideEffect_EvaluatesOnlyOnce() Action act = () => Assert.That(() => box.GetNumber() == expected); - try - { - act(); - } - catch (AssertFailedException) - { - // Expected. - } + act.Should().Throw(); // Each textual occurrence of GetNumber() should have been evaluated exactly once. box.CallCount.Should().Be(1); @@ -1148,21 +1146,32 @@ public void That_ShortCircuitedExpression_DoesNotEvaluateUnreachedSideEffect() Action act = () => Assert.That(() => a && box.GetNumber() > 0); - try - { - act(); - } - catch (AssertFailedException) - { - // Expected. - } + act.Should().Throw(); // Counter must not have been called. box.CallCount.Should().Be(0); } - // ---- Tests for issue #6691 (method call display names) --------------------------------- + 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); + } + + // ---- Tests for issue #6691 (method call display names) --------------------------------- public string SameClassInstanceMethod() => "Giraffe"; public static string SameClassStaticMethod() => "Giraffe"; @@ -1179,7 +1188,7 @@ 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 animal = string.Empty; Action act = () => Assert.That(() => SameClassInstanceMethod() == animal); @@ -1197,7 +1206,7 @@ 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 animal = string.Empty; Action act = () => Assert.That(() => SameClassStaticMethod() == animal); @@ -1214,7 +1223,7 @@ public void That_StaticMethodOnSameType_RendersWithTypeName() public void That_InstanceMethodOnOtherType_RendersWithObjectName() { // The "happy path" that was already correct: instance method on a local variable. - string animal = ""; + string animal = string.Empty; var zoo = new Zoo(); Action act = () => Assert.That(() => zoo.GetAnimal() == animal); @@ -1232,7 +1241,7 @@ public void That_InstanceMethodOnOtherType_RendersWithObjectName() public void That_StaticMethodOnOtherType_RendersWithTypeName() { // Static method on a non-enclosing class should include its type name. - string animal = ""; + string animal = string.Empty; Action act = () => Assert.That(() => Zoo.GetAnimalStatic() == animal); @@ -1241,7 +1250,7 @@ public void That_StaticMethodOnOtherType_RendersWithTypeName() """ Assert.That(() => Zoo.GetAnimalStatic() == animal) failed. Details: - Zoo.GetAnimalStatic() = "Giraffe" + AssertTests.Zoo.GetAnimalStatic() = "Giraffe" animal = "" """); } @@ -1277,7 +1286,6 @@ public void That_InstanceMethodOnLocalOfBaseType_DoesNotRenderAsThis() } // ---- Documented-trade-off test: properties in short-circuited branches are re-evaluated ---- - private sealed class PropertyCounter { public int GetCount { get; private set; } From 7d953d6fa90e55c0cdbce17b2883c27a79b5c80d Mon Sep 17 00:00:00 2001 From: Evangelink Date: Mon, 18 May 2026 12:13:37 +0200 Subject: [PATCH 4/8] Fix AreAllDistinct to report comparer short name Aligns with the same fix in PR #8307 / PR #8262 so the assertion message shows the comparer's short type name instead of the full namespaced string. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../TestFramework/Assertions/Assert.AreAllDistinct.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs b/src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs index 0929bfe3dc..183ac9d884 100644 --- a/src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs +++ b/src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs @@ -67,7 +67,7 @@ public static void AreAllDistinct([NotNull] IEnumerable? collection, [NotN { CheckParameterNotNull(collection, "Assert.AreAllDistinct", "collection"); CheckParameterNotNull(comparer, "Assert.AreAllDistinct", "comparer"); - AreAllDistinctImpl(collection, comparer, comparerTypeName: comparer.GetType().ToString(), message, collectionExpression); + AreAllDistinctImpl(collection, comparer, comparerTypeName: comparer.GetType().Name, message, collectionExpression); } /// @@ -121,7 +121,7 @@ public static void AreAllDistinct([NotNull] IEnumerable? collection, [NotNull] I { CheckParameterNotNull(collection, "Assert.AreAllDistinct", "collection"); CheckParameterNotNull(comparer, "Assert.AreAllDistinct", "comparer"); - AreAllDistinctImpl(collection.Cast(), new NonGenericEqualityComparerAdapter(comparer), comparerTypeName: comparer.GetType().ToString(), message, collectionExpression); + AreAllDistinctImpl(collection.Cast(), new NonGenericEqualityComparerAdapter(comparer), comparerTypeName: comparer.GetType().Name, message, collectionExpression); } #pragma warning disable CS8714 // The type cannot be used as type parameter in the generic type or method. Nullability of type argument doesn't match 'notnull' constraint. From caea834f31eefe17e48c4bff479ac8e54b2dd069 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 18 May 2026 13:27:25 +0000 Subject: [PATCH 5/8] Fix Assert.That fallback safety and inherited this rendering Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com> --- .../TestFramework/Assertions/Assert.That.cs | 51 ++++++---- .../Assertions/AssertTests.That.cs | 92 +++++++++++++++++++ 2 files changed, 126 insertions(+), 17 deletions(-) diff --git a/src/TestFramework/TestFramework/Assertions/Assert.That.cs b/src/TestFramework/TestFramework/Assertions/Assert.That.cs index abe2fb49d5..b5f4f352c8 100644 --- a/src/TestFramework/TestFramework/Assertions/Assert.That.cs +++ b/src/TestFramework/TestFramework/Assertions/Assert.That.cs @@ -322,7 +322,7 @@ private static void AnalyzeMethodCallExpression(MethodCallExpression callExpr, A 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)); @@ -400,24 +400,24 @@ private static string GetMethodCallDisplayName(MethodCallExpression callExpr) /// /// 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 whose value is of - /// exactly (which Roslyn may emit when only this is - /// captured). The exact-type check (rather than IsInstanceOfType) prevents mis-labeling a - /// non-this receiver as this when the asserted method is declared on a base type - /// and the receiver happens to be a local of that base type. + /// (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 whose runtime type is exactly - // the declaring type. Exact-type (rather than IsInstanceOfType) avoids mis-labeling - // a base-typed local as "this" when the asserted method is inherited from a base. + // 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.Value.GetType() == declaringType); + && ce.Type == ce.Value.GetType() + && declaringType.IsAssignableFrom(ce.Type)); private static bool IsFuncOrActionType(Type? type) { @@ -447,28 +447,45 @@ private static bool IsFuncOrActionType(Type? type) /// /// This heuristic intentionally treats (which covers both /// fields and properties) and the well-known indexer-style method calls - /// (get_Item/Get) as pure, even though property getters and user-defined + /// (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). + /// for issue #6690). Safety is recursive: each child expression must also be safe. /// private static bool IsSafeToReevaluate(Expression expr) => expr switch { - MemberExpression => true, - BinaryExpression { NodeType: ExpressionType.ArrayIndex } => true, - UnaryExpression { NodeType: ExpressionType.ArrayLength } => true, + 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 { Method.Name: "get_Item" } => true, - MethodCallExpression { Method.Name: "Get", Method.DeclaringType: { } declaringType } when declaringType == typeof(Array) => true, + 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 + && callExpr.Method.DeclaringType == typeof(Array) + && callExpr.Arguments.Count > 0; + private sealed class AnalysisContext { #pragma warning disable IDE0028 // Collection initialization can be simplified - Dictionary needs ReferenceEqualityComparer diff --git a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs index 3504802180..17f6413faf 100644 --- a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs +++ b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs @@ -1171,6 +1171,67 @@ public void That_ShortCircuitedExpression_DoesNotReevaluateArbitraryGetMethod() 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"; @@ -1260,6 +1321,30 @@ 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 { } @@ -1285,6 +1370,13 @@ public void That_InstanceMethodOnLocalOfBaseType_DoesNotRenderAsThis() """); } + 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 { From 9a096452590be36ea6d3e60b10b265527ff4f2d3 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Mon, 18 May 2026 19:40:32 +0200 Subject: [PATCH 6/8] Fix multidim array Get detection and nested display class regex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two test failures surfaced on CI for the latest Assert.That changes: 1. `IsArrayGetMethod` only recognized `System.Array.Get` — multidimensional array indexers (`int[,]`) expose runtime-synthesized `Get` methods declared on the array type itself, so the indexer-style display path was bypassed and the failure message rendered `matrix.Get(row, col)` instead of `matrix[row, col]`. Switched the check to look at `Object.Type.IsArray`. 2. `CompilerGeneratedDisplayClassRegex` did not cross nested-type `+` boundaries, so captured locals inside a nested helper class (e.g. `InheritedMethodProbe` inside `AssertTests`) were rendered as `AssertTests+expected` instead of just `expected`. Allowed the regex to walk the full `A+B+<>c__DisplayClass...` prefix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../TestFramework/Assertions/Assert.That.cs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/TestFramework/TestFramework/Assertions/Assert.That.cs b/src/TestFramework/TestFramework/Assertions/Assert.That.cs index b5f4f352c8..64de3e8260 100644 --- a/src/TestFramework/TestFramework/Assertions/Assert.That.cs +++ b/src/TestFramework/TestFramework/Assertions/Assert.That.cs @@ -483,7 +483,10 @@ private static bool IsCollectionIndexerRead(MethodCallExpression callExpr) private static bool IsArrayGetMethod(MethodCallExpression callExpr) => callExpr.Method.Name == "Get" && callExpr.Object is not null - && callExpr.Method.DeclaringType == typeof(Array) + // 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; private sealed class AnalysisContext @@ -1058,10 +1061,10 @@ private static bool TryRemoveWrapper(string input, ref int index, string pattern } #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 } From 45f44001d0e3f68c37b324c7e215d3f4bcfdc884 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 19 May 2026 04:18:36 +0000 Subject: [PATCH 7/8] Address review: skip assignment/update nodes in capture analysis, fix extension-method this detection Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com> --- .../TestFramework/Assertions/Assert.That.cs | 54 +++++++++++++- .../Assertions/AssertTests.That.cs | 73 +++++++++++++++++++ 2 files changed, 126 insertions(+), 1 deletion(-) diff --git a/src/TestFramework/TestFramework/Assertions/Assert.That.cs b/src/TestFramework/TestFramework/Assertions/Assert.That.cs index 64de3e8260..08f08673c6 100644 --- a/src/TestFramework/TestFramework/Assertions/Assert.That.cs +++ b/src/TestFramework/TestFramework/Assertions/Assert.That.cs @@ -196,6 +196,12 @@ private static void AnalyzeExpression(Expression? expr, AnalysisContext context, 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: AnalyzeExpression(binaryExpr.Left, context, suppressIntermediateValues); AnalyzeExpression(binaryExpr.Right, context, suppressIntermediateValues); @@ -218,6 +224,12 @@ private static void AnalyzeExpression(Expression? expr, AnalysisContext context, 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); break; @@ -372,7 +384,14 @@ private static string GetMethodCallDisplayName(MethodCallExpression callExpr) && callExpr.Method.IsDefined(typeof(ExtensionAttribute), inherit: false) && callExpr.Arguments.Count > 0) { - string receiver = GetCleanMemberName(callExpr.Arguments[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})"; } @@ -489,6 +508,39 @@ private static bool IsArrayGetMethod(MethodCallExpression callExpr) && 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 diff --git a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs index 17f6413faf..1caf1dec37 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; @@ -1414,4 +1417,74 @@ public void That_ShortCircuitedPropertyAccess_IsReevaluatedForReporting() // 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 + { + public int Value; + } + + 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"; } From 33b208cb8b752491138a366cebd91749942cb0c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Tue, 19 May 2026 09:31:34 +0200 Subject: [PATCH 8/8] Suppress SA1401 on MutableBox.Value test helper field The test type intentionally exposes a public field so Expression.Assign can target it (Expression.Field requires FieldInfo). Suppress SA1401 around the field rather than reshape the type. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../TestFramework.UnitTests/Assertions/AssertTests.That.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs index 1caf1dec37..b3a5256c6b 100644 --- a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs +++ b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs @@ -1445,7 +1445,9 @@ public void That_ExtensionMethodOnThis_RendersAsThis() // 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()