ExpandoObject PropertyChanged event fails to detect changes in value types

[JackUkleja]

ExpandoObject uses a IDictionary<string, object> under the hood for storing properties. It also implements INotifyProperyChanged such that when the value for a property changes (but only when it changes) it will raise the PropertyChanged event - i.e. the normal INPC pattern.

The problem is this does not work for value types - the PropertyChanged event is raised every time a value type property is set, regardless of whether the value has changed.

The problem is due to the code which deals with setting the property value TrySetValue which takes an object as the parameter type for the value. This forces value types to be boxed when passed in, and then the equality operator != is used to check if the value has changed - but for two ints which have been boxed into an object this operator is always going to return true.

           // Notify property changed, outside of the lock.
            var propertyChanged = _propertyChanged;
            if (propertyChanged != null && value != oldValue) {
                // Use the canonical case for the key.
                propertyChanged(this, new PropertyChangedEventArgs(data.Class.Keys[index]));
            }

I think the code should be:

           // Notify property changed, outside of the lock.
            var propertyChanged = _propertyChanged;
            if (propertyChanged != null && !value.Equals(oldValue)) {
                // Use the canonical case for the key.
                propertyChanged(this, new PropertyChangedEventArgs(data.Class.Keys[index]));
            }

[JonHanna]

A reference-type value changed to a new instance that was Equals() the previous should still be seen as a change though.

We could though replace value != oldValue with:

if (value == null)    // covers reference types and nullables, either way we can't value.GetType()
{
    if (oldValue == null)
    {
        return; // early return on not-changed makes the following branching simpler
    }
}
else if (value.GetTypeInfo().IsValueType)
{
    if (value.Equals(oldValue))
    {
        return;
    }
}
else if (value == oldValue)
{
    return;
}

_propertyChanged.?Invoke(this, new PropertyChangedEventArgs(data.Class.Keys[index]))

It'd still fail on some strange overrides for Equals(), but that's the Equals() overloads fault for being strange and maybe should be considered just that.

I'd like to roll this into my fix for dotnet/runtime#19357 and dotnet/runtime#19358 if nobody minds, as a fix for this should add test cases covering the fix and as there are currently no tests on this type and that PR also introduces some, there'd be an annoying conflict between two such fixes.

[JackUkleja]

I'm not convinced by the argument that:

"A reference-type value changed to a new instance that was Equals() the previous should still be seen as a change though."

I don't think setting a property to "Test Value" and then setting it to another instance of "Test Value" should raise a property changed event. The whole point of the Equals is that it can be overriden to make semantic sense for the type in question and if two strings are logically equal, I won't care to know if they happen to be different references.

I suspect there is some prior art already in the framework which must solve this problem - we might look there for a definitive implementation that handles this situation. Does it use your logic quoted above?

If we want to do things even more correctly we could be looking for inspiration in things like EqualityComparison<T>.Default which checks if IEqualityComparer implementations are available, otherwise uses fallbacks depending on type.

[JonHanna]

If there was a good way to detect immutability then that argument might hold for immutable types like string, but that x.Equals(y) holds for two instances of a reference type now does not mean that it will hold after that. Anyone depending on the event to keep track of the value of the property would now be tracking the wrong object.

we might look there for a definitive implementation that handles this situation

The closest thing to definitive that I can find is:

1. The documentation at https://msdn.microsoft.com/en-us/library/ms743695(v=vs.110).aspx that triggers the event for every call of the setter, whether it changes state or not.

2. ObservableCollection<T> which likewise triggers it every time.

Try this:

int count = 0;
ObservableCollection<int> col = new ObservableCollection<int>();
INotifyPropertyChanged inpc = col;
inpc.PropertyChanged += (sender, e) =>
{
    Console.WriteLine(e.PropertyName);
    ++count;
};
Console.WriteLine(count); // 0
col.Add(2);
Console.WriteLine(count); // 2 (once for [] and once for Count)
col[0] = 2;
Console.WriteLine(count); // 3
col[0] = 2;
Console.WriteLine(count); // 4

Linq2SQL on the other hand produces classes that don't call the event if it can identify it as a nop change, IIRC.

[JonHanna]

The more I think about the potential for something unusual in Equals, the more I think the implementation as is goes about as far as is safe. While a value-type Equals considering two instances equal when some observer might care about a difference is unlikely (but consider the difference between decimal.Parse("0.00") and decimal.Parse("0.000") which could impact on some rendering), not triggering the event because of an Equals() could still harm correctness when skipping it is only a bonus to efficiency.

[jnm2]

For reference types, comparing references is the most important thing. Instances have identity. I need to know, even if the values in the instances are equal, whether I'm working with the same instance or a new one.

Ideally you'd compare the data that's being set, whether it's a reference or a value. ReferenceEquals does that for reference types and Equals is the closest we can get to doing that for value types. What would be best and would work symmetrically for both ref and value is a RuntimeHelpers.Equals, just like RuntimeHelpers.GetHashCode, which calls Object.Equals non-virtually for both refs and values. This guarantees 100% that you ignore overrides, catching any possible change. I've really wanted this.

[JonHanna]

@jnm2 I was thinking similarly yesterday when I did the commit that I since withdrew (it didn't go through Runtime, but explicitly checked for ValueType) but on thinking further I'm not sure we can trust Equals() not to be overridden in a way that hides a distinction that could be important.

[JonHanna]

Ah. I only just remembered now that RuntimeHelpers.Equals() is field-by-field and ignores Equals, so it won't get caught out by weirdness there. That would be the safest bet.

[JonHanna]

Here's a concern: What if someone was holding onto the actual box in use? Perhaps consolidating common values to save memory? Such a use sees even differently-boxed copies of the same valuetype value as distinct. Would such a use be plausible? If it is then the current behaviour really is as loose as any filtering of no-op changes can be.

[jnm2]

I can't think of a scenario where an ExpandoObject should ever need to know whether or not value type objects are the same instance. If there is consolidation of boxed values, you'd want the consolidation to have no effect on the logic of the code you use them with. Conversely if the code is aware of box identity of values, you cannot start consolidating boxes without danger of changing the behavior of existing code.

Ideally you compare by instance for reference types and ignore the boxing for value types, pretending that the value was simply set generically with no boxing involved.