Page MenuHomePhabricator

Avoid using mutable default parameter values
ClosedPublic

Authored by PiRK on Oct 13 2020, 16:50.

Details

Reviewers
deadalnix
Fabien
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABCa8201635f893: Avoid using mutable default parameter values
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 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.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Owners added a reviewer: Restricted Owners Package.Oct 13 2020, 16:50
PiRK requested review of this revision.Oct 13 2020, 16:50

remove mutable default parameter in abc-transaction-ordering.py

deadalnix requested changes to this revision.Oct 13 2020, 16:56
deadalnix added subscribers: Fabien, deadalnix.

When you add a linter, make sure it runs with arc lint. @Fabien can help you with this.

This revision now requires changes to proceed.Oct 13 2020, 16:56

create a proper linter for this.

PiRK edited the test plan for this revision. (Show Details)
PiRK added inline comments.
test/functional/abc-transaction-ordering.py
148 ↗(On Diff #24610)

I could not find any call to that function that provided new_transactions, so I just removed the argument altogether.

PiRK edited the test plan for this revision. (Show Details)

use raiseLintAtOffset to cleanly show the problematic code
Revert the change to abc-transaction-ordering.py to split this into another diff

PiRK edited the test plan for this revision. (Show Details)
Fabien requested changes to this revision.Oct 14 2020, 15:17
Fabien added inline comments.
arcanist/linter/PythonMutableDefaultLinter.php
50 ↗(On Diff #24613)

If you loop over $matches[1] you get the correct line highligted

This revision now requires changes to proceed.Oct 14 2020, 15:17

loop over matches[1] rather than matches[0]

This revision is now accepted and ready to land.Oct 14 2020, 21:54