-
Notifications
You must be signed in to change notification settings - Fork 118
Narrow AdvancedCollectionView trimming warning so that it only triggers in unsafe usages #724
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
base: main
Are you sure you want to change the base?
Changes from all commits
2dff7ae
68766f7
c19cac2
ed2ef3b
fc613e9
69fc96c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |||
| // See the LICENSE file in the project root for more information. | ||||
|
|
||||
| using System.Collections; | ||||
| using System.Diagnostics.CodeAnalysis; | ||||
|
|
||||
| namespace CommunityToolkit.WinUI.Collections; | ||||
|
|
||||
|
|
@@ -14,7 +15,7 @@ public class SortDescription | |||
| /// <summary> | ||||
| /// Gets the name of property to sort on | ||||
| /// </summary> | ||||
| public string PropertyName { get; } | ||||
| public virtual string? PropertyName { get; } | ||||
|
|
||||
| /// <summary> | ||||
| /// Gets the direction of sort | ||||
|
|
@@ -33,8 +34,10 @@ public class SortDescription | |||
| /// <param name="direction">Direction of sort</param> | ||||
| /// <param name="comparer">Comparer to use. If null, will use default comparer</param> | ||||
| public SortDescription(SortDirection direction, IComparer? comparer = null) | ||||
| : this(null!, direction, comparer!) | ||||
| { | ||||
| PropertyName = null; | ||||
| Direction = direction; | ||||
| Comparer = comparer ?? ObjectComparer.Instance; | ||||
| } | ||||
|
|
||||
| /// <summary> | ||||
|
|
@@ -43,13 +46,22 @@ public SortDescription(SortDirection direction, IComparer? comparer = null) | |||
| /// <param name="propertyName">Name of property to sort on</param> | ||||
| /// <param name="direction">Direction of sort</param> | ||||
| /// <param name="comparer">Comparer to use. If null, will use default comparer</param> | ||||
| #if NET5_0_OR_GREATER | ||||
| [RequiresUnreferencedCode("Item sorting with the property name uses reflection to get the property and is not trim-safe. Either use SortDescription<T> to preserve the required metadata, or use the other constructor without a property name.")] | ||||
| #endif | ||||
| public SortDescription(string propertyName, SortDirection direction, IComparer? comparer = null) | ||||
| { | ||||
| PropertyName = propertyName; | ||||
| Direction = direction; | ||||
| Comparer = comparer ?? ObjectComparer.Instance; | ||||
| } | ||||
|
|
||||
|
|
||||
|
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: extra blank line
Suggested change
|
||||
| [UnconditionalSuppressMessage("Trimming", "IL2070:'this' argument does not satisfy 'DynamicallyAccessedMembersAttribute' in call to target method. The parameter of method does not have matching annotations.", | ||||
|
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. This is not safe. Someone could derive from 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. I went with this design to avoid adding a suppression in |
||||
| Justification = "The path which does reflection is only triggered if the user uses the constructor with RequiresUnreferencedCode (which bubbles the warning to them)")] | ||||
| internal virtual PropertyInfo? GetProperty(Type type) | ||||
| => PropertyName != null ? type.GetProperty(PropertyName) : null; | ||||
|
|
||||
| private class ObjectComparer : IComparer | ||||
| { | ||||
| public static readonly IComparer Instance = new ObjectComparer(); | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System.Collections; | ||
| using System.Diagnostics.CodeAnalysis; | ||
|
|
||
| namespace CommunityToolkit.WinUI.Collections; | ||
|
|
||
| /// <summary> | ||
| /// A generic version of <see cref="SortDescription"/> which preserves the required metadata for reflection-based sorting. | ||
| /// </summary> | ||
| /// <typeparam name="T">The type to sort</typeparam> | ||
| public class SortDescription< | ||
| #if NET5_0_OR_GREATER | ||
| [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] | ||
| #endif | ||
| T> : SortDescription | ||
| { | ||
| private readonly PropertyInfo _prop; | ||
|
|
||
| /// <inheritdoc/> | ||
| public override string PropertyName => _prop.Name; | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="SortDescription{T}"/> class. | ||
| /// </summary> | ||
| /// <param name="propertyName">Name of property to sort on</param> | ||
| /// <param name="direction">Direction of sort</param> | ||
| /// <param name="comparer">Comparer to use. If null, will use default comparer</param> | ||
| public SortDescription(string propertyName, SortDirection direction, IComparer? comparer = null) : base(direction, comparer) | ||
| { | ||
| _prop = typeof(T).GetProperty(propertyName) ?? throw new ArgumentException("Type does not have the expected property"); | ||
| } | ||
|
|
||
| internal override PropertyInfo? GetProperty(Type type) => | ||
| type.IsAssignableTo(_prop.DeclaringType) ? _prop : throw new ArgumentException("This SortDescription is not compatible with this type"); | ||
| } |
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 a breaking change technically