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

capfirst annotation does not appear usable -- or probably anything using _StrOrPromiseT #1626

Closed
asottile-sentry opened this issue Jul 12, 2023 · 7 comments · Fixed by #1841
Labels
bug Something isn't working stubs Issues in stubs files (.pyi)

Comments

@asottile-sentry
Copy link

Bug report

it appears that the _StrOrPromiseT in capfirst decays to Sequence[str] -- probably because mypy infers the union of str | _StrPromise to be Sequence[str] (because str is also a Sequence[str])

What's wrong

the original error comes from this line -- though I've isolated the bug to a standalone script: https://github.com/getsentry/sentry/blob/c19f762c70afa9145efb10e8a524498316dcea4e/src/sentry/web/forms/accounts.py#L79

here's the standalone version:

from __future__ import annotations
from typing import TYPE_CHECKING
from django.utils.translation import gettext_lazy
from django.utils.text import capfirst

if TYPE_CHECKING:
    from django.utils.functional import _StrPromise

s: _StrPromise | str = gettext_lazy('example')
t = capfirst(s)
reveal_type(t)
$ mypy t2.py 
t2.py:10: error: Value of type variable "_StrOrPromiseT" of "capfirst" cannot be "Sequence[str]"  [type-var]
t2.py:11: note: Revealed type is "Union[typing.Sequence[builtins.str], None]"
Found 1 error in 1 file (checked 1 source file)

How is that should be

if I change this instead to an overload it appears to work correctly:

from __future__ import annotations
from typing import TYPE_CHECKING, overload
from django.utils.translation import gettext_lazy
from django.utils.text import capfirst

if TYPE_CHECKING:
    from django.utils.functional import _StrPromise


@overload
def capfirst2(x: None) -> None: ...
@overload
def capfirst2(x: str) -> str: ...
@overload
def capfirst2(x: _StrPromise) -> _StrPromise: ...
def capfirst2(x: str | _StrPromise | None) -> str | _StrPromise | None: return x


s: _StrPromise | str = gettext_lazy('example')
t = capfirst2(s)
reveal_type(t)
$ mypy t.py 
t.py:21: note: Revealed type is "Union[django.utils.functional._StrPromise, builtins.str]"
Success: no issues found in 1 source file

let me know if you'd be interested in a patch for this or if there's some other ideas on how to fix this

System information

  • OS:
  • python version: 3.8.16
  • django version: (old, lol)
  • mypy version: 1.4.1
  • django-stubs version: 4.2.3
  • django-stubs-ext version: 4.2.2
@asottile-sentry asottile-sentry added the bug Something isn't working label Jul 12, 2023
@adamchainz
Copy link
Contributor

Thank you for the clear bug report. I'm a little surprised by Mypy's behaviour here but I guess it has some logic.

A PR would be very welcome. It looks like all the functions in django.utils.text would need such overloads.

@intgr
Copy link
Collaborator

intgr commented Jul 24, 2023

It's pretty surprising to me that _StrPromise subclasses Sequence[str]. The point of _StrPromise was to create a "newtype" that isn't compatible with str itself. I think it follows that it also shouldn't be compatible with Sequence[str].

class _StrPromise(Promise, Sequence[str]):

Explanation from the original PR #689 (comment)

_StrPromise is a subclass of Sequence[str] because this is how builtins.str is typed in typeshed. _StrPromise is defined only because operators don't work well with mypy plugins, while we want methods on a regular str to be all available. These are just the implementation details.

Now this implementation detail is leaking out. I don't understand how subclassing Sequence[str] helps.

@PIG208 as the author of StrPromise logic, any thoughts? Can't we just remove Sequence[str] from bases and reimplement its methods in _StrPromise itself? Any downsides to that that you can think of?

@adamchainz
Copy link
Contributor

It's pretty surprising to me that _StrPromise subclasses Sequence[str].

I had not checked that. Wow, that's confusing... I would also expect we can remove Sequence[str] from the bases.

@intgr
Copy link
Collaborator

intgr commented Jul 24, 2023

Offtopic but maybe helpful. Add django-stubs-ext to your non-dev dependencies, and you can import it from there:

# Instead of this:
if TYPE_CHECKING:
    from django.utils.functional import _StrPromise

# Use this:
from django_stubs_ext import StrPromise

@PIG208
Copy link
Contributor

PIG208 commented Jul 26, 2023

Can't we just remove Sequence[str] from bases and reimplement its methods in _StrPromise itself

I think this is a fine fix. Sequence[str] was originally used to just minimally port typeshed's definition of str to _StrPromise. This is not a hard requirement.

@asottile-sentry
Copy link
Author

it looks like this may have magically fixed with the new mypy inference?

@intgr
Copy link
Collaborator

intgr commented Nov 15, 2023

Thanks for the update. The given test case seems to be fixed in mypy 1.7.0 even with --old-type-inference, so it's probably some other change that fixed it.

But regardless, we should still remove Sequence[str] from bases, I opened PR #1841

@intgr intgr added the stubs Issues in stubs files (.pyi) label Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stubs Issues in stubs files (.pyi)
Development

Successfully merging a pull request may close this issue.

4 participants