Transformer: hoistVariableDeclaration does not work correctly with class fields

[Jack-Works]

Bug Report

The hoisted variables in class fields are not pre-class. This makes all instance of the class shares the same temp variable. TS class transformers should be applied if there are hoist temp variables in the class field initializer position.

🔎 Search Terms

class fields hoistVariableDeclaration

🕗 Version & Regression Information

• Tested on 4.3.2

💻 Code

import * as ts from 'typescript'
const code = `class T {
    field = 1
}`

const out = ts.transpileModule(code, {
    compilerOptions: { target: ts.ScriptTarget.ESNext },
    transformers: { before: [transfromer()] },
})
console.log(out.outputText)

function transfromer() {
    return (ctx: ts.TransformationContext) => {
        return (sourceFile: ts.SourceFile) => {
            function visitor(node: ts.Node): ts.Node {
                const parent = node.parent
                if (ts.isPropertyDeclaration(parent) && parent.initializer === node) {
                    const temp = ctx.factory.createTempVariable(ctx.hoistVariableDeclaration, true)
                    return ctx.factory.createImmediatelyInvokedArrowFunction([ctx.factory.createReturnStatement(temp)])
                }
                return ts.visitEachChild(node, visitor, ctx)
            }
            return ts.visitEachChild(sourceFile, visitor, ctx)
        }
    }
}

🙁 Actual behavior

var _a;
class T {
    field = (() => {
        return _a;
    })();
}

🙂 Expected behavior

class T {
    constructor() {
        var _a;
        this.field = (() => {
            return _a;
        })();
    }
}

[orta]

/cc Ron Buckton (@rbuckton) - what do you think should happen here?

[rbuckton]

Exactly what is happening. The transformer in the example runs before any other transforms, meaning that it gets exactly what was written:

class T {
  field = 1;
}

Since no other transformations have run, we haven't created a constructor() {} into which to hoist the variable declaration. As a result, the current nearest scope into which a variable declaration can be hoisted is the source file.

[rbuckton]

Jack Works (@Jack-Works), this is working as intended. It would help to have more context as to what you are trying to accomplish with the above code.

[Jack-Works]

Ron Buckton (@rbuckton) I want to do some transform that's need an extra temporal variable in the class field position. That requires an extra scope to contain the variable, because the class field is initialized each instances, even in the specification the class field evaluates like an IIFE. I think the transformer should automatically add an IIFE if the variable hosting happened in the class field position.

[Jack-Works]

class T {
    field = () => {
        var _a
        return _a
    }()
}

[rbuckton]

Our transformer doesn't do anything automatic like that. If you want to declare variables in an IIFE, that's fine, but you have to do your own scope management:

function transformer() {
    return (ctx: ts.TransformationContext) => {
        return (sourceFile: ts.SourceFile) => {
            function visitor(node: ts.Node): ts.Node {
                const parent = node.parent
                if (ts.isPropertyDeclaration(parent) && parent.initializer === node) {
                    // start your own lexical environment for the IIFE
                    ctx.startLexicalEnvironment();

                    const temp = ctx.factory.createTempVariable(ctx.hoistVariableDeclaration, true)

                    // end your IIFE's lexical environment
                    const declarations = ctx.endLexicalEnvironment();

                    // merge the hoisted declarations and your return statement
                    const statements: Statement[] = [];
                    if (declarations) statements.push(...declarations);
                    statements.push(ctx.factory.createReturnStatement(temp));
                    
                    // return your IIFE
                    return ctx.factory.createImmediatelyInvokedArrowFunction(statements)
                }
                return ts.visitEachChild(node, visitor, ctx)
            }
            return ts.visitEachChild(sourceFile, visitor, ctx)
        }
    }
}

[rbuckton]

NOTE: Even with the above example, its never safe to rely on .parent in a transformer. Any transformer that might come before your transformer can produce a tree where .parent isn't valid.

[Jack-Works]

I think it's a footgun because it works normally "by accident" because it only breaks when the target is ESNext. And if I cannot use .parent what should I rely on?

[rbuckton]

I think it's a footgun because it works normally "by accident" because it only breaks when the target is ESNext.

I'm not sure what you mean by "by accident"? A class body is not a valid var declaration scope. Neither is a field initializer. The only valid var declaration scope is outside the class declaration. Since you're registering your transformer as a "before" transformer, as far as I can tell it should always put the temporary variable outside of the class. An "after" transformer would process after all other transformations (in which case you might not have a PropertyDeclaration to visit, depending on your emit target).

And if I cannot use .parent what should I rely on?

Rather than visiting a child and looking up for its parent, you would visit the parent and look down to its child:

function transformer() {
    return (ctx: ts.TransformationContext) => {
        return (sourceFile: ts.SourceFile) => {
            function visitor(node: ts.Node): ts.Node {
                if (ts.isPropertyDeclaration(node)) {
                    return ctx.factory.updatePropertyDeclaration(node,
                        ts.visitNodes(node.decorators, visitor),
                        ts.visitNodes(node.modifiers, visitor),
                        ts.visitNode(node.name, visitor),
                        node.questionToken || node.exclamationToken,
                        node.type,
                        ts.visitNode(node.initializer, visitInitializer));
                }
                return ts.visitEachChild(node, visitor, ctx)
            }
            function visitInitializer(node: Expression) {
                // start your own lexical environment for the IIFE
                ctx.startLexicalEnvironment();

                const temp = ctx.factory.createTempVariable(ctx.hoistVariableDeclaration, true)

                // end your IIFE's lexical environment
                const declarations = ctx.endLexicalEnvironment();

                // merge the hoisted declarations and your return statement
                const statements: Statement[] = [];
                if (declarations) statements.push(...declarations);
                statements.push(ctx.factory.createReturnStatement(temp));
                    
                // return your IIFE
                return ctx.factory.createImmediatelyInvokedArrowFunction(statements);
            }
            return ts.visitEachChild(sourceFile, visitor, ctx)
        }
    }
}

.parent pointers are unsafe in transformations because we reuse untransformed parts of the parse tree. We can't change the .parent pointers of parse tree nodes because it would break type checker operations that occur during transformation, and because parse tree nodes can be reused during incremental parsing scenarios such as in the language service and watch mode.