From 3e769edc91885e5d3a2703bad32b6625fddefe1e Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Mon, 10 Apr 2023 08:21:53 -0700 Subject: [PATCH 01/11] Expression formatting --- .../ExpressionFormatter.cs | 344 ++++++++++++++++++ .../IntKeyedMemoryPool.cs | 98 +++++ .../ReverseStringBuilder.cs | 234 ++++++++++++ ...Microsoft.AspNetCore.Components.Web.csproj | 1 + .../Web/test/Forms/ExpressionFormatterTest.cs | 179 +++++++++ .../test/Forms/ReverseStringBuilderTest.cs | 165 +++++++++ 6 files changed, 1021 insertions(+) create mode 100644 src/Components/Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs create mode 100644 src/Components/Web/src/Forms/ExpressionFormatting/IntKeyedMemoryPool.cs create mode 100644 src/Components/Web/src/Forms/ExpressionFormatting/ReverseStringBuilder.cs create mode 100644 src/Components/Web/test/Forms/ExpressionFormatterTest.cs create mode 100644 src/Components/Web/test/Forms/ReverseStringBuilderTest.cs diff --git a/src/Components/Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs b/src/Components/Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs new file mode 100644 index 000000000000..cb702bbc3821 --- /dev/null +++ b/src/Components/Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs @@ -0,0 +1,344 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Concurrent; +using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; +using System.Globalization; +using System.Linq; +using System.Linq.Expressions; +using System.Reflection; + +namespace Microsoft.AspNetCore.Components.Forms; + +internal sealed class ExpressionFormatter +{ + internal const int StackallocBufferSize = 128; + internal const int EstimatedStringLengthPadding = 8; + + // Return value of 'true' means non-constant result, 'false' means constant. + private delegate bool IndexArgumentFormatter(Expression expression, ref ReverseStringBuilder builder); + + private readonly ConcurrentDictionary _topLevelExpressionInfoCache = new(); + private readonly ConcurrentDictionary _indexArgumentFormatterCache = new(); + + public string FormatLambda(LambdaExpression lambdaExpression) + { + if (_topLevelExpressionInfoCache.TryGetValue(lambdaExpression, out var expressionInfo)) + { + if (expressionInfo.CachedResult is { } cachedResult) + { + // We've seen this expression before and know it yields a constant result, + // so we'll return the cached result. + return cachedResult; + } + + var estimatedLength = expressionInfo.EstimatedLength; + Debug.Assert(estimatedLength >= 0); + + // We've seen the expression before, but it may not format the same way every time. + // We use the previous formatted string length as an estimate for the new formatted string. + // Some extra padding gets added to avoid allocating an extra buffer if we exceed + // the estimated amount. + return FormatLambdaCore(lambdaExpression, new(estimatedLength + EstimatedStringLengthPadding)); + } + else + { + // Either we haven't seen the expression before, or its formatted representation fit + // within the stack-allocated buffer last time. In either case, we provide a stack-allocated + // buffer to hold the initial contents of the formatted string. Additional buffers will be + // allocated by the ReverseStringBuilder if we exceed this initial buffer. + Span initialBuffer = stackalloc char[StackallocBufferSize]; + return FormatLambdaCore(lambdaExpression, new(initialBuffer)); + } + } + + private string FormatLambdaCore(LambdaExpression lambdaExpression, ReverseStringBuilder builder) + { + var node = lambdaExpression.Body; + var wasLastExpressionMemberAccess = false; + var shouldNotCache = false; + + while (node is not null) + { + switch (node.NodeType) + { + case ExpressionType.Call: + var methodCallExpression = (MethodCallExpression)node; + + if (!IsSingleArgumentIndexer(methodCallExpression)) + { + throw new InvalidOperationException("Method calls cannot be formatted."); + } + + if (wasLastExpressionMemberAccess) + { + wasLastExpressionMemberAccess = false; + builder.InsertFront("."); + } + + builder.InsertFront("]"); + shouldNotCache |= FormatIndexArgument(methodCallExpression.Arguments.Single(), ref builder); + builder.InsertFront("["); + node = methodCallExpression.Object; + break; + + case ExpressionType.ArrayIndex: + var binaryExpression = (BinaryExpression)node; + + if (wasLastExpressionMemberAccess) + { + wasLastExpressionMemberAccess = false; + builder.InsertFront("."); + } + + builder.InsertFront("]"); + shouldNotCache |= FormatIndexArgument(binaryExpression.Right, ref builder); + builder.InsertFront("["); + node = binaryExpression.Left; + break; + + case ExpressionType.MemberAccess: + var memberExpression = (MemberExpression)node; + var nextNode = memberExpression.Expression; + + // FIXME: Better way of detecting this? + if (nextNode?.Type.Name.StartsWith('<') ?? false) + { + // The next node has a compiler-generated closure type, + // which means the current member access is on the captured model. + // We don't want to include the model variable name in the generated + // string, so we exit. + node = null; + break; + } + + if (wasLastExpressionMemberAccess) + { + builder.InsertFront("."); + } + wasLastExpressionMemberAccess = true; + + var name = memberExpression.Member.Name; + builder.InsertFront(name); + + node = nextNode; + break; + + default: + // Unsupported expression type. + // TODO: Should we throw here? + node = null; + break; + } + } + + var result = builder.ToString(); + + if (shouldNotCache) + { + if (result.Length < StackallocBufferSize - EstimatedStringLengthPadding) + { + // We can be fairly certain that the formatted string will fit + // within a stack-allocated buffer next time, so let's not estimate + // a size to use for a heap-allocated buffer. + } + else + { + // We can't cache the string, but we can make a good guess on how big the + // heap-allocated buffer should be next time. + _topLevelExpressionInfoCache[lambdaExpression] = new() + { + EstimatedLength = result.Length, + }; + } + } + else + { + // Cache the result to return next time. + _topLevelExpressionInfoCache[lambdaExpression] = new() + { + CachedResult = result, + }; + } + + builder.Dispose(); + + return result; + } + + [UnconditionalSuppressMessage("Trimming", "IL2072", Justification = "The relevant members should be preserved since they were referenced in a Linq expression")] + private static bool IsSingleArgumentIndexer(Expression expression) + { + // TODO: This was copied from MVC. Investigate if we need to change anything. + + if (expression is not MethodCallExpression methodExpression || methodExpression.Arguments.Count != 1) + { + return false; + } + + var declaringType = methodExpression.Method.DeclaringType; + if (declaringType is null) + { + return false; + } + + // Check whether GetDefaultMembers() (if present in CoreCLR) would return a member of this type. Compiler + // names the indexer property, if any, in a generated [DefaultMember] attribute for the containing type. + var defaultMember = declaringType.GetCustomAttribute(inherit: true); + if (defaultMember is null) + { + return false; + } + + // Find default property (the indexer) and confirm its getter is the method in this expression. + var runtimeProperties = declaringType.GetRuntimeProperties(); + if (runtimeProperties is null) + { + return false; + } + + foreach (var property in runtimeProperties) + { + if ((string.Equals(defaultMember.MemberName, property.Name, StringComparison.Ordinal) && + property.GetMethod == methodExpression.Method)) + { + return true; + } + } + + return false; + } + + private bool FormatIndexArgument( + Expression indexExpression, + ref ReverseStringBuilder builder) + { + if (!_indexArgumentFormatterCache.TryGetValue(indexExpression, out var format)) + { + format = MakeFormatter(indexExpression); + _indexArgumentFormatterCache[indexExpression] = format; + } + + return format(indexExpression, ref builder); + } + + private static IndexArgumentFormatter MakeFormatter(Expression indexExpression) + { + switch (indexExpression.NodeType) + { + case ExpressionType.MemberAccess: + var memberExpression = (MemberExpression)indexExpression; + return CreateCapturedVariableFormatter(memberExpression); + case ExpressionType.Constant: + var constantExpression = (ConstantExpression)indexExpression; + return CreateConstantIndexFormatter(constantExpression); + default: + throw new InvalidOperationException($"Unable to evaluate index expressions of type '{indexExpression.GetType().Name}'."); + } + } + + private static IndexArgumentFormatter CreateCapturedVariableFormatter(MemberExpression memberExpression) + { + var memberType = memberExpression.Type; + + if (memberType == typeof(int)) + { + // Use the "INumber" string builder overload when possible because it doesn't allocate + // an extra string. + // TODO: Consider handling ISpanFormattable types more generally. That way we can write to + // rented arrays rather than allocate new strings. + var func = CompileMemberEvaluator(memberExpression); + + return (Expression _, ref ReverseStringBuilder builder) => + { + var value = func.Invoke(); + builder.InsertFront(value); + return true; + }; + } + + if (memberType == typeof(string)) + { + var func = CompileMemberEvaluator(memberExpression); + + return (Expression _, ref ReverseStringBuilder builder) => + { + var value = func.Invoke(); + builder.InsertFront(value); + return true; + }; + } + + if (typeof(IFormattable).IsAssignableFrom(memberType)) + { + var func = CompileMemberEvaluator(memberExpression); + + return (Expression _, ref ReverseStringBuilder builder) => + { + var value = func.Invoke(); + builder.InsertFront(value.ToString(null, CultureInfo.InvariantCulture)); + return true; + }; + } + + throw new InvalidOperationException($"Cannot format an index argument of type '{memberType}'."); + + static Func CompileMemberEvaluator(MemberExpression memberExpression) + { + var convertExpression = Expression.Convert(memberExpression, typeof(TResult)); + var lambdaExpression = Expression.Lambda>(convertExpression); + return lambdaExpression.Compile(); + } + } + + private static IndexArgumentFormatter CreateConstantIndexFormatter(ConstantExpression constantExpression) + { + // As much as possible, we return static delegates to avoid creating closures. + var constantValue = constantExpression.Value; + + if (constantValue is null) + { + // TODO: Should we literally write out "null" in the generated string? + return static (Expression _, ref ReverseStringBuilder _) => false; + } + + var constantType = constantValue.GetType(); + + if (constantType == typeof(int)) + { + return static (Expression expression, ref ReverseStringBuilder builder) => + { + var value = (int)((ConstantExpression)expression).Value!; + builder.InsertFront(value); + return false; + }; + } + + if (constantType == typeof(string)) + { + return static (Expression expression, ref ReverseStringBuilder builder) => + { + var value = (string)((ConstantExpression)expression).Value!; + builder.InsertFront(value); + return false; + }; + } + + if (constantValue is IFormattable formattable) + { + // In this case, we prefer to allocate a string and create a closure once + // instead of avoiding the closure but allocating a string on every call. + var formattedValue = formattable.ToString(null, CultureInfo.InvariantCulture); + return (Expression _, ref ReverseStringBuilder builder) => + { + builder.InsertFront(formattedValue); + return false; + }; + } + + throw new InvalidOperationException($"Cannot format an index argument of type '{constantType}'."); + } + + private readonly record struct FormattedExpressionInfo(string? CachedResult, int EstimatedLength); +} diff --git a/src/Components/Web/src/Forms/ExpressionFormatting/IntKeyedMemoryPool.cs b/src/Components/Web/src/Forms/ExpressionFormatting/IntKeyedMemoryPool.cs new file mode 100644 index 000000000000..01da1e7bb77a --- /dev/null +++ b/src/Components/Web/src/Forms/ExpressionFormatting/IntKeyedMemoryPool.cs @@ -0,0 +1,98 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Buffers; + +namespace Microsoft.AspNetCore.Components.Forms; + +// In ReverseStringBuilder, we use a 'fixed' array to track the buffers +// we use to build the string. However, fixed arrays cannot contain managed types, +// so we instead store integer keys that we can use to fetch the actual buffers. +// This type serves as a memory pool whose rented memory can be fetched and returned +// using integer keys. +internal sealed class IntKeyedMemoryPool +{ + public static readonly IntKeyedMemoryPool Shared = new(); + + private readonly List _pool = new(); + + private int _nextFree = -1; + + public int Rent(int length, out Memory result) + { + var array = ArrayPool.Shared.Rent(length); + int id; + + lock (this) + { + if (_nextFree < 0) + { + id = _pool.Count; + var rentedMemory = new RentedMemory(id, array, length); + _pool.Add(rentedMemory); + } + else + { + id = _nextFree; + _nextFree = _pool[_nextFree].Id; + var rentedMemory = new RentedMemory(id, array, length); + _pool[id] = rentedMemory; + } + } + + result = new(array, 0, length); + return id; + } + + public Memory GetRentedMemory(int id) + { + RentedMemory rentedMemory; + + lock (this) + { + rentedMemory = GetRentedMemoryCore(id); + } + + return new(rentedMemory.Array, 0, rentedMemory.Length); + } + + public void Return(int id) + { + RentedMemory rentedMemory; + + lock (this) + { + rentedMemory = GetRentedMemoryCore(id); + _pool[id] = rentedMemory with { Id = _nextFree }; + _nextFree = id; + } + + ArrayPool.Shared.Return(rentedMemory.Array); + } + + // This method assumes it's being called from within a 'lock' statement. + private RentedMemory GetRentedMemoryCore(int id) + { + if (id < 0 || id > _pool.Count - 1) + { + throw UnknownMemoryIdException(id); + } + + var rentedMemory = _pool[id]; + if (rentedMemory.Id != id) + { + // The memory has already been returned to the pool and is now part of the + // "free list". + throw UnknownMemoryIdException(id); + } + + return rentedMemory; + + static InvalidOperationException UnknownMemoryIdException(int id) + => new($"Unknown rented memory ID '{id}'."); + } + + // If 'Id' is different than the RentedMemory's index in the pool, it acts + // as a pointer to the next 'free' RentedMemory. + private readonly record struct RentedMemory(int Id, T[] Array, int Length); +} diff --git a/src/Components/Web/src/Forms/ExpressionFormatting/ReverseStringBuilder.cs b/src/Components/Web/src/Forms/ExpressionFormatting/ReverseStringBuilder.cs new file mode 100644 index 000000000000..151b957d12e0 --- /dev/null +++ b/src/Components/Web/src/Forms/ExpressionFormatting/ReverseStringBuilder.cs @@ -0,0 +1,234 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics; +using System.Globalization; +using System.Numerics; + +namespace Microsoft.AspNetCore.Components.Forms; + +internal unsafe ref struct ReverseStringBuilder +{ + public const int FixedMemorySlotLength = 4; + public const int MinimumRentedMemorySize = 128; + + private static readonly IntKeyedMemoryPool s_memoryPool = IntKeyedMemoryPool.Shared; + + private int _nextEndIndex; + private int _filledMemoryTotalLength = 0; + private int _rentedMemoryIndex; + private Span _currentBuffer; + private List? _fallbackRentedMemoryIds; + + private fixed int _rentedMemoryIds[FixedMemorySlotLength]; + + // For testing. + internal readonly int RentedMemoryIndex => _rentedMemoryIndex; + + public ReverseStringBuilder(int conservativeEstimatedStringLength) + { + _rentedMemoryIds[_rentedMemoryIndex] = s_memoryPool.Rent(conservativeEstimatedStringLength, out var initialRentedMemory); + _currentBuffer = initialRentedMemory.Span; + _nextEndIndex = _currentBuffer.Length; + _rentedMemoryIndex = 0; + } + + public ReverseStringBuilder(Span initialBuffer) + { + _rentedMemoryIndex = -1; + _currentBuffer = initialBuffer; + _nextEndIndex = _currentBuffer.Length; + } + + public void InsertFront(scoped ReadOnlySpan span) + { + var startIndex = _nextEndIndex - span.Length; + if (startIndex >= 0) + { + // The common case. There is enough space in the current buffer to copy the given span. + // No additional work needs to be done here after the copy. + span.CopyTo(_currentBuffer[startIndex..]); + _nextEndIndex = startIndex; + return; + } + + // There wasn't enough space in the current buffer. + // What we do next depends on whether we're writing to the provided "initial" buffer or a rented one. + + if (_rentedMemoryIndex < 0) + { + // We've been writing to a stack-allocated buffer, but there is no more room on the stack. + // We rent new memory with a length sufficiently larger than the initial buffer + // and copy the contents over. + + // Using multiple buffers requires us to use string.Create() to generate the final string. + // We can't use the stack-allocated buffer in that case, because it would + // require moving the Span (which by definition lives on the stack) to the heap. + // Hence the reason we do this copy up front. Making the new buffer sufficiently large + // also gives us some wiggle room to take the "happy path" during string generation and + // avoid using string.Create(). + + // If ever, this should ideally only happen in one case: We're seeing an expression for + // the first time and its length was larger than the stack-allocated buffer. + // Assuming we have an accurate but conservative length estimate, we will either never + // need to rent from the array pool (if the string is short enough), or we'll ask for an + // appropriate intial array size so we never run out of space. + + var remainingLength = -startIndex; + var sizeToRent = _currentBuffer.Length + Math.Max(MinimumRentedMemorySize, remainingLength * 2); + var rentedMemoryId = s_memoryPool.Rent(sizeToRent, out var rentedMemory); + + _rentedMemoryIndex = 0; + _rentedMemoryIds[_rentedMemoryIndex] = rentedMemoryId; + + var newBuffer = rentedMemory.Span; + _nextEndIndex = newBuffer.Length - _currentBuffer.Length; + _currentBuffer.CopyTo(newBuffer[_nextEndIndex..]); + _currentBuffer = newBuffer; + + startIndex = _nextEndIndex - span.Length; + span.CopyTo(_currentBuffer[startIndex..]); + _nextEndIndex = startIndex; + } + else + { + // We can't fit the whole string in the current heap-allocated buffer. + // Copy as much as we can to the current buffer, rent a new buffer, and + // continue copying the remaining contents. + var remainingLength = -startIndex; + span[remainingLength..].CopyTo(_currentBuffer); + span = span[..remainingLength]; + + var sizeToRent = Math.Max(MinimumRentedMemorySize, remainingLength * 2); + var rentedMemoryId = s_memoryPool.Rent(sizeToRent, out var rentedMemory); + + _rentedMemoryIndex++; + _filledMemoryTotalLength += _currentBuffer.Length; + + if (_rentedMemoryIndex < FixedMemorySlotLength) + { + _rentedMemoryIds[_rentedMemoryIndex] = rentedMemoryId; + } + else + { + _fallbackRentedMemoryIds ??= new(); + _fallbackRentedMemoryIds.Add(rentedMemoryId); + } + + _currentBuffer = rentedMemory.Span; + + startIndex = _currentBuffer.Length - remainingLength; + span.CopyTo(_currentBuffer[startIndex..]); + _nextEndIndex = startIndex; + } + } + + public void InsertFront(TNumber value) where TNumber : INumber, ISpanFormattable + { + // This is large enough for any integer value (10 digits plus the possible sign). + // We won't try to optimize for anything larger. + Span result = stackalloc char[11]; + + if (value.TryFormat(result, out var charsWritten, default, default)) + { + InsertFront(result[..charsWritten]); + } + else + { + InsertFront(value.ToString(null, CultureInfo.InvariantCulture)); + } + } + + public override string ToString() + { + if (_rentedMemoryIndex <= 0) + { + // Only one buffer was used, so we can create the string directly. + // This will happen in the most common cases: + // 1. We didn't have an initial string length estimate, but the string was shorter than the size + // of the initial buffer (either stack-allocated or copied to the heap). Or... + // 2. We were provided a string length estimate, and the resulting string was shorter than that estimate. + return new(_currentBuffer[_nextEndIndex..]); + } + + var totalLength = _filledMemoryTotalLength + (_currentBuffer.Length - _nextEndIndex); + return string.Create(totalLength, new BufferCollection(ref this), CombineBuffers); + } + + public readonly void Dispose() + { + for (var i = 0; i <= _rentedMemoryIndex; i++) + { + // TODO: We can probably avoid doing this check on every iteration. + // Note that this also isn't super important to optimize because it will + // be extremely rare to ever rent more than 2 buffers. + var memoryId = i < FixedMemorySlotLength + ? _rentedMemoryIds[i] + : _fallbackRentedMemoryIds?[i - FixedMemorySlotLength] ?? throw new UnreachableException(); + s_memoryPool.Return(memoryId); + } + } + + private static void CombineBuffers(Span span, BufferCollection buffers) + { + Debug.Assert(buffers.Length > 0); + + for (var i = buffers.Length - 1; i >= 0; i--) + { + Debug.Assert(span.Length > 0); + + var rentedBuffer = buffers[i]; + rentedBuffer.CopyTo(span); + span = span[rentedBuffer.Length..]; + } + + Debug.Assert(span.Length == 0); + } + + private unsafe struct BufferCollection + { + private readonly int _lastBufferEndIndex; + private readonly List? _fallbackRentedMemoryIds; + + private fixed int _rentedMemoryIds[FixedMemorySlotLength]; + + public readonly int Length { get; } + + public BufferCollection(ref ReverseStringBuilder source) + { + _lastBufferEndIndex = source._nextEndIndex; + _fallbackRentedMemoryIds = source._fallbackRentedMemoryIds; + + for (var i = 0; i < FixedMemorySlotLength; i++) + { + _rentedMemoryIds[i] = source._rentedMemoryIds[i]; + } + + Length = source._rentedMemoryIndex + 1; + } + + public readonly Span this[int index] + { + get + { + // TODO: We can probably move some of this logic to the caller + // and avoid doing this check on every iteration of the loop. + // Note that this also isn't super important to optimize because it will + // be extremely rare to ever rent more than 2 buffers. + var memoryId = index < FixedMemorySlotLength + ? _rentedMemoryIds[index] + : _fallbackRentedMemoryIds?[index - FixedMemorySlotLength] ?? throw new ArgumentOutOfRangeException(nameof(index)); + + var rentedMemory = s_memoryPool.GetRentedMemory(memoryId); + var rentedSpan = rentedMemory.Span; + + if (index == Length - 1) + { + rentedSpan = rentedSpan[_lastBufferEndIndex..]; + } + + return rentedSpan; + } + } + } +} diff --git a/src/Components/Web/src/Microsoft.AspNetCore.Components.Web.csproj b/src/Components/Web/src/Microsoft.AspNetCore.Components.Web.csproj index ed97ef06d0c2..37e7786f8349 100644 --- a/src/Components/Web/src/Microsoft.AspNetCore.Components.Web.csproj +++ b/src/Components/Web/src/Microsoft.AspNetCore.Components.Web.csproj @@ -10,6 +10,7 @@ true false + true diff --git a/src/Components/Web/test/Forms/ExpressionFormatterTest.cs b/src/Components/Web/test/Forms/ExpressionFormatterTest.cs new file mode 100644 index 000000000000..5de0dcc869cd --- /dev/null +++ b/src/Components/Web/test/Forms/ExpressionFormatterTest.cs @@ -0,0 +1,179 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Linq.Expressions; + +namespace Microsoft.AspNetCore.Components.Forms; + +public class ExpressionFormatterTest +{ + [Fact] + public void Works_MemberAccessOnly() + { + // Arrange + var formatter = new ExpressionFormatter(); + var person = new Person(); + LambdaExpression lambda = () => person.Parent.Name; + + // Act + var result = formatter.FormatLambda(lambda); + + // Assert + Assert.Equal("Parent.Name", result); + } + + [Fact] + public void Works_MemberAccessWithConstIndex() + { + // Arrange + var formatter = new ExpressionFormatter(); + var person = new Person(); + LambdaExpression lambda = () => person.Parent.Children[3].Name; + + // Act + var result = formatter.FormatLambda(lambda); + + // Assert + Assert.Equal("Parent.Children[3].Name", result); + } + + [Fact] + public void Works_MemberAccessWithConstIndex_SameLambdaMultipleTimes() + { + // TODO: Somehow validate the caching mechanism. + + // Arrange + var formatter = new ExpressionFormatter(); + var person = new Person(); + var result = new string[3]; + LambdaExpression lambda = () => person.Parent.Children[3].Name; + + // Act + for (var i = 0; i < result.Length; i++) + { + result[i] = formatter.FormatLambda(lambda); + } + + // Assert + Assert.Equal("Parent.Children[3].Name", result[0]); + Assert.Equal("Parent.Children[3].Name", result[1]); + Assert.Equal("Parent.Children[3].Name", result[2]); + } + + [Fact] + public void Works_MemberAccessWithVariableIndex() + { + // Arrange + var formatter = new ExpressionFormatter(); + var person = new Person(); + var i = 42; + LambdaExpression lambda = () => person.Parent.Children[i].Name; + + // Act + var result = formatter.FormatLambda(lambda); + + // Assert + Assert.Equal("Parent.Children[42].Name", result); + } + + [Fact] + public void Works_ForLoopIteraterVariableIndex_Short() + { + // Arrange + var formatter = new ExpressionFormatter(); + var person = new Person(); + var i = 0; + LambdaExpression lambda = () => person.Parent.Children[i].Name; + var result = new string[3]; + + // Act + for (; i < result.Length; i++) + { + result[i] = formatter.FormatLambda(lambda); + } + + // Assert + Assert.Equal("Parent.Children[0].Name", result[0]); + Assert.Equal("Parent.Children[1].Name", result[1]); + Assert.Equal("Parent.Children[2].Name", result[2]); + } + + [Fact] + public void Works_ForLoopIteraterVariableIndex_Long() + { + // Arrange + var formatter = new ExpressionFormatter(); + var person = new Person(); + var i = 0; + LambdaExpression lambda = () => person.Parent.Parent.Children[i].Parent.Children[i].Children[i].Name; + var result = new string[3]; + + // Act + for (; i < result.Length; i++) + { + result[i] = formatter.FormatLambda(lambda); + } + + // Assert + Assert.Equal("Parent.Parent.Children[0].Parent.Children[0].Children[0].Name", result[0]); + Assert.Equal("Parent.Parent.Children[1].Parent.Children[1].Children[1].Name", result[1]); + Assert.Equal("Parent.Parent.Children[2].Parent.Children[2].Children[2].Name", result[2]); + } + + [Fact] + public void Works_ForLoopIteraterVariableIndex_SuperLong() + { + // Arrange + var formatter = new ExpressionFormatter(); + var person = new Person(); + var i = 0; + LambdaExpression lambda = () => person.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Children[i].Age; + var result = new string[3]; + + // Act + for (; i < result.Length; i++) + { + result[i] = formatter.FormatLambda(lambda); + } + + // Assert + Assert.Equal("Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Children[0].Age", result[0]); + Assert.Equal("Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Children[1].Age", result[1]); + Assert.Equal("Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Children[2].Age", result[2]); + } + + [Fact] + public void Works_ForLoopIteraterVariableIndex_NonArrayType() + { + // Arrange + var formatter = new ExpressionFormatter(); + var person = new Person(); + var i = 0; + LambdaExpression lambda = () => person.Parent.Nicknames[i]; + var result = new string[3]; + + // Act + for (; i < result.Length; i++) + { + result[i] = formatter.FormatLambda(lambda); + } + + // Assert + Assert.Equal("Parent.Nicknames[0]", result[0]); + Assert.Equal("Parent.Nicknames[1]", result[1]); + Assert.Equal("Parent.Nicknames[2]", result[2]); + } + + private class Person + { + public string Name { get; init; } + + public int Age { get; init; } + + public Person Parent { get; init; } + + public Person[] Children { get; init; } = Array.Empty(); + + public IReadOnlyList Nicknames { get; init; } = Array.Empty(); + } +} diff --git a/src/Components/Web/test/Forms/ReverseStringBuilderTest.cs b/src/Components/Web/test/Forms/ReverseStringBuilderTest.cs new file mode 100644 index 000000000000..171aacce404d --- /dev/null +++ b/src/Components/Web/test/Forms/ReverseStringBuilderTest.cs @@ -0,0 +1,165 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Microsoft.AspNetCore.Components.Forms; + +public class ReverseStringBuilderTest +{ + [Fact] + public void ToString_ReturnsEmptyString_WhenNoWritesOccur() + { + // Arrange + Span initialBuffer = stackalloc char[128]; + using var builder = new ReverseStringBuilder(initialBuffer); + + // Act + var result = builder.ToString(); + + // Assert + Assert.Equal(string.Empty, result); + } + + [Fact] + public void ToString_ReturnsEmptyString_WhenBufferIsEmpty() + { + // Arrange + using var builder = new ReverseStringBuilder(Span.Empty); + + // Act + var result = builder.ToString(); + + // Assert + Assert.Equal(string.Empty, result); + } + + [Fact] + public void ToString_Works_WhenOnlyUsingStackAllocatedBuffer() + { + // Arrange + Span initialBuffer = stackalloc char[128]; + using var builder = new ReverseStringBuilder(initialBuffer); + + // Act + builder.InsertFront("world!"); + builder.InsertFront(" "); + builder.InsertFront(","); + builder.InsertFront("Hello"); + var result = builder.ToString(); + + // Assert + Assert.Equal("Hello, world!", result); + Assert.Equal(-1, builder.RentedMemoryIndex); + } + + [Fact] + public void ToString_Works_WithNumbers() + { + // Arrange + Span initialBuffer = stackalloc char[128]; + using var builder = new ReverseStringBuilder(initialBuffer); + + // Act + builder.InsertFront("worlds!"); + builder.InsertFront(" "); + builder.InsertFront(123); + builder.InsertFront(", "); + builder.InsertFront("Hello"); + var result = builder.ToString(); + + // Assert + Assert.Equal("Hello, 123 worlds!", result); + Assert.Equal(-1, builder.RentedMemoryIndex); + } + + [Fact] + public void ToString_Works_AfterExceedingStackAllocatedBuffer() + { + // Arrange + Span initialBuffer = stackalloc char[8]; + using var builder = new ReverseStringBuilder(initialBuffer); + + // Act + builder.InsertFront("world!"); + builder.InsertFront(" "); + builder.InsertFront(","); + builder.InsertFront("Hello"); + var result = builder.ToString(); + + // Assert + Assert.Equal("Hello, world!", result); + Assert.Equal(0, builder.RentedMemoryIndex); + } + + [Fact] + public void ToString_Works_AfterExpandingIntoMultipleBuffersFromStackAllocatedBuffer() + { + // Arrange + Span initialBuffer = stackalloc char[8]; + using var builder = new ReverseStringBuilder(initialBuffer); + var padding = new string('A', ReverseStringBuilder.MinimumRentedMemorySize + 10); + var expected = padding + "Hello, world!"; + + // Act + builder.InsertFront("world!"); + builder.InsertFront(" "); + builder.InsertFront(","); + builder.InsertFront("Hello"); + builder.InsertFront(padding); + var result = builder.ToString(); + + // Assert + Assert.Equal(expected, result); + Assert.Equal(1, builder.RentedMemoryIndex); + } + + [Fact] + public void ToString_Works_AfterExpandingIntoMultipleBuffersFromEstimatedStringSize() + { + // Arrange + using var builder = new ReverseStringBuilder(8); + var padding = new string('A', ReverseStringBuilder.MinimumRentedMemorySize - 10); + var expected = padding + "Hello, world!"; + + // Act + builder.InsertFront("world!"); + builder.InsertFront(" "); + builder.InsertFront(","); + builder.InsertFront("Hello"); + builder.InsertFront(padding); + var result = builder.ToString(); + + // Assert + Assert.Equal(expected, result); + Assert.Equal(1, builder.RentedMemoryIndex); + } + + [Fact] + public void ToString_Works_AfterUsingFallbackBuffer() + { + // Arrange + using var builder = new ReverseStringBuilder(ReverseStringBuilder.MinimumRentedMemorySize); + var slotCount = ReverseStringBuilder.FixedMemorySlotLength + 3; + var expected = string.Empty; + + // Act + for (var i = 0; i < slotCount; i++) + { + var c = (char)(i + 65); + + // Update the expected string. + expected = new string(c, ReverseStringBuilder.MinimumRentedMemorySize) + expected; + + // Append just one character to ensure we get a buffer with the minimum possible + // length. + builder.InsertFront(c.ToString()); + + // Fill up the rest of the buffer. + var s = new string(c, ReverseStringBuilder.MinimumRentedMemorySize - 1); + builder.InsertFront(s); + } + + var actual = builder.ToString(); + Assert.Equal(expected, actual); + Assert.Equal(slotCount - 1, builder.RentedMemoryIndex); + } +} From 3078a23b4a721673c1fd844729dd4449a7f1f081 Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Mon, 17 Apr 2023 16:27:21 -0700 Subject: [PATCH 02/11] Simplification, started on E2E scenario --- .../ExpressionFormatter.cs | 231 +++++------------- .../IntKeyedMemoryPool.cs | 98 -------- .../ReverseStringBuilder.cs | 165 ++++--------- src/Components/Web/src/Forms/InputBase.cs | 12 + src/Components/Web/src/Forms/InputCheckbox.cs | 9 +- src/Components/Web/src/Forms/InputDate.cs | 9 +- src/Components/Web/src/Forms/InputNumber.cs | 9 +- src/Components/Web/src/Forms/InputSelect.cs | 17 +- src/Components/Web/src/Forms/InputText.cs | 9 +- src/Components/Web/src/Forms/InputTextArea.cs | 9 +- .../Web/test/Forms/ExpressionFormatterTest.cs | 68 ++++-- .../test/Forms/ReverseStringBuilderTest.cs | 46 +--- 12 files changed, 221 insertions(+), 461 deletions(-) delete mode 100644 src/Components/Web/src/Forms/ExpressionFormatting/IntKeyedMemoryPool.cs diff --git a/src/Components/Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs b/src/Components/Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs index cb702bbc3821..ca66c3eb3990 100644 --- a/src/Components/Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs +++ b/src/Components/Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs @@ -2,9 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Collections.Concurrent; -using System.Diagnostics; using System.Diagnostics.CodeAnalysis; -using System.Globalization; using System.Linq; using System.Linq.Expressions; using System.Reflection; @@ -16,46 +14,14 @@ internal sealed class ExpressionFormatter internal const int StackallocBufferSize = 128; internal const int EstimatedStringLengthPadding = 8; - // Return value of 'true' means non-constant result, 'false' means constant. - private delegate bool IndexArgumentFormatter(Expression expression, ref ReverseStringBuilder builder); + private delegate void CapturedValueFormatter(object closure, ref ReverseStringBuilder builder); - private readonly ConcurrentDictionary _topLevelExpressionInfoCache = new(); - private readonly ConcurrentDictionary _indexArgumentFormatterCache = new(); + private readonly ConcurrentDictionary _capturedValueFormatterCache = new(); - public string FormatLambda(LambdaExpression lambdaExpression) + public string FormatLambda(LambdaExpression expression) { - if (_topLevelExpressionInfoCache.TryGetValue(lambdaExpression, out var expressionInfo)) - { - if (expressionInfo.CachedResult is { } cachedResult) - { - // We've seen this expression before and know it yields a constant result, - // so we'll return the cached result. - return cachedResult; - } - - var estimatedLength = expressionInfo.EstimatedLength; - Debug.Assert(estimatedLength >= 0); - - // We've seen the expression before, but it may not format the same way every time. - // We use the previous formatted string length as an estimate for the new formatted string. - // Some extra padding gets added to avoid allocating an extra buffer if we exceed - // the estimated amount. - return FormatLambdaCore(lambdaExpression, new(estimatedLength + EstimatedStringLengthPadding)); - } - else - { - // Either we haven't seen the expression before, or its formatted representation fit - // within the stack-allocated buffer last time. In either case, we provide a stack-allocated - // buffer to hold the initial contents of the formatted string. Additional buffers will be - // allocated by the ReverseStringBuilder if we exceed this initial buffer. - Span initialBuffer = stackalloc char[StackallocBufferSize]; - return FormatLambdaCore(lambdaExpression, new(initialBuffer)); - } - } - - private string FormatLambdaCore(LambdaExpression lambdaExpression, ReverseStringBuilder builder) - { - var node = lambdaExpression.Body; + var builder = new ReverseStringBuilder(stackalloc char[StackallocBufferSize]); + var node = expression.Body; var wasLastExpressionMemberAccess = false; var shouldNotCache = false; @@ -102,7 +68,8 @@ private string FormatLambdaCore(LambdaExpression lambdaExpression, ReverseString var memberExpression = (MemberExpression)node; var nextNode = memberExpression.Expression; - // FIXME: Better way of detecting this? + // FIXME: This doesn't work in all cases. Need a better + // way of detecting when we've reached the model object. if (nextNode?.Type.Name.StartsWith('<') ?? false) { // The next node has a compiler-generated closure type, @@ -127,7 +94,6 @@ private string FormatLambdaCore(LambdaExpression lambdaExpression, ReverseString default: // Unsupported expression type. - // TODO: Should we throw here? node = null; break; } @@ -135,32 +101,7 @@ private string FormatLambdaCore(LambdaExpression lambdaExpression, ReverseString var result = builder.ToString(); - if (shouldNotCache) - { - if (result.Length < StackallocBufferSize - EstimatedStringLengthPadding) - { - // We can be fairly certain that the formatted string will fit - // within a stack-allocated buffer next time, so let's not estimate - // a size to use for a heap-allocated buffer. - } - else - { - // We can't cache the string, but we can make a good guess on how big the - // heap-allocated buffer should be next time. - _topLevelExpressionInfoCache[lambdaExpression] = new() - { - EstimatedLength = result.Length, - }; - } - } - else - { - // Cache the result to return next time. - _topLevelExpressionInfoCache[lambdaExpression] = new() - { - CachedResult = result, - }; - } + // TODO: Top-level caching. builder.Dispose(); @@ -170,8 +111,6 @@ private string FormatLambdaCore(LambdaExpression lambdaExpression, ReverseString [UnconditionalSuppressMessage("Trimming", "IL2072", Justification = "The relevant members should be preserved since they were referenced in a Linq expression")] private static bool IsSingleArgumentIndexer(Expression expression) { - // TODO: This was copied from MVC. Investigate if we need to change anything. - if (expression is not MethodCallExpression methodExpression || methodExpression.Arguments.Count != 1) { return false; @@ -200,8 +139,8 @@ private static bool IsSingleArgumentIndexer(Expression expression) foreach (var property in runtimeProperties) { - if ((string.Equals(defaultMember.MemberName, property.Name, StringComparison.Ordinal) && - property.GetMethod == methodExpression.Method)) + if (string.Equals(defaultMember.MemberName, property.Name, StringComparison.Ordinal) && + property.GetMethod == methodExpression.Method) { return true; } @@ -214,131 +153,89 @@ private bool FormatIndexArgument( Expression indexExpression, ref ReverseStringBuilder builder) { - if (!_indexArgumentFormatterCache.TryGetValue(indexExpression, out var format)) + switch (indexExpression) { - format = MakeFormatter(indexExpression); - _indexArgumentFormatterCache[indexExpression] = format; + case MemberExpression memberExpression when memberExpression.Expression is ConstantExpression constantExpression: + FormatCapturedValue(memberExpression, constantExpression, ref builder); + return true; + case ConstantExpression constantExpression: + FormatConstantValue(constantExpression, ref builder); + return false; + default: + throw new InvalidOperationException($"Unable to evaluate index expressions of type '{indexExpression.GetType().Name}'."); } - - return format(indexExpression, ref builder); } - private static IndexArgumentFormatter MakeFormatter(Expression indexExpression) + private void FormatCapturedValue(MemberExpression memberExpression, ConstantExpression constantExpression, ref ReverseStringBuilder builder) { - switch (indexExpression.NodeType) + var member = memberExpression.Member; + if (!_capturedValueFormatterCache.TryGetValue(member, out var format)) { - case ExpressionType.MemberAccess: - var memberExpression = (MemberExpression)indexExpression; - return CreateCapturedVariableFormatter(memberExpression); - case ExpressionType.Constant: - var constantExpression = (ConstantExpression)indexExpression; - return CreateConstantIndexFormatter(constantExpression); - default: - throw new InvalidOperationException($"Unable to evaluate index expressions of type '{indexExpression.GetType().Name}'."); + format = CreateCapturedValueFormatter(memberExpression); + _capturedValueFormatterCache[member] = format; } + + format(constantExpression.Value!, ref builder); } - private static IndexArgumentFormatter CreateCapturedVariableFormatter(MemberExpression memberExpression) + private static CapturedValueFormatter CreateCapturedValueFormatter(MemberExpression memberExpression) { var memberType = memberExpression.Type; if (memberType == typeof(int)) { - // Use the "INumber" string builder overload when possible because it doesn't allocate - // an extra string. - // TODO: Consider handling ISpanFormattable types more generally. That way we can write to - // rented arrays rather than allocate new strings. var func = CompileMemberEvaluator(memberExpression); - - return (Expression _, ref ReverseStringBuilder builder) => - { - var value = func.Invoke(); - builder.InsertFront(value); - return true; - }; + return (object closure, ref ReverseStringBuilder builder) => builder.InsertFront(func.Invoke(closure)); } - - if (memberType == typeof(string)) + else if (memberType == typeof(string)) { var func = CompileMemberEvaluator(memberExpression); - - return (Expression _, ref ReverseStringBuilder builder) => - { - var value = func.Invoke(); - builder.InsertFront(value); - return true; - }; + return (object closure, ref ReverseStringBuilder builder) => builder.InsertFront(func.Invoke(closure)); } - - if (typeof(IFormattable).IsAssignableFrom(memberType)) + else if (typeof(ISpanFormattable).IsAssignableFrom(memberType)) { - var func = CompileMemberEvaluator(memberExpression); - - return (Expression _, ref ReverseStringBuilder builder) => - { - var value = func.Invoke(); - builder.InsertFront(value.ToString(null, CultureInfo.InvariantCulture)); - return true; - }; + var func = CompileMemberEvaluator(memberExpression); + return (object closure, ref ReverseStringBuilder builder) => builder.InsertFront(func.Invoke(closure)); } - - throw new InvalidOperationException($"Cannot format an index argument of type '{memberType}'."); - - static Func CompileMemberEvaluator(MemberExpression memberExpression) + else if (typeof(IFormattable).IsAssignableFrom(memberType)) { - var convertExpression = Expression.Convert(memberExpression, typeof(TResult)); - var lambdaExpression = Expression.Lambda>(convertExpression); - return lambdaExpression.Compile(); - } - } - - private static IndexArgumentFormatter CreateConstantIndexFormatter(ConstantExpression constantExpression) - { - // As much as possible, we return static delegates to avoid creating closures. - var constantValue = constantExpression.Value; - - if (constantValue is null) - { - // TODO: Should we literally write out "null" in the generated string? - return static (Expression _, ref ReverseStringBuilder _) => false; + var func = CompileMemberEvaluator(memberExpression); + return (object closure, ref ReverseStringBuilder builder) => builder.InsertFront(func.Invoke(closure)); } - - var constantType = constantValue.GetType(); - - if (constantType == typeof(int)) + else { - return static (Expression expression, ref ReverseStringBuilder builder) => - { - var value = (int)((ConstantExpression)expression).Value!; - builder.InsertFront(value); - return false; - }; + throw new InvalidOperationException($"Cannot format an index argument of type '{memberType}'."); } - if (constantType == typeof(string)) + static Func CompileMemberEvaluator(MemberExpression memberExpression) { - return static (Expression expression, ref ReverseStringBuilder builder) => - { - var value = (string)((ConstantExpression)expression).Value!; - builder.InsertFront(value); - return false; - }; + var parameterExpression = Expression.Parameter(typeof(object)); + var convertExpression = Expression.Convert(parameterExpression, memberExpression.Member.DeclaringType!); + var replacedMemberExpression = memberExpression.Update(convertExpression); + var replacedExpression = Expression.Lambda>(replacedMemberExpression, parameterExpression); + return replacedExpression.Compile(); } + } - if (constantValue is IFormattable formattable) - { - // In this case, we prefer to allocate a string and create a closure once - // instead of avoiding the closure but allocating a string on every call. - var formattedValue = formattable.ToString(null, CultureInfo.InvariantCulture); - return (Expression _, ref ReverseStringBuilder builder) => - { - builder.InsertFront(formattedValue); - return false; - }; + private static void FormatConstantValue(ConstantExpression constantExpression, ref ReverseStringBuilder builder) + { + switch (constantExpression.Value) + { + case string s: + builder.InsertFront(s); + break; + case ISpanFormattable spanFormattable: + // This is better than the formattable case because we don't allocate an extra string. + builder.InsertFront(spanFormattable); + break; + case IFormattable formattable: + builder.InsertFront(formattable); + break; + case null: + builder.InsertFront("null"); + break; + case var x: + throw new InvalidOperationException($"Unable to format constant values of type '{x.GetType()}'."); } - - throw new InvalidOperationException($"Cannot format an index argument of type '{constantType}'."); } - - private readonly record struct FormattedExpressionInfo(string? CachedResult, int EstimatedLength); } diff --git a/src/Components/Web/src/Forms/ExpressionFormatting/IntKeyedMemoryPool.cs b/src/Components/Web/src/Forms/ExpressionFormatting/IntKeyedMemoryPool.cs deleted file mode 100644 index 01da1e7bb77a..000000000000 --- a/src/Components/Web/src/Forms/ExpressionFormatting/IntKeyedMemoryPool.cs +++ /dev/null @@ -1,98 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.Buffers; - -namespace Microsoft.AspNetCore.Components.Forms; - -// In ReverseStringBuilder, we use a 'fixed' array to track the buffers -// we use to build the string. However, fixed arrays cannot contain managed types, -// so we instead store integer keys that we can use to fetch the actual buffers. -// This type serves as a memory pool whose rented memory can be fetched and returned -// using integer keys. -internal sealed class IntKeyedMemoryPool -{ - public static readonly IntKeyedMemoryPool Shared = new(); - - private readonly List _pool = new(); - - private int _nextFree = -1; - - public int Rent(int length, out Memory result) - { - var array = ArrayPool.Shared.Rent(length); - int id; - - lock (this) - { - if (_nextFree < 0) - { - id = _pool.Count; - var rentedMemory = new RentedMemory(id, array, length); - _pool.Add(rentedMemory); - } - else - { - id = _nextFree; - _nextFree = _pool[_nextFree].Id; - var rentedMemory = new RentedMemory(id, array, length); - _pool[id] = rentedMemory; - } - } - - result = new(array, 0, length); - return id; - } - - public Memory GetRentedMemory(int id) - { - RentedMemory rentedMemory; - - lock (this) - { - rentedMemory = GetRentedMemoryCore(id); - } - - return new(rentedMemory.Array, 0, rentedMemory.Length); - } - - public void Return(int id) - { - RentedMemory rentedMemory; - - lock (this) - { - rentedMemory = GetRentedMemoryCore(id); - _pool[id] = rentedMemory with { Id = _nextFree }; - _nextFree = id; - } - - ArrayPool.Shared.Return(rentedMemory.Array); - } - - // This method assumes it's being called from within a 'lock' statement. - private RentedMemory GetRentedMemoryCore(int id) - { - if (id < 0 || id > _pool.Count - 1) - { - throw UnknownMemoryIdException(id); - } - - var rentedMemory = _pool[id]; - if (rentedMemory.Id != id) - { - // The memory has already been returned to the pool and is now part of the - // "free list". - throw UnknownMemoryIdException(id); - } - - return rentedMemory; - - static InvalidOperationException UnknownMemoryIdException(int id) - => new($"Unknown rented memory ID '{id}'."); - } - - // If 'Id' is different than the RentedMemory's index in the pool, it acts - // as a pointer to the next 'free' RentedMemory. - private readonly record struct RentedMemory(int Id, T[] Array, int Length); -} diff --git a/src/Components/Web/src/Forms/ExpressionFormatting/ReverseStringBuilder.cs b/src/Components/Web/src/Forms/ExpressionFormatting/ReverseStringBuilder.cs index 151b957d12e0..92655f769e95 100644 --- a/src/Components/Web/src/Forms/ExpressionFormatting/ReverseStringBuilder.cs +++ b/src/Components/Web/src/Forms/ExpressionFormatting/ReverseStringBuilder.cs @@ -1,41 +1,34 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Diagnostics; +using System.Buffers; using System.Globalization; -using System.Numerics; namespace Microsoft.AspNetCore.Components.Forms; internal unsafe ref struct ReverseStringBuilder { - public const int FixedMemorySlotLength = 4; - public const int MinimumRentedMemorySize = 128; + public const int MinimumRentedArraySize = 1024; - private static readonly IntKeyedMemoryPool s_memoryPool = IntKeyedMemoryPool.Shared; + private static readonly ArrayPool s_arrayPool = ArrayPool.Shared; private int _nextEndIndex; - private int _filledMemoryTotalLength = 0; - private int _rentedMemoryIndex; private Span _currentBuffer; - private List? _fallbackRentedMemoryIds; - - private fixed int _rentedMemoryIds[FixedMemorySlotLength]; + private SequenceSegment? _fallbackSequenceSegment; // For testing. - internal readonly int RentedMemoryIndex => _rentedMemoryIndex; + internal int SequenceSegmentCount => _fallbackSequenceSegment?.Count() ?? 0; public ReverseStringBuilder(int conservativeEstimatedStringLength) { - _rentedMemoryIds[_rentedMemoryIndex] = s_memoryPool.Rent(conservativeEstimatedStringLength, out var initialRentedMemory); - _currentBuffer = initialRentedMemory.Span; + var array = s_arrayPool.Rent(conservativeEstimatedStringLength); + _fallbackSequenceSegment = new(array); + _currentBuffer = array; _nextEndIndex = _currentBuffer.Length; - _rentedMemoryIndex = 0; } public ReverseStringBuilder(Span initialBuffer) { - _rentedMemoryIndex = -1; _currentBuffer = initialBuffer; _nextEndIndex = _currentBuffer.Length; } @@ -55,35 +48,18 @@ public void InsertFront(scoped ReadOnlySpan span) // There wasn't enough space in the current buffer. // What we do next depends on whether we're writing to the provided "initial" buffer or a rented one. - if (_rentedMemoryIndex < 0) + if (_fallbackSequenceSegment is null) { // We've been writing to a stack-allocated buffer, but there is no more room on the stack. // We rent new memory with a length sufficiently larger than the initial buffer // and copy the contents over. - - // Using multiple buffers requires us to use string.Create() to generate the final string. - // We can't use the stack-allocated buffer in that case, because it would - // require moving the Span (which by definition lives on the stack) to the heap. - // Hence the reason we do this copy up front. Making the new buffer sufficiently large - // also gives us some wiggle room to take the "happy path" during string generation and - // avoid using string.Create(). - - // If ever, this should ideally only happen in one case: We're seeing an expression for - // the first time and its length was larger than the stack-allocated buffer. - // Assuming we have an accurate but conservative length estimate, we will either never - // need to rent from the array pool (if the string is short enough), or we'll ask for an - // appropriate intial array size so we never run out of space. - var remainingLength = -startIndex; - var sizeToRent = _currentBuffer.Length + Math.Max(MinimumRentedMemorySize, remainingLength * 2); - var rentedMemoryId = s_memoryPool.Rent(sizeToRent, out var rentedMemory); + var sizeToRent = _currentBuffer.Length + Math.Max(MinimumRentedArraySize, remainingLength * 2); + var newBuffer = s_arrayPool.Rent(sizeToRent); + _fallbackSequenceSegment = new(newBuffer); - _rentedMemoryIndex = 0; - _rentedMemoryIds[_rentedMemoryIndex] = rentedMemoryId; - - var newBuffer = rentedMemory.Span; _nextEndIndex = newBuffer.Length - _currentBuffer.Length; - _currentBuffer.CopyTo(newBuffer[_nextEndIndex..]); + _currentBuffer.CopyTo(newBuffer.AsSpan()[_nextEndIndex..]); _currentBuffer = newBuffer; startIndex = _nextEndIndex - span.Length; @@ -99,23 +75,10 @@ public void InsertFront(scoped ReadOnlySpan span) span[remainingLength..].CopyTo(_currentBuffer); span = span[..remainingLength]; - var sizeToRent = Math.Max(MinimumRentedMemorySize, remainingLength * 2); - var rentedMemoryId = s_memoryPool.Rent(sizeToRent, out var rentedMemory); - - _rentedMemoryIndex++; - _filledMemoryTotalLength += _currentBuffer.Length; - - if (_rentedMemoryIndex < FixedMemorySlotLength) - { - _rentedMemoryIds[_rentedMemoryIndex] = rentedMemoryId; - } - else - { - _fallbackRentedMemoryIds ??= new(); - _fallbackRentedMemoryIds.Add(rentedMemoryId); - } - - _currentBuffer = rentedMemory.Span; + var sizeToRent = Math.Max(MinimumRentedArraySize, remainingLength * 2); + var newBuffer = s_arrayPool.Rent(sizeToRent); + _fallbackSequenceSegment = new(newBuffer, _fallbackSequenceSegment); + _currentBuffer = newBuffer; startIndex = _currentBuffer.Length - remainingLength; span.CopyTo(_currentBuffer[startIndex..]); @@ -123,7 +86,7 @@ public void InsertFront(scoped ReadOnlySpan span) } } - public void InsertFront(TNumber value) where TNumber : INumber, ISpanFormattable + public void InsertFront(T value) where T : ISpanFormattable { // This is large enough for any integer value (10 digits plus the possible sign). // We won't try to optimize for anything larger. @@ -135,13 +98,16 @@ public void InsertFront(TNumber value) where TNumber : INumber } else { - InsertFront(value.ToString(null, CultureInfo.InvariantCulture)); + InsertFront((IFormattable)value); } } + public void InsertFront(IFormattable formattable) + => InsertFront(formattable.ToString(null, CultureInfo.InvariantCulture)); + public override string ToString() { - if (_rentedMemoryIndex <= 0) + if (_fallbackSequenceSegment is null) { // Only one buffer was used, so we can create the string directly. // This will happen in the most common cases: @@ -151,83 +117,56 @@ public override string ToString() return new(_currentBuffer[_nextEndIndex..]); } - var totalLength = _filledMemoryTotalLength + (_currentBuffer.Length - _nextEndIndex); - return string.Create(totalLength, new BufferCollection(ref this), CombineBuffers); + return _fallbackSequenceSegment.ToString(_nextEndIndex); } public readonly void Dispose() { - for (var i = 0; i <= _rentedMemoryIndex; i++) - { - // TODO: We can probably avoid doing this check on every iteration. - // Note that this also isn't super important to optimize because it will - // be extremely rare to ever rent more than 2 buffers. - var memoryId = i < FixedMemorySlotLength - ? _rentedMemoryIds[i] - : _fallbackRentedMemoryIds?[i - FixedMemorySlotLength] ?? throw new UnreachableException(); - s_memoryPool.Return(memoryId); - } + _fallbackSequenceSegment?.Dispose(); } - private static void CombineBuffers(Span span, BufferCollection buffers) + private sealed class SequenceSegment : ReadOnlySequenceSegment, IDisposable { - Debug.Assert(buffers.Length > 0); + private readonly char[] _array; - for (var i = buffers.Length - 1; i >= 0; i--) + public SequenceSegment(char[] array, SequenceSegment? next = null) { - Debug.Assert(span.Length > 0); - - var rentedBuffer = buffers[i]; - rentedBuffer.CopyTo(span); - span = span[rentedBuffer.Length..]; + _array = array; + Memory = array; + Next = next; } - Debug.Assert(span.Length == 0); - } - - private unsafe struct BufferCollection - { - private readonly int _lastBufferEndIndex; - private readonly List? _fallbackRentedMemoryIds; - - private fixed int _rentedMemoryIds[FixedMemorySlotLength]; - - public readonly int Length { get; } + // For testing. + internal int Count() + { + var count = 0; + for (var current = this; current is not null; current = current.Next as SequenceSegment) + { + count++; + } + return count; + } - public BufferCollection(ref ReverseStringBuilder source) + public string ToString(int startIndex) { - _lastBufferEndIndex = source._nextEndIndex; - _fallbackRentedMemoryIds = source._fallbackRentedMemoryIds; + RunningIndex = 0; - for (var i = 0; i < FixedMemorySlotLength; i++) + var tail = this; + while (tail.Next is SequenceSegment next) { - _rentedMemoryIds[i] = source._rentedMemoryIds[i]; + next.RunningIndex = tail.RunningIndex + tail.Memory.Length; + tail = next; } - Length = source._rentedMemoryIndex + 1; + var sequence = new ReadOnlySequence(this, startIndex, tail, tail.Memory.Length); + return sequence.ToString(); } - public readonly Span this[int index] + public void Dispose() { - get + for (var current = this; current is not null; current = current.Next as SequenceSegment) { - // TODO: We can probably move some of this logic to the caller - // and avoid doing this check on every iteration of the loop. - // Note that this also isn't super important to optimize because it will - // be extremely rare to ever rent more than 2 buffers. - var memoryId = index < FixedMemorySlotLength - ? _rentedMemoryIds[index] - : _fallbackRentedMemoryIds?[index - FixedMemorySlotLength] ?? throw new ArgumentOutOfRangeException(nameof(index)); - - var rentedMemory = s_memoryPool.GetRentedMemory(memoryId); - var rentedSpan = rentedMemory.Span; - - if (index == Length - 1) - { - rentedSpan = rentedSpan[_lastBufferEndIndex..]; - } - - return rentedSpan; + s_arrayPool.Return(current._array); } } } diff --git a/src/Components/Web/src/Forms/InputBase.cs b/src/Components/Web/src/Forms/InputBase.cs index 8761648d0bdc..b7867c9f63d0 100644 --- a/src/Components/Web/src/Forms/InputBase.cs +++ b/src/Components/Web/src/Forms/InputBase.cs @@ -14,6 +14,8 @@ namespace Microsoft.AspNetCore.Components.Forms; /// public abstract class InputBase : ComponentBase, IDisposable { + private static readonly ExpressionFormatter s_expressionFormatter = new(); + private readonly EventHandler _validationStateChangedHandler; private bool _hasInitializedParameters; private bool _parsingFailed; @@ -147,6 +149,12 @@ protected string? CurrentValueAsString } } + /// + /// Gets the formatted value of . This can be used as the value for the + /// HTML input's "name" attribute. + /// + protected string? ValueExpressionAsString { get; private set; } + /// /// Constructs an instance of . /// @@ -203,6 +211,10 @@ public override Task SetParametersAsync(ParameterView parameters) $"parameter. Normally this is provided automatically when using 'bind-Value'."); } + // TODO: We don't always want to format the value expression for the "name" attribute. + // Also, FieldIdentifier already does its own processing of the ValueExpression. + // Maybe we can deduplicate some work here. + ValueExpressionAsString = s_expressionFormatter.FormatLambda(ValueExpression); FieldIdentifier = FieldIdentifier.Create(ValueExpression); if (CascadedEditContext != null) diff --git a/src/Components/Web/src/Forms/InputCheckbox.cs b/src/Components/Web/src/Forms/InputCheckbox.cs index 806f63c29105..a0ff64d982ee 100644 --- a/src/Components/Web/src/Forms/InputCheckbox.cs +++ b/src/Components/Web/src/Forms/InputCheckbox.cs @@ -34,11 +34,12 @@ protected override void BuildRenderTree(RenderTreeBuilder builder) builder.OpenElement(0, "input"); builder.AddMultipleAttributes(1, AdditionalAttributes); builder.AddAttribute(2, "type", "checkbox"); - builder.AddAttribute(3, "class", CssClass); - builder.AddAttribute(4, "checked", BindConverter.FormatValue(CurrentValue)); - builder.AddAttribute(5, "onchange", EventCallback.Factory.CreateBinder(this, __value => CurrentValue = __value, CurrentValue)); + builder.AddAttributeIfNotNullOrEmpty(3, "name", ValueExpressionAsString); + builder.AddAttribute(4, "class", CssClass); + builder.AddAttribute(5, "checked", BindConverter.FormatValue(CurrentValue)); + builder.AddAttribute(6, "onchange", EventCallback.Factory.CreateBinder(this, __value => CurrentValue = __value, CurrentValue)); builder.SetUpdatesAttributeName("checked"); - builder.AddElementReferenceCapture(6, __inputReference => Element = __inputReference); + builder.AddElementReferenceCapture(7, __inputReference => Element = __inputReference); builder.CloseElement(); } diff --git a/src/Components/Web/src/Forms/InputDate.cs b/src/Components/Web/src/Forms/InputDate.cs index 0833c6e32094..a3cf3aaeca46 100644 --- a/src/Components/Web/src/Forms/InputDate.cs +++ b/src/Components/Web/src/Forms/InputDate.cs @@ -80,11 +80,12 @@ protected override void BuildRenderTree(RenderTreeBuilder builder) builder.OpenElement(0, "input"); builder.AddMultipleAttributes(1, AdditionalAttributes); builder.AddAttribute(2, "type", _typeAttributeValue); - builder.AddAttribute(3, "class", CssClass); - builder.AddAttribute(4, "value", CurrentValueAsString); - builder.AddAttribute(5, "onchange", EventCallback.Factory.CreateBinder(this, __value => CurrentValueAsString = __value, CurrentValueAsString)); + builder.AddAttributeIfNotNullOrEmpty(3, "name", ValueExpressionAsString); + builder.AddAttribute(4, "class", CssClass); + builder.AddAttribute(5, "value", CurrentValueAsString); + builder.AddAttribute(6, "onchange", EventCallback.Factory.CreateBinder(this, __value => CurrentValueAsString = __value, CurrentValueAsString)); builder.SetUpdatesAttributeName("value"); - builder.AddElementReferenceCapture(6, __inputReference => Element = __inputReference); + builder.AddElementReferenceCapture(7, __inputReference => Element = __inputReference); builder.CloseElement(); } diff --git a/src/Components/Web/src/Forms/InputNumber.cs b/src/Components/Web/src/Forms/InputNumber.cs index ef477af31279..0c197d353530 100644 --- a/src/Components/Web/src/Forms/InputNumber.cs +++ b/src/Components/Web/src/Forms/InputNumber.cs @@ -55,11 +55,12 @@ protected override void BuildRenderTree(RenderTreeBuilder builder) builder.AddAttribute(1, "step", _stepAttributeValue); builder.AddMultipleAttributes(2, AdditionalAttributes); builder.AddAttribute(3, "type", "number"); - builder.AddAttributeIfNotNullOrEmpty(4, "class", CssClass); - builder.AddAttribute(5, "value", CurrentValueAsString); - builder.AddAttribute(6, "onchange", EventCallback.Factory.CreateBinder(this, __value => CurrentValueAsString = __value, CurrentValueAsString)); + builder.AddAttributeIfNotNullOrEmpty(4, "name", ValueExpressionAsString); + builder.AddAttributeIfNotNullOrEmpty(5, "class", CssClass); + builder.AddAttribute(6, "value", CurrentValueAsString); + builder.AddAttribute(7, "onchange", EventCallback.Factory.CreateBinder(this, __value => CurrentValueAsString = __value, CurrentValueAsString)); builder.SetUpdatesAttributeName("value"); - builder.AddElementReferenceCapture(7, __inputReference => Element = __inputReference); + builder.AddElementReferenceCapture(8, __inputReference => Element = __inputReference); builder.CloseElement(); } diff --git a/src/Components/Web/src/Forms/InputSelect.cs b/src/Components/Web/src/Forms/InputSelect.cs index e6fe5c723db6..4cd7f7f6be7b 100644 --- a/src/Components/Web/src/Forms/InputSelect.cs +++ b/src/Components/Web/src/Forms/InputSelect.cs @@ -40,24 +40,25 @@ protected override void BuildRenderTree(RenderTreeBuilder builder) { builder.OpenElement(0, "select"); builder.AddMultipleAttributes(1, AdditionalAttributes); - builder.AddAttributeIfNotNullOrEmpty(2, "class", CssClass); - builder.AddAttribute(3, "multiple", _isMultipleSelect); + builder.AddAttributeIfNotNullOrEmpty(2, "name", ValueExpressionAsString); + builder.AddAttributeIfNotNullOrEmpty(3, "class", CssClass); + builder.AddAttribute(4, "multiple", _isMultipleSelect); if (_isMultipleSelect) { - builder.AddAttribute(4, "value", BindConverter.FormatValue(CurrentValue)?.ToString()); - builder.AddAttribute(5, "onchange", EventCallback.Factory.CreateBinder(this, SetCurrentValueAsStringArray, default)); + builder.AddAttribute(5, "value", BindConverter.FormatValue(CurrentValue)?.ToString()); + builder.AddAttribute(6, "onchange", EventCallback.Factory.CreateBinder(this, SetCurrentValueAsStringArray, default)); builder.SetUpdatesAttributeName("value"); } else { - builder.AddAttribute(6, "value", CurrentValueAsString); - builder.AddAttribute(7, "onchange", EventCallback.Factory.CreateBinder(this, __value => CurrentValueAsString = __value, default)); + builder.AddAttribute(7, "value", CurrentValueAsString); + builder.AddAttribute(8, "onchange", EventCallback.Factory.CreateBinder(this, __value => CurrentValueAsString = __value, default)); builder.SetUpdatesAttributeName("value"); } - builder.AddElementReferenceCapture(8, __selectReference => Element = __selectReference); - builder.AddContent(9, ChildContent); + builder.AddElementReferenceCapture(9, __selectReference => Element = __selectReference); + builder.AddContent(10, ChildContent); builder.CloseElement(); } diff --git a/src/Components/Web/src/Forms/InputText.cs b/src/Components/Web/src/Forms/InputText.cs index a36d402238eb..e669c8b33027 100644 --- a/src/Components/Web/src/Forms/InputText.cs +++ b/src/Components/Web/src/Forms/InputText.cs @@ -33,11 +33,12 @@ protected override void BuildRenderTree(RenderTreeBuilder builder) { builder.OpenElement(0, "input"); builder.AddMultipleAttributes(1, AdditionalAttributes); - builder.AddAttributeIfNotNullOrEmpty(2, "class", CssClass); - builder.AddAttribute(3, "value", CurrentValueAsString); - builder.AddAttribute(4, "onchange", EventCallback.Factory.CreateBinder(this, __value => CurrentValueAsString = __value, CurrentValueAsString)); + builder.AddAttributeIfNotNullOrEmpty(2, "name", ValueExpressionAsString); + builder.AddAttributeIfNotNullOrEmpty(3, "class", CssClass); + builder.AddAttribute(4, "value", CurrentValueAsString); + builder.AddAttribute(5, "onchange", EventCallback.Factory.CreateBinder(this, __value => CurrentValueAsString = __value, CurrentValueAsString)); builder.SetUpdatesAttributeName("value"); - builder.AddElementReferenceCapture(5, __inputReference => Element = __inputReference); + builder.AddElementReferenceCapture(6, __inputReference => Element = __inputReference); builder.CloseElement(); } diff --git a/src/Components/Web/src/Forms/InputTextArea.cs b/src/Components/Web/src/Forms/InputTextArea.cs index 84b9344f63f3..0596ba12abdb 100644 --- a/src/Components/Web/src/Forms/InputTextArea.cs +++ b/src/Components/Web/src/Forms/InputTextArea.cs @@ -33,11 +33,12 @@ protected override void BuildRenderTree(RenderTreeBuilder builder) { builder.OpenElement(0, "textarea"); builder.AddMultipleAttributes(1, AdditionalAttributes); - builder.AddAttributeIfNotNullOrEmpty(2, "class", CssClass); - builder.AddAttribute(3, "value", CurrentValueAsString); - builder.AddAttribute(4, "onchange", EventCallback.Factory.CreateBinder(this, __value => CurrentValueAsString = __value, CurrentValueAsString)); + builder.AddAttributeIfNotNullOrEmpty(2, "name", ValueExpressionAsString); + builder.AddAttributeIfNotNullOrEmpty(3, "class", CssClass); + builder.AddAttribute(4, "value", CurrentValueAsString); + builder.AddAttribute(5, "onchange", EventCallback.Factory.CreateBinder(this, __value => CurrentValueAsString = __value, CurrentValueAsString)); builder.SetUpdatesAttributeName("value"); - builder.AddElementReferenceCapture(5, __inputReference => Element = __inputReference); + builder.AddElementReferenceCapture(6, __inputReference => Element = __inputReference); builder.CloseElement(); } diff --git a/src/Components/Web/test/Forms/ExpressionFormatterTest.cs b/src/Components/Web/test/Forms/ExpressionFormatterTest.cs index 5de0dcc869cd..a78c3ea9f862 100644 --- a/src/Components/Web/test/Forms/ExpressionFormatterTest.cs +++ b/src/Components/Web/test/Forms/ExpressionFormatterTest.cs @@ -1,8 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Linq.Expressions; - namespace Microsoft.AspNetCore.Components.Forms; public class ExpressionFormatterTest @@ -13,10 +11,9 @@ public void Works_MemberAccessOnly() // Arrange var formatter = new ExpressionFormatter(); var person = new Person(); - LambdaExpression lambda = () => person.Parent.Name; // Act - var result = formatter.FormatLambda(lambda); + var result = formatter.FormatLambda(() => person.Parent.Name); // Assert Assert.Equal("Parent.Name", result); @@ -28,10 +25,9 @@ public void Works_MemberAccessWithConstIndex() // Arrange var formatter = new ExpressionFormatter(); var person = new Person(); - LambdaExpression lambda = () => person.Parent.Children[3].Name; // Act - var result = formatter.FormatLambda(lambda); + var result = formatter.FormatLambda(() => person.Parent.Children[3].Name); // Assert Assert.Equal("Parent.Children[3].Name", result); @@ -46,12 +42,11 @@ public void Works_MemberAccessWithConstIndex_SameLambdaMultipleTimes() var formatter = new ExpressionFormatter(); var person = new Person(); var result = new string[3]; - LambdaExpression lambda = () => person.Parent.Children[3].Name; // Act for (var i = 0; i < result.Length; i++) { - result[i] = formatter.FormatLambda(lambda); + result[i] = formatter.FormatLambda(() => person.Parent.Children[3].Name); } // Assert @@ -67,29 +62,27 @@ public void Works_MemberAccessWithVariableIndex() var formatter = new ExpressionFormatter(); var person = new Person(); var i = 42; - LambdaExpression lambda = () => person.Parent.Children[i].Name; // Act - var result = formatter.FormatLambda(lambda); + var result = formatter.FormatLambda(() => person.Parent.Children[i].Name); // Assert Assert.Equal("Parent.Children[42].Name", result); } [Fact] - public void Works_ForLoopIteraterVariableIndex_Short() + public void Works_ForLoopIteratorVariableIndex_Short() { // Arrange var formatter = new ExpressionFormatter(); var person = new Person(); var i = 0; - LambdaExpression lambda = () => person.Parent.Children[i].Name; var result = new string[3]; // Act for (; i < result.Length; i++) { - result[i] = formatter.FormatLambda(lambda); + result[i] = formatter.FormatLambda(() => person.Parent.Children[i].Name); } // Assert @@ -99,19 +92,52 @@ public void Works_ForLoopIteraterVariableIndex_Short() } [Fact] - public void Works_ForLoopIteraterVariableIndex_Long() + public void Works_ForLoopIteratorVariableIndex_MultipleClosures() + { + // Arrange + var formatter = new ExpressionFormatter(); + var person = new Person(); + + // Act + var result1 = ComputeResult(); + var result2 = ComputeResult(); + + // Assert + Assert.Equal("Parent.Children[0].Name", result1[0]); + Assert.Equal("Parent.Children[1].Name", result1[1]); + Assert.Equal("Parent.Children[2].Name", result1[2]); + + Assert.Equal("Parent.Children[0].Name", result2[0]); + Assert.Equal("Parent.Children[1].Name", result2[1]); + Assert.Equal("Parent.Children[2].Name", result2[2]); + + string[] ComputeResult() + { + var result = new string[3]; + + for (var i = 0; i < result.Length; i++) + { + // TODO: Verify that caching happens here. + result[i] = formatter.FormatLambda(() => person.Parent.Children[i].Name); + } + + return result; + } + } + + [Fact] + public void Works_ForLoopIteratorVariableIndex_Long() { // Arrange var formatter = new ExpressionFormatter(); var person = new Person(); var i = 0; - LambdaExpression lambda = () => person.Parent.Parent.Children[i].Parent.Children[i].Children[i].Name; var result = new string[3]; // Act for (; i < result.Length; i++) { - result[i] = formatter.FormatLambda(lambda); + result[i] = formatter.FormatLambda(() => person.Parent.Parent.Children[i].Parent.Children[i].Children[i].Name); } // Assert @@ -121,19 +147,18 @@ public void Works_ForLoopIteraterVariableIndex_Long() } [Fact] - public void Works_ForLoopIteraterVariableIndex_SuperLong() + public void Works_ForLoopIteratorVariableIndex_SuperLong() { // Arrange var formatter = new ExpressionFormatter(); var person = new Person(); var i = 0; - LambdaExpression lambda = () => person.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Children[i].Age; var result = new string[3]; // Act for (; i < result.Length; i++) { - result[i] = formatter.FormatLambda(lambda); + result[i] = formatter.FormatLambda(() => person.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Children[i].Age); } // Assert @@ -143,19 +168,18 @@ public void Works_ForLoopIteraterVariableIndex_SuperLong() } [Fact] - public void Works_ForLoopIteraterVariableIndex_NonArrayType() + public void Works_ForLoopIteratorVariableIndex_NonArrayType() { // Arrange var formatter = new ExpressionFormatter(); var person = new Person(); var i = 0; - LambdaExpression lambda = () => person.Parent.Nicknames[i]; var result = new string[3]; // Act for (; i < result.Length; i++) { - result[i] = formatter.FormatLambda(lambda); + result[i] = formatter.FormatLambda(() => person.Parent.Nicknames[i]); } // Assert diff --git a/src/Components/Web/test/Forms/ReverseStringBuilderTest.cs b/src/Components/Web/test/Forms/ReverseStringBuilderTest.cs index 171aacce404d..df2209305bb1 100644 --- a/src/Components/Web/test/Forms/ReverseStringBuilderTest.cs +++ b/src/Components/Web/test/Forms/ReverseStringBuilderTest.cs @@ -17,6 +17,7 @@ public void ToString_ReturnsEmptyString_WhenNoWritesOccur() // Assert Assert.Equal(string.Empty, result); + Assert.Equal(0, builder.SequenceSegmentCount); } [Fact] @@ -30,6 +31,7 @@ public void ToString_ReturnsEmptyString_WhenBufferIsEmpty() // Assert Assert.Equal(string.Empty, result); + Assert.Equal(0, builder.SequenceSegmentCount); } [Fact] @@ -48,7 +50,7 @@ public void ToString_Works_WhenOnlyUsingStackAllocatedBuffer() // Assert Assert.Equal("Hello, world!", result); - Assert.Equal(-1, builder.RentedMemoryIndex); + Assert.Equal(0, builder.SequenceSegmentCount); } [Fact] @@ -68,7 +70,7 @@ public void ToString_Works_WithNumbers() // Assert Assert.Equal("Hello, 123 worlds!", result); - Assert.Equal(-1, builder.RentedMemoryIndex); + Assert.Equal(0, builder.SequenceSegmentCount); } [Fact] @@ -87,29 +89,7 @@ public void ToString_Works_AfterExceedingStackAllocatedBuffer() // Assert Assert.Equal("Hello, world!", result); - Assert.Equal(0, builder.RentedMemoryIndex); - } - - [Fact] - public void ToString_Works_AfterExpandingIntoMultipleBuffersFromStackAllocatedBuffer() - { - // Arrange - Span initialBuffer = stackalloc char[8]; - using var builder = new ReverseStringBuilder(initialBuffer); - var padding = new string('A', ReverseStringBuilder.MinimumRentedMemorySize + 10); - var expected = padding + "Hello, world!"; - - // Act - builder.InsertFront("world!"); - builder.InsertFront(" "); - builder.InsertFront(","); - builder.InsertFront("Hello"); - builder.InsertFront(padding); - var result = builder.ToString(); - - // Assert - Assert.Equal(expected, result); - Assert.Equal(1, builder.RentedMemoryIndex); + Assert.Equal(1, builder.SequenceSegmentCount); } [Fact] @@ -117,7 +97,7 @@ public void ToString_Works_AfterExpandingIntoMultipleBuffersFromEstimatedStringS { // Arrange using var builder = new ReverseStringBuilder(8); - var padding = new string('A', ReverseStringBuilder.MinimumRentedMemorySize - 10); + var padding = new string('A', ReverseStringBuilder.MinimumRentedArraySize - 10); var expected = padding + "Hello, world!"; // Act @@ -130,36 +110,36 @@ public void ToString_Works_AfterExpandingIntoMultipleBuffersFromEstimatedStringS // Assert Assert.Equal(expected, result); - Assert.Equal(1, builder.RentedMemoryIndex); + Assert.Equal(2, builder.SequenceSegmentCount); } [Fact] public void ToString_Works_AfterUsingFallbackBuffer() { // Arrange - using var builder = new ReverseStringBuilder(ReverseStringBuilder.MinimumRentedMemorySize); - var slotCount = ReverseStringBuilder.FixedMemorySlotLength + 3; + using var builder = new ReverseStringBuilder(ReverseStringBuilder.MinimumRentedArraySize); + var segmentCount = 5; var expected = string.Empty; // Act - for (var i = 0; i < slotCount; i++) + for (var i = 0; i < segmentCount; i++) { var c = (char)(i + 65); // Update the expected string. - expected = new string(c, ReverseStringBuilder.MinimumRentedMemorySize) + expected; + expected = new string(c, ReverseStringBuilder.MinimumRentedArraySize) + expected; // Append just one character to ensure we get a buffer with the minimum possible // length. builder.InsertFront(c.ToString()); // Fill up the rest of the buffer. - var s = new string(c, ReverseStringBuilder.MinimumRentedMemorySize - 1); + var s = new string(c, ReverseStringBuilder.MinimumRentedArraySize - 1); builder.InsertFront(s); } var actual = builder.ToString(); Assert.Equal(expected, actual); - Assert.Equal(slotCount - 1, builder.RentedMemoryIndex); + Assert.Equal(segmentCount, builder.SequenceSegmentCount); } } From 159cf1eec2555b5f8465545a9eaf3f96cad5bd3a Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Mon, 17 Apr 2023 16:39:45 -0700 Subject: [PATCH 03/11] Update PublicAPI.Unshipped.txt --- src/Components/Web/src/PublicAPI.Unshipped.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Components/Web/src/PublicAPI.Unshipped.txt b/src/Components/Web/src/PublicAPI.Unshipped.txt index cb6f4221f02d..1b6647b41ac5 100644 --- a/src/Components/Web/src/PublicAPI.Unshipped.txt +++ b/src/Components/Web/src/PublicAPI.Unshipped.txt @@ -1,5 +1,6 @@ #nullable enable *REMOVED*override Microsoft.AspNetCore.Components.Forms.InputFile.OnInitialized() -> void +Microsoft.AspNetCore.Components.Forms.InputBase.ValueExpressionAsString.get -> string? Microsoft.AspNetCore.Components.HtmlRendering.Infrastructure.StaticHtmlRenderer Microsoft.AspNetCore.Components.HtmlRendering.Infrastructure.StaticHtmlRenderer.BeginRenderingComponent(System.Type! componentType, Microsoft.AspNetCore.Components.ParameterView initialParameters) -> Microsoft.AspNetCore.Components.Web.HtmlRendering.HtmlRootComponent Microsoft.AspNetCore.Components.HtmlRendering.Infrastructure.StaticHtmlRenderer.StaticHtmlRenderer(System.IServiceProvider! serviceProvider, Microsoft.Extensions.Logging.ILoggerFactory! loggerFactory) -> void From 61df0b5724713e93db6df78dd74d310b40b3a776 Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Mon, 17 Apr 2023 16:42:37 -0700 Subject: [PATCH 04/11] Update ReverseStringBuilder.cs --- .../ReverseStringBuilder.cs | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/src/Components/Web/src/Forms/ExpressionFormatting/ReverseStringBuilder.cs b/src/Components/Web/src/Forms/ExpressionFormatting/ReverseStringBuilder.cs index 92655f769e95..5b25eb4e329e 100644 --- a/src/Components/Web/src/Forms/ExpressionFormatting/ReverseStringBuilder.cs +++ b/src/Components/Web/src/Forms/ExpressionFormatting/ReverseStringBuilder.cs @@ -105,20 +105,10 @@ public void InsertFront(T value) where T : ISpanFormattable public void InsertFront(IFormattable formattable) => InsertFront(formattable.ToString(null, CultureInfo.InvariantCulture)); - public override string ToString() - { - if (_fallbackSequenceSegment is null) - { - // Only one buffer was used, so we can create the string directly. - // This will happen in the most common cases: - // 1. We didn't have an initial string length estimate, but the string was shorter than the size - // of the initial buffer (either stack-allocated or copied to the heap). Or... - // 2. We were provided a string length estimate, and the resulting string was shorter than that estimate. - return new(_currentBuffer[_nextEndIndex..]); - } - - return _fallbackSequenceSegment.ToString(_nextEndIndex); - } + public override readonly string ToString() + => _fallbackSequenceSegment is null + ? new(_currentBuffer[_nextEndIndex..]) + : _fallbackSequenceSegment.ToString(_nextEndIndex); public readonly void Dispose() { From 66071431f01f360ff957c34536e664b713283a5e Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Tue, 18 Apr 2023 16:13:46 -0700 Subject: [PATCH 05/11] Improvements --- src/Components/Forms/src/EditContext.cs | 5 + .../Forms/src/PublicAPI.Unshipped.txt | 1 + .../ExpressionFormatter.cs | 108 +++++++++++------- .../ReverseStringBuilder.cs | 4 +- src/Components/Web/src/Forms/InputBase.cs | 11 +- src/Components/Web/src/Forms/InputRadio.cs | 2 +- .../Web/src/Forms/InputRadioContext.cs | 1 + .../Web/src/Forms/InputRadioGroup.cs | 1 + .../Web/test/Forms/ExpressionFormatterTest.cs | 55 +++------ .../test/E2ETest/Tests/FormsTest.cs | 8 +- 10 files changed, 100 insertions(+), 96 deletions(-) diff --git a/src/Components/Forms/src/EditContext.cs b/src/Components/Forms/src/EditContext.cs index a8360ec06aa5..00452b75cb0b 100644 --- a/src/Components/Forms/src/EditContext.cs +++ b/src/Components/Forms/src/EditContext.cs @@ -66,6 +66,11 @@ public FieldIdentifier Field(string fieldName) /// public EditContextProperties Properties { get; } + /// + /// Gets whether field identifiers should be generated for <input> elements. + /// + public bool ShouldUseFieldIdentifiers => !OperatingSystem.IsBrowser(); + /// /// Signals that the value for the specified field has changed. /// diff --git a/src/Components/Forms/src/PublicAPI.Unshipped.txt b/src/Components/Forms/src/PublicAPI.Unshipped.txt index 7dc5c58110bf..872fb8e15380 100644 --- a/src/Components/Forms/src/PublicAPI.Unshipped.txt +++ b/src/Components/Forms/src/PublicAPI.Unshipped.txt @@ -1 +1,2 @@ #nullable enable +Microsoft.AspNetCore.Components.Forms.EditContext.ShouldUseFieldIdentifiers.get -> bool diff --git a/src/Components/Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs b/src/Components/Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs index ca66c3eb3990..3ba2f996ed6c 100644 --- a/src/Components/Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs +++ b/src/Components/Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs @@ -9,21 +9,26 @@ namespace Microsoft.AspNetCore.Components.Forms; -internal sealed class ExpressionFormatter +internal static class ExpressionFormatter { - internal const int StackallocBufferSize = 128; - internal const int EstimatedStringLengthPadding = 8; + internal const int StackAllocBufferSize = 128; private delegate void CapturedValueFormatter(object closure, ref ReverseStringBuilder builder); - private readonly ConcurrentDictionary _capturedValueFormatterCache = new(); + private static readonly ConcurrentDictionary s_capturedValueFormatterCache = new(); + private static readonly ConcurrentDictionary s_methodInfoDataCache = new(); - public string FormatLambda(LambdaExpression expression) + public static void ClearCache() { - var builder = new ReverseStringBuilder(stackalloc char[StackallocBufferSize]); + s_capturedValueFormatterCache.Clear(); + s_methodInfoDataCache.Clear(); + } + + public static string FormatLambda(LambdaExpression expression) + { + var builder = new ReverseStringBuilder(stackalloc char[StackAllocBufferSize]); var node = expression.Body; var wasLastExpressionMemberAccess = false; - var shouldNotCache = false; while (node is not null) { @@ -44,7 +49,7 @@ public string FormatLambda(LambdaExpression expression) } builder.InsertFront("]"); - shouldNotCache |= FormatIndexArgument(methodCallExpression.Arguments.Single(), ref builder); + FormatIndexArgument(methodCallExpression.Arguments.Single(), ref builder); builder.InsertFront("["); node = methodCallExpression.Object; break; @@ -59,7 +64,7 @@ public string FormatLambda(LambdaExpression expression) } builder.InsertFront("]"); - shouldNotCache |= FormatIndexArgument(binaryExpression.Right, ref builder); + FormatIndexArgument(binaryExpression.Right, ref builder); builder.InsertFront("["); node = binaryExpression.Left; break; @@ -68,9 +73,7 @@ public string FormatLambda(LambdaExpression expression) var memberExpression = (MemberExpression)node; var nextNode = memberExpression.Expression; - // FIXME: This doesn't work in all cases. Need a better - // way of detecting when we've reached the model object. - if (nextNode?.Type.Name.StartsWith('<') ?? false) + if (nextNode?.NodeType == ExpressionType.Constant) { // The next node has a compiler-generated closure type, // which means the current member access is on the captured model. @@ -101,14 +104,11 @@ public string FormatLambda(LambdaExpression expression) var result = builder.ToString(); - // TODO: Top-level caching. - builder.Dispose(); return result; } - [UnconditionalSuppressMessage("Trimming", "IL2072", Justification = "The relevant members should be preserved since they were referenced in a Linq expression")] private static bool IsSingleArgumentIndexer(Expression expression) { if (expression is not MethodCallExpression methodExpression || methodExpression.Arguments.Count != 1) @@ -116,40 +116,58 @@ private static bool IsSingleArgumentIndexer(Expression expression) return false; } - var declaringType = methodExpression.Method.DeclaringType; - if (declaringType is null) - { - return false; - } + var methodInfoData = GetOrCreateMethodInfoData(methodExpression.Method); + return methodInfoData.IsSingleArgumentIndexer; + } - // Check whether GetDefaultMembers() (if present in CoreCLR) would return a member of this type. Compiler - // names the indexer property, if any, in a generated [DefaultMember] attribute for the containing type. - var defaultMember = declaringType.GetCustomAttribute(inherit: true); - if (defaultMember is null) + private static MethodInfoData GetOrCreateMethodInfoData(MethodInfo methodInfo) + { + if (!s_methodInfoDataCache.TryGetValue(methodInfo, out var methodInfoData)) { - return false; + methodInfoData = GetMethodInfoData(methodInfo); + s_methodInfoDataCache[methodInfo] = methodInfoData; } - // Find default property (the indexer) and confirm its getter is the method in this expression. - var runtimeProperties = declaringType.GetRuntimeProperties(); - if (runtimeProperties is null) - { - return false; - } + return methodInfoData; - foreach (var property in runtimeProperties) + [UnconditionalSuppressMessage("Trimming", "IL2072", Justification = "The relevant members should be preserved since they were referenced in a LINQ expression")] + static MethodInfoData GetMethodInfoData(MethodInfo methodInfo) { - if (string.Equals(defaultMember.MemberName, property.Name, StringComparison.Ordinal) && - property.GetMethod == methodExpression.Method) + var declaringType = methodInfo.DeclaringType; + if (declaringType is null) { - return true; + return new(IsSingleArgumentIndexer: false); } - } - return false; - } + // Check whether GetDefaultMembers() (if present in CoreCLR) would return a member of this type. Compiler + // names the indexer property, if any, in a generated [DefaultMember] attribute for the containing type. + var defaultMember = declaringType.GetCustomAttribute(inherit: true); + if (defaultMember is null) + { + return new(IsSingleArgumentIndexer: false); + } + + // Find default property (the indexer) and confirm its getter is the method in this expression. + var runtimeProperties = declaringType.GetRuntimeProperties(); + if (runtimeProperties is null) + { + return new(IsSingleArgumentIndexer: false); + } + + foreach (var property in runtimeProperties) + { + if (string.Equals(defaultMember.MemberName, property.Name, StringComparison.Ordinal) && + property.GetMethod == methodInfo) + { + return new(IsSingleArgumentIndexer: true); + } + } - private bool FormatIndexArgument( + return new(IsSingleArgumentIndexer: false); + } + } + + private static void FormatIndexArgument( Expression indexExpression, ref ReverseStringBuilder builder) { @@ -157,22 +175,22 @@ private bool FormatIndexArgument( { case MemberExpression memberExpression when memberExpression.Expression is ConstantExpression constantExpression: FormatCapturedValue(memberExpression, constantExpression, ref builder); - return true; + break; case ConstantExpression constantExpression: FormatConstantValue(constantExpression, ref builder); - return false; + break; default: throw new InvalidOperationException($"Unable to evaluate index expressions of type '{indexExpression.GetType().Name}'."); } } - private void FormatCapturedValue(MemberExpression memberExpression, ConstantExpression constantExpression, ref ReverseStringBuilder builder) + private static void FormatCapturedValue(MemberExpression memberExpression, ConstantExpression constantExpression, ref ReverseStringBuilder builder) { var member = memberExpression.Member; - if (!_capturedValueFormatterCache.TryGetValue(member, out var format)) + if (!s_capturedValueFormatterCache.TryGetValue(member, out var format)) { format = CreateCapturedValueFormatter(memberExpression); - _capturedValueFormatterCache[member] = format; + s_capturedValueFormatterCache[member] = format; } format(constantExpression.Value!, ref builder); @@ -238,4 +256,6 @@ private static void FormatConstantValue(ConstantExpression constantExpression, r throw new InvalidOperationException($"Unable to format constant values of type '{x.GetType()}'."); } } + + private record struct MethodInfoData(bool IsSingleArgumentIndexer); } diff --git a/src/Components/Web/src/Forms/ExpressionFormatting/ReverseStringBuilder.cs b/src/Components/Web/src/Forms/ExpressionFormatting/ReverseStringBuilder.cs index 5b25eb4e329e..6c296e842c71 100644 --- a/src/Components/Web/src/Forms/ExpressionFormatting/ReverseStringBuilder.cs +++ b/src/Components/Web/src/Forms/ExpressionFormatting/ReverseStringBuilder.cs @@ -6,7 +6,7 @@ namespace Microsoft.AspNetCore.Components.Forms; -internal unsafe ref struct ReverseStringBuilder +internal ref struct ReverseStringBuilder { public const int MinimumRentedArraySize = 1024; @@ -92,7 +92,7 @@ public void InsertFront(T value) where T : ISpanFormattable // We won't try to optimize for anything larger. Span result = stackalloc char[11]; - if (value.TryFormat(result, out var charsWritten, default, default)) + if (value.TryFormat(result, out var charsWritten, format: default, CultureInfo.InvariantCulture)) { InsertFront(result[..charsWritten]); } diff --git a/src/Components/Web/src/Forms/InputBase.cs b/src/Components/Web/src/Forms/InputBase.cs index b7867c9f63d0..c8afa322d682 100644 --- a/src/Components/Web/src/Forms/InputBase.cs +++ b/src/Components/Web/src/Forms/InputBase.cs @@ -14,8 +14,6 @@ namespace Microsoft.AspNetCore.Components.Forms; /// public abstract class InputBase : ComponentBase, IDisposable { - private static readonly ExpressionFormatter s_expressionFormatter = new(); - private readonly EventHandler _validationStateChangedHandler; private bool _hasInitializedParameters; private bool _parsingFailed; @@ -211,16 +209,17 @@ public override Task SetParametersAsync(ParameterView parameters) $"parameter. Normally this is provided automatically when using 'bind-Value'."); } - // TODO: We don't always want to format the value expression for the "name" attribute. - // Also, FieldIdentifier already does its own processing of the ValueExpression. - // Maybe we can deduplicate some work here. - ValueExpressionAsString = s_expressionFormatter.FormatLambda(ValueExpression); FieldIdentifier = FieldIdentifier.Create(ValueExpression); if (CascadedEditContext != null) { EditContext = CascadedEditContext; EditContext.OnValidationStateChanged += _validationStateChangedHandler; + + if (EditContext.ShouldUseFieldIdentifiers) + { + ValueExpressionAsString = ExpressionFormatter.FormatLambda(ValueExpression); + } } _nullableUnderlyingType = Nullable.GetUnderlyingType(typeof(TValue)); diff --git a/src/Components/Web/src/Forms/InputRadio.cs b/src/Components/Web/src/Forms/InputRadio.cs index 6b7088d37a31..5847cb2255ac 100644 --- a/src/Components/Web/src/Forms/InputRadio.cs +++ b/src/Components/Web/src/Forms/InputRadio.cs @@ -66,7 +66,7 @@ protected override void BuildRenderTree(RenderTreeBuilder builder) builder.AddMultipleAttributes(1, AdditionalAttributes); builder.AddAttributeIfNotNullOrEmpty(2, "class", AttributeUtilities.CombineClassNames(AdditionalAttributes, Context.FieldClass)); builder.AddAttribute(3, "type", "radio"); - builder.AddAttribute(4, "name", Context.GroupName); + builder.AddAttribute(4, "name", Context.NameAttributeValue); builder.AddAttribute(5, "value", BindConverter.FormatValue(Value?.ToString())); builder.AddAttribute(6, "checked", Context.CurrentValue?.Equals(Value) == true ? GetToggledTrueValue() : null); builder.AddAttribute(7, "onchange", Context.ChangeEventCallback); diff --git a/src/Components/Web/src/Forms/InputRadioContext.cs b/src/Components/Web/src/Forms/InputRadioContext.cs index aee261960e54..f373b1750cad 100644 --- a/src/Components/Web/src/Forms/InputRadioContext.cs +++ b/src/Components/Web/src/Forms/InputRadioContext.cs @@ -13,6 +13,7 @@ internal sealed class InputRadioContext // Mutable properties that may change any time an InputRadioGroup is rendered public string? GroupName { get; set; } + public string? NameAttributeValue { get; set; } public object? CurrentValue { get; set; } public string? FieldClass { get; set; } diff --git a/src/Components/Web/src/Forms/InputRadioGroup.cs b/src/Components/Web/src/Forms/InputRadioGroup.cs index f4ca4b94c733..61132d4df27b 100644 --- a/src/Components/Web/src/Forms/InputRadioGroup.cs +++ b/src/Components/Web/src/Forms/InputRadioGroup.cs @@ -45,6 +45,7 @@ protected override void OnParametersSet() // Mutate the InputRadioContext instance in place. Since this is a non-fixed cascading parameter, the descendant // InputRadio/InputRadioGroup components will get notified to re-render and will see the new values. _context.GroupName = !string.IsNullOrEmpty(Name) ? Name : _defaultGroupName; + _context.NameAttributeValue = ValueExpressionAsString ?? _context.GroupName; _context.CurrentValue = CurrentValue; _context.FieldClass = EditContext?.FieldCssClass(FieldIdentifier); } diff --git a/src/Components/Web/test/Forms/ExpressionFormatterTest.cs b/src/Components/Web/test/Forms/ExpressionFormatterTest.cs index a78c3ea9f862..d3188d0553d4 100644 --- a/src/Components/Web/test/Forms/ExpressionFormatterTest.cs +++ b/src/Components/Web/test/Forms/ExpressionFormatterTest.cs @@ -3,17 +3,16 @@ namespace Microsoft.AspNetCore.Components.Forms; -public class ExpressionFormatterTest +public sealed class ExpressionFormatterTest : IDisposable { [Fact] public void Works_MemberAccessOnly() { // Arrange - var formatter = new ExpressionFormatter(); var person = new Person(); // Act - var result = formatter.FormatLambda(() => person.Parent.Name); + var result = ExpressionFormatter.FormatLambda(() => person.Parent.Name); // Assert Assert.Equal("Parent.Name", result); @@ -23,11 +22,10 @@ public void Works_MemberAccessOnly() public void Works_MemberAccessWithConstIndex() { // Arrange - var formatter = new ExpressionFormatter(); var person = new Person(); // Act - var result = formatter.FormatLambda(() => person.Parent.Children[3].Name); + var result = ExpressionFormatter.FormatLambda(() => person.Parent.Children[3].Name); // Assert Assert.Equal("Parent.Children[3].Name", result); @@ -36,17 +34,14 @@ public void Works_MemberAccessWithConstIndex() [Fact] public void Works_MemberAccessWithConstIndex_SameLambdaMultipleTimes() { - // TODO: Somehow validate the caching mechanism. - // Arrange - var formatter = new ExpressionFormatter(); var person = new Person(); var result = new string[3]; // Act for (var i = 0; i < result.Length; i++) { - result[i] = formatter.FormatLambda(() => person.Parent.Children[3].Name); + result[i] = ExpressionFormatter.FormatLambda(() => person.Parent.Children[3].Name); } // Assert @@ -59,12 +54,11 @@ public void Works_MemberAccessWithConstIndex_SameLambdaMultipleTimes() public void Works_MemberAccessWithVariableIndex() { // Arrange - var formatter = new ExpressionFormatter(); var person = new Person(); var i = 42; // Act - var result = formatter.FormatLambda(() => person.Parent.Children[i].Name); + var result = ExpressionFormatter.FormatLambda(() => person.Parent.Children[i].Name); // Assert Assert.Equal("Parent.Children[42].Name", result); @@ -74,7 +68,6 @@ public void Works_MemberAccessWithVariableIndex() public void Works_ForLoopIteratorVariableIndex_Short() { // Arrange - var formatter = new ExpressionFormatter(); var person = new Person(); var i = 0; var result = new string[3]; @@ -82,7 +75,7 @@ public void Works_ForLoopIteratorVariableIndex_Short() // Act for (; i < result.Length; i++) { - result[i] = formatter.FormatLambda(() => person.Parent.Children[i].Name); + result[i] = ExpressionFormatter.FormatLambda(() => person.Parent.Children[i].Name); } // Assert @@ -95,7 +88,6 @@ public void Works_ForLoopIteratorVariableIndex_Short() public void Works_ForLoopIteratorVariableIndex_MultipleClosures() { // Arrange - var formatter = new ExpressionFormatter(); var person = new Person(); // Act @@ -117,8 +109,7 @@ string[] ComputeResult() for (var i = 0; i < result.Length; i++) { - // TODO: Verify that caching happens here. - result[i] = formatter.FormatLambda(() => person.Parent.Children[i].Name); + result[i] = ExpressionFormatter.FormatLambda(() => person.Parent.Children[i].Name); } return result; @@ -129,7 +120,6 @@ string[] ComputeResult() public void Works_ForLoopIteratorVariableIndex_Long() { // Arrange - var formatter = new ExpressionFormatter(); var person = new Person(); var i = 0; var result = new string[3]; @@ -137,7 +127,7 @@ public void Works_ForLoopIteratorVariableIndex_Long() // Act for (; i < result.Length; i++) { - result[i] = formatter.FormatLambda(() => person.Parent.Parent.Children[i].Parent.Children[i].Children[i].Name); + result[i] = ExpressionFormatter.FormatLambda(() => person.Parent.Parent.Children[i].Parent.Children[i].Children[i].Name); } // Assert @@ -146,32 +136,10 @@ public void Works_ForLoopIteratorVariableIndex_Long() Assert.Equal("Parent.Parent.Children[2].Parent.Children[2].Children[2].Name", result[2]); } - [Fact] - public void Works_ForLoopIteratorVariableIndex_SuperLong() - { - // Arrange - var formatter = new ExpressionFormatter(); - var person = new Person(); - var i = 0; - var result = new string[3]; - - // Act - for (; i < result.Length; i++) - { - result[i] = formatter.FormatLambda(() => person.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Children[i].Age); - } - - // Assert - Assert.Equal("Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Children[0].Age", result[0]); - Assert.Equal("Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Children[1].Age", result[1]); - Assert.Equal("Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Parent.Children[2].Age", result[2]); - } - [Fact] public void Works_ForLoopIteratorVariableIndex_NonArrayType() { // Arrange - var formatter = new ExpressionFormatter(); var person = new Person(); var i = 0; var result = new string[3]; @@ -179,7 +147,7 @@ public void Works_ForLoopIteratorVariableIndex_NonArrayType() // Act for (; i < result.Length; i++) { - result[i] = formatter.FormatLambda(() => person.Parent.Nicknames[i]); + result[i] = ExpressionFormatter.FormatLambda(() => person.Parent.Nicknames[i]); } // Assert @@ -188,6 +156,11 @@ public void Works_ForLoopIteratorVariableIndex_NonArrayType() Assert.Equal("Parent.Nicknames[2]", result[2]); } + public void Dispose() + { + ExpressionFormatter.ClearCache(); + } + private class Person { public string Name { get; init; } diff --git a/src/Components/test/E2ETest/Tests/FormsTest.cs b/src/Components/test/E2ETest/Tests/FormsTest.cs index f150e6bfe573..e253945a40aa 100644 --- a/src/Components/test/E2ETest/Tests/FormsTest.cs +++ b/src/Components/test/E2ETest/Tests/FormsTest.cs @@ -423,9 +423,13 @@ public void InputRadioGroupsWithNamesNestedInteractWithEditContext() Browser.True(() => FindColorInputs().All(i => string.Equals("modified valid", i.GetAttribute("class")))); - IReadOnlyCollection FindCountryInputs() => group.FindElements(By.Name("country")); + IReadOnlyCollection FindCountryInputs() => group.FindElements(By.TagName("input")) + .Where((_, i) => i % 2 == 0) + .ToArray(); - IReadOnlyCollection FindColorInputs() => group.FindElements(By.Name("color")); + IReadOnlyCollection FindColorInputs() => group.FindElements(By.TagName("input")) + .Where((_, i) => i % 2 == 1) + .ToArray(); } [Fact] From f0917dca3d68790b7d24ebf1d052d315973b9914 Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Tue, 18 Apr 2023 16:15:49 -0700 Subject: [PATCH 06/11] Update Microsoft.AspNetCore.Components.Web.csproj --- .../Web/src/Microsoft.AspNetCore.Components.Web.csproj | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Components/Web/src/Microsoft.AspNetCore.Components.Web.csproj b/src/Components/Web/src/Microsoft.AspNetCore.Components.Web.csproj index 37e7786f8349..ed97ef06d0c2 100644 --- a/src/Components/Web/src/Microsoft.AspNetCore.Components.Web.csproj +++ b/src/Components/Web/src/Microsoft.AspNetCore.Components.Web.csproj @@ -10,7 +10,6 @@ true false - true From fd4e1c4acc6d6c100ea292da16db44a3899af74b Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Wed, 19 Apr 2023 10:40:48 -0700 Subject: [PATCH 07/11] PR feedback. --- src/Components/Forms/src/EditContext.cs | 2 +- src/Components/Forms/src/PublicAPI.Unshipped.txt | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Components/Forms/src/EditContext.cs b/src/Components/Forms/src/EditContext.cs index 00452b75cb0b..940781d1cadd 100644 --- a/src/Components/Forms/src/EditContext.cs +++ b/src/Components/Forms/src/EditContext.cs @@ -69,7 +69,7 @@ public FieldIdentifier Field(string fieldName) /// /// Gets whether field identifiers should be generated for <input> elements. /// - public bool ShouldUseFieldIdentifiers => !OperatingSystem.IsBrowser(); + public bool ShouldUseFieldIdentifiers { get; set; } = !OperatingSystem.IsBrowser(); /// /// Signals that the value for the specified field has changed. diff --git a/src/Components/Forms/src/PublicAPI.Unshipped.txt b/src/Components/Forms/src/PublicAPI.Unshipped.txt index 872fb8e15380..20d22e152809 100644 --- a/src/Components/Forms/src/PublicAPI.Unshipped.txt +++ b/src/Components/Forms/src/PublicAPI.Unshipped.txt @@ -1,2 +1,3 @@ #nullable enable Microsoft.AspNetCore.Components.Forms.EditContext.ShouldUseFieldIdentifiers.get -> bool +Microsoft.AspNetCore.Components.Forms.EditContext.ShouldUseFieldIdentifiers.set -> void From 5482a168c79b23e0b0bc169e5a5b65eda8c19aff Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Wed, 19 Apr 2023 16:03:30 -0700 Subject: [PATCH 08/11] Prefer explicit 'name' attribute --- src/Components/Web/src/Forms/InputBase.cs | 41 +++++++++++++------ src/Components/Web/src/Forms/InputCheckbox.cs | 2 +- src/Components/Web/src/Forms/InputDate.cs | 2 +- src/Components/Web/src/Forms/InputNumber.cs | 2 +- src/Components/Web/src/Forms/InputRadio.cs | 2 +- .../Web/src/Forms/InputRadioContext.cs | 1 - .../Web/src/Forms/InputRadioGroup.cs | 17 +++++++- src/Components/Web/src/Forms/InputSelect.cs | 2 +- src/Components/Web/src/Forms/InputText.cs | 2 +- src/Components/Web/src/Forms/InputTextArea.cs | 2 +- .../Web/src/PublicAPI.Unshipped.txt | 2 +- .../test/E2ETest/Tests/FormsTest.cs | 8 +--- 12 files changed, 54 insertions(+), 29 deletions(-) diff --git a/src/Components/Web/src/Forms/InputBase.cs b/src/Components/Web/src/Forms/InputBase.cs index c8afa322d682..6041ddee9a4c 100644 --- a/src/Components/Web/src/Forms/InputBase.cs +++ b/src/Components/Web/src/Forms/InputBase.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Diagnostics.CodeAnalysis; +using System.Globalization; using System.Linq; using System.Linq.Expressions; @@ -18,6 +19,7 @@ public abstract class InputBase : ComponentBase, IDisposable private bool _hasInitializedParameters; private bool _parsingFailed; private string? _incomingValueBeforeParsing; + private string? _formattedValueExpression; private bool _previousParsingAttemptFailed; private ValidationMessageStore? _parsingValidationMessages; private Type? _nullableUnderlyingType; @@ -147,12 +149,6 @@ protected string? CurrentValueAsString } } - /// - /// Gets the formatted value of . This can be used as the value for the - /// HTML input's "name" attribute. - /// - protected string? ValueExpressionAsString { get; private set; } - /// /// Constructs an instance of . /// @@ -162,7 +158,7 @@ protected InputBase() } /// - /// Formats the value as a string. Derived classes can override this to determine the formating used for . + /// Formats the value as a string. Derived classes can override this to determine the formatting used for . /// /// The value to format. /// A string representation of the value. @@ -193,6 +189,32 @@ protected string CssClass } } + /// + /// Gets the value to be used for the input's "name" attribute. + /// + protected string NameAttributeValue + { + get + { + if (AdditionalAttributes?.TryGetValue("name", out var nameAttributeValue) ?? false) + { + return Convert.ToString(nameAttributeValue, CultureInfo.InvariantCulture) ?? string.Empty; + } + + if (EditContext?.ShouldUseFieldIdentifiers ?? false) + { + if (_formattedValueExpression is null && ValueExpression is not null) + { + _formattedValueExpression = ExpressionFormatter.FormatLambda(ValueExpression); + } + + return _formattedValueExpression ?? string.Empty; + } + + return string.Empty; + } + } + /// public override Task SetParametersAsync(ParameterView parameters) { @@ -215,11 +237,6 @@ public override Task SetParametersAsync(ParameterView parameters) { EditContext = CascadedEditContext; EditContext.OnValidationStateChanged += _validationStateChangedHandler; - - if (EditContext.ShouldUseFieldIdentifiers) - { - ValueExpressionAsString = ExpressionFormatter.FormatLambda(ValueExpression); - } } _nullableUnderlyingType = Nullable.GetUnderlyingType(typeof(TValue)); diff --git a/src/Components/Web/src/Forms/InputCheckbox.cs b/src/Components/Web/src/Forms/InputCheckbox.cs index a0ff64d982ee..ad78a4ff8074 100644 --- a/src/Components/Web/src/Forms/InputCheckbox.cs +++ b/src/Components/Web/src/Forms/InputCheckbox.cs @@ -34,7 +34,7 @@ protected override void BuildRenderTree(RenderTreeBuilder builder) builder.OpenElement(0, "input"); builder.AddMultipleAttributes(1, AdditionalAttributes); builder.AddAttribute(2, "type", "checkbox"); - builder.AddAttributeIfNotNullOrEmpty(3, "name", ValueExpressionAsString); + builder.AddAttributeIfNotNullOrEmpty(3, "name", NameAttributeValue); builder.AddAttribute(4, "class", CssClass); builder.AddAttribute(5, "checked", BindConverter.FormatValue(CurrentValue)); builder.AddAttribute(6, "onchange", EventCallback.Factory.CreateBinder(this, __value => CurrentValue = __value, CurrentValue)); diff --git a/src/Components/Web/src/Forms/InputDate.cs b/src/Components/Web/src/Forms/InputDate.cs index a3cf3aaeca46..0c1865fd8877 100644 --- a/src/Components/Web/src/Forms/InputDate.cs +++ b/src/Components/Web/src/Forms/InputDate.cs @@ -80,7 +80,7 @@ protected override void BuildRenderTree(RenderTreeBuilder builder) builder.OpenElement(0, "input"); builder.AddMultipleAttributes(1, AdditionalAttributes); builder.AddAttribute(2, "type", _typeAttributeValue); - builder.AddAttributeIfNotNullOrEmpty(3, "name", ValueExpressionAsString); + builder.AddAttributeIfNotNullOrEmpty(3, "name", NameAttributeValue); builder.AddAttribute(4, "class", CssClass); builder.AddAttribute(5, "value", CurrentValueAsString); builder.AddAttribute(6, "onchange", EventCallback.Factory.CreateBinder(this, __value => CurrentValueAsString = __value, CurrentValueAsString)); diff --git a/src/Components/Web/src/Forms/InputNumber.cs b/src/Components/Web/src/Forms/InputNumber.cs index 0c197d353530..fa4d31c0708e 100644 --- a/src/Components/Web/src/Forms/InputNumber.cs +++ b/src/Components/Web/src/Forms/InputNumber.cs @@ -55,7 +55,7 @@ protected override void BuildRenderTree(RenderTreeBuilder builder) builder.AddAttribute(1, "step", _stepAttributeValue); builder.AddMultipleAttributes(2, AdditionalAttributes); builder.AddAttribute(3, "type", "number"); - builder.AddAttributeIfNotNullOrEmpty(4, "name", ValueExpressionAsString); + builder.AddAttributeIfNotNullOrEmpty(4, "name", NameAttributeValue); builder.AddAttributeIfNotNullOrEmpty(5, "class", CssClass); builder.AddAttribute(6, "value", CurrentValueAsString); builder.AddAttribute(7, "onchange", EventCallback.Factory.CreateBinder(this, __value => CurrentValueAsString = __value, CurrentValueAsString)); diff --git a/src/Components/Web/src/Forms/InputRadio.cs b/src/Components/Web/src/Forms/InputRadio.cs index 5847cb2255ac..6b7088d37a31 100644 --- a/src/Components/Web/src/Forms/InputRadio.cs +++ b/src/Components/Web/src/Forms/InputRadio.cs @@ -66,7 +66,7 @@ protected override void BuildRenderTree(RenderTreeBuilder builder) builder.AddMultipleAttributes(1, AdditionalAttributes); builder.AddAttributeIfNotNullOrEmpty(2, "class", AttributeUtilities.CombineClassNames(AdditionalAttributes, Context.FieldClass)); builder.AddAttribute(3, "type", "radio"); - builder.AddAttribute(4, "name", Context.NameAttributeValue); + builder.AddAttribute(4, "name", Context.GroupName); builder.AddAttribute(5, "value", BindConverter.FormatValue(Value?.ToString())); builder.AddAttribute(6, "checked", Context.CurrentValue?.Equals(Value) == true ? GetToggledTrueValue() : null); builder.AddAttribute(7, "onchange", Context.ChangeEventCallback); diff --git a/src/Components/Web/src/Forms/InputRadioContext.cs b/src/Components/Web/src/Forms/InputRadioContext.cs index f373b1750cad..aee261960e54 100644 --- a/src/Components/Web/src/Forms/InputRadioContext.cs +++ b/src/Components/Web/src/Forms/InputRadioContext.cs @@ -13,7 +13,6 @@ internal sealed class InputRadioContext // Mutable properties that may change any time an InputRadioGroup is rendered public string? GroupName { get; set; } - public string? NameAttributeValue { get; set; } public object? CurrentValue { get; set; } public string? FieldClass { get; set; } diff --git a/src/Components/Web/src/Forms/InputRadioGroup.cs b/src/Components/Web/src/Forms/InputRadioGroup.cs index 61132d4df27b..d3a4d02dad34 100644 --- a/src/Components/Web/src/Forms/InputRadioGroup.cs +++ b/src/Components/Web/src/Forms/InputRadioGroup.cs @@ -44,8 +44,21 @@ protected override void OnParametersSet() // Mutate the InputRadioContext instance in place. Since this is a non-fixed cascading parameter, the descendant // InputRadio/InputRadioGroup components will get notified to re-render and will see the new values. - _context.GroupName = !string.IsNullOrEmpty(Name) ? Name : _defaultGroupName; - _context.NameAttributeValue = ValueExpressionAsString ?? _context.GroupName; + if (!string.IsNullOrEmpty(Name)) + { + // Prefer the explicitly-specified group name over anything else. + _context.GroupName = Name; + } + else if (!string.IsNullOrEmpty(NameAttributeValue)) + { + // If the user specifies a "name" attribute, or we're using "name" as a form field identifier, use that. + _context.GroupName = NameAttributeValue; + } + else + { + // Otherwise, just use a GUID to disambiguate this group's radio inputs from any others on the page. + _context.GroupName = _defaultGroupName; + } _context.CurrentValue = CurrentValue; _context.FieldClass = EditContext?.FieldCssClass(FieldIdentifier); } diff --git a/src/Components/Web/src/Forms/InputSelect.cs b/src/Components/Web/src/Forms/InputSelect.cs index 4cd7f7f6be7b..1fa8ecb1df72 100644 --- a/src/Components/Web/src/Forms/InputSelect.cs +++ b/src/Components/Web/src/Forms/InputSelect.cs @@ -40,7 +40,7 @@ protected override void BuildRenderTree(RenderTreeBuilder builder) { builder.OpenElement(0, "select"); builder.AddMultipleAttributes(1, AdditionalAttributes); - builder.AddAttributeIfNotNullOrEmpty(2, "name", ValueExpressionAsString); + builder.AddAttributeIfNotNullOrEmpty(2, "name", NameAttributeValue); builder.AddAttributeIfNotNullOrEmpty(3, "class", CssClass); builder.AddAttribute(4, "multiple", _isMultipleSelect); diff --git a/src/Components/Web/src/Forms/InputText.cs b/src/Components/Web/src/Forms/InputText.cs index e669c8b33027..c3eb9b5f1d89 100644 --- a/src/Components/Web/src/Forms/InputText.cs +++ b/src/Components/Web/src/Forms/InputText.cs @@ -33,7 +33,7 @@ protected override void BuildRenderTree(RenderTreeBuilder builder) { builder.OpenElement(0, "input"); builder.AddMultipleAttributes(1, AdditionalAttributes); - builder.AddAttributeIfNotNullOrEmpty(2, "name", ValueExpressionAsString); + builder.AddAttributeIfNotNullOrEmpty(2, "name", NameAttributeValue); builder.AddAttributeIfNotNullOrEmpty(3, "class", CssClass); builder.AddAttribute(4, "value", CurrentValueAsString); builder.AddAttribute(5, "onchange", EventCallback.Factory.CreateBinder(this, __value => CurrentValueAsString = __value, CurrentValueAsString)); diff --git a/src/Components/Web/src/Forms/InputTextArea.cs b/src/Components/Web/src/Forms/InputTextArea.cs index 0596ba12abdb..2495ce3d07f7 100644 --- a/src/Components/Web/src/Forms/InputTextArea.cs +++ b/src/Components/Web/src/Forms/InputTextArea.cs @@ -33,7 +33,7 @@ protected override void BuildRenderTree(RenderTreeBuilder builder) { builder.OpenElement(0, "textarea"); builder.AddMultipleAttributes(1, AdditionalAttributes); - builder.AddAttributeIfNotNullOrEmpty(2, "name", ValueExpressionAsString); + builder.AddAttributeIfNotNullOrEmpty(2, "name", NameAttributeValue); builder.AddAttributeIfNotNullOrEmpty(3, "class", CssClass); builder.AddAttribute(4, "value", CurrentValueAsString); builder.AddAttribute(5, "onchange", EventCallback.Factory.CreateBinder(this, __value => CurrentValueAsString = __value, CurrentValueAsString)); diff --git a/src/Components/Web/src/PublicAPI.Unshipped.txt b/src/Components/Web/src/PublicAPI.Unshipped.txt index 1b6647b41ac5..f490997416ed 100644 --- a/src/Components/Web/src/PublicAPI.Unshipped.txt +++ b/src/Components/Web/src/PublicAPI.Unshipped.txt @@ -1,6 +1,6 @@ #nullable enable *REMOVED*override Microsoft.AspNetCore.Components.Forms.InputFile.OnInitialized() -> void -Microsoft.AspNetCore.Components.Forms.InputBase.ValueExpressionAsString.get -> string? +Microsoft.AspNetCore.Components.Forms.InputBase.NameAttributeValue.get -> string! Microsoft.AspNetCore.Components.HtmlRendering.Infrastructure.StaticHtmlRenderer Microsoft.AspNetCore.Components.HtmlRendering.Infrastructure.StaticHtmlRenderer.BeginRenderingComponent(System.Type! componentType, Microsoft.AspNetCore.Components.ParameterView initialParameters) -> Microsoft.AspNetCore.Components.Web.HtmlRendering.HtmlRootComponent Microsoft.AspNetCore.Components.HtmlRendering.Infrastructure.StaticHtmlRenderer.StaticHtmlRenderer(System.IServiceProvider! serviceProvider, Microsoft.Extensions.Logging.ILoggerFactory! loggerFactory) -> void diff --git a/src/Components/test/E2ETest/Tests/FormsTest.cs b/src/Components/test/E2ETest/Tests/FormsTest.cs index e253945a40aa..f150e6bfe573 100644 --- a/src/Components/test/E2ETest/Tests/FormsTest.cs +++ b/src/Components/test/E2ETest/Tests/FormsTest.cs @@ -423,13 +423,9 @@ public void InputRadioGroupsWithNamesNestedInteractWithEditContext() Browser.True(() => FindColorInputs().All(i => string.Equals("modified valid", i.GetAttribute("class")))); - IReadOnlyCollection FindCountryInputs() => group.FindElements(By.TagName("input")) - .Where((_, i) => i % 2 == 0) - .ToArray(); + IReadOnlyCollection FindCountryInputs() => group.FindElements(By.Name("country")); - IReadOnlyCollection FindColorInputs() => group.FindElements(By.TagName("input")) - .Where((_, i) => i % 2 == 1) - .ToArray(); + IReadOnlyCollection FindColorInputs() => group.FindElements(By.Name("color")); } [Fact] From 975376786b233ed60434c55864f9405de605e0d6 Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Thu, 20 Apr 2023 10:54:45 -0700 Subject: [PATCH 09/11] PR feedback --- .../Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Components/Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs b/src/Components/Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs index 3ba2f996ed6c..448ec191ea65 100644 --- a/src/Components/Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs +++ b/src/Components/Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs @@ -49,7 +49,7 @@ public static string FormatLambda(LambdaExpression expression) } builder.InsertFront("]"); - FormatIndexArgument(methodCallExpression.Arguments.Single(), ref builder); + FormatIndexArgument(methodCallExpression.Arguments[0], ref builder); builder.InsertFront("["); node = methodCallExpression.Object; break; From 47f0d0f869a203dc9cb0e1c5ae79026d743007d9 Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Thu, 20 Apr 2023 11:27:43 -0700 Subject: [PATCH 10/11] Update ExpressionFormatter.cs --- .../Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Components/Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs b/src/Components/Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs index 448ec191ea65..e3fc1b199616 100644 --- a/src/Components/Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs +++ b/src/Components/Web/src/Forms/ExpressionFormatting/ExpressionFormatter.cs @@ -3,7 +3,6 @@ using System.Collections.Concurrent; using System.Diagnostics.CodeAnalysis; -using System.Linq; using System.Linq.Expressions; using System.Reflection; From 0b017a9dca5fb90287ea225d77ae7b6c53044f6c Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Thu, 20 Apr 2023 13:03:51 -0700 Subject: [PATCH 11/11] Update InputRadioTest.cs --- src/Components/Web/test/Forms/InputRadioTest.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Components/Web/test/Forms/InputRadioTest.cs b/src/Components/Web/test/Forms/InputRadioTest.cs index 53438ceba14e..483b80697986 100644 --- a/src/Components/Web/test/Forms/InputRadioTest.cs +++ b/src/Components/Web/test/Forms/InputRadioTest.cs @@ -30,7 +30,10 @@ public async Task GroupGeneratesNameGuidWhenInvalidNameSupplied() var model = new TestModel(); var rootComponent = new TestInputRadioHostComponent { - EditContext = new EditContext(model), + EditContext = new EditContext(model) + { + ShouldUseFieldIdentifiers = false, + }, InnerContent = RadioButtonsWithGroup(null, () => model.TestEnum) };