PropertyChanging.Fody weaver interfering with PropertyChanged.Fody equality check option

[xeniorn]

Describe the issue

PropertyChanging.Fody interferes with PropertyChanged.Fody option "CheckForEquality".

Assuming that the two modules are intended to be compatible:
#169

Basic behavior seems to be correct. But PropertyChanged.Fody's CheckForEquality = false will be ignored for classes also implementing INotifyPropertyChanging, I assume due to weavers conflicting in some way.

For a class like:

[AddINotifyPropertyChangedInterface]
public class TestClassImplicitNoCheckEquality
{
    [DoNotCheckEquality]
    public object Value { get; set; }
}

The relevant part of IL code with PropertyChanged.Fody alone will look like:

  .method public hidebysig specialname instance void
    set_Value(
      object 'value'
    ) cil managed
  {
    .custom instance void [System.Runtime]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor()
      = (01 00 00 00 )
    .maxstack 2

    // [12 36 - 12 40]
    IL_0000: ldarg.0      // this
    IL_0001: ldarg.1      // 'value'
    IL_0002: stfld        object FodyTest.TestClassImplicit::'<Value>k__BackingField'
    IL_0007: ldarg.0      // this
    IL_0008: ldsfld       class [System.ObjectModel]System.ComponentModel.PropertyChangedEventArgs 'FodyCDoubleNet.<>PropertyChangedEventArgs'::Value
    IL_000d: callvirt     instance void FodyTest.TestClassImplicit::'<>OnPropertyChanged'(class [System.ObjectModel]System.ComponentModel.PropertyChangedEventArgs)
    IL_0012: nop
    IL_0013: ret

  } // end of method TestClassImplicit::set_Value

With added INotifyPropertyChanging and PropertyChanging.Fody:

.method public hidebysig specialname instance void
    set_Value(
      object 'value'
    ) cil managed
  {
    .custom instance void [System.Runtime]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor()
      = (01 00 00 00 )
    .maxstack 2

    IL_0000: ldarg.0      // this
    IL_0001: ldfld        object FodyTest.TestClassImplicit::'<Value>k__BackingField'
    IL_0006: ldarg.1      // 'value'
    IL_0007: call         bool [System.Runtime]System.Object::Equals(object, object)
    IL_000c: brfalse.s    IL_000f
    IL_000e: ret
    IL_000f: nop

    // [18 36 - 18 40]
    IL_0010: ldarg.0      // this
    IL_0011: ldarg.1      // 'value'
    IL_0012: stfld        object FodyTest.TestClassImplicit::'<Value>k__BackingField'
    IL_0017: ldarg.0      // this
    IL_0018: ldsfld       class [System.ObjectModel]System.ComponentModel.PropertyChangedEventArgs 'FodyCDoubleNet.<>PropertyChangedEventArgs'::Value
    IL_001d: callvirt     instance void FodyTest.TestClassImplicit::'<>OnPropertyChanged'(class [System.ObjectModel]System.ComponentModel.PropertyChangedEventArgs)
    IL_0022: nop
    IL_0023: ret

  } // end of method TestClassImplicit::set_Value

Diff of IL codes:

The first class will fire an event on assignment of same value; the second class will ignore it despite the DoNotCheckEquality attribute.

The same happens if the option is configured via weavers.xml

The same happens if the interface is explicitly implemented without the AddINotifyPropertyChangedInterface attribute.

Minimal Repro

Used the following test on a set of test classes:

public class UnitTestReflection
{
    [Theory]
    [InlineData(typeof(TestClassExplicit), 1, 1, 1)]
    [InlineData(typeof(TestClassExplicit), 1, 2, 2)]
    [InlineData(typeof(TestClassImplicit), 1, 1, 1)]
    [InlineData(typeof(TestClassImplicit), 1, 2, 2)]
    [InlineData(typeof(TestClassImplicitNoCheckEquality), 1, 1, 2)]
    [InlineData(typeof(TestClassImplicitNoCheckEquality), 1, 2, 2)]

