C#: Refactor Array & Dictionary interface implementations

modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs

@@ -14,7 +14,9 @@ namespace Godot.Collections
     /// such as <see cref="System.Array"/> or <see cref="List{T}"/>.
     /// </summary>
     public sealed class Array :
-        IList,
+        IList<object>,
+        IReadOnlyList<object>,
+        ICollection,
         IDisposable
     {
         internal godot_array.movable NativeValue;
@@ -163,12 +165,6 @@ namespace Godot.Collections
             return newArray;
         }
 
-        // IList
-
-        bool IList.IsReadOnly => false;
-
-        bool IList.IsFixedSize => false;
-
         /// <summary>
         /// Returns the object at the given <paramref name="index"/>.
         /// </summary>
@@ -194,21 +190,21 @@ namespace Godot.Collections
         /// Adds an object to the end of this <see cref="Array"/>.
         /// This is the same as <c>append</c> or <c>push_back</c> in GDScript.
         /// </summary>
-        /// <param name="value">The object to add.</param>
+        /// <param name="item">The object to add.</param>
         /// <returns>The new size after adding the object.</returns>
-        public int Add(object value)
+        public void Add(object item)
         {
-            using godot_variant variantValue = Marshaling.ConvertManagedObjectToVariant(value);
+            using godot_variant variantValue = Marshaling.ConvertManagedObjectToVariant(item);
             var self = (godot_array)NativeValue;
-            return NativeFuncs.godotsharp_array_add(ref self, variantValue);
+            _ = NativeFuncs.godotsharp_array_add(ref self, variantValue);
         }
 
         /// <summary>
         /// Checks if this <see cref="Array"/> contains the given object.
         /// </summary>
-        /// <param name="value">The item to look for.</param>
+        /// <param name="item">The item to look for.</param>
         /// <returns>Whether or not this array contains the given object.</returns>
-        public bool Contains(object value) => IndexOf(value) != -1;
+        public bool Contains(object item) => IndexOf(item) != -1;
 
         /// <summary>
         /// Erases all items from this <see cref="Array"/>.
@@ -219,11 +215,11 @@ namespace Godot.Collections
         /// Searches this <see cref="Array"/> for an object
         /// and returns its index or -1 if not found.
         /// </summary>
-        /// <param name="value">The object to search for.</param>
+        /// <param name="item">The object to search for.</param>
         /// <returns>The index of the object, or -1 if not found.</returns>
-        public int IndexOf(object value)
+        public int IndexOf(object item)
         {
-            using godot_variant variantValue = Marshaling.ConvertManagedObjectToVariant(value);
+            using godot_variant variantValue = Marshaling.ConvertManagedObjectToVariant(item);
             var self = (godot_array)NativeValue;
             return NativeFuncs.godotsharp_array_index_of(ref self, variantValue);
         }
@@ -235,27 +231,32 @@ namespace Godot.Collections
         /// Existing items will be moved to the right.
         /// </summary>
         /// <param name="index">The index to insert at.</param>
-        /// <param name="value">The object to insert.</param>
-        public void Insert(int index, object value)
+        /// <param name="item">The object to insert.</param>
+        public void Insert(int index, object item)
         {
             if (index < 0 || index > Count)
                 throw new ArgumentOutOfRangeException(nameof(index));
 
-            using godot_variant variantValue = Marshaling.ConvertManagedObjectToVariant(value);
+            using godot_variant variantValue = Marshaling.ConvertManagedObjectToVariant(item);
             var self = (godot_array)NativeValue;
             NativeFuncs.godotsharp_array_insert(ref self, index, variantValue);
         }
 
         /// <summary>
-        /// Removes the first occurrence of the specified <paramref name="value"/>
+        /// Removes the first occurrence of the specified <paramref name="item"/>
         /// from this <see cref="Array"/>.
         /// </summary>
-        /// <param name="value">The value to remove.</param>
-        public void Remove(object value)
+        /// <param name="item">The value to remove.</param>
+        public bool Remove(object item)
         {
-            int index = IndexOf(value);
+            int index = IndexOf(item);
             if (index >= 0)
+            {
                 RemoveAt(index);
+                return true;
+            }
+
+            return false;
         }
 
         /// <summary>
@@ -280,17 +281,49 @@ namespace Godot.Collections
         /// <returns>The number of elements.</returns>
         public int Count => NativeValue.DangerousSelfRef.Size;
 
-        object ICollection.SyncRoot => this;
+        public bool IsSynchronized => false;
 
-        bool ICollection.IsSynchronized => false;
+        public object SyncRoot => false;
+
+        public bool IsReadOnly => false;
 
         /// <summary>
         /// Copies the elements of this <see cref="Array"/> to the given
         /// untyped C# array, starting at the given index.
         /// </summary>
         /// <param name="array">The array to copy to.</param>
-        /// <param name="index">The index to start at.</param>
-        public void CopyTo(System.Array array, int index)
+        /// <param name="arrayIndex">The index to start at.</param>
+        public void CopyTo(object[] array, int arrayIndex)
+        {
+            if (array == null)
+                throw new ArgumentNullException(nameof(array), "Value cannot be null.");
+
+            if (arrayIndex < 0)
+            {
+                throw new ArgumentOutOfRangeException(nameof(arrayIndex),
+                    "Number was less than the array's lower bound in the first dimension.");
+            }
+
+            int count = Count;
+
+            if (array.Length < (arrayIndex + count))
+            {
+                throw new ArgumentException(
+                    "Destination array was not long enough. Check destIndex and length, and the array's lower bounds.");
+            }
+
+            unsafe
+            {
+                for (int i = 0; i < count; i++)
+                {
+                    object obj = Marshaling.ConvertVariantToManagedObject(NativeValue.DangerousSelfRef.Elements[i]);
+                    array[arrayIndex] = obj;
+                    arrayIndex++;
+                }
+            }
+        }
+
+        void ICollection.CopyTo(System.Array array, int index)
         {
             if (array == null)
                 throw new ArgumentNullException(nameof(array), "Value cannot be null.");
@@ -357,7 +390,7 @@ namespace Godot.Collections
         /// Gets an enumerator for this <see cref="Array"/>.
         /// </summary>
         /// <returns>An enumerator.</returns>
-        public IEnumerator GetEnumerator()
+        public IEnumerator<object> GetEnumerator()
         {
             int count = Count;
 
@@ -367,6 +400,8 @@ namespace Godot.Collections
             }
         }
 
+        IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
+
         /// <summary>
         /// Converts this <see cref="Array"/> to a string.
         /// </summary>
@@ -632,7 +667,7 @@ namespace Godot.Collections
         /// <param name="array">The C# array to copy to.</param>
         /// <param name="arrayIndex">The index to start at.</param>
         public void CopyTo(T[] array, int arrayIndex) =>
-            _underlyingArray.CopyToGeneric<T>(array, arrayIndex, TypeOfElements);
+            _underlyingArray.CopyToGeneric(array, arrayIndex, TypeOfElements);
 
         /// <summary>
         /// Removes the first occurrence of the specified value

modules/mono/glue/GodotSharp/GodotSharp/Core/Dictionary.cs

@@ -4,6 +4,7 @@ using System.Collections;
 using Godot.NativeInterop;
 using System.Diagnostics.CodeAnalysis;
 using System.Runtime.CompilerServices;
