Skip to content

Submodule.iter_items sometimes attaches second empty iterator to its StopIteration #1779

Closed
@EliahKagan

Description

@EliahKagan

This is minor bug, and the main benefit of fixing it may be an improvement in code clarity rather than the behavioral difference. I think it makes sense to fix this in the same pull request that fixes #1775, and I've opened #1780 for that, but I think it's clearer to document it here than in the pull request.

Submodule.iter_items is a generator function as intended, because yield appears in its scope:

yield sm
# END for each section

Sometimes when one writes a function that returns a generator object, it is a regular function rather than a generator function, because the function returns a generator expression or because it calls another function that returns a generator object. That technique is sometimes used to fail fast on errors, since calling a generator function does not immediately run its code, but only returns a generator object, and the code runs when that generator object is iterated. However, that is not done here. That it is not appears intentional and reasonable, and even if not, changing it would probably break backwards compatibility.

However, it contains this code, at the top:

try:
pc = repo.commit(parent_commit) # Parent commit instance
parser = cls._config_parser(repo, pc, read_only=True)
except (IOError, BadName):
return iter([])
# END handle empty iterator

The intention there is to make it so that calling iter_items returns an empty iterator when either of those two exceptions is raised. It succeeds at doing that, but its success is unrelated to using iter([]) as the operand of return. Returning from a generator function--like falling off the end, which is more common--causes there to be no more items to iterate through, so a next call in which this happens raises StopIteration.

The operand of return in a generator function is neither returned to the caller--a generator object is returned--nor returned when next is called on that generator object. Instead, it becomes the value attribute of the StopIteration exception raised on the next call to indicate there are no more values. This feature of generators is rarely used, and the common ways of using generators, such as a for loop, ignore it. (When yield from is used as an expression, it evaluates to that value, which allows generators to get status information from their subgenerators, in the rare case their subgenerators return a meaningful value. It can also be accessed directly by explicitly catching StopIteration, binding the exception object to a variable, and accessing its value attribute. That's about it.)

I suppose it might make sense for Submodule.iter_items to have its returned generator object signal, in some way, that it is empty due to OSError (IOError and OSError are the same in Python 3) or BadName. But I think that having it attach a second empty iterator object to the StopIteration exception is clearly not intended to do so.

The statement was originally a bare return. The iter([]) operand was added in 82b131c (#1282). It came in along with numerous additions and improvements in type hinting, and it looks like it was mistakenly added with the idea it might be needed for make type-checking succeed. However, I don't believe the major type checkers were as stable back then as they are today, so perhaps it was instead added as a workaround for a bug in a static type checker.

Interestingly, this is conceptually related to #1755, and I would've fixed it there, had I been aware of it at that time. The logic in the body Submodule.iter_items usually implicitly "returns" None by falling off the end (and None is bound as the StopIteration exception's value attribute), which is its intended behavior, but sometimes it inadvertently "returns" a list_iterator object instead.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions