ENB: About new asserts in leoNodes.py and leoCommands.py

[Edward K. Ream]

This Engineering Notebook post discusses new asserts in leoNodes.py and leoCommands.py.

Unlike the previous ENB, the changes discussed here have no practical effect on Leo's reliability. In effect, the new asserts convert AttributeErrors to AssertionErrors.

Background

All the recent changes arise from changing the annotation of p.v. The new (correct!) annotation is:

self.v: VNode | None = v  # PR #4767: Yes, p.v may be None.

As you would expect, declaring that p.v may be None has ripple effects throughout Leo, especially in leoNodes.py, but to a lesser extent in leoCommands.py.

Discussion

I would not be all that terrible to keep the old annotation for p.v. As mentioned above, it really doesn't matter whether mypy knows that p.v can be None.

However, I see no reason not to correctly annotate p.v. New asserts tell mypy (and the human reader) that the code depends on p.v being a valid VNode instead of None. Moreover, the correct annotation allows the code to do without some type: ignore comments.

It would also have been possible to raise ValueError instead of AssertionError, but that would have been much more clumsy. mypy knows enough about assert statements to suppress the corresponding complaints.

Note that p._parentVnode is a special case. ValueError there makes more sense. Recent revs beef up the tests in this method.

Summary

Recent revs correct annotate p.v as VNode | None.

As a result, mypy initially complained about dozens of methods. In all cases, adding one or more asserts placated mypy. I have attached the following comment to the new asserts:

# Silence mypy warning that p.v may be None.

All your questions, comments, and suggestions are welcome.

Edward

[Thomas Passin]

On Thursday, July 2, 2026 at 12:30:43 PM UTC-4 Edward K. Ream wrote:

...

All the recent changes arise from changing the annotation of p.v. The new (correct!) annotation is:

self.v: VNode | None = v  # PR #4767: Yes, p.v may be None.

As you would expect, declaring that p.v may be None has ripple effects throughout Leo, especially in leoNodes.py, but to a lesser extent in leoCommands.py.

Discussion

I would not be all that terrible to keep the old annotation for p.v. As mentioned above, it really doesn't matter whether mypy knows that p.v can be None.

However, I see no reason not to correctly annotate p.v. New asserts tell mypy (and the human reader) that the code depends on p.v being a valid VNode instead of None. Moreover, the correct annotation allows the code to do without some type: ignore comments.

I am really getting confused here. First you say that   Yes, p.v may be None. Then you say "New asserts tell mypy (and the human reader) that the code depends on p.v being a valid VNode instead of None".  So the code cannot in fact accept p.v being None. These two statements seem to contradict each other.

Beyond that, what should happen if one of those asserts should fail? Using try/except at least gives the code a chance to do something. If nothing needs to be done, then the assert is functionally the equivalent of #type: ignore except for being more complex and an abuse of the intent of asserts. I'd rather stick with #type: ignore.

The whole approach feels rushed to me and not ready for prime time. It needs more thought, IMHO. Since as you wrote, Leo is not crashing right and left, nothing like this needs to be implemented for the next release. The approach can afford to evolve and get refined for weeks or even months until a good approach has definitely emerged.

I suspect that all of this could be avoided by declaring a NullVNode object (which should be interned) and making p.v be a property instead of an attribute such that when a bit of code sets p.v to none: p.v = None then the None gets converted to a NullVNode object. There might be an operational issue if this slows some loops too much but that can be tested to be sure. The NullVNode object would do nothing for any VNode method call. It should evaluate to False for Boolean tests.

With this approach, all existing code should keep working and mypy should be happy, since it would be impossible to construct a Position object that contains an actual None for its VNode. I don't really know Typescript but something similar ought to be possible, and that will help Felix with LeoJS and LeoWeb too.

[Edward K. Ream]

On Thu, Jul 2, 2026 at 1:01 PM Thomas Passin wrote:

