Improve JSON parser error recovery #42657
Conversation
typescript-bot
added
For Backlog Bug
src/compiler/commandLineParser.ts
Outdated
| @@ -1790,7 +1790,26 @@ namespace ts { | |||
| return returnValue ? {} : undefined; | |||
| } | |||
|
|
|||
| return convertPropertyValueToJson(sourceFile.statements[0].expression, knownRootOptions); | |||
| const rootExpression: Expression = sourceFile.statements[0].expression; | |||
| if (rootExpression.kind !== SyntaxKind.ObjectLiteralExpression) { | |||
There was a problem hiding this comment.
This is common functionality for json modules as well as tsconfig.
The way tsconfig ensures its object is by passing knownRootOptions? This should not break module which has array.
There was a problem hiding this comment.
It does break, because there’s no name for the root of the object itself. We try to create an error message that says Compiler option '{0}' requires a value of type 'object' but there’s nothing to substitute for {0}, and the error doesn’t make much sense calling the root object itself a “compiler option.” So I think there should be dedicated logic for the root somewhere, because there should be a dedicated error message.
This is common functionality for json modules as well as tsconfig.
Hah, ok, the failing test shows this. Very surprising since this is in commandLineParser.ts, but I’ll find another place for the tsconfig root error.
There was a problem hiding this comment.
Hah, ok, the failing test shows this. Very surprising since this is in commandLineParser.ts
that’s just from fact that we wrote this for tsconfig first and then added support for json modules
|
The approach seems reasonable - the only thing I'm a little concerned about is whether there are any places in the compiler/LS where we try to ask for brackets on a node (since they won't exist here) |
We generally don’t do any interactive stuff with JSON files in the language service since editors (at least VS Code) use another set of providers for JSON. The only way we discovered this was through API use and the playground. But if you have a suggestion of something else to test, I’ll add it. |
| @@ -1782,15 +1806,16 @@ namespace ts { | |||
| /*@internal*/ | |||
| export function convertToObjectWorker( | |||
| sourceFile: JsonSourceFile, | |||
| rootExpression: Expression | undefined, | |||
There was a problem hiding this comment.
Would this potentially be a polymorphism deopt?
|
@typescript-bot perf test this faster I don’t know if any of our perf tests resolve JSON modules though |
|
Heya @andrewbranch, I've started to run the abridged perf test suite on this PR at b6a2f39. You can monitor the build here. Update: The results are in! |
|
@andrewbranch Here they are:Comparison Report - master..42657
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
src/compiler/commandLineParser.ts
Outdated
| @@ -1767,11 +1767,35 @@ namespace ts { | |||
| onSetUnknownOptionKeyValueInRoot(key: string, keyNode: PropertyName, value: CompilerOptionsValue, valueNode: Expression): void; | |||
| } | |||
|
|
|||
| function convertConfigFileToObject(sourceFile: JsonSourceFile, errors: Push<Diagnostic>, optionsIterator: JsonConversionNotifier): any { | |||
There was a problem hiding this comment.
You want to use this in parseConfigFileTextToJson as well since thats tsconfig ?
Does two loosely related things:
{} identifier,identifieractually belongs to a node, instead of being a token totally unaccounted for in the parse tree (which triggers a debug failure if the language service finds out about it). This fixes the remaining repro of Debug Failure. Did not expect SourceFile to have an Identifier in its trivia at addSyntheticNodes #39854.Neither of these bugs are very important, and parser changes are inherently somewhat risky, so this should wait until 4.3.
Fixes #39854
Fixes #37850