createNodeBuilder, declaration emit, and associated utility port
#791
Conversation
…ogic excluding ID, JSX, and symbol names
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... ¯\_(ツ)_/¯
…to builder-backed since thats now
…ans type parameter smuggling and module specifier generation
…/stub some missing functionality
…bol printback behavior
…lity and currently unsupported features
|
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. |
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 weswigham#1, but I can't recommend opening that in the github web UI. |
|
|
||
| if len(candidateChains) > 0 { | ||
| // pick first, shortest | ||
| slices.SortStableFunc(candidateChains, ch.compareSymbolChains) |
There was a problem hiding this comment.
I feel obligated to point out that this logic - exhaustively traversing symbol tables looking for a way to name a symbol - is the most expensive part of declaration emit, and here in the go port, it is always worst case performance if anything is exported through a renaming alias and thus dodges the direct lookup fast path - we can't early bail on finding a matching symbol because of the need to sort the results for stability.
So this logic is incredibly slow. You will notice how much slower tests are with this PR (though to some degree that was guaranteed!). This stuff has been a candidate for a rewrite for approximately forever - it dates all the way back to the old string-based declaration emitter logic, and has just grown in weird preference-encoding and edge-case-handling ever since. Making a more efficient equivalent that behaves the same way is almost impossible (I've tried), but the randomly iterated and-then-sorted symbol tables are already potentially going to break strict equivalence with old output (since the symbols we select will first be sorted by declaration location and alphabetically according to the new symbol sort algorithm, and not by creation time). Given that, this whole suite of functions (and wrappers in the node builder) is ripe for a rewrite at some point, while we're here breaking things.
weswigham
changed the title
createNodeBuilder and associated utility portcreateNodeBuilder, declaration emit, and associated utility port
|
The baselines at weswigham#1 have something like 2026 deleted |
…on emit diagnostics
…lines aside from diagnostic output)
There was a problem hiding this comment.
I've just reviewed modulespecifiers so far—aside from the transcription error, I think we should drop the auto-imports cache support code. The auto imports implementation is going to be very different and I’d prefer to revisit what optimizations are needed once it’s running. Here’s a patch:
git diff patch
diff --git a/internal/compiler/emitHost.go b/internal/compiler/emitHost.go
index d59c3ccd3..3cb278bbb 100644
--- a/internal/compiler/emitHost.go
+++ b/internal/compiler/emitHost.go
@@ -46,10 +46,6 @@ func (host *emitHost) GetGlobalTypingsCacheLocation() string {
return "" // !!! see src/tsserver/nodeServer.ts for strada's node-specific implementation
}
-func (host *emitHost) GetModuleSpecifierCache() modulespecifiers.ModuleSpecifierCache {
- return nil /// !!! see src/server/moduleSpecifierCache.ts for strada's services implementation
-}
-
func (host *emitHost) GetNearestAncestorDirectoryWithPackageJson(dirname string) string {
scoped := host.program.resolver.GetPackageScopeForPath(dirname)
if scoped != nil && scoped.Exists() {
diff --git a/internal/modulespecifiers/specifiers.go b/internal/modulespecifiers/specifiers.go
index 43daa9671..2bffae09f 100644
--- a/internal/modulespecifiers/specifiers.go
+++ b/internal/modulespecifiers/specifiers.go
@@ -22,74 +22,23 @@ func GetModuleSpecifiers(
userPreferences UserPreferences,
options ModuleSpecifierOptions,
) []string {
- return GetModuleSpecifiersWithCacheInfo(
- moduleSymbol,
- checker,
- compilerOptions,
- importingSourceFile,
- host,
- userPreferences,
- options,
- false,
- ).moduleSpecifiers
-}
-
-type ModuleSpecifierResult struct {
- kind ResultKind
- moduleSpecifiers []string
- computedWithoutCache bool
-}
-
-func GetModuleSpecifiersWithCacheInfo(
- moduleSymbol *ast.Symbol,
- checker CheckerShape,
- compilerOptions *core.CompilerOptions,
- importingSourceFile SourceFileForSpecifierGeneration,
- host ModuleSpecifierGenerationHost,
- userPreferences UserPreferences,
- options ModuleSpecifierOptions,
- forAutoImport bool,
-) ModuleSpecifierResult {
ambient := tryGetModuleNameFromAmbientModule(moduleSymbol, checker)
if len(ambient) > 0 {
- return ModuleSpecifierResult{
- kind: ResultKindAmbient,
- moduleSpecifiers: []string{ambient},
- computedWithoutCache: false,
- }
+ return []string{ambient}
}
- cacheResults := tryGetModuleSpecifiersFromCacheWorker(
- moduleSymbol,
- importingSourceFile,
- host,
- userPreferences,
- options,
- )
- if cacheResults == nil || cacheResults.moduleSourceFile == nil {
- return ModuleSpecifierResult{
- kind: ResultKindNone,
- computedWithoutCache: false,
- }
- }
- if len(cacheResults.moduleSpecifiers) > 0 {
- return ModuleSpecifierResult{
- kind: cacheResults.kind,
- moduleSpecifiers: cacheResults.moduleSpecifiers,
- computedWithoutCache: false,
- }
+ moduleSourceFile := ast.GetSourceFileOfModule(moduleSymbol)
+ if moduleSourceFile == nil {
+ return nil
}
- modulePaths := cacheResults.modulePaths
- if len(modulePaths) == 0 {
- modulePaths = getAllModulePathsWorker(
- getInfo(importingSourceFile.FileName(), host),
- cacheResults.moduleSourceFile.OriginalFileName(),
- host,
- // compilerOptions,
- // options,
- )
- }
+ modulePaths := getAllModulePathsWorker(
+ getInfo(importingSourceFile.FileName(), host),
+ moduleSourceFile.OriginalFileName(),
+ host,
+ // compilerOptions,
+ // options,
+ )
result := computeModuleSpecifiers(
modulePaths,
@@ -98,21 +47,9 @@ func GetModuleSpecifiersWithCacheInfo(
host,
userPreferences,
options,
- forAutoImport,
+ false, // forAutoImport
)
- if cacheResults.cache != nil {
- cacheResults.cache.Set(
- string(importingSourceFile.Path()),
- string(cacheResults.moduleSourceFile.Path()),
- userPreferences,
- options,
- result.kind,
- modulePaths,
- result.moduleSpecifiers,
- )
- }
-
return result
}
@@ -166,48 +103,6 @@ func tryGetModuleNameFromAmbientModule(moduleSymbol *ast.Symbol, checker Checker
return ""
}
-type cacheResult struct {
- cache ModuleSpecifierCache
- kind ResultKind
- moduleSpecifiers []string
- moduleSourceFile SourceFileForSpecifierGeneration
- modulePaths []ModulePath
-}
-
-func tryGetModuleSpecifiersFromCacheWorker(
- moduleSymbol *ast.Symbol,
- importingSourceFile SourceFileForSpecifierGeneration,
- host ModuleSpecifierGenerationHost,
- userPreferences UserPreferences,
- options ModuleSpecifierOptions,
-) *cacheResult {
- moduleSourceFile := ast.GetSourceFileOfModule(moduleSymbol)
- if moduleSourceFile == nil {
- return nil
- }
-
- cache := host.GetModuleSpecifierCache()
- if cache == nil {
- return &cacheResult{
- moduleSourceFile: moduleSourceFile,
- }
- }
- result := cache.Get(string(importingSourceFile.Path()), string(moduleSourceFile.Path()), userPreferences, options)
- if result == nil {
- return &cacheResult{
- cache: cache,
- moduleSourceFile: moduleSourceFile,
- }
- }
- return &cacheResult{
- cache: cache,
- moduleSourceFile: moduleSourceFile,
- kind: result.Kind,
- moduleSpecifiers: result.ModuleSpecifiers,
- modulePaths: result.ModulePaths,
- }
-}
-
type Info struct {
UseCaseSensitiveFileNames bool
ImportingSourceFileName string
@@ -389,7 +284,7 @@ func computeModuleSpecifiers(
userPreferences UserPreferences,
options ModuleSpecifierOptions,
forAutoImport bool,
-) ModuleSpecifierResult {
+) []string {
info := getInfo(importingSourceFile.FileName(), host)
preferences := getModuleSpecifierPreferences(userPreferences, host, compilerOptions, importingSourceFile, "")
@@ -439,7 +334,7 @@ func computeModuleSpecifiers(
if modulePath.IsRedirect {
// If we got a specifier for a redirect, it was a bare package specifier (e.g. "@foo/bar",
// not "@foo/bar/path/to/file"). No other specifier will be this good, so stop looking.
- return ModuleSpecifierResult{kind: ResultKindNodeModules, moduleSpecifiers: nodeModulesSpecifiers, computedWithoutCache: true}
+ return nodeModulesSpecifiers
}
}
@@ -483,15 +378,15 @@ func computeModuleSpecifiers(
}
if len(pathsSpecifiers) > 0 {
- return ModuleSpecifierResult{kind: ResultKindPaths, moduleSpecifiers: pathsSpecifiers, computedWithoutCache: true}
+ return pathsSpecifiers
}
if len(redirectPathsSpecifiers) > 0 {
- return ModuleSpecifierResult{kind: ResultKindRedirect, moduleSpecifiers: redirectPathsSpecifiers, computedWithoutCache: true}
+ return redirectPathsSpecifiers
}
if len(nodeModulesSpecifiers) > 0 {
- return ModuleSpecifierResult{kind: ResultKindNodeModules, moduleSpecifiers: nodeModulesSpecifiers, computedWithoutCache: true}
+ return nodeModulesSpecifiers
}
- return ModuleSpecifierResult{kind: ResultKindRelative, moduleSpecifiers: relativeSpecifiers, computedWithoutCache: true}
+ return relativeSpecifiers
}
func getLocalModuleSpecifier(
diff --git a/internal/modulespecifiers/types.go b/internal/modulespecifiers/types.go
index 6b2a842cc..5022f2547 100644
--- a/internal/modulespecifiers/types.go
+++ b/internal/modulespecifiers/types.go
@@ -37,51 +37,12 @@ type ModulePath struct {
IsRedirect bool
}
-type ResolvedModuleSpecifierInfo struct {
- Kind ResultKind
- ModulePaths []ModulePath
- PackageName string
- ModuleSpecifiers []string
- IsBlockedByPackageJsonDependencies bool
-}
-
-type ModuleSpecifierCache interface {
- Get(from string, to string, preferences UserPreferences, options ModuleSpecifierOptions) *ResolvedModuleSpecifierInfo
- Set(
- from string,
- to string,
- preferences UserPreferences,
- options ModuleSpecifierOptions,
- kind ResultKind,
- modulePaths []ModulePath,
- moduleSpecifiers []string,
- )
- SetBlockedByPackageJsonDependencies(
- from string,
- to string,
- preferences UserPreferences,
- options ModuleSpecifierOptions,
- packageName string,
- isBlockedByPackageJsonDependencies bool,
- )
- SetModulePaths(
- from string,
- to string,
- preferences UserPreferences,
- options ModuleSpecifierOptions,
- modulePaths []ModulePath,
- )
- Clear()
- Count() int
-}
-
type PackageJsonInfo interface {
GetDirectory() string
GetContents() *packagejson.PackageJson
}
type ModuleSpecifierGenerationHost interface {
- GetModuleSpecifierCache() ModuleSpecifierCache
// GetModuleResolutionCache() any // !!! TODO: adapt new resolution cache model
// GetSymlinkCache() any // !!! TODO: adapt new resolution cache model
// GetFileIncludeReasons() any // !!! TODO: adapt new resolution cache model
I’ll continue reviewing the rest, though my familiarity with Strada’s node builder is pretty poor compared to module specifier stuff—I don’t anticipate finding much interesting since it’s a faithful port.
| // Node builder utility functions | ||
|
|
||
| // You probably don't mean to use this - use `NewNodeBuilderAPI` instead | ||
| func NewNodeBuilder(ch *Checker, e *printer.EmitContext) NodeBuilder { |
There was a problem hiding this comment.
If this (and the NodeBuilder struct altogether) are not to be used directly, does it need to be exported? If this were nodeBuilder, then NodeBuilderInterface could become NodeBuilder. All the small structs above in this file also seem not to escape the checker package either.
There was a problem hiding this comment.
My goal was to move the implementation into the nodebuilder package by abstracting the Checker APIs it uses, but turns out the builder uses over 120(!) checker APIs - which isn't itself an issue beyond the effort involved, but they also require moving Type/Signature/IndexInfo (and all the derived subtypes of Type) up the stack, too (and, in turn, making their fields public or into methods), which I know I'll get... opinions on, so I'm saving that for a followup. I can lowercase it for now... but it doesn't matter either way, since it's in the internal package. Honestly, I don't know why we wouldn't default to package exported in this environment.
There was a problem hiding this comment.
Yeah, I think the reasons to keep stuff unexported are just for us, not for future importers. It keeps completions in other packages cleaner and faster, and my understanding is it’s good for whole project builds (you can edit signatures of unexported stuff without invalidating the build of downstream packages). But mainly having to name interfaces FooInterface makes me sad 😄
| } | ||
|
|
||
| func GetThisParameter(signature *Node) *Node { | ||
| // callback tags do not currently support this parameters |
There was a problem hiding this comment.
Is this comment still relevant in light of the new approach to JSDoc?
There was a problem hiding this comment.
I dunno! Maybe, maybe not? Some things are still done the old way. I implemented as much of the JS declaration emit as it seemed like we supported, but ultimately it's gonna need some touch-ups when we actually enable testing it.
| if !moduleResolutionSupportsPackageJsonExportsAndImports(moduleResolution) { | ||
| return false | ||
| } | ||
| if options.ResolvePackageJsonExports != TSUnknown { |
There was a problem hiding this comment.
Shouldn't this be options.ResolvePackageJsonImports? Or is this the Strada bug alluded to below? Definitely doesn't look right though.
There was a problem hiding this comment.
Confirmed this is a Strada bug. Luckily I think nobody ever changes imports/exports support independently. I’ll go ahead and open a Strada PR to fix.
Note also that you don’t need to check moduleResolution in Corsa, because the only values that don’t support imports/exports are gone.
| return nil | ||
| } | ||
|
|
||
| func getEffectiveBaseTypeNode(node *ast.Node) *ast.Node { |
There was a problem hiding this comment.
Same as elsewhere, I don't think we'll need this with the new jsdoc approach.
| ctx.visitedSymbolTablesMap[symId] = visitedSymbolTables | ||
| } | ||
|
|
||
| id := reflect.ValueOf(t).Pointer() // TODO: Is this seriously the only way to check reference equality of maps? |
There was a problem hiding this comment.
This is definitely unsafe. The symbol tables could get GC'd because this is hiding the pointer behind a uintptr. Do we really need to be checking reference equality?
The only way that this could be okay is if we used .UnsafePointer() (the recommended function) instead of uintptr to ensure we actually have a reference. But, this is all quite spooky.
There was a problem hiding this comment.
We use the identity of the symbol tables to check which ones we've already traversed, and by rights they should never mutate or be GC'd under us here, since we should have calculated and cached the tables by this point (which should only lose their last referrer when the checker itself is collected). Can't use symbols or symbol ids for identity, since they contain multiple tables (and, surprisingly, the same table can end up in multiple places). And exhaustive map comparison is obviously a nonstarter, as these are often huge. I can swap the pointer kind in use, if that'll help, though.
| _jsxNamespace string | ||
| _jsxFactoryEntity *ast.Node | ||
| skipDirectInferenceNodes core.Set[*ast.Node] | ||
| } | ||
|
|
||
| func NewChecker(program Program) *Checker { | ||
| func NewChecker(program Program, host Host) *Checker { |
There was a problem hiding this comment.
I don’t love this—the only implementation of Host is the private compiler.emitHost, which means you can’t easily make a fully functional checker from outside the compiler package, which will cause problems in my async LSP branch (the Project system’s checker pool will break with no clear resolution), even though compiler.emitHost is just trivially a wrapper around Program.
There was a problem hiding this comment.
I could flatten all the methods needed to satisfy all the various host interfaces onto Program, but then that headache just gets coupled to implementing a Program, no? This at least gives you the option to provide them separately, as we already do. Basically all the Host functionality requested by the checker has to do with file metadata that should be sharable cross-program...
There was a problem hiding this comment.
I don’t think there’s actually an expectation that a Checker will be backed by anything other than a compiler.Program struct; I think the interface is just there to untangle the circular dependency. (The only implementation of TypeCheckerHost in Strada is the Program that gets returned from createProgram.) I think minimal contracts are good, but I don’t see a need to pick them apart if they only ever get used in combination by one monolith. At the very least, for practical immediate purposes, there needs to be a convenient way to create a checker with just a compiler.Program from outside the compiler or checker packages.
| return identifier | ||
| } | ||
|
|
||
| // TODO: Audit usages of symbolToEntityNameNode - they should probably all be symbolToName |
There was a problem hiding this comment.
+1 for this, I remember being confused about this when working on expandable hover.
|
I don't want to spam the PR with this comment, but I think we're trying to put any exported function shims in |
This branch to replaces the current
typeToStringimplementation 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.gois the inner contents ofcreateNodeBuilder's closure environment, lifted into members of a struct. Allcontextparameters have been removed and replaced with lookups ofcontexton the struct itself, since we're OO now, but that's about the only refactor.nodebuilderapi.gois the wrapper returned bycreateNodeBuilderwhich maps all the internal closed over methods to the internal node builder API shape (which is recorded as theNodeBuilderInterface... interface)- basically it's the logic that handles setting upcontextobjects for each request. Some of that might get renamed to reduce confusion eventually, but the structure seems sound.nodebuilderscopes.gomay or may not go away -NodeBuilder.enterNewScopewas 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.gois 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:
isolatedDeclarationssupport and associated node reuse logic (this is a large amount of error checking code for very little practical payoff)EmitContexts don't need to be arguments to functions on thenodebuilderapicreateSemicolonDeferringWriteractually needs to be ported or if it overlaps with the printer flag of the same)getOutputPathsForon the emit host (or decide if this is cheap enough a cache isn't worth bothering with)stripInternalsupport? Unsure if we planned to support this. It's not bad in the declaration emitter so long as jsdoc tag parsing is in place...module a.b {}intomodule 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!resolutionModeis 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.modulespecifierspackage)