Skip to content

Make builtins.type generic #12983

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

Closed
wants to merge 5 commits into from
Closed

Make builtins.type generic #12983

wants to merge 5 commits into from

Conversation

rchen152
Copy link
Collaborator

@rchen152 rchen152 commented Nov 8, 2024

I'm fairly certain type should be generic. Unless there's some reason it's intentionally not marked as such?

This comment has been minimized.

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.

Typeshed still supports Python 3.8. We should also discuss whether default=Any is desirable, since I think this would affect whether type would be flagged by something like mypy's --disallow-any-generics (I say "like" because mypy's actual behaviour here is a little custom due to all the history, see also python/mypy#16366)

@rchen152
Copy link
Collaborator Author

rchen152 commented Nov 8, 2024

Do you have a suggestion for a better default? default=Any was just me copying how the TypeVars for slice are defined, to try to suppress errors about missing type arguments XD

I don't think typeshed still supporting 3.8 should stop us from marking type as generic. There are plenty of builtins that aren't subscriptable in 3.8 but are marked as generic in typeshed.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Nov 8, 2024

Yeah, I guess the choices are:

  • default=Any
  • default=object
  • No default

There was some discussion about this in https://discuss.python.org/t/inconsistencies-between-type-and-type/37404. However that predates PEP 696, so you'd have to have added type checker special casing for type to mean type[object] at the time of that discussion.

