Skip to content

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

Merged
3 commits merged into from
Aug 28, 2018

Conversation

ghost
Copy link

@ghost ghost commented Aug 2, 2018

@@ -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);
Copy link
Member

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

Copy link
Author

@ghost ghost Aug 2, 2018

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.

Copy link
Member

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

Copy link
Author

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sheetalkamat Thoughts?

Copy link
Member

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);
        }

@ghost ghost merged commit cff04e6 into master Aug 28, 2018
@ghost ghost deleted the getEditsForFileRename_resolveJsonModule branch August 28, 2018 23:43
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant