Various Set fixes and improvements

CHANGELOG.md

@@ -34,6 +34,7 @@
   * `Function` - proxies calls to a supplied function.
 
   Custom loggers can derive from the `Logger` class and provide their own implementation for the `Log` method or use `Function` and provide an `Action<string>`. (PR [#2276](https://github.com/realm/realm-dotnet/pull/2276))
+* `RealmObjectBase` now correctly overrides and implements `GetHashCode()`. (Issue [#1650](https://github.com/realm/realm-dotnet/issues/1650))
 
 ### Compatibility
 * Realm Studio: 10.0.0 or later.

Realm/Realm.Fody/Common/ImportedReferences.cs

@@ -57,16 +57,10 @@ internal abstract class ImportedReferences
 
         public abstract TypeReference System_Collections_Generic_ListOfT { get; }
 
-        public MethodReference System_Collections_Generic_ListOfT_Constructor { get; private set; }
-
         public abstract TypeReference System_Collections_Generic_HashSetOfT { get; }
 
-        public MethodReference System_Collections_Generic_HashSetOfT_Constructor { get; private set; }
-
         public abstract TypeReference System_Collections_Generic_DictionaryOfTKeyTValue { get; }
 
-        public MethodReference System_Collections_Generic_DictionaryOfTKeyTValue_Constructor { get; private set; }
-
         public abstract TypeReference System_Linq_Enumerable { get; }
 
         public MethodReference System_Linq_Enumerable_Empty { get; private set; }
@@ -248,12 +242,6 @@ private void InitializeFrameworkMethods()
                 Parameters = { new ParameterDefinition(new GenericInstanceType(IEnumerableOfT) { GenericArguments = { ISetOfT.GenericParameters.Single() } }) }
             };
 
-            System_Collections_Generic_ListOfT_Constructor = new MethodReference(".ctor", Types.Void, System_Collections_Generic_ListOfT) { HasThis = true };
-
-            System_Collections_Generic_HashSetOfT_Constructor = new MethodReference(".ctor", Types.Void, System_Collections_Generic_HashSetOfT) { HasThis = true };
-
-            System_Collections_Generic_DictionaryOfTKeyTValue_Constructor = new MethodReference(".ctor", Types.Void, System_Collections_Generic_DictionaryOfTKeyTValue) { HasThis = true };
-
             {
                 System_Linq_Enumerable_Empty = new MethodReference("Empty", Types.Void, System_Linq_Enumerable);
                 var T = new GenericParameter(System_Linq_Enumerable_Empty);

Realm/Realm.Fody/Common/RealmWeaver.cs

@@ -405,32 +405,19 @@ private WeavePropertyResult WeaveProperty(PropertyDefinition prop, TypeDefinitio
                 switch (collectionType)
                 {
                     case RealmCollectionType.IList:
-                        var concreteListConstructor = _references.System_Collections_Generic_ListOfT_Constructor.MakeHostInstanceGeneric(elementType);
-
-                        // weaves list getter which also sets backing to List<T>, forcing it to accept us setting it post-init
-                        if (backingField is FieldDefinition backingListField)
-                        {
-                            backingListField.Attributes &= ~FieldAttributes.InitOnly;  // without a set; auto property has this flag we must clear
-                        }
-
                         ReplaceCollectionGetter(prop, backingField, columnName,
-                                            new GenericInstanceMethod(_references.RealmObject_GetListValue) { GenericArguments = { elementType } },
-                                            concreteListConstructor);
+                                            new GenericInstanceMethod(_references.RealmObject_GetListValue) { GenericArguments = { elementType } });
                         break;
                     case RealmCollectionType.ISet:
                         _logger.Warning($"{type.Name}.{prop.Name} is of type ISet which is not officially supported yet. Some functionality may not exist yet or there may be bugs/known issues that can result in undefined behavior, including data loss. Official support for the datatype will come in a future Realm release.");
 
-                        var concreteSetConstructor = _references.System_Collections_Generic_HashSetOfT_Constructor.MakeHostInstanceGeneric(elementType);
-
-                        // weaves set getter which also sets backing to List<T>, forcing it to accept us setting it post-init
-                        if (backingField is FieldDefinition backingSetField)
+                        if (elementType.Resolve().IsEmbeddedObjectInheritor(_references))
                         {
-                            backingSetField.Attributes &= ~FieldAttributes.InitOnly;  // without a set; auto property has this flag we must clear
+                            return WeavePropertyResult.Error($"{type.Name}.{prop.Name} is a Set<EmbeddedObject> which is not supported. Embedded objects are always unique which is why List<EmbeddedObject> already has Set semantics.");
                         }
 
                         ReplaceCollectionGetter(prop, backingField, columnName,
-                                            new GenericInstanceMethod(_references.RealmObject_GetSetValue) { GenericArguments = { elementType } },
-                                            concreteSetConstructor);
+                                            new GenericInstanceMethod(_references.RealmObject_GetSetValue) { GenericArguments = { elementType } });
                         break;
                     case RealmCollectionType.IDictionary:
                         _logger.Warning($"{type.Name}.{prop.Name} is of type IDictionary which is not officially supported yet. Some functionality may not exist yet or there may be bugs/known issues that can result in undefined behavior, including data loss. Official support for the datatype will come in a future Realm release.");
@@ -441,17 +428,8 @@ private WeavePropertyResult WeaveProperty(PropertyDefinition prop, TypeDefinitio
                             return WeavePropertyResult.Error($"{type.Name}.{prop.Name} is a Dictionary<{keyType.Name}, {elementType.Name}> but only string keys are currently supported by Realm.");
                         }
 
-                        var concreteDictionaryConstructor = _references.System_Collections_Generic_DictionaryOfTKeyTValue_Constructor.MakeHostInstanceGeneric(keyType, elementType);
-
-                        // weaves set getter which also sets backing to List<T>, forcing it to accept us setting it post-init
-                        if (backingField is FieldDefinition backingDictionaryField)
-                        {
-                            backingDictionaryField.Attributes &= ~FieldAttributes.InitOnly;  // without a set; auto property has this flag we must clear
-                        }
-
                         ReplaceCollectionGetter(prop, backingField, columnName,
-                                            new GenericInstanceMethod(_references.RealmObject_GetDictionaryValue) { GenericArguments = { elementType } },
-                                            concreteDictionaryConstructor);
+                                            new GenericInstanceMethod(_references.RealmObject_GetDictionaryValue) { GenericArguments = { elementType } });
                         break;
                 }
             }
@@ -586,8 +564,13 @@ private void ReplaceGetter(PropertyDefinition prop, string columnName, MethodRef
             il.InsertBefore(start, il.Create(OpCodes.Ret));
         }
 
-        private void ReplaceCollectionGetter(PropertyDefinition prop, FieldReference backingField, string columnName, MethodReference getCollectionValueReference, MethodReference collectionConstructor)
+        private static void ReplaceCollectionGetter(PropertyDefinition prop, FieldReference backingField, string columnName, MethodReference getCollectionValueReference)
         {
+            if (backingField is FieldDefinition backingFieldDef)
+            {
+                backingFieldDef.Attributes &= ~FieldAttributes.InitOnly;  // without a set; auto property has this flag we must clear
+            }
+
             //// A synthesized property getter looks like this:
             ////   0: ldarg.0  // load the this pointer
             ////   1: ldfld <backingField>
@@ -596,10 +579,7 @@ private void ReplaceCollectionGetter(PropertyDefinition prop, FieldReference bac
             ////
             ////  if (<backingField> == null)
             ////  {
-            ////     if (IsManaged)
-            ////           <backingField> = GetCollectionValue<T>(<columnName>);
-            ////     else
-            ////           <backingField> = new Collection<T>();
+            ////     <backingField> = GetCollectionValue<T>(<columnName>);
             ////  }
             ////  // original auto-generated getter starts here
             ////  return <backingField>; // supplied by the generated getter
@@ -612,26 +592,13 @@ private void ReplaceCollectionGetter(PropertyDefinition prop, FieldReference bac
             il.InsertBefore(start, il.Create(OpCodes.Ldfld, backingField));
             il.InsertBefore(start, il.Create(OpCodes.Brtrue_S, start));
 
-            // if (IsManaged)
+            // <backingField> is null -> <backingField> = GetCollectionValue<T>(<columnName>)
             il.InsertBefore(start, il.Create(OpCodes.Ldarg_0));
-            il.InsertBefore(start, il.Create(OpCodes.Call, _references.RealmObject_get_IsManaged));
-
-            // push in the label then go relative to that - so we can forward-ref the label insert if/else blocks backwards
-            // <backingField> = new Collection<T>()
-            var unmanagedStart = il.Create(OpCodes.Ldarg_0);
-            il.InsertBefore(start, unmanagedStart);
-            il.InsertBefore(start, il.Create(OpCodes.Newobj, collectionConstructor));
+            il.InsertBefore(start, il.Create(OpCodes.Ldarg_0));
+            il.InsertBefore(start, il.Create(OpCodes.Ldstr, columnName));
+            il.InsertBefore(start, il.Create(OpCodes.Call, getCollectionValueReference));
             il.InsertBefore(start, il.Create(OpCodes.Stfld, backingField));
 
-            // if (!IsManaged) <backingField> = GetSetValue(<columnName>)
-            il.InsertBefore(unmanagedStart, il.Create(OpCodes.Brfalse_S, unmanagedStart));
-            il.InsertBefore(unmanagedStart, il.Create(OpCodes.Ldarg_0));
-            il.InsertBefore(unmanagedStart, il.Create(OpCodes.Ldarg_0));
-            il.InsertBefore(unmanagedStart, il.Create(OpCodes.Ldstr, columnName));
-            il.InsertBefore(unmanagedStart, il.Create(OpCodes.Call, getCollectionValueReference));
-            il.InsertBefore(unmanagedStart, il.Create(OpCodes.Stfld, backingField));
-            il.InsertBefore(unmanagedStart, il.Create(OpCodes.Br_S, start));
-
             // note that we do NOT insert a ret, unlike other weavers, as usual path branches and
             // FALL THROUGH to return the backing field.
 
@@ -1112,4 +1079,4 @@ i.Operand is MethodReference mRef &&
                                  mRef.ConstructsType(type));
         }
     }
-}
+}

Realm/Realm/DatabaseTypes/RealmCollectionBase.cs

@@ -203,7 +203,7 @@ protected void AddToRealmIfNecessary(in RealmValue value)
         protected static EmbeddedObject EnsureUnmanagedEmbedded(in RealmValue value)
         {
             var result = value.AsRealmObject<EmbeddedObject>();
-            if (result.IsManaged)
+            if (result?.IsManaged == true)
             {
                 throw new RealmException("Can't add to the collection an embedded object that is already managed.");
             }

Realm/Realm/DatabaseTypes/RealmDictionary.cs

@@ -53,7 +53,7 @@ public TValue this[string key]
                 EnsureKeyNotNull(key);
                 var realmValue = Operator.Convert<TValue, RealmValue>(value);
 
-                if (_isEmbedded)
+                if (_isEmbedded && realmValue.Type != RealmValueType.Null)
                 {
                     Realm.ManageEmbedded(EnsureUnmanagedEmbedded(realmValue), _dictionaryHandle.SetEmbedded(key));
                     return;
@@ -95,7 +95,7 @@ public void Add(string key, TValue value)
             EnsureKeyNotNull(key);
             var realmValue = Operator.Convert<TValue, RealmValue>(value);
 
-            if (_isEmbedded)
+            if (_isEmbedded && realmValue.Type != RealmValueType.Null)
             {
                 Realm.ManageEmbedded(EnsureUnmanagedEmbedded(realmValue), _dictionaryHandle.AddEmbedded(key));
                 return;

Realm/Realm/DatabaseTypes/RealmObjectBase.cs

@@ -43,6 +43,9 @@ namespace Realms
     [Serializable]
     public abstract class RealmObjectBase : INotifyPropertyChanged, IThreadConfined, NotificationsHelper.INotifiable, IReflectableType
     {
+        [NonSerialized, XmlIgnore]
+        private Lazy<int> _hashCode;
+
         [NonSerialized, XmlIgnore]
         private Realm _realm;
 
@@ -169,6 +172,7 @@ internal void SetOwner(Realm realm, ObjectHandle objectHandle, Metadata metadata
             _realm = realm;
             _objectHandle = objectHandle;
             _metadata = metadata;
+            _hashCode = new Lazy<int>(() => _objectHandle.GetObjKey().GetHashCode());
 
             if (_propertyChanged != null)
             {
@@ -203,23 +207,32 @@ protected void SetValueUnique(string propertyName, RealmValue val)
 
         protected internal IList<T> GetListValue<T>(string propertyName)
         {
-            Debug.Assert(IsManaged, "Object is not managed, but managed access was attempted");
+            if (!IsManaged)
+            {
+                return new List<T>();
+            }
 
             _metadata.Schema.TryFindProperty(propertyName, out var property);
             return _objectHandle.GetList<T>(_realm, _metadata.PropertyIndices[propertyName], property.ObjectType);
         }
 
         protected internal ISet<T> GetSetValue<T>(string propertyName)
         {
-            Debug.Assert(IsManaged, "Object is not managed, but managed access was attempted");
+            if (!IsManaged)
+            {
+                return new HashSet<T>(RealmSet<T>.Comparer);
+            }
 
             _metadata.Schema.TryFindProperty(propertyName, out var property);
             return _objectHandle.GetSet<T>(_realm, _metadata.PropertyIndices[propertyName], property.ObjectType);
         }
 
         protected internal IDictionary<string, TValue> GetDictionaryValue<TValue>(string propertyName)
         {
-            Debug.Assert(IsManaged, "Object is not managed, but managed access was attempted");
+            if (!IsManaged)
+            {
+                return new Dictionary<string, TValue>();
+            }
 
             _metadata.Schema.TryFindProperty(propertyName, out var property);
             return _objectHandle.GetDictionary<TValue>(_realm, _metadata.PropertyIndices[propertyName], property.ObjectType);
@@ -303,6 +316,14 @@ public override bool Equals(object obj)
             return ObjectHandle.Equals(robj.ObjectHandle);
         }
 
+        /// <inheritdoc/>
+        public override int GetHashCode()
+        {
+            // _hashCode is only set for managed objects - for unmanaged ones, we
+            // fall back to the default behavior.
+            return _hashCode?.Value ?? base.GetHashCode();
+        }
+
         /// <summary>
         /// Allows you to raise the PropertyChanged event.
         /// </summary>
@@ -453,4 +474,4 @@ public Metadata(TableKey tableKey, IRealmObjectHelper helper, IDictionary<string
             }
         }
     }
-}
+}