    public void CorrectFireAll(Type type, object firstValue, object secondValue, int expectedCount)
    {
        var a = Activator.CreateInstance(type);

        var props = type.GetProperties();

        var valueProp = props.Single(x => x.PropertyType == typeof(object) && x.Name == "Value");

        var ap = (INotifyPropertyChanged)a;
        var count = 0;

        ap.PropertyChanged += (p, e) => count++;

        valueProp.SetValue(a, firstValue);
        valueProp.SetValue(a, secondValue);

        Assert.Equal(expectedCount, count);
    }
}

The class & test projects (VS2022 Community 17.12, NET9.0, Fody 6.9.1, PropertyChanged.Fody 4.1.0, PropertyChanging.Fody 1.30.3) :

FodyTest.zip

I can add a PR with the tests if desired, just wasn't sure which repo it should go to - here makes more sense I think.

[xeniorn]

@SimonCropp let me know if you want me to add a PR for a test for this

[tom-englert]

Since weavers don't know about each other, there is nothing PropertyChanged can do about this.
You would need the have both PropertyChanged and PropertyChanging to have the same CheckForEquality setting, so they don't interfere.

[xeniorn]

I don't know enough about the internals, but my hunch is that it should be possible to adjust the weaver in a way that this is not the case, with possibly a slight hit to performance.

Right now the check for equality IL code is something like:

if (!ValueDiffers()) return;
SetBackingField();
EmitPropertyChanged();

and it could be:

if (ValueDiffers())
{
 SetBackingField();
}

if (ValueDiffers())
{
 EmitPropertyChanged();
}

which would allow a composable:

if (ValueDiffers())
{
 EmitPropertyChanging();
}

if (ValueDiffers())
{
 SetBackingField();
}

if (ValueDiffers())
{
 EmitPropertyChanged();
}

It boils down to whether weavers are supposed to be maximally compatible or maximally performant. Defensive programming has a cost. Might possibly make sense to have a config option for this, which you would set if you want to apply multiple weavers? I assume this would happen for PropChanging/PropChanged.Fody with CheckEquality true alongside any weaver that adds anything to the property.

If the weaver's intention is to "add a PropertyChang(ing/ed).Fody" automatically, but without intending to "prevent any other calls in the property to happen", then it would make sense to call this a bug or undocumented behavior.

It's not only weavers that are interferred with. If you use the following, the non-weaved CustomCallback will also be skipped because of the early return:

[AddINotifyPropertyChangedInterface]
public class WithCustomCallback
{
    private object _value;
    public object Value
    {
        get => _value;
        set
        {
            _value = value;
            CustomCallback();
        }
    }

    private void CustomCallback()
    {
        Debug.Write("bla");
    }
}

---

Worst case scenario that still resolves the bug is to document the incompatibility. Then it's simply a limitation by design.

[tom-englert]

I never found a real use case for INotifyPropertyChanging, so I would vote for simply documenting this limitation in PropertyChanging.

PropertyChanging seems to be not so widely used and does not have all feature of PropertyChanged, like configuring to omit the equality check.
Since it does not really make sense to check equality for the one event, but omit it for the other, you would need to extend PropertyChanging with the same features to get the proper behavior.

[xeniorn]

I agree about relative limited utility of PropertyChanging compared to this repo.

Inserting equality check option in both wouldn't fix it though, they'd still conflict when one is set to false other to true.

More importantly, this has uncovered a broader issue which this one can probably be seen simply as a subset of:

#1101

I.e. this isn't only a multi-weaver issue, it's PropertyChanged.Fody and PropertyChanging.Fody's weavers doing undocumented changes to the setters, applying their equality checks at a too broad scope, interfering with expected behavior (insert PropertyChanged, don't break anything else).

[xeniorn]

Added pull request with tests demonstrating this:

#1102

[xeniorn]

The same issue exists in PropertyChanging.Fody, I'll add a separate issue there.

[tom-englert]

This behavior is now property documented: https://github.com/Fody/PropertyChanged/wiki/EqualityChecking