sourcery icon indicating copy to clipboard operation
sourcery copied to clipboard

Replace mutable default arguments doesn't respect type annotations

Open Gobot1234 opened this issue 2 years ago • 3 comments

Checklist

  • [x] I have searched the Sourcery documentation for the issue, and found nothing
  • [x] I have checked there are no open bugs referencing the same bug or problem

Description

The annotation for the parameter that is a Mapping, Sequence or Set aren't expected to support mutation so I think this is a false positive.

Code snippet that reproduces issue

from collections.abc import Mapping
def foo(
    self,
    passwords: Mapping[int, str] = {},  
) -> None:
	...

# We suggest making the following changes to Function foo:
# Replace mutable default arguments with None (default-mutable-arg)

Debug Information

Sourcery Version:

v1.0.3

Gobot1234 avatar Jan 23 '23 10:01 Gobot1234

Thanks for reporting it, this is a great question. :+1:

I'm not sure what the right behavior here is. :thinking:

  • The type annotation collections.abc.Mapping indicates that passwords shouldn't be mutated.
  • The {} literal creates a dict. It can be mutated, even if the type annotation is collections.abc.Mapping.

For example, this is possible:

In [1]: from collections.abc import Mapping

In [2]: nrs: Mapping[int, str] = {}

In [3]: type(nrs)
Out[3]: dict

In [4]: nrs[3] = "tres"

In [5]: nrs
Out[5]: {3: 'tres'}

reka avatar Jan 23 '23 18:01 reka

I think if the annotation says it shouldn't be mutated this wouldn't type check without narrowing the type to dict inside the function but I feel like that would never actually be a source of bugs.

Gobot1234 avatar Jan 23 '23 18:01 Gobot1234

As a bit of prior art, dataclasses checks if an argument is "mutable" by checking for a __hash__ method https://github.com/python/cpython/blob/main/Lib/dataclasses.py#L829-L834

Gobot1234 avatar Jan 25 '23 22:01 Gobot1234