Skip to content

createNodeBuilder, declaration emit, and associated utility port #791

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 66 commits into from
May 29, 2025

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Apr 11, 2025

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 weswigham#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

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be interested to see if we can move any of this out of the checker package; it's getting pretty big in there.

(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.)

This needs more pulled in from stashed old work to fill it out,
(part of the way through the declaration transform itself and resolver)
and it's useless without the nodebuilder itself done, but has the full
declaration emit pipeline hooked in and tests enabled - another thing
showing just how not complete it is! If the PR was already massive,
this won't help~

For real, this is all going to have to be un-integrated to be at all
mergable in the end, but all up it will be able to signal completion
with green CI in the end, so long as nobody blind baseline-accepts...
¯\_(ツ)_/¯
…ans type parameter smuggling and module specifier generation
@weswigham weswigham marked this pull request as ready for review April 30, 2025 22:06
@weswigham
Copy link
Member Author

I have weswigham#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.

@jakebailey
Copy link
Member

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
Copy link
Member

jakebailey commented May 29, 2025

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

weswigham commented May 29, 2025

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 weswigham enabled auto-merge May 29, 2025 04:53
@weswigham
Copy link
Member Author

And CI finally green 👍

@weswigham weswigham added this pull request to the merge queue May 29, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch May 29, 2025
@andrewbranch andrewbranch added this pull request to the merge queue May 29, 2025
Merged via the queue into microsoft:main with commit 6fc5cb8 May 29, 2025
23 checks passed
@hamidrezahanafi
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants