ENB: A *valid* mypy complaint

[Edward K. Ream]

This Engineering Notebook post discusses a mypy complaint that probably should never be eliminated. Feel free to ignore, unless you are Félix :-)

Two Position methods, p.moveToNthChild and p.moveToParent, set p.v = None if there is no such position.  This is a fundamental feature of all of the p.moveTo* methods! All these methods change p in place. They do not return a new position.

Here is p.moveToNthChild:

def moveToNthChild(self, n: int):
    p = self
    if p.v and len(p.v.children) > n:
        p.stack.append((p.v, p._childIndex))
        p.v = p.v.children[n]
        p._childIndex = n
    else:
        # mypy rightly doesn't like setting p.v to None.
        # Leo's code must use the test `if p:` as appropriate.
        p.v = None  # type: ignore
    return p

As noted in the comment, the only valid way to test whether any of the p.moveTo methods yields a valid position is to test `if p:` or `if not p:`.  The p.__bool__ special method enforces this convention:

def __bool__(self):
    """
    Return True if a position is valid.
    
    The tests 'if p' or 'if not p' are the _only_ correct ways

    to test whether a position p is valid.
    
    Tests like 'if p is None' or 'if p is not None' will not

    work properly.
    """
    return self.v is not None

To repeat, mypy rightly complains that assigning None to p.v is a type error. Changing the declaration of p.v in p.__init__ from:

self.v: "VNode" = v

to

self.v: Optional["VNode"] = v

creates a cascade of (valid!) mypy complaints. Rather than attempt a (possibly heroic) fix, I think it best to leave the two `type: ignore` statements in place, with comments explaining their significance.

Summary

Setting p.v to None is a real type error.  Imo, it would not be helpful to pretend otherwise. Setting p.v to None works only because:

1. p.__bool__ tests p.v.

2. Leonine scripts must test `if p:` or `if not p:` after calling p.moveTo* methods.

There is no way to change this special case now. It affects all Leonine scripts. Any anyway, I would not likely change this coding convention even if I could.

All comments welcome. I would especially like to know how leojs handles this issue.

Edward

[tbp1...@gmail.com]

There could be a special instance of p that plays the role of null, in that it returns None when tested.  Call it p_none.  mypy would recognize it as the same type as type(p) so it would be type safe, and p.__bool__ would return False when tested if p were p_none.  There would only need to be one p_none instance.

I'm not sure how many different places in the codebase that p is set to None.  If it's only a few, changing them all to set p to p_none instead shouldn't be hard or dangerous.

[Edward K. Ream]

On Wednesday, March 3, 2021 at 10:37:01 AM UTC-6 tbp1...@gmail.com wrote:

Thanks for these comments. They inspired me to dig a little deeper into what is going on.

There could be a special instance of p that plays the role of null, in that it returns None when tested.  Call it p_none.  mypy would recognize it as the same type as type(p) so it would be type safe, and p.__bool__ would return False when tested if p were p_none.  There would only need to be one p_none instance.

This won't work. Recall that p.moveTo* methods change p in place. They don't return a value, and there is no way in python to alter references to p in the caller.

I'm not sure how many different places in the codebase that p is set to None.  If it's only a few, changing them all to set p to p_none instead shouldn't be hard or dangerous.

There are only two methods, but several other position methods call those two methods, so all told there are about a dozen. In any case, setting p to p_none is simply impossible.

mypy gives 27 errors when I annotate p.v to be Optional["VNode"] . These errors basically all say that p.v.any is invalid if p.v is None. This is perfectly proper on mypy's part. As I said earlier, Leo's scripting api works because all scripts, including Leo's core, are supposed to test p after any of the p.moveTo* methods. In practice, these tests happen naturally. The typical patterns are:

for p in c.all_positions():

    whatever

or

while p:

   whatever

   p.moveToThreadNext()

In the first example, the test happens behind the scenes. In the second example, the test is part of the loop.

One could imagine removing the 27 errors by using guards, but nothing would be gained by doing so.  Suppose the guard fails.  Now what? I'm not going to pollute code with error returns, so the only choice is to raise a Leo-specific exception, say, ScriptingError. But Leo's main loop already catches all exceptions, so there is no real benefit.

Edward

[tbp1...@gmail.com]

Oops, I see that I should ave written about a special value for p.v, not for p.  For example, leo.core.leoNodes.VNode could have an "invalid" flag that would be tested for in p.__bool__.  Would that make it more feasible?

OTOH, this is legacy code, so probably your solution of having mypy ignore the type mismatch is perfectly fine as you said.

[vitalije]

Setting p.v to None is just a convention. I don't think that any other part of Leo depends on this convention except p.__bool__

It would be trivial to change p.__bool__ implementation for example: instead of: return self.v is not None, we could have return self._valid. And then instead of setting self.v = None, just: self._valid = False. This would remove mypy complains, although I am not sure if it is really necessary.  The only thing that needs to be dealt with is searching for tests p.v is None or p.v is not None, but I doubt that there are many such places.

[Edward K. Ream]

On Wed, Mar 3, 2021 at 1:38 PM tbp1...@gmail.com <tbp1...@gmail.com> wrote:

Oops, I see that I should ave written about a special value for p.v, not for p. 

The special value is None :-) I want to emphasize the following important points:

1. Leo should crash when any script, including Leo's core, tries to access p.v.whatever when p.v is None.

Imo, it would be a serious blunder to set p.v to anything other than None in p.moveToParent and p.moveToNthChild. We want Leo to crash as soon as possible. Setting p.v to anything other than None would make debugging harder by delaying the point at which the problems arise. In effect, setting p.v to anything other than None would put a time bomb in Leo.

2. There is nothing wrong with the present code that should be "fixed". The only question is how to keep mypy happy.

3. leojs works exactly the same way. Here are the relevant parts of the Félix's code:

/**
 * Create a new position with the given childIndex and parent stack.
 */
constructor(v: VNode, childIndex: number = 0, stack?: StackEntry[]) {
    this._childIndex = childIndex;
    this.v = v;
    if (stack) {
        this.stack = [...stack]; // Creating a copy here is safest and best.
    } else {
        this.stack = [];
    }
}

/**
 * Return True if a position is valid.
 *  
 * The tests 'if p' or 'if not p' are the _only_ correct ways to test
 * whether a position p is valid.
 *  
 * Tests like 'if p is None' or 'if p is not None' will not work properly.
 */
public __bool__():boolean {
    return !!this.v;
}

/**
 * Move to Nth child
 */
public moveToNthChild(n:number):Position {
    const p:Position = this;
    if (p.v && p.v.children.length > n){
        p.stack.push([p.v, p._childIndex]);

        p.v = p.v.children[n];
        p._childIndex = n;

    }else{
        // * For now, use undefined p.v to signal null/invalid positions
                    //@ts-ignore
        p.v = undefined;
    }
    return p;
}

Summary

Leo's devs must not become confused by type checking issues. Setting p.v to None is the best way to indicate an invalid position.

Edward