8000 tests: Avoid common Python default parameter gotcha when mutable dict/list:s are used as default parameter values by practicalswift · Pull Request #16726 · bitcoin/bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

tests: Avoid common Python default parameter gotcha when mutable dict/list:s are used as default parameter values #16726

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Aug 28, 2019
Merged

tests: Avoid common Python default parameter gotcha when mutable dict/list:s are used as default parameter values #16726

merged 2 commits into from
Aug 28, 2019

Conversation

practicalswift
Copy link
Contributor

Avoid common Python default parameter gotcha when mutable dict/list:s are used as default parameter values.

Examples of this gotcha caught during review:

Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python:

>>> def f(i, j=[], k={}):
...     j.append(i)
...     k[i] = True
...     return j, k
...
>>> f(1)
([1], {1: True})
>>> f(1)
([1, 1], {1: True})
>>> f(2)
([1, 1, 2], {1: True, 2: True})

In contrast to:

>>> def f(i, j=None, k=None):
...     if j is None:
...         j = []
...     if k is None:
...         k = {}
...     j.append(i)
...     k[i] = True
...     return j, k
...
>>> f(1)
([1], {1: True})
>>> f(1)
([1], {1: True})
>>> f(2)
([2], {2: True})

The latter is typically the intended behaviour.

This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-)

@fanquake fanquake added the Tests label Aug 26, 2019
@Sjors
Copy link
Member
Sjors commented Aug 27, 2019

Oh Python... ACK e4f4ea4. Testing tip: swap the two commits.

@fanquake fanquake requested a review from maflcko August 28, 2019 03:49
promag
promag reviewed Aug 28, 2019
View reviewed changes
maflcko pushed a commit that referenced this pull request Aug 28, 2019
Merge #16726: tests: Avoid common Python default parameter gotcha whe…
cc40b55 
…n mutable dict/list:s are used as default parameter values

e4f4ea4 lint: Catch use of [] or {} as default parameter values in Python functions (practicalswift)
25dd867 Avoid using mutable default parameter values (practicalswift)

Pull request description:

  Avoid common Python default parameter gotcha when mutable `dict`/`list`:s are used as default parameter values.

  Examples of this gotcha caught during review:
  * #16673 (comment)
  * #14565 (comment)

  Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python:

  ```
  >>> def f(i, j=[], k={}):
  ...     j.append(i)
  ...     k[i] = True
  ...     return j, k
  ...
  >>> f(1)
  ([1], {1: True})
  >>> f(1)
  ([1, 1], {1: True})
  >>> f(2)
  ([1, 1, 2], {1: True, 2: True})
  ```

  In contrast to:

  ```
  >>> def f(i, j=None, k=None):
  ...     if j is None:
  ...         j = []
  ...     if k is None:
  ...         k = {}
  ...     j.append(i)
  ...     k[i] = True
  ...     return j, k
  ...
  >>> f(1)
  ([1], {1: True})
  >>> f(1)
  ([1], {1: True})
  >>> f(2)
  ([2], {2: True})
  ```

  The latter is typically the intended behaviour.

  This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-)

ACKs for top commit:
  Sjors:
    Oh Python... ACK e4f4ea4. Testing tip: swap the two commits.

Tree-SHA512: 56e14d24fc866211a20185c9fdb274ed046c3aed2dc0e07699e58b6f9fa3b79f6d0c880fb02d72b7fe5cc5eb7c0ff6da0ead33123344e1a872209370c2e49e3f
@maflcko maflcko merged commit e4f4ea4 into bitcoin:master Aug 28, 2019
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 28, 2019
@sidhujag
Merge bitcoin#16726: tests: Avoid common Python default parameter got…
20a512a 
…cha when mutable dict/list:s are used as default parameter values

e4f4ea4 lint: Catch use of [] or {} as default parameter values in Python functions (practicalswift)
25dd867 Avoid using mutable default parameter values (practicalswift)

Pull request description:

  Avoid common Python default parameter gotcha when mutable `dict`/`list`:s are used as default parameter values.

  Examples of this gotcha caught during review:
  * bitcoin#16673 (comment)
  * bitcoin#14565 (comment)

  Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python:

  ```
  >>> def f(i, j=[], k={}):
  ...     j.append(i)
  ...     k[i] = True
  ...     return j, k
  ...
  >>> f(1)
  ([1], {1: True})
  >>> f(1)
  ([1, 1], {1: True})
  >>> f(2)
  ([1, 1, 2], {1: True, 2: True})
  ```

  In contrast to:

  ```
  >>> def f(i, j=None, k=None):
  ...     if j is None:
  ...         j = []
  ...     if k is None:
  ...         k = {}
  ...     j.append(i)
  ...     k[i] = True
  ...     return j, k
  ...
  >>> f(1)
  ([1], {1: True})
  >>> f(1)
  ([1], {1: True})
  >>> f(2)
  ([2], {2: True})
  ```

  The latter is typically the intended behaviour.

  This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-)

ACKs for top commit:
  Sjors:
    Oh Python... ACK e4f4ea4. Testing tip: swap the two commits.

Tree-SHA512: 56e14d24fc866211a20185c9fdb274ed046c3aed2dc0e07699e58b6f9fa3b79f6d0c880fb02d72b7fe5cc5eb7c0ff6da0ead33123344e1a872209370c2e49e3f
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 15, 2020
@practicalswift @PiRK
Avoid using mutable default parameter values
a820163 
Summary:
Using mutable default arguments (lists, dicts) in python is usually a very bad idea.
https://florimond.dev/blog/articles/2018/08/python-mutable-defaults-are-the-source-of-all-evil/

This fixes a few instances in the main CI test directory, and adds a linter to detect new cases.
I found some more cases in `contrib/buildbot` and `contrib/teamcity`, that I would like to address in a separate diff because they are a bit trickier to investigate and harder to debug.

Backport of Core [[bitcoin/bitcoin#16726 | PR16726]]

Test Plan:
`ninja && ninja check-functional`

`arc lint --everything`, to make sure there are no false positive.

Example of detection (mutable default parameter added on purpose):

```>>> Lint for test/functional/abc-transaction-ordering.py:

   Warning  (PYTHON_MUTABLE_DEFAULT1) Mutable default arguments should generally not be used in python.
    Found mutable default argument in function.

              49         self.blocks = {}
              50         self.extra_args = [['-whitelist=127.0.0.1']]
              51
    >>>       52     def add_transactions_to_block(self, block, tx_list=[]):
              53         [tx.rehash() for tx in tx_list]
              54         block.vtx.extend(tx_list)
              55
```

I also tested it with dictionaries or set literals, and functions with parameters on multiple lines.

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, Fabien

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, Fabien

Subscribers: deadalnix, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D7910
@practicalswift practicalswift deleted the python-mutable-default-parameter-values branch April 10, 2021 19:38
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@promag promag promag left review comments

@maflcko maflcko Awaiting requested review from maflcko

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@practicalswift @Sjors @promag @maflcko @fanquake
0