Add printer.NodeFactory for printer/transformer-specific extensions

[rbuckton]

The NodeFactory in Strada has a large number of utility methods that are useful for transforms but are otherwise unnecessary for parse. In Corsa, we've either been adding these methods as loose top-level utility functions, or adding them to EmitContext, which pollutes EmitContext with unrelated methods. Both alternatives make it more cumbersome to port other transforms as you must look in multiple places to find methods missing from factory when porting.

To address this, I've added printer.NodeFactory which embeds ast.NodeFactory and extends it with these additional methods. To avoid additional complexity when invoking methods like ast.Node.Clone(factory), I've also added an ast.NodeFactoryCoercible interface that can be used to accept either an ast.NodeFactory or printer.NodeFactory, without the need to litter call sites with node.Clone(&tx.factory.Factory). The ast.NodeFactoryCoercible interface avoids needing to introduce an interface that covers the whole of ast.NodeFactory that would need to be maintained in parallel. The .AsNodeFactory() method also emulates the .AsNode() method we generally prefer to use when working with Node subtypes over the use of &node.Node.

[Copilot]

Pull Request Overview

This PR refactors the way node creation and utility methods are accessed by replacing direct calls on ast.NodeFactory with a new printer.NodeFactory that encapsulates printer/transformer–specific extensions. The key changes include:

• Migrating node creation methods (e.g. NewAssignmentExpression, NewUniqueName) from ast.NodeFactory to printer.NodeFactory.
• Updating call sites across several transformer and printer files to use the new printer.NodeFactory API.
• Removing legacy utility functions from EmitContext to reduce duplication of functionality.

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
internal/transformers/utilities.go	Removal of unused name utility functions and update of factory types.
internal/transformers/transformer.go	Refactor of factory field type from ast.NodeFactory to printer.NodeFactory.
internal/transformers/runtimesyntax.go	Update of node creation calls (e.g. assignment, logical operators) to use the new printer.NodeFactory.
internal/transformers/esmodule.go	Refactored usages of EmitContext methods to rely on printer.NodeFactory APIs.
internal/transformers/commonjsmodule.go	Consistent migration of factory calls, aligning with the new API.
internal/scanner/scanner.go	Updated parameter types to use NodeFactoryCoercible.
internal/printer/*	Updated test and helper methods to use the new factory methods.
internal/printer/emitcontext.go	Removal of EmitContext utility methods now replaced by NodeFactory.

Comments suppressed due to low confidence (2)

internal/transformers/runtimesyntax.go:1609

• Ensure that NewCommaExpression preserves the evaluation order and semantics of the comma operator as intended by the previous use of NewBinaryExpression with a comma token.

expression = tx.factory.NewCommaExpression(expression, node.Operand.Clone(tx.factory))

internal/printer/emitcontext.go:338

• Verify that the removal of EmitContext utility methods (for prologues and name generation) is fully covered by tests and that all call sites now correctly use the new NodeFactory methods.

// (removed utility functions for enforcing prologues and generating names)

[jakebailey]

Why is the NodeFactoryCoercible pattern required here to move the methods off of EmitContext? My mental model had been that transforms already run with a "context" (the EmitContext), so always have a reference to something (e.g. CommonJSModuleTransformer has Transformer which has EmitContext) and therefore don't need to have an additional way to plumb information through. I would think that you could still embed NodeFactory in a struct for convenience methods, but then have that on the emit context.

[rbuckton]

Why is the NodeFactoryCoercible pattern required here to move the methods off of EmitContext?

It's a convenience and helps to better organize related functionality. EmitContext should primarily be for storing emit related data, not for creating new nodes. NodeFactory should be used to create new nodes, but ast.NodeFactory can't take a dependence on printer.

This approach offers both consistency and convenience. Emit-related factory methods move to a NodeFactory subtype that is EmitContext aware, which is also convenient for porting since we generally refer to those methods via factory.foo in Strada.

My mental model had been that transforms already run with a "context" (the EmitContext), so always have a reference to something (e.g. CommonJSModuleTransformer has Transformer which has EmitContext) and therefore don't need to have an additional way to plumb information through.

A reference to something that is growing haphazardly as we continue to stuff more "factory"-like functionality on what is just supposed to hold a bunch of side tables.

I would think that you could still embed NodeFactory in a struct for convenience methods, but then have that on the emit context.

We already do, but spelling out tx.emitContext.Factory is fairly wordy given how frequently it's used, so we already use tx.factory as a convenient shortcut. What's inconsistent currently is that in Strada you might write:

factory.createIdentifier("...");
factory.createTempIdentifier(...);
factory.getLocalName(...);
// etc.

but in Corsa you have to write

tx.factory.NewIdentifier("...")
tx.emitContext.NewTempIdentifier(...)
getLocalName(tx.emitContext, ...)

so you need to reach to multiple places to do the same thing.

The main value of NodeFactoryCoercible is that you don't have to use the & syntax to pass along a pointer to *NodeFactory to methods like Clone or to GetLeadingCommentRanges, nor do you have to constantly write .AsNodeFactory() like we do for Node subtypes today. To be honest, due to the sheer amount of node.AsNode() calls I've written, I've been tempted to add a NodeCoercible interface as well to use with emitContext.SetOriginal et al.

[jakebailey]

I guess I can understand it, I'm just wary of introducing dynamic dispatch for convenience only if we can be a little more verbose but stay concrete / static. I would personally be fine doing address-of-embedded or adding AsNodeFactory(), which will always be free, whereas hopping through an interface may not be.

[rbuckton]

I guess I can understand it, I'm just wary of introducing dynamic dispatch for convenience only if we can be a little more verbose but stay concrete / static. I would personally be fine doing address-of-embedded or adding AsNodeFactory(), which will always be free, whereas hopping through an interface may not be.

IMO, it would be nice if Go could automatically handle address-of-embedded for arguments at call sites since it should have enough information to do so with static analysis, given it already does so for method calls.

The ~99% use case for calling node.Clone is from a transformer, so I'd much rather have that be convenient.

[jakebailey]

IMO, it would be nice if Go could automatically handle address-of-embedded for arguments at call sites since it should have enough information to do so with static analysis, given it already does so for method calls.

It can, but IIRC only if the function is inlined, which then means the interface will be devirtualized to a concrete type and then be static.

---

Locally, I took this PR and removed the NodeFactoryCoercible type and the diff gets way smaller, but one does definitely end up having to write more complicated expressions elsewhere. gopls does auto-fill them most of the time, though, completing the entire thing including the missing &.

But, if you feel strongly that this is going to be more convenient... It's not a big deal and is very easy to test later. Eventually we'll have emit benchmarks to see either way.

The other big places that Clone matters are for declaration emit, diagnostics, and the LS, so it's not all emit stuff per se, but cloning is basically only done for some form of emit, yeah.

[rbuckton]

The other big places that Clone matters are for declaration emit, diagnostics, and the LS, so it's not all emit stuff per se, but cloning is basically only done for some form of emit, yeah.

Declaration emit, diagnostics, and the LS all rely on the same emitter in any place they would use Clone, at least, that's how it works in Strada.

[jakebailey]

I don't want to block #819, so LGTM, but I still don't know if I like the interface idea and once we have emit benchmarks, I think it'd be worth double checking.

[ahejlsberg]

FWIW, I agree with @jakebailey on the interface indirection. It's one of those aspects that makes our old codebase so hard to comprehend and navigate. I really want us to avoid unnecessary abstraction.