-
Notifications
You must be signed in to change notification settings - Fork 637
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
Conversation
…ogic excluding ID, JSX, and symbol names
There was a problem hiding this 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... ¯\_(ツ)_/¯
…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. |
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. |
(for example) |
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 |
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. |
I think I've quashed all the |
And CI finally green 👍 |
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 |
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 ofcreateNodeBuilder
's closure environment, lifted into members of a struct. Allcontext
parameters have been removed and replaced with lookups ofcontext
on the struct itself, since we're OO now, but that's about the only refactor.nodebuilderapi.go
is the wrapper returned bycreateNodeBuilder
which 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 upcontext
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:
isolatedDeclarations
support and associated node reuse logic (this is a large amount of error checking code for very little practical payoff)EmitContext
s don't need to be arguments to functions on thenodebuilderapi
createSemicolonDeferringWriter
actually needs to be ported or if it overlaps with the printer flag of the same)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...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!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.modulespecifiers
package)