Add FixupConstructorAutoPropertyInitializers and related tests

PropertyChanged.Fody/FixupConstructorAutoPropertyInitializers.cs

@@ -0,0 +1,90 @@
+using System;
+using System.Linq;
+
+using Mono.Cecil;
+using Mono.Cecil.Cil;
+
+public partial class ModuleWeaver
+{
+    private static void FixupConstructorAutoPropertyInitializers(TypeNode node)
+    {
+        /* 
+         * Initializing auto-properties in the constructor(s) will implicityl generate 
+         * a call to the virtual "OnPropertyChanged" methdod after the class is weaved,
+         * which will generate a CA2214:DoNotCallOverridableMethodsInConstructors:
+         * 
+         *  [ImplementPropertyChanged]
+         *  public class Class
+         *  {
+         *      public Class(string value)
+         *      {
+         *          Property = value;
+         *      }
+         *
+         *      public string Property { get; set; }
+         *      
+         *      public bool IsChanged { get; set; }
+         *  }
+         *  
+         * Another annoying side effect is that if the class has an IsChanged property, 
+         * IsChanged returns true right after creating the object.
+         * 
+         * Usually you would initialize the backing field instead of the property, 
+         * but with auto-properties there is no way to access the backing field, and converting
+         * all auto-properties to properties with backing fields would make PropertyChanged.Fody nearly obsolete.
+         * 
+         * To avoid this issue, replace all auto-property setter calls in constructors with a setter of the backing field.
+         * 
+         * This will however not catch initializing auto-properties from methods called by the constructor.
+         */
+
+        foreach (var ctor in node.TypeDefinition.Methods.Where(method => method.IsConstructor && method.HasBody))
+        {
+            var instructions = ctor.Body.Instructions;
+
+            for (var index = 0; index < instructions.Count; index++)
+            {
+                var instruction = instructions[index];
+
+                if (!IsPropertySetterCall(instruction, out string propertyName))
+                    continue;
+
+                var property = node.PropertyDatas.FirstOrDefault(item => string.Equals(item.PropertyDefinition.Name, propertyName, StringComparison.Ordinal));
+
+                var backingField = property?.BackingFieldReference;
+
+                var customAttributes = backingField?.Resolve()?.CustomAttributes;
+
+                if (true != customAttributes?.Any(item => item.AttributeType.FullName == "System.Runtime.CompilerServices.CompilerGeneratedAttribute"))
+                    continue;
+
+                //if (backingField?.Name != $"<{propertyName}>k__BackingField")
+                //    continue;
+
+                instructions[index] = Instruction.Create(OpCodes.Stfld, backingField);
+            }
+        }
+    }
+
+    private static bool IsPropertySetterCall(Instruction instruction, out string propertyName)
+    {
+        propertyName = null;
+
+        if (instruction.OpCode.Code != Code.Call)
+            return false;
+
+        var operand = instruction.Operand as MethodDefinition;
+        if (operand == null)
+            return false;
+
+        if (!operand.IsSetter)
+            return false;
+
+        var operandName = operand.Name;
+        if (!operandName.StartsWith("set_"))
+            return false;
+
+        propertyName = operandName.Substring(4);
+        return true;
+    }
+}

PropertyChanged.Fody/TypeProcessor.cs

@@ -24,9 +24,9 @@ void ProcessTypes(List<TypeNode> notifyNodes)
                 var body = propertyData.PropertyDefinition.SetMethod.Body;
 
                 var alreadyHasEquality = HasEqualityChecker.AlreadyHasEquality(propertyData.PropertyDefinition, propertyData.BackingFieldReference);
-               
+
                 body.SimplifyMacros();
-             
+
                 body.MakeLastStatementReturn();
 
                 var propertyWeaver = new PropertyWeaver(this, propertyData, node, ModuleDefinition.TypeSystem);
@@ -42,6 +42,8 @@ void ProcessTypes(List<TypeNode> notifyNodes)
                 body.OptimizeMacros();
             }
 
+            FixupConstructorAutoPropertyInitializers(node);
+
             ProcessTypes(node.Nodes);
         }
     }

PropertyChangedTests/BaseTaskTests.cs

@@ -9,7 +9,7 @@
 using System.Reflection;
 using NUnit.Framework;
 
-public abstract class BaseTaskTests
+public abstract partial class BaseTaskTests
 {
     string assemblyName;
     Assembly assembly;

PropertyChangedTests/TypesWithInitializedAutoPropertiesTests.cs

@@ -0,0 +1,161 @@
+using System.ComponentModel;
+
+using NUnit.Framework;
+
+public abstract partial class BaseTaskTests
+{
+    [Test]
+    [TestCase("WithInlineInitializedAutoProperties")]
+    [TestCase("WithExplicitConstructorInitializedAutoProperties")]
+    public void WithInitializedAutoPropertiesTest(string className)
+    {
+        var instance = assembly.GetInstance(className);
+
+        var propertyEventCount = 0;
+        ((INotifyPropertyChanged)instance).PropertyChanged += (sender, args) =>
+        {
+            propertyEventCount++;
+        };
+
+        Assert.IsFalse(instance.IsChanged);
+        Assert.AreEqual("Test", instance.Property1);
+        Assert.AreEqual("Test2", instance.Property2);
+
+        // setting any property inplicitly sets "IsChanged", so we get 2 events!
+
+        instance.Property1 = "a";
+        Assert.AreEqual(2, propertyEventCount);
+        Assert.IsTrue(instance.IsChanged);
+
+        instance.IsChanged = false;
+        Assert.AreEqual(3, propertyEventCount);
+
+        instance.Property2 = "b";
+        Assert.AreEqual(5, propertyEventCount);
+        Assert.IsTrue(instance.IsChanged);
+    }
+
+    [Test]
+    public void WithInitializedAutoPropertiesDerivedWeakDesignTest()
+    {
+        var instance = assembly.GetInstance("WithExplicitConstructorInitializedAutoPropertiesDerivedWeakDesign");
+
+        var propertyEventCount = 0;
+        ((INotifyPropertyChanged)instance).PropertyChanged += (sender, args) =>
+        {
+            propertyEventCount++;
+        };
+
+        // weak class design: derived class can't access backing field of base class
+        Assert.IsTrue(instance.IsChanged);
+        Assert.AreEqual("Derived", instance.Property1);
+        Assert.AreEqual("Derived2", instance.Property2);
+
+        // setting any property inplicitly sets "IsChanged", so we sometimes get 2 events!
+
+        instance.Property1 = "a";
+        Assert.AreEqual(1, propertyEventCount);
+        Assert.IsTrue(instance.IsChanged);
+
+        instance.IsChanged = false;
+        Assert.AreEqual(2, propertyEventCount);
+
+        instance.Property2 = "b";
+        Assert.AreEqual(4, propertyEventCount);
+        Assert.IsTrue(instance.IsChanged);
+    }
+
+    [Test]
+    public void WithInitializedAutoPropertiesDerivedProperDesignTest()
+    {
+        var instance = assembly.GetInstance("WithExplicitConstructorInitializedAutoPropertiesDerivedProperDesign");
+
+        var propertyEventCount = 0;
+        ((INotifyPropertyChanged)instance).PropertyChanged += (sender, args) =>
+        {
+            propertyEventCount++;
+        };
+
+        // works with derived classes if class design is properly done...
+        Assert.IsFalse(instance.IsChanged);
+        Assert.AreEqual("Derived", instance.Property1);
+        Assert.AreEqual("Derived2", instance.Property2);
+
+        // setting any property inplicitly sets "IsChanged", so we sometimes get 2 events!
+
+        instance.Property1 = "a";
+        Assert.AreEqual(2, propertyEventCount);
+        Assert.IsTrue(instance.IsChanged);
+
+        instance.IsChanged = false;
+        Assert.AreEqual(3, propertyEventCount);
+
+        instance.Property2 = "b";
+        Assert.AreEqual(5, propertyEventCount);
+        Assert.IsTrue(instance.IsChanged);
+    }
+
+    [Test]
+    public void WithInitializedAutoPropertiesInMethodTest()
+    {
+        var instance = assembly.GetInstance("WithMethodInitializedAutoProperties");
+
+        var propertyEventCount = 0;
+        ((INotifyPropertyChanged)instance).PropertyChanged += (sender, args) =>
+        {
+            propertyEventCount++;
+        };
+
+        // setting a property from within an extra method is not handled.
+        Assert.IsTrue(instance.IsChanged);
+
+        Assert.AreEqual("Test", instance.Property1);
+        Assert.AreEqual("Test2", instance.Property2);
+
+        // setting any property inplicitly sets "IsChanged", so we get 2 events!
+
+        instance.Property1 = "a";
+        Assert.AreEqual(1, propertyEventCount);
+        Assert.IsTrue(instance.IsChanged);
+
+        instance.IsChanged = false;
+        Assert.AreEqual(2, propertyEventCount);
+
+        instance.Property2 = "b";
+        Assert.AreEqual(4, propertyEventCount);
+        Assert.IsTrue(instance.IsChanged);
+    }
+
+    [Test]
+    public void WithInitializedAutoPropertiesAndINPCImplementationFromBaseClassTest()
+    {
+        var instance = assembly.GetInstance("WithObservableBaseClass");
+
+        Assert.AreEqual("Test", instance.Property1);
+        Assert.AreEqual("Test2", instance.Property2);
+        Assert.AreEqual(0, instance.VirtualMethodCalls);
+
+        instance.Property1 = "a";
+        Assert.IsTrue(instance.BaseNotifyCalled); // has changed indirectly => 2 VM calls.
+        Assert.AreEqual(2, instance.VirtualMethodCalls);
+
+        instance.Property2 = "b";
+        Assert.AreEqual(3, instance.VirtualMethodCalls);
+    }
+    [Test]
+    public void WithInitializedBackingFieldPropertiesAndINPCImplementationFromBaseClassTest()
+    {
+        var instance = assembly.GetInstance("WithBackingFieldsAndPropertySetterInConstructor");
+
+        Assert.AreEqual("Test", instance.Property1);
+        Assert.AreEqual("Test2", instance.Property2);
+        Assert.IsTrue(instance.BaseNotifyCalled); // has changed indirectly => 3 VM calls.
+        Assert.AreEqual(3, instance.VirtualMethodCalls);
+
+        instance.Property1 = "a";
+        Assert.AreEqual(4, instance.VirtualMethodCalls);
+
+        instance.Property2 = "b";
+        Assert.AreEqual(5, instance.VirtualMethodCalls);
+    }
+}