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

Class attributes annotated as mutable types when mutability is not required #1992

Closed
samueljsb opened this issue Mar 6, 2024 · 2 comments
Closed
Labels
bug Something isn't working

Comments

@samueljsb
Copy link
Contributor

Bug report

What's wrong

There are many attributes that are annotated here as mutable types (lists, dicts) that don't actually need to be mutable. It would be perfectly valid to use immutable types (tuples, MappingProxyType or frozendicts), but the type checker then complains because the annotation here appear to require mutability.

There are a lot of these, and I don't want to do a half-baked job of listing them exhaustively, so as an example, we can consider TemplateView's extra_context:

from types import MappingProxyType

from django.views import generic


class MyView(generic.TemplateView):
    extra_context = MappingProxyType({
        'foo': 1,
        'bar': 42,
    })
t.py:7: error: Incompatible types in assignment (expression has type "MappingProxyType[str, int]", base class "ContextMixin" defined the type as "dict[str, Any] | None")  [assignment]
Found 1 error in 1 file (checked 1 source file)

How is that should be

I think this case should be annotated as Mapping[str, Any] | None, because a mapping is required but not necessarily a mutable one.

System information

This example was with the following, but I've experienced this problem with various combinations and versions.

  • OS: macOS 14.3.1
  • python version: v3.11.5
  • django version: 5.0.3
  • mypy version: 1.8.0
  • django-stubs version: 4.2.7
  • django-stubs-ext version: 4.2.7
@samueljsb samueljsb added the bug Something isn't working label Mar 6, 2024
@sobolevn
Copy link
Member

sobolevn commented Mar 7, 2024

PR is welcome.

Generally we annotate attributes with their concrete types. Because Django does not provide any promises about how they would be used.

So, if you have any speicific examples, we can decide on a per-case basis.

samueljsb added a commit to samueljsb/django-stubs that referenced this issue Mar 7, 2024
Prior to this change, the `extra_context` attribute of a template veiw
was annotated as a `dict` (or `None`). This means that if a value is
provided, it *must* be a mutable object. Since this is an attribute on
the class, this creates mutable shared state, which is a source of bugs.
It was not possible to use an immutable mapping (`MappingProxyType or
the 3rd-party `frozendict`) without getting type-checking errors.

However, there is no need for this attribute to be mutable. This change
allows *any* mapping type, which allows authors to write template views
with immutable `extra_context` attributes.

Related to typeddjango#1992.
samueljsb added a commit to samueljsb/django-stubs that referenced this issue Mar 7, 2024
Prior to this change, the `extra_context` attribute of a template view
was annotated as a `dict` (or `None`). This means that if a value is
provided, it *must* be a mutable object. Since this is an attribute on
the class, this creates mutable shared state, which is a source of bugs.
It was not possible to use an immutable mapping (`MappingProxyType or
the 3rd-party `frozendict`) without getting type-checking errors.

However, there is no need for this attribute to be mutable. This change
allows *any* mapping type, which allows authors to write template views
with immutable `extra_context` attributes.

Refs typeddjango#1992
samueljsb added a commit to samueljsb/django-stubs that referenced this issue Mar 7, 2024
Prior to this change, the `extra_context` attribute of a template view
was annotated as a `dict` (or `None`). This means that if a value is
provided, it *must* be a mutable object. Since this is an attribute on
the class, this creates mutable shared state, which is a source of bugs.
It was not possible to use an immutable mapping (`MappingProxyType or
the 3rd-party `frozendict`) without getting type-checking errors.

However, there is no need for this attribute to be mutable. This change
allows *any* mapping type, which allows authors to write template views
with immutable `extra_context` attributes.

Refs typeddjango#1992
sobolevn pushed a commit that referenced this issue Mar 8, 2024
Prior to this change, the `extra_context` attribute of a template view
was annotated as a `dict` (or `None`). This means that if a value is
provided, it *must* be a mutable object. Since this is an attribute on
the class, this creates mutable shared state, which is a source of bugs.
It was not possible to use an immutable mapping (`MappingProxyType or
the 3rd-party `frozendict`) without getting type-checking errors.

However, there is no need for this attribute to be mutable. This change
allows *any* mapping type, which allows authors to write template views
with immutable `extra_context` attributes.

Refs #1992
@sobolevn
Copy link
Member

Please, open new issues for any other places you find this problem 👍

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

No branches or pull requests

2 participants