Ticket #828 (closed defect: fixed)

Opened 9 years ago

Last modified 9 years ago

clean up use of mutable objects as argument defaults

Reported by: jsamuel Owned by: cemeyer
Priority: major Milestone:
Component: multiple components Version: 0.1o
Severity: Medium Keywords:
Cc: armon, cemeyer Blocking:
Blocked By:

Description

The use of mutable objects for function argument default values in python is usually unintentional and bug-prone. Example:

def myfunc(firstarg, secondarg=[]):

as the list in the above example will be a single object persistent across all function calls, so modification affects future calls to the function.

We should clean these up in our codebase. Here are the places where it appears lists and dictionaries are used in such a way:

trunk/seattlelib/deserialize.repy:def deserialize_removeObjects(strIn, partitions=[]):
trunk/seattlelib/dylink.repy:def _default_context(import_context, additional_globals=[]):
trunk/seattlelib/dylink.repy:def dylink_import_module(module,import_context, new_callfunc="import", additional_globals=[]):
trunk/seattlelib/dylink.repy:  def _dy_import_module(module,new_callfunc="import",additional_globals=[]):
trunk/seattlelib/advertise.repy:def advertise_lookup(key, maxvals=100, lookuptype=['central','opendht','DOR'], \
trunk/softwareupdater/test/test_updater_local.py:def runRsyncTest(testtype, updatefolder, otherargs=[]):
trunk/softwareupdater/test/test_rsync.py:def test_rsync(testtype, softwareurl, chgFile=[]):
trunk/multiplexer/src/deserialize.py:def deserialize_removeObjects(strIn, partitions=[]):
trunk/seattlelib/Multiplexer.repy:  def __init__(self, socket, info={}):
trunk/repy/tests/run_tests.py:def exec_repy_script(filename, restrictionsfile, arguments={}, script_args=''):
trunk/multiplexer/src/Multiplexer.py:  def __init__(self, socket, info={}):

The above list intentionally leaves out:

  • trunk/continuousbuild/PyRSS2Gen.py because it's 3rd-party code not part of the actual seattle codebase and changing it would probably risk adding bugs.
  • trunk/autograder/emulab/sshxmlrpc.py because it's 3rd-party code and there doesn't appear to be a bug in the usage (the list is never modified).

We also need to add a mention of this to the style guide.

I'm CC'ing people who I think are the current maintainers of various of the above files so they can jump in and fix them.

Even if the usages aren't buggy, we should change the above code to avoid the risk that someone could later modify the code and make it buggy. That is, we should fix using mutable objects in this way even if there is no bug currently. If there turns out to be a really good reason for the usage, it should be commented (along with a reminder of the risk of using the mutable object).

Change History

Changed 9 years ago by cemeyer

Good catch!

Changed 9 years ago by jsamuel

Style guide updated.

Changed 9 years ago by jsamuel

  • owner changed from jsamuel to cemeyer
  • status changed from new to assigned

Fixed in r3396:

  • trunk/softwareupdater/test/test_rsync.py
  • trunk/softwareupdater/test/test_updater_local.py

Reassigning to cemeyer because I don't think there are any more in the list that I maintain. Feel free to reassign to armon if you finish updating the ones you maintain first.

Changed 9 years ago by armon

Yes, good catch. I didn't think it worked like that. I've updated the Multiplexer, deserialize library, and dylink.

Changed 9 years ago by cemeyer

It looks like advertise and run_tests are all that's left.

Changed 9 years ago by cemeyer

  • status changed from assigned to closed
  • resolution set to fixed

Fixed in r3400:

  • trunk/seattlelib/advertise.repy
  • trunk/repy/tests/run_tests.py

Closing because I think we're done.

Note: See TracTickets for help on using tickets.