From 7713614f6a2075c603c9e43cd5c62b4252f17c48 Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Thu, 24 Aug 2023 00:39:37 -0700 Subject: [PATCH] Throw Flow syntax errors instead of continuing to process the AST (#39035) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/39035 Changelog: [General][Fixed] Flow syntax errors in Codegen specs are no longer ignored. Instead of throwing errors like most parsers, the `flow-parser` package returns errors as part of the AST (along with a best-effort parse). It turns out that `react-native/codegen` ignores such errors and only detects a subset of them after the fact. Here we change the behaviour to immediately throwing a descriptive error message (containing the file name and a code frame). **This change is theoretically breaking** for any published packages that already contain broken Flow code (that somehow doesn't happen to affect the Codegen output today). Hopefully, anyone using Flow-flavoured RN Codegen is also typechecking with Flow and/or building with Metro (which would both flag the same errors), so the impact should be fairly contained. Reviewed By: huntie Differential Revision: D48385786 fbshipit-source-id: 240ac5b8dce6cfd496bdc42d2e959e540384451d --- .../react-native-modules.js | 2 +- packages/react-native-codegen/package.json | 1 + .../module-parser-snapshot-test.js.snap | 25 +++++- .../__tests__/module-parser-e2e-test.js | 12 +-- .../parsers/flow/parseFlowAndThrowErrors.js | 83 +++++++++++++++++++ .../src/parsers/flow/parser.js | 9 +- .../src/parsers/parser.js | 4 +- .../src/parsers/parserMock.js | 10 +-- .../src/parsers/parsers-commons.js | 2 +- .../src/parsers/typescript/parser.js | 2 +- 10 files changed, 126 insertions(+), 24 deletions(-) create mode 100644 packages/react-native-codegen/src/parsers/flow/parseFlowAndThrowErrors.js diff --git a/packages/eslint-plugin-specs/react-native-modules.js b/packages/eslint-plugin-specs/react-native-modules.js index 9f142919d9e075..a57b450ae6030a 100644 --- a/packages/eslint-plugin-specs/react-native-modules.js +++ b/packages/eslint-plugin-specs/react-native-modules.js @@ -154,7 +154,7 @@ function rule(context) { const [parsingErrors, tryParse] = createParserErrorCapturer(); const sourceCode = context.getSourceCode().getText(); - const ast = parser.getAst(sourceCode); + const ast = parser.getAst(sourceCode, filename); tryParse(() => { buildModuleSchema( diff --git a/packages/react-native-codegen/package.json b/packages/react-native-codegen/package.json index 45f23b160b78d6..f8f2182e76ba7e 100644 --- a/packages/react-native-codegen/package.json +++ b/packages/react-native-codegen/package.json @@ -30,6 +30,7 @@ ], "dependencies": { "@babel/parser": "^7.20.0", + "@babel/code-frame": "^7.0.0", "flow-parser": "^0.206.0", "jscodeshift": "^0.14.0", "nullthrows": "^1.1.1" diff --git a/packages/react-native-codegen/src/parsers/flow/modules/__tests__/__snapshots__/module-parser-snapshot-test.js.snap b/packages/react-native-codegen/src/parsers/flow/modules/__tests__/__snapshots__/module-parser-snapshot-test.js.snap index 00d66fd81de929..6ba4c7709483b7 100644 --- a/packages/react-native-codegen/src/parsers/flow/modules/__tests__/__snapshots__/module-parser-snapshot-test.js.snap +++ b/packages/react-native-codegen/src/parsers/flow/modules/__tests__/__snapshots__/module-parser-snapshot-test.js.snap @@ -2,7 +2,18 @@ exports[`RN Codegen Flow Parser Fails with error message EMPTY_ENUM_NATIVE_MODULE 1`] = `"Module NativeSampleTurboModule: Failed parsing the enum SomeEnum in NativeSampleTurboModule with the error: Enums should have at least one member and member values can not be mixed- they all must be either blank, number, or string values."`; -exports[`RN Codegen Flow Parser Fails with error message MIXED_VALUES_ENUM_NATIVE_MODULE 1`] = `"Module NativeSampleTurboModule: Failed parsing the enum SomeEnum in NativeSampleTurboModule with the error: Enums should have at least one member and member values can not be mixed- they all must be either blank, number, or string values."`; +exports[`RN Codegen Flow Parser Fails with error message MIXED_VALUES_ENUM_NATIVE_MODULE 1`] = ` +"Syntax error in path/NativeSampleTurboModule.js: Enum \`SomeEnum\` has inconsistent member initializers. Either use no initializers, or consistently use literals (either booleans, numbers, or strings) for all member initializers. + 15 | import * as TurboModuleRegistry from '../TurboModuleRegistry'; + 16 | +> 17 | export enum SomeEnum { + | ^^^^^^^^ + 18 | NUM = 1, + 19 | STR = 'str', + 20 | } + +and 1 other error in the same file." +`; exports[`RN Codegen Flow Parser Fails with error message NATIVE_MODULES_WITH_ARRAY_WITH_NO_TYPE_FOR_CONTENT 1`] = `"Module NativeSampleTurboModule: Generic 'Array' must have type parameters."`; @@ -18,7 +29,17 @@ exports[`RN Codegen Flow Parser Fails with error message NATIVE_MODULES_WITH_UNN exports[`RN Codegen Flow Parser Fails with error message TWO_NATIVE_EXTENDING_TURBO_MODULE 1`] = `"Module NativeSampleTurboModule: Every NativeModule spec file must declare exactly one NativeModule Flow interface. This file declares 2: 'Spec', and 'Spec2'. Please remove the extraneous Flow interface declarations."`; -exports[`RN Codegen Flow Parser Fails with error message TWO_NATIVE_MODULES_EXPORTED_WITH_DEFAULT 1`] = `"Module NativeSampleTurboModule: No Flow interfaces extending TurboModule were detected in this NativeModule spec."`; +exports[`RN Codegen Flow Parser Fails with error message TWO_NATIVE_MODULES_EXPORTED_WITH_DEFAULT 1`] = ` +"Syntax error in path/NativeSampleTurboModule.js: Duplicate export for \`default\` + 17 | + 18 | export default TurboModuleRegistry.getEnforcing('SampleTurboModule1'); +> 19 | export default TurboModuleRegistry.getEnforcing('SampleTurboModule2'); + | ^^^^^^^ + 20 | + 21 | + +and 1 other error in the same file." +`; exports[`RN Codegen Flow Parser can generate fixture ANDROID_ONLY_NATIVE_MODULE 1`] = ` "{ diff --git a/packages/react-native-codegen/src/parsers/flow/modules/__tests__/module-parser-e2e-test.js b/packages/react-native-codegen/src/parsers/flow/modules/__tests__/module-parser-e2e-test.js index 6cda41648a0013..4e677fd014d7c4 100644 --- a/packages/react-native-codegen/src/parsers/flow/modules/__tests__/module-parser-e2e-test.js +++ b/packages/react-native-codegen/src/parsers/flow/modules/__tests__/module-parser-e2e-test.js @@ -85,7 +85,7 @@ describe('Flow Module Parser', () => { import type {TurboModule} from 'RCTExport'; import * as TurboModuleRegistry from 'TurboModuleRegistry'; export interface Spec extends TurboModule { - +useArg(arg: any): void; + useArg(arg: any): void; } export default TurboModuleRegistry.get('Foo'); `); @@ -99,7 +99,7 @@ describe('Flow Module Parser', () => { import type {TurboModule} from 'RCTExport'; import * as TurboModuleRegistry from 'TurboModuleRegistry'; export interface Spec extends TurboModule { - +useArg(boolean): void; + useArg(boolean): void; } export default TurboModuleRegistry.get('Foo'); `); @@ -146,7 +146,7 @@ describe('Flow Module Parser', () => { ${TYPE_ALIAS_DECLARATIONS} export interface Spec extends TurboModule { - +useArg(${annotateArg(paramName, paramType)}): void; + useArg(${annotateArg(paramName, paramType)}): void; } export default TurboModuleRegistry.get('Foo'); `); @@ -357,7 +357,7 @@ describe('Flow Module Parser', () => { type AnimalPointer = Animal; export interface Spec extends TurboModule { - +useArg(${annotateArg('arg', 'AnimalPointer')}): void; + useArg(${annotateArg('arg', 'AnimalPointer')}): void; } export default TurboModuleRegistry.get('Foo'); `); @@ -694,7 +694,7 @@ describe('Flow Module Parser', () => { import type {TurboModule} from 'RCTExport'; import * as TurboModuleRegistry from 'TurboModuleRegistry'; export interface Spec extends TurboModule { - +useArg(): void; + useArg(): void; } export default TurboModuleRegistry.get('Foo'); `); @@ -728,7 +728,7 @@ describe('Flow Module Parser', () => { ${TYPE_ALIAS_DECLARATIONS} export interface Spec extends TurboModule { - +useArg(): ${annotateRet(flowType)}; + useArg(): ${annotateRet(flowType)}; } export default TurboModuleRegistry.get('Foo'); `); diff --git a/packages/react-native-codegen/src/parsers/flow/parseFlowAndThrowErrors.js b/packages/react-native-codegen/src/parsers/flow/parseFlowAndThrowErrors.js new file mode 100644 index 00000000000000..c3153e5a0f5980 --- /dev/null +++ b/packages/react-native-codegen/src/parsers/flow/parseFlowAndThrowErrors.js @@ -0,0 +1,83 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow strict + * @format + */ + +'use strict'; + +// $FlowFixMe[untyped-import] there's no flowtype flow-parser +const flowParser = require('flow-parser'); + +// $FlowFixMe[untyped-import] +const {codeFrameColumns} = require('@babel/code-frame'); + +class FlowParserSyntaxError extends Error { + constructor( + code: string, + filename: ?string, + errors: $ReadOnlyArray<{ + loc: $ReadOnly<{ + start: $ReadOnly<{line: number, column: number}>, + end: $ReadOnly<{line: number, column: number}>, + }>, + message: string, + }>, + ) { + const firstError = errors[0]; + const codeFrame = codeFrameColumns( + code, + { + start: { + line: firstError.loc.start.line, + // flow-parser returns 0-indexed columns but Babel expects 1-indexed + column: firstError.loc.start.column + 1, + }, + end: { + line: firstError.loc.end.line, + // flow-parser returns 0-indexed columns but Babel expects 1-indexed + column: firstError.loc.end.column + 1, + }, + }, + { + forceColor: false, + }, + ); + const additionalErrorsMessage = errors.length + ? '\n\nand ' + + errors.length + + ' other error' + + (errors.length > 1 ? 's' : '') + + ' in the same file.' + : ''; + + super( + (filename != null ? `Syntax error in ${filename}: ` : 'Syntax error: ') + + firstError.message + + '\n' + + codeFrame + + additionalErrorsMessage, + ); + } +} + +function parseFlowAndThrowErrors( + code: string, + options: $ReadOnly<{filename?: ?string}> = {}, +): $FlowFixMe { + const ast = flowParser.parse(code, { + enums: true, + }); + if (ast.errors && ast.errors.length) { + throw new FlowParserSyntaxError(code, options.filename, ast.errors); + } + return ast; +} + +module.exports = { + parseFlowAndThrowErrors, +}; diff --git a/packages/react-native-codegen/src/parsers/flow/parser.js b/packages/react-native-codegen/src/parsers/flow/parser.js index b0d1f2df2b50d8..0408817c914911 100644 --- a/packages/react-native-codegen/src/parsers/flow/parser.js +++ b/packages/react-native-codegen/src/parsers/flow/parser.js @@ -52,8 +52,7 @@ const { const {flowTranslateTypeAnnotation} = require('./modules'); -// $FlowFixMe[untyped-import] there's no flowtype flow-parser -const flowParser = require('flow-parser'); +const {parseFlowAndThrowErrors} = require('./parseFlowAndThrowErrors'); const { buildSchema, @@ -149,10 +148,8 @@ class FlowParser implements Parser { return this.parseString(contents, 'path/NativeSampleTurboModule.js'); } - getAst(contents: string): $FlowFixMe { - return flowParser.parse(contents, { - enums: true, - }); + getAst(contents: string, filename?: ?string): $FlowFixMe { + return parseFlowAndThrowErrors(contents, {filename}); } getFunctionTypeAnnotationParameters( diff --git a/packages/react-native-codegen/src/parsers/parser.js b/packages/react-native-codegen/src/parsers/parser.js index 7835107642da3e..ee1d6c02e1254d 100644 --- a/packages/react-native-codegen/src/parsers/parser.js +++ b/packages/react-native-codegen/src/parsers/parser.js @@ -169,9 +169,11 @@ export interface Parser { /** * Given the content of a file, it returns an AST. * @parameter contents: the content of the file. + * @parameter filename: the name of the file, if available. + * @throws if there is a syntax error. * @returns: the AST of the file. */ - getAst(contents: string): $FlowFixMe; + getAst(contents: string, filename?: ?string): $FlowFixMe; /** * Given a FunctionTypeAnnotation, it returns an array of its parameters. diff --git a/packages/react-native-codegen/src/parsers/parserMock.js b/packages/react-native-codegen/src/parsers/parserMock.js index 1a775b3079cb23..5ddf217a36d2dc 100644 --- a/packages/react-native-codegen/src/parsers/parserMock.js +++ b/packages/react-native-codegen/src/parsers/parserMock.js @@ -46,8 +46,8 @@ const {flattenProperties} = require('./typescript/components/componentsUtils'); const {buildPropSchema} = require('./parsers-commons'); -// $FlowFixMe[untyped-import] there's no flowtype flow-parser -const flowParser = require('flow-parser'); +const {parseFlowAndThrowErrors} = require('./flow/parseFlowAndThrowErrors'); + const { UnsupportedObjectPropertyTypeAnnotationParserError, } = require('./errors'); @@ -122,10 +122,8 @@ export class MockedParser implements Parser { return schemaMock; } - getAst(contents: string): $FlowFixMe { - return flowParser.parse(contents, { - enums: true, - }); + getAst(contents: string, filename?: ?string): $FlowFixMe { + return parseFlowAndThrowErrors(contents, {filename}); } getFunctionTypeAnnotationParameters( diff --git a/packages/react-native-codegen/src/parsers/parsers-commons.js b/packages/react-native-codegen/src/parsers/parsers-commons.js index 46fc5ef397937f..6d3f630b5aabd3 100644 --- a/packages/react-native-codegen/src/parsers/parsers-commons.js +++ b/packages/react-native-codegen/src/parsers/parsers-commons.js @@ -482,7 +482,7 @@ function buildSchema( return {modules: {}}; } - const ast = parser.getAst(contents); + const ast = parser.getAst(contents, filename); const configType = getConfigType(ast, Visitor); return buildSchemaFromConfigType( diff --git a/packages/react-native-codegen/src/parsers/typescript/parser.js b/packages/react-native-codegen/src/parsers/typescript/parser.js index 9e8dc836b4ed3a..e11893991e9d02 100644 --- a/packages/react-native-codegen/src/parsers/typescript/parser.js +++ b/packages/react-native-codegen/src/parsers/typescript/parser.js @@ -146,7 +146,7 @@ class TypeScriptParser implements Parser { return this.parseString(contents, 'path/NativeSampleTurboModule.ts'); } - getAst(contents: string): $FlowFixMe { + getAst(contents: string, filename?: ?string): $FlowFixMe { return babelParser.parse(contents, { sourceType: 'module', plugins: ['typescript'],