Revert "memoryview: remove inheritance from Sequence"#12800
Revert "memoryview: remove inheritance from Sequence"#12800JelleZijlstra merged 2 commits intomainfrom
memoryview: remove inheritance from Sequence"#12800Conversation
This reverts commit f625e92.
This comment has been minimized.
This comment has been minimized.
|
Is it possible to override the missing methods in a way that makes them unusable / almost always raise typing errors? Maybe something like a NoReturn + deprecated + Never args? |
|
Done. I set index and count to None as @AlexWaygood suggested in the other thread. I didn't attempt to set |
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
I like it much more with the None overrides, and your point about this causing unsound type narrowing with isinstance() is a good one. I still think you're underrating the impact of IDEs and autocomplete tools suggesting nonexistent "public-API" methods in autocompletions (and failing to error when the user accesses them in their code). But fixing it in CPython is clearly the correct long-term fix (thank you for taking up that cause!).
TL;DR: still not sure it's holistically better overall to have it inherit from Sequence, but fine with it being reverted, especially with the None overrides for index/count
|
Diff from mypy_primer, showing the effect of this PR on open source code: hydra-zen (https://github.com/mit-ll-responsible-ai/hydra-zen)
- 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 | memoryview[int] | Path | DataClass_ | <6 more items> | 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[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:2666: error: Incompatible types in assignment (expression has type "tuple[int | float | memoryview[int] | Path | DataClass_ | <6 more items> | None, ...]", variable has type "list[Importable | Callable[P, R] | type[Builds[Importable]] | type[BuildsWithSig[Importable, Any]] | type[BuildsWithSig[type[R], P]] | Any]") [assignment]
+ src/hydra_zen/structured_configs/_implementations.py:2666: error: Incompatible types in assignment (expression has type "tuple[int | float | Path | DataClass_ | type[DataClass_] | ListConfig | DictConfig | Enum | Sequence[HydraSupportedType] | Mapping[Any, HydraSupportedType] | None, ...]", variable has type "list[Importable | Callable[P, R] | type[Builds[Importable]] | type[BuildsWithSig[Importable, Any]] | type[BuildsWithSig[type[R], P]] | Any]") [assignment]
|
3 participants
Reverts #12781
Although it does not implement the full Sequence API at runtime, memoryview is in fact a subclass of Sequence at runtime (through ABC registration). I will work soon on changes in CPython to remedy the discrepancy, but that doesn't solve the problem on existing versions of Python.
I'd prefer to revert to the previous behavior where memoryview is a subclass of Sequence, for a few reasons:
isinstance(..., Sequence)becomes inconsistent between type checkers and the runtime. That's confusing and can lead to soundness issues.memoryview: remove inheritance fromSequence#12781 (comment) convincing. The missing methods are relatively rarely used, not core parts of the Sequence API. Sequence may not appear in the MRO ofmemoryview, but that's hos we handle ABC inheritance throughout typeshed. There wasn't fallout in mypy-primer, but most typechecked code isn't in mypy-primer. Inheritance from ABCs can lead to metaclass problems in type checkers, but removing the ABC from a single builtin class hardly fixes that problem.