+using System.Linq;
 
 namespace Godot.Collections
 {
@@ -13,7 +14,9 @@ namespace Godot.Collections
     /// interfacing with the engine.
     /// </summary>
     public sealed class Dictionary :
+        IDictionary<object, object>,
         IDictionary,
+        IReadOnlyDictionary<object, object>,
         IDisposable
     {
         internal godot_dictionary.movable NativeValue;
@@ -94,7 +97,7 @@ namespace Godot.Collections
         /// <summary>
         /// Gets the collection of keys in this <see cref="Dictionary"/>.
         /// </summary>
-        public ICollection Keys
+        public ICollection<object> Keys
         {
             get
             {
@@ -108,7 +111,7 @@ namespace Godot.Collections
         /// <summary>
         /// Gets the collection of elements in this <see cref="Dictionary"/>.
         /// </summary>
-        public ICollection Values
+        public ICollection<object> Values
         {
             get
             {
@@ -119,6 +122,14 @@ namespace Godot.Collections
             }
         }
 
+        IEnumerable<object> IReadOnlyDictionary<object, object>.Keys => Keys;
+
+        IEnumerable<object> IReadOnlyDictionary<object, object>.Values => Values;
+
+        ICollection IDictionary.Keys => Keys.ToList();
+
+        ICollection IDictionary.Values => Values.ToList();
+
         private (Array keys, Array values, int count) GetKeyValuePairs()
         {
             var self = (godot_dictionary)NativeValue;
@@ -189,6 +200,9 @@ namespace Godot.Collections
             NativeFuncs.godotsharp_dictionary_add(ref self, variantKey, variantValue);
         }
 
+        void ICollection<KeyValuePair<object, object>>.Add(KeyValuePair<object, object> item)
+            => Add(item.Key, item.Value);
+
         /// <summary>
         /// Erases all items from this <see cref="Dictionary"/>.
         /// </summary>
@@ -203,28 +217,72 @@ namespace Godot.Collections
         /// </summary>
         /// <param name="key">The key to look for.</param>
         /// <returns>Whether or not this dictionary contains the given key.</returns>
-        public bool Contains(object key)
+        public bool ContainsKey(object key)
         {
             using godot_variant variantKey = Marshaling.ConvertManagedObjectToVariant(key);
             var self = (godot_dictionary)NativeValue;
             return NativeFuncs.godotsharp_dictionary_contains_key(ref self, variantKey).ToBool();
         }
 
-        /// <summary>
-        /// Gets an enumerator for this <see cref="Dictionary"/>.
-        /// </summary>
-        /// <returns>An enumerator.</returns>
-        public IDictionaryEnumerator GetEnumerator() => new DictionaryEnumerator(this);
+        public bool Contains(KeyValuePair<object, object> item)
+        {
+            using godot_variant variantKey = Marshaling.ConvertManagedObjectToVariant(item.Key);
+            var self = (godot_dictionary)NativeValue;
+            bool found = NativeFuncs.godotsharp_dictionary_try_get_value(ref self,
+                variantKey, out godot_variant retValue).ToBool();
+
+            using (retValue)
+            {
+                if (!found)
+                    return false;
+
+                using godot_variant variantValue = Marshaling.ConvertManagedObjectToVariant(item.Value);
+                return NativeFuncs.godotsharp_variant_equals(variantValue, retValue).ToBool();
+            }
+        }
+
+        bool IDictionary.Contains(object key)
+        {
+            throw new NotImplementedException();
+        }
 
         /// <summary>
         /// Removes an element from this <see cref="Dictionary"/> by key.
         /// </summary>
         /// <param name="key">The key of the element to remove.</param>
-        public void Remove(object key)
+        public bool Remove(object key)
         {
             using godot_variant variantKey = Marshaling.ConvertManagedObjectToVariant(key);
             var self = (godot_dictionary)NativeValue;
-            NativeFuncs.godotsharp_dictionary_remove_key(ref self, variantKey);
+            return NativeFuncs.godotsharp_dictionary_remove_key(ref self, variantKey).ToBool();
+        }
+
+        public bool Remove(KeyValuePair<object, object> item)
+        {
+            using godot_variant variantKey = Marshaling.ConvertManagedObjectToVariant(item.Key);
+            var self = (godot_dictionary)NativeValue;
+            bool found = NativeFuncs.godotsharp_dictionary_try_get_value(ref self,
+                variantKey, out godot_variant retValue).ToBool();
+
+            using (retValue)
+            {
+                if (!found)
+                    return false;
+
+                using godot_variant variantValue = Marshaling.ConvertManagedObjectToVariant(item.Value);
+                if (NativeFuncs.godotsharp_variant_equals(variantValue, retValue).ToBool())
+                {
+                    return NativeFuncs.godotsharp_dictionary_remove_key(
+                        ref self, variantKey).ToBool();
+                }
+
+                return false;
+            }
+        }
+
+        void IDictionary.Remove(object key)
+        {
+            _ = Remove(key);
         }
 
         // ICollection
@@ -247,38 +305,91 @@ namespace Godot.Collections
             }
         }
 
+        public bool IsReadOnly => false;
+
+        public bool TryGetValue(object key, out object value)
+        {
+            using godot_variant variantKey = Marshaling.ConvertManagedObjectToVariant(key);
+            var self = (godot_dictionary)NativeValue;
+            bool found = NativeFuncs.godotsharp_dictionary_try_get_value(ref self,
+                variantKey, out godot_variant retValue).ToBool();
+
+            using (retValue)
+            {
+                value = found ? Marshaling.ConvertVariantToManagedObject(retValue) : default;
+            }
+
+            return found;
+        }
+
         /// <summary>
-        /// Copies the elements of this <see cref="Dictionary"/> to the given
-        /// untyped C# array, starting at the given index.
+        /// Copies the elements of this <see cref="Dictionary"/> to the given untyped
+        /// <see cref="KeyValuePair{TKey, TValue}"/> array, starting at the given index.
         /// </summary>
         /// <param name="array">The array to copy to.</param>
-        /// <param name="index">The index to start at.</param>
-        public void CopyTo(System.Array array, int index)
+        /// <param name="arrayIndex">The index to start at.</param>
+        public void CopyTo(KeyValuePair<object, object>[] array, int arrayIndex)
         {
             if (array == null)
                 throw new ArgumentNullException(nameof(array), "Value cannot be null.");
 
-            if (index < 0)
-                throw new ArgumentOutOfRangeException(nameof(index),
+            if (arrayIndex < 0)
+                throw new ArgumentOutOfRangeException(nameof(arrayIndex),
                     "Number was less than the array's lower bound in the first dimension.");
 
             var (keys, values, count) = GetKeyValuePairs();
 
-            if (array.Length < (index + count))
+            if (array.Length < (arrayIndex + count))
                 throw new ArgumentException(
                     "Destination array was not long enough. Check destIndex and length, and the array's lower bounds.");
 
             for (int i = 0; i < count; i++)
             {
-                array.SetValue(new DictionaryEntry(keys[i], values[i]), index);
-                index++;
+                array[arrayIndex] = new(keys[i], values[i]);
+                arrayIndex++;
+            }
+        }
+
+        void ICollection.CopyTo(System.Array array, int arrayIndex)
+        {
+            if (array == null)
+                throw new ArgumentNullException(nameof(array), "Value cannot be null.");
+
+            if (arrayIndex < 0)
+                throw new ArgumentOutOfRangeException(nameof(arrayIndex),
+                    "Number was less than the array's lower bound in the first dimension.");
+
+            var (keys, values, count) = GetKeyValuePairs();
+
+            if (array.Length < (arrayIndex + count))
+                throw new ArgumentException(
+                    "Destination array was not long enough. Check destIndex and length, and the array's lower bounds.");
+
+            for (int i = 0; i < count; i++)
+            {
+                array.SetValue(new DictionaryEntry(keys[i], values[i]), arrayIndex);
+                arrayIndex++;
             }
         }
 
         // IEnumerable
 
+        /// <summary>
+        /// Gets an enumerator for this <see cref="Dictionary"/>.
+        /// </summary>
+        /// <returns>An enumerator.</returns>
+        public IEnumerator<KeyValuePair<object, object>> GetEnumerator()
+        {
+            for (int i = 0; i < Count; i++)
+            {
+                yield return GetKeyValuePair(i);
+            }
+        }
+
         IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
 
+        IDictionaryEnumerator IDictionary.GetEnumerator() => new DictionaryEnumerator(this);
+
         private class DictionaryEnumerator : IDictionaryEnumerator
         {
             private readonly Dictionary _dictionary;
@@ -343,6 +454,20 @@ namespace Godot.Collections
             }
         }
 
+        private KeyValuePair<object, object> GetKeyValuePair(int index)
+        {
+            var self = (godot_dictionary)NativeValue;
+            NativeFuncs.godotsharp_dictionary_key_value_pair_at(ref self, index,
+                out godot_variant key,
+                out godot_variant value);
+            using (key)
+            using (value)
+            {
+                return new KeyValuePair<object, object>(Marshaling.ConvertVariantToManagedObject(key),
+                    Marshaling.ConvertVariantToManagedObject(value));
+            }
+        }
+
         /// <summary>
         /// Converts this <see cref="Dictionary"/> to a string.
         /// </summary>
@@ -518,8 +643,9 @@ namespace Godot.Collections
             using (key)
             using (value)
             {
-                return new KeyValuePair<TKey, TValue>((TKey)Marshaling.ConvertVariantToManagedObject(key),
-                    (TValue)Marshaling.ConvertVariantToManagedObject(value));
+                return new KeyValuePair<TKey, TValue>(
+                    (TKey)Marshaling.ConvertVariantToManagedObjectOfType(key, TypeOfKeys),
+                    (TValue)Marshaling.ConvertVariantToManagedObjectOfType(value, TypeOfValues));
             }
         }
 
@@ -541,7 +667,7 @@ namespace Godot.Collections
         /// <returns>Whether or not this dictionary contains the given key.</returns>
         public bool ContainsKey(TKey key)
         {
-            return _underlyingDict.Contains(key);
+            return _underlyingDict.ContainsKey(key);
         }
 
         /// <summary>
@@ -621,21 +747,7 @@ namespace Godot.Collections
         }
 
         bool ICollection<KeyValuePair<TKey, TValue>>.Contains(KeyValuePair<TKey, TValue> item)
-        {
-            using godot_variant variantKey = Marshaling.ConvertManagedObjectToVariant(item.Key);
-            var self = (godot_dictionary)_underlyingDict.NativeValue;
-            bool found = NativeFuncs.godotsharp_dictionary_try_get_value(ref self,
-                variantKey, out godot_variant retValue).ToBool();
-
-            using (retValue)
-            {
-                if (!found)
-                    return false;
-
-                using godot_variant variantValue = Marshaling.ConvertManagedObjectToVariant(item.Value);
-                return NativeFuncs.godotsharp_variant_equals(variantValue, retValue).ToBool();
-            }
-        }
+            => _underlyingDict.Contains(new(item.Key, item.Value));
 
         /// <summary>
         /// Copies the elements of this <see cref="Dictionary{TKey, TValue}"/> to the given
@@ -666,27 +778,7 @@ namespace Godot.Collections
         }
 
         bool ICollection<KeyValuePair<TKey, TValue>>.Remove(KeyValuePair<TKey, TValue> item)
-        {
-            using godot_variant variantKey = Marshaling.ConvertManagedObjectToVariant(item.Key);
-            var self = (godot_dictionary)_underlyingDict.NativeValue;
-            bool found = NativeFuncs.godotsharp_dictionary_try_get_value(ref self,
-                variantKey, out godot_variant retValue).ToBool();
-
-            using (retValue)
-            {
-                if (!found)
-                    return false;
-
-                using godot_variant variantValue = Marshaling.ConvertManagedObjectToVariant(item.Value);
-                if (NativeFuncs.godotsharp_variant_equals(variantValue, retValue).ToBool())
-                {
-                    return NativeFuncs.godotsharp_dictionary_remove_key(
-                        ref self, variantKey).ToBool();
-                }
-
-                return false;
-            }
-        }
+            => _underlyingDict.Remove(new(item.Key, item.Value));
 
         // IEnumerable<KeyValuePair<TKey, TValue>>
 

modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/Marshaling.cs

@@ -686,7 +686,7 @@ namespace Godot.NativeInterop
                         /* capacity: */ godotDictionary.Count
                     }, null)!;
 
-                foreach (System.Collections.DictionaryEntry pair in godotDictionary)
+                foreach (KeyValuePair<object, object> pair in godotDictionary)
                     dictionary.Add(pair.Key, pair.Value);
 
                 return dictionary;

modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/VariantUtils.cs

@@ -703,8 +703,8 @@ namespace Godot.NativeInterop
 
             var res = new System.Collections.Generic.Dictionary<TKey, TValue>(godotDictionary.Count);
 
-            foreach (System.Collections.Generic.KeyValuePair<TKey, TValue> pair in godotDictionary)
-                res.Add(pair.Key, pair.Value);
+            foreach (System.Collections.Generic.KeyValuePair<object, object> pair in godotDictionary)
+                res.Add((TKey)pair.Key, (TValue)pair.Value);
 
             return res;
         }