Ensure nodes have real parents, fix parent race #970
Conversation
andrewbranch
requested review from
Copilot and
sandersn
and removed request for
Copilot
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that JSDoc import tags get real parent pointers during reparse and analysis, preventing races between the real AST and reparsed nodes.
- Attach the newly constructed
ImportDeclarationto itsJSDocImportTagin the parser. - Update checker routines to handle
KindJSDocImportTagwhen resolving import specifiers and module specifiers. - Amend the binder to avoid overwriting valid parents on reparsed nodes and add an
JSImportDeclarationfield in the AST type.
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/parser/reparser.go | Set JSImportDeclaration on JSDocImportTag nodes. |
| internal/checker/checker.go | Handle KindJSDocImportTag in import resolution. |
| internal/binder/binder.go | Only overwrite node.Parent for nil or reparsed flags. |
| internal/ast/ast.go | Add JSImportDeclaration field to JSDocImportTag. |
| @@ -30,22 +30,22 @@ module.exports = Timer; | |||
| * @param {HookHandler} handle | |||
| */ | |||
| function Hook(handle) { | |||
| >Hook : (handle: HookHandler) => void | |||
| >handle : HookHandler | |||
| >Hook : (handle: (arg: any) => void) => void | |||
There was a problem hiding this comment.
the main different effect of this alternate-parent technique is that symbols no longer print as aliases. I'll need to figure out why when I start burning down the jsdoc diffs.
| ==== index.js (4 errors) ==== | ||
| /** | ||
| * @typedef {{ [K in keyof B]: { fn: (a: A, b: B) => void; thing: B[K]; } }} Funcs | ||
| ~ |
There was a problem hiding this comment.
my best guess is that the something set the type expression parents to the synthetic type alias, inconsistent with other type expressions, and the binder change now makes it consistent with the others. I have a branch that sets Host on typedef children which should fix this.
Edit:actually it may be that all type aliases were working by mistake?
There was a problem hiding this comment.
actually it may be that all typedefs were setting their type's parent to the synthetic node. Not in the reparser, though, so through the binder's existing code.
There was a problem hiding this comment.
This is what we agreed on yesterday. Making parents consistently point to real nodes undoes the serendipitous binder set that makes typedefs work by mistake, but I have a fix for that.
I still need to figure out why symbols that resolve through synth-parent redirection no longer print as aliases. Since the type is right, it could be a quirk of the temp type printer.
|
It's probably a temporary thing, though we'll see how much this affects the node builder and declaration emit PR. |
|
waiting for #791 to merge first, probably will have to update baselines |
|
There seems to be a new logical conflict with #791 around declaration emit for JSDoc |
This is an alternative to #966 that doesn’t do any cloning. Any nodes that are shared between the real tree and reparsed nodes have parent pointers into the real tree.