Skip to content

Commit

Permalink
Throw Flow syntax errors instead of continuing to process the AST (fa…
Browse files Browse the repository at this point in the history
…cebook#39035)

Summary:

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
  • Loading branch information
motiz88 authored and facebook-github-bot committed Nov 8, 2023
1 parent 5a7ae61 commit f6f9a7c
Show file tree
Hide file tree
Showing 10 changed files with 124 additions and 24 deletions.
2 changes: 1 addition & 1 deletion packages/eslint-plugin-specs/react-native-modules.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
1 change: 1 addition & 0 deletions packages/react-native-codegen/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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."`;

Expand All @@ -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<Spec1>('SampleTurboModule1');
> 19 | export default TurboModuleRegistry.getEnforcing<Spec2>('SampleTurboModule2');
| ^^^^^^^
20 |
21 |
and 1 other error in the same file."
`;
exports[`RN Codegen Flow Parser can generate fixture ANDROID_ONLY_NATIVE_MODULE 1`] = `
"{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,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<Spec>('Foo');
`);
Expand All @@ -98,7 +98,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<Spec>('Foo');
`);
Expand Down Expand Up @@ -145,7 +145,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<Spec>('Foo');
`);
Expand Down Expand Up @@ -356,7 +356,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<Spec>('Foo');
`);
Expand Down Expand Up @@ -693,7 +693,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<Spec>('Foo');
`);
Expand Down Expand Up @@ -727,7 +727,7 @@ describe('Flow Module Parser', () => {
${TYPE_ALIAS_DECLARATIONS}
export interface Spec extends TurboModule {
+useArg(): ${annotateRet(flowType)};
useArg(): ${annotateRet(flowType)};
}
export default TurboModuleRegistry.get<Spec>('Foo');
`);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/**
* 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]
const {codeFrameColumns} = require('@babel/code-frame');
// $FlowFixMe[untyped-import] there's no flowtype flow-parser
const flowParser = require('flow-parser');

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,
};
9 changes: 3 additions & 6 deletions packages/react-native-codegen/src/parsers/flow/parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,7 @@ const {
getTypeAnnotation,
} = require('./components/componentsUtils');
const {flowTranslateTypeAnnotation} = require('./modules');
// $FlowFixMe[untyped-import] there's no flowtype flow-parser
const flowParser = require('flow-parser');
const {parseFlowAndThrowErrors} = require('./parseFlowAndThrowErrors');
const fs = require('fs');
const invariant = require('invariant');

Expand Down Expand Up @@ -143,10 +142,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(
Expand Down
4 changes: 3 additions & 1 deletion packages/react-native-codegen/src/parsers/parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
9 changes: 3 additions & 6 deletions packages/react-native-codegen/src/parsers/parserMock.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,9 @@ import invariant from 'invariant';
const {
UnsupportedObjectPropertyTypeAnnotationParserError,
} = require('./errors');
const {parseFlowAndThrowErrors} = require('./flow/parseFlowAndThrowErrors');
const {buildPropSchema} = require('./parsers-commons');
const {flattenProperties} = require('./typescript/components/componentsUtils');
// $FlowFixMe[untyped-import] there's no flowtype flow-parser
const flowParser = require('flow-parser');

type ExtendsForProp = null | {
type: 'ReactNativeBuiltInType',
Expand Down Expand Up @@ -122,10 +121,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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,7 @@ function buildSchema(
return {modules: {}};
}

const ast = parser.getAst(contents);
const ast = parser.getAst(contents, filename);
const configType = getConfigType(ast, Visitor);

return buildSchemaFromConfigType(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,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'],
Expand Down

0 comments on commit f6f9a7c

Please sign in to comment.