Skip to content

feat: type-check-only dtype protocol #31

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat: type-check-only dtype protocol #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,11 @@ version_tuple = {version_tuple!r}
"D107", # Missing docstring in __init__
"D203", # 1 blank line required before class docstring
"D213", # Multi-line docstring summary should start at the second line
"D401", # First line of docstring should be in imperative mood
"FBT", # flake8-boolean-trap
"FIX", # flake8-fixme
"ISC001", # Conflicts with formatter
"PLW1641", # Object does not implement `__hash__` method
Copy link
Member

Choose a reason for hiding this comment

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

this is a pretty good check actually:

>>> class A:
...     def __eq__(self, other):
...         return True
...         
>>> hash(A())
Traceback (most recent call last):
  File "<python-input-1>", line 1, in <module>
    hash(A())
    ~~~~^^^^^
TypeError: unhashable type: 'A'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As in, we should add specific ignores for this rather a blanket ignore, like I'm doing here?

Copy link
Member

Choose a reason for hiding this comment

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

We should not prohibit dtypes from being unhashable, which is what this does (but not on mypy: python/mypy#18622)

]

[tool.ruff.lint.per-file-ignores]
Expand Down
21 changes: 21 additions & 0 deletions src/array_api_typing/_misc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
__all__ = ("DType",)

from typing import Protocol, type_check_only
Copy link
Member

Choose a reason for hiding this comment

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

type_check_only does not exist at runtime

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This decorator is itself not available at runtime. It is mainly intended to mark classes that are defined in type stub files if an implementation returns an instance of a private class:

Dagnabbit.

Copy link
Member

Choose a reason for hiding this comment

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

I honestly have no idea why that is...

Copy link
Member

Choose a reason for hiding this comment

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

Dagnabbit.

I agree



@type_check_only
class DType(Protocol):
"""Protocol for classes that represent a data type.

This `typing.Protocol` is `typing.type_check_only` and cannot be used at
runtime. This limitation is intentional since the array API structurally
defines a ``dtype`` object as anything with an ``__eq__`` method that
compares to another ``dtype`` object. This broad definition means that most
Python objects will satisfy this protocol and can be erroneously considered
a ``dtype``.

"""

def __eq__(self, other: object, /) -> bool:
"""Computes the truth value of ``self == other`` in order to test for data type object equality.""" # noqa: E501
...
Comment on lines +6 to +21
Copy link
Member

Choose a reason for hiding this comment

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

This is equivalent to

class DType(Protocol):
    __hash__ = None

The __eq__ method is already implemented by builtins.object. But if you override it, and don't also provide a __hash__ method, then __hash__ = None (I hate it, but it's a python quirk). Pyright understands this, but mypy does not. So this will lead to inconsistent outcomes between type-checkers.

If you were to add a __hash__ method here that is identical to object.__hash__, then the resulting protocol would be exactly equivalent to a builtins.object. So given the current array api spec, it's not possible to structurally type a dtype in a meaningful way, I'm afraid.

And just to be clear, I'm not saying that we should replace this protocol with a type alias to object or Any or something. There are no problems that it would solve.

So although I certainly appreciate your work here, I just don't think this is a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are dtypes not guaranteed to be hashable?

Copy link
Member

Choose a reason for hiding this comment

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

They neither are nor aren't guaranteed to be hashable. But there's no way to express that in Python's type system.