Unintended side effects of weaver on setters with custom logic

[xeniorn]

Describe the issue

PropertyChanged.Fody weaver makes unintended changes to behavior of setters with custom logic. Specifically, it prevents any custom code in the setter from firing when the value is unchanged, except when configured not to check for equality.

Minimal Repro

Test cases demonstrating this added here
Pull request will be created.

Make an effort to fix the bug

To fix this behavior, the weaver would have to be adjusted so that the control logic using the equality check is constrained only to the OnPropertyChanged injection, rather than indiscriminately applying to the setter as a whole.

So EnqualityCheckWeaver.cs:InjectEqualityCheck(Instruction targetInstruction, TypeReference targetType, TypeReference declaringType) would have to be adjusted.

1. to ensure custom code prior to backing field/property assignment is run, it should accept an insertion site as input, or detect the proper insertion site internally

2. to ensure custom code after backing field/property assignment is run, the inserted instructions should not be equivalent to if (true) return; else //continue i.e. using OpCodes.Ret, but rather if (true) //continue; else OnPropChanged();

[xeniorn]

If a fix was introduced, it would technically be a breaking change. Even if undocumented, the current impl. has a side effect of controlling the entire setter's execution, which would change with the fix.

Using PropertyChanged injection with custom setters is probably a relatively niche use case, but for full backward compatibility one could add a configuration option for this, with the default being legacy behavior, until you're ready for a major version change.

[ltrzesniewski]

I think changing this now would be too much of a breaking change. This weaver is used by many people, and I'm pretty sure at least some of them rely on this behavior.

[xeniorn]

I agree, I would suggest adding a config option instead. Then it's a feature, not a breaking change.

[tom-englert]

This weaver is used by many people, and I'm pretty sure at least some of them rely on this behavior.

Fully agree, I'd also rather see this as the expected behavior,

I think this request is targeting a rare edge case, the primary target of PropertyChanged.Fody is to use auto-properties and avoid any custom setter code.
And since this change would cause a huge impact, I'd rather document this behavior, and advise to use [DoNotNotify] and then implement everything by your own if you have such a special requirement.

[tom-englert]

Updated Wiki to document the behavior: https://github.com/Fody/PropertyChanged/wiki/EqualityChecking