Mutable Default Arguments

[zsef123]

Mutable arguments can have unintended behavior.
like calling repeatedly under function

def foo(x=[]):
    x.append(1)
    return x

if call foo 3 times, the result is [1,1,1] Python Reference, The Hitchhiker’s Guide to Python

In summerization/graph.py:175

def add_node(self, node, attrs=None):
        if attrs is None:
            attrs = []```

def add_edge(self, edge, wt=1, label='', attrs=[]):

add_edge function is used [] in default argument but add_node function is used None.
And then check if attrs is None then assigns empty list

I think way of add_node is more secure than add_edge.

How about changing [] to None?

[poornagurram]

Repeated calls will not change default argument if it is none. It's more secure than [].

[piskvorky]

@zsef123 @poornagurram I'm not familiar with that code base, but it looks like a bug at a glance. Thanks for catching this!

Can you send a PR with a fix? Any other places with unintended mutable defaults?

[menshikh-iv]

Resolved in #1562

[gojomo]

Just noticed this; a perhaps better fix (at least in the word2vec/keyedvectors/doc2vec cases I've looked closely at) could be to supply empty tuples as the defaults. (They'd thus be armored against any inadvertent future mutations – but no None-check & assign lines would later be necessary.)

[zsef123]

@gojomo I'm just following type on original code(list).
Empty tuple sounds nice.
That is more simple than checking None.
I'll check soon

[piskvorky]

Same with set vs frozenset (if that came up).

[zsef123]

@gojomo @piskvorky
In PR #1572 ( I missed test in my fault )

Test failed at keyedvector.py:426, most_similar_cosmul method.

File "/home/travis/build/RaRe-Technologies/gensim/gensim/models/keyedvectors.py", line 476, in most_similar_cosmul
all_words = set([self.vocab[word].index for word in positive + negative

Once change from None to (), will have to exclude keyedvector class or changed to positive + list(negative)

In case of negative used before reassign(under the code)

negative = [
            self.word_vec(word, use_norm=True) if isinstance(word, string_types) else word
            for word in negative
        ]

So I think should cope with using positive, negative as list.
And I think None is more expose intention.

[gojomo]

Or could move all_words initialization to before line where positive is reassigned to be a list. Or could change all_words composition to be …(word in positive) or (word in negative)….

Unsure of issue with use of immutable negative before re-assign; reading the immutable default is fine, only mutation attempts are a concern that we'd like to fail noisily rather than have side-effects on future calls.