FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

More precise type hints for `slice` constructor by randolf-scholz · Pull Request #12899 · python/typeshed · GitHub

  1. python
  2. typeshed

Repository navigation

More precise type hints for slice constructor - #12899

Closed
randolf-scholz wants to merge 11 commits into
python:mainfrom
randolf-scholz:slice_generic_defaults
ConversationCommits (11)ChecksFiles changed
Closed

More precise type hints for slice constructor#12899
randolf-scholz wants to merge 11 commits into
python:mainfrom
randolf-scholz:slice_generic_defaults

Conversation

randolf-scholz commented Oct 24, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • pin start and step to None if not given.
  • ensure all signatures are positional-only.

Some thoughts:

  • pandas initalizes list via [slice(None)], then modifies with slice(None, n).
  • xarray swaps a slice(None, T) with a slice(T, None).

There are 2 coupled questions:

  1. What are the types of slice, slice[A], slice[A, B] and slice[A, B, C]? This is controlled via the defaults of the TypeVar.
  2. What are the types of slice(None), slice(b), slice(a,b) and slice(a, b, c)? This is controlled via the __new__ overloads.

It seems most common that people want slice(None, x) and slice(x, None) to be compatible, which makes sense as the None simply indicates open-ended slice.

Idea: create a complete classification:

  • Type hint slice should be compatible with the "all-slices":
    • slice(None), slice(None, None) and slice(None, None, None). (⟿ slice[?, ?, ?])
  • Type hint slice[T] should be compatible with:
    • slice(None), slice(None, None) and slice(None, None, None) (⟿ slice[?, ?, ?])
    • slice(t), slice(None, t) and slice(None, t, None). (⟿ slice[?, T, ?])
    • slice(t, None) and slice(t, None, None). (⟿ slice[T, ?, ?])
    • slice(t, t) and slice(t, t, None). (⟿ slice[T, T, ?])
  • Type hint slice[X, Y] should be compatible with:
    • slice(None), slice(None, None) and slice(None, None, None) (⟿ slice[?, ?, ?])
    • slice(y), slice(None, y) and slice(None, y, None). (⟿ slice[?, Y, ?])
    • slice(x, None) and slice(x, None, None) (⟿ slice[X, ?, ?])
    • slice(x, y) and slice(x, y, None). (⟿ slice[X, Y, ?])
  • Type hint slice[X, Y, Z] should be compatible with:
    • slice(None), slice(None, None) and slice(None, None, None). (⟿ slice[?, ?, ?])
    • slice(y), slice(None, y) and slice(None, y, None). (⟿ slice[?, Y, ?])
    • slice(x, None) and slice(x, None, None) (⟿ slice[X, ?, ?])
    • slice(x, y) and slice(x, y, None). (⟿ slice[X, Y, ?])
    • slice(None, None, z) (⟿ slice[?, ?, Z])
    • slice(None, y, z) (⟿ slice[?, Y, Z])
    • slice(x, None, z) (⟿ slice[X, ?, Z])
    • slice(x, y, z) (⟿ slice[X, Y, Z])

This comment has been minimized.

Sign in to view

AlexWaygood commented Oct 24, 2024

Copy link
Copy Markdown
Member

Please do the positional-only fix in its own PR and I will instantly merge it. Then we can evaluate the other changes you'd like to make in isolation and consider what the costs and benefits might be.

randolf-scholz commented Oct 24, 2024

Copy link
Copy Markdown
Contributor Author

@AlexWaygood #12900

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

randolf-scholz commented Oct 24, 2024

Copy link
Copy Markdown
Contributor Author

This line in pandas raises an important issue: we likely want slice to be compatible with range:

elif isinstance(loc, slice):
    step = loc.step if loc.step is not None else 1
    inds.extend(range(loc.start, loc.stop, step))

However, range actually does not support None inputs. So, using a slice to initialize a range is only safe when we know for sure that None of start, stop and step are None.

This issue potentially makes it more sensible to, instead of making start/stop/step be T | None, to instead be precise with the types, but change the __new__ overloads to return slice[T | None] types.

This comment has been minimized.

Sign in to view
AlexWaygood reviewed Oct 25, 2024
View reviewed changes

AlexWaygood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Impressive! There's some opportunities for simplification here.

This is complex enough that I think we could probably do with some test cases in stdlib/@tests/test_cases.

Comment thread CONTRIBUTING.md Outdated
Comment thread stdlib/builtins.pyi Outdated
Comment thread stdlib/builtins.pyi Outdated
Comment thread stdlib/builtins.pyi Outdated
Comment thread stdlib/builtins.pyi Outdated

This comment has been minimized.

Sign in to view
AlexWaygood reviewed Oct 25, 2024
View reviewed changes
Comment thread stdlib/builtins.pyi Outdated
Comment thread stdlib/builtins.pyi Outdated

This comment has been minimized.

Sign in to view
randolf-scholz and others added 2 commits October 25, 2024 12:40
Update stdlib/builtins.pyi
c73c28c 
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
always specify all generic parameters
2be2aa0
randolf-scholz mentioned this pull request Oct 25, 2024
Closed

randolf-scholz commented Oct 25, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@AlexWaygood One thing I am not super sure if I am happy with is that:

s = slice(None, 2)
reveal_type(s)  # `slice[int | None, int | None, Any]` with current overloads
reveal_type(s.stop)  # `int | None` with current overloads

But I think a lot of users would expect that slice(x, None) and slice(None, x) have compatible types, so the __new__ constructors need to respect that. A possible alternative is to let any None input correspond to an Any in the generic type.

I created #12904 to check if it has any difference on mypy primer.

AlexWaygood commented Oct 25, 2024

Copy link
Copy Markdown
Member

But I think a lot of users would expect that slice(x, None) and slice(None, x) have compatible types, so the __new__ constructors need to respect that. A possible alternative is to let any None input correspond to an Any in the generic type.

You can also experiment with Any | None. (Yes, that's a different type to Any!)

github-actions Bot commented Oct 25, 2024

Copy link
Copy Markdown
Contributor

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

xarray (https://github.com/pydata/xarray)
+ xarray/tests/test_backends.py: note: In member "test_write_region_mode" of class "ZarrBase":
+ xarray/tests/test_backends.py:3019: error: No overload variant of "to_zarr" of "Dataset" matches argument types "Any", "object", "Any", "dict[str, Any]"  [call-overload]
+ xarray/tests/test_backends.py:3019: note: Possible overload variants:
+ xarray/tests/test_backends.py:3019: note:     def to_zarr(self, store: MutableMapping[Any, Any] | str | PathLike[str] | None = ..., chunk_store: MutableMapping[Any, Any] | str | PathLike[Any] | None = ..., mode: Literal['w', 'w-', 'a', 'a-', 'r+', 'r'] | None = ..., synchronizer: Any = ..., group: str | None = ..., encoding: Mapping[Any, Any] | None = ..., *, compute: Literal[True] = ..., consolidated: bool | None = ..., append_dim: Hashable | None = ..., region: Mapping[str, slice[Any, Any, Any] | Literal['auto']] | Literal['auto'] | None = ..., safe_chunks: bool = ..., storage_options: dict[str, str] | None = ..., zarr_version: int | None = ..., zarr_format: int | None = ..., write_empty_chunks: bool | None = ..., chunkmanager_store_kwargs: dict[str, Any] | None = ...) -> ZarrStore
+ xarray/tests/test_backends.py:3019: note:     def to_zarr(self, store: MutableMapping[Any, Any] | str | PathLike[str] | None = ..., chunk_store: MutableMapping[Any, Any] | str | PathLike[Any] | None = ..., mode: Literal['w', 'w-', 'a', 'a-', 'r+', 'r'] | None = ..., synchronizer: Any = ..., group: str | None = ..., encoding: Mapping[Any, Any] | None = ..., *, compute: Literal[False], consolidated: bool | None = ..., append_dim: Hashable | None = ..., region: Mapping[str, slice[Any, Any, Any] | Literal['auto']] | Literal['auto'] | None = ..., safe_chunks: bool = ..., storage_options: dict[str, str] | None = ..., zarr_version: int | None = ..., zarr_format: int | None = ..., write_empty_chunks: bool | None = ..., chunkmanager_store_kwargs: dict[str, Any] | None = ...) -> Any
+ xarray/tests/test_backends.py:3019: error: Argument 1 to "isel" of "Dataset" has incompatible type "object"; expected "Mapping[Any, Any] | None"  [arg-type]

AlexWaygood reviewed Oct 25, 2024
View reviewed changes
Comment thread stdlib/builtins.pyi

randolf-scholz commented Oct 25, 2024

Copy link
Copy Markdown
Contributor Author

Closing in favor of #12904 due to #12899 (comment)

randolf-scholz mentioned this pull request Mar 19, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

AlexWaygood AlexWaygood left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL