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

conditional_escape() should return SafeString #1474

Closed
levic opened this issue May 4, 2023 · 0 comments · Fixed by #1575
Closed

conditional_escape() should return SafeString #1474

levic opened this issue May 4, 2023 · 0 comments · Fixed by #1575
Labels
bug Something isn't working

Comments

@levic
Copy link

levic commented May 4, 2023

Bug report

What's wrong

The stub definition for conditional_escape says that it returns a str

The django code follows one of two paths:

  • if already escaped, then call __html__ (this calls into SafeString/SafeData which just returns the same object back).
  • it not escaped then call escape which returns a new SafeString

Running mypy on test.py:

from django.utils.html import conditional_escape
from django.utils.safestring import mark_safe
from django.utils.safestring import SafeString
from typing import TYPE_CHECKING

a = SafeString("<")
b = conditional_escape("<")
c = mark_safe("<")

if TYPE_CHECKING:
    reveal_type(a)
    reveal_type(b)
    reveal_type(c)

print(a.__class__)
print(b.__class__)
print(c.__class__)

mypy output is:

test.py:11: note: Revealed type is "django.utils.safestring.SafeString"
test.py:12: note: Revealed type is "builtins.str"
test.py:13: note: Revealed type is "django.utils.safestring.SafeString"

runtime output is:

<class 'django.utils.safestring.SafeString'>
<class 'django.utils.safestring.SafeString'>
<class 'django.utils.safestring.SafeString'>

How is that should be

mypy output:

test.py:11: note: Revealed type is "django.utils.safestring.SafeString"
test.py:12: note: Revealed type is "django.utils.safestring.SafeString"
test.py:13: note: Revealed type is "django.utils.safestring.SafeString"

System information

  • OS: MacOS
  • python version: 3.10
  • django version: 3.2
  • mypy version: 1.2.0
  • django-stubs version: 4.2.0
  • django-stubs-ext version: 4.2.0

Pedantic Note

Strictly speaking django allows for objects that implement __html__ and are not instances of SafeData.

To be 100% correct the declarations would have to create a protocol for objects that implement __html__, but IMO that's a somewhat obscure edge case. I note also that the existing mark_safe type definition already assumes SafeData so just marking conditional_escape as returning SafeString wouldn't be introducing an issue that doesn't already exist.

@levic levic added the bug Something isn't working label May 4, 2023
GabDug added a commit to GabDug/django-stubs that referenced this issue Jun 24, 2023
sobolevn added a commit that referenced this issue Jun 26, 2023
* Type all utils.* against Django 4.2

* fix: cached_property.name could be None

* fix: SessionMiddleware was not typed properly

* fix: conditional_escape should return SafeString, closes #1474

* chore: remove utils from allowlist

* chore: remove unrelated unused ignores from allowlist for CI

* chore: mark Protocols as type_check_only

* Update scripts/stubtest/allowlist.txt

Co-authored-by: Nikita Sobolev <[email protected]>

* Update django-stubs/utils/deprecation.pyi

Co-authored-by: Nikita Sobolev <[email protected]>

* fix: MultieValueDict should return Self

* fixes from MR review

* chore: close #1269

* chore: remove legacy Self import from _typeshed

* chore: move async with its sync counterpart

* chore: remove Trans from typings

* chore: fix tests

* Apply suggestions from code review

Co-authored-by: Nikita Sobolev <[email protected]>

* chore: use decorator syntax for cached_property

---------

Co-authored-by: Nikita Sobolev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging a pull request may close this issue.

1 participant