-
Notifications
You must be signed in to change notification settings - Fork 280
Allow methods returning IIterable<T> to be used as synchronous generators#361
Allow methods returning IIterable<T> to be used as synchronous generators #361Fulgen301 wants to merge 10 commits intomicrosoft:master from
Conversation
dunhor
commented
Sep 19, 2023
IIterable<T> is expected to be multi-pass. Please correct me if I'm wrong, but wouldn't this break if you tried to get more than one IIterator<T> out of it?
sylveon
commented
Sep 19, 2023
Yes, but C# already violates that expectation with its own generator methods.
dunhor
commented
Sep 19, 2023
That seems... bad. But I'll defer to @jonwis on this one
dunhor
commented
Sep 19, 2023
I realize that this problem is made worse by the fact that APIs tend to accept an IIterable<T> and rarely an IIterator<T>, even if they don't need multiple passes over the data. My primary concern is that code can pretty easily silently break by new versions of Windows (or other frameworks like WinAppSDK) because an update is made that enumerates more than once. I don't think that's likely to happen often, but it seems easy enough to happen sooner or later.
jonwis
commented
Sep 19, 2023
I realize that this problem is made worse by the fact that APIs tend to accept an
IIterable<T>and rarely anIIterator<T>, even if they don't need multiple passes over the data. My primary concern is that code can pretty easily silently break by new versions of Windows (or other frameworks like WinAppSDK) because an update is made that enumerates more than once. I don't think that's likely to happen often, but it seems easy enough to happen sooner or later.
Agree on this; IIterable<T>::First should be returning a fresh new iterator that can be walked over again. It's usually rare, as most implementation code takes the iterable, obtains an iterator, and then pulls values out of that into a vector.
But, it would be a very sharp edge if the IIterable<T> this produces is one-shot.
My mistake, the C# iterator methods do support being iterated multiple times, as long as they return IEnumerable<T> (IEnumerator<T> does not, however). C# achieves this by storing the parameters that where passed to the function and rerunning the function from the start when enumerating it a second time, as if you had called the method a second time with the same parameters (which sounds like a potential footgun if any input reference type is modified). C++'s coroutines do not have the required level of introspection to achieve this, they are indeed one-shot.
We could constrain this implementation to IIterator<T> (which is one-shot, as it can only advance forwards and cannot be reset) only but unfortunately this greatly limits its usefulness (can't easily iterate, no API directly taking IIterator<T>).
nit: MoveNext should throw hresult_out_of_bounds if it is called but the state is already Done
`wil::make_iterable_from_iterator` to create an `IIterable<T>` helper object C++ coroutines can only be evaluated once, which doesn't fulfill `IIterable<T>`'s requirement that it can return multiple independent iterators. Thus the generator implementation is constrained to `IIterator<T>`, which may only be evaluated once, and a helper method is added that returns an `IIterable<T>` which creates a new generator instance every time an iterator is requested.
Fulgen301
commented
Sep 20, 2023
I've now changed the implementation so that the generator is constrained onto IIterator<T>. To get an IIterable<T>, one needs to use a new helper method, wil::make_iterable_from_iterator, which takes the address of a function returning an IIterator<T> and invokes it for every call to First():
IIterator<winrt::hstring> Generator(int x) { co_yield winrt::to_hstring(x); } void Foo() { IIterable<winrt::hstring> iterable = wil::make_iterable_from_iterator(&Generator, 3); }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest adding a way to keep a strong or weak reference to a class here, as well as a pointer. Maybe something similar to C++/WinRT's delegate implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Func accepts those arguments, it's already possible (e.g. calling a member function with a strong reference that is stored in the IIterable<T> helper with wil::make_iterable_from_iterator(&Foo::Bar, get_strong()). That doesn't work with weak references, but what would the semantics be if the weak pointer's object has been destroyed? Should First return an empty iterator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A member function pointer won't take a strong reference as argument directly via std::invoke/std::apply.
Good point reguarding weak references, lets just ignore that for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah looks like std::invoke will try (*arg).(*f)(), I did not know that. Nothing to do here, then.
dunhor
commented
Oct 20, 2023
Mostly looks good to me, however it's been far too long since I've dealt with coroutines in C++. @oldnewthing is probably the most resident coroutine expert if he has any input
oldnewthing
commented
Oct 20, 2023
I think this is solving the problem at the wrong level. I think we should add a basic wil::generator to wil/coroutine.h for general-purpose generation, and then the C++/WinRT version can be a wrapper around that.
sylveon
commented
Oct 20, 2023
Why not use std::generator then
Because it requires C++23, not C++17/20 like the coroutine infrastructure, and has yet to be implemented by the STL.
It also does not provide a way to yield multiple items in sequence without suspending, which I have taken advantage of so that GetMany() produces the requested items without unnecessarily suspending the coroutine just to immediately resume it again. (i have benchmarked this and it improved execution times.)
Co-authored-by: Duncan Horn <40036384+dunhor@users.noreply.github.com>
Fulgen301
commented
Nov 2, 2023
Given the one approval, should I still follow the suggestion of adding wil::generator instead?
oldnewthing
commented
Nov 2, 2023
I'm not sure what the use case is for returning IIterable<T> instead of a C++ generator. Is this so that you can pass a custom iterator to Windows Runtime methods? Can you give an example of when somebody would actually want this?
sylveon
commented
Nov 2, 2023
Either that or implement a WinRT interface which returns IIterable via a generator
Fulgen301
commented
Nov 2, 2023
The use cases for this feature are the same ones as in C# - as IIterable<T> is mapped to IEnumerable<T> and IIterator<T> to IEnumerator<T>, it is possible to implement Windows Runtime methods returning IIterable<T> and IIterator<T> using yield statements. This PR adds the same functionality to C++, but as C++ coroutines cannot be reused multiple times - with IEnumerable<T>, you can just ask it for a new enumerator - the actual generator was moved to methods returning IIterator<T>, which cannot be reused to iterate from the beginning anyway. This PR also provides a helper method and a helper object implementing IIterable<T> which creates a new generator every time an IIterator<T> is requested, as most of the Windows Runtime passes around IIterable<T> as opposed to IIterator<T>.
Just like in C#, this can be used any time someone wants to use a generator to implement a collection; right now, the choices are implementing a custom iterator or prefilling and returning a collection of which all elements may not even bee needed, both of which aren't as ergonomic as generators.
oldnewthing
commented
Nov 2, 2023
Yes, I understand how this could be used in theory. But are there practical examples of cases in which you want to pass a generator to a Windows Runtime method? I'm hoping to see a compelling scenario like
struct watcher_desires { bool want_adds; bool want_removes; bool want_updates; }; auto trigger = DeviceWatcher::GetBackgroundTrigger(make_iterable_from_generator( [=](auto desires) -> IIterator<DeviceWatcherEventKind> { if (desires.want_adds) co_yield DeviceWatcherEventKind::Add; if (desires.want_removes) co_yield DeviceWatcherEventKind::Remove; if (desires.want_updates) co_yield DeviceWatcherEventKind::Update; }, desires));
See #349.
This PR adds a synchronous generator implementation for methods that return
IIterable<T>.The generator promise implements both
IIterator<T>andIIterable<T>and therefore saves an allocation; however, this can be observed viaQueryInterface, so I'm not too sure whether this is a good idea in practice.