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

Disallow calling type on Protocol #18095

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hauntsaninja
Copy link
Collaborator

@hauntsaninja hauntsaninja commented Nov 3, 2024

Fixes #16919, fixes #16890

It's possible we should not issue the diagnostic and only return object, but that may confuse users. Let's see the primer. Now trying just returning type | types.ModuleType | Any

Should also probably disallow type[P] as well.

@hauntsaninja hauntsaninja force-pushed the type-protocol branch 2 times, most recently from 3d3a3be to afa72b9 Compare November 3, 2024 09:03

This comment has been minimized.

Copy link
Contributor

github-actions bot commented Nov 3, 2024

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

websockets (https://github.com/aaugustin/websockets)
+ src/websockets/legacy/protocol.py:683: error: Item "type" of "type | Module | Any" has no attribute "__aiter__" (not async iterable)  [union-attr]
+ src/websockets/legacy/protocol.py:690: error: Item "type" of "type | Module | Any" has no attribute "__anext__"  [union-attr]

scrapy (https://github.com/scrapy/scrapy)
+ scrapy/utils/python.py:311: error: Unused "type: ignore" comment  [unused-ignore]
+ scrapy/utils/python.py:311: error: Module not callable  [operator]
+ scrapy/utils/python.py:311: note: Error code "operator" not covered by "type: ignore" comment

hydra-zen (https://github.com/mit-ll-responsible-ai/hydra-zen)
+ src/hydra_zen/structured_configs/_implementations.py:1117: error: Argument 1 to "signature" has incompatible type "type | Module | Any"; expected "Callable[..., Any]"  [arg-type]
- src/hydra_zen/structured_configs/_implementations.py:1137: error: No overload variant of "builds" of "BuildsFn" matches argument types "type[DataclassInstance]", "dict[str, int | float | Path | DataClass_ | type[DataClass_] | ListConfig | DictConfig | Enum | Sequence[HydraSupportedType] | Mapping[Any, HydraSupportedType] | None]", "bool | None", "Literal['none', 'partial', 'all', 'object'] | None", "DataclassOptions"  [call-overload]
+ src/hydra_zen/structured_configs/_implementations.py:1137: error: No overload variant of "builds" of "BuildsFn" matches argument types "type | Module | Any", "dict[str, int | float | Path | DataClass_ | type[DataClass_] | ListConfig | DictConfig | Enum | Sequence[HydraSupportedType] | Mapping[Any, HydraSupportedType] | None]", "bool | None", "Literal['none', 'partial', 'all', 'object'] | None", "DataclassOptions"  [call-overload]

xarray-dataclasses (https://github.com/astropenguin/xarray-dataclasses)
+ xarray_dataclasses/datamodel.py:214: error: Incompatible types in assignment (expression has type "type | Module | Any", variable has type "type[DataClass[PInit]] | DataClass[PInit]")  [assignment]
+ xarray_dataclasses/datamodel.py:216: error: Argument 1 to "get_type_hints" has incompatible type "type[DataClass[PInit]] | DataClass[PInit]"; expected "Callable[..., Any]"  [arg-type]

antidote (https://github.com/Finistere/antidote)
+ src/antidote/lib/interface_ext/_internal.py:171: error: Argument 1 to "setdefault" of "MutableMapping" has incompatible type "type | Module | Any"; expected "type[PredicateConstraint[Any]]"  [arg-type]
+ src/antidote/lib/interface_ext/_internal.py:172: error: Argument 1 to "issubclass" has incompatible type "type | Module | Any"; expected "type"  [arg-type]
+ src/antidote/lib/interface_ext/_internal.py:173: error: Unused "type: ignore" comment  [unused-ignore]

@JukkaL
Copy link
Collaborator

JukkaL commented Nov 4, 2024

The websockets error is an interesting one:

        elif isinstance(message, AsyncIterable):
            ... type(message).__aiter__ ...

This is kind of reasonable, since modules can't really implement __aiter__.

Maybe we could infer that protocols with certain dunder methods can only be implemented by regular class instances, and fall back to the to the old type(x) behavior? This would imply that a module object, for example, can never be assignable to AsyncIterable.

ret_type=UnionType(
[
self.named_type("builtins.type"),
self.named_type("types.ModuleType"),
Copy link
Contributor

Choose a reason for hiding this comment

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

(sorry, I got confused myself, deleted & reposted)

self.named_type('types.ModuleType') isn't the correct type here:

import types
assert type(types) is types.ModuleType

When you call type(some_module), you obtain types.ModuleType class itself, not its instance. So this union isn't type | types.ModuleType | Any, it should be type | type[ModuleType] | Any, and at this point type[ModuleType] member is redundant (it's also a type, isn't it?).

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.

Disallow type(x) if x has a protocol type (🐞) Modules don't really conform to Protocols do they
3 participants