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

Recursive sequence type of Sequence[NotAString] should not accept str #18184

Closed
jorenham opened this issue Nov 24, 2024 · 7 comments
Closed

Recursive sequence type of Sequence[NotAString] should not accept str #18184

jorenham opened this issue Nov 24, 2024 · 7 comments
Labels
bug mypy got something wrong topic-recursive-types

Comments

@jorenham
Copy link

jorenham commented Nov 24, 2024

With at least mypy 1.13.0, the false negative can be reproduced with:

from collections.abc import Sequence
from typing import TypeAlias

class Scalar: ...

Vector: TypeAlias = Sequence[Scalar]
Tensor: TypeAlias = Vector | Sequence["Tensor"]

accepted_vector: Vector = [Scalar()]     # true negative
rejected_vector: Vector = "duck"         # true positive: ducks aren't vectors

accepted_tensor: Tensor = [[[Scalar()]]] # true negative
rejected_tensor1: Tensor = object()      # true positive: objects aren't tensors
rejected_tensor2: Tensor = "duck"        # false negative: ducks are tensors...?

originally posted in #731 (comment)

@ilevkivskyi
Copy link
Member

This works as expected (even if surprising) because str is a subclass of Sequence[str]. As an exercise, try writing a code where an actual runtime error happens because of this behavior (the point is that you can't).

@ilevkivskyi ilevkivskyi closed this as not planned Won't fix, can't repro, duplicate, stale Nov 24, 2024
jorenham added a commit to jorenham/optype that referenced this issue Nov 24, 2024
@jorenham
Copy link
Author

jorenham commented Nov 24, 2024

This works as expected (even if surprising) because str is a subclass of Sequence[str].

but str is not assignable to Scalar

edit: your argument assumes an infinite horizon, but within the context of typing, I think that a finite horizon would be more appropriate, analogous to how there's also a limit on the recursion depth at runtime

edit edit: if you'd parametrize str on its length then that will allow you to define an inductive basecase as str[1], and Sequence[str[1]] otherwise; so there's also no need for an infinite horizon

@jorenham
Copy link
Author

As an exercise, try writing a code where an actual runtime error happens because of this behavior (the point is that you can't).

>>> import numpy as np
>>> np.array("not a tensor", np.float64)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: could not convert string to float: 'not a tensor'

@davidhalter
Copy link

@ilevkivskyi I also feel like this is a Mypy issue. I am passing AFAIK all of Mypy's relevant recursion tests (except the ones with --disable-recursive-aliases) and this case seems to behave correct in my implementation.

@jorenham
Copy link
Author

@KotlinIsland any thoughts on this?

@KotlinIsland
Copy link
Contributor

I think its consistent with the concept/spec, but impractical... maybe. I think pyright is wrong/inconsistent here...

let's change how it works in basedmypy 🚀🚀🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong topic-recursive-types
Projects
None yet
Development

No branches or pull requests

5 participants