`createNodeBuilder`, declaration emit, and associated utility port

[weswigham]

This branch to replaces the current typeToString implementation with the ported node-builder backed implementation, which is the top-down goal of this PR, ultimately. It also enables declaration emit and tests for it. CI on it should not pass until the node builder is mostly (if not totally) completely ported, due to interdependencies between many of its' subsystems. Baseline diffs are seperately in https://github.com/weswigham/typescript-go/pull/1 because if they were merged into this PR, it would become impossible to view in the github web UI - so we'll only do that once reviewers are done with it.

Structurally, nodebuilder.go is the inner contents of createNodeBuilder's closure environment, lifted into members of a struct. All context parameters have been removed and replaced with lookups of context on the struct itself, since we're OO now, but that's about the only refactor. nodebuilderapi.go is the wrapper returned by createNodeBuilder which maps all the internal closed over methods to the internal node builder API shape (which is recorded as the NodeBuilderInterface... interface)- basically it's the logic that handles setting up context objects for each request. Some of that might get renamed to reduce confusion eventually, but the structure seems sound. nodebuilderscopes.go may or may not go away - NodeBuilder.enterNewScope was pretty big but isolated (used by mapped type and signature construction), so felt like it could stand alone, and it has some utilities only it uses. symbolaccessibility.go is for the checker's symbol accessibility functionality - these are also mostly self-contained, though do depend on one-another and some common utilities (though I only have stubs here right now - my previous attempts to optimize them as I ported them have broken them, so we're just gonna port them straight as we can for now).

This is already a bit of a beast to review, size-wise, and I'd say there's still a fair bit left for a full port - but if we add some extra unit tests, some subsystems, like the specifier generation and maybe accessibility, can reasonably stand alone as changes. Those things just aren't currently unit tested outside of their integrations into the builder in strada, though, so those tests'd all be additional greenfield work.

The remaining features to port (from the TODOs left in the code), which may or may not make this PR or followups depending on reviewer satisfaction, are:

