B006 (unsafe fix) : does not fix correctly with @overload

[yoann9344]

     @overload
     @classmethod
     def query(
         cls,
-        my_list: list[Whatever | int] = [],
+        my_list: list[Whatever | int] | None = None,
     ) -> Whatever:
-        ...
+        if my_list is None:
+            my_list = []

     @overload
     @classmethod
     def query(
         cls,
-        my_list: list[Whatever | int] = [],
+        my_list: list[Whatever | int] | None = None,
     ) -> Whatever:
-        ...
+        if my_list is None:
+            my_list = []

     @classmethod
-    def query(cls, session, my_list=[]):
+    def query(cls, session, my_list=None):
+        if my_list is None:
+            my_list = []

It should not touch overloaded functions except to change the right argument :

-        my_list: list[Whatever | int] = [],
+        my_list: list[Whatever | int] | None = None,

But must not add logic in it :

-        ...
+        if my_list is None:
+            my_list = []

So the result should look like this for overloaded methods :

     @overload
     @classmethod
     def query(
         cls,
-        my_list: list[Whatever | int] = [],
+        my_list: list[Whatever | int] | None = None,
     ) -> Whatever:
    ...

NB : this overloaded method has no sens, I removed the unnecessary.

[MichaReiser]

Thanks for reporting this improvement. I summarised the issue again in my own words after I have played with the example for a bit.

The B006 replaces the [] default value with None and inserts the

if <argument> is None:
	<argument> = []

as the first statement in the body which in itself is the semantically correct refactor.

However, inserting the statement is undesired if the function has a stub body. I don't think we necessarily have to test if the function has the @overload signature because changing the body is also undesired in typing stub files.

Playground

[yoann9344]

The B006 replaces the [] default value with None and inserts the

if <argument> is None:
	<argument> = []

as the first statement in the body which in itself is the semantically correct refactor.

Yes that's correct, it also replaces the typing by <initial type> | None

However, inserting the statement is undesired if the function has a stub body. I don't think we necessarily have to test if the function has the @overload signature because changing the body is also undesired in typing stub files.

Well see ! I think it is the best way to handle it ! (to be clear : no statement added in stubs but method signature modified)

Playground

Except at the end there's not a stub body.

class Test:
    @overload
    @classmethod
    def query(
        cls,
        my_list: list[Whatever | int] = [],
    ) -> Whatever:
        ...
    
    @overload
    @classmethod
    def query(
        cls,
        my_list: list[Whatever | int] = [],
    ) -> Whatever:
        ...
    
    @classmethod
    def query(cls, my_list: list[Whatever | int] = []) -> Whatever:
        raise NotImplementedError("")

[Philipp-Thiel]

I added code that identifies all functions, where the body consists only out of a docstring, the ... literal and/or the pass keyword as a stub and doesn't modify the body then. But now that I think more about it, I also find the case where the function body only raises a NotImplementedError compelling. Should that maybe also be identified as a stub?

[charliermarsh]

@Philipp-Thiel - Sure, that seems like a reasonable extension. You can use the SemanticModel to identify the exception type.