[Python-bugs-list] [ python-Bugs-628246 ] Set constructor fails with NameError

noreply@sourceforge.net noreply@sourceforge.net
Thu, 07 Nov 2002 17:07:19 -0800


Bugs item #628246, was opened at 2002-10-24 16:52
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=105470&aid=628246&group_id=5470

Category: Python Library
Group: Python 2.3
Status: Open
Resolution: None
Priority: 5
Submitted By: Jeremy Hylton (jhylton)
>Assigned to: Raymond Hettinger (rhettinger)
Summary: Set constructor fails with NameError

Initial Comment:
Here is a toy demo of the problem:

def baditer():
    raise TypeError
    yield 1

import sets
sets.Set(baditer())

Traceback (most recent call last):
  File "/tmp/baditer.py", line 8, in ?
    sets.Set(baditer())
  File "/usr/local/lib/python2.3/sets.py", line 367, in
__init__
    self._update(iterable)
  File "/usr/local/lib/python2.3/sets.py", line 330, in
_update
    transform = getattr(element, "_as_immutable", None)
UnboundLocalError: local variable 'element' referenced
before assignment

The _update() method has a try/except for TypeError
that is trying to catch failures to place set elements
in the dictionary.  The try/except also wraps a for
loop over an iterator.  So if something goes wrong and
the iterator raises TypeError, the
failed-to-insert-element-in-dict code is run.

The obvious solution, it seems, is to have the
try/except inside the for loop so that it only catches
errors in dict insertion.  Tim points out that this
would be slow.

The example above is a toy, but I ran into this bug in
a real application where debugging was made harded
because the Set code was catching an exception when it
shouldn't.


----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-11-07 20:06

Message:
Logged In: YES 
user_id=6380

Yikes! You meant type(iterable), not type(iter).

I expect this will get complaints from people who write
careful iterators.

But it seems the best compromise for now, so check it in
(with above fix). I doubt this is the last word though. :-(

It needs a unittest that tries this with each of the
mentioned types as well as with something that's not one of
them.

----------------------------------------------------------------------

Comment By: Raymond Hettinger (rhettinger)
Date: 2002-11-07 18:07

Message:
Logged In: YES 
user_id=80475

Revised patch that avoids performance issues in cases 
where the iterator won't raise errors during iteration.

For the other cases, list(iter) is still faster than inserting a 
try/except inside the loop.

Please pronounce one way or the other and then re-assign 
to me.  I'll move the try/except back inside the loop if 
that's what you want.

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-11-02 19:02

Message:
Logged In: YES 
user_id=6380

Hmm...  It's the performance waste of always making an
unnecessary copy (the argument will often already be a list,
or a tuple) vs. the performance waste of doing try/except
each iteration.

Maybe it should all be replaced by this?

        for element in iterable:
            transform = getattr(element, "_as_immutable", None)
            if transform is not None:
                element = transform()
            data[element] = value


----------------------------------------------------------------------

Comment By: Raymond Hettinger (rhettinger)
Date: 2002-10-31 23:09

Message:
Logged In: YES 
user_id=80475

Yes.  The intended behavior is to have list(iterable) run the 
iterable start to finish so that any exceptions raised will 
not be trapped by the Sets code during dictionary 
construction.

Feel free to disregard the idea. I thought it had some merit 
because it is simple, fast and fixes the problem.  Still, it 
looks weird and I won't lose any sleep if you toss the idea.





----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-10-31 21:03

Message:
Logged In: YES 
user_id=6380

That patch makes no sense -- it first materializes the
iterator as a list, then iterates over that. Was that what
you intended?

I'd rather give up on the speediness and put the try/except
where it belongs.

----------------------------------------------------------------------

Comment By: Raymond Hettinger (rhettinger)
Date: 2002-10-25 23:39

Message:
Logged In: YES 
user_id=80475

A simple solution is to run through the iterator once before 
starting the update.  A one line patch is attached.

The only thing I don't like about it is that the code is 
starting to look hare-brained.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=105470&aid=628246&group_id=5470