Random thoughts:

  • If we remove the default, mypy will probably need to patch or add an extra error code. While in general I don't like when typeshed forces mypy's hand, it's not the end of the world. I'd rather typeshed have the behaviour we want going forward for something fundamental like builtins.type
  • I do like that PEP 696 gives us more options here!
  • default=Any is the least type safe, because type checkers won't flag non-subscripted default=Any generics with the equivalent of mypy's --disallow-any-generics (this is a feature)
  • I think in general, typeshed should use default=Any if a) the new generic provides relatively marginal value to the type, b) the change is disruptive
  • I can't tell how much value the new generic provides aka default=object might be fine? It's basically changing the return type of def __call__ to object instead of Any (although I guess we don't express that in the stubs). The args to __call__ are already a little bit of a lost cause. See also the primer hits in Interpret type annotations as type[Any] annotations mypy#16366
  • I'm not sure I'm saying anything useful so I'm going to go to sleep and see what you all have to say in the morning :-)

I don't think typeshed still supporting 3.8 should stop us

Yup, you're right!

@rchen152
Copy link
Collaborator Author

rchen152 commented Nov 8, 2024

Thanks for the detailed reply! I had missed the discussion about Type vs. type, so that was enlightening to read up on.

Having no default is the first thing I tried, and that caused both mypy and pyright to generate errors about missing type arguments: https://github.com/python/typeshed/actions/runs/11737283657.

Based on that Discourse thread, it seems to me that Any might be the better default? But I don't have a strong opinion and could be easily swayed the other way.

@JelleZijlstra
Copy link
Member

I don't know that this is actually worth doing. type[] behaves sort of like a generic, but it is its own special thing (for example, type[Literal[1]] is invalid). This may be better handled by special-casing in type checkers.

@AlexWaygood
Copy link
Member

I like that this gets rid of a few false positives in mypy, where it seems not to understand that type is subscriptable in some contexts -- presumably runtime contexts where it sees type[] being treated as a value rather than as a type? But I think the principled way to solve that would be to add a __class_getitem__ method to type1. If mypy doesn't understand that a __class_getitem__ method makes type subscriptable, that feels like a mypy issue.

And I agree that type isn't really generic in a conventional sense. You can see this from the fact that there are no methods or attributes on type where it would make sense to use the new _TypeT parameter in any annotations.

Giving my perspective as somebody actively building a type checker, we've already worked in special casing to red-knot so that we understand that type[] is subscriptable even though it isn't generic and has no __class_getitem__ method. I don't mind adding a __class_getitem__ method to type, but I feel like making it generic in the stubs is probably more likely to confuse red-knot than help red-knot.

Footnotes

  1. Of course, type doesn't actually have a __class_getitem__ method at runtime either; type is actually subscriptable at runtime due to some special-casing hardcoded directly into PyObject_GetItem in CPython. But adding __class_getitem__ to type feels like a reasonable white lie I'd be happy to live with.

@JelleZijlstra
Copy link
Member

the principled way to solve that would be to add a class_getitem method to type1

The problem with this is that subclasses of type are not subscriptable. As you mention, the subscriptability of type is hardcoded deep in CPython. That suggests to me that type checkers also need to hardcode this subscriptability.

@AlexWaygood
Copy link
Member

The problem with this is that subclasses of type are not subscriptable.

I hadn't considered that; that's a great point!

@JukkaL
Copy link
Contributor

JukkaL commented Nov 8, 2024

I wonder if this has some concrete benefits? Most type checkers probably already have a bunch of special casing to make typical use cases work as expected. Maybe some of these could be simplified with this change. However, it's not clear to me if this is a net win, as any change to such a foundational and special-cased class would also risks causing regressions.

Here's an example that further illustrates that type is pretty special:

class Foo(type[int]):   # Should probably generate an error
    ...

I wouldn't give much weight to the fixed mypy false positives in mypy_primer, at least until we understand the root cause.

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

This comment has been minimized.

Copy link
Contributor

github-actions bot commented Nov 8, 2024

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

prefect (https://github.com/PrefectHQ/prefect)
- src/prefect/utilities/collections.py:184: error: Item "type" of "type[T] | tuple[type[T], ...]" has no attribute "__iter__" (not iterable)  [union-attr]
+ src/prefect/utilities/collections.py:184: error: Item "type[Any]" of "type[T] | tuple[type[T], ...]" has no attribute "__iter__" (not iterable)  [union-attr]
- src/prefect/utilities/pydantic.py:189: note:     def __new__(type[type], object, /) -> type
+ src/prefect/utilities/pydantic.py:189: note:     def __new__(type[type[Any]], object, /) -> type[Any]
- src/prefect/utilities/pydantic.py:189: note:     def [_typeshed.Self] __new__(type[_typeshed.Self], str, tuple[type, ...], dict[str, Any], /, **kwds: Any) -> _typeshed.Self
+ src/prefect/utilities/pydantic.py:189: note:     def [_typeshed.Self] __new__(type[_typeshed.Self], str, tuple[type[Any], ...], dict[str, Any], /, **kwds: Any) -> _typeshed.Self
- src/prefect/utilities/pydantic.py:191: note:     def __new__(type[type], object, /) -> type
+ src/prefect/utilities/pydantic.py:191: note:     def __new__(type[type[Any]], object, /) -> type[Any]
- src/prefect/utilities/pydantic.py:191: note:     def [_typeshed.Self] __new__(type[_typeshed.Self], str, tuple[type, ...], dict[str, Any], /, **kwds: Any) -> _typeshed.Self
+ src/prefect/utilities/pydantic.py:191: note:     def [_typeshed.Self] __new__(type[_typeshed.Self], str, tuple[type[Any], ...], dict[str, Any], /, **kwds: Any) -> _typeshed.Self
- src/prefect/blocks/core.py:244: error: "type" has no attribute "_to_block_schema_reference_dict"  [attr-defined]
+ src/prefect/blocks/core.py:244: error: "type[Any]" has no attribute "_to_block_schema_reference_dict"  [attr-defined]
- src/prefect/blocks/core.py:248: error: "type" has no attribute "_to_block_schema_reference_dict"  [attr-defined]
+ src/prefect/blocks/core.py:248: error: "type[Any]" has no attribute "_to_block_schema_reference_dict"  [attr-defined]
- src/prefect/blocks/core.py:251: error: "type" has no attribute "_to_block_schema_reference_dict"  [attr-defined]
+ src/prefect/blocks/core.py:251: error: "type[Any]" has no attribute "_to_block_schema_reference_dict"  [attr-defined]
- src/prefect/blocks/core.py:1081: error: "type" has no attribute "register_type_and_schema"  [attr-defined]
+ src/prefect/blocks/core.py:1081: error: "type[Any]" has no attribute "register_type_and_schema"  [attr-defined]
- src/prefect/flow_engine.py:243: error: Item "type" of "BaseResult[Any] | type[NotSet]" has no attribute "get"  [union-attr]
+ src/prefect/flow_engine.py:243: error: Item "type[Any]" of "BaseResult[Any] | type[NotSet]" has no attribute "get"  [union-attr]
- src/prefect/task_engine.py:470: error: Item "type" of "BaseResult[Any] | type[NotSet]" has no attribute "get"  [union-attr]
+ src/prefect/task_engine.py:470: error: Item "type[Any]" of "BaseResult[Any] | type[NotSet]" has no attribute "get"  [union-attr]
- src/prefect/task_engine.py:475: error: Item "type" of "ResultRecord[Any] | type[NotSet]" has no attribute "result"  [union-attr]
+ src/prefect/task_engine.py:475: error: Item "type[Any]" of "ResultRecord[Any] | type[NotSet]" has no attribute "result"  [union-attr]
- src/prefect/task_engine.py:986: error: Item "type" of "BaseResult[Any] | type[NotSet]" has no attribute "get"  [union-attr]
+ src/prefect/task_engine.py:986: error: Item "type[Any]" of "BaseResult[Any] | type[NotSet]" has no attribute "get"  [union-attr]
- src/prefect/task_engine.py:988: error: Item "type" of "ResultRecord[Any] | type[NotSet]" has no attribute "result"  [union-attr]
+ src/prefect/task_engine.py:988: error: Item "type[Any]" of "ResultRecord[Any] | type[NotSet]" has no attribute "result"  [union-attr]
- src/prefect/flow_runs.py:415: error: Item "type" of "type[AutomaticRunInput[Never]] | type[T]" has no attribute "load"  [union-attr]
+ src/prefect/flow_runs.py:415: error: Item "type[Any]" of "type[AutomaticRunInput[Never]] | type[T]" has no attribute "load"  [union-attr]
- src/prefect/flow_runs.py:425: error: Item "type" of "type[AutomaticRunInput[Never]] | type[T]" has no attribute "save"  [union-attr]
+ src/prefect/flow_runs.py:425: error: Item "type[Any]" of "type[AutomaticRunInput[Never]] | type[T]" has no attribute "save"  [union-attr]
- src/prefect/cli/config.py:240: error: Item "type" of "Setting | type[NotSet]" has no attribute "name"  [union-attr]
+ src/prefect/cli/config.py:240: error: Item "type[Any]" of "Setting | type[NotSet]" has no attribute "name"  [union-attr]

ibis (https://github.com/ibis-project/ibis)
- ibis/expr/operations/udf.py:155: error: Argument 2 to "type" has incompatible type "tuple[Callable[[_UDF], type[B]]]"; expected "tuple[type, ...]"  [arg-type]
+ ibis/expr/operations/udf.py:155: error: Argument 2 to "type" has incompatible type "tuple[Callable[[_UDF], type[B]]]"; expected "tuple[type[Any], ...]"  [arg-type]
- ibis/expr/operations/core.py:45: note:          def __coerce__(cls, value: Any, T: type | None = ..., S: type | None = ...) -> Value[T, S]
+ ibis/expr/operations/core.py:45: note:          def __coerce__(cls, value: Any, T: type[Any] | None = ..., S: type[Any] | None = ...) -> Value[T, S]
- ibis/examples/__init__.py:167: error: "type" has no attribute "fetch"  [attr-defined]
+ ibis/examples/__init__.py:167: error: "type[Any]" has no attribute "fetch"  [attr-defined]

streamlit (https://github.com/streamlit/streamlit)
- lib/tests/streamlit/elements/selectbox_test.py:263:29: error: Item "type" of "Union[Any, Type[None]]" has no attribute "C"  [union-attr]
+ lib/tests/streamlit/elements/selectbox_test.py:263:29: error: Item "type[Any]" of "Union[Any, Type[None]]" has no attribute "C"  [union-attr]
- lib/tests/streamlit/elements/radio_test.py:305:25: error: Item "type" of "Union[Any, Type[None]]" has no attribute "C"  [union-attr]
+ lib/tests/streamlit/elements/radio_test.py:305:25: error: Item "type[Any]" of "Union[Any, Type[None]]" has no attribute "C"  [union-attr]

discord.py (https://github.com/Rapptz/discord.py)
- discord/utils.py:745: error: "type" has no attribute "__slots__"  [attr-defined]
+ discord/utils.py:745: error: "type[Any]" has no attribute "__slots__"  [attr-defined]
- discord/ext/tasks/__init__.py:488: error: Incompatible types in assignment (expression has type "tuple[type, ...]", variable has type "tuple[type[OSError], type[GatewayNotFound], type[ConnectionClosed], type[ClientError], type[TimeoutError]]")  [assignment]
+ discord/ext/tasks/__init__.py:488: error: Incompatible types in assignment (expression has type "tuple[type[Any], ...]", variable has type "tuple[type[OSError], type[GatewayNotFound], type[ConnectionClosed], type[ClientError], type[TimeoutError]]")  [assignment]

yarl (https://github.com/aio-libs/yarl)
- tests/test_quoting.py:504:13: error: "type" expects no type arguments, but 1 given  [type-arg]
- tests/test_quoting.py:505:15: error: "type" expects no type arguments, but 1 given  [type-arg]
- tests/test_quoting.py:525:13: error: "type" expects no type arguments, but 1 given  [type-arg]
- tests/test_quoting.py:526:15: error: "type" expects no type arguments, but 1 given  [type-arg]
- tests/test_quoting.py:546:13: error: "type" expects no type arguments, but 1 given  [type-arg]
- tests/test_quoting.py:547:15: error: "type" expects no type arguments, but 1 given  [type-arg]

hydra-zen (https://github.com/mit-ll-responsible-ai/hydra-zen)
- src/hydra_zen/structured_configs/_implementations.py:2601: error: "type" has no attribute "__fields__"  [attr-defined]
+ src/hydra_zen/structured_configs/_implementations.py:2601: error: "type[Any]" has no attribute "__fields__"  [attr-defined]
- src/hydra_zen/structured_configs/_implementations.py:2833: error: Argument 1 to "append" of "list" has incompatible type "tuple[str, Any]"; expected "tuple[str, type, Field[Any]]"  [arg-type]
+ src/hydra_zen/structured_configs/_implementations.py:2833: error: Argument 1 to "append" of "list" has incompatible type "tuple[str, Any]"; expected "tuple[str, type[Any], Field[Any]]"  [arg-type]
- src/hydra_zen/structured_configs/_implementations.py:2921: error: Argument "type_" to "_retain_type_info" has incompatible type "str | Any"; expected "type"  [arg-type]
+ src/hydra_zen/structured_configs/_implementations.py:2921: error: Argument "type_" to "_retain_type_info" has incompatible type "str | Any"; expected "type[Any]"  [arg-type]
- src/hydra_zen/structured_configs/_implementations.py:3238: error: List comprehension has incompatible type List[tuple[str, Any | <typing special form>, Any | Field[Any]]]; expected List[tuple[str, type] | tuple[str, type, Any]]  [misc]
+ src/hydra_zen/structured_configs/_implementations.py:3238: error: List comprehension has incompatible type List[tuple[str, Any | <typing special form>, Any | Field[Any]]]; expected List[tuple[str, type[Any]] | tuple[str, type[Any], Any]]  [misc]

pandas-stubs (https://github.com/pandas-dev/pandas-stubs)
+ tests/test_series.py:3265: error: Argument 1 to "assert_never" has incompatible type "Series[type[object]]"; expected "Never"  [arg-type]

beartype (https://github.com/beartype/beartype)
+ beartype/_decor/decorcore.py:137: error: Argument 1 to "beartype_nontype" has incompatible type "BeartypeableT"; expected "type[Any]"  [arg-type]
+ beartype/_decor/decorcore.py:137: note: Error code "arg-type" not covered by "type: ignore" comment

artigraph (https://github.com/artigraph/artigraph)
- src/arti/internal/type_hints.py:54: error: Non-overlapping identity check (left operand type: "type", right operand type: "<typing special form>")  [comparison-overlap]
+ src/arti/internal/type_hints.py:54: error: Non-overlapping identity check (left operand type: "type[Any]", right operand type: "<typing special form>")  [comparison-overlap]
- src/arti/internal/type_hints.py:55: error: Non-overlapping identity check (left operand type: "type", right operand type: "<typing special form>")  [comparison-overlap]
+ src/arti/internal/type_hints.py:55: error: Non-overlapping identity check (left operand type: "type[Any]", right operand type: "<typing special form>")  [comparison-overlap]
- src/arti/internal/type_hints.py:104: error: Returning Any from function declared to return "tuple[type, ...]"  [no-any-return]
+ src/arti/internal/type_hints.py:104: error: Returning Any from function declared to return "tuple[type[Any], ...]"  [no-any-return]

pydantic (https://github.com/pydantic/pydantic)
- pydantic/_internal/_generate_schema.py:122: error: List item 1 has incompatible type "<typing special form>"; expected "type"  [list-item]
+ pydantic/_internal/_generate_schema.py:122: error: List item 1 has incompatible type "<typing special form>"; expected "type[Any]"  [list-item]

trio (https://github.com/python-trio/trio)
+ src/trio/_tests/test_exports.py:83: error: Explicit "Any" is not allowed  [misc]

@rchen152
Copy link
Collaborator Author

rchen152 commented Nov 8, 2024

Ha, I didn't expect this to be so controversial.

To expand a bit on my motivation here: there's no indication in the definition of type that it's subscriptable (it's not generic, doesn't have __class_getitem__, isn't a _SpecialForm which would make it obvious that it needs special handling). So any tool that reads typeshed has to somehow know and remember to special-case type everywhere. Which is surprising and easy to get wrong - based on the primer results, it certainly looks like mypy missed it somewhere =P

Re: subclasses of type not being subscriptable - was that an intentional decision or a side effect of how subscripting type was implemented? If the subclass behavior is deliberate, then I do think that's a convincing argument that we shouldn't make this change. But I'm not sure it is, given that it's possible to create a generic subclass of typing.Type, which I believe is supposed to be equivalent to builtins.type.

@AlexWaygood
Copy link
Member

Re: subclasses of type not being subscriptable - was that an intentional decision or a side effect of how subscripting type was implemented?

I'm pretty sure that's deliberate. If it wasn't so, then all metaclasses would be subscriptable. But that would be pretty confusing, because metaclasses like abc.ABCMeta and enum.EnumMeta aren't generic. Elsewhere in the language, only generic types are subscriptable.

@rchen152
Copy link
Collaborator Author

rchen152 commented Nov 8, 2024

Re: subclasses of type not being subscriptable - was that an intentional decision or a side effect of how subscripting type was implemented?

I'm pretty sure that's deliberate. If it wasn't so, then all metaclasses would be subscriptable. But that would be pretty confusing, because metaclasses like abc.ABCMeta and enum.EnumMeta aren't generic. Elsewhere in the language, only generic types are subscriptable.

Ah, that makes sense. Seems like we definitely shouldn't add a __class_getitem__ to type then, since that would also cause type checkers to model metaclasses as subscriptable.

I don't think a class type(Generic[_TypeT_co]): ... definition would have the same problem, though? Since inheriting from a bare type wouldn't make a subclass subscriptable.

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 8, 2024

I don't think a class type(Generic[_TypeT_co]): ... definition would have the same problem, though? Since inheriting from a bare type wouldn't make a subclass subscriptable.

I think that would mean that mypy would start complaining if you created a custom metaclass that inherited from bare type, though, and you had mypy's strictest settings enabled? E.g. mypy issues a complaint on this snippet if you have its --disallow-any-generics setting enabled (which is enabled as part of --strict):

class Foo[T]: ...
class Bar(Foo): ...  # error: Missing type parameters for generic type "Foo"  [type-arg]

@rchen152
Copy link
Collaborator Author

rchen152 commented Nov 8, 2024

I think that would mean that mypy would start complaining if you created a custom metaclass that inherited from bare type, though, and you had mypy's strictest settings enabled? E.g. mypy issues a complaint on this snippet if you have its --disallow-any-generics setting enabled (which is enabled as part of --strict):

class Foo[T]: ...
class Bar(Foo): ...  # error: Missing type parameters for generic type "Foo"  [type-arg]

Yeah, I ran into lots of those errors on my first attempt. Adding a default to the TypeVar seems to fix it (example), which is why the latest version has default=Any.

@AlexWaygood
Copy link
Member

Oh, I see, of course -- sorry! I guess I still feel like this is more likely to cause issues for us at red-knot than it would help us, though. The meaning that subscripting type[] has is just very different to the meaning subscripting any other generic class has.

@rchen152
Copy link
Collaborator Author

rchen152 commented Nov 8, 2024

The meaning that subscripting type[] has is just very different to the meaning subscripting any other generic class has.

I agree that type is special. I guess what I'm struggling with right now is, why does that mean that it's better to not mark it as generic?

If it's defined as class type: ..., type checkers have to have a special case for type when they see a class being parameterized and additionally do checks for invalid things like type[Literal[1]] and class Foo(type[int]): ....

If it's defined as class type(Generic[_TypeT_co]): ..., the first special case is no longer needed, the checks for invalid things still are.

(This isn't just a theoretical concern, I'm doing some work related to parsing typeshed right now, and with the current stubs, code that otherwise does not need any knowledge of type's special-ness has to special-case it.)

@JelleZijlstra
Copy link
Member

why does that mean that it's better to not mark it as generic?

It's not a great argument, but I think the best reason to keep it as is is that it's always been this way, which probably means that existing type checkers have written their code to work with the current definition. Making a change at such a fundamental level risks breaking things. And any type checker will at some point have to special-case type, because it is a special form in the type system, not a regular generic.

I'm not strongly opposed to this change; it does make some sense. But we need to be careful.

@rchen152
Copy link
Collaborator Author

rchen152 commented Nov 8, 2024

why does that mean that it's better to not mark it as generic?

It's not a great argument, but I think the best reason to keep it as is is that it's always been this way, which probably means that existing type checkers have written their code to work with the current definition. Making a change at such a fundamental level risks breaking things. And any type checker will at some point have to special-case type, because it is a special form in the type system, not a regular generic.

I'm not strongly opposed to this change; it does make some sense. But we need to be careful.

That's fair. I don't love that argument, but there's certainly practical value in not risking breaking things that are working unless there's a clear benefit. And yeah, special-casing type to some degree is unavoidable anyway.

Thanks for humoring me; I learned a surprising amount about type from trying to make this change =)

@rchen152 rchen152 closed this Nov 8, 2024
@rchen152 rchen152 deleted the type branch November 8, 2024 20:30
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.

5 participants