Fix mutable default arguments#5329
Conversation
biswajeetdev
left a comment
There was a problem hiding this comment.
Good catch on both instances. The mutable default argument antipattern in Python means the same list or dict object is shared across all calls, so mutations in one call silently affect subsequent calls — a notoriously hard bug to track down in production.
The messages: Optional[List] = None fix with messages = messages or [] inside the body is the standard corrective pattern. One small thing: messages or [] treats an explicitly passed empty list as falsy and replaces it with a new []. This is fine here since an empty message list is semantically equivalent, but if messages is None: messages = [] is more precise if callers might explicitly pass [] to mean "no messages" in a distinguishable way from not passing the arg at all.
The test using inspect.signature(Completions.create).parameters["messages"].default is None is a nice way to guard this at the API boundary — it will catch any future regression without needing to actually instantiate and mutate the class.
|
Thanks for the review @biswajeetdev! I've reviewed the suggestion about In this specific case, the two are behaviorally equivalent since an explicitly-passed empty list is semantically the same as "no messages" — there's no code path that meaningfully distinguishes between them. That said, I see the style preference argument and am happy to switch to the more explicit Otherwise, would love to get this merged when you have a moment. The mutable default issue, while not actively causing bugs today, is one of those things that silently bites the next person who adds a mutation downstream. |
|
Closing as duplicate — the fix for this was merged via #5302. Thank you for the contribution! |
Summary
Testing