Make slice generic #11637

Sachaa-Thanasius · 2024-03-20T14:18:15Z

Mainly doing this out of curiosity to see the primer hits.

EDIT: Would close #8647.

Want to see the primer hits.

JelleZijlstra · 2024-03-20T18:39:16Z

cc @cdce8p I think the primer hits suggest a mypy bug where it looks up "_StartT" in the wrong scope.

cdce8p · 2024-03-20T22:17:46Z

cc @cdce8p I think the primer hits suggest a mypy bug where it looks up "_StartT" in the wrong scope.

Recursive TypeVar defaults was one of the things that didn't make it into 1.9.0 unfortunately. I did do some work on it already, but that isn't released yet: python/mypy#16878. Don't know if it's enough for slice yet though.

If you're interested, maybe that's also something for @AlexWaygood, a few weeks back I created a custom repo to upload dev releases of mypy to PyPI under the mypy-dev project name. They always track a mypy commit upstream and are great to test / use the lasted version (even in production if you feel like it). mypy --version still works as expected. I originally created it as it was unclear when 1.9.0 would actually be released. In the past the release frequency was also a bit more unpredictable.

https://github.com/cdce8p/mypy-dev
https://github.com/cdce8p/mypy-dev/releases

Sachaa-Thanasius · 2024-03-20T23:22:49Z

I just assumed my attempt at implementation was wrong and gave up. Should this be reopened, then?

JelleZijlstra · 2024-03-20T23:31:34Z

Thanks @cdce8p, I forgot that PR didn't make it into 1.9. Hopefully we'll have another release soon. Your mypy-dev project is interesting; maybe that's even something we could do on mypy itself. For example, we could have a weekly cron job uploading alpha releases.

@Sachaa-Thanasius I think we may be able to merge this PR after mypy 1.10 is released (when that will happen, I don't know). You can leave it open in the meantime and mark it as deferred. With some hackery it may be possible to point typeshed's CI at the mypy master branch so we can test earlier whether everything works as expected.

Sachaa-Thanasius · 2024-03-21T01:27:28Z

Understood. Should slice be changed in the runtime cpython as well to support __class_getitem__ via GenericAlias, or is this purely meant to be a typeshed thing? Not something I considered when originally opening this, but now that this actually has a chance of going somewhere . . .

Sachaa-Thanasius · 2024-03-21T01:31:34Z

Also, sidenote: The pyright errors specifically for slice.stop and and the first overload of slice.__new__ (with text like the following) are the result of a bug that was fixed earlier today, I think:

Type parameter "_StopT" has a default type that refers to one or more type variables that are out of scope
    Type variable "_StartT" is not in scope

cdce8p · 2024-03-21T02:38:57Z

With some hackery it may be possible to point typeshed's CI at the mypy master branch so we can test earlier whether everything works as expected.

@Sachaa-Thanasius You could try to replace the mypy version listed in requirements-tests.txt.

typeshed/requirements-tests.txt

Line 8 in 5c75292

mypy==1.9.0

-mypy==1.9.0
+mypy-dev==1.10.0a2

That release tracks python/mypy@b013cc0 which was commited fairly recently to mypy master (5 days ago).

Update: Seems like the mypy primer doesn't work for it, unfortunately.

Sachaa-Thanasius · 2024-04-24T16:35:02Z

Well, that's far more promising than before. Still not sure if this needs to be reflected in cpython by adding a __class_getitem__ to slice.

srittau · 2024-04-24T16:44:02Z

I've removed the Deferred label. I've just looked at spark primer output. The problem is on their side, since they reuse the start variable for different types.

Sachaa-Thanasius · 2024-04-24T17:24:26Z

The error that pyright is hitting is that the usage of itertools.starmap doesn't resolve the types of slice.

# Truncated setup.

from collections.abc import Iterator, Sequence
from itertools import combinations, repeat, starmap
from operator import getitem
from typing import TypeVar, cast

_T = TypeVar("_T")


# Actual function

def subslices(seq: Sequence[_T]) -> Iterator[Sequence[_T]]:
    slices = starmap(slice, combinations(range(len(seq) + 1), 2))
    reveal_type(slices)  # Type of "slices" is "starmap[slice[_StartT@slice, _StopT@slice, _StepT@slice]]"

    # Adding this cast removes the error on the final line:
    # slices = cast(starmap[slice[int, int, int | None]], slices)

    # And using a generator instead of starmap removes the error on the final line as well:
    # slices = (slice(*args) for args in combinations(range(len(seq) + 1), 2))

    return map(getitem, repeat(seq), slices)  # Long error.

Full playground example here to recreate the conditions of the error.

Might be more of a deficiency in the typing of starmap (or something else with pyright’s analysis) than anything with slice. Not sure how to fix it though.

hamdanal · 2024-04-27T11:42:50Z

I might be missing something obvious but it is not clear to me why the type of stop has to default to the type of start. At runtime start is None if it is not passed to the constructor:

$ cat t.py
from typing_extensions import reveal_type

s = slice(10)
reveal_type(s)
reveal_type(s.start)
reveal_type(s.stop)
reveal_type(s.step)
$ python t.py
Runtime type is 'slice'
Runtime type is 'NoneType'
Runtime type is 'int'
Runtime type is 'NoneType'

Similarly, the type of step should default to None.

So I would expect the definitions of the type variables to be:

_StartT = TypeVar("_StartT", default=None)
_StopT = TypeVar("_StopT", default=int)
_StepT = TypeVar("_StepT", default=None)

Sachaa-Thanasius · 2024-05-03T15:24:05Z

I won’t lie; I just copy-pasted the example implementation from PEP 696 without much thought, since this PR wasn’t originally opened with the intention of being merged. Haven’t double-checked since. Definitely will take another look though.

srittau · 2024-05-10T09:52:59Z

stdlib/builtins.pyi

+_StartT = TypeVar("_StartT", default=int)
+_StopT = TypeVar("_StopT", default=_StartT)
+_StepT = TypeVar("_StepT", default=int | None)


Maybe as a very first step, try using Any as default. We could change this later, but this should allow explicit annotations without regressions.

@hamdanal

Courtesy of the feedback from @hamdanal.

Sachaa-Thanasius · 2024-06-03T10:33:30Z

Maybe I'm misreading, but some of these errors indicate that mypy is using the typevar defaults as the typevar bounds as well, which wouldn't be correct afaik.

EDIT: Removed a comment about the one pyright error; I'm not sure how that works right now.

hamdanal · 2024-06-04T19:58:04Z

I'd go with @srittau's idea of setting defaults to Any #11637 (comment). This will allow users to parametrize slice without any regressions and we can always change the defaults later if needed.

Co-authored-by: Sebastian Rittau <srittau@rittau.biz>

hamdanal · 2024-06-04T20:08:34Z

One more thing, shouldn't the type variables be covariant?

Sachaa-Thanasius · 2024-06-04T20:19:39Z

One more thing, shouldn't the type variables be covariant?

I feel like I don't know enough about how slices are used beyond the basics to know the answer to this, tbh. More than willing to defer to someone else's knowledge.

JelleZijlstra · 2024-06-04T20:47:33Z

Determining variance doesn't really require knowing how the class is used. Roughly speaking, a type parameter can be covariant if it's only used in method return types and contravariant if it's only used in parameter types. Here the typevars are only used in return types (read-only properties), so they can be covariant.

Type checkers will tell you if you use a covariant or contravariant typevar where you should have used an invariant one.

srittau · 2024-06-04T21:01:16Z

Covariance might help with some of the primer hits. I also don't understand why mypy wants explicit annotations in the test cases.

github-actions · 2024-06-05T10:34:40Z

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

manticore (https://github.com/trailofbits/manticore)
- tests/wasm/json2mc.py:103: note:     def __getitem__(self, slice, /) -> list[Any]
+ tests/wasm/json2mc.py:103: note:     def __getitem__(self, slice[Any, Any, Any], /) -> list[Any]

pandas (https://github.com/pandas-dev/pandas)
+ pandas/core/array_algos/transforms.py:41: error: Argument 2 to "slice" has incompatible type "int"; expected "None"  [arg-type]
+ pandas/core/algorithms.py:1372: error: Argument 2 to "slice" has incompatible type "int"; expected "None"  [arg-type]
+ pandas/core/algorithms.py:1372: error: Argument 1 to "slice" has incompatible type "int"; expected "None"  [arg-type]
+ pandas/core/algorithms.py:1372: error: Argument 2 to "slice" has incompatible type "None"; expected "int"  [arg-type]
+ pandas/core/algorithms.py:1383: error: Argument 1 to "slice" has incompatible type "None"; expected "int"  [arg-type]
+ pandas/core/algorithms.py:1383: error: Argument 2 to "slice" has incompatible type "int"; expected "None"  [arg-type]
+ pandas/core/algorithms.py:1387: error: Argument 2 to "slice" has incompatible type "int"; expected "None"  [arg-type]
+ pandas/core/algorithms.py:1387: error: Argument 1 to "slice" has incompatible type "int"; expected "None"  [arg-type]
+ pandas/core/algorithms.py:1387: error: Argument 2 to "slice" has incompatible type "None"; expected "int"  [arg-type]

kornia (https://github.com/kornia/kornia)
+ kornia/contrib/models/sam/architecture/mask_decoder.py:92: error: Argument 2 to "slice" has incompatible type "int"; expected "None"  [arg-type]

werkzeug (https://github.com/pallets/werkzeug)
- tests/test_wrappers.py:522: note:     def __setitem__(self, slice, Iterable[tuple[str, str]], /) -> None
+ tests/test_wrappers.py:522: note:     def __setitem__(self, slice[Any, Any, Any], Iterable[tuple[str, str]], /) -> None
- tests/test_wrappers.py:563: note:     def __setitem__(self, slice, Iterable[tuple[str, str]], /) -> None
+ tests/test_wrappers.py:563: note:     def __setitem__(self, slice[Any, Any, Any], Iterable[tuple[str, str]], /) -> None

operator (https://github.com/canonical/operator)
- ops/framework.py:1291: note:          def __getitem__(self, slice, /) -> MutableSequence[Any]
+ ops/framework.py:1291: note:          def __getitem__(self, slice[Any, Any, Any], /) -> MutableSequence[Any]
- ops/framework.py:1291: note:          def __getitem__(self, slice, /) -> Sequence[Any]
+ ops/framework.py:1291: note:          def __getitem__(self, slice[Any, Any, Any], /) -> Sequence[Any]
- ops/framework.py:1294: note:          def __setitem__(self, slice, Iterable[Any], /) -> None
+ ops/framework.py:1294: note:          def __setitem__(self, slice[Any, Any, Any], Iterable[Any], /) -> None
- ops/framework.py:1298: note:          def __delitem__(self, slice, /) -> None
+ ops/framework.py:1298: note:          def __delitem__(self, slice[Any, Any, Any], /) -> None

speedrun.com_global_scoreboard_webapp (https://github.com/Avasam/speedrun.com_global_scoreboard_webapp)
- backend/api/tournament_scheduler_api.py:327: note:     def __getitem__(self, slice, /) -> list[Any]
+ backend/api/tournament_scheduler_api.py:327: note:     def __getitem__(self, slice[Any, Any, Any], /) -> list[Any]
- backend/api/tournament_scheduler_api.py:331: note:     def __getitem__(self, slice, /) -> list[Any]
+ backend/api/tournament_scheduler_api.py:331: note:     def __getitem__(self, slice[Any, Any, Any], /) -> list[Any]
- backend/api/tournament_scheduler_api.py:335: note:     def __getitem__(self, slice, /) -> list[Any]
+ backend/api/tournament_scheduler_api.py:335: note:     def __getitem__(self, slice[Any, Any, Any], /) -> list[Any]

ibis (https://github.com/ibis-project/ibis)
- ibis/selectors.py:646: error: Item "slice" of "slice | Iterable[int | str]" has no attribute "__iter__" (not iterable)  [union-attr]
+ ibis/selectors.py:646: error: Item "slice[Any, Any, Any]" of "slice[Any, Any, Any] | Iterable[int | str]" has no attribute "__iter__" (not iterable)  [union-attr]
- ibis/selectors.py:649: error: Item "Iterable[int | str]" of "slice | Iterable[int | str]" has no attribute "start"  [union-attr]
+ ibis/selectors.py:649: error: Item "Iterable[int | str]" of "slice[Any, Any, Any] | Iterable[int | str]" has no attribute "start"  [union-attr]
- ibis/selectors.py:650: error: Item "Iterable[int | str]" of "slice | Iterable[int | str]" has no attribute "stop"  [union-attr]
+ ibis/selectors.py:650: error: Item "Iterable[int | str]" of "slice[Any, Any, Any] | Iterable[int | str]" has no attribute "stop"  [union-attr]
- ibis/selectors.py:651: error: Item "Iterable[int | str]" of "slice | Iterable[int | str]" has no attribute "step"  [union-attr]
+ ibis/selectors.py:651: error: Item "Iterable[int | str]" of "slice[Any, Any, Any] | Iterable[int | str]" has no attribute "step"  [union-attr]

jax (https://github.com/google/jax)
+ jax/_src/core.py:1956: error: Argument 1 to "slice" has incompatible type "None"; expected "int"  [arg-type]
+ jax/_src/numpy/lax_numpy.py:814: error: Argument 2 to "slice" has incompatible type "int"; expected "None"  [arg-type]
+ jax/experimental/sparse/bcoo.py:1980: error: Argument 2 to "slice" has incompatible type "int"; expected "None"  [arg-type]
+ jax/experimental/sparse/bcoo.py:1981: error: Argument 2 to "slice" has incompatible type "int"; expected "None"  [arg-type]
+ jax/experimental/sparse/bcoo.py:1985: error: Argument 2 to "slice" has incompatible type "int"; expected "None"  [arg-type]

discord.py (https://github.com/Rapptz/discord.py)
- discord/http.py:231: note:     def __setitem__(self, slice, Iterable[Embed], /) -> None
+ discord/http.py:231: note:     def __setitem__(self, slice[Any, Any, Any], Iterable[Embed], /) -> None

streamlit (https://github.com/streamlit/streamlit)
- lib/tests/streamlit/elements/lib/column_config_utils_test.py:382:30: error: Invalid index type "str" for "Union[ColumnConfig, None, str]"; expected type "Union[SupportsIndex, slice]"  [index]
+ lib/tests/streamlit/elements/lib/column_config_utils_test.py:382:30: error: Invalid index type "str" for "Union[ColumnConfig, None, str]"; expected type "Union[SupportsIndex, slice[Any, Any, Any]]"  [index]
- lib/tests/streamlit/runtime/state/query_params_test.py:264:37: note:         def __getitem__(self, slice, /) -> Tuple[Any, ...]
+ lib/tests/streamlit/runtime/state/query_params_test.py:264:37: note:         def __getitem__(self, slice[Any, Any, Any], /) -> Tuple[Any, ...]

srittau · 2024-06-05T10:57:46Z

Looking at the primer hits:

pandas does variations of this:
```
  axis_indexer = [slice(None)] * values.ndim
  if periods > 0:
      axis_indexer[axis] = slice(None, periods)
  else:
      axis_indexer[axis] = slice(periods, None)
```
This will need an explicit annotation of axis_indexer. Not great, but not unheard of for lists. I also expect that collecting slices into a list is a fairly uncommon use, outside of pandas.
kornia is collecting the slice in a variable, before using it. Will also need an explicit annotation.

jax hit number 1 is a bit more problematic:

slices = tuple(slice(int(d._data)) if type(d) is DArray and
                   type(d.dtype) is bint else slice(None) for d in self.shape)

The other jax hits need explicit annotations for lists.

While this introduces a bit of churn in more complex/unorthodox uses of slice, I'm +1 on this change if we can figure out why the mypy tests fail.

hamdanal · 2024-06-06T19:18:24Z

I don't know why the mypy test is failing but I figured out how to make it work. If we decouple the class scoped type variables from the __new__ method scoped type variables mypy will be able to figure out how to handle them and the failing test passes. Here is a patch:

diff --git a/stdlib/builtins.pyi b/stdlib/builtins.pyi
index 6e26bab06..bcc44c0c5 100644
--- a/stdlib/builtins.pyi
+++ b/stdlib/builtins.pyi
@@ -75,6 +75,7 @@ if sys.version_info >= (3, 9):
     from types import GenericAlias

 _T = TypeVar("_T")
+_R = TypeVar("_R")
 _T_co = TypeVar("_T_co", covariant=True)
 _T_contra = TypeVar("_T_contra", contravariant=True)
 _R_co = TypeVar("_R_co", covariant=True)
@@ -925,9 +926,11 @@ class slice(Generic[_StartT, _StopT, _StepT]):
     @property
     def stop(self) -> _StopT: ...
     @overload
-    def __new__(cls, stop: _StopT, /) -> Self: ...
+    def __new__(cls, stop: _S, /) -> slice[Any, _S]: ...
     @overload
-    def __new__(cls, start: _StartT, stop: _StopT, step: _StepT = ..., /) -> Self: ...
+    def __new__(cls, start: _R, stop: _S, /) -> slice[_R, _S]: ...
+    @overload
+    def __new__(cls, start: _R, stop: _S, step: _T, /) -> slice[_R, _S, _T]: ...
     def __eq__(self, value: object, /) -> bool: ...
     __hash__: ClassVar[None]  # type: ignore[assignment]
     def indices(self, len: SupportsIndex, /) -> tuple[int, int, int]: ...

I don't see a downside to returning slice instead of Self as the class is marked with final.

Sachaa-Thanasius added 2 commits March 20, 2024 10:17

Make slice generic

9628936

Want to see the primer hits.

Fix typo in _StartT as default for _StopT

9588575

Sachaa-Thanasius closed this Mar 20, 2024