Reduce null safety issues in Statement and Expression subclasses

[josephjunker]

This fixes most of the null safety issues in Statements and Expressions. I left a few that seemed like they'd be disruptive to modify.

The largest changes were widening the type of TranspileResult to be a tree rather than a flat array, and wrapping constructions of SourceNode in a helper which does a type coercion.

Most of the changes were setting the Range field to be optional everywhere and adding manual type annotations to the result variable in transpile methods, to avoid issues with never[] being inferred as a type.

Please let me know if any of these changes cause excessive merge conflicts and I can back them out.

[TwitchBronBron]

This only seems to be used once. Is there a reason we couldn't do something a little smarter that that one callsite for this specific instance? I'm okay keeping this function if you want it here, but just seemed a bit much for just one call.

[josephjunker]

Hmm, I see your point. I think that I wrote this assuming it would show up all over the place, and somehow got by without it for the rest of my changes.

The problem here is that this is to apply runtime safety, not just to satisfy the typechecker. Here's the invocation site:

    constructor(
        public left: Expression,
        public operator: Token,
        public right: Expression
    ) {
        super();
        this.range = util.createRangeFromPositionsOptional(this.left.range?.start, this.right.range?.end);
    }

Since these values can actually be undefined, I can't just add a ! to force it through or I could be covering up a potential runtime error. I also can't use some sort of default value for start or end going into the usual util.createRangeFromPositions like { line: 0, character: 0 }, because that would lead to incorrect ranges in the case where the range ought to be undefined.

I could inline the function:

    constructor(
        public left: Expression,
        public operator: Token,
        public right: Expression
    ) {
        super();

        const startPosition = this.left.range?.start;
        const endPosition = this.right.range?.end;

        if (startPosition && endPosition) {
            this.range = util.createRangeFromPositions(startPosition, endPosition);
        } else if (startPosition) {
            this.range = util.createRangeFromPositions(startPosition, startPosition);
        } else if (endPosition) {
            this.range = util.createRangeFromPositions(endPosition, endPosition);
        }
    }

Alternatively, I could give up on the idea of falling back to a single range when one range is present and the other is not, and just make the whole range undefined when either one is missing:

    constructor(
        public left: Expression,
        public operator: Token,
        public right: Expression
    ) {
        super();
        this.range = this.left.range && this.right.range
            ? util.createRangeFromPositions(this.left.range.start, this.right.range.end)
            : undefined;
    }

Do you have a preference?

[TwitchBronBron]

I think I like the "alternatively" example best. It seems to be an edge case, and it reads clean enough. Let's go with that.

[josephjunker]

Made that change 👍

[TwitchBronBron]

Looks great! Thanks for this.