-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Ensure JsonSourceFile has all the non-optional properties of SourceFile #26162
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
src/compiler/parser.ts
Outdated
@@ -719,6 +719,7 @@ namespace ts { | |||
// Set source file so that errors will be reported with this file name | |||
sourceFile = createSourceFile(fileName, ScriptTarget.ES2015, ScriptKind.JSON, /*isDeclaration*/ false); | |||
sourceFile.flags = contextFlags; | |||
noPragmas(sourceFile); |
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.
Why is this done here. You dont want to set these for tsconfig files but set them only for modules which is done is parseSourceFile
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.
In types.ts
we declare interface JsonSourceFile extends SourceFile
. So we will sometimes have a SourceFile
object that actually comes from a .json
file. When we try accessing properties that are non-optional on SourceFile
, we'll crash if the json file didn't define them. (Like at for (const ref of sourceFile.referencedFiles)
in getEditsForFileRename.ts
which is the source of microsoft/vscode#55665)
The other option would be to make the properties added by noPragmas
optional and handle them at the use-site, if you'd prefer that.
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 think you want to set them you want to do that in parseSourceFile
https://github.com/Microsoft/TypeScript/blob/master/src/compiler/parser.ts#L693
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.
parseSourceFile
won't be called for a json source file though. (See the branch in createSourceFile
that calls parseJsonText
instead of parseSourceFile
.) But we could move the code to createSourceFile
which they both call, would that be better?
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.
@sheetalkamat Thoughts?
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 think you want to change
if (languageVersion === ScriptTarget.JSON) {
result = Parser.parseJsonText(fileName, sourceText, languageVersion, /*syntaxCursor*/ undefined, setParentNodes);
}
else {
result = Parser.parseSourceFile(fileName, sourceText, languageVersion, /*syntaxCursor*/ undefined, setParentNodes, scriptKind);
}
to
if (languageVersion === ScriptTarget.JSON) {
result = Parser.parseSourceFile(fileName, sourceText, languageVersion, /*syntaxCursor*/ undefined, setParentNodes, ScriptKind.Json);
}
else {
result = Parser.parseSourceFile(fileName, sourceText, languageVersion, /*syntaxCursor*/ undefined, setParentNodes, scriptKind);
}
Fixes microsoft/vscode#55665