Skip to content
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

createNodeBuilder and associated utility port #791

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

weswigham
Copy link
Member

TODO, annotated with current thoughts on each:

  • Mapped type node creation (Generates tons of custom scopes and symbols, so useless without other in-progress work anyway. Also kinda just lost it among the symbol functions 'til I went back and surveyed what was still missing. Might hop back to this and get it in so at least all the type structures are done.)
  • JSDoc (mostly in ID related content, but a little bit in js declaration and ts @overload content)
  • JS Declarations more generally (symbolTableToDeclarationStatements and friends)
  • Synthetic comments (scattered blocks of functionality throughout the node builder to attach comments to the output - mostly for quickfixes)
  • String quote style preservation (a recent TS feature is preserving single vs double quotes through the emit pipeline, but some API changes precluded initial support)
  • Isolated declarations (expressionToTypeNode.ts, plus some wrappers)
    • Contained node reuse logic also used by non-ID, so also needed for good declaration map support now that sourcemaps are in (but maybe we unwind the extraction to a separate syntactic resolver object to also fix all the bugs the extraction introduced?)
  • Symbol name construction (symbolToNode and other flavors - lots of very similar but subtly different functions with little corner cases for services support)
    • Module specifier generation (moduleSpecifiers.ts) <-- current work item, looking at generating this more automatically with our tooling since it's fairly standalone and doesn't interact with many refactored APIs
    • Entity name accessibility checking and chain generation (isSymbolAccessible, getAccessibleSymbolChain and the like - these are unimplemented stubs right now, for want of the above, and are a bit unwieldy - also likely candidates for a perf rewrite at some point in the future, as the historically slow part of declaration emit)
  • Any unported factory/checker/utils that those depend on implicitly that I haven't run into yet (insert mining meme here)
  • Updates from expandable hover after that merges to strada (notably, lots of scattered approximatelength additions that may not show up as any test failures if not ported)
  • Make singlelinetextwriter usage threadsafe to support concurrent checking (likely one writer per checker instead of a global)
  • Factor all the node building logic into a subfolder so the growing set of files/loc is less painful on the compile times of checker (probably do this very last)
  • Allow threading through EmitContext to individual factory calls (via symbol tracker?) so the context used by the declaration transform can be correctly used within the node builder, while it still falls back to the checker's internal emit context for diagnostic production or quickfixes and the like that don't need their own emit context
  • Memoize printer setup per-checker, rather than making a new printer on each builder request to stay threadsafe (maybe the checker should have its' own emit context wrapper to track emit/print related state like this?)
    • Investigate if OmitTrailingSemicolon printer option is redundant with the trailing semicolon eliding wrapper-writer we use in strada - only one of these two may need to be ported
  • Reintroduce some way to smuggle signature type arguments out of the node builder for quickinfo (previously we patched a .typeArguments onto every signature kind)
  • Actual declarations.ts transform once those are all (or mostly) in
    • TS declarations
    • JS declarations
    • Isolated declarations

I've updated my branch to replace the current typeToString implementation with the ported node-builder backed implementation, which is the top-down goal of this PR, ultimately. 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. I'll try to keep this PR in a state where it at least compiles without error, which is usually in between big chunks of functionality, but the tests are going to keep throwing unimplemented panics until all the missing functions are either fully ported or at least sufficiently stubbed (which doesn't feel worth it, really).

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.

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

@@ -8,6 +8,7 @@ import (
"github.com/microsoft/typescript-go/internal/stringutil"
)

// TODO: Definitely not threadsafe - make one per checker instead of one global one (threadlocal storage would be neat)
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, a sync.Pool of these would be best.

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.

2 participants