Page MenuHomePhabricator

Avoid using mutable default parameter values
ClosedPublic

Authored by PiRK on Tue, Oct 13, 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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

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

deadalnix requested changes to this revision.Tue, Oct 13, 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.Tue, Oct 13, 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.Wed, Oct 14, 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.Wed, Oct 14, 15:17

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

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