Skip to content

Return Supports(A)Next from (a)iter #6035

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Dec 21, 2021
Merged

Return Supports(A)Next from (a)iter #6035

merged 14 commits into from
Dec 21, 2021

Conversation

srittau
Copy link
Collaborator

@srittau srittau commented Sep 14, 2021

Explore the impact of changing the return type, see #6030.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@hauntsaninja
Copy link
Collaborator

We'll need to change the types of next and anext as well. Should hopefully clear up a lot of primer damage

@srittau
Copy link
Collaborator Author

srittau commented Sep 14, 2021

We'll need to change the types of next and anext as well. Should hopefully clear up a lot of primer damage

Regardless of whether we change iter() and aiter(), we should probably change next() and anext() in either case, since they clearly don't require __iter__() and the change should have no negative effects.

@github-actions

This comment has been minimized.

def __next__(self) -> _T_co: ...

# stable
class SupportsANext(Protocol[_T_co]):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitty: Maybe this should be SupportsAnext?

We have SupportsAclose elsewhere.
Anext is the camel case equivalent of anext / maybe reads more distinguishably from SupportsNext? Don't feel strongly though!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth 3.10rc2 renamed PyAiter_Check to PyAIter_Check in the C API (https://docs.python.org/3.10/whatsnew/changelog.html#id2).

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@srittau srittau marked this pull request as ready for review September 14, 2021 11:28
_SupportsNextT = TypeVar("_SupportsNextT", bound=SupportsNext[Any], covariant=True)
_SupportsAnextT = TypeVar("_SupportsAnextT", bound=SupportsAnext[Any], covariant=True)

class _Iterable2(Protocol[_T_co]):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not HasIter?

Copy link
Collaborator Author

@srittau srittau Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've renamed to protocols (as _Supports...). Do you think it would make sense to add them to _typeshed?

@github-actions

This comment has been minimized.

@Akuli
Copy link
Collaborator

Akuli commented Sep 14, 2021

I like how using a TypeVar makes this much less terrible than I thought this was going to be. My complaints in #6030 don't really apply anymore.

That said, the new error message from mypy_primer itself is not exactly very intuitive or user-friendly. At a first glance, it looks like it is saying that a list isn't iterable. So to me it looks like this change introduces one practical problem without fixing any other practical problems.

@srittau
Copy link
Collaborator Author

srittau commented Sep 15, 2021

I prefer the correctness and the fact that it fixes two separate problems over the minor inconvenience that one particular type checker has a slightly obtuse error message in a complicated case. This really shouldn't be a factor.

@Akuli
Copy link
Collaborator

Akuli commented Sep 15, 2021

it fixes two separate problems

What two problems? I have seen no actual problems that this fixes, besides some theorethical correctness that really doesn't seem to apply in practice and therefore is meaningless to me.

one particular type checker has a slightly obtuse error message in a complicated case

It is true that mypy is just "one particular type checker", but it is an important type checker. In other areas of typeshed, we already have "unnecessary" overloads just to help mypy in complicated situations.

I'm fine with merging this, even though I still don't see why it would be a good thing.

@srittau
Copy link
Collaborator Author

srittau commented Sep 15, 2021

What two problems?

That __iter__ is not required as mentioned by @brettcannon and that the type returned by iter() is a generic iterator, not the concrete iterator returned by the argument's __iter__,. see my example in #6035. This might both not be super relevant, but it's an easy fix with basically no drawback.

@Akuli
Copy link
Collaborator

Akuli commented Sep 15, 2021

A non-concrete Iterator type might actually be better. mypy_primer does this:

    project_iter = iter(p for p in PROJECTS)
    if ARGS.project_selector:
        projects = [ ... ]
        ...
        project_iter = iter(projects)

If iter() returns the actual type of the iterator, which is a generator in the first case but list_iterator in the second case, project_iter becomes a generator object, not a list_iterator object. In fact, the purpose of the first iter() is to cast the generator to an iterator. Again, mypy-specific doesn't mean not important.

That said, this was previously considered not very important in #5145, and maybe we should go with that here too. It is also easy to fix by adding a type annotation, project_iter: Iterator[Project] = ...

However, typeshed is not just for type checkers. What autocompletions do we want to get in this situation?

iterator = iter(some_function_that_yields())
iterator.<Tab>

I wouldn't want to want to see iterator.send() and iterator.throw() in the autocompletion list, and it would be nice if I noticed that I have an unnecessary and confusing iter() by not seeing the expected autocompletions. But I don't think this really comes up much in practice.

So again, I don't really like this change, but I'm fine with it getting merged.

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

mypy_primer (https://github.com/hauntsaninja/mypy_primer)
+ mypy_primer.py:557: error: Argument 1 to "iter" has incompatible type "List[Project]"; expected "_SupportsIter[Generator[Project, None, None]]"

@JelleZijlstra JelleZijlstra merged commit 387ef81 into python:master Dec 21, 2021
@srittau srittau deleted the iter branch December 21, 2021 06:40
BvB93 pushed a commit to BvB93/numpy that referenced this pull request Mar 11, 2022
charris pushed a commit to charris/numpy that referenced this pull request Mar 13, 2022
melissawm pushed a commit to melissawm/numpy that referenced this pull request Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants