Use parser to parse tsconfig json instead of using Json.parse by sheetalkamat · Pull Request #12336 · microsoft/TypeScript

sheetalkamat · 2016-11-17T23:52:36Z

This uses our object literal parser to parse the json text. New api called parseJsonText and return JsonSourceFile - syntax tree
There is also api convertToJson that converts JsonSourceFile to json
New api called parseJsonSourceFileConfigFileContent that takes this JsonSourceFile to convert to actual ParsedCommandLine and convert the JsonSourceFile to json in one shot so there are error locations for the invalid json options in the tsconfig.json
As part of this change I had to refactor code a little bit so that parser can be included with minimal changes into the jsTypingsInstaller
JsonSourceFile is stored as configFile in compilerOptions so the options related diagnostics can be reported in the actual tsconfig file
config file names are included in project files, and getSemanticDiagnostic on the config file returns the project errors and options diagnostics in that file

This fixes build of typings installer

Also handle the JsonNode when converting to parsedCommandLine

… json node

…roject errors

mhegazy · 2017-01-27T23:04:17Z

src/compiler/diagnosticMessages.json

+        "category": "Error",
+        "code": 1320
+    },
+    "String, number, object, array, true, false or null expected.": {


how about: A property value can only string literal, numeric literal, 'true', 'false', 'null', object literal or array literal.

mhegazy · 2017-01-27T23:05:01Z

src/compiler/diagnosticMessages.json

        "category": "Error",
        "code": 1319
    },
