Skip to content
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

Issues encountered with SequenceNotStr #21

Open
bluenote10 opened this issue Feb 14, 2024 · 1 comment
Open

Issues encountered with SequenceNotStr #21

bluenote10 opened this issue Feb 14, 2024 · 1 comment

Comments

@bluenote10
Copy link

First of all, thanks for maintaining this repo! It's great to a place that collects such ideas.

I'm opening this issue mainly to avoid going too much off-topic in python/typing#256.

We've experimented with SequenceNotStr a bit more now, and while it helps a little bit, we also encountered some issues with it. Perhaps there are ways to mitigate them? This example demonstrates the issues (I've made it self-contained, so that it can be tested directly on mypy playground):

from __future__ import annotations

from typing import Any, Iterator, Protocol, Sequence, SupportsIndex, TypeVar, overload

_T_co = TypeVar("_T_co", covariant=True)

# Source from https://github.com/python/typing/issues/256#issuecomment-1442633430
# This works because str.__contains__ does not accept object (either in typeshed or at runtime)
class SequenceNotStr(Protocol[_T_co]):
    @overload
    def __getitem__(self, index: SupportsIndex, /) -> _T_co:
        ...

    @overload
    def __getitem__(self, index: slice, /) -> Sequence[_T_co]:
        ...

    def __contains__(self, value: object, /) -> bool:
        ...

    def __len__(self) -> int:
        ...

    def __iter__(self) -> Iterator[_T_co]:
        ...

    def index(self, value: Any, /, start: int = 0, stop: int = ...) -> int:
        ...

    def count(self, value: Any, /) -> int:
        ...

    def __reversed__(self) -> Iterator[_T_co]:
        ...


# Functions for checks

def accept_seq(seq: Sequence[_T_co]) -> None:
    ...

def accept_seq_not_string(seq: SequenceNotStr[_T_co]) -> None:
    ...

# -----------------------------------------------------------------------------
# Issue 1: Slicing does not work.
# -----------------------------------------------------------------------------

"""
It looks like slicing does not work properly. The following usage should probably type check.
"""

def issue_1(seq: SequenceNotStr[_T_co]) -> SequenceNotStr[_T_co]:
    # This errors with: Incompatible return value type (got "Sequence[_T_co]", expected "SequenceNotStr[_T_co]")
    return seq[1:-1]


# -----------------------------------------------------------------------------
# Issue 2: Does not accept range.
# -----------------------------------------------------------------------------

"""
A regular Sequence[T] is expected to accept a `range`, which seems to satisfy the Sequence protocol.
Unfortunately this doesn't seem to be the case for `SequenceNotStr`.
"""

def issue_2() -> None:
    # This works:
    accept_seq(range(4))
    # This errors with: Argument 1 to "accept_seq_not_string" has incompatible type "range"; expected "SequenceNotStr[Never]"
    accept_seq_not_string(range(4))


# -----------------------------------------------------------------------------
# Issue 3: `SequenceNotStr` cannot be used as `Sequence` making it viral.
# -----------------------------------------------------------------------------

"""
Imagine a third-party interface that happens to use `Sequence` (either generically or
specifically with `str`). On our side, we want to work with e.g. a "list of names" and
we want to make sure that `str` does not accidentally get accepted as a list of names.
Therefore, we'd like to use `SequenceNotStr`. However, then we are not able to use any
third party interface that want a `Sequence`.
"""

def issue_3() -> None:
    def third_party_interface(seq: Sequence[_T_co]) -> _T_co:
        raise NotImplementedError()

    # Our type alias for a "sequence of names" avoiding the bug of accepting `str`
    Names = SequenceNotStr[str]

    def do_something_with_names(names: Names) -> None:
        # This errors with: Argument 1 to "third_party_interface" has incompatible type "SequenceNotStr[str]"; expected "Sequence[Never]"
        third_party_interface(names)

A few notes on the issues:

  • Issue 1: This issues may be easy to fix by changing the return type of __getitem__ to return an instance of
    SequenceNotStr. Or do you see any reason not to do so? I can open a tiny PR if desired.
  • Issue 2: So far I haven't found a work-around for that.
  • Issue 3: From a practical perspective this issue seems to be the most annoying, because it basically means that SequenceNotStr is "viral", and should practically be used (almost) everywhere. (Only not is places where the "broken" Sequence semantics are desired -- none for us, because we model support of str explicitly...). In fact, this limitation feels strange, because SequenceNotStr should be a stricter type, no? It should behave like a Sequence, but just exclude str. So shouldn't it be usable in places that expect Sequence?
@JelleZijlstra
Copy link
Collaborator

Issue 1 can indeed be fixed by changing this line:

def __getitem__(self, index: slice, /) -> Sequence[_T_co]: ...
.

Issue 2: pyright was kind enough to tell me the incompatibility. It's because range.index does not take a start and stop argument. We have to type ignore this in typeshed. As a workaround, we could remove these extra arguments from SequenceNotStr.index, but that may break other users who rely on them.

Issue 3 I don't see a solution for. Sequence is a nominal type in the type system and our SequenceNotStr is a structural type, and a structural type is never a nominal type. Note that issue 1 provides a workaround at some runtime cost: you can use [:] to get a Sequence out of your SequenceNotStr.

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

No branches or pull requests

2 participants