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. |
|
I don't want to spam the PR with this comment, but I think we're trying to put any exported function shims in |
| return &SymbolTrackerImpl{context, inner, false, tchost} | ||
| } | ||
|
|
||
| func (this *SymbolTrackerImpl) GetModuleSpecifierGenerationHost() modulespecifiers.ModuleSpecifierGenerationHost { |
There was a problem hiding this comment.
Hey, it is analogous and it's not a keyword, so why not keep it similar where we actually did use objects/classes in Strada?
There was a problem hiding this comment.
I guess this is the only class in the old codebase, but I still don't like it 😄
| type SymbolTrackerImpl struct { | ||
| context *NodeBuilderContext | ||
| inner nodebuilder.SymbolTracker |
There was a problem hiding this comment.
The number of things that are types that are Impl or Interface and members that are impl or inner makes me feel like something is in the wrong place and/or we’re trying to be too clever with polymorphism. How much of this is temporary until more things are moved out of checker.ts?
There was a problem hiding this comment.
The SymbolTracker interface defined outside checker is required - otherwise the printer and checker end up circularly dependent on one another, so don't work. I did just push a removal of the NodeBuilderInterface since, for now at least, it's not strictly required.
|
|
|
Definite perf issue - it completes locally for me in checks around 50 seconds. |
|
I think the perf issue is in the test suite - pprof shows almost all 50 seconds spent in |
|
Hm, unfortunate; the test suite doesn't have a way to skip a diff yet, but could a la |
|
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. |
|
No, unfortunately the test timeouts are global as golang/go#48157 isn't yet implemented. |
| return &SymbolTrackerImpl{context, inner, false, tchost} | ||
| } | ||
|
|
||
| func (this *SymbolTrackerImpl) GetModuleSpecifierGenerationHost() modulespecifiers.ModuleSpecifierGenerationHost { |
There was a problem hiding this comment.
I guess this is the only class in the old codebase, but I still don't like it 😄
| @@ -519,6 +520,7 @@ type WideningContext struct { | |||
| } | |||
|
|
|||
| type Program interface { | |||
| Host | |||
There was a problem hiding this comment.
I don't really like us going back to the old days of "everything's a host you have to forward though"; is there a way to avoid this?
There was a problem hiding this comment.
...? No? The program is, by request, the only argument to making a checker, ergo it must provide all external, configurable/queryable state functionality the checker needs. All the metadata/state must flow through it, since it's the only point of contact between the checker and anything else. None of the methods added by this host are onerous for a normal program to implement, either (hence why the emit host, which wraps a program, can also easily provide them) - they're either methods on its own internal FS/host, or metadata it's already calculating during setup (like package json cache info). Like, you can expose the underlying things the program owns to implement some of the additional methods directly, like the FS or Resolver, but all that does is push complexity farther down and make implementing the interface for test/secondary use harder by accidentally exposing more scope...
There was a problem hiding this comment.
Well, this goes back to my question about "is a Program a host, or does it have a host"? And I prefer the latter but I can get why the former is more convenient
There was a problem hiding this comment.
Historically, it is a host (with an internal host of it's own for fs access) that we wrap/window/subset/augment as needed. In strada it's the primary implementation of TypeCheckerHost - we've just changed the term in use here - rather than naming the interface checker.TypeCheckerHost or checker.Host, we named it checker.Program. Just because, I guess.
There was a problem hiding this comment.
It used to be that they were in the same package (compiler), so it was concrete before and was just left as an interface when the refactor happened.
| @@ -38,7 +38,7 @@ func NewCommonJSModuleTransformer(emitContext *printer.EmitContext, compilerOpti | |||
| tx.assignmentPatternVisitor = emitContext.NewNodeVisitor(tx.visitAssignmentPattern) | |||
| tx.languageVersion = compilerOptions.GetEmitScriptTarget() | |||
| tx.moduleKind = compilerOptions.GetEmitModuleKind() | |||
| return tx.newTransformer(tx.visit, emitContext) | |||
| return tx.NewTransformer(tx.visit, emitContext) | |||
There was a problem hiding this comment.
Is this just because the declaration emitter is not in this same package?
There was a problem hiding this comment.
Yeah, but defining transforms outside the core transform package could also be done in some quickfixes and would be done for some lint rule (fixes) - these methods on the base transform should be exported, imo, to support defining transforms where's they're needed, rather than only in one place.
There was a problem hiding this comment.
Yeah, I think declaration emit being "different" compared to the other transforms seems fine. It really does do something different than downleveling transforms. (I kinda want the other transforms in a different package, too.)
| @@ -22,6 +23,42 @@ type fakeProgram struct { | |||
| getResolvedModule func(currentSourceFile *ast.SourceFile, moduleReference string) *ast.SourceFile | |||
| } | |||
|
|
|||
| func (p *fakeProgram) FileExists(path string) bool { | |||
There was a problem hiding this comment.
Yeah, having to have all of these is sort of a shame but maybe there's no way around it
There was a problem hiding this comment.
Yup. I certainly don't wanna leak all the stuff on program's internal bits into the checker scope - minimally it'd make writing the interfaces incredibly painful.
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)