Skip to content
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

Defined _NoneType class in typing.pyi that type checkers can use to… #4519

Merged
merged 7 commits into from Sep 20, 2020

Conversation

erictraut
Copy link
Contributor

… handle protocol matching for None (e.g. foo: Hashable = None).

… handle protocol matching for None (e.g. `foo: Hashable = None`).
@erictraut
Copy link
Contributor Author

@JukkaL, this is what we discussed in this thread: python/mypy#9371

@gvanrossum
Copy link
Member

Why reformat the whole file?

@erictraut
Copy link
Contributor Author

Sorry, not intended. Auto-formatter snafu. I'll fix it.

@hauntsaninja
Copy link
Collaborator

def __hash__(self) -> int: ...
def __repr__(self) -> str: ...
def __sizeof__(self) -> int: ...
def __str__(self) -> str: ...
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of these seem redundant, since they are the same as in object. Maybe only __bool__ is worth including here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

object exposes a bunch of methods that are not applicable for _NoneType, but I'm unaware of any way to "hide" these methods from a subclass. So my plan was to special-case _NoneType in the type checker such that it doesn't implicitly derive from object. Do you see a better way of doing this?


class _NoneType:
__doc__: Optional[str]
def __bool__(self) -> bool: ...
Copy link
Contributor

Choose a reason for hiding this comment

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

The return type of __bool__ could be Literal[False].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we do this, we'd need to make it conditional on , since Literal isn't defined in typing.pyi prior to 3.8, and I don't think we want to import typing_extensions from typing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be solved by @hauntsaninja's suggestion above to move it to _typeshed. But I assume you want to special-case it in pyright and would prefer to have it in typing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I have a slight preference for having it in typing, but if the consensus is that it belongs in _typeshed.pyi, I could just as easily special-case that.

Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to put this in a pyright_extensions module that pyright itself distributes with stubs? That's what we've done in the past with mypy- and pyre-specific primitives.

It would be more work to maintain though; I'm also fine with putting it here or in _typeshed.

Does it also need to be in stdlib/2/typing.pyi?

Copy link
Contributor

Choose a reason for hiding this comment

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

Especially if this class has the special property of basically not inheriting from object, putting it in a pyright-specific location such as pyright_extensions seem like the best option for me. Otherwise we'd be introducing this new concept to typeshed that other type checkers (at least mypy) don't support. Mypy treats everything as deriving from object. If there is interest by other type checkers, we can later reconsider including this, but right now it feels quite specialized.

For historical reasons there are some mypy-specific things in stdlib stubs, and I'd like to move away type checker specific things instead of adding more of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intention here isn't to provide pyright-specific functionality. Of course I can hard-code behaviors into pyright, but that defeats the purpose. My goal here was to provide a standardized way that all type checkers could leverage such that they could properly handle None without hard-coding behaviors.

If you don't like this proposal, can you offer other proposals that achieve this goal? Or do you think that the goal is misguided?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if I was unclear. My concern is about what makes _NoneType special in that it can't be treated as a subclass of object. I don't understand why that would be needed and assumed that this would be specific to pyright, but I may be mistaken there. Can give more motivation why _NoneType is special in this way?

My alternative proposal would be to treat _NoneType as a subclass of object, since this seems to match how things work at runtime:

>>> type(None).mro()
[<class 'NoneType'>, <class 'object'>]

I'd also leave out all methods except __bool__ and perhaps others such as __repr__ that are not inherited from object (these seem to include __subclasshook__, __new__ and __init_subclass__, but specifying these explicitly may not add any value).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that proposal works for me. There are some attributes that are defined for object that are not available on None (__dict__, __slots__, __module__), and the __class__ property setter doesn't work for None, but those are edge cases that are probably fine to ignore.

Copy link
Member

Choose a reason for hiding this comment

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

Note that None is hardly the only type that doesn't actually have __dict__ or other attributes you mention. The actual semantics are just hard to model in the type system.

@@ -663,3 +663,15 @@ if sys.version_info >= (3, 7):
def __eq__(self, other: Any) -> bool: ...
def __hash__(self) -> int: ...
def __repr__(self) -> str: ...

class _NoneType:
Copy link
Contributor

Choose a reason for hiding this comment

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

The class could be final, since it's not subclassable at runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The final decorator is available only for 3.8 and newer, but _NoneType needs to be available regardless of version.

Copy link
Member

Choose a reason for hiding this comment

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

But it's always in typing_extensions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I don't think we want to import typing_extensions from typing. Is the consensus that I should move this out of typing.pyi and into _typeshed.pyi? That sounds inconsistent to me given that other types like this (e.g. _TypedDict) are defined in typing.pyi, but if that's what you'd prefer, I can move it. And then I could add both the final decorator and change __bool__ to return Literal[False].

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

I'm fine with it as is, but I'd like to wait for @JukkaL's and @hauntsaninja's approval before merging.

@erictraut erictraut closed this Sep 15, 2020
@erictraut erictraut reopened this Sep 15, 2020
@erictraut
Copy link
Contributor Author

The CI system seems to have gotten stuck, so I closed and re-opened the PR to rerun tests.

@erictraut
Copy link
Contributor Author

@JukkaL We need to decide whether this belongs in _typeshed.pyi or typing.pyi. I mentioned above that I have a slight preference for the latter since it seems consistent with other types like _TypedDict that already exist in typing.pyi, but if the consensus is that it should be in _typeshed.pyi, I'll move it.

Also, the CI system appears to be struggling with this PR. It has been stuck in the "queued" state. I've tried to reset it by closing and reopening the PR, but that didn't seem to fix it. Perhaps someone with write permissions needs to requeue it?

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Sep 19, 2020

I'd say move it to _typeshed (and change the return type of bool to Literal[False]).
_TypedDict predates the introduction of _typeshed (personally I'd accept a PR that moves it and reexports for compatibility). Using private members of typing and builtins is pretty obscure; if we're hoping to make a type that can be reused, we should put it in the place for reusable types.

CI is green if you click through, so I wouldn't worry too much about it. We can take care of merging it when the time comes.

… and changed `__bool__` method to return `Literal[False]` rather than `bool`.
@erictraut
Copy link
Contributor Author

Thanks @hauntsaninja, I incorporated the code review suggestions.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

@erictraut Since it's in _typeshed (and meant to be used), we can remove the underscore prefix. But nit aside, looks merge ready to me :-)


# Used by type checkers for checks involving None (does not exist at runtime)
@final
class _NoneType:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
class _NoneType:
class NoneType:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I renamed as suggested.

@hauntsaninja hauntsaninja merged commit c41f3e9 into python:master Sep 20, 2020
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.

None yet

7 participants