• [ ] Expando function declaration emit (is this fully implemented in checker now?)
• [ ] Late bound index signature declaration emit (checker implementation here is currently partial, and missing the parts required for accurate declaration emit)
• [ ] JS declaration emit support (will need to be substantively rewritten given different upfront parsing and checking of JSDoc structures - likely for the better)
• [ ] isolatedDeclarations support and associated node reuse logic (this is a large amount of error checking code for very little practical payoff)
• [ ] support for attaching type arguments to identifiers in the node builder for quickinfo
• [ ] support attaching synthetic comments to nodes for better truncated output in some modes
• [ ] extract most nodebuilder logic from the checker package into the nodebuilder package with an interface indirection over the checker (likely requires renaming a lot of things on checker to make them "public" and reworking how they're accessed)
• [ ] clean up layering of emitContext/emitResolver/nodebuilderapi layers to automatically pass through more bits so things like EmitContexts don't need to be arguments to functions on the nodebuilderapi
• [ ] bundled emit support? seems mostly cut for js output, but we're the only provider of upfront-bundled declaration output
• [ ] support preserving input quote style in declaration emit (not currently supported on the AST itself)
• [ ] Memoize printers used for diagnostics and node builder in the checker (is this even actually needed? spawning new printers on demand seems like a pretty cheap allocation)
• [ ] Support for the OmitTrailingSemicolon and NeverAsciiEscape printer options (only affects some diagnostic output minorly - also warrants investigating if createSemicolonDeferringWriter actually needs to be ported or if it overlaps with the printer flag of the same)
• [ ] project references support in module specifier generation (currently stubbed, since project references don't exist)
• [ ] cached getOutputPathsFor on the emit host (or decide if this is cheap enough a cache isn't worth bothering with)
• [ ] stripInternal support? Unsure if we planned to support this. It's not bad in the declaration emitter so long as jsdoc tag parsing is in place...
• [ ] Flatten immediately nested module objects? We now parse module a.b {} into module a { export module b {} }, but AFAIK the AST doesn't even support representing the former anymore, which makes printing it back that way for declaration emit difficult!
• [ ] resolutionMode is not currently varied with usage declarations - the helper for calculating it is missing and unused at module lookup sites. This is a more general issue across the compiler presently, but persists into this declaration emit logic.
• [ ] Symlink cache support in module specifier generation to support generating specifiers for modules not directly imported in a project (the loader doesn't keep a symlink cache around anymore as far as I can tell, so that's required before it can be reused in specifier generation)
• [ ] output path remapping in specifier generation for import maps (output path calculation helpers are currently in weird places and need some refactoring to be reused in the modulespecifiers package)
• [ ] underlineing type baseliner to resume measuring node reuse, likely after isolated declarations logic is ported

[weswigham]

I have https://github.com/weswigham/typescript-go/pull/1 separately open with the baseline changes, because if I merge those into this PR, this change goes from "practically unreviewable" to "actually impossible to navigate in the github web UI", so it's probably best if reviews of this are done before any baseline updates get merged in.

[weswigham]

I'm not sure how much you want eyes on the contents of the PR yet but I'm happy to look if you do

At this point, eyes are welcome, since we could probably merge this any time we wanted given the completeness bar for corsa right now - full stack declaration emit is functional and non-panicing on our test suite, despite some missing features (js support, for example). I've already worked through some of the more egregious bugs and differences from the initial version, too. If you wanted to look at this + baselines, there's https://github.com/weswigham/typescript-go/pull/1, but I can't recommend opening that in the github web UI.

[weswigham]

The baselines at https://github.com/weswigham/typescript-go/pull/1 have something like 2026 deleted .types.diff and .js.diff files, so there's certainly some amount of progress here~

[jakebailey]

I don't want to spam the PR with this comment, but I think we're trying to put any exported function shims in exports.go rather than rename functions to be exported, just to make it super clear where "public" things are sitting.

[jakebailey]

hugeDeclarationOutputGetsTruncatedWithError appears to time out on my machine; maybe we need to skip its output? Or, there's a perf bug?

[weswigham]

Definite perf issue - it completes locally for me in checks around 50 seconds.

[weswigham]

I think the perf issue is in the test suite - pprof shows almost all 50 seconds spent in myers.Diff...

[jakebailey]

Hm, unfortunate; the test suite doesn't have a way to skip a diff yet, but could a la fixupOld plumbed downward.

[weswigham]

Can we just specify a higher timeout for an individual test in the interim? For now, the diff is technically still useful - just expensive to calculate for a large output such as that test.

[jakebailey]

No, unfortunately the test timeouts are global as golang/go#48157 isn't yet implemented.

[jakebailey]

Seems like there's still some raciness and non-determinism; the smoke tests have a race failure and the baselines appear to have some ordering differences when using concurrent program with info from multiple checkers.

[jakebailey]

Write at 0x00c0154f5e90 by goroutine 747:
  runtime.mapassign_fast64ptr()
      /opt/hostedtoolcache/go/1.24.3/x64/src/internal/runtime/maps/runtime_fast64_swiss.go:367 +0x0
  github.com/microsoft/typescript-go/internal/core.(*LinkStore[go.shape.*github.com/microsoft/typescript-go/internal/ast.Node,go.shape.struct { github.com/microsoft/typescript-go/internal/checker.isVisible github.com/microsoft/typescript-go/internal/core.Tristate }]).Get()
      /home/runner/work/typescript-go/typescript-go/internal/core/linkstore.go:19 +0x15b
  github.com/microsoft/typescript-go/internal/checker.(*emitResolver).isDeclarationVisible()
      /home/runner/work/typescript-go/typescript-go/internal/checker/emitresolver.go:91 +0x5e
  github.com/microsoft/typescript-go/internal/checker.(*emitResolver).IsDeclarationVisible()
      /home/runner/work/typescript-go/typescript-go/internal/checker/emitresolver.go:79 +0x9b
  github.com/microsoft/typescript-go/internal/transformers/declarations.(*DeclarationTransformer).transformImportDeclaration.func1()
      /home/runner/work/typescript-go/typescript-go/internal/transformers/declarations/transform.go:1672 +0x4d
  github.com/microsoft/typescript-go/internal/core.Filter[go.shape.*uint8]()
      /home/runner/work/typescript-go/typescript-go/internal/core/core.go:20 +0x9c

Previous read at 0x00c0154f5e90 by goroutine 739:
  runtime.mapaccess1_fast64()
      /opt/hostedtoolcache/go/1.24.3/x64/src/internal/runtime/maps/runtime_fast64_swiss.go:17 +0x0
  github.com/microsoft/typescript-go/internal/core.(*LinkStore[go.shape.*github.com/microsoft/typescript-go/internal/ast.Node,go.shape.struct { github.com/microsoft/typescript-go/internal/checker.isVisible github.com/microsoft/typescript-go/internal/core.Tristate }]).Get()
      /home/runner/work/typescript-go/typescript-go/internal/core/linkstore.go:11 +0x7b
  github.com/microsoft/typescript-go/internal/checker.(*emitResolver).isDeclarationVisible()
      /home/runner/work/typescript-go/typescript-go/internal/checker/emitresolver.go:91 +0x5e
  github.com/microsoft/typescript-go/internal/checker.(*emitResolver).hasVisibleDeclarations()
      /home/runner/work/typescript-go/typescript-go/internal/checker/emitresolver.go:350 +0x187
  github.com/microsoft/typescript-go/internal/checker.(*emitResolver).isEntityNameVisible()
      /home/runner/work/typescript-go/typescript-go/internal/checker/emitresolver.go:315 +0x424
  github.com/microsoft/typescript-go/internal/checker.(*emitResolver).IsEntityNameVisible()
      /home/runner/work/typescript-go/typescript-go/internal/checker/emitresolver.go:278 +0x84
  github.com/microsoft/typescript-go/internal/transformers/declarations.(*DeclarationTransformer).checkEntityNameVisibility()
      /home/runner/work/typescript-go/typescript-go/internal/transformers/declarations/transform.go:1039 +0x7a

(for example)

[weswigham]

Seems like there's still some raciness and non-determinism; the smoke tests have a race failure and the baselines appear to have some ordering differences when using concurrent program with info from multiple checkers.

Probably missing some locks on some emit resolver APIs, I'm guessing. I fixed all the run-to-run output stability issues locally, but hadn't actually run the race tests. Looks like IsEntityNameVisible is missing a lock, from that output.

[jakebailey]

Interestingly, this is not a problem during compiler test runs it seems, only in the CLI, which we should definitely have more tests for than just the one-off smoke tests.

[weswigham]

I think I've quashed all the race issues (and gotten race mode testing running locally), but the last fix for the last one might not fix the underlying raciness, just hide it in our tests by fixing the logic bug I saw while looking over the relevant code closely - there was a missing ! in computed name elision in the declaration emitter, so weather we elided or kept late bound names was partially inverted (which, worryingly, affects surprisingly few tests). That error, in turn, removed a location where we marked late-painted statements (since we were eliding the name that definitely marks them) and caused the late painted statement visibility to only depend on weather a prior checker API call has witnessed the link between the type node and the statement and painted the visibility of the target. On reflection, I... think it's fine, with the fixed logic? So long as visibility is painted at-least-once, it's fine - so with the fixed logic, it's no longer maybe-visibility-painted-zero-times, as the declaration emitter itself is ensuring at least one check.

[weswigham]

And CI finally green 👍

[hamidrezahanafi]

hey @weswigham I filed an issue here and it seems this PR is culprit (local TSGO build with commit before this PR works fine), let me know if I can help in anyway