Aspnetcore: Blazor: EditContext fieldstate tracking falls apart when model implements GetHashCode

[aaronhudon-ts]

Describe the bug

When a model implements GetHashCode, the _fieldState dictionary within EditContext continually adds to it's collection for the same field, effectively breaking the field-state logic.

To Reproduce

In my case, I used a protocol buffer to generate a simple C# class. Here's a gist: https://gist.github.com/aaronhudon-ts/6f9b079014115f539feef9b599a2c14d

• Put a breakpoint in EditContext.cs GetFieldState. Start changing values in fields. You will notice that the call to TryGetValue always fails due to the underlying GetHashCode call of the FieldIdentifier.

[pranavkm]

Thanks for the issue report @aaronhudon-ts. We'll look in to how to resolve this soon.

[pranavkm]

We might be able to address this by using https://docs.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.runtimehelpers.gethashcode?view=netframework-4.8#remarks

[aaronhudon-ts]

Thanks for the issue report @aaronhudon-ts. We'll look in to how to resolve this soon.

Thanks @pranavkm If you can suggest any workaround for the time being that does not require me to abandon protobufs as the data model, it would be most appreciated. I'm targeting a production release by the end of the month.

[pranavkm]

Does using a derived type as a model that overrides GetHashCode work? Looking at EditContext FieldIdentifier, nothing immediately jumps at me that would work as a suitable work around.

[aaronhudon-ts]

@pranavkm Ok, assuming I can override GetHashCode in the protobuf generated code (I think it's possible with partials), what would the fix be?

[pranavkm]

```C#
public override int GetHashCode() => RuntimeHelpers.GetHashCode(this);

public override bool Equals(object other) => ReferenceEquals(this, other);
```

[aaronhudon-ts]

public override int GetHashCode() => RuntimeHelpers.GetHashCode(this);

public override bool Equals(object other) => ReferenceEquals(this, other);

Thanks! I'll give it a try. Would be awesome to summon @jskeet for feedback on any potential side effects of doing this in the C# proto runtime

[jskeet]

Not sure where the proto runtime comes in here - protobuf messages are explicitly equal by value.

[aaronhudon-ts]

The generated C# for the protobuf implements GetHashCode (e.g. like below). What are the implications of changing the generated code to be like @pranavkm suggests
E.g.

` ``
public override int GetHashCode() {
int hash = 1;
if (String1.Length != 0) hash ^= String1.GetHashCode();
if (String2.Length != 0) hash ^= String2.GetHashCode();
if (Number1 != 0D) hash ^= pbc::ProtobufEqualityComparers.BitwiseDoubleEqualityComparer.GetHashCode(Number1);
if (Number2 != 0D) hash ^= pbc::ProtobufEqualityComparers.BitwiseDoubleEqualityComparer.GetHashCode(Number2);
if (_unknownFields != null) {
hash ^= _unknownFields.GetHashCode();
}
return hash;
}
```

[jskeet]

Will need to look at the whole thread again when I'm on a laptop.

[pranavkm]

@aaronhudon-ts the plan is to fix this specific issue in 3.1 patch release. With a fix in place, you shouldn't need changes to protobuf's code gen, which is absolutely fine as it is.

[rynowak]

@pranavkm - what's the root cause here? The object overrides GetHashCode and then.... ?

[pranavkm]

• EditContext uses FieldIdentifier as the key for lookups: https://github.com/aspnet/AspNetCore/blob/master/src/Components/Forms/src/EditContext.cs#L22 using the default comparer
• FieldIdentifier uses the Model instance in it's hash and equality comparison: https://github.com/aspnet/AspNetCore/blob/master/src/Components/Forms/src/FieldIdentifier.cs#L66-L80

If the model produces hashcodes based on it's property values, the lookups become unstable. The hypothesis is that we need reference equality to compare model instances.

[mrpmorris]

GetHashCode should not be calculated based on state unless it is immutable.

This isn't a Blazor issue, its fundamental to .net dictionaries.

You can override GetHashCode() for immutable reference types. In general, for mutable reference types, you should override GetHashCode() only if:

You can compute the hash code from fields that are not mutable; or

You can ensure that the hash code of a mutable object does not change while the object is contained in a collection that relies on its hash code.

[jskeet]

This is definitely not going to change in protobuf. It would be massively breaking - and a bad idea, IMO. Yes, you shouldn't mutate protos which have already been stored as keys in dictionaries - but in every other situation it's useful for protos to be easily comparable by current value (and it's useful to be able to use them as dictionary keys and not mutate them).

[mrpmorris]

This is definitely _not_ going to change in protobuf. It would be massively breaking - and a bad idea, IMO. Yes, you shouldn't mutate protos which have already been stored as keys in dictionaries - but in _every other situation_ it's useful for protos to be easily comparable by current value (and it's useful to be able to use them as dictionary keys and _not_ mutate them).

The answer is to create the Dictionary passing in a ReferenceEqualityComparer as its IEqualityComparer.

So I suppose it kind of is a Blazor issue.

[aaronhudon-ts]

This is definitely _not_ going to change in protobuf. It would be massively breaking - and a bad idea, IMO. Yes, you shouldn't mutate protos which have already been stored as keys in dictionaries - but in _every other situation_ it's useful for protos to be easily comparable by current value (and it's useful to be able to use them as dictionary keys and _not_ mutate them).

In my case, the protobuf object is being used as the "bound" data model in the Blazor Edit context, and the UI data entry is causing the protobuf's properties to change.

[mrpmorris]

@aaronhudon-ts There is a solution -> https://github.com/dotnet/aspnetcore/issues/18069#issuecomment-572218294