Skip to content

builtins.slice: more precise __new__ overloads and let StopT default to StartT. #12904

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

Conversation

randolf-scholz
Copy link
Contributor

@randolf-scholz randolf-scholz commented Oct 25, 2024

Different overloads than #12899, testing mypy primer.

  • added check_slice.py stubtest, testing both slice.__new__ constructor and slice[T] annotation
  • changed StopT to default to StartT instead of Any. For example:
    • slice[int] = slice[int, int, Any] represents finite intervals
    • slice[int | None] = slice[int | None, int | None, Any] arbitrary intervals
  • Added more comprehensive suite of overloads for __new__, removed special casing for int types.

slice.__new__ now follows a very simply rule: any None argument will result in the corresponding Generic Type to be considered as Any.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@randolf-scholz randolf-scholz marked this pull request as ready for review October 25, 2024 12:25
@randolf-scholz randolf-scholz changed the title [test] Slice generic alternative builtins.slice: more precise __new__ overloads and let StopT default to StartT. Oct 25, 2024
@randolf-scholz randolf-scholz marked this pull request as draft October 25, 2024 12:31
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Looks promising!

@randolf-scholz randolf-scholz marked this pull request as ready for review October 25, 2024 12:35

This comment has been minimized.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Nice! Looks like you now just need to update some assertions in the test-case file to use Union[int, Any] rather than Any in order to make the test case pass.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@randolf-scholz
Copy link
Contributor Author

One doubt I have is whether we should change it so that slice[A] = slice[A, A, A] after all.

Without this convention, describing the most commonly used slice type, which I think is

slice[SupportsIndex | None, SupportsIndex | None, SupportsIndex | None]

Probably requires an alias since it is so long.

Moreover, I think we need to clarify whether the collections.abc.Sequence protocol requires __getitem__ to support slice[SupportsIndex | None, SupportsIndex | None, SupportsIndex | None], or if slice[int | None, int | None, int | None] is sufficient. This is likely a critical case as many users want to subclass Sequence, and it shouldn't be too inconvenient to type hint the implementation.

@randolf-scholz
Copy link
Contributor Author

randolf-scholz commented Nov 13, 2024

@AlexWaygood #13008 is the same as this, but defaults step to start | stop, so that slice[SupportsIndex | None] gives us the most commonly used slice type. In this case, people who want to use datetime/timedelta need to annotate with 3 values.

A decision needs to be made between:

  1. Use this patch. For type hinting index-slices there are 3 options:
    1. Guide users to type hint slice[SupportsIndex | None], which means step will not be checked.
    2. Guide users to use an IndexSlice type alias provided by typing_extensions / typing.
    3. Guide users to use their own alias.
  2. Use builtins.slice: more precise __new__ overloads and defaults for StopT and StepT. #13008, and guide users to annotate index-slices with slice[SupportsIndex | None], and users who want to use non-standard slices to provide all 3 type variables.

This comment has been minimized.

Copy link
Contributor

github-actions bot commented Jan 6, 2025

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@srittau
Copy link
Collaborator

srittau commented Feb 28, 2025

Closing in favor of #13008.

@srittau srittau closed this Feb 28, 2025
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.

3 participants