I'm making this a release blocker for 2.7.2 because I want to ensure that the change is deliberate and appropriate.  This is triggered by a bug report in Ubuntu where the change in behavior was noticed:

https://bugs.launchpad.net/nova/+bug/791221/+index

I have two problems with the change that I'd like to describe.  First is the NEWS file entry:

- Correct lookup of __dir__ on objects. Among other things, this causes errors
  besides AttributeError found on lookup to be propagated.

This isn't associated with a tracker issue, so it's hard to know why this change was made, or whether there was any discussion of its impact.

Second, is this change appropriate for a maintenance release?  What problem does it fix and is that worth breaking existing packages for it?

I'll attach a simple test program.  Run this under 2.7.1 and it works, run it under 2.7.2rc1 and you get a TypeError.

As I recall the issue that triggered the change was reported by Michael Foord, so I'm adding him as nosy too.

Hmm, that behaviour looks unrelated to the specific problem Michael reported.

The initial problem in this space was that defining __dir__() completely determined the result of dir() calls, but object.__dir__() didn't actually work, so you couldn't easily get the standard list of attributes in order to supplement it.

I don't believe there is any reason to have tightened up the type constraints while fixing that - dir() should be returning sorted(obj.__dir__()) and not caring about the exact return type of the magic method.

> I don't believe there is any reason to have tightened up
> the type constraints while fixing that - dir() should be
> returning sorted(obj.__dir__()) and not caring about the 
> exact return type of the magic method.

+1

No additional type-checking was added. The problem is that __dir__ didn't work on old-style classes at all in 2.7.1:

$ python
Python 2.7.1 (r271:86832, Mar 24 2011, 22:44:47)
[GCC 4.4.5] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> class Foo:
... def __dir__(self):
... return ['a', 'b', 'c']
...
>>> class Bar:
... def __dir__(self):
... return ('a', 'b', 'c')
...
>>> print dir(Foo())
['__dir__', '__doc__', '__module__']
>>> print dir(Bar())
['__dir__', '__doc__', '__module__']

Loosening type-checks on dir() is fine with me but is really a 3.3 issue.

Ah, I wondered about that when I saw Barry was using old-style classes in his example. Perhaps the answer then is to add a PyInstance_Check() to skip invocation of __dir__() completely for old-style classes?

2011/6/3 Nick Coghlan <report@bugs.python.org>:
>
> Nick Coghlan <ncoghlan@gmail.com> added the comment:
>
> Ah, I wondered about that when I saw Barry was using old-style classes in his example. Perhaps the answer then is to add a PyInstance_Check() to skip invocation of __dir__() completely for old-style classes?

Uh, well, part of point of my checkin was to fix old style class
__dir__(). I think this is a bug in the package we're just exposing.

I would guess that if you instead skipped __dir__ completely for old style classes it would expose a different bug in this or some other package.

It would be broken in the same way that it was broken in 2.7.1 though. That can be a plus when it comes to maintenance releases.

OTOH, this does turn a silent failure (__dir__() ignored on old-style classes) into a noisy failure (must return a list).

If you make Barry's classes new-style, they break in 2.7.1 as well, so I'm coming around to a point of view that this is a legitimate fix that reveals a real bug in third party code (i.e. anyone that hits this had a __dir__ that previously wasn't getting invoked)

I believe the type check was gratuitous to begin with and should be removed.  There's no reason the result has to be a list.

2011/6/3 Raymond Hettinger <report@bugs.python.org>:
>
> Raymond Hettinger <raymond.hettinger@gmail.com> added the comment:
>
> I believe the type check was gratuitous to begin with and should be removed.  There's no reason the result has to be a list.

The reason for it is that the sort() method is called on it.

Can sorted() be used instead?

or something like:

   result = obj.__dir__()
   if not isinstance(result, list):
       result = list(result)
   result.sort()
   return result

Using sorted() makes sense to me.  

Note that I've at least accomplished one goal, which is to have a tracker issue that discusses the merits of the change.  That way, no matter what the RM decides, I can at least point to an issue for justification when I resolve the downstream bug. :)

Thanks!

When I first investigated this problem (I reported the original bug on Launchpad), my first attempt to address this issue in pymox had me quite stumped. The class in question has a __getattr__ method. Up until now, this hasn't affected the use of dir(), but it does now. I really just wanted it return whatever it used to return (since that has worked so far), but realising that this was an old-style class, I couldn't just call super(TheClass, self).__dir__().

So my question is: If this change stays (which seems clear given that the only changes proposed here are ways of relaxing the type requirement of the __dir__ method's return value, not reverting the change altogether), and I have an old-style class with a __getattr__ defined, how do I make that class return whatever it would have usually returned for __dir__()?

2011/6/4 Soren Hansen <report@bugs.python.org>:
>
> Soren Hansen <soren@linux2go.dk> added the comment:
>
> When I first investigated this problem (I reported the original bug on Launchpad), my first attempt to address this issue in pymox had me quite stumped. The class in question has a __getattr__ method. Up until now, this hasn't affected the use of dir(), but it does now. I really just wanted it return whatever it used to return (since that has worked so far), but realising that this was an old-style class, I couldn't just call super(TheClass, self).__dir__().
>
> So my question is: If this change stays (which seems clear given that the only changes proposed here are ways of relaxing the type requirement of the __dir__ method's return value, not reverting the change altogether), and I have an old-style class with a __getattr__ defined, how do I make that class return whatever it would have usually returned for __dir__()?

Yes, this is a limitation of magic methods on old style classes. The
usual method is something like this:

    def __getattr__(self, name):
        if name == "__dir__":
            return self.__dir__
        # Other stuff

Of course, the best fix is to use new-style classes. :)

2011/6/4 Benjamin Peterson <report@bugs.python.org>:
> 2011/6/4 Soren Hansen <report@bugs.python.org>:
>> So my question is: If this change stays (which seems clear given that the only changes proposed here are ways of relaxing the type requirement of the __dir__ method's return value, not reverting the change altogether), and I have an old-style class with a __getattr__ defined, how do I make that class return whatever it would have usually returned for __dir__()?
>
> Yes, this is a limitation of magic methods on old style classes. The
> usual method is something like this:
>
>    def __getattr__(self, name):
>        if name == "__dir__":
>            return self.__dir__
>        # Other stuff
>
> Of course, the best fix is to use new-style classes. :)

If I do this:

===== test.py ======
class Foo:
    def __getattr__(self, name):
        if name == '__dir__':
            return self.__dir__
        return 'something else'

a = Foo()
print dir(a)
====================

After a lot of this:
  File "test.py", line 4, in __getattr__
    return self.__dir__
  File "test.py", line 4, in __getattr__
    return self.__dir__
  File "test.py", line 4, in __getattr__
    return self.__dir__

...I end up with a "RuntimeError: maximum recursion depth exceeded". I
can't say I'm surprised :)

2011/6/4 Soren Hansen <report@bugs.python.org>:
>
> Soren Hansen <soren@linux2go.dk> added the comment:
>
> 2011/6/4 Benjamin Peterson <report@bugs.python.org>:
>> 2011/6/4 Soren Hansen <report@bugs.python.org>:
>>> So my question is: If this change stays (which seems clear given that the only changes proposed here are ways of relaxing the type requirement of the __dir__ method's return value, not reverting the change altogether), and I have an old-style class with a __getattr__ defined, how do I make that class return whatever it would have usually returned for __dir__()?
>>
>> Yes, this is a limitation of magic methods on old style classes. The
>> usual method is something like this:
>>
>>    def __getattr__(self, name):
>>        if name == "__dir__":
>>            return self.__dir__
>>        # Other stuff
>>
>> Of course, the best fix is to use new-style classes. :)
>
> If I do this:
>
> ===== test.py ======
> class Foo:
>    def __getattr__(self, name):
>        if name == '__dir__':
>            return self.__dir__
>        return 'something else'
>
> a = Foo()
> print dir(a)
> ====================
>
> After a lot of this:
>  File "test.py", line 4, in __getattr__
>    return self.__dir__
>  File "test.py", line 4, in __getattr__
>    return self.__dir__
>  File "test.py", line 4, in __getattr__
>    return self.__dir__
>
> ...I end up with a "RuntimeError: maximum recursion depth exceeded". I
> can't say I'm surprised :)

Ah, sorry I should have thought before writing that. :)
self.__class__.__dir__.__get__(self, self.__class__) should work,
though.

2011/6/5 Benjamin Peterson <report@bugs.python.org>:
> 2011/6/4 Soren Hansen <report@bugs.python.org>:
>> ...I end up with a "RuntimeError: maximum recursion depth exceeded". I
>> can't say I'm surprised :)
> Ah, sorry I should have thought before writing that. :)
> self.__class__.__dir__.__get__(self, self.__class__) should work,
> though.

Yeah, that one does seem to work. Excellent, thanks!

Raymond, I like your patch and I think it addresses the issue nicely.  I made one small change, which is to add a test for non-list-sequenceness instead of changing the existing __dir__is_list test.  I think both tests are useful to keep.

Benjamin, what do you think about this for 2.7.2?

2011/6/9 Barry A. Warsaw <report@bugs.python.org>:
>
> Barry A. Warsaw <barry@python.org> added the comment:
>
> Raymond, I like your patch and I think it addresses the issue nicely.  I made one small change, which is to add a test for non-list-sequenceness instead of changing the existing __dir__is_list test.  I think both tests are useful to keep.
>
> Benjamin, what do you think about this for 2.7.2?

My preference is to leave 2.7 alone (actually all maintenance
releases) and apply the patch to 3.3. It seems to me the type
restricts are tangentially only related to the issue. There's no need
to change the behavior for new-style classes, too.

My preference is to remove the type check.  It hasn't and won't serve a useful purpose so there's no reason to freeze it into the API and have it live on.

That being said, it probably doesn't matter one bit how this report gets resolved.

I look at it this way: If I was browsing the 2.7 code and saw that type check, would I remove it in the bugfix release? No.

I'm going to mark this resolved. Thanks everyone.