sourcery-ai / sourcery

Instant AI code reviews
https://sourcery.ai
MIT License
1.51k stars 65 forks source link

Replace mutable default arguments doesn't respect type annotations #318

Open Gobot1234 opened 1 year ago

Gobot1234 commented 1 year ago

Checklist

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

reka commented 1 year ago

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

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

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'}
Gobot1234 commented 1 year ago

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 commented 1 year ago

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