Ptvs: Debugger silently calls __iter__() and next()

[mehrdadn]

Try to debug this code:

import os
class MyIterator(list):
    def __iter__(self): return self
    def next(self): os._exit(1)
a = MyIterator()
print(a)   # put a breakpoint here

You will find that the debugger will quit before you even attempt to do anything.

This should not happen. If the debugger insists on iterating through members of every subclass of list silently, then it should really call list.__iter__(instance) rather than iter(instance). Otherwise it breaks debugging on the entire codebase, since the iterator is silently consumed and hence the program no longer behaves correctly when stepping through.

Note that similar problems probably exist for other types, not just list. More generally, I think the debugging experience of subclasses of basic types needs improvement; it is currently not robust.

(This bug probably touches on similar aspects of the debugger as #3505.)

[huguesv]

Thanks for the detailed report. You are correct that we shouldn't be doing this without explicit user action (expand the node).

[zooba]

As we discussed on the other bug, "improving" debugging of subclasses really just means disabling it. We need to balance that against user expectations, which is that objects implementing a certain protocol will adhere to that protocol - by convention, iteration should not terminate the process, and in particular iteration of an iterable should not have observable side effects. Our debugger breaking here is a good indication that anyone using that code will likely also be broken.

Basically, we don't want to cripple most users' experiences just in case someone broke the rules and expects their broken code to still work correctly.

[mehrdadn]

iteration should not terminate the process, and in particular iteration of an iterable should not have observable side effects. Our debugger breaking here is a good indication that anyone using that code will likely also be broken.

we don't want to cripple most users' experiences just in case someone broke the rules and expects their broken code to still work correctly.

It appears you are missing the point. I am, obviously, not going to literally call os._exit in my iterator's next method. os._exit was just an easy placeholder to prove that the code is in fact getting called. The point is that the iterator is being implicitly consumed by the debugger immediately, making it impossible to debug code that uses a custom iterator that happens to inherit from a basic type like list.

[zooba]

And that's what I mean about "iteration of an iterable". We check for iter(x) is x before consuming the iterator, so if you look at an _iterator_ object then we will detect that you've followed the pattern correctly and will not consume it.

If you have implemented a custom _iterable_, then calling iter(x) should return different _iterator_ objects each time, each of which can be consumed without affecting the original iterable.

You should not subclass a sequence type to create an iterator type. Perhaps you could share the relevant part of your code so we can see why your iterable is not behaving correctly?

[mehrdadn]

Edit: sorry, I think I misunderstood your comment, I'll reply after reading it again.

[mehrdadn]

Sorry, I just realized I may have misunderstood your comment. Let me reply when I'm back at my computer.

[zooba]

To provide a richer experience, we break the "execute code arbitrarily" rule for the normal Python debugger in many places already. You can switch to the mixed C/Python debugger (and just ignore C code) to use one that does not execute your code, but then I think you'll see how valuable it is to actually run the code that you've written.

It is required to implement an iterator's __iter__ by returning itself. This is why we do not consume iterators in our debugger. An _iterable_ (sometimes _container_) is a different type of object: see the iterator protocol.

When we find an __iter__ method that does not return self, we assume we have a container and can safely consume the iterator it returned. Otherwise, we assume we have an iterator that cannot safely be consumed and do not attempt it until requested.

[mehrdadn]

OK, so I don't understand what you're saying. The behavior you claim you have here directly contradicts what the debugger is actually doing:

When we find an __iter__ method that does not return self, we assume we have a container and can safely consume the iterator it returned. Otherwise, we assume we have an iterator that cannot safely be consumed and do not attempt it until requested.

Again, the code I had was this:

class MyIterator(list):
    def __iter__(self): return self
    def next(self): os._exit(1)

__iter__ is returning self, so you do not have a container (and you agreed with this yourself), so why are you consuming it when you yourself agree you should not? That's the entire bug.

[zooba]

I never said that wasn't a bug. What I was arguing against was your insistence that we need to "fix" subclass handling (and primarily making sure my colleagues are aware that we are not blindly going down that route). Using list.__iter__ on all subclasses of list is one example of "blindly" fixing it.

[zooba]

The issue is actually in util.py, where we check for custom __repr__ but not iterators when they are subclasses of known containers. AFAICT, the check I referenced earlier isn't used anymore.

[mehrdadn]

Could you explain what you meant by this then?

Basically, we don't want to cripple most users' experiences just in case someone broke the rules and expects their broken code to still work correctly.

In the context of the preceding discussion, I interpreted this to mean "we don't want to stop having a useful UI (e.g. displaying list contents automatically) merely because someone might decide to write a broken iterator that defies all expectations (e.g. has a next that calls os._exit) and still expects their code to work in the debugger correctly"... but now you're saying you were actually arguing against wanting to "fix" subclass handling? I'm very confused how to interpret what you wrote.

[zooba]

If someone wants to subclass the type, we should respect that (and yes, it's an arbitrary design decision from 7-8 years ago, but it's served us well so far). Ignoring user code just because they subclassed a built-in type (or more likely, used a third-party subclass of a built-in type) is something that we're just not going to do.

Ultimately, this means that arbitrary code can do arbitrary things, and may do them sooner than expected under a live Python debugger. Since we can't poke directly into memory, we can't do non-invasive debugging (and where we do, it's limited compared to when we allow user code to run). Some operations are obviously and detectably bad, such as consuming a one-shot iterator, but even that relies on convention rather than technical protection. This is inherently the nature of Python anyway, and it's a tradeoff that we and most of our users are comfortable with.

[int19h]

To sum it up:

We shouldn't be consuming an iterator if we can detect it. The fact that we don't in this case is a bug.

At the same time, we do need to invoke __iter__ to detect it. And in general, it's certainly not feasible for a pure Python debugger to never ever run any user code - this is one of those cases where it's okay, on the assumption that the usual protocols are followed.

[mehrdadn]

To clarify, I was not saying that user code should never be called. In the comment that I later edited, I was only saying user code shouldn't be called arbitrarily, i.e. methods should only be called in predictable ways at predictable times.

For example, even something as benign as __repr__—which I think we all agree can be assumed to be side-effect-free—should not be called unless the programmer actually attempts to view the contents of a variable, in which case __repr__ should only be called on that variable. The same should probably go for __iter__—and notice that this is the first third of the bug. Now, a <user-defined-type>.next() method should ONLY be called if the user explicitly requests an explicit expansion of the items (the "Expanding this will enumerate the results" thing); merely hovering on an iterable and/or an iterator should not be enough. This is the second third of the bug. And the last third of the bug is that your check for iter(self) is self is merely a hack, and immediately breaks if the callee decides to add an extra level of indirection to self for some reason (whether through a proxy, or through a super call, etc.). Hence, even on hovering, I argue that __iter__ should only be called implicitly if its behavior is known (i.e. it is <built-in-type>.__iter__); it should not be called for arbitrary types unless explicitly requested by an expansion.

[int19h]

The check for iter(self) is self follows the iterator protocol - per PEP 234:

A class that wants to be an iterator should implement two methods: a next() method that behaves as described above, and an __iter__() method that returns self.

I agree that it's a hack - it's not really meant to detect iterators - but it's the best one that we can come up with, since there's no standard way to do so with Python (although it would be nice to have a way to ask any object if iterating it is one-off or not, and whether it's expensive or not).

That said, looking at the code more closely, I'm not sure if this is actually the code path that leads to your problem. It appears that when we're reporting children (this is after retrieving them in enum_child_locally), we're actually calling len() on each of those values:

def report_children(execution_id, children):
    children = [(name, expression, flags, safe_repr(result), safe_hex_repr(result), type(result), type(result).__name__, get_object_len(result)) for name, expression, result, flags in children]

where get_object_len is:

def get_object_len(obj):
    try:
        return len(obj)
    except:
        return None

and ditto in report_execution_result. And when used on an iterator, len will iterate it. The same logic should be extended there.

[mehrdadn]

Yeah I'm not sure, there are multiple issues I see here (the call occurring without a trigger and the call occurring improperly); it may require a lot of changes. Hope you folks can find a solution :-)

[zooba]

len won't consume an iterator - it looks for the __len__ method and if it isn't found raises TypeError.

And I don't recall the exact email, but there's a python-dev thread somewhere where iter(x) is x is explicitly declared to be the official way of checking whether it's an iterable or an iterator. Iterators _must_ return self, while anything that adds a level of indirection is an iterable (and if iterating an iterable consumes the iterable, it's a poorly implemented iterable).

[mehrdadn]

Your definitions are wrong (and whoever said that on python-dev is wrong)...

An iterator is simply an object supporting next:

This document proposes an iteration interface that objects can provide to control the behaviour of for loops. Looping is customized by providing a method that produces an iterator object. The iterator provides a get next value operation that produces the next item in the sequence each time it is called, raising an exception when no more items are available. [...] Iterator objects returned by either form of iter() have a next() method.

Similarly, an iterable is an object such that if you call iter on it, you get back an iterator.

It does not automatically even make sense to call iter on an iterator, let alone checking whether it returns self or not.

[zooba]

The PEP literally says in two places (the C API spec and the Python API spec) that iterators should implement __iter__ and return self.

[mehrdadn]

Have you read the sentences immediately following the one you are referencing?

You are referencing:

A class that wants to be an iterator should implement two methods: a next() method that behaves as described above, and an __iter__() method that returns self.

The immediately following sentences are:

The two methods [i.e. above^] __correspond to two distinct protocols__:

1. An object __can be iterated over__ [i.e. is _iterable_] with for if it implements __iter__() or __getitem__().
2. An object can function as an __iterator__ if it implements next().

As you can see, they make it crystal clear that the two protocols are separate, and that the suggestion that an iterator "should" implement both the iterator and the iterable protocols is just a recommendation, not a requirement. (In this case, the recommendation is because this practice often leads to less redundant code.)

[zooba]

Be very careful about reading too much into specific word choices in decades-old PEPs. These are descriptive documents, not formal documents, and particularly in the early stages of Python's existence were not as deeply reviewed as they are these days.

Meanwhile, the collections.abc.Iterator class includes __iter__, which is as good as documentation. And even if it's "only" a recommendation that an iterator implement __iter__() -> self, we've already established that our debugger is designed to encourage well-written code. In particular, if you have an iterator that does implement __iter__ but _doesn't_ return itself, you have an incorrectly implemented iterator (though in that case, we'd iterate over the newly returned object and would only consume the original one if that's what you designed it to do - again, against recommendation and convention).

[mehrdadn]

This is getting quite frustrating.

First, the implementation is not the documentation. The documentation is the documentation, and the implementation is the implementation. The fact that you first tried to use PEP as evidence in your own favor and then tried to dismiss it when I proved to you that it says the exact opposite says a lot on its own. It is quite insincere of you.

Second, you should understand that even if the implementation did qualify as documentation, documentation found in the CPython codebase would not be the documentation of the Python language.

Third, I'm _not_ "reading too much into the specific word choices". The PEP's statement that _"the two methods __correspond to two distinct protocols__"_ is 100% crystal clear as to its meaning. You cannot deny this and interpret "two" to be "one" no matter how little or loosely you look into the word choices.

Fourth, you just claim with no logical basis that __iter__ returning anything other than self is "incorrect" in an effort to subtly dismiss the issue, but you can see yourself that there is literally no evidence that an iterator must implement __iter__ -- neither as a requirement, nor as "good" practice. You somehow seem to blindly assume it must be good practice without giving a single reason as to _why_ the user of either protocol should care why __iter__ is returning self or not. It is an actively harmful constraint in an iterable protocol and it is completely useless in an iterator protocol, and the caller would already know whether it is dealing with an iterable or an iterator. It makes no logical sense for this to be a requirement on an iterator, but that doesn't seem to concern you the slightest.

Fifth, __everything__ you and I have both quoted -- including the code you just linked to -- supports the notion that it is __handy__ for __iter__ to return self so that __implementers__ of the protocol can avoid redundantly defining two classes. I've repeated this several times and you've constantly refused to even comment on it, and instead you keep repeating your own stance, despite the fact that it provides a very clear explanation of everything you have seen without contradicting anything.

Sixth, I'm sure you understand that abstract base classes are intended to make it __easy__ to implement an interface, and therefore frequently support more methods than required for interfaces or protocols they implement, so you cannot point to an ABC and then claim it imposes a requirement on the interface whose implementation it is facilitating. There is a _lot_ of ease-of-use benefit and _no harm_ to having such an __iter__ method in the Iterator class, and hence that is why it is there. It does not mean the "iterator protocol" needs an __iter__ method. Again: the PEP was quite clear in stating that this is specifically not intended.

Lastly, if it's too much trouble and you don't want to implement it, then just don't. You don't need to reject it on the basis that "iteration should not terminate the process" and then try to twist all the facts or impose a double standard as to what sources you consider authoritative in the process and waste everyone's time. You can just close the issue.

---

Oh, one more final note:

The __official Python Wiki__ is perfectly clear about this:

An __iterable__ object is an object that implements __iter__.
__iter__ is expected to return an _iterator_ object.

An __iterator__ is an object that implements next.
next is expected to return the next element of the _iterable_ object that returned it, and raise a StopIteration exception when no more elements are available.

__In the simplest case the _iterable_ will implement next itself and return self in __iter__.__

I rest my case.

[zooba]

I'm no longer interested in this discussion, and it seems you aren't either. Unfortunately, I still need to use this venue to provide guidance to whoever on our team makes any change, so I can't simply ignore this issue and stop replying. You can choose to stop, though, so feel free.

To my team:
In our debugger and particularly the safe repr helper, we should continue to assume that anything with __iter__ that doesn't return itself is safe to enumerate up to our usual limits. Right now we don't check that it doesn't return self for subclasses of known container types when generating the repr, so let's add that check to handle subclasses of list/tuple/dict that redefine iteration.

[mehrdadn]

Sure thing. I've lost interest in reporting bugs entirely so I wish you the best of luck in obtaining bug reports as well.

[2Dews]

I am having this same issue with sqlalchemy query results. I tried the experimental debugger in 15.7 preview 2.0, and its still a problem there as well.

[huguesv]

This now works properly with ptvsd 4.x (I tested 4.1.3).