-
Notifications
You must be signed in to change notification settings - Fork 702
Implement getJSSyntacticDiagnosticsForFile
#1723
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
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.
Pull Request Overview
This PR implements getJSSyntacticDiagnosticsForFile
to generate diagnostics for TypeScript-only constructs in JavaScript files. The implementation also removes extra blank lines in -pretty false
mode and includes semantic diagnostics even when syntax errors exist.
- Adds new JS-specific syntactic error checking that identifies TypeScript constructs in JS files
- Cleans up diagnostic output formatting by removing unnecessary blank lines
- Improves language service behavior by including semantic diagnostics alongside syntax errors
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.
Seems fine to me, though it does slow down parsing of JS files to do the second pass:
goos: linux
goarch: amd64
pkg: github.com/microsoft/typescript-go/internal/parser
cpu: Intel(R) Core(TM) i9-10900K CPU @ 3.70GHz
│ old.txt │ new.txt │
│ sec/op │ sec/op vs base │
Parse/Herebyfile.mjs/tsc-20 941.2µ ± 1% 1032.6µ ± 1% +9.71% (p=0.000 n=10)
Parse/Herebyfile.mjs/server-20 956.7µ ± 3% 1020.3µ ± 4% +6.65% (p=0.000 n=10)
geomean 948.9µ 1.026m +8.17%
│ old.txt │ new.txt │
│ B/op │ B/op vs base │
Parse/Herebyfile.mjs/tsc-20 461.8Ki ± 0% 462.0Ki ± 0% +0.04% (p=0.000 n=10)
Parse/Herebyfile.mjs/server-20 461.8Ki ± 0% 462.0Ki ± 0% +0.04% (p=0.000 n=10)
geomean 461.8Ki 462.0Ki +0.04%
│ old.txt │ new.txt │
│ allocs/op │ allocs/op vs base │
Parse/Herebyfile.mjs/tsc-20 1.761k ± 0% 1.766k ± 0% +0.28% (p=0.000 n=10)
Parse/Herebyfile.mjs/server-20 1.761k ± 0% 1.766k ± 0% +0.28% (p=0.000 n=10)
geomean 1.761k 1.766k +0.28%
This is nothing "new", the old code had this problem.
I don't see a way around this unless we move this code into the parser itself (which, I was surprised to learn it wasn't).
if program.Options().GetEmitDeclarations() { | ||
diagnostics = append(diagnostics, program.GetDeclarationDiagnostics(ctx, file)) | ||
} | ||
diagnostics := make([][]*ast.Diagnostic, 0, 4) |
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 had thought this code was correct originally; I guess you're changing it to match that VS Code on the client side requested syntax and semantic in sequence? (I've been trying to find equivalent code but I think this is just somewhere that differs in general with Strada given the fine-grained-ness of tsserver.)
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 simply noticed a behavioral difference, with semantic error red squiggles appearing and disappearing depending on whether there are syntax errors as you're typing. LSP doesn't distinguish between syntactic/semantic so we need to return all of them to get consistent behavior.
-plainJSGrammarErrors.js(29,5): error TS8009: The 'export' modifier can only be used in TypeScript files. | ||
plainJSGrammarErrors.js(30,5): error TS1031: 'export' modifier cannot appear on class elements of this kind. | ||
+plainJSGrammarErrors.js(34,9): error TS8017: Signature declarations can only be used in TypeScript files. |
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.
Seemingly a difference here.
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.
Probably inconsequential, given it's a missing body error in JS. Which, sort of funny that we can detect that there's a syntax error for JS, as that implies that this check could be elsewhere.
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.
We already have grammar checks for all modifiers, so the check for export
in Strada is duplicative (as evident from the diagnostics). Also, it's not even correct, export
is permitted in JS.
plainJSGrammarErrors.js(60,7): error TS1030: 'async' modifier already seen. | ||
plainJSGrammarErrors.js(61,1): error TS1042: 'async' modifier cannot be used here. | ||
plainJSGrammarErrors.js(62,5): error TS1042: 'async' modifier cannot be used here. | ||
-plainJSGrammarErrors.js(62,5): error TS8009: The 'async' modifier can only be used in TypeScript files. |
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.
This error seems to still be missing? But, probably "good" because the old error was duplicated for some reason.
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.
Another case of duplicate checks.
In this PR:
getJSSyntacticDiagnosticsForFile
which generates diagnostics for TypeScript-only constructs in .js files.-pretty false
mode.Note that
getJSSyntacticDiagnosticsForFile
is now called byParseSourceFile
such that it becomes part of the concurrent parsing of source files. In Strada it was deferred and called upon the first request to obtain syntactic diagnostics for a file. The function effectively constitutes an extra pass over every parsed .js file. Longer term we could consider integrating the logic with the parser itself.This PR supersedes #1387.
Fixes #1386.