microsoft / microsoft-ui-xaml

Windows UI Library: the latest Windows 10 native controls and Fluent styles for your applications
MIT License
6.3k stars 676 forks source link

Microsoft.UI.Xaml.Interop.IBindableIterable and IBindableIterator should derive from IIterable<IInspectable> and IIterator<IInspectable> respectively #7891

Open sylveon opened 1 year ago

sylveon commented 1 year ago

Describe the bug

Currently, these two types do not derive from IIterable/IIterator, which makes support in projections more complex than it needs to be (need to special case these types, duplicating already existing code). Notably, it means that in cppwinrt you can't use .begin() and .end(), C++20 ranges, or a for loop over IBindableIterable because this is only implemented for IIterable. C++/WinRT is trying to avoid special casing code for third parties, so a fix on the cppwinrt side is not happening. Meanwhile CsWinRT just projects it as IEnumerable, lol.

By making the interface derive from IIterable, it means cppwinrt's collections support automatically lights up, and it can be treated as just another type by most projections (cswinrt could drop its special casing to project it as IEnumerable as well).

Steps to reproduce the bug

N/A

Expected behavior

No response

Screenshots

No response

NuGet package version

WinUI 3 - Windows App SDK 1.2 Preview 1: 1.2.0-preview1

Windows app type

Device form factor

Desktop

Windows version

Windows 11 (22H2): Build 22621

Additional context

This will probably require an ABI break.

DarranRowe commented 1 year ago

The issue here is that there is a contractual requirement that all WinRT interfaces must derive from IInspectable.

The MIDL 3.0 introduction has the following paragraph.

"An interface type defines a Windows Runtime interface, which is a named set of function members. An interface may specify that an implementation of the interface must also implement of one or more specified additional (required) interfaces. Every interface type directly derives from the Windows Runtime IInspectable interface."

Emphasis mine. This means that it is against the rules for IBindableIterable to derive from IIterator and IBindableIterable to derive from IIterable.

sylveon commented 1 year ago

Deriving was more C#/C++ parlance. If we wanted to be speak in WinRT terms, IBindableIterable should require IIterable<IInspectable>. In the end, this gets projected in both languages as deriving from IIterable<IInspectable> (or IEnumerable<object> in C#).

DarranRowe commented 1 year ago

Right, if you mean require then fine.

I mentioned this because when we use the word derive, we tend to imply inherit. The act of inheritance is valid in the domain of interface definitions, hence the documentation explicitly using the word derive. Even in IDL, an interface inherits/derives from a base interface. What's more, in the world of COM, where WinRT has its roots, interfaces could inherit from interfaces other than IUnknown (which is the base COM interface).

The use of the term derive was not that clear to me, and to my mind it implied interface inheritance and not requiring another interface.

Scottj1s commented 1 year ago

@sylveon Not sure I see the utility here, other than as a convenience for you to avoid a QI? IBindable* are meant solely for XAML binding purposes, where parametric WinRT interfaces aren't supported. So from XAML's perspective, there would be no value in a requires relationship. C#/WinRT maintains compat with the C# compiler support for UWP, which also kept these interfaces hidden from client code, by projecting to IEnumerable. If you're really just looking for begin()/end() support in C++/WinRT, maybe open a feature request there?

sylveon commented 1 year ago

This issue was specifically opened because C++/WinRT does not want to special case third party libraries (of which WinUI 3 is one): https://github.com/microsoft/CsWinRT/pull/1256#issuecomment-1306372403. C++/WinRT supports iteration through W.F.C.IIterable<T> and W.F.C.IIterator<T> only.

This issue was found in cswinrt test code, where they were iterating through IBindableVectorView in code. This worked fine before because C++/WinRT's begin() and end() functions did not impose a type check (they do now) and called into fast_iterator, which took advantage of GetAt() via duck typing. Iterating through IBindableIterable was always broken however (C++/WinRT does not add the required operator++ on IBindableIterator for iteration).

Without a requires, we can't be sure that a QI from IBindableIterable to IIterable<IInspectable> would work. If a requires is added, C++/WinRT becomes aware of it and automatically lights up all collection support (range-for loop, begin() and end() functions, C++20 ranges, etc.).

sylveon commented 1 year ago

Similarly, IBindableVector and IBindableVectorView should probably implement IVector and IVectorView in case we stop duck-typing fast_iterator, otherwise it would lead to a perf degradation.

sylveon commented 1 year ago

Bump, still a suggestion