diff --git a/src/TestFramework/TestFramework/Assertions/Assert.That.cs b/src/TestFramework/TestFramework/Assertions/Assert.That.cs index dadb59c4db..f0d137b95f 100644 --- a/src/TestFramework/TestFramework/Assertions/Assert.That.cs +++ b/src/TestFramework/TestFramework/Assertions/Assert.That.cs @@ -148,10 +148,17 @@ private static bool EvaluateExpression(Expression expr, Dictionary expr switch { + // Assignments and compound assignments are side-effecting by definition; ensure they + // always take the single-pass evaluator so the fast path doesn't run them once before + // EvaluateAllSubExpressions runs them again on the failure-diagnostic path. + BinaryExpression binaryExpr when IsAssignmentBinaryNodeType(binaryExpr.NodeType) => true, + BinaryExpression binaryExpr => binaryExpr.Method is not null || RequiresSinglePassEvaluation(binaryExpr.Left) || RequiresSinglePassEvaluation(binaryExpr.Right), + UnaryExpression unaryExpr when IsAssignmentUnaryNodeType(unaryExpr.NodeType) => true, + UnaryExpression unaryExpr => unaryExpr.Method is not null || RequiresSinglePassEvaluation(unaryExpr.Operand), @@ -217,6 +224,39 @@ private static void EvaluateAllSubExpressions(Expression expr, Dictionary details, Dictionary evaluationCache) { string arrayName = GetCleanMemberName(arrayIndexExpr.Left); - string indexValue = GetIndexArgumentDisplay(arrayIndexExpr.Right, evaluationCache); + string indexValue = GetIndexArgumentDisplay(arrayIndexExpr.Right); string indexerDisplay = $"{arrayName}[{indexValue}]"; TryAddExpressionValue(arrayIndexExpr, indexerDisplay, details, evaluationCache); @@ -820,7 +883,7 @@ private static void HandleMethodCallExpression(MethodCallExpression callExpr, Di { // Display as "listName[indexValue]" for readability string objectName = GetCleanMemberName(callExpr.Object); - string indexValue = GetIndexArgumentDisplay(callExpr.Arguments[0], evaluationCache); + string indexValue = GetIndexArgumentDisplay(callExpr.Arguments[0]); string indexerDisplay = $"{objectName}[{indexValue}]"; TryAddExpressionValue(callExpr, indexerDisplay, details, evaluationCache); @@ -832,7 +895,7 @@ private static void HandleMethodCallExpression(MethodCallExpression callExpr, Di { // Handle multi-dimensional array indexers (e.g., array.Get(i, j) displayed as array[i, j]) string objectName = GetCleanMemberName(callExpr.Object); - string indexDisplay = string.Join(", ", callExpr.Arguments.Select(arg => GetIndexArgumentDisplay(arg, evaluationCache))); + string indexDisplay = string.Join(", ", callExpr.Arguments.Select(GetIndexArgumentDisplay)); string indexerDisplay = $"{objectName}[{indexDisplay}]"; TryAddExpressionValue(callExpr, indexerDisplay, details, evaluationCache); @@ -872,6 +935,60 @@ private static void HandleMethodCallExpression(MethodCallExpression callExpr, Di } } + /// + /// Evaluates only the *sub-children* of a writable assignment target (the Left of an Assign or + /// compound assignment, or the Operand of a Pre/Post Increment/Decrement). Walking the writable + /// wrapper itself would invoke its property/indexer getter once for caching, and the rebuilt + /// assignment would then invoke it a second time (compound assignments and Increment/Decrement + /// must read the current value). Restricting the walk to receiver/index arguments ensures the + /// getter runs exactly once — inside the rebuilt assignment. + /// + /// Writable targets are restricted by LINQ expression semantics to , + /// , or ; the latter has no + /// sub-children to walk. + /// +#if NET7_0_OR_GREATER + [RequiresDynamicCode("Calls Microsoft.VisualStudio.TestTools.UnitTesting.AssertExtensions.EvaluateAllSubExpressions(Expression, Dictionary)")] +#endif + private static void EvaluateAssignmentTargetSubChildren(Expression target, Dictionary cache) + { + // ParameterExpression and static-member MemberExpression are also valid writable targets, + // but they have no sub-children to walk. + switch (target) + { + case MemberExpression memberExpr when memberExpr.Expression is not null: + EvaluateAllSubExpressions(memberExpr.Expression, cache); + break; + + case IndexExpression indexExpr: + if (indexExpr.Object is not null) + { + EvaluateAllSubExpressions(indexExpr.Object, cache); + } + + foreach (Expression argument in indexExpr.Arguments) + { + EvaluateAllSubExpressions(argument, cache); + } + + break; + } + } + + private static bool IsAssignmentBinaryNodeType(ExpressionType nodeType) + => nodeType is ExpressionType.Assign + or ExpressionType.AddAssign or ExpressionType.AddAssignChecked + or ExpressionType.SubtractAssign or ExpressionType.SubtractAssignChecked + or ExpressionType.MultiplyAssign or ExpressionType.MultiplyAssignChecked + or ExpressionType.DivideAssign or ExpressionType.ModuloAssign + or ExpressionType.PowerAssign + or ExpressionType.AndAssign or ExpressionType.OrAssign or ExpressionType.ExclusiveOrAssign + or ExpressionType.LeftShiftAssign or ExpressionType.RightShiftAssign; + + private static bool IsAssignmentUnaryNodeType(ExpressionType nodeType) + => nodeType is ExpressionType.PreIncrementAssign or ExpressionType.PreDecrementAssign + or ExpressionType.PostIncrementAssign or ExpressionType.PostDecrementAssign; + private static bool IsFuncOrActionType(Type? type) { if (type is null) @@ -879,15 +996,19 @@ private static bool IsFuncOrActionType(Type? type) return false; } - // Check for Action types - if (type == typeof(Action) || - (type.IsGenericType && type.GetGenericTypeDefinition().Name.StartsWith(ActionTypePrefix, StringComparison.Ordinal))) + if (type == typeof(Action)) { return true; } - // Check for Func types - return type.IsGenericType && type.GetGenericTypeDefinition().Name.StartsWith(FuncTypePrefix, StringComparison.Ordinal); + if (!type.IsGenericType) + { + return false; + } + + string genericDefinitionName = type.GetGenericTypeDefinition().Name; + return genericDefinitionName.StartsWith(ActionTypePrefix, StringComparison.Ordinal) + || genericDefinitionName.StartsWith(FuncTypePrefix, StringComparison.Ordinal); } private static string GetCleanMemberName(Expression? expr) @@ -897,45 +1018,19 @@ private static string GetCleanMemberName(Expression? expr) /// /// Gets a display string for an index argument in an indexer expression. - /// Preserves variable names for readability (e.g., "i" instead of "0" if i is a variable). + /// Preserves variable names and source-style expression text for readability (e.g., "i" or + /// "start + offset" rather than the evaluated constant value), so the failure message keeps + /// the user's intent visible. /// - private static string GetIndexArgumentDisplay(Expression indexArg, Dictionary evaluationCache) + private static string GetIndexArgumentDisplay(Expression indexArg) { try { - // For constant values, just format the value - if (indexArg is ConstantExpression constExpr) - { - return FormatValue(constExpr.Value); - } - - // For member expressions that are fields or simple variable references, - // preserve the variable name to help with readability (e.g., "myIndex" instead of "5") - if (indexArg is MemberExpression memberExpr && IsVariableReference(memberExpr)) - { - return CleanExpressionText(indexArg.ToString()); - } - - // For parameter expressions (method parameters), preserve the parameter name - if (indexArg is ParameterExpression) - { - return CleanExpressionText(indexArg.ToString()); - } - - // For complex expressions (e.g., "i + 1"), preserve the expression text for clarity - if (!IsSimpleExpression(indexArg)) - { - return CleanExpressionText(indexArg.ToString()); - } - - // For other simple expressions, try to use cached value - if (evaluationCache.TryGetValue(indexArg, out object? cachedValue)) - { - return FormatValue(TranslateFailureSentinel(cachedValue)); - } - - // Fallback to expression text - return CleanExpressionText(indexArg.ToString()); + // For literal constant indices, formatting the value gives the cleanest output + // (e.g., the literal 0 in `array[0]`). + return indexArg is ConstantExpression constExpr + ? FormatValue(constExpr.Value) + : CleanExpressionText(indexArg.ToString()); } catch { @@ -943,33 +1038,6 @@ private static string GetIndexArgumentDisplay(Expression indexArg, Dictionary - /// Determines if a member expression is a simple variable reference (field or captured variable) - /// rather than a property access on an object. - /// - private static bool IsVariableReference(MemberExpression memberExpr) - // Fields typically have Expression as null (static) or ConstantExpression (instance field on captured variable) - => memberExpr.Expression is null or ConstantExpression; - - /// - /// Determines if an expression is simple enough to evaluate directly or if its - /// textual representation should be preserved for better diagnostic messages. - /// - private static bool IsSimpleExpression(Expression expr) - => expr switch - { - // Constants are simple and can be evaluated - ConstantExpression => true, - // Parameter references should preserve their names for indices (e.g., "i" not "0") - ParameterExpression => false, - // Member expressions should be evaluated case by case - MemberExpression => false, - // Simple unary operations on members like "!flag" should preserve the expression text - UnaryExpression unary when unary.Operand is MemberExpression or ParameterExpression => false, - // Everything else is considered complex (binary operations, method calls, etc.) - _ => false, - }; - private static string FormatValue(object? value) => value switch { @@ -1025,9 +1093,11 @@ private static string RemoveAnonymousTypeWrappers(string input) while (i < input.Length) { - // Look for anonymous type pattern: new <>f__AnonymousType followed by generic parameters - if (i <= input.Length - NewKeyword.Length && input.Substring(i, NewKeyword.Length) == NewKeyword && - i + NewKeyword.Length < input.Length && input.Substring(i + NewKeyword.Length).StartsWith(AnonymousTypePrefix, StringComparison.Ordinal)) + // Look for anonymous type pattern: new <>f__AnonymousType followed by generic parameters. + // Use position-aware ordinal comparisons to avoid allocating substring instances on + // every character of the input. + if (HasSubstringAt(input, i, NewKeyword) && + HasSubstringAt(input, i + NewKeyword.Length, AnonymousTypePrefix)) { // Find the start of the constructor parameters int constructorStart = input.IndexOf('(', i + NewKeyword.Length); @@ -1122,14 +1192,12 @@ private static string CleanListInitializers(string input) string initContent = input.Substring(braceStart + 1, braceEnd - braceStart - 2); // Extract the generic type parameter and arguments from the Add method calls - string addMethodPattern = @"Void\s+Add\([^)]+\)\(([^)]+)\)"; - MatchCollection addMatches = Regex.Matches(initContent, addMethodPattern); + MatchCollection addMatches = ListInitAddArgumentRegex().Matches(initContent); if (addMatches.Count > 0) { // Extract type from the first Add method call - string firstAddPattern = @"Void\s+Add\(([^)]+)\)"; - Match typeMatch = Regex.Match(initContent, firstAddPattern); + Match typeMatch = ListInitAddTypeRegex().Match(initContent); string genericType = "object"; // default fallback if (typeMatch.Success) @@ -1140,7 +1208,7 @@ private static string CleanListInitializers(string input) } // Extract all arguments from Add method calls - var arguments = new List(); + var arguments = new List(addMatches.Count); foreach (Match addMatch in addMatches) { string argument = addMatch.Groups[1].Value; @@ -1172,8 +1240,8 @@ private static bool TryMatchListInitPattern(string input, int startIndex, out st collectionType = string.Empty; patternEnd = startIndex; - // Check for "new " at the start - if (startIndex + NewKeyword.Length >= input.Length || !input.Substring(startIndex, NewKeyword.Length).Equals(NewKeyword, StringComparison.Ordinal)) + // Check for "new " at the start (non-allocating ordinal compare). + if (!HasSubstringAt(input, startIndex, NewKeyword)) { return false; } @@ -1192,8 +1260,7 @@ private static bool TryMatchListInitPattern(string input, int startIndex, out st foreach (string type in collectionTypes) { - if (pos + type.Length < input.Length && - input.Substring(pos, type.Length).Equals(type, StringComparison.Ordinal)) + if (HasSubstringAt(input, pos, type)) { matchedType = type; pos += type.Length; @@ -1207,7 +1274,7 @@ private static bool TryMatchListInitPattern(string input, int startIndex, out st } // Check for "`1()" pattern - if (pos + ListInitPattern.Length >= input.Length || !input.Substring(pos, ListInitPattern.Length).Equals(ListInitPattern, StringComparison.Ordinal)) + if (!HasSubstringAt(input, pos, ListInitPattern)) { return false; } @@ -1231,6 +1298,17 @@ private static bool TryMatchListInitPattern(string input, int startIndex, out st return true; } + /// + /// Returns if occurs in + /// at using ordinal comparison. Avoids the + /// substring allocation that input.Substring(start, value.Length) == value would + /// incur for every probe in the diagnostic text cleanup pipeline. + /// + private static bool HasSubstringAt(string input, int start, string value) + => start >= 0 + && start <= input.Length - value.Length + && string.CompareOrdinal(input, start, value, 0, value.Length) == 0; + private static string CleanTypeName(string typeName) => typeName switch { @@ -1503,20 +1581,37 @@ private static bool TryAddExpressionValue(Expression expr, string displayName, D } /// - /// Converts the internal failure sentinel into the localized - /// display string. Any other - /// value (including ) is returned unchanged. + /// Cached regular expressions used by the diagnostic text cleanup pipeline. + /// On NET we use the source-generated regex attribute; on netstandard/net462 we cache + /// a single compiled instance to avoid the per-call allocation and JIT cost that a + /// freshly-constructed Regex would incur on every failed assertion. /// - private static object? TranslateFailureSentinel(object? value) - => ReferenceEquals(value, FailedToEvaluateSentinel) - ? FrameworkMessages.AssertThatFailedToEvaluate - : value; - #if NET [GeneratedRegex(@"[A-Za-z0-9_\.]+\+<>c__DisplayClass\d+_\d+\.(\w+(?:\.\w+)*(?:\[[^\]]+\])?)")] private static partial Regex CompilerGeneratedDisplayClassRegex(); + + [GeneratedRegex(@"Void\s+Add\([^)]+\)\(([^)]+)\)")] + private static partial Regex ListInitAddArgumentRegex(); + + [GeneratedRegex(@"Void\s+Add\(([^)]+)\)")] + private static partial Regex ListInitAddTypeRegex(); #else - private static Regex CompilerGeneratedDisplayClassRegex() - => new(@"[A-Za-z0-9_\.]+\+<>c__DisplayClass\d+_\d+\.(\w+(?:\.\w+)*(?:\[[^\]]+\])?)", RegexOptions.Compiled); + private static readonly Regex CompilerGeneratedDisplayClass = new( + @"[A-Za-z0-9_\.]+\+<>c__DisplayClass\d+_\d+\.(\w+(?:\.\w+)*(?:\[[^\]]+\])?)", + RegexOptions.Compiled); + + private static readonly Regex ListInitAddArgument = new( + @"Void\s+Add\([^)]+\)\(([^)]+)\)", + RegexOptions.Compiled); + + private static readonly Regex ListInitAddType = new( + @"Void\s+Add\(([^)]+)\)", + RegexOptions.Compiled); + + private static Regex CompilerGeneratedDisplayClassRegex() => CompilerGeneratedDisplayClass; + + private static Regex ListInitAddArgumentRegex() => ListInitAddArgument; + + private static Regex ListInitAddTypeRegex() => ListInitAddType; #endif } diff --git a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs index 7b63361e37..b1dc3503a1 100644 --- a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs +++ b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs @@ -1323,6 +1323,176 @@ public void That_ManuallyConstructedAssignExpression_SurfacesRhsAndPreservesSide container.Inner.Value.Should().Be(42); } + // Regression for Assign double-evaluation: the RHS of Expression.Assign was evaluated + // once during the bottom-up pass, but the generic "rebuild and compile" path then + // produced an invalid Assign(Constant, Constant) that the catch sentineled, and the + // parent LessThan was re-invoked from the original tree — re-running ComputeValue a + // second time. Assign is now special-cased so the RHS runs exactly once. + public void That_ManuallyConstructedAssign_EvaluatesRhsExactlyOnce() + { + var container = new MutableContainer(); + var counter = new CountingComputeValue(); + FieldInfo innerFi = typeof(MutableContainer).GetField(nameof(MutableContainer.Inner))!; + FieldInfo valueFi = typeof(MutableBox).GetField(nameof(MutableBox.Value))!; + + MemberExpression innerAccess = Expression.Field(Expression.Constant(container), innerFi); + MemberExpression valueAccess = Expression.Field(innerAccess, valueFi); + MethodCallExpression rhs = Expression.Call(Expression.Constant(counter), typeof(CountingComputeValue).GetMethod(nameof(CountingComputeValue.Get))!); + BinaryExpression assign = Expression.Assign(valueAccess, rhs); + BinaryExpression body = Expression.LessThan(assign, Expression.Constant(0)); + var lambda = Expression.Lambda>(body); + + Action act = () => Assert.That(lambda); + + act.Should().Throw(); + counter.Calls.Should().Be(1); + container.Inner.Value.Should().Be(42); + } + + // Regression: a compound assignment whose only side effect is the assignment itself (no + // method or property reads triggering single-pass) was taking the side-effect-free fast + // path. The fast path ran the assignment once, then EvaluateAllSubExpressions ran it + // again on the failure-diagnostic path. RequiresSinglePassEvaluation now recognizes + // assignment node types so these expressions always take the single-pass evaluator. + public void That_CompoundAssignmentOnField_AppliesExactlyOnceOnFailure() + { + var container = new MutableContainer(); + FieldInfo innerFi = typeof(MutableContainer).GetField(nameof(MutableContainer.Inner))!; + FieldInfo valueFi = typeof(MutableBox).GetField(nameof(MutableBox.Value))!; + + MemberExpression innerAccess = Expression.Field(Expression.Constant(container), innerFi); + MemberExpression valueAccess = Expression.Field(innerAccess, valueFi); + BinaryExpression addAssign = Expression.AddAssign(valueAccess, Expression.Constant(7)); + BinaryExpression body = Expression.LessThan(addAssign, Expression.Constant(0)); + var lambda = Expression.Lambda>(body); + + Action act = () => Assert.That(lambda); + + act.Should().Throw(); + // Exactly one += 7 must apply — not two. + container.Inner.Value.Should().Be(7); + } + + // Regression: assigning to a member whose receiver chain has a side effect (e.g., + // `provider.GetBox().Value = 42`) previously ran the receiver method twice because the + // rebuild kept the original (unevaluated) Left in place. The Left's sub-children are + // now substituted with cached constants while the writable MemberExpression wrapper is + // preserved, so the receiver runs exactly once. + public void That_AssignToFieldOfSideEffectingReceiver_EvaluatesReceiverExactlyOnce() + { + var provider = new MutableBoxProvider(); + + MethodInfo getBoxMethod = typeof(MutableBoxProvider).GetMethod(nameof(MutableBoxProvider.GetBox))!; + MethodCallExpression getBoxCall = Expression.Call(Expression.Constant(provider), getBoxMethod); + FieldInfo valueFi = typeof(MutableBox).GetField(nameof(MutableBox.Value))!; + MemberExpression valueAccess = Expression.Field(getBoxCall, valueFi); + BinaryExpression assign = Expression.Assign(valueAccess, Expression.Constant(42)); + BinaryExpression body = Expression.LessThan(assign, Expression.Constant(0)); + var lambda = Expression.Lambda>(body); + + Action act = () => Assert.That(lambda); + act.Should().Throw(); + + // Receiver method (GetBox) must run exactly once. + provider.Calls.Should().Be(1); + provider.Box.Value.Should().Be(42); + } + + // Regression: when the Left of Expression.Assign is a property, the wrapper must NOT be + // evaluated to populate the cache before the assignment runs. Previously, EvaluateAllSubExpressions + // invoked the getter once to capture a pre-assignment value, then the rebuilt Assign invoked + // the setter — leaving the getter unused but having been called. Plain Assign does not need + // to read the current value, so the getter should run zero times. + public void That_AssignToProperty_DoesNotInvokeGetter() + { + var holder = new CountingPropertyHolder(); + PropertyInfo valuePi = typeof(CountingPropertyHolder).GetProperty(nameof(CountingPropertyHolder.Value))!; + MemberExpression valueAccess = Expression.Property(Expression.Constant(holder), valuePi); + BinaryExpression assign = Expression.Assign(valueAccess, Expression.Constant(42)); + BinaryExpression body = Expression.LessThan(assign, Expression.Constant(0)); + var lambda = Expression.Lambda>(body); + + Action act = () => Assert.That(lambda); + + act.Should().Throw(); + // Plain Assign on a property: setter once, getter never. + holder.GetCalls.Should().Be(0); + holder.SetCalls.Should().Be(1); + } + + // Regression: when the Left of a compound assignment (e.g., AddAssign) is a property, the + // getter must run exactly once — inside the rebuilt compound assignment. Previously the + // getter was invoked twice: once by EvaluateAllSubExpressions(Left) to cache the pre-value, + // and once again by the rebuilt AddAssign (which must read the current value). The fix walks + // only the Left's sub-children, leaving the getter call to happen exactly once during the + // actual assignment execution. + public void That_CompoundAssignToProperty_InvokesGetterExactlyOnce() + { + var holder = new CountingPropertyHolder(); + PropertyInfo valuePi = typeof(CountingPropertyHolder).GetProperty(nameof(CountingPropertyHolder.Value))!; + MemberExpression valueAccess = Expression.Property(Expression.Constant(holder), valuePi); + BinaryExpression addAssign = Expression.AddAssign(valueAccess, Expression.Constant(7)); + BinaryExpression body = Expression.LessThan(addAssign, Expression.Constant(0)); + var lambda = Expression.Lambda>(body); + + Action act = () => Assert.That(lambda); + + act.Should().Throw(); + // Compound assignment on a property: getter once (to read), setter once (to write). + holder.GetCalls.Should().Be(1); + holder.SetCalls.Should().Be(1); + } + + private sealed class CountingPropertyHolder + { +#pragma warning disable IDE0032 // Use auto property - intentional: Value has side-effecting accessors that need a backing field. + private int _value; +#pragma warning restore IDE0032 + + public int GetCalls { get; private set; } + + public int SetCalls { get; private set; } + + public int Value + { + get + { + GetCalls++; + return _value; + } + + set + { + SetCalls++; + _value = value; + } + } + } + + private sealed class MutableBoxProvider + { + public int Calls { get; private set; } + + public MutableBox Box { get; } = new(); + + public MutableBox GetBox() + { + Calls++; + return Box; + } + } + + private sealed class CountingComputeValue + { + public int Calls { get; private set; } + + public int Get() + { + Calls++; + return 42; + } + } + // Regression: a member whose static type is Func/Action but whose runtime value is null should // still appear in the failure details. Filtering must remain runtime-typed (via the cached // value's GetType()) rather than static-typed at analysis time.