Default values should be immutable

Nope. Because `{}` is mutable, if it's set as a default argument it "sticks" around for the lifecycle of the function (so until the server is shut down), and if `context` is modified anywhere within the function (or within the functions it calls itself) then next time around the key set in it will appear again:

>>> def foo(k, v, context={}):
... print context
... context[k] = v
... print context
...
>>> foo('a','b')
{}
{'a': 'b'}
>>> foo('c','d')
{'a': 'b'}
{'a': 'b', 'c': 'd'}
>>> foo('e','f')
{'a': 'b', 'c': 'd'}
{'a': 'b', 'c': 'd', 'e': 'f'}

This issue can create pretty subtle bugs (though it's the worst when it impacts appended/extended lists).

In fact, any instance of `context={}` should very much be replaced by `context=None`, not the other way around.

> In fact, any instance of `context={}` should very much be replaced by `context=None`, not the other way around.
Thanks for the clarification Xavier, I'm marking this bug "confirmed" so this can be done eventually.
Lionel.

I suppose this also applies to cases when arguments have an empty list ([]) as a default value doesn't it?
Lionel

Hi

I launch a check on addons module in 5.0, see the list of context that can be replace.

account/project/project.py:37: def _credit_calc(self, cr, uid, ids, name, arg, context={}):
account/project/project.py:52: def _debit_calc(self, cr, uid, ids, name, arg, context={}):
account/project/project.py:68: def _balance_calc(self, cr, uid, ids, name, arg, context={}):
account/project/project.py:114: def _quantity_calc(self, cr, uid, ids, name, arg, context={}):
account/project/project.py:149: def name_get(self, cr, uid, ids, context={}):
account/project/project.py:165: def _get_company_currency(self, cr, uid, ids, field_name, arg, context={}):
account/project/project.py:196: def _default_company(self, cr, uid, context={}):
account/project/project.py:225: def copy(self, cr, uid, id, default=None, context={}):
account/company.py:52: def get_message(self,cr,uid,context={}):
account/account.py:49: def compute(self, cr, uid, id, value, date_ref=False, context={}):
account/account.py:97: def _check_percent(self, cr, uid, ids, context={}):
account/account.py:129:def _code_get(self, cr, uid, context={}):
account/account.py:184: def _get_children_and_consol(self, cr, uid, ids, context={}):
account/account.py:195: def __compute(self, cr, uid, ids, field_names, arg, context={}, query=''):
account/account.py:228: def _get_account_values(self, cr, uid, id, accounts, field_names, context={}):
account/account.py:247: def _get_company_currency(self, cr, uid, ids, field_name, arg, context={}):
account/account.py:253: def _get_child_ids(self, cr, uid, ids, field_name, arg, context={}):
account/account.py:311: def _default_company(self, cr, uid, context={}):
account/account.py:380: def name_get(self, cr, uid, ids, context={}):
account/account.py:392: def copy(self, cr, uid, id, default={}, context={}, done_list=[], local=False):
account/account.py:431: def unlink(self, cr, uid, ids, context={}):
account/account.py:449: def _col_get(self, cr, user, context={}):
account/account.py:502: def create(self, cr, uid, vals, context={}):
account/account.py:557: def create_period3(self,cr, uid, ids, context={}):
account/account.py:560: def create_period(self,cr, uid, ids, context={}, interval=1):
account/account.py:579: def find(self, cr, uid, dt=None, exception=True, context={}):
account/account.py:609: def _check_duration(self,cr,uid,ids,context={}):
account/account.py:615: def _check_year_limit(self,cr,uid,ids,context={}):
account/account.py:637: def next(self, cr, uid, period, step, context={}):
account/account.py:643: def find(self, cr, uid, dt=None, context={}):
account/account.py:668: def _icon_get(self, cr, uid, ids, field_name, arg=None, context={}):
account/account.py:688: def _check(self, cr, uid, ids, context={}):
account/account.py:696: def write(self, cr, uid, ids, vals, context={}):
account/account.py:700: def create(self, cr, uid, vals, context={}):
account/account.py:707: def unlink(self, cr, uid, ids, context={}):
account/account.py:861: def button_cancel(self, cr, uid, ids, context={}):
account/account.py:869: def write(self, cr, uid, ids, vals, cont...

Wow, I hadn't noticed this weird 'feature' of Python that can cause so much havoc.

It isn't even listed on this Strangest language feature collection: http://stackoverflow.com/questions/1995113

Maybe this is the root cause of some not-freeing-memory problems I have seen when doing batch imports into OpenERP. After all, if you pass a BrowseObject into the context of a method with this problem, and never call again that method... the BrowseObject (and all the child objects) will never be freed!

is it the same as
https://bugs.launchpad.net/openobject-client/+bug/351169
and
http://www.openobject.com/forum/viewtopic.php?p=39245

?

> I suppose this also applies to cases when arguments have an empty list ([]) as a default value doesn't it?

It does.

> Maybe this is the root cause of some not-freeing-memory problems I have seen when doing batch imports into OpenERP. After all, if you pass a BrowseObject into the context of a method with this problem, and never call again that method... the BrowseObject (and all the child objects) will never be freed!

Absolutely not. This is specifically an issue with *mutable default values* of functions and methods. Objects passed by argument (as a browse object would be, even if you pass it through the context) don't have this issue.

Unless, somewhere up your stack, you have a dict as default context argument and you put your Browse object in *that*. In that case, yes the Browse object will stay around forever.

> is it the same as

Off the cuff, I'd say no. This issue tends to generate weird bugs, but is rarely problematic enough to spawn actual memory leaks (in my experience, YMMV). Though if we're really unlucky, it could be happening. Some memory profiling could be in order here, but there are many way to leak memories even in python.

many *ways*

Thanks for the info Xavier :)

> Unless, somewhere up your stack, you have a dict as default context
> argument and you put your Browse object in *that*.
> In that case, yes the Browse object will stay around forever.

That's just what I meant, if we pass a list (for example a list of ids to filter data), or a tuple or a browseobject (anything big), in the context of one of those methods (with context={}), then it can't be garbage collected.

And I think I just made that mistake (passing some big fellows on the context, instead of creating a new parameter, so we didn't break the API [think about overriding 'write' on some openobject]) :)

> And I think I just made that mistake

Well, I don't know if it's a mistake per se, surely the context is useful to carry information within the stack. As far as I know, there's no documentation relative to what you can and can't do (and what you should and shouldn't do) with @context. There's no mistake to be made when there's nothing saying it's a mistake ;)

> The test should *only ever* performed when the current function fetches data from the context.
On the contrary I think it should be tested even on dummy 1-line
functions, because one day someone will change them and use the context,
and forget to test it. That's trouble waiting to happen.
Moreover this test is common practice so it's absence surely makes
pythonists unconfortable.
Finally, it can be checked/enforced with a regex once in a while if it's
everywhere.
Lionel

> because one day someone will change them and use the context,
and forget to test it

And that's why we should have code reviews. Or find another solution, because having this test at the top of every single god damn method would bother me much more than

> so it's absence surely makes pythonists unconfortable.

Which most definitely doesn't make me uncomfortable (quite the opposite in fact, Pythonic philosophy tends to be based on EAFP, not LBYL)

Ok Xavier I see your point.
I thought a good regex could have done the job but then it's more complex.
Given the huge number of occurences in the addos+extra+community, I'm writing a python script to update the source code, so we only have to do a code review afterwards.
I'll post it here.
Lionel

Here is a first draft script.
It fails on some scripts (I'll investigate if I can), but it mostly works.
It still adds unneeded tests, but it could be made it smarter.
As a side effects it also cleanly reformats "def" lines.

On the current trunk addons it can generate a >12000 lines patch...

Here's a sample patch file generated with:
    find . -name '*.py' -print -exec python ../../tools/fix_default_values.py {} /tmp/a ';' -exec cp /tmp/a {} ';'
It's not a fix yet of course, just a proof of concept.
What about that ?

Here's an improved version of the script: it adds the test only if needed and it handles function calls used as defaults. It still fails in some files but handles most cases.
Lionel

Here's a new sample patch file generated with:
    find . -name '*.py' -print -exec python ../tools/fix_default_values.py {} /tmp/a ';' -exec mv /tmp/a {} ';'
(mv makes sure no file is destroyed when the script fails)
Lionel

Your patch is not usable.
Here is a sample of your patch:

@@ -443,7 +465,9 @@
             self._check_moves(cr, uid, ids, "write", context)
         return super(account_account, self).write(cr, uid, ids, vals, context=context)

- def unlink(self, cr, uid, ids, context={}):
+ def unlink(self, cr, uid, ids, context=None):
+ if context is None:
+ context = {}
         self._check_moves(cr, uid, ids, "unlink", context)
         return super(account_account, self).unlink(cr, uid, ids, context)

@@ -461,7 +485,7 @@

 class account_journal_column(osv.osv):
- def _col_get(self, cr, user, context={}):
+ def _col_get(self, cr, user, context=None):
         result = []
         cols = self.pool.get('account.move.line')._columns
         for col in cols:

In the first case, no need to initialize context as the function don't access it directly (it just pass the context to other functions).
In the second case, the context is not initialized. I assume that's a bug in your script.
Also, there is are some place in the code where "if not context:" is done instead of "if context is None:". Your patch can't detect it.
For this reasons, I refuse your patch. In fact this changes have to be done by the developer when he found it when working on a piece of code. An automatic massive change will lead to errors and more bugs.

Thanks for your comments.
I should have made it clear I was not proposing a complete fix. The patch I sent was only intended to serve as an example and an aid for developers. Fixing all with a script requires deeper python knowledge than I have.

In the first case you emphasized, I don't think I can avoid, maybe someone else can.
In the second case, it was firmly demanded by Xavier in comment 12 that we skip the test when the parameter is unused.
Finally, the existing "if not context:" cases can be fixed with a simple regex and a careful review.

Lionel

I gave another try at improving the script. It will add slightly less useless code, and change as little as possible.

Here's a new sample patch. As before, this is only supposed to ease the work of a human developer fixing this bug.
This is not an acceptable fix in itself, so I'm not marking this a patch in Launchpad.

Also note that while this script adds less useless code, it could also
fail to add tests when they are needed, and provoke bugs similar to Bug
#535642.

Guys, sorry, if that's a silly question, but will that script be part of the
automatic quality checking in the CI server http://test.openobject.com ? I
think it would be better be part of it, is that planned?

Raphaël Valyi
http://www.akretion.com

2010/3/10 Numérigraphe <email address hidden>

> ** Description changed:
>
> Often in the code base we have:
> def foo(...., context={},....):
> ....
> As explained in comment #1, this should be changed to
> def foo(...., context=None,....):
> - if not context: # test incorrect, see below
> + # you should add this test only if the context is actually used
> + if context is None:
> context={}
> ....
>
> (This bug originally proposed the other way round hence the "nope" in
> comment #1.)
>
> (xmo) Complements of information:
>
> * The test should be `context is None` rather than `not context`: the
> caller could provide an empty dict argument {} and expect to have data in it
> after the call, if a test is done using `not context` an empty dict will
> match and the function will be creating a brand new context.
> * The test should *only ever* performed when the current function fetches
> data from the context. If you don't do anything with the context but forward
> it to other functions and methods (e.g. read() or create()), no need to test
> for anything.
>
> --
> Default values should be immutable
> https://bugs.launchpad.net/bugs/525808
> You received this bug notification because you are subscribed to
> OpenObject Addons.
>
> Status in OpenObject Addons Modules: Confirmed
> Status in OpenObject Addons extra-trunk series: New
> Status in OpenObject Addons trunk series: Confirmed
> Status in OpenObject Server: New
> Status in OpenObject Server trunk series: New
>
> Bug description:
> Often in the code base we have:
> def foo(...., context={},....):
> ....
> As explained in comment #1, this should be changed to
> def foo(...., context=None,....):
> # you should add this test only if the context is actually used
> if context is None:
> context={}
> ....
>
> (This bug originally proposed the other way round hence the "nope" in
> comment #1.)
>
> (xmo) Complements of information:
>
> * The test should be `context is None` rather than `not context`: the
> caller could provide an empty dict argument {} and expect to have data in it
> after the call, if a test is done using `not context` an empty dict will
> match and the function will be creating a brand new context.
> * The test should *only ever* performed when the current function fetches
> data from the context. If you don't do anything with the context but forward
> it to other functions and methods (e.g. read() or create()), no need to test
> for anything.
>
>
>
>
>

Raphaël, it's a good question. The way I wrote the script it can not be
100% reliable, it's just a quick hack for a one-time massive fix - it's
not usable on the integration server..
But indeed the integration server should treat as "suspicious" any line
matching a regex like the following one:
     '\s*def \w*\(.*={}\)'
Lionel

Reporting progress on this issue, counting the occurrences of "variable={}" in "def" lines :

Good news:
Addons: trunk has 350, down from 493 in 5.0
Web client: trunk has 29, down from 32 in 5.0

Bad news:
Server: trunk has 114, up from 100 in 5.0 (on the server! shame!)
Client: trunk has 70, up from 58 in 5.0
Extra: trunk has 868, up from 702 in 5.0

That means there is still a LOT of mindless coding and silly copy/paste going on.
That the partners didn't improve was expected - some clearly said they didn't give a damn - too bad.
But OpenERP SA said they would fix this over time, one function at a time as other improvements were being done. Obviously that doesn't work.

Dear maintainers, please either step in and have the coding practices improve at OpenERP SA, or consider again making a massive one-time fix across the whole code base. The script I wrote does not fix it all, but it sure will help.
Lionel.

This bug is not invalid.

It's difficult to fix and can't be done through a massive script, but still that does not make it invalid.

What should be done, is that this should be well documented, so that people that are developing code take this into account.

Margarita Manterola, I agree with you but it seems like a cleanup is going on on Launchpad.
Let's mark this bug "opinion" so we can get back to is when v6 is released.
Lionel

Setting status to "Opinion" in Extra-trunk... community developers should work on it in the long-term

Fix landed for server in rev 3009 [merge] <email address hidden>, courtesy of Lionel.
Thanks a lot Lionel!

setting status as Opinion too... FYI, 310 remaining today for addons trunk.

FYI, this is still being worked on in R&D, to gradually clean it up. For example see:
 - https://code.launchpad.net/~openerp-dev/openobject-addons/trunk-addons-context/+merge/41682
 - http://bazaar.launchpad.net/~openerp/openobject-server/trunk/revision/3084

I stumbled on another small nasty thing I'd like to draw your attention to: if I'm not mistaken, when a caller passes a context to a function, this function receives the context's "pointer", not a distinct copy of the data.
As a consequence, functions that modify or added keys to the context must do so on a copy of the context, otherwise they may change the behavior of the caller.
That step is still missing in a good number of places I think: I suggest we do this at the same time as testing for None contexts in case it may be modified, in a test like this:
    if context is None:
        context = {}
    else:
        context=context.copy()
Lionel Sausin.

This depends on whether or not the called function wants to alter the behavior (or rather the context dictionary values) of the calling function and thus seems optional and depends on the case.

I'd just like to point out that the context is not passed with a "pointer" or anything like that, it's mostly a semantic issue but you can read up on it here: http://effbot.org/zone/call-by-object.htm

Le 06/03/2012 10:26, Niels Huylebroeck a écrit :
> This depends on whether or not the called function wants to alter the behavior (...) of the calling function and thus seems optional and depends on the case.
Ghostbusters don't cross the streams, though it depends on the case too.
Usually in OpenERP the context is updated to adjust the behavior of
subsequent function calls, not that of the caller.
For example, there's no reason to mutate the context in an ORM method is
there?
Furthermore, there is no way a developer can know the impact of changing
a key in the caller's context, so at first sight I'd consider it a mistake.
Legitimate uses are possible but they would be rare, wouldn't they? They
should be explicitly documented with a comment.
> I'd just like to point out that the context is not passed with a "pointer" or anything like that(...)
Sorry about the C-ish description. The important part was : it's mutable.

Lionel.

Lionel is of course right here. However in the few functions that do amend
context, the vast majority already copy and pass the copied dict, certainly
I don't recall ever seeing it in main addons branch where it wasn't. But
most function calls do nothing with the context (in which case the entire
recommended code block is redundant) and the very few that do are for the
most part just looking up values, which makes the second half of the code
really unnecessary. Why add extra processing instructions if they are
simply not required. Also the recommendation code cause more issues.
 Often the modified context is to add a variable like a date, period,
location whatever to another call. Generally where I see it it is always
done on a copy of the context, ctx, ctx2, ctx3 being the common
conventions. Given that differently modified contexts are foreseeably
passed to different functions and then require the original context in
future calls, I think leaving the context copying until it is required is
more readable.

The only exception I see to this regularly is in wizard code, which
possibly should be cleaned up, although possibly it is the special case.

I also fail to see a case where a caller would expect to have its context
modified, except maybe that of a helper function specifically designed to
amend the context of multiple callers to facilitate reuse, e.g.
_set_location_context(blah blah) in which case documenting it may be
unnecessary but even then for most cases you wouldn't do it that way.

But would be nice to have added to developer guidelines. The other one I
still see which really bugs me is the good old default context={} in
function defs.

2012/3/7 Numérigraphe <email address hidden>

> Le 06/03/2012 10:26, Niels Huylebroeck a écrit :
> > This depends on whether or not the called function wants to alter the
> behavior (...) of the calling function and thus seems optional and depends
> on the case.
> Ghostbusters don't cross the streams, though it depends on the case too.
> Usually in OpenERP the context is updated to adjust the behavior of
> subsequent function calls, not that of the caller.
> For example, there's no reason to mutate the context in an ORM method is
> there?
> Furthermore, there is no way a developer can know the impact of changing
> a key in the caller's context, so at first sight I'd consider it a mistake.
> Legitimate uses are possible but they would be rare, wouldn't they? They
> should be explicitly documented with a comment.
> > I'd just like to point out that the context is not passed with a
> "pointer" or anything like that(...)
> Sorry about the C-ish description. The important part was : it's mutable.
>
> Lionel.
>
> --
> You received this bug notification because you are subscribed to OpenERP
> Server.
> https://bugs.launchpad.net/bugs/525808
>
> Title:
> Default values should be immutable
>
> Status in OpenERP Addons (modules):
> Opinion
> Status in OpenERP Addons extra-trunk series:
> Opinion
> Status in OpenERP Addons trunk series:
> Opinion
> Status in OpenERP GTK Client:
> Opinion
> Status in OpenERP Server:
> Fix Released
> Status in OpenERP Server trunk series:
> Fix Released
>
> Bug...

On 03/06/2012 12:12 PM, Numérigraphe wrote:
> Le 06/03/2012 10:26, Niels Huylebroeck a écrit :
>> This depends on whether or not the called function wants to alter the
>> behavior (...) of the calling function and thus seems optional and depends
>> on the case.

I tend to agree with Niels on this. Contrary to the mutable default parameter
which is an obscure Python caveat, developers cannot ignore when they are
mutating the request context.

> Usually in OpenERP the context is updated to adjust the behavior of
> subsequent function calls, not that of the caller. For example, there's no
> reason to mutate the context in an ORM method is there?

There are many subtle cases where changing the context is needed, sometimes for
callees only, but sometimes also for the entire request.
For example in the ORM the copy_data() method needs both:
- it needs to read a different translation of a column by setting
context['lang'] (for callees only)
- it needs to maintain something like a context['visited_records'] set, in
order to detect cycles in an object graph (for the entire request)

> Furthermore, there is no way a developer can know the impact of changing a
> key in the caller's context, so at first sight I'd consider it a mistake.

The impact is usually pretty obvious. If you're altering a known context value
such as 'lang' or 'tz' you obviously know what you're doing, and that it should
be done in a copy of the context in most cases (but not always).
If you're simply setting a custom context key that your method and its callees
agree upon, then you should properly namespace it in order to avoid name
conflicts with someone else's custom key. And it does not matter that much if
you do it in a copy of the context or not.

In most cases I think the meaning of altering the context is pretty clear and
developers are already taking care of copying it when needed.

Thanks for your comments. I totally we shouldn't copy the context if it's not needed. I'll make dedicated merge proposals when I stumble upon a suspect context mutation, to either make a copy or add an explicit comment.
Lionel.

I could find only 59 mutable defaults left in addons (I made a MP to fix them) and 84 in the client. At this point I think it's realistic to target 6.2.
Lionel.

We wont change this now (i mean replacing context by context.copy()), as we are designing a new high level api that will remove the cr, uid, ids context from function arguments. we will add an helper to update the context, something will probably look like:

self.models.crm_stage(context_update={'lang':'fr'}).name = "blabla"

to update the french translation of all the crm_stages.

On 03/07/2012 11:22 AM, Antony Lesuisse (OpenERP) wrote:
> We wont change this now (i mean replacing context by context.copy()), as
> we are designing a new high level api that will remove the cr, uid, ids
> context from function arguments.

If you are wondering, this is in fact an early draft idea, and there is of
course no definitive choice for the new API. We will try to build (one or more)
proof of concept of it, including a way to make the system backwards compatible
with the old API, and then ask the Framework Experts to review :-)

PS: for such discussion we should stick to the list, not use bug comments