Use NodeFlags to detect nodes in ambient contexts instead of climbing ancestors#17831
Conversation
|
@rbuckton Any comments? |
| let parseErrorBeforeNextFinishedNode = false; | ||
|
|
||
| export function parseSourceFile(fileName: string, sourceText: string, languageVersion: ScriptTarget, syntaxCursor: IncrementalParser.SyntaxCursor, setParentNodes?: boolean, scriptKind?: ScriptKind): SourceFile { | ||
| const isDeclarationFile = isDeclarationFileName(fileName); |
There was a problem hiding this comment.
Do we need to thread this through? Can we not just set the context flag in createSourceFile?
|
|
||
| // Cannot set breakpoint in ambient declarations | ||
| if (isInAmbientContext(tokenAtLocation)) { | ||
| // (Tokens do not have flags set, but their parents do.) |
There was a problem hiding this comment.
This is surprising, wouldn't this result in breaks in the incremental parser?
There was a problem hiding this comment.
We call parseTokenNode for tokens. parseTokenNode in turn calls finishNode, which should set these flags. If a token doesn't have the correct context then we should investigate why.
|
@rbuckton good to go? |
|
@rbuckton Bump |
rbuckton
left a comment
There was a problem hiding this comment.
A few more minor comments.
| const fullStart = getNodePos(); | ||
| const decorators = parseDecorators(); | ||
| const modifiers = parseModifiers(); | ||
| if (modifiers && modifiers.some(m => m.kind === SyntaxKind.DeclareKeyword)) { |
There was a problem hiding this comment.
Could just do if (some(modifiers, m => m.kind === SyntaxKind.DeclareKeyword)) as this does the existential check for you.
| const decl = getDeclarationOfKind(resolvedRequire, targetDeclarationKind); | ||
| // function/variable declaration should be ambient | ||
| return isInAmbientContext(decl); | ||
| return decl.flags & NodeFlags.Ambient; |
There was a problem hiding this comment.
!!(decl.flags & NodeFlags.Ambient), otherwise the return type becomes number | boolean.
| } | ||
| else { | ||
| if (modulekind >= ModuleKind.ES2015 && !isInAmbientContext(node)) { | ||
| if (modulekind >= ModuleKind.ES2015 && !((node as ImportEqualsDeclaration).flags & NodeFlags.Ambient)) { |
|
The build is failing due to a public API change. Can you verify whether the baseline changes are acceptable? |
|
Approved, once the build passes. |
This is basically the same as #17721.
Previously when checking for an ambient context, we would climb ancestors looking for something with an ambient modifier -- but usually there would be none and we would climb all the way to the root.
Now we just check against a flag that is set in the parser.
This seems to improve bind time by an average of 5% and check time by an average of 3%, with average total improvement of 1%.
Monaco - node (v8.2.1, x64)
Monaco - tsc (x86)
TFS - node (v8.2.1, x64)
TFS - tsc (x86)