> I am really getting confused here. First you say that   Yes, p.v may be None. Then you say "New asserts tell mypy (and the human reader) that the code depends on p.v being a valid VNode instead of None". 

> So the code cannot in fact accept p.v being None. These two statements seem to contradict each other.

They do indeed. This is an excellent question.

Happily, there's a simple explanation for the contradiction. Let's look at the big picture. Consider:

for p in c.all_positions():

    whatever

The c.all_positions() generator will eventually set p.v = None, thereby making p "falsey".

So the only correct annotation for p.v is Position | None.  That much is straightforward. But now consider the following:

for p in c.all_positions():

    whatever

print(p.h) # Wrong! p.v is None here.

The last statement is equivalent to:

print(p.headstring())

where p.headstring should be (and now is):

def headString(self) -> str:
    p = self
    assert p.v  # Silence mypy warning that p.v may be None.
    return p.v.headString()

So within either loop, p.v will be a VNode, but after each loop, p.v will be None.

The program error of using an empty Position (the result of an exhausted generator) does not occur within Leo. But the error could easily occur within scripts or plugins.

Please note: p.headString will throw an exception with or without the assert statement. With it, p.headString will throw an AssertionError. Without the assert, p.headString will throw an AttributeError. Either way, Leo will recover gracefully from scripting errors.

The only reason for adding the assert p.v statement is to tell mypy not to warn that p.v might be None in the statement:

return p.v.headString()

Yes, this is a lot of explanation for an assert, but the pattern holds in many places. Please feel free to ask more questions. Surely, you aren't the only person confused by this code.

Edward

[Thomas Passin]

On Thursday, July 2, 2026 at 4:30:49 PM UTC-4 Edward K. Ream wrote:

On Thu, Jul 2, 2026 at 1:01 PM Thomas Passin wrote:

> I am really getting confused here. First you say that   Yes, p.v may be None. Then you say "New asserts tell mypy (and the human reader) that the code depends on p.v being a valid VNode instead of None". 

> So the code cannot in fact accept p.v being None. These two statements seem to contradict each other.

They do indeed. This is an excellent question.

Happily, there's a simple explanation for the contradiction. Let's look at the big picture. Consider:

for p in c.all_positions():

    whatever

The c.all_positions() generator will eventually set p.v = None, thereby making p "falsey".

So the only correct annotation for p.v is Position | None.  

I don't get this. The iterator should stop when there are no more positions available. I don't see why p.v should be None in this situation. p should be the last valid position the iterator returns. I would expect it to have a non-None VNode, since it should be a valid position. How would anyone know that the last position returned may or may not have a non-None VNode? When is there a reason to have or return a position like that? And if there is a good reason, I think the docstrings and docs should make that very clear.

>>> help(c.all_positions)

Help on method all_positions in module leo.core.leoCommands:

all_positions(copy: 'bool' = True) -> 'Generator' method of leo.core.leoCommands.Commands instance

A generator return all positions of the outline, in outline order.

This doesn't even hint that a Position might have a None VNode. The Position class does have a comment that says:

A position marks the spot in a tree traversal. A position p consists of a VNode
p.v, a child index p._childIndex, and a stack of tuples (v,childIndex), one for
each ancestor **at the spot in tree traversal. Positions p has a unique set of
parents.

The p.moveToX methods may return a null (invalid) position p with p.v = None.

But your example is not a p.moveToX method.

[Edward K. Ream]

On Thu, Jul 2, 2026 at 9:38 PM Thomas Passin <tbp1...@gmail.com> wrote:

> I don't get this. The iterator should stop when there are no more positions available. I don't see why p.v should be None in this situation. p should be the last valid position the iterator returns.

That's not what happens and it never will happen. The PR annotates what is, not what you or I want it to be.

This morning I thought there was a clever workaround that would hide all the new asserts, namely raising ValueError in the p.v property. Alas, there is no such property: p.v is a plain ivar. However, I'm going to dig deeper into mypy's toolbox to see what might be possible. Stay tuned.

Edward

[Edward K. Ream]

On Friday, July 3, 2026 at 4:17:34 AM UTC-5 Edward K. Ream wrote:

On Thu, Jul 2, 2026 at 9:38 PM Thomas Passin wrote:

>> I don't get this. The iterator should stop when there are no more positions available. I don't see why p.v should be None in this situation. p should be the last valid position the iterator returns.

> That's not what happens and it never will happen. The PR annotates what is, not what you or I want it to be.

> This morning I thought there was a clever workaround that would hide all the new asserts, namely raising ValueError in the p.v property.

> Alas, there is no such property: p.v is a plain ivar. However, I'm going to dig deeper into mypy's toolbox to see what might be possible.

My idea was to use TypeGuards, but assert statements are simpler. PR #4767 is ready for review!

Edward

[Thomas Passin]

On Friday, July 3, 2026 at 5:17:34 AM UTC-4 Edward K. Ream wrote:

On Thu, Jul 2, 2026 at 9:38 PM Thomas Passin <tbp1...@gmail.com> wrote:

> I don't get this. The iterator should stop when there are no more positions available. I don't see why p.v should be None in this situation. p should be the last valid position the iterator returns.

That's not what happens and it never will happen. The PR annotates what is, not what you or I want it to be.

I didn't mean "should" in the sense of mandating a change. I meant that I didn't grasp why the iterator could produce a Position that has a None VNode.

[Edward K. Ream]

On Fri, Jul 3, 2026 at 5:10 AM Thomas Passin <tbp1...@gmail.com> wrote:

>> That's not what happens and it never will happen. The PR annotates what is, not what you or I want it to be.

> I didn't mean "should" in the sense of mandating a change. I meant that I didn't grasp why the iterator could produce a Position that has a None VNode.

Thanks for the clarification!

Edward

[Thomas Passin]

In my workbook.leo outline,

empties = []
for p in c.all_positions():
    if not p.v:
        empties.append(p.h)
g.es(len(empties))          # prints 0

In what circumstances can the iterator end with a Position having p.v = None?

[Edward K. Ream]

On Fri, Jul 3, 2026 at 7:05 AM Thomas Passin <tbp1...@gmail.com> wrote:

In my workbook.leo outline,

empties = []
for p in c.all_positions():
    if not p.v:
        empties.append(p.h)
g.es(len(empties))          # prints 0

In what circumstances can the iterator end with a Position having p.v = None?

Every time, except when there is a break or return inside the loop.

That doesn't matter in this example, because 'empties' isn't the generator.

Edward

[Edward K. Ream]

On Friday, July 3, 2026 at 7:18:10 AM UTC-5 Edward K. Ream wrote:

On Fri, Jul 3, 2026 at 7:05 AM Thomas Passin wrote:

In my workbook.leo outline,

empties = []
for p in c.all_positions():
    if not p.v:
        empties.append(p.h)
g.es(len(empties))          # prints 0

In what circumstances can the iterator end with a Position having p.v = None?

Every time, except when there is a break or return inside the loop.

That doesn't matter in this example, because 'empties' isn't the generator.

Let me say a few more words:

In this example, empties is a list of strings, so there is no need to worry about their lifetimes.

But even it empties were a list of Positions, empties would work as expected because c.all_positions is defined as:

def all_positions(self, copy: bool = True) -> Generator:

    """A generator return all positions of the outline, in outline order."""

    c = self
    p = c.rootPosition()
    while p:
        yield p.copy() if copy else p
        p.moveToThreadNext()

As you see, by default the generator returns copies of the Position objects it yields. This has been so for at least 15 years, iirc. As a result, naive users aren't going to get nasty surprises. Within Leo, there are a few places where the bool kwarg is False so that, for example, Leo's redraw code doesn't create unnecessary copies of Position object. 

HTH.

Edward