Use NodeFlags to detect nodes in ambient contexts instead of climbing ancestors #17831
Conversation
ghost
requested a review
from 
ghost
mentioned this pull request
|
@rbuckton Any comments? |
src/compiler/parser.ts
Outdated
| @@ -613,19 +613,21 @@ namespace ts { | |||
| 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?
src/services/breakpoints.ts
Outdated
| @@ -31,7 +31,8 @@ namespace ts.BreakpointResolver { | |||
| } | |||
|
|
|||
| // 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 |
src/compiler/parser.ts
Outdated
| @@ -5089,6 +5104,18 @@ namespace ts { | |||
| 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.
src/compiler/checker.ts
Outdated
| @@ -17081,7 +17081,7 @@ namespace ts { | |||
| if (targetDeclarationKind !== SyntaxKind.Unknown) { | |||
| 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.
src/compiler/checker.ts
Outdated
| @@ -22740,7 +22740,7 @@ namespace ts { | |||
| } | |||
| } | |||
| 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)