C#: Implement IReadOnly*<> interfaces on Godot's Array<> and Dictionary<,> classes

[cgbeutler]

Describe the project you are working on

Platformer.
I have some custom resources with a dictionary of arrays within. The resource generates an inverted lookup for the collection when it is edited through the proper setters.

Describe the problem or limitation you are having in your project

I have a dictionary of arrays from which I generate an inverted lookup. That lookup is kept up-to-date when it is edited through the proper setters. With data is dependent on this collection, it would be nice to be able to expose the collection as read-only to ensure that users don't mistakenly get it out-of-sync. Currently, there is no way to expose an immutable version of a Godot collection without converting the whole thing over to a C#-standard collection.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

This feature would allow me to share out a collection that is more immutable. I could then rest easy knowing no synchronization bugs could arise.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

This would add an interfaces to Array<>

https://github.com/godotengine/godot/blob/3a41bf0e044915ca45440901e9beeea81c3d5505/modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs#L345

so it would become:

    public class Array<T> : IList<T>, IReadOnlyList<T>, ICollection<T>, IReadOnlyCollection<T>, IEnumerable<T>

Similarly Dictionary<,>

https://github.com/godotengine/godot/blob/3a41bf0e044915ca45440901e9beeea81c3d5505/modules/mono/glue/GodotSharp/GodotSharp/Core/Dictionary.cs#L367

would become:

    public class Dictionary<TKey, TValue> : IDictionary<TKey, TValue>, IReadOnlyDictionary<TKey, TValue>

The read only interfaces are mostly a reduced set of their mutable counterparts, so not much code would need to be added. I haven't looked into it too much, but IReadOnlyCollection sometimes needs a bit of code.

If this enhancement will not be used often, can it be worked around with a few lines of script?

This feature should not get in the way otherwise.

Is there a reason why this should be core and not an add-on in the asset library?

Interfaces cannot be added post-hoc.

[cgbeutler]

For those unfamiliar who would like a more detailed example, you could do something along the lines of:

    // The mutable version is kept protected. Export can still set and get it for saving/loading.
    private Dictionary<KeyCode, string> __keyBindings = new Dictionary<KeyCode, string>();
    [Export] protected Dictionary<KeyCode, string> _KeyBindings
    {
      get => __keyBindings;
      set
      {
        /* ...  Do a full set here that builds lookups or other dependent data */ 
      }
    }
    // The public version is exposed as read-only to make sure it cannot be changed
    public IReadOnlyDictionary<KeyCode, string> KeyBindings => __keyBindings;
    // Edit functions are then provided to ensure dependent data is kept up-to-date
    public void AddBinding( KeyCode code, string action )
    {
      /* ... add a new binding and update dependent data. */
    }

[raulsntos]

Implementing the IReadOnly*<> interfaces won't make them immutable, keep in mind users can just cast it to the modifiable type. I don't think this is a good idea because it might create the impression that these collections are immutable when they aren't.

Instead, I prefer the AsReadOnly pattern as implemented by System.Collections.Generic.List<T>. This method returns a System.Collections.ObjectModel.ReadOnlyCollection<T> which is a separate type that is a wrapper for a System.Collections.Generic.IList<T>. The benefits of using this wrapper is trying to access any of the mutable methods will result in throwing an exception.

For dictionaries, the equivalent is System.Collections.ObjectModel.ReadOnlyDictionary<TKey, TValue> which wraps a System.Collections.Generic.IDictionary<TKey, TValue>.

The good news is you can already implement an AsReadOnly method as an extension method like this¹:

public ReadOnlyCollection<T> AsReadOnly<T>(this Godot.Collections.Array<T> array)
{
	new ReadOnlyCollection<T>(array);
}

public ReadOnlyDictionary<TKey, TValue> AsReadOnly<TKey, TValue>(this Godot.Collections.Dictionary<TKey, TValue> dictionary)
{
	new ReadOnlyDictionary<TKey, TValue>(dictionary);
}

Since Godot's collections already implement IList<T> and IDictionary<TKey, TValue> you can just use the ReadOnlyCollection<T> and ReadOnlyDictionary<TKey, TValue>, no need to implement our own immutable wrapper.

Also, since it's a wrapper there is no conversion (which is what I'm guessing you are worried about since you don't want to use C# collections), trying to access the values of the readonly collection will just directly access the underlying collection (this also means if you change the underlying collection the changes are reflected in the readonly collection²).

What we could do is include those methods in Godot's collections but I'm not sure it's worth it since it's a one-liner, specially since .NET 7.0 will already provide those extension methods for you (dotnet/runtime#61172) all you need to do is include the System.Collections namespace.

Footnotes

1. This is exactly how the method is implemented in System.Collections.Generic.List<T>. See https://source.dot.net/#System.Private.CoreLib/List.cs,2b710ab0bc8866ad,references. ↩

2. This means you can modify the underlying collection and accessing the readonly collection will always give you the updated data, so you don't have to worry about keeping them synchronized or creating a new wrapper on every change. ↩

[cgbeutler]

TLDR; For full ReadOnly functionality, both wrappers and interfaces are useful and have their place.

Perhaps I shouldn't have used the term immutable. The interface IReadOnlyList<> and others do not guarantee immutability (and never have.) The regular System.Collections.Generic.List<> is declared with IReadOnlyList<> and IReadOnlyCollection<> in order to express that it can conform to a read-only use.

The main reason the interfaces exist is to allow better contract expression by allowing methods to declare that they are not going to change your collection. By marking the input parameter type as IReadOnlyList<>, you signify a promise or contract (much like a const method.) I am a big fan of contract-style programming.

So I guess the main question is:
Should a method that takes an IReadOnlyList<string> be able to take a Godot.Array<string>? I would think the answer should be 'yes'.

My example in my previous comment uses the interface to express a return contract. Returned contracts are a bit more controversial, it's true. When working alone or with a small team they can be safe enough while keeping things simple. (I mean, if one of your team mates casts an IReadOnly to a regular collection, pretty sure you should fire that teammate.)
Regardless of how you feel about using it as a return-contract, that doesn't invalidate it's use as an input-parameter type.

[cgbeutler]

I ran into this again when trying to write an extension method.

I wanted to write:

    public static TVal GetOrDefault<TKey, TVal>( this IReadOnlyDictionary<TKey, TVal> d, TKey key, TVal @default )
      => d.TryGetValue( key, out var value ) ? value : @default;

Which then covers 12 standard dictionary types, but not Godot dictionaries. Had to duplicate the extension method just for the Godot Dict.

[FyiurAmron]

I second what @cgbeutler said. Yes,

Implementing the IReadOnly*<> interfaces won't make them immutable

but that was never the idea nor point here. It's actually

Implementing the IReadOnly*<> interfaces AND accessing the object via one of those interfaces MAKES them immutable in the consumer context

That is the point of this proposed change.

And yes, both the logical precedent of System.Collections.Generic.List<> etc. and the logical answer (i.e. 'yes' :D) to the

Should a method that takes an IReadOnlyList be able to take a Godot.Array?

are for me, by themselves, enough to implement those interfaces here (not to mention the contract side of the problem highlighted above, the convenience of having those interfaces implemented, and the fact that there are no real downsides in doing so).

(sorry for the repost, previously I used my company account instead of personal one by mistake :D)

[raulsntos]

This was implemented in 4.0 by godotengine/godot#64089