Microsoft.Toolkit.Mvvm package (part 2) (#3413)

msftbot[bot] · web-flow · commit ff7f24de1a98 · 2020-08-11T17:36:09.000Z
## Follow up to #3229  ### NOTE Marking this not as a draft so that the CI will run, but added the "DO NOT MERGE ⚠" tag as we might come up with further improvements to add to this PR before actually deciding to merge this and finalize the package.  ## PR Type What kind of change does this PR introduce?  - Bugfix - Feature - Refactoring (no functional changes, no api changes)     ## Notable changes  This PR includes some refinements and tweaks to the `Microsoft.Toolkit.Mvvm` package, like: - Added new `ObservableRecipient.SetProperty` overloads with `Expression<Func<T>>` and `bool broadcast` params - Fixed a possible bug in `Messenger.Type2.Equals` (just to be extra sure) - Improved codegen for `ObservableRecipient.Set<T>(ref T, T, string)` (missing `EqualityComparer<T>.Default.Equals` inlining) ## PR Checklist Please check if your PR fulfills the following requirements: - [X] Tested code with current [supported SDKs](../readme.md#supported) - [ ] ~~Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: ~~ - [ ] ~~Sample in sample app has been added / updated (for bug fixes / features)~~ - [ ] ~~Icon has been created (if new sample) following the [Thumbnail Style Guide and templates](https://github.com/windows-toolkit/WindowsCommunityToolkit-design-assets)~~ - [X] Tests for the changes have been added (for bug fixes / features) (if applicable) - [X] Header has been added to all new source files (run *build/UpdateHeaders.bat*) - [X] Contains **NO** breaking changes
diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs
@@ -66,7 +66,26 @@ protected virtual void OnPropertyChanging([CallerMemberName] string? propertyNam
         /// </remarks>
         protected bool SetProperty<T>(ref T field, T newValue, [CallerMemberName] string? propertyName = null)
         {
-            return SetProperty(ref field, newValue, EqualityComparer<T>.Default, propertyName);
+            // We duplicate the code here instead of calling the overload because we can't
+            // guarantee that the invoked SetProperty<T> will be inlined, and we need the JIT
+            // to be able to see the full EqualityComparer<T>.Default.Equals call, so that
+            // it'll use the intrinsics version of it and just replace the whole invocation
+            // with a direct comparison when possible (eg. for primitive numeric types).
+            // This is the fastest SetProperty<T> overload so we particularly care about
+            // the codegen quality here, and the code is small and simple enough so that
+            // duplicating it still doesn't make the whole class harder to maintain.
+            if (EqualityComparer<T>.Default.Equals(field, newValue))
+            {
+                return false;
+            }
+
+            OnPropertyChanging(propertyName);
+
+            field = newValue;
+
+            OnPropertyChanged(propertyName);
+
+            return true;
         }
 
         /// <summary>
@@ -203,7 +222,7 @@ protected bool SetProperty<T>(T oldValue, T newValue, IEqualityComparer<T> compa
         /// </remarks>
         protected bool SetProperty<T>(Expression<Func<T>> propertyExpression, T newValue, [CallerMemberName] string? propertyName = null)
         {
-            return SetProperty(propertyExpression, newValue, EqualityComparer<T>.Default, propertyName);
+            return SetProperty(propertyExpression, newValue, EqualityComparer<T>.Default, out _, propertyName);
         }
 
         /// <summary>
@@ -220,6 +239,21 @@ protected bool SetProperty<T>(Expression<Func<T>> propertyExpression, T newValue
         /// <param name="propertyName">(optional) The name of the property that changed.</param>
         /// <returns><see langword="true"/> if the property was changed, <see langword="false"/> otherwise.</returns>
         protected bool SetProperty<T>(Expression<Func<T>> propertyExpression, T newValue, IEqualityComparer<T> comparer, [CallerMemberName] string? propertyName = null)
+        {
+            return SetProperty(propertyExpression, newValue, comparer, out _, propertyName);
+        }
+
+        /// <summary>
+        /// Implements the shared logic for <see cref="SetProperty{T}(Expression{Func{T}},T,IEqualityComparer{T},string)"/>
+        /// </summary>
+        /// <typeparam name="T">The type of property to set.</typeparam>
+        /// <param name="propertyExpression">An <see cref="Expression{TDelegate}"/> returning the property to update.</param>
+        /// <param name="newValue">The property's value after the change occurred.</param>
+        /// <param name="comparer">The <see cref="IEqualityComparer{T}"/> instance to use to compare the input values.</param>
+        /// <param name="oldValue">The resulting initial value for the target property.</param>
+        /// <param name="propertyName">(optional) The name of the property that changed.</param>
+        /// <returns><see langword="true"/> if the property was changed, <see langword="false"/> otherwise.</returns>
+        private protected bool SetProperty<T>(Expression<Func<T>> propertyExpression, T newValue, IEqualityComparer<T> comparer, out T oldValue, [CallerMemberName] string? propertyName = null)
         {
             PropertyInfo? parentPropertyInfo;
             FieldInfo? parentFieldInfo = null;
@@ -236,13 +270,15 @@ parentExpression.Expression is ConstantExpression instanceExpression &&
                 ThrowArgumentExceptionForInvalidPropertyExpression();
 
                 // This is never executed, as the method above always throws
+                oldValue = default!;
+
                 return false;
             }
 
             object parent = parentPropertyInfo is null
                 ? parentFieldInfo!.GetValue(instance)
                 : parentPropertyInfo.GetValue(instance);
-            T oldValue = (T)targetPropertyInfo.GetValue(parent);
+            oldValue = (T)targetPropertyInfo.GetValue(parent);
 
             if (comparer.Equals(oldValue, newValue))
             {
diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableRecipient.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableRecipient.cs
@@ -9,6 +9,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Linq.Expressions;
 using System.Runtime.CompilerServices;
 using Microsoft.Toolkit.Mvvm.Messaging;
 using Microsoft.Toolkit.Mvvm.Messaging.Messages;
@@ -140,7 +141,18 @@ protected virtual void Broadcast<T>(T oldValue, T newValue, string? propertyName
         /// </remarks>
         protected bool SetProperty<T>(ref T field, T newValue, bool broadcast, [CallerMemberName] string? propertyName = null)
         {
-            return SetProperty(ref field, newValue, EqualityComparer<T>.Default, broadcast, propertyName);
+            T oldValue = field;
+
+            // We duplicate the code as in the base class here to leverage
+            // the intrinsics support for EqualityComparer<T>.Default.Equals.
+            bool propertyChanged = SetProperty(ref field, newValue, propertyName);
+
+            if (propertyChanged && broadcast)
+            {
+                Broadcast(oldValue, newValue, propertyName);
+            }
+
+            return propertyChanged;
         }
 
         /// <summary>
@@ -158,21 +170,16 @@ protected bool SetProperty<T>(ref T field, T newValue, bool broadcast, [CallerMe
         /// <returns><see langword="true"/> if the property was changed, <see langword="false"/> otherwise.</returns>
         protected bool SetProperty<T>(ref T field, T newValue, IEqualityComparer<T> comparer, bool broadcast, [CallerMemberName] string? propertyName = null)
         {
-            if (!broadcast)
-            {
-                return SetProperty(ref field, newValue, comparer, propertyName);
-            }
-
             T oldValue = field;
 
-            if (SetProperty(ref field, newValue, comparer, propertyName))
+            bool propertyChanged = SetProperty(ref field, newValue, comparer, propertyName);
+
+            if (propertyChanged && broadcast)
             {
                 Broadcast(oldValue, newValue, propertyName);
-
-                return true;
             }
 
-            return false;
+            return propertyChanged;
         }
 
         /// <summary>
@@ -216,19 +223,61 @@ protected bool SetProperty<T>(T oldValue, T newValue, Action<T> callback, bool b
         /// <returns><see langword="true"/> if the property was changed, <see langword="false"/> otherwise.</returns>
         protected bool SetProperty<T>(T oldValue, T newValue, IEqualityComparer<T> comparer, Action<T> callback, bool broadcast, [CallerMemberName] string? propertyName = null)
         {
-            if (!broadcast)
+            bool propertyChanged = SetProperty(oldValue, newValue, comparer, callback, propertyName);
+
+            if (propertyChanged && broadcast)
             {
-                return SetProperty(oldValue, newValue, comparer, callback, propertyName);
+                Broadcast(oldValue, newValue, propertyName);
             }
 
-            if (SetProperty(oldValue, newValue, comparer, callback, propertyName))
+            return propertyChanged;
+        }
+
+        /// <summary>
+        /// Compares the current and new values for a given nested property. If the value has changed,
+        /// raises the <see cref="ObservableObject.PropertyChanging"/> event, updates the property and then raises the
+        /// <see cref="ObservableObject.PropertyChanged"/> event. The behavior mirrors that of
+        /// <see cref="ObservableObject.SetProperty{T}(Expression{Func{T}},T,string)"/>, with the difference being that this
+        /// method is used to relay properties from a wrapped model in the current instance. For more info, see the docs for
+        /// <see cref="ObservableObject.SetProperty{T}(Expression{Func{T}},T,string)"/>.
+        /// </summary>
+        /// <typeparam name="T">The type of property to set.</typeparam>
+        /// <param name="propertyExpression">An <see cref="Expression{TDelegate}"/> returning the property to update.</param>
+        /// <param name="newValue">The property's value after the change occurred.</param>
+        /// <param name="broadcast">If <see langword="true"/>, <see cref="Broadcast{T}"/> will also be invoked.</param>
+        /// <param name="propertyName">(optional) The name of the property that changed.</param>
+        /// <returns><see langword="true"/> if the property was changed, <see langword="false"/> otherwise.</returns>
+        protected bool SetProperty<T>(Expression<Func<T>> propertyExpression, T newValue, bool broadcast, [CallerMemberName] string? propertyName = null)
+        {
+            return SetProperty(propertyExpression, newValue, EqualityComparer<T>.Default, broadcast, propertyName);
+        }
+
+        /// <summary>
+        /// Compares the current and new values for a given nested property. If the value has changed,
+        /// raises the <see cref="ObservableObject.PropertyChanging"/> event, updates the property and then raises the
+        /// <see cref="ObservableObject.PropertyChanged"/> event. The behavior mirrors that of
+        /// <see cref="ObservableObject.SetProperty{T}(Expression{Func{T}},T,IEqualityComparer{T},string)"/>,
+        /// with the difference being that this method is used to relay properties from a wrapped model in the
+        /// current instance. For more info, see the docs for
+        /// <see cref="ObservableObject.SetProperty{T}(Expression{Func{T}},T,IEqualityComparer{T},string)"/>.
+        /// </summary>
+        /// <typeparam name="T">The type of property to set.</typeparam>
+        /// <param name="propertyExpression">An <see cref="Expression{TDelegate}"/> returning the property to update.</param>
+        /// <param name="newValue">The property's value after the change occurred.</param>
+        /// <param name="comparer">The <see cref="IEqualityComparer{T}"/> instance to use to compare the input values.</param>
+        /// <param name="broadcast">If <see langword="true"/>, <see cref="Broadcast{T}"/> will also be invoked.</param>
+        /// <param name="propertyName">(optional) The name of the property that changed.</param>
+        /// <returns><see langword="true"/> if the property was changed, <see langword="false"/> otherwise.</returns>
+        protected bool SetProperty<T>(Expression<Func<T>> propertyExpression, T newValue, IEqualityComparer<T> comparer, bool broadcast, [CallerMemberName] string? propertyName = null)
+        {
+            bool propertyChanged = SetProperty(propertyExpression, newValue, comparer, out T oldValue, propertyName);
+
+            if (propertyChanged && broadcast)
             {
                 Broadcast(oldValue, newValue, propertyName);
-
-                return true;
             }
 
-            return false;
+            return propertyChanged;
         }
     }
 }
diff --git a/Microsoft.Toolkit.Mvvm/DependencyInjection/Ioc.cs b/Microsoft.Toolkit.Mvvm/DependencyInjection/Ioc.cs
@@ -51,7 +51,7 @@ public sealed class Ioc : IServiceProvider
         /// <summary>
         /// The <see cref="ServiceProvider"/> instance to use, if initialized.
         /// </summary>
-        private ServiceProvider? serviceProvider;
+        private volatile ServiceProvider? serviceProvider;
 
         /// <inheritdoc/>
         object? IServiceProvider.GetService(Type serviceType)
diff --git a/Microsoft.Toolkit.Mvvm/Messaging/Messenger.cs b/Microsoft.Toolkit.Mvvm/Messaging/Messenger.cs
@@ -638,9 +638,14 @@ public Type2(Type tMessage, Type tToken)
             [MethodImpl(MethodImplOptions.AggressiveInlining)]
             public bool Equals(Type2 other)
             {
+                // We can't just use reference equality, as that's technically not guaranteed
+                // to work and might fail in very rare cases (eg. with type forwarding between
+                // different assemblies). Instead, we can use the == operator to compare for
+                // equality, which still avoids the callvirt overhead of calling Type.Equals,
+                // and is also implemented as a JIT intrinsic on runtimes such as .NET Core.
                 return
-                    ReferenceEquals(this.tMessage, other.tMessage) &&
-                    ReferenceEquals(this.tToken, other.tToken);
+                    this.tMessage == other.tMessage &&
+                    this.tToken == other.tToken;
             }
 
             /// <inheritdoc/>
diff --git a/Microsoft.Toolkit.Mvvm/Messaging/MessengerExtensions.cs b/Microsoft.Toolkit.Mvvm/Messaging/MessengerExtensions.cs
@@ -17,9 +17,37 @@ namespace Microsoft.Toolkit.Mvvm.Messaging
     public static partial class MessengerExtensions
     {
         /// <summary>
-        /// The <see cref="MethodInfo"/> instance associated with <see cref="Register{TMessage,TToken}(IMessenger,IRecipient{TMessage},TToken)"/>.
+        /// A class that acts as a container to load the <see cref="MethodInfo"/> instance linked to
+        /// the <see cref="Register{TMessage,TToken}(IMessenger,IRecipient{TMessage},TToken)"/> method.
+        /// This class is needed to avoid forcing the initialization code in the static constructor to run as soon as
+        /// the <see cref="MessengerExtensions"/> type is referenced, even if that is done just to use methods
+        /// that do not actually require this <see cref="MethodInfo"/> instance to be available.
+        /// We're effectively using this type to leverage the lazy loading of static constructors done by the runtime.
         /// </summary>
-        private static readonly MethodInfo RegisterIRecipientMethodInfo;
+        private static class MethodInfos
+        {
+            /// <summary>
+            /// Initializes static members of the <see cref="MethodInfos"/> class.
+            /// </summary>
+            static MethodInfos()
+            {
+                RegisterIRecipient = (
+                    from methodInfo in typeof(MessengerExtensions).GetMethods()
+                    where methodInfo.Name == nameof(Register) &&
+                          methodInfo.IsGenericMethod &&
+                          methodInfo.GetGenericArguments().Length == 2
+                    let parameters = methodInfo.GetParameters()
+                    where parameters.Length == 3 &&
+                          parameters[1].ParameterType.IsGenericType &&
+                          parameters[1].ParameterType.GetGenericTypeDefinition() == typeof(IRecipient<>)
+                    select methodInfo).First();
+            }
+
+            /// <summary>
+            /// The <see cref="MethodInfo"/> instance associated with <see cref="Register{TMessage,TToken}(IMessenger,IRecipient{TMessage},TToken)"/>.
+            /// </summary>
+            public static readonly MethodInfo RegisterIRecipient;
+        }
 
         /// <summary>
         /// A class that acts as a static container to associate a <see cref="ConditionalWeakTable{TKey,TValue}"/> instance to each
@@ -39,23 +67,6 @@ public static readonly ConditionalWeakTable<Type, Action<IMessenger, object, TTo
                 = new ConditionalWeakTable<Type, Action<IMessenger, object, TToken>[]>();
         }
 
-        /// <summary>
-        /// Initializes static members of the <see cref="MessengerExtensions"/> class.
-        /// </summary>
-        static MessengerExtensions()
-        {
-            RegisterIRecipientMethodInfo = (
-                from methodInfo in typeof(MessengerExtensions).GetMethods()
-                where methodInfo.Name == nameof(Register) &&
-                      methodInfo.IsGenericMethod &&
-                      methodInfo.GetGenericArguments().Length == 2
-                let parameters = methodInfo.GetParameters()
-                where parameters.Length == 3 &&
-                      parameters[1].ParameterType.IsGenericType &&
-                      parameters[1].ParameterType.GetGenericTypeDefinition() == typeof(IRecipient<>)
-                select methodInfo).First();
-        }
-
         /// <summary>
         /// Checks whether or not a given recipient has already been registered for a message.
         /// </summary>
@@ -110,7 +121,7 @@ from interfaceType in type.GetInterfaces()
                     where interfaceType.IsGenericType &&
                           interfaceType.GetGenericTypeDefinition() == typeof(IRecipient<>)
                     let messageType = interfaceType.GenericTypeArguments[0]
-                    let registrationMethod = RegisterIRecipientMethodInfo.MakeGenericMethod(messageType, typeof(TToken))
+                    let registrationMethod = MethodInfos.RegisterIRecipient.MakeGenericMethod(messageType, typeof(TToken))
                     let registrationAction = GetRegistrationAction(type, registrationMethod)
                     select registrationAction).ToArray();
             }
diff --git a/Microsoft.Toolkit.Mvvm/Microsoft.Toolkit.Mvvm.csproj b/Microsoft.Toolkit.Mvvm/Microsoft.Toolkit.Mvvm.csproj
@@ -4,16 +4,16 @@
     <TargetFrameworks>netstandard2.0;netstandard2.1</TargetFrameworks>
     <LangVersion>8.0</LangVersion>
     <Nullable>enable</Nullable>
-    <Title>Windows Community Toolkit Mvvm .NET Standard</Title>
+    <Title>Windows Community Toolkit MVVM Toolkit</Title>
     <Description>
-      This package includes Mvvm .NET Standard code only helpers such as:
+      This package includes a .NET Standard MVVM library with helpers such as:
       - ObservableObject: a base class for objects implementing the INotifyPropertyChanged interface.
       - ObservableRecipient: a base class for observable objects with support for the IMessenger service.
       - RelayCommand: a simple delegate command implementing the ICommand interface.
       - Messenger: a messaging system to exchange messages through different loosely-coupled objects.
       - Ioc: a helper class to configure dependency injection service containers.
     </Description>
-    <PackageTags>UWP Toolkit Windows Mvvm observable Ioc dependency injection services extensions helpers</PackageTags>
+    <PackageTags>UWP Toolkit Windows MVVM MVVMToolkit observable Ioc dependency injection services extensions helpers</PackageTags>
   </PropertyGroup>
 
   <!-- .NET Standard 2.0 doesn't have the Span<T> type -->