+    "String literal with double quotes expected.": {


how about: A property name must be wrapped in double quotes.

This is not just for the property name though. It could be value that is string literal. Json file can only accept string literals that are double quoted

mhegazy · 2017-01-27T23:31:37Z

src/compiler/parser.ts

+                sourceFile.endOfFileToken = <EndOfFileToken>parseTokenNode();
+            }
+            else if (token() === SyntaxKind.OpenBraceToken ||
+                lookAhead(() => token() === SyntaxKind.StringLiteral)) {


does this happen often? i am assuming this is to handle [ "compilerOptions" : ..., but do we need to ?

This is to handle one of the failing test case which was used to sanitize the tsconfig.json

Could you give a comment why we need a look ahead to be string literal? also would the the property is allow to be identifier since we use parseObjectLiteralExpression to parse the object?

According to ECMA-404, the key for the property of a JSON object must be a string literal.

mhegazy · 2017-01-27T23:39:09Z

src/compiler/commandLineParser.ts

      * @param jsonText The text of the config file
      */
-    export function parseConfigFileTextToJson(fileName: string, jsonText: string, stripComments = true): { config?: any; error?: Diagnostic } {
+    export function parseConfigFileTextToJson(fileName: string, jsonText: string): { config?: any; error?: Diagnostic } {


this is a public method, so i would leave the optional parameter to avoid breaking users who pass it along.

What do we do if strip comments is false?

mhegazy · 2017-01-27T23:45:10Z

src/compiler/commandLineParser.ts

+      * Read tsconfig.json file
+      * @param fileName The path to the config file
+      */
+    export function readConfigFileToJsonSourceFile(fileName: string, readFile: (path: string) => string): JsonSourceFile {


how about readJsonConfigFile.

mhegazy · 2017-01-28T00:13:08Z

src/compiler/types.ts

    export interface TsConfigOnlyOption extends CommandLineOptionBase {
        type: "object";
+        optionDeclarations?: CommandLineOption[];
+        extraKeyDiagnosticMessage?: DiagnosticMessage;


just report one message unknown option '{0}' for all of the top level nodes.

Hmm.. currently we report unknown compiler option | unknown type acquisition option. I tried to keep the same behavior. Is it ok to change the behavior?

mhegazy · 2017-01-28T00:57:21Z

src/compiler/commandLineParser.ts

+        {
+            name: "compilerOptions",
+            type: "object",
+            optionDeclarations: optionDeclarations,


Ok, here is an idea, just merge them all in one big object, and make type accept an array of CommandlineOptions along with string, number, and list. then for the maps, we can switch them to have a new type called "map", and an optional mapElements member, similar to how we handle list today.

you can also just keep the optionsDeclarations, but can we just combine all the options in one object, instead of two separate hierarchies?

But those are two different hierarchies isn't it? The options specified here can only be present at root of json object and other command line options are possible only inside object called compiler Options?

mhegazy · 2017-01-28T00:58:44Z

src/compiler/commandLineParser.ts

+     * @param jsonNode
+     * @param errors
+     */
+    export function convertToJson(sourceFile: JsonSourceFile, errors: Diagnostic[]): any {


nit. i would say these functions should be convert*ToObject and not to json.

mhegazy · 2017-01-28T01:09:26Z

src/compiler/commandLineParser.ts

+            compileOnSave = convertCompileOnSaveOptionFromJson(json, basePath, errors);
+        }
+        else {
+            options = getDefaultCompilerOptions(configFileName);


Combined with my other suggestion of having one options definition, can we just parse the whole file, and generate diagnostics for all known options, then extract them here afterwards?

mhegazy · 2017-01-28T01:14:49Z

src/compiler/program.ts

+        }
+
+        function createDiagnosticForOptionName(message: DiagnosticMessage, option1: string, option2?: string) {
+            createDiagnosticForOption(/*onKey*/ true, option1, option2, message, option1, option2);


it would be better to call the same createDiagnosticForOption twice, once for each option, instead of passing the second key through out the whole stack.

This avoid iterating in compiler options object literal multiple times to find those two options. We have most of the error messages reported on 2 options and there are very select messages that are reported on single option so it seems ok to pass that undefined for those few scenarios?

…ctual json object, allowing to continue compilation even if there are errors in the tsconfig files

sheetalkamat · 2017-06-01T21:44:27Z

The build is failing because of incorrect version of tslint.

ghost · 2017-06-06T19:45:22Z

src/compiler/types.ts

        sourceFiles: SourceFile[];
    }

+    export interface JsonSourceFile extends SourceFile {


This shouldn't extend from SourceFile. There's an existing SourceFileLike interface, could you use that? Or just create a SourceFileBase with common properties.

ghost

I don't really understand why some functions need to be moved, but if it works, 👍
Be sure to test that we give an error for trailing commas.
We should also have a test for trying to use single quotes or unquoted properties, or a spread property.
The absolute nittiest of nits: We should allow \u2028 in json files.

ghost · 2017-06-06T21:39:23Z

src/compiler/commandLineParser.ts

+    /**
+     * Convert the json syntax tree into the json value
+     */
+    export function convertToObject(sourceFile: JsonSourceFile, errors: Diagnostic[]): any {


Should this be @internal?
Also, I'd recommend using errors: Push<Diagnostic>, since it's really an output, not an input.
It would be a good idea to separate parse tree -> object conversion and options validation. This function does both at once.

ghost · 2017-06-06T21:40:50Z

src/compiler/commandLineParser.ts

+                    errors.push(createDiagnosticForNodeInSourceFile(sourceFile, element.name, extraKeyDiagnosticMessage, keyText));
+                }
+                const value = convertPropertyValueToJson(element.initializer, option);
+                if (typeof keyText !== undefined && typeof value !== undefined) {


Did you mean keyText !== undefined?

This was merged like that??

Looks like an overlooked bug.

https://github.com/Microsoft/TypeScript/blob/master/src/compiler/commandLineParser.ts#L1056

Surely it was meant to be typeof keyText !== 'undefined' and same for value.

@sheetalkamat @Andy-MS can you please have a look? This looks like a bug.
Thanks!

ghost · 2017-06-06T21:41:17Z

src/compiler/commandLineParser.ts

+        let extendedConfigPath: Path;
+
+        const optionsIterator: JsonConversionNotifier = {
+            onSetValidOptionKeyValueInParent(parentOption: string, option: CommandLineOption, value: CompilerOptionsValue) {


Type annotations on parameters are not necessary, those should be inherited from JsonConversionNotifier.

ghost · 2017-06-06T21:42:23Z

src/compiler/commandLineParser.ts

+                    if (!isDoubleQuotedString(valueExpression)) {
+                        errors.push(createDiagnosticForNodeInSourceFile(sourceFile, valueExpression, Diagnostics.String_literal_with_double_quotes_expected));
+                    }
+                    reportInvalidOptionValue(option && (typeof option.type === "string" && option.type !== "string"));


Nit: refactor to check option && typeof option.type === "string" only once.

ghost · 2017-06-06T21:42:53Z

src/compiler/commandLineParser.ts

+                    // vs what we set in the json
+                    // If need arises, we can modify this interface and callbacks as needed
+                    if (option) {
+                        const { elementOptions, extraKeyDiagnosticMessage, name: optionName } = <TsConfigOnlyOption>option;


This cast isn't valid. Someone might have passed an object literal to an option that shouldn't have an object value. We will report an error, but still continue on to this point.

ghost · 2017-06-06T21:55:46Z

src/server/project.ts

            return result;
        }

+        hasConfigFile(configFilePath: NormalizedPath) {


I would prefer isConfigFile -- it doesn't return whether the project has a config file, it returns whether the parameter is the path to one of the project's config files.

ghost · 2017-06-06T21:56:06Z

src/server/project.ts

                // if current structure version is the same - return info without any changes
                if (this.projectStructureVersion === this.lastReportedVersion && !updatedFileNames) {
-                    return { info, projectErrors: this.projectErrors };
+                    return { info, projectErrors: this.getGlobalProjectErrors() };


Can you explain the change here? It's gone from erporting every error to returning only errors with no associated file. (What errors would those be?)

ghost · 2017-06-06T21:57:19Z

src/server/session.ts

+        }
+
+        private convertToDiagnosticsWithLinePositionFromDiagnosticFile(diagnostics: Diagnostic[]) {
+            return diagnostics.map(d => <protocol.DiagnosticWithLinePosition>{


Casted object literal

ghost · 2017-06-06T21:57:37Z

src/services/utilities.ts

        return false;
    }

+    export function cloneCompilerOptions(options: CompilerOptions): CompilerOptions {


Maybe we should just make sure clone correctly handles enumerability?

ghost · 2017-06-06T21:57:50Z

tests/cases/unittests/matchFiles.ts

+        {
+            const actual = ts.parseJsonConfigFileContent(json, host, basePath, existingOptions, configFileName, resolutionStack);
+            expected.errors = map(expected.errors, error => {
+                return <Diagnostic>{


Casted object literal

yuit · 2017-06-07T19:44:45Z

src/compiler/transformers/utilities.ts

@@ -0,0 +1,180 @@
+/* @internal */


I like that these function are pulled from factory.ts but would get @rbuckton opinion as well.

I'm curious as to why they needed to be moved.

yuit · 2017-06-07T20:05:51Z

src/compiler/parser.ts

+                sourceFile.endOfFileToken = <EndOfFileToken>parseTokenNode();
+            }
+            else if (token() === SyntaxKind.OpenBraceToken ||
+                lookAhead(() => token() === SyntaxKind.StringLiteral)) {


Could you give a comment why we need a look ahead to be string literal? also would the the property is allow to be identifier since we use parseObjectLiteralExpression to parse the object?

yuit · 2017-06-07T20:06:50Z

src/compiler/parser.ts

+        }
+
        function getLanguageVariant(scriptKind: ScriptKind) {
            // .tsx and .jsx files are treated as jsx language variant.


this comment probably should be removed

yuit · 2017-06-07T20:07:58Z

src/compiler/parser.ts

        function getLanguageVariant(scriptKind: ScriptKind) {
            // .tsx and .jsx files are treated as jsx language variant.
-            return scriptKind === ScriptKind.TSX || scriptKind === ScriptKind.JSX || scriptKind === ScriptKind.JS ? LanguageVariant.JSX : LanguageVariant.Standard;
+            return scriptKind === ScriptKind.TSX || scriptKind === ScriptKind.JSX || scriptKind === ScriptKind.JS  || scriptKind === ScriptKind.JSON ? LanguageVariant.JSX : LanguageVariant.Standard;


Should we LanguageVariant.JSX be renamed ? since it is more than just TSX and JSX

yuit · 2017-06-07T20:43:09Z

src/compiler/types.ts


+    export interface JsonSourceFile extends SourceFile {
+        jsonObject?: ObjectLiteralExpression;
+        extendedSourceFiles?: string[];


Is this another tsconfig. this tsconfig extends from??

mhegazy · 2017-06-13T00:11:12Z

@sheetalkamat let's get this ready to be merged

rbuckton · 2017-06-13T01:25:58Z

src/compiler/types.ts

    }

+    export interface JsonSourceFile extends SourceFile {
+        jsonObject?: ObjectLiteralExpression;


While it makes sense for parsing tsconfig.json, JSON isn't necessarily always an object literal. In the lib reference PR, I started using ts.parseConfigFileTextToJson to parse a single libs.json file shared between the gulpfile and jakefile. I'd have to make some changes since libs.json has an array as its root.

rbuckton · 2017-06-13T01:29:25Z

src/compiler/transformers/utilities.ts

@@ -0,0 +1,180 @@
+/* @internal */


I'm curious as to why they needed to be moved.

rbuckton · 2017-06-13T01:31:15Z

src/compiler/utilities.ts

+    /**
+     * Gets flags that control emit behavior of a node.
+     */
+    export function getEmitFlags(node: Node): EmitFlags | undefined {


Why was this moved here? All of the other emitNode related functions are still in factory, including the corresponding setEmitFlags function.

rbuckton · 2017-06-13T01:32:12Z

src/compiler/utilities.ts

        return node.kind === SyntaxKind.ParenthesizedExpression;
    }

+    export function skipPartiallyEmittedExpressions(node: Expression): Expression;


Why was this moved? All of the other related skip functions are still in factory.ts.

rbuckton · 2017-06-13T01:42:52Z

src/compiler/parser.ts

+                sourceFile.endOfFileToken = parseExpectedToken(SyntaxKind.EndOfFileToken, /*reportAtCurrentPosition*/ false, Diagnostics.Unexpected_token);
+            }
+            else {
+                parseExpected(SyntaxKind.OpenBraceToken);


If we are only planning on using this to parse tsconfig.json, then this is fine. If we are planning on using this to parse other JSON files, there are other legal root objects for a JSON file (e.g. Array Literals, string literals, numeric literals, true, false, and null).

mihailik · 2017-06-19T17:10:02Z

Is there a written up rationale for this change anywhere?

I can think of several reasons to do custom-parse JSON (and a few reasons not to as well!) — so it would be very helpful to clarify the intention here. Or add a link to the detailed explanation, if it's already mentioned.

Many thanks!

kitsonk · 2017-06-20T06:33:53Z

It is part of the Roadmap for 2.4 in order to provide better error reporting for tsconfig.json.

DanielRosenwasser · 2017-06-22T23:48:23Z

This will probably be going into the 2.4.2 which is slated for next month.

sheetalkamat added 3 commits November 17, 2016 14:45

Parse json using our own parser

cca98c3

Reorganize functions so commandline parser can include parser.

b3f816b

This fixes build of typings installer

Keep the original api and add new api that handles the JsonNode

0b3074f

Also handle the JsonNode when converting to parsedCommandLine

msftclas added the cla-not-required label Nov 17, 2016

sheetalkamat added 4 commits November 23, 2016 13:17

Merge branch 'master' into ownJsonParsing

25be912

Handle typingOptions to typeAcquisition rename when converting parsed…

005123f

… json node

Merge branch 'master' into ownJsonParsing

6419de9

Clean up api so that parsing returns JsonSourceFile

0dd944a

sheetalkamat force-pushed the ownJsonParsing branch from 68cc8fa to 0dd944a Compare November 30, 2016 00:01

sheetalkamat added 7 commits November 29, 2016 16:08

Merge branch 'master' into ownJsonParsing

c7744a8

Merge branch 'master' into ownJsonParsing

f83e754

Merge branch 'master' into ownJsonParsing

236e15a

Merge branch 'master' into ownJsonParsing

0bbbc51

Merge branch 'master' into ownJsonParsing

d3ce95f

Report option diagnostic in tsconfig.json if possible

911511e

Update the protocol to return file name in compiler diagnostics and p…

98bd310

…roject errors

sheetalkamat requested review from mhegazy and vladima January 23, 2017 23:09

mhegazy suggested changes Jan 30, 2017

View reviewed changes

kieferrm mentioned this pull request Feb 6, 2017

February Iteration Plan microsoft/vscode#20021

Closed

38 tasks

mjbvz mentioned this pull request Feb 8, 2017

Ensure the tsconfig.json validation is exposed microsoft/vscode#20163

Closed

sheetalkamat added 7 commits February 8, 2017 11:45

Encorporated PR feedback

cb1b164

Merge branch 'master' into ownJsonParsing

6f568b3

Merge branch 'master' into ownJsonParsing

d7e9609

Merge branch 'master' into ownJsonParsing

b60de87

Simplifying the json conversion notifier

c97b389

Merge branch 'master' into ownJsonParsing

ec2f967

Get configFiles as part of file names

ea60e99

This was referenced May 15, 2017

Add config file names as part of project file names #15867

Merged

Support importing json files as modules with commonjs or node module resolution #13665

Closed

sheetalkamat added 3 commits May 30, 2017 11:37

Always return empty object when converting the json sourcefile into a…

7db76ed

…ctual json object, allowing to continue compilation even if there are errors in the tsconfig files

Merge branch 'master' into ownJsonParsing

8d771ca

Merge branch 'master' into ownJsonParsing

8f4f6c5

sheetalkamat added 2 commits June 5, 2017 10:28

Merge branch 'master' into ownJsonParsing

d680720

Merge branch 'master' into ownJsonParsing

c4ad151

mhegazy approved these changes Jun 6, 2017

View reviewed changes

Merge branch 'master' into ownJsonParsing

48189c8

ghost reviewed Jun 6, 2017

View reviewed changes

yuit reviewed Jun 7, 2017

View reviewed changes

rbuckton suggested changes Jun 13, 2017

View reviewed changes

sheetalkamat mentioned this pull request Jun 15, 2017

Resolve the PR comments from #12336 #16560

Closed

Merge branch 'master' into ownJsonParsing

09f0b34

sheetalkamat closed this Jun 15, 2017

sheetalkamat reopened this Jun 15, 2017

msftclas added the cla-not-required label Jun 15, 2017

sheetalkamat merged commit 4b3e661 into master Jun 15, 2017

cevek mentioned this pull request Jun 18, 2017

ts-node doesn't work with latest typescript 2.5 TypeStrong/ts-node#360

Closed

mihailik mentioned this pull request Jun 20, 2017

Comparison typeof with undefined (not "undefined" string) in commandParser #16660

Closed

DanielRosenwasser added this to the TypeScript 2.4.2 milestone Jun 22, 2017

ghost mentioned this pull request Jun 23, 2017

Breaking change: Does not accept "lib": ["ES5"], #16710

Closed

rbuckton modified the milestones: TypeScript 2.5, TypeScript 2.4.2 Jul 19, 2017

sheetalkamat deleted the ownJsonParsing branch October 10, 2017 18:07

microsoft locked and limited conversation to collaborators Jun 19, 2018

Comments

Conversation

sheetalkamat commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sheetalkamat commented Jun 1, 2017

Uh oh!

ghost Jun 6, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost left a comment • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sheetalkamat commented Nov 17, 2016 •

edited

Loading

ghost Jun 6, 2017 •

edited by ghost

Loading

ghost left a comment •

edited by ghost

Loading