-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Replace AssignToProperties with SetParameterProperties, which also clears unspecified parameter properties (imported from Blazor PR 1108) #4797
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
4aa2491
Change AssignToProperties to clear all unset bindables
RemiBou f69c79e
Avoid unnecessary boxing of default values
SteveSandersonMS b35a7e4
Make SetParameterProperties free of allocations
SteveSandersonMS 4175517
E2E test showing the new parameter removal capability works
SteveSandersonMS aa842c0
Fix FetchData samples
SteveSandersonMS File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 changes: 2 additions & 1 deletion
3
src/Components/src/Microsoft.AspNetCore.Components/Microsoft.AspNetCore.Components.csproj
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,18 +16,16 @@ public static class ParameterCollectionExtensions | |
{ | ||
private const BindingFlags _bindablePropertyFlags = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.IgnoreCase; | ||
|
||
private delegate void WriteParameterAction(object target, object parameterValue); | ||
|
||
private readonly static IDictionary<Type, IDictionary<string, WriteParameterAction>> _cachedParameterWriters | ||
= new ConcurrentDictionary<Type, IDictionary<string, WriteParameterAction>>(); | ||
private readonly static ConcurrentDictionary<Type, WritersForType> _cachedWritersByType | ||
= new ConcurrentDictionary<Type, WritersForType>(); | ||
|
||
/// <summary> | ||
/// Iterates through the <see cref="ParameterCollection"/>, assigning each parameter | ||
/// to a property of the same name on <paramref name="target"/>. | ||
/// For each parameter property on <paramref name="target"/>, updates its value to | ||
/// match the corresponding entry in the <see cref="ParameterCollection"/>. | ||
/// </summary> | ||
/// <param name="parameterCollection">The <see cref="ParameterCollection"/>.</param> | ||
/// <param name="target">An object that has a public writable property matching each parameter's name and type.</param> | ||
public static void AssignToProperties( | ||
public unsafe static void SetParameterProperties( | ||
in this ParameterCollection parameterCollection, | ||
object target) | ||
{ | ||
|
@@ -37,23 +35,36 @@ public static void AssignToProperties( | |
} | ||
|
||
var targetType = target.GetType(); | ||
if (!_cachedParameterWriters.TryGetValue(targetType, out var parameterWriters)) | ||
if (!_cachedWritersByType.TryGetValue(targetType, out var writers)) | ||
{ | ||
parameterWriters = CreateParameterWriters(targetType); | ||
_cachedParameterWriters[targetType] = parameterWriters; | ||
writers = new WritersForType(targetType); | ||
_cachedWritersByType[targetType] = writers; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: changing this to |
||
} | ||
|
||
// We only want to iterate through the parameterCollection once, and by the end of it, | ||
// need to have tracked which of the parameter properties haven't yet been written. | ||
// To avoid allocating any list/dictionary to track that, here we stackalloc an array | ||
// of flags and set them based on the indices of the writers we use. | ||
var numWriters = writers.WritersByIndex.Count; | ||
var numUsedWriters = 0; | ||
|
||
// TODO: Once we're able to move to netstandard2.1, this can be changed to be | ||
// a Span<bool> and then the enclosing method no longer needs to be 'unsafe' | ||
bool* usageFlags = stackalloc bool[numWriters]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see now. |
||
|
||
foreach (var parameter in parameterCollection) | ||
{ | ||
var parameterName = parameter.Name; | ||
if (!parameterWriters.TryGetValue(parameterName, out var parameterWriter)) | ||
if (!writers.WritersByName.TryGetValue(parameterName, out var writerWithIndex)) | ||
{ | ||
ThrowForUnknownIncomingParameterName(targetType, parameterName); | ||
} | ||
|
||
try | ||
{ | ||
parameterWriter(target, parameter.Value); | ||
writerWithIndex.Writer.SetValue(target, parameter.Value); | ||
usageFlags[writerWithIndex.Index] = true; | ||
numUsedWriters++; | ||
} | ||
catch (Exception ex) | ||
{ | ||
|
@@ -62,43 +73,28 @@ public static void AssignToProperties( | |
$"type '{target.GetType().FullName}'. The error was: {ex.Message}", ex); | ||
} | ||
} | ||
} | ||
|
||
internal static IEnumerable<PropertyInfo> GetCandidateBindableProperties(Type targetType) | ||
=> MemberAssignment.GetPropertiesIncludingInherited(targetType, _bindablePropertyFlags); | ||
|
||
private static IDictionary<string, WriteParameterAction> CreateParameterWriters(Type targetType) | ||
{ | ||
var result = new Dictionary<string, WriteParameterAction>(StringComparer.OrdinalIgnoreCase); | ||
|
||
foreach (var propertyInfo in GetCandidateBindableProperties(targetType)) | ||
// Now we can determine whether any writers have not been used, and if there are | ||
// some unused ones, find them. | ||
for (var index = 0; numUsedWriters < numWriters; index++) | ||
{ | ||
var shouldCreateWriter = propertyInfo.IsDefined(typeof(ParameterAttribute)) | ||
|| propertyInfo.IsDefined(typeof(CascadingParameterAttribute)); | ||
if (!shouldCreateWriter) | ||
if (index >= numWriters) | ||
{ | ||
continue; | ||
// This should not be possible | ||
throw new InvalidOperationException("Ran out of writers before marking them all as used."); | ||
} | ||
|
||
var propertySetter = MemberAssignment.CreatePropertySetter(targetType, propertyInfo); | ||
|
||
var propertyName = propertyInfo.Name; | ||
if (result.ContainsKey(propertyName)) | ||
if (!usageFlags[index]) | ||
{ | ||
throw new InvalidOperationException( | ||
$"The type '{targetType.FullName}' declares more than one parameter matching the " + | ||
$"name '{propertyName.ToLowerInvariant()}'. Parameter names are case-insensitive and must be unique."); | ||
writers.WritersByIndex[index].SetDefaultValue(target); | ||
numUsedWriters++; | ||
} | ||
|
||
result.Add(propertyName, (object target, object parameterValue) => | ||
{ | ||
propertySetter.SetValue(target, parameterValue); | ||
}); | ||
} | ||
|
||
return result; | ||
} | ||
|
||
internal static IEnumerable<PropertyInfo> GetCandidateBindableProperties(Type targetType) | ||
=> MemberAssignment.GetPropertiesIncludingInherited(targetType, _bindablePropertyFlags); | ||
|
||
private static void ThrowForUnknownIncomingParameterName(Type targetType, string parameterName) | ||
{ | ||
// We know we're going to throw by this stage, so it doesn't matter that the following | ||
|
@@ -126,5 +122,47 @@ private static void ThrowForUnknownIncomingParameterName(Type targetType, string | |
$"matching the name '{parameterName}'."); | ||
} | ||
} | ||
|
||
class WritersForType | ||
{ | ||
public Dictionary<string, (int Index, IPropertySetter Writer)> WritersByName { get; } | ||
public List<IPropertySetter> WritersByIndex { get; } | ||
|
||
public WritersForType(Type targetType) | ||
{ | ||
var propertySettersByName = new Dictionary<string, IPropertySetter>(StringComparer.OrdinalIgnoreCase); | ||
foreach (var propertyInfo in GetCandidateBindableProperties(targetType)) | ||
{ | ||
var shouldCreateWriter = propertyInfo.IsDefined(typeof(ParameterAttribute)) | ||
|| propertyInfo.IsDefined(typeof(CascadingParameterAttribute)); | ||
if (!shouldCreateWriter) | ||
{ | ||
continue; | ||
} | ||
|
||
var propertySetter = MemberAssignment.CreatePropertySetter(targetType, propertyInfo); | ||
|
||
var propertyName = propertyInfo.Name; | ||
if (propertySettersByName.ContainsKey(propertyName)) | ||
{ | ||
throw new InvalidOperationException( | ||
$"The type '{targetType.FullName}' declares more than one parameter matching the " + | ||
$"name '{propertyName.ToLowerInvariant()}'. Parameter names are case-insensitive and must be unique."); | ||
} | ||
|
||
propertySettersByName.Add(propertyName, propertySetter); | ||
} | ||
|
||
// Now we know all the entries, construct the resulting list/dictionary | ||
// with well-defined indices | ||
WritersByIndex = new List<IPropertySetter>(); | ||
WritersByName = new Dictionary<string, (int, IPropertySetter)>(StringComparer.OrdinalIgnoreCase); | ||
foreach (var pair in propertySettersByName) | ||
{ | ||
WritersByName.Add(pair.Key, (WritersByIndex.Count, pair.Value)); | ||
WritersByIndex.Add(pair.Value); | ||
} | ||
} | ||
} | ||
} | ||
} |
6 changes: 3 additions & 3 deletions
6
src/Components/src/Microsoft.AspNetCore.Components/Reflection/IPropertySetter.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,12 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
|
||
namespace Microsoft.AspNetCore.Components.Reflection | ||
{ | ||
internal interface IPropertySetter | ||
{ | ||
void SetValue(object target, object value); | ||
|
||
void SetDefaultValue(object target); | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because this PR adds a usage of
stackalloc
, and prior tonetstandard2.1
, that is always treated as unsafe.Once we go to
netstandard2.1
theunsafe
modifier can be removed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, I think that's due to the language version not the TFM right? Either way I'm not too nervous about this as long as it works in wasm.