Warn about mutable defaults arguments (lists, dicts and sets)

[dnaaun]

Pyright already warns about function calls in default arguments (reportCallInDefaultInitializer), but I would like to suggest that it also warn about mutable default arguments such as:

def func(ls: list[int]=[]) -> None:
    ls.append(len(ls))
    print(ls)

The fact that the default argument expression is evaluated at module load time means that all invocations of the function like func() will use the same reference to a single list. Meaning,

func() # prints [0]
func() # prints [0, 1], which some (many?) might find surpising

This came up in this Hacker News thread, and I suggested Pyright thinking it already warns about such cases, but it doesn't.

It might be worth noting that Javascript's default argument implementation evaluates default arguments at call time, further demonstrating the unpopularity of Python's approach.

[erictraut]

I agree that Python's handling of default arguments is problematic. When I first encountered Python, I assumed it did what all other languages do and allocated the default argument at call time. I was dismayed to learn that it does so at function declaration time.

The challenge with your proposed change is that the Python type system doesn't have a notion of "mutable" vs. "not mutable". A feature like this would need to have a hard-coded "allow list" (or conversely, a "disallow list") of types. That's something we generally try to avoid because such lists are inevitably incomplete resulting in false positive errors. Do you have any thoughts on how such a feature could be implemented without maintaining a hard-coded type list?

[dnaaun]

Yes, I was also thinking that special casing for specific expression patterns, namely exactly the set of expressions that you just listed would strike the right balance in trying to warn the user about mutable defaults while avoiding a high false positive rate.

[erictraut]

I've extended the logic for the reportCallInDefaultInitializer diagnostic check so it also detects list, set and dict expressions. This will be included in the next release.

[dnaaun]

This is great. Thanks! I'm curious, would it be better to include these checks in a separate flag such as reportMutableDefaultArgument, because it is not apparent from the name of the reportCallInDefaultInitializer flag that mutable defaults are also checked?

[erictraut]

There's a maintenance cost for every new diagnostic rule, and deprecating rules is disruptive. I don't think this merits adding a new one. I've updated the documentation to explain that the rule covers list, set and dict expressions.

[Jaakkonen]

Quite many mutable types can be detected by checking if those are Hashable (dict, list and set are those). However that approach would possibly cause problems with normal classes that just don't implement __hash__. Maybe something like var.__class__.__module__ == '__builtin__' and not isinstance(var, Hashable) could work?

[tibdex]

Could reportCallInDefaultInitializer tolerate mutable default arguments when they are annotated as immutable types?

Indeed, if this advice is respected:

pyright/docs/typed-libraries.md

Line 219 in 5422827

As a specific application of the “use the widest type possible” rule, libraries should generally use immutable forms of container types instead of mutable forms (unless the function needs to modify the container). Use `Sequence` rather than `List`, `Mapping` rather than `Dict`, etc. Immutable containers allow for more flexibility because their type parameters are covariant rather than invariant. A parameter that is typed as `Sequence[Union[str, int]]` can accept a `List[int]`, `Sequence[str]`, and a `Sequence[int]`. But a parameter typed as `List[Union[str, int]]` is much more restrictive and accepts only a `List[Union[str, int]]`.

a signature like this:

def increment(numbers: Iterable[int] = []) -> List[int]:
    return [number + 1 for number in numbers]

is safe since Pyright already ensures that the numbers argument (and so the default argument too) won't be mutated.

Pyright even catches subtler unsafe types:

def increment(numbers: Iterable[int], *, value: int = 1) -> List[int]:
    return numbers if value == 0 else [number + value for number in numbers]

by raising the error:

Expression of type "Iterable[int] | list[int]" cannot be assigned to return type "List[int]"
  Type "Iterable[int] | list[int]" cannot be assigned to type "List[int]"
    "Iterable[int]" is incompatible with "List[int]"PylancereportGeneralTypeIssues

Allowing safe usage of mutable default arguments like this has some advantages:

• Shorter code: no need to do a numbers = [] if numbers is None else numbers in the body of the function.
• The default value appears in the signature so it's self documented.

What do you think?

[erictraut]

when they are annotated as immutable types

There is no way in the Python type system to specify that a type is immutable. So, while you happen to know that Iterable implies immutability, a type checker doesn't know this. You might then ask "why don't you just hard-code a list of common built-in types that are immutable?". We generally don't hard-code type information like this because it's not maintainable or scalable. All type information, even for common stdlib types, should come from type annotations.

If you don't want to modify your code to work with the current reportCallInDefaultInitializer check, you can disable it on a per-file or project basis.

[tibdex]

Alright, thanks for your answer.