From e741de7213e463d2ac9520a64489197156dfbe3a Mon Sep 17 00:00:00 2001 From: Andrii Rodionov Date: Mon, 27 Jan 2025 14:33:57 +0100 Subject: [PATCH 1/2] Throwing SyntaxError if a parsed source file is syntactically incorrect If a source file has syntax errors, the ParseError is generated, and the file is skipped from parsing. Syntactically check is based on TS compiler function `ts.getPreEmitDiagnostics`. There are a list of error codes, that can be ignored or added. --- openrewrite/src/javascript/parser.ts | 56 ++++--------------- openrewrite/src/javascript/parserUtils.ts | 48 ++++++++++++++++ .../e2e/electron-server.files.test.ts | 6 +- .../test/javascript/parser/arrow.test.ts | 24 ++++---- .../test/javascript/parser/await.test.ts | 2 +- .../test/javascript/parser/class.test.ts | 6 -- .../test/javascript/parser/decorator.test.ts | 2 +- .../test/javascript/parser/enum.test.ts | 2 +- .../javascript/parser/flowAnnotation.test.ts | 16 +++++- .../test/javascript/parser/for.test.ts | 16 +++--- openrewrite/test/javascript/parser/if.test.ts | 10 +++- .../test/javascript/parser/import.test.ts | 8 +-- .../test/javascript/parser/importType.test.ts | 15 ++++- .../test/javascript/parser/namespace.test.ts | 3 +- .../test/javascript/parser/return.test.ts | 4 +- .../test/javascript/parser/while.test.ts | 2 +- .../test/javascript/parser/with.test.ts | 1 + .../test/javascript/parser/yield.test.ts | 2 +- 18 files changed, 133 insertions(+), 90 deletions(-) diff --git a/openrewrite/src/javascript/parser.ts b/openrewrite/src/javascript/parser.ts index 391dd115..e7b042c9 100644 --- a/openrewrite/src/javascript/parser.ts +++ b/openrewrite/src/javascript/parser.ts @@ -23,7 +23,7 @@ import { randomId, SourceFile } from "../core"; -import {binarySearch, compareTextSpans, getNextSibling, getPreviousSibling, TextSpan} from "./parserUtils"; +import {binarySearch, compareTextSpans, getNextSibling, getPreviousSibling, TextSpan, hasFlowAnnotation, checkSyntaxErrors} from "./parserUtils"; import {JavaScriptTypeMapping} from "./typeMapping"; import path from "node:path"; import {ExpressionStatement, TypeTreeExpression} from "."; @@ -41,6 +41,8 @@ export class JavaScriptParser extends Parser { module: ts.ModuleKind.CommonJS, allowJs: true, esModuleInterop: true, + experimentalDecorators: true, + emitDecoratorMetadata: true }; } @@ -147,19 +149,18 @@ export class JavaScriptParser extends Parser { continue; } - if (this.hasFlowAnnotation(sourceFile)) { + if (hasFlowAnnotation(sourceFile)) { result.push(ParseError.build(this, input, relativeTo, ctx, new FlowSyntaxNotSupportedError(`Flow syntax not supported: ${input.path}`), null)); continue; } - // ToDo: uncomment code after tests fixing - // const syntaxErrors = this.checkSyntaxErrors(program, sourceFile); - // if (syntaxErrors.length > 0) { - // syntaxErrors.forEach( - // e => result.push(ParseError.build(this, input, relativeTo, ctx, new SyntaxError(`Compiler error: ${e[0]} [${e[1]}]`), null)) - // ); - // continue; - // } + const syntaxErrors = checkSyntaxErrors(program, sourceFile); + if (syntaxErrors.length > 0) { + syntaxErrors.forEach( + e => result.push(ParseError.build(this, input, relativeTo, ctx, new SyntaxError(`Compiler error: ${e[0]} [${e[1]}]`), null)) + ); + continue; + } try { const parsed = new JavaScriptParserVisitor(this, sourceFile, typeChecker).visit(sourceFile) as SourceFile; @@ -171,39 +172,6 @@ export class JavaScriptParser extends Parser { return result; } - private checkSyntaxErrors(program: ts.Program, sourceFile: ts.SourceFile) { - const diagnostics = ts.getPreEmitDiagnostics(program, sourceFile); - // checking Parsing and Syntax Errors - let syntaxErrors : [errorMsg: string, errorCode: number][] = []; - if (diagnostics.length > 0) { - const errors = diagnostics.filter(d => d.code >= 1000 && d.code < 2000); - if (errors.length > 0) { - syntaxErrors = errors.map(e => { - let errorMsg; - if (e.file) { - let {line, character} = ts.getLineAndCharacterOfPosition(e.file, e.start!); - let message = ts.flattenDiagnosticMessageText(e.messageText, "\n"); - errorMsg = `${e.file.fileName} (${line + 1},${character + 1}): ${message}`; - } else { - errorMsg = ts.flattenDiagnosticMessageText(e.messageText, "\n"); - } - return [errorMsg, e.code]; - }); - } - } - return syntaxErrors; - } - - private hasFlowAnnotation(sourceFile: ts.SourceFile) { - if (sourceFile.fileName.endsWith('.js') || sourceFile.fileName.endsWith('.jsx')) { - const comments = sourceFile.getFullText().match(/\/\*[\s\S]*?\*\/|\/\/.*(?=[\r\n])/g); - if (comments) { - return comments.some(comment => comment.includes("@flow")); - } - } - return false; - } - accept(path: string): boolean { return path.endsWith('.ts') || path.endsWith('.tsx') || path.endsWith('.js') || path.endsWith('.jsx'); } @@ -1866,7 +1834,7 @@ export class JavaScriptParserVisitor { Markers.EMPTY ), node.qualifier ? this.leftPadded(this.prefix(this.findChildNode(node, ts.SyntaxKind.DotToken)!), this.visit(node.qualifier)): null, - node.typeArguments ? this.mapTypeArguments(this.suffix(node.qualifier!), node.typeArguments) : null, + node.typeArguments ? this.mapTypeArguments(this.prefix(this.findChildNode(node, ts.SyntaxKind.LessThanToken)!), node.typeArguments) : null, this.mapType(node) ); } diff --git a/openrewrite/src/javascript/parserUtils.ts b/openrewrite/src/javascript/parserUtils.ts index 9da333b4..ebcebc0f 100644 --- a/openrewrite/src/javascript/parserUtils.ts +++ b/openrewrite/src/javascript/parserUtils.ts @@ -180,3 +180,51 @@ export function binarySearch(arr: T[], target: T, compare: (a: T, b: T) => nu } return ~low; // Element not found, return bitwise complement of the insertion point } + +export function hasFlowAnnotation(sourceFile: ts.SourceFile) { + if (sourceFile.fileName.endsWith('.js') || sourceFile.fileName.endsWith('.jsx')) { + const comments = sourceFile.getFullText().match(/\/\*[\s\S]*?\*\/|\/\/.*(?=[\r\n])/g); + if (comments) { + return comments.some(comment => comment.includes("@flow")); + } + } + return false; +} + +export function checkSyntaxErrors(program: ts.Program, sourceFile: ts.SourceFile) { + const diagnostics = ts.getPreEmitDiagnostics(program, sourceFile); + // checking Parsing and Syntax Errors + let syntaxErrors : [errorMsg: string, errorCode: number][] = []; + if (diagnostics.length > 0) { + const errors = diagnostics.filter(d => (d.category === ts.DiagnosticCategory.Error) && isCriticalDiagnostic(d.code)); + if (errors.length > 0) { + syntaxErrors = errors.map(e => { + let errorMsg; + if (e.file) { + let {line, character} = ts.getLineAndCharacterOfPosition(e.file, e.start!); + let message = ts.flattenDiagnosticMessageText(e.messageText, "\n"); + errorMsg = `${e.file.fileName} (${line + 1},${character + 1}): ${message}`; + } else { + errorMsg = ts.flattenDiagnosticMessageText(e.messageText, "\n"); + } + return [errorMsg, e.code]; + }); + } + } + return syntaxErrors; +} + +const additionalCriticalCodes = new Set([ + // Syntax errors + 17019, // "'{0}' at the end of a type is not valid TypeScript syntax. Did you mean to write '{1}'?" + 17020, // "'{0}' at the start of a type is not valid TypeScript syntax. Did you mean to write '{1}'?" + + // Other critical errors +]); + +const excludedCodes = new Set([1039, 1064, 1101, 1107, 1111, 1155, 1166, 1170, 1183, 1203, 1207, 1215, 1238, 1239, 1240, 1241, 1244, 1250, + 1251, 1252, 1253, 1254, 1308, 1314, 1315, 1324, 1329, 1335, 1338, 1340, 1343, 1344, 1345, 1360, 1378, 1432]); + +function isCriticalDiagnostic(code: number): boolean { + return (code > 1000 && code < 2000 && !excludedCodes.has(code)) || additionalCriticalCodes.has(code); +} diff --git a/openrewrite/test/javascript/e2e/electron-server.files.test.ts b/openrewrite/test/javascript/e2e/electron-server.files.test.ts index d1a12fb9..9eefce25 100644 --- a/openrewrite/test/javascript/e2e/electron-server.files.test.ts +++ b/openrewrite/test/javascript/e2e/electron-server.files.test.ts @@ -281,12 +281,10 @@ describe('electron-release-server files tests', () => { } // If not the first changenote, prefix with new line - var newChangeNote = !prevNotes.length ? '' : ' - '; + var newChangeNote = !prevNotes.length ? '' : ''; // Add the version name and notes - newChangeNote += '## ' + newVersion.name + ' - ' + newVersion.notes; + newChangeNote += '## ' + newVersion.name + '' + newVersion.notes; // Add the new changenote to the previous ones return prevNotes + newChangeNote; diff --git a/openrewrite/test/javascript/parser/arrow.test.ts b/openrewrite/test/javascript/parser/arrow.test.ts index 8484439c..6859ec8d 100644 --- a/openrewrite/test/javascript/parser/arrow.test.ts +++ b/openrewrite/test/javascript/parser/arrow.test.ts @@ -195,17 +195,19 @@ describe('arrow mapping', () => { rewriteRun( //language=typescript typeScript(` - prop: ( - name: string, - schemas: S, - self: TestFunction< - A, - E, - R, - [{ [K in keyof S]: Schema.Schema.Type }, V.TaskContext> & V.TestContext] - >, - timeout?: number | V.TestOptions - ) => void + class A { + prop: ( + name: string, + schemas: S, + self: TestFunction< + A, + E, + R, + [{ [K in keyof S]: Schema.Schema.Type }, V.TaskContext> & V.TestContext] + >, + timeout?: number | V.TestOptions + ) => void; + } `) ); }); diff --git a/openrewrite/test/javascript/parser/await.test.ts b/openrewrite/test/javascript/parser/await.test.ts index 7169b6ec..fe929a7e 100644 --- a/openrewrite/test/javascript/parser/await.test.ts +++ b/openrewrite/test/javascript/parser/await.test.ts @@ -7,7 +7,7 @@ describe('await mapping', () => { test('simple', () => { rewriteRun( //language=typescript - typeScript('await 1') + typeScript('export {}; await 1') ); }); }); diff --git a/openrewrite/test/javascript/parser/class.test.ts b/openrewrite/test/javascript/parser/class.test.ts index f6a2c2d4..b47d6b25 100644 --- a/openrewrite/test/javascript/parser/class.test.ts +++ b/openrewrite/test/javascript/parser/class.test.ts @@ -52,12 +52,6 @@ describe('class mapping', () => { typeScript('export class A {}') ); }); - test('public', () => { - rewriteRun( - //language=typescript - typeScript('public class A {}') - ); - }); test('export default', () => { rewriteRun( //language=typescript diff --git a/openrewrite/test/javascript/parser/decorator.test.ts b/openrewrite/test/javascript/parser/decorator.test.ts index eaeaad9d..ae5bfd83 100644 --- a/openrewrite/test/javascript/parser/decorator.test.ts +++ b/openrewrite/test/javascript/parser/decorator.test.ts @@ -67,7 +67,7 @@ describe('class decorator mapping', () => { `) ); }); - test('decorator on class expression', () => { + test.skip('decorator on class expression', () => { rewriteRun( //language=typescript typeScript(` diff --git a/openrewrite/test/javascript/parser/enum.test.ts b/openrewrite/test/javascript/parser/enum.test.ts index e387478a..eff75ee7 100644 --- a/openrewrite/test/javascript/parser/enum.test.ts +++ b/openrewrite/test/javascript/parser/enum.test.ts @@ -142,7 +142,7 @@ describe('empty mapping', () => { typeScript(` enum Test { A = "AA", - B, + B = undefined, C = 10, D = globalThis.NaN, E = (2 + 2), diff --git a/openrewrite/test/javascript/parser/flowAnnotation.test.ts b/openrewrite/test/javascript/parser/flowAnnotation.test.ts index 7349e917..95a812bf 100644 --- a/openrewrite/test/javascript/parser/flowAnnotation.test.ts +++ b/openrewrite/test/javascript/parser/flowAnnotation.test.ts @@ -4,7 +4,7 @@ describe('flow annotation checking test', () => { beforeAll(() => connect()); afterAll(() => disconnect()); - test('@flow in a comment in js', () => { + test('@flow in a one line comment in js', () => { const faultyTest = () => rewriteRun( //language=javascript javaScript(` @@ -18,6 +18,20 @@ describe('flow annotation checking test', () => { expect(faultyTest).toThrow(/FlowSyntaxNotSupportedError/); }); + test('@flow in a comment in js', () => { + const faultyTest = () => rewriteRun( + //language=javascript + javaScript(` + /* @flow */ + + import Rocket from './rocket'; + import RocketLaunch from './rocket-launch'; + `) + ); + + expect(faultyTest).toThrow(/FlowSyntaxNotSupportedError/); + }); + test('@flow in a multiline comment in js', () => { const faultyTest = () => rewriteRun( //language=javascript diff --git a/openrewrite/test/javascript/parser/for.test.ts b/openrewrite/test/javascript/parser/for.test.ts index 0bd49cc1..37a6deb0 100644 --- a/openrewrite/test/javascript/parser/for.test.ts +++ b/openrewrite/test/javascript/parser/for.test.ts @@ -125,7 +125,7 @@ describe('for mapping', () => { test('for-of with await and comments', () => { rewriteRun( //language=typescript - typeScript('/*a*/for/*b*/ await/*bb*/(/*c*/const /*d*/char /*e*/of /*f*/ "text"/*g*/)/*h*/ {/*j*/} /*k*/;/*l*/') + typeScript('export {};/*a*/for/*b*/ await/*bb*/(/*c*/const /*d*/char /*e*/of /*f*/ "text"/*g*/)/*h*/ {/*j*/} /*k*/;/*l*/') ); }); @@ -181,12 +181,14 @@ describe('for mapping', () => { rewriteRun( //language=typescript typeScript(` - let b; - for (b in a) return !1; - for (b of a) return !1; - let i, str; - for (i = 0; i < 9; i++) { - str = str + i; + function foo () { + let b; + for (b in a) return !1; + for (b of a) return !1; + let i, str; + for (i = 0; i < 9; i++) { + str = str + i; + } } `) ); diff --git a/openrewrite/test/javascript/parser/if.test.ts b/openrewrite/test/javascript/parser/if.test.ts index 02d26295..14ac74c2 100644 --- a/openrewrite/test/javascript/parser/if.test.ts +++ b/openrewrite/test/javascript/parser/if.test.ts @@ -82,8 +82,10 @@ describe('if mapping', () => { rewriteRun( //language=typescript typeScript(` + function foo() { for (let opt of el.options) - if (opt.selected !== opt.defaultSelected) return !0; + if (opt.selected !== opt.defaultSelected) return !0; + } `) ); }); @@ -104,8 +106,10 @@ describe('if mapping', () => { rewriteRun( //language=typescript typeScript(` - if (prevProps) - return abs(prevProps)/*a*/;/*b*/ + function foo() { + if (prevProps) + return abs(prevProps)/*a*/;/*b*/ + } `) ); }); diff --git a/openrewrite/test/javascript/parser/import.test.ts b/openrewrite/test/javascript/parser/import.test.ts index f7ea1bcc..5808f08e 100644 --- a/openrewrite/test/javascript/parser/import.test.ts +++ b/openrewrite/test/javascript/parser/import.test.ts @@ -70,7 +70,7 @@ describe('import mapping', () => { test('dynamic import', () => { rewriteRun( //language=typescript - typeScript('const module = await import("module-name");') + typeScript('export {};const module = await import("module-name");') ) }); @@ -105,13 +105,13 @@ describe('import mapping', () => { ); }); - test('experimental: import with import attributes', () => { + test('import with import attributes', () => { rewriteRun( //language=typescript typeScript(` + import type { SpyInstance } from 'jest'; + ///*{1}*/import /*{2}*/type /*{3}*/{ /*{4}*/ SpyInstance /*{5}*/} /*{6}*/ from /*{7}*/ 'jest' /*{8}*/; import SpyInstance = jest.SpyInstance; - import type SpyInstance = jest.SpyInstance; - /*{1}*/import /*{2}*/type /*{3}*/SpyInstance /*{4}*/= /*{5}*/jest.SpyInstance/*{6}*/; `) ); }); diff --git a/openrewrite/test/javascript/parser/importType.test.ts b/openrewrite/test/javascript/parser/importType.test.ts index 39aa53ed..d3d06038 100644 --- a/openrewrite/test/javascript/parser/importType.test.ts +++ b/openrewrite/test/javascript/parser/importType.test.ts @@ -131,12 +131,23 @@ describe('import type mapping', () => { //language=typescript typeScript(` export type LocalInterface = - & import("pkg", { with: {"resolution-mode": "foobar"} }).RequireInterface + & import("pkg", { with: {"resolution-mode": "require"} }).RequireInterface & import("pkg", { with: {"resolution-mode": "import"} }).ImportInterface; - export const a = (null as any as import("pkg", { with: {"resolution-mode": "foobar"} }).RequireInterface); + export const a = (null as any as import("pkg", { with: {"resolution-mode": "require"} }).RequireInterface); export const b = (null as any as import("pkg", { with: {"resolution-mode": "import"} }).ImportInterface); `) ); }); + + test('import type without qualifier an with type argument', () => { + rewriteRun( + //language=typescript + typeScript(` + declare module "ContextUtils" { + export function createContext(config: ReturnType>,): import("./types.ts").TailwindContext; + } + `) + ); + }); }); diff --git a/openrewrite/test/javascript/parser/namespace.test.ts b/openrewrite/test/javascript/parser/namespace.test.ts index 17886c2a..abb2b09d 100644 --- a/openrewrite/test/javascript/parser/namespace.test.ts +++ b/openrewrite/test/javascript/parser/namespace.test.ts @@ -166,7 +166,7 @@ describe('namespace mapping', () => { typeScript(` /*pref*/ declare namespace /*middle*/ TestNamespace /*after*/ { /*bcd*/ - /*1*/ a = 10 + /*1*/ const a = 10; /*efg*/ /*2*/ function abc() { return null @@ -244,6 +244,7 @@ describe('namespace mapping', () => { rewriteRun( //language=typescript typeScript(` + export {} declare namespace MyLibrary { function sayHello(name: string): void; } diff --git a/openrewrite/test/javascript/parser/return.test.ts b/openrewrite/test/javascript/parser/return.test.ts index ba5be7ce..553e8655 100644 --- a/openrewrite/test/javascript/parser/return.test.ts +++ b/openrewrite/test/javascript/parser/return.test.ts @@ -7,13 +7,13 @@ describe('return mapping', () => { test('simple', () => { rewriteRun( //language=typescript - typeScript('return 1;') + typeScript('function f() {return 1;}') ); }); test('empty', () => { rewriteRun( //language=typescript - typeScript('return') + typeScript('function f() {return}') ); }); }); diff --git a/openrewrite/test/javascript/parser/while.test.ts b/openrewrite/test/javascript/parser/while.test.ts index 68a66493..733226c6 100644 --- a/openrewrite/test/javascript/parser/while.test.ts +++ b/openrewrite/test/javascript/parser/while.test.ts @@ -56,7 +56,7 @@ describe('while mapping', () => { test('while-if with semicolon', () => { rewriteRun( //language=typescript - typeScript('while (i--) if (nodeList[i] == elem) return true;') + typeScript('function foo() { while (i--) if (nodeList[i] == elem) return true;}') ); }); diff --git a/openrewrite/test/javascript/parser/with.test.ts b/openrewrite/test/javascript/parser/with.test.ts index 03a40484..8a071546 100644 --- a/openrewrite/test/javascript/parser/with.test.ts +++ b/openrewrite/test/javascript/parser/with.test.ts @@ -57,6 +57,7 @@ describe('with mapping', () => { rewriteRun( //language=typescript typeScript(` + export {}; with ( await obj?.foo) {} `) ); diff --git a/openrewrite/test/javascript/parser/yield.test.ts b/openrewrite/test/javascript/parser/yield.test.ts index 90684aaa..8976eaff 100644 --- a/openrewrite/test/javascript/parser/yield.test.ts +++ b/openrewrite/test/javascript/parser/yield.test.ts @@ -9,7 +9,7 @@ describe('yield mapping', () => { test('simple', () => { rewriteRun( //language=typescript - typeScript('yield 42') + typeScript('function* generatorFunction() { yield 42;}') ); }); test('empty', () => { From bc68b5aad915b23f0c9148fa57e905f4d2f12e1f Mon Sep 17 00:00:00 2001 From: Andrii Rodionov Date: Mon, 27 Jan 2025 15:04:05 +0100 Subject: [PATCH 2/2] added exclusion code --- openrewrite/src/javascript/parserUtils.ts | 2 +- openrewrite/test/javascript/parser/enum.test.ts | 2 +- openrewrite/test/javascript/parser/qualifiedName.test.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/openrewrite/src/javascript/parserUtils.ts b/openrewrite/src/javascript/parserUtils.ts index ebcebc0f..a0a0628e 100644 --- a/openrewrite/src/javascript/parserUtils.ts +++ b/openrewrite/src/javascript/parserUtils.ts @@ -223,7 +223,7 @@ const additionalCriticalCodes = new Set([ ]); const excludedCodes = new Set([1039, 1064, 1101, 1107, 1111, 1155, 1166, 1170, 1183, 1203, 1207, 1215, 1238, 1239, 1240, 1241, 1244, 1250, - 1251, 1252, 1253, 1254, 1308, 1314, 1315, 1324, 1329, 1335, 1338, 1340, 1343, 1344, 1345, 1360, 1378, 1432]); + 1251, 1252, 1253, 1254, 1308, 1314, 1315, 1324, 1329, 1335, 1338, 1340, 1343, 1344, 1345, 1355, 1360, 1378, 1432]); function isCriticalDiagnostic(code: number): boolean { return (code > 1000 && code < 2000 && !excludedCodes.has(code)) || additionalCriticalCodes.has(code); diff --git a/openrewrite/test/javascript/parser/enum.test.ts b/openrewrite/test/javascript/parser/enum.test.ts index eff75ee7..e054c143 100644 --- a/openrewrite/test/javascript/parser/enum.test.ts +++ b/openrewrite/test/javascript/parser/enum.test.ts @@ -1,6 +1,6 @@ import {connect, disconnect, rewriteRun, typeScript} from '../testHarness'; -describe('empty mapping', () => { +describe('enum mapping', () => { beforeAll(() => connect()); afterAll(() => disconnect()); diff --git a/openrewrite/test/javascript/parser/qualifiedName.test.ts b/openrewrite/test/javascript/parser/qualifiedName.test.ts index e38870b0..871cf2be 100644 --- a/openrewrite/test/javascript/parser/qualifiedName.test.ts +++ b/openrewrite/test/javascript/parser/qualifiedName.test.ts @@ -1,6 +1,6 @@ import {connect, disconnect, rewriteRun, typeScript} from '../testHarness'; -describe('empty mapping', () => { +describe('qualified name mapping', () => { beforeAll(() => connect()); afterAll(() => disconnect());