Skip to content

clean up use of mutable objects as argument defaults #828

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.

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

Closed
choksi81 opened this issue May 25, 2014 · 6 comments
Closed

clean up use of mutable objects as argument defaults #828

choksi81 opened this issue May 25, 2014 · 6 comments

Comments

@choksi81
Copy link

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

def myfunc(firstarg, secondarg=[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=:


as)):
trunk/seattlelib/dylink.repy:def _default_context(import_context, additional_globals=[dylink_import_module(module,import_context, new_callfunc="import", additional_globals=[](]):
trunk/seattlelib/dylink.repy:def)):
trunk/seattlelib/dylink.repy:  def _dy_import_module(module,new_callfunc="import",additional_globals=[advertise_lookup(key, maxvals=100, lookuptype=['central','opendht','DOR'](]):
trunk/seattlelib/advertise.repy:def), \
trunk/softwareupdater/test/test_updater_local.py:def runRsyncTest(testtype, updatefolder, otherargs=[test_rsync(testtype, softwareurl, chgFile=[](]):
trunk/softwareupdater/test/test_rsync.py:def)):
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).

@choksi81 choksi81 self-assigned this May 25, 2014
@choksi81
Copy link
Author

Author: cemeyer
Good catch!

@choksi81
Copy link
Author

Author: jsamuel
Style guide updated.

@choksi81
Copy link
Author

Author: jsamuel
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.

@choksi81
Copy link
Author

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

@choksi81
Copy link
Author

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

@choksi81
Copy link
Author

Author: cemeyer
Fixed in r3400:
* trunk/seattlelib/advertise.repy
* trunk/repy/tests/run_tests.py

Closing because I think we're done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant