Thanks—this mostly looks fine, modulo one question I have. Also I'd like to get a second opinion from @dennwc.

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dennwc and @zurk)

bblfsh/node.py, line 61 at r1 (raw file):

        self._children = self._sync_children()

    def _sync_children(self) -> List["Node"]:

The fact that this didn't trigger errors before suggests we must not be checking type annotations in our tests.

bblfsh/node.py, line 247 at r1 (raw file):

            d["@pos"] = {
                "@type": "uast:Positions",
                "start": Node.DEFAULT_POSITION,

The problem with this change is that if the caller edits any one of the nodes that uses the default position, every node already extant with an alias of that dict will be effectively moved.

If you do want to consolidate the default, I suggest using a private function to generate a new instance of the default position each time. In any case, it probably doesn't need to be part of the public interface.

Cannot comment about type annotations though. Can be changed later on our side.

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @zurk)

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @zurk)

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @zurk)

a discussion (no related file):
Hmm, tests fail now in the CI.

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @zurk)

bblfsh/node.py, line 260 at r4 (raw file):

        return {
            "@type": "uast:Position",
            "offset": 0,

On the second thought, it might not be a good idea to change offset from -1 to 0. It will make the position appear valid and break sorting and other things. line and col set to 0 are fine though - those are 1-based, thus 0 is considered an invalid value.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @zurk)

bblfsh/node.py, line 260 at r4 (raw file):

OK, as long as those positions remain in the compatibility layer we may change them to 0 and update the test.

But instead of updating the position to 1, can you please change the expected order instead?

Thanks! from my side, waiting for @creachadair.

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @zurk)

bblfsh/node.py, line 260 at r4 (raw file):

Yes, exactly. It calls to libuast and SDK, where we check positions for a valid range. The 0 line and column is considered invalid, but it seems like it accepts -1 for offset, which is incorrect. Will open an issue for it.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @zurk)

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dennwc)

a discussion (no related file):

Done.

bblfsh/node.py, line 247 at r1 (raw file):

yes, totally makes sense.

Reviewable status: complete! all files reviewed, all discussions resolved