Skip to content

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

Merged
JelleZijlstra merged 14 commits intopython:masterfrom
srittau:iter
Dec 21, 2021
Merged

Return Supports(A)Next from (a)iter#6035
JelleZijlstra merged 14 commits intopython:masterfrom
srittau:iter

Conversation

@srittau
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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