Fix typing and a few bugs

[mikenerone]

I needed type annotations for Slurry to make the type checks in my own project happy. I'd been using my own type stubs for a couple of days, but thought I'd contribute back.

These typing fixes include a few that were tripping up my thing plus everything reported by Pyright in standard mode (only with slurry/, not the tests). A few of the issues noticed by Pyright were actually bugs, so I took the liberty of fixing those, as well. See commit messages for more info.

A couple of things I didn't do, but I'd be happy to if you're open to it. They're actually kinda related and I'd like to do both:

1. I spotted a (rare) bug that Pyright didn't explicitly warn about: aclosing() is used in many places on async iterators, but not all async iterators have an aclose() method. This will error if someone passes a source that doesn't. This is admittedly unusual because they're almost always from generator functions or comprehensions (and more importantly this is the case for memory receive channels, which covers 99% of the time), but it's a non-zero chance kind of problem. I would fix it by creating a safe_aclosing() context manager to replace aclosing(). It would have no dependency on aclosing(), which has the side benefit that the async_generator package dependency would no longer be needed at all. That would be good because... - Edit: ended up fixing in later commits
2. Fix all typing issues reported by Pyright in strict mode. There are many errors right now, mostly coming from the fact that the async_generator package is untyped (and there are no type stubs available for it). It's only used for aclosing(), which is just a few lines anyway. - Edit: ended up fixing in later commits

Edit: Oh, and another rare bug I didn't fix:
3. sources are (async-)iterated directly in many places, but they typed as iterables, not iterators. This isn't usually a problem because most iterables act as their own iterators, but not all. Iteration (with or without aclose() wrapping it) will error in those cases. If the typing claims AsyncIterable, then any AsyncIterable should be supported. - Edit: ended up fixing in later commits

Btw, I suspect that fixing the lack of typing on aclose() mentioned in item 2 would result in Pyright properly warning about both of the bugs. In any case, they're not regressions, and as I said, quite rare to strike anyway. IMO they don't need to hold up a release or anything.

[mikenerone]

I force-pushed to add tests to each of the bugfix commits. The one about some iterators not having an aclose() actually required fixing both of the bugs I discussed in the description, specifically for Map, because Map is used in the weld tests, so I've added that safe_aclosing() context manager to a new _util module (let me know if you'd like that renamed - I couldn't think of anything). Map now serves as an example of its use.

Please let me know if you're comfortable with this safe_aclosing(), and I can completely replace aclosing() with it in this same PR.

[andersea]

Overall looks great. I have some questions though.

You added import 'as' to all from x import y. I am not up to date on what this achieves. Can you provide some source for this?

I don't think I am completely sold on the safe_aclosing concept. One of the main pain points I had with async iterators throughout my projects was the unpredictability of them, if you didn't force them to explicitly close, so it is just something I got into the habit of forcing myself to think about. And I guess my worry is, if someone is really creating manual async iterators that don't support aclose, are these people sure that they actually know what they are doing? Do I want to take that chance, or am I fine with my code just failing in this case? The patch adds a not insignificant amount of extra code, for a corner case that we might not ever run into.

[mikenerone]

You added import 'as' to all from x import y. I am not up to date on what this achieves. Can you provide some source for this?

@andersea This "redundant import" form signals type-checkers that these symbols are intended as public interface (i.e. it's ok to import them from this module even though that's not where they were originally defined). Without this, type checkers will complain about imports from here (can confirm that Pyright and MyPy both do). See https://typing.readthedocs.io/en/latest/source/libraries.html#library-interface-public-and-private-symbols (the alternative is to define an __all__, but the redundant-imports technique seems generally preferred because __all__ encourages * imports, and nobody likes those :P).

[mikenerone]

I don't think I am completely sold on the safe_aclosing concept. One of the main pain points I had with async iterators throughout my projects was the unpredictability of them, if you didn't force them to explicitly close, so it is just something I got into the habit of forcing myself to think about. And I guess my worry is, if someone is really creating manual async iterators that don't support aclose, are these people sure that they actually know what they are doing? Do I want to take that chance, or am I fine with my code just failing in this case? The patch adds a not insignificant amount of extra code, for a corner case that we might not ever run into.

@andersea That's an understandable concern - in fact I agree, as I've run into similar issues myself in the past. The problem is that as it is, all of the uses of aclosing() on objects that may not have aclose() cause type-checking failures, which is increasingly impactful on developers trying to actually use Slurry, as type-checking has started to become the norm in the Python community. Indeed, the impact on me in my own project is what prompted me to create this PR.

The alternative is to change the type hints instead: rather than having all of the inputs annotated as AsyncIterable[Any], to make them guarantee that aclose() is present, we need to annotate them as a new protocol, AsyncIterableWithAcloseableIterator, with an __aiter__() returning an AcloseableIterator. I've pushed another commit so you can see what that looks like (along with some knock-on changes - see the commit message). This works fine, but it has an unfortunate impact: people generally annotate async generators as typing.AsyncGenerator[Something, None]. That protocol's __aiter__() definition is typed as returning a plain old typing.AsyncIterator, which now fails a type-check against AcloseableIterator (and same problem when they annotate something as typing.Iterable[Something]).

Of course, none of this is our fault. This was a miss when the language maintainers defined the async iteration protocol, but we're stuck with the consequences. Users could import the new protocol from Slurry to work around the above problem, but IMO, requiring people to break the common convention if they want proper type-checkability is pretty onerous and a bit too opinionated. I would suggest letting me remove those new protocols and switch back to the standard ones. Instead we can just document the need for aclose() support, and in the rare case that someone makes a bad class, they'll get an AttributeError on aclose, but that's on them. Basically, back to the original state regarding this issue (except with the necessary cast() calls to make type checkers happy + at least it's documented + the bug fixes that fell out of the properly typed aclosing()).

[andersea]

I am sorry. I tried looking through this, but there are just too many changes, for me to be able to mentalize it. Can we focus on one (general) issue at a time per pr, please? Like, if there are obvious typing changes that doesn't significantly change the underlying code.

For example fixing marking the package as typed in a separate pr?

Cleaning up type hints that doesn't change the (possibly broken?) aclosing semantics in another. I am thinking all the no-brainer stuff.

Then maybe we can deal with the aclosing issue in the end?

Edit: If possible, I would like to go module by module, even if we don't completely fix one issue with a single pr. I apologize but I just don't have the headspace right now to look through the entire code for each pr.

[mikenerone]

@andersea No worries, I get it. Following all of the typing implications at once did, indeed, balloon a bit. :P Yes, I'll close this PR and try to break it down more in the next few days. It will mean that each step along the way still won't pass a type checker (which is the current state), but they'll be more manageable.