For Avalonia.Controls.ListBox for Items Property, Support for IList<T> without non-generic IList

[ShadowMarker789]

Describe the bug
For a DataContext View-Model that has Specialized Collections, collections may implement the IList<T> or IReadOnlyList<T> interfaces, neither of which implements non-generic IList interface.

Any such collections when they also implement INotifyCollectionChanged cause assignment of the DataContext involved throw an exception stating, correctly, that the collection in question does not implement IList, which is correct - it does not.

Thrown here

My expectations were that ListBoxes would work with generic collections, not only collections that implement IList

Related PR

To Reproduce
Steps to reproduce the behavior:

1.) Create a ViewModel that implements INPC, create a collection that implements IList<T> or IReadOnlyList<T> and INotifyCollectionChanged but does not implement IList
2.) Assign the collection as the DataContext of a Window or view such that you have a ListBox that has its Items bound to the collection.
3.) Observe the argument exception in question.

Expected behavior
The expectation is that ListBox classes should be able to display the items of both IList<T> and IReadOnlyList<T> and update their views as the collections change so long as they implement INotifyCollectionChanged

Screenshots
Not really relevant to this.

Desktop (please complete the following information):

Also not really relevant to this either.

Additional context
Could ItemsSourceView be modified around Line 49 to check before throwing an exception if Source is IReadOnlyList<object>? In my testing _qc2 is IReadOnlyList<object> returned true for instances of a class definition internal class QuickCollectionWithoutIList : IReadOnlyList<QuickModel>, INotifyCollectionChanged

But that's incompatible with the non-generic IList interface and can't be cast to IList properly. What a pain! And the Inner property of ItemsSourceView needs filling.

I feel like this is perhaps stepping onto an earlier pain-point of the .Net BCL similar to the generic IEnumerable and the non-generic IEnumerable. 🙃

I suppose you could if you wanted to you could do some interesting castings, but I don't know if there are any IList implementing classes that don't also implement IReadOnlyList<T> - and all IReadOnlyList<T> are castable to IReadOnlyList<object> because of the rules of Covariance. But after checking the IList documentation there's plenty of types out there that implement IList but don't implement IReadOnlyList - what a pain!

Would it be too much to have some sort of interface of an indexing provider, that could have implementations that use either IList as a parameter, or IReadOnlyList<T>, and all it needs to do is provide a method of indexing the lists?

[ShadowMarker789]

I have added an example reproduction, with one INPC view-model, one collection with IList that works, and one IReadOnlyCollection that does not function.

This example app asynchronously populates a list, and then removes items from the list updating the UI as it goes. It then slowly clears the IList and then tries to reattempt with the IReadOnlyList - which fails with the stack-trace appearing on-screen instead.

[maxkatz6]

INotifyCollectionChanged doesn't make sense together with IReadOnlyList semantically.
You can't change a readonly list.

[timunie]

INotifyCollectionChanged doesn't make sense together with IReadOnlyList semantically. You can't change a readonly list.

How can there be a ReadOnlyObservableCollection then? I think you can change it privately but not public. For the user of the lib, it would be readonly in that case. Or am I wrong?

https://docs.microsoft.com/en-us/dotnet/api/system.collections.objectmodel.readonlyobservablecollection-1?view=net-6.0

[ShadowMarker789]

INotifyCollectionChanged doesn't make sense together with IReadOnlyList semantically. You can't change a readonly list.

Yes, but it also fails with IList<T>. It only works with IList, which is a different and incompatible interface.

My target interface in mind was IReadOnlyList<T> because in my mind a ListBox is unlike a DataGrid in that there's not the expectation of the control having inbuilt functionality to delete items from the collection. IList<T> extends IReadOnlyList<T> - so if we can get ListBox to work for IReadOnlyList<T> then it should also work for IList<T>

[timunie]

I had a look in M$ - source code, how they implemented their ReadOnlyCollection and yeah, looks like they also implement IList for some reason. They just throw exceptions when the user wants to add something, see: https://github.com/microsoft/referencesource/blob/master/mscorlib/system/collections/objectmodel/readonlycollection.cs

This is just for information. If Avalonia finds a way to ship around that issue, even better. I'm just suprised they designed stuff like that.

Happy coding
Tim

[ShadowMarker789]

I had a look in M$ - source code, how they implemented their ReadOnlyCollection and yeah, looks like they also implement IList for some reason. They just throw exceptions when the user wants to add something, see: https://github.com/microsoft/referencesource/blob/master/mscorlib/system/collections/objectmodel/readonlycollection.cs

This is just for information. If Avalonia finds a way to ship around that issue, even better. I'm just suprised they designed stuff like that.

Happy coding Tim

I did have some early success in modifying the way the underlying provider indexes lists, by adding a non-IList interface, and then having multiple classes that implement that non-IList interface - one that takes in an IList, and another that takes in an IReadOnlyList<object> - which works because all IReadOnlyList<T> are castable to IReadOnlyList<object> because of the rules of Covariance.

I may tidy that up later, and perhaps issue a PR as a fix, but @maxkatz6

INotifyCollectionChanged doesn't make sense together with IReadOnlyList semantically. You can't change a readonly list.

Are you certain that you do not want ListBox to work with List types that do not implement IList (non-generic list) specifically? IList<T> does not extend IList - so you can have lists that are read-write but do not implement IList and the current implementation will never work with those kinds of lists if they also implement INotifyCollectionChanged.

[jp2masa]

which works because all IReadOnlyList<T> are castable to IReadOnlyList<object> because of the rules of Covariance.

Not totally true, if T is a struct I think it won't work.

[ShadowMarker789]

which works because all IReadOnlyList<T> are castable to IReadOnlyList<object> because of the rules of Covariance.

Not totally true, if T is a struct I think it won't work.

That is a really good pickup.

I can confirm that it does not function for T is struct

List<String> stringList = new List<string>(); // start declaration
//List<object> objList = (List<object>)stringList; // does not compile
//IList<object> iObjList = (IList<object>)stringList; // invalid cast exception at runtime
IReadOnlyList<object> iReadOnlyObjList = (IReadOnlyList<Object>) stringList; // valid at both compile and run

List<Guid> guidList = new List<Guid>();// another declaration
//List<object> objList = (List<object>)guidList; // does not compile
//IReadOnlyList<object> objList = (IReadOnlyList<object>)guidList; // Invalid Cast Exception at runtime
//IReadOnlyList<ValueType> valueList = (IReadOnlyList<ValueType>)guidList; // Invalid Cast Exception at runtime
Span<Guid> spanValue = CollectionsMarshal.AsSpan(guidList); // kind of useless because it requires that we know what T is, and requires something that inherits from List<T> 

Which absolutely sinks my idea of how to solve this.

Diving further into this, I'd like to thank Windows10CE#8553 on Discord for sharing their LabSharp experiments.

With Code:

using SharpLab.Runtime;

[JitGeneric(typeof(object))]
[JitGeneric(typeof(string))]
[JitGeneric(typeof(System.ValueType))]
[JitGeneric(typeof(System.Guid))]
public class MyFakeList<T> : MyFakeInterface<T>
{
    public T Get()
    {
        return default;
    }
}

public interface MyFakeInterface<out T>
{
    T Get();
}

Has Assembly

; Core CLR 6.0.722.32202 on x86

MyFakeList`1[[System.__Canon, System.Private.CoreLib]]..ctor()
    L0000: ret

MyFakeList`1[[System.__Canon, System.Private.CoreLib]].Get()
    L0000: xor eax, eax
    L0002: ret

MyFakeList`1[[System.__Canon, System.Private.CoreLib]]..ctor()
    L0000: ret

MyFakeList`1[[System.__Canon, System.Private.CoreLib]].Get()
    L0000: xor eax, eax
    L0002: ret

MyFakeList`1[[System.__Canon, System.Private.CoreLib]]..ctor()
    L0000: ret

MyFakeList`1[[System.__Canon, System.Private.CoreLib]].Get()
    L0000: xor eax, eax
    L0002: ret

MyFakeList`1[[System.Guid, System.Private.CoreLib]]..ctor()
    L0000: ret

MyFakeList`1[[System.Guid, System.Private.CoreLib]].Get()
    L0000: vzeroupper
    L0003: vxorps xmm0, xmm0, xmm0
    L0007: vmovdqu [edx], xmm0
    L000b: ret

And explains further

all the reference type ones are the same, whether it be object, string, or ValueType
Guid has a different implementation
at runtime, if you were allowed to cast from a List to a IReadOnlyList, it would have to silently switch out the implementations for the methods
it doesn't have to do that when going from one class to another

I can't think of a great solution for making this work with structs as well as classes.

If no-one can think of any solutions to this, I'll close the issue accepting 'By Design' and this closed issue can remain to serve as documentation that helps explain this behaviour and the IList (non-generic) requirement.

[TomEdwardsEnscape]

This task can be achieved by creating wrapper classes that looks a bit like this:

class ReadOnlyListWrapper<T> : IList, INotifyCollectionChanged
{
    public ReadOnlyListWrapper(IReadOnlyList<T> source)
    {
         // store source as a field
    }

    // IList and INotifyCollectionChanged implementations that pass through to source
}

There would be a second version for IList<T>.

Either users can create such an object themselves, or Avalonia can use reflection to create one for its internal use when appropriate.

[ShadowMarker789]

This task can be achieved by creating wrapper classes that looks a bit like this:

class ReadOnlyListWrapper<T> : IList, INotifyCollectionChanged
{
    public ReadOnlyListWrapper(IReadOnlyList<T> source)
    {
         // store source as a field
    }

    // IList and INotifyCollectionChanged implementations that pass through to source
}

There would be a second version for IList<T>.

Either users can create such an object themselves, or Avalonia can use reflection to create one for its internal use when appropriate.

I was thinking of something like that - but for IList<T> without IList is problematic where T : struct. I don't have a performant solution to making that implementation, and this is starting to become larger in scope than I had originally thought it could be when investigating this originally.

[TomEdwardsEnscape]

What exactly is problematic? There is no casting of generic types going on here.

Do you mean performance penalty of boxing of struct to object for IList?

[ShadowMarker789]

What exactly is problematic? There is no casting of generic types going on here.

Do you mean performance penalty of boxing of struct to object for IList?

I meant the reflection-y check for the generic of IList<T> and/or IReadOnlyList<T>

Turns out I was mistaken! IList<T> does NOT extend IReadOnlyList<T> !

But even so it's very possible for a collection to be supporting both IList<T> and INotifyCollectionChanged without implementing IList (non-generic)

In those circumstances, the ListBox will fail to bind and throw an unhandled exception.

Leveraging @tomenscape 's idea, I have the following, which works, but I dislike its use of reflection to construct the required object(s).

private IList ProbeIEnumerableForAppropriateIndexer(IEnumerable source)
{
	if (source is IList<object> castSource)
	{
		return new GenericListWrapper<object>(castSource);
	}
	if (source is IReadOnlyList<object> castSourceReadOnly)
	{
		return new ReadOnlyListWrapper<object>(castSourceReadOnly);
	}

	// It is not an IReadOnlyList<object> - but it could be an IReadOnlyList<T> where T : struct
	// Reflection use here, will be slower, but the ideal scenarios have already been covered 
	// This is for an edge-case, if they don't want this to be slow, they should implement IList, or IReadOnlyList of an object 
	// Is there a way to warn in Avalonia Diagnostics of a slower compat-code-path ? 

	var type = source.GetType();
	var interfaces = type.GetInterfaces();
	for (int i = 0; i < interfaces.Length; i++)
	{
		var @interface = interfaces[i];
		if (@interface is null)
			continue;
		// null check satisifed 

		if (@interface.GenericTypeArguments.Length == 1 && @interface.GetGenericTypeDefinition() == typeof(IList<>))
		{
			// found it. It is in fact an IList<T> where T : struct 
			try
			{
				var listWrapperType = typeof(GenericListWrapper<>);
				var instance = Activator.CreateInstance(listWrapperType.MakeGenericType(@interface.GenericTypeArguments.Single()), source);
				if (instance is not null)
					return (IList)instance;
			}
			catch (Exception ex)
			{
				// TODO: Log cast failure here
			}
		}

		if (@interface.GenericTypeArguments.Length == 1 && @interface.GetGenericTypeDefinition() == typeof(IReadOnlyList<>))
		{
			// found it. It is in fact an IReadOnlyList<T> where T : struct 
			try
			{
				var readOnlyListWrapperType = typeof(ReadOnlyListWrapper<>);
				var instance = Activator.CreateInstance(readOnlyListWrapperType.MakeGenericType(@interface.GenericTypeArguments.Single()), source);
				if(instance is not null)
					return (IList)instance;
			}
			catch(Exception ex)
			{
				// TODO: Log cast failure here
			}
		}
	}
	// alternative code:
	// if (gs.GetType().GetInterfaces().Any(x => x.GenericTypeArguments.Length == 1 && x.GetGenericTypeDefinition() == typeof(IReadOnlyList<>))) { /* ... */ }
	
	// could check other specialized collection types here too 
	return new List<object>(source.Cast<object>());
}

This code needs cleaning up, but it does the job.

What are your opinions on this?

[TomEdwardsEnscape]

My thoughts:

1. The last line of the method takes a copy of the collection, which we 100% do not want to do. The method should return the input IEnumerable in this case.
2. source is IList<object> will only match IList<object>, since that interface is not covariant (unlike IReadOnlyList<T>).
3. I'd suggest source.GetType().GetInterface(typeof(IList<>).Name) instead of the foreach loops.
4. Assigning new values to ItemsSource is not a frequent operation, so I would not be worried about performance.

[timunie]

Avalonia currently tries to reduce reflection as it has performance and trimming issues. I don't know if that rare case is worth that much effort. Just my 2 cents.

[ShadowMarker789]

My thoughts:

1. The last line of the method takes a copy of the collection, which we 100% do not want to do. The method should return the input IEnumerable in this case.
2. source is IList<object> will only match IList<object>, since that interface is not covariant (unlike IReadOnlyList<T>).
3. I'd suggest source.GetType().GetInterface(typeof(IList<>).Name) instead of the foreach loops.
4. Assigning new values to ItemsSource is not a frequent operation, so I would not be worried about performance.

That line is identical to Line 53 - but perhaps you're right and it should be Line 52 instead - creating a new List which implements IList and is just a container for the enumerable.

Agreed, we don't want to copy the items of the IEnumerable, Line 52 is preferred to Line 53.

Gah, you're right about the IList<T> not being IList<object> for other Ts. I naively thought they were covariant like IReadOnlyList<T>.

I thought since we're testing for multiple interfaces here that the loop would be preferred to the GetInterface by string name form for performance reasons, but maybe I'm prematurely optimizing again.

Avalonia currently tries to reduce reflection as it has performance and trimming issues. I don't know if that rare case is worth that much effort. Just my 2 cents.

Right, I agree with that, but if we're hitting that code section then we've already exhausted all of the high performing options - no IList (non-generic) and does not implement IReadOnlyList<T> where T : class

[Gillibald]

We can't accept code that relies on reflection in the main code base

[ShadowMarker789]

We can't accept code that relies on reflection in the main code base

More nails in the coffin of this approach.

Everything past the is and as clauses can be dropped. I am definitely seeing why working with collection types that do not implement non-generic IList can be a pain.

[TomEdwardsEnscape]

This leaves the following options, IMO:

1. Provide wrapper types to the user and let them construct them manually before assignment.
2. Improve the error message to suggest implementing IList or using the wrapper types.
3. Consider supporting INotifyCollectionChanged + IEnumerable by re-enumerating after changes. I believe this is what WPF does. It's slow, of course; a "collection dirty" flag could be used to rate limit this to once per UI frame instead of responding to each event.

Separately, both of the lines in ItemsSourceView which copy the source collection are dodgy. This is a view of a mutable object, not a copy!

[ShadowMarker789]

This leaves the following options, IMO:

1. Provide wrapper types to the user and let them construct them manually before assignment.
2. Improve the error message to suggest implementing IList or using the wrapper types.
3. Consider supporting INotifyCollectionChanged + IEnumerable by re-enumerating after changes. I believe this is what WPF does. It's slow, of course; a "collection dirty" flag could be used to rate limit this to once per UI frame instead of responding to each event.

Separately, both of the lines in ItemsSourceView which copy the source collection are dodgy. This is a view of a mutable object, not a copy!

What could be changed regarding L52 and L53 to get an IList (non-generic) implementation out of an IEnumerable (non-generic) ? Is it inappropriate to create a new List<object> out of an IEnumerable ? My interpretation of an IEnumerable that doesn't implement INotifyCollectionChanged would be that it's not going to be mutated, but is that a wrong interpretation?

More stuff to think about, I may try out some things later tonight when I get home from work.

[timunie]

I don't think IEnumerable can be cast to IList in any case. The enumeration may be infinite, while IList may not.

[TomEdwardsEnscape]

A user may want to display the contents of a simple enumerable and then refresh the UI manually when they know/judge that it has changed. But since a copy was taken, this won't work.

The case of an infinite enumerable is a pain, though you could throw an exception after enumerating int32.MaxValue times. I suspect that the List<T> constructors currently in use do this already.

[MalzSmith]

This leaves the following options, IMO:

1. Provide wrapper types to the user and let them construct them manually before assignment.

2. Improve the error message to suggest implementing `IList` or using the wrapper types.

3. Consider supporting `INotifyCollectionChanged` + `IEnumerable` by re-enumerating after changes. I believe this is what WPF does. It's slow, of course; a "collection dirty" flag could be used to rate limit this to once per UI frame instead of responding to each event.

After running into this issue today I'd like to express my support for the third option. Currently the behavior when using INCC + IEnumerable is a bit difficult to troubleshoot.

View code behind:

ViewModel.WhenAnyValue(vm => vm.ResultsBuffer)
    .BindTo(this, v => v.ListBox.ItemsSource)
    .DisposeWith(d);

If ResultsBuffer is INCC+IEnumerable but not IList, this code doesn't throw an exception. Doing the same binding in axaml doesn't throw an exception either. According to the debugger the ItemsSource property is set during runtime, but the Items collection just simply never updates.

If I change my code to this:

ViewModel.WhenAnyValue(vm => vm.ResultsBuffer)
    .Subscribe(buffer => ListBox.ItemsSource = buffer)
    .DisposeWith(d);

I get an exception that tells me that my collection needs to implement IList. But normally I'd expect there to be no difference between the two versions.