Integrate JS syntax checks with parser and remove separate pass #1732
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR integrates JavaScript syntax checks directly into the parser instead of performing them as a separate pass. This optimization eliminates the ~10% parsing overhead previously incurred when parsing JavaScript files by consolidating error detection into the main parsing process.
Key changes:
- Removed the separate
getJSSyntacticDiagnosticsForFilefunction and replaced it with inline JS validation during parsing - Added helper functions like
checkNoJSExtraModifiers,checkNoJSTypeParameters, etc. to validate JS-specific constraints - Modified multiple parsing functions to call these validation helpers for JS files
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
internal/parser/parser.go |
Main implementation replacing separate JS syntax pass with inline validation during parsing |
testdata/baselines/reference/submodule/conformance/*.errors.txt |
Updated baseline error outputs reflecting improved error positioning and categorization |
testdata/baselines/reference/submoduleAccepted/conformance/*.errors.txt.diff |
Diff files showing baseline changes for accepted submodule tests |
internal/parser/parser.go
Outdated
| } | ||
|
|
||
| func (p *Parser) jsErrorAtRange(loc core.TextRange, message *diagnostics.Message, args ...any) { | ||
| p.jsDiagnostics = append(p.jsDiagnostics, ast.NewDiagnostic(nil, core.NewTextRange(scanner.SkipTrivia(p.sourceText, loc.Pos()), loc.End()), message, args...)) |
There was a problem hiding this comment.
The first parameter to ast.NewDiagnostic is nil but should be the source file. This will cause issues when the diagnostic is processed since the file information won't be properly attached until later. Consider passing p.sourceFile or similar reference here.
| p.jsDiagnostics = append(p.jsDiagnostics, ast.NewDiagnostic(nil, core.NewTextRange(scanner.SkipTrivia(p.sourceText, loc.Pos()), loc.End()), message, args...)) | |
| p.jsDiagnostics = append(p.jsDiagnostics, ast.NewDiagnostic(p.sourceFile, core.NewTextRange(scanner.SkipTrivia(p.sourceText, loc.Pos()), loc.End()), message, args...)) |
Copilot uses AI. Check for mistakes.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
internal/parser/parser.go
Outdated
| p.parseSemicolon() | ||
| result := p.finishNode(p.factory.NewVariableStatement(modifiers, declarationList), pos) | ||
| p.withJSDoc(result, hasJSDoc) | ||
| p.checkNoJSExtraModifiers(result) |
There was a problem hiding this comment.
[nitpick] The JS validation checks are scattered throughout multiple parsing functions. Consider consolidating these checks into a single validation function that can be called after node creation to reduce code duplication and improve maintainability.
| p.checkNoJSExtraModifiers(result) | |
| p.validateNode(result) |
Copilot uses AI. Check for mistakes.
| collectExternalModuleReferences(result) | ||
| if ast.IsInJSFile(node) { | ||
| result.SetJSDiagnostics(getJSSyntacticDiagnosticsForFile(result)) | ||
| result.SetJSDiagnostics(attachFileToDiagnostics(p.jsDiagnostics, result)) |
There was a problem hiding this comment.
Do we even need the concept of JS diagnostics anymore? Could these just be parse errors?
There was a problem hiding this comment.
The distinction only matters for whether we report grammar errors: If there are syntactic errors, we suppress generation of grammar errors. But JS diagnostics don't count towards this (which is reasonable given that we didn't have an issue with the actual syntax, it's just that it isn't allowed). None of this matters with the command-line compiler because we only report grammar and semantic errors when there are no syntax errors of any kind. But it does make a difference in the IDE and in tests.
There was a problem hiding this comment.
BTW, I'm not sure if any of the above was ever part of a well considered plan, or just happened to be that way over time.
There was a problem hiding this comment.
It seems weird that the effect of this is that people can use the API to parse JS files and not actually get any diagnostics. But it's not a regression so it's probably ok.
There was a problem hiding this comment.
Oh, they'll get diagnostics. The Program.GetSyntacticDiagnostics function always returns a concatenation of regular diagnostics and JS diagnostics. It's just that grammar checks are guarded by Checker.hasParseDiagnostics which only checks if there are regular syntactic diagnostics (and ignores JS diagnostics). It's not particularly intuitive, but that's how it was, so that's how it is.
This is a follow-up to #1723 that integrates JS syntax checks with the parser and removes the separate pass that, for some reason, was used to implement the checks previously. This eliminates any additional parsing overhead for parsing JS files (the separate pass added ~10% to parse time).
The PR also fixes a few oddities in the JS syntax checks. In particular, we now include full text ranges of signatures to make it clearer that the entire construct is disallowed. I have reviewed the baseline changes and they're all intended.