Skip to content

Commit

Permalink
metro-react-native-interop-tools added function type comparison
Browse files Browse the repository at this point in the history
Summary:
I added function type comparison, this is the first more complex type. In the returned type, the rules change because we are not calling native code from js anymore, but we are getting something back from native code, so we switched the left and right to keep the checking same as for previous types. We are updating the isInFunctionReturn flag to know that the left was switched with the right type.
So this change is compatible in function parameters:
```(test: ?string) => string;``` ----> ```(test: string) => string```
Because js will call the native only with a string value, and the null case will not be handled by the native anymore. But if we are going from narrower to wider scope(non-nullable to nullable), native code doesn't know how to handle the null case so the app will crash/behave in an unexpected way.
If the same change happens in the return type will cause an incompatibility:
```(test: string) => ?string``` -----> ```(test:string) => string```
In this case native can return null | void | string, and the flow will expect to always get a string.

Reviewed By: GijsWeterings

Differential Revision: D39353779

fbshipit-source-id: 72733b208e6d6d5ed535c572e788a852c6cbd1d1
  • Loading branch information
mirtleonard authored and facebook-github-bot committed Sep 16, 2022
1 parent 059e9ed commit 26f7ec7
Show file tree
Hide file tree
Showing 7 changed files with 241 additions and 0 deletions.
Expand Up @@ -432,7 +432,18 @@ Object {
},
"params": Array [
Object {
"loc": Object {
"end": Object {
"column": 54,
"line": 14,
},
"start": Object {
"column": 25,
"line": 14,
},
},
"name": "screenShouldBeKeptOn",
"optional": false,
"typeAnnotation": Object {
"loc": Object {
"end": Object {
Expand Down
Expand Up @@ -247,3 +247,97 @@ exports[`comparing basic types 24`] = `
output:
no errors"
`;

exports[`comparing basic types 25`] = `
"
left-type:
() => boolean
right-type:
() => ?boolean
output:
no errors"
`;

exports[`comparing basic types 26`] = `
"
left-type:
() => ?boolean
right-type:
() => boolean
output:
Error: cannot change nullable boolean to boolean because is incompatible with what the native code returns.
"
`;

exports[`comparing basic types 27`] = `
"
left-type:
() => true
right-type:
() => boolean
output:
no errors"
`;

exports[`comparing basic types 28`] = `
"
left-type:
(test: ?boolean) => true
right-type:
(test: boolean) => true
output:
no errors"
`;

exports[`comparing basic types 29`] = `
"
left-type:
(test?: string) => void
right-type:
() => void
output:
no errors"
`;

exports[`comparing basic types 30`] = `
"
left-type:
(test: string) => void
right-type:
() => void
output:
Error: cannot remove required parameter string because native code will break when js calls it.
"
`;

exports[`comparing basic types 31`] = `
"
left-type:
() => void
right-type:
(test?: string) => void
output:
no errors"
`;

exports[`comparing basic types 32`] = `
"
left-type:
() => void
right-type:
(test?: string, test2: number) => void
output:
Error: cannot add new required parameter number because native will not provide it.
"
`;

exports[`comparing basic types 33`] = `
"
left-type:
(test?: boolean) => true
right-type:
(test?: string) => true
output:
Error: cannot change boolean to string because native code will break when js calls it.
"
`;
Expand Up @@ -118,14 +118,18 @@ test('getFunctionTypeAnnotation, function has a function as parameter', () => {
loc: null,
params: [
{
loc: null,
name: 'screenShoudBeKeptOn',
optional: undefined,
typeAnnotation: {
type: 'AnyTypeAnnotation',
loc: null,
},
},
{
loc: null,
name: 'callback',
optional: undefined,
typeAnnotation: {
type: 'FunctionTypeAnnotation',
loc: null,
Expand All @@ -150,7 +154,9 @@ test('getFunctionTypeParameter, testig basic type parameter', () => {
t.anyTypeAnnotation(),
);
expect(getFunctionTypeParameter(param)).toEqual({
loc: null,
name: 'testParam',
optional: undefined,
typeAnnotation: {
type: 'AnyTypeAnnotation',
loc: null,
Expand Down
Expand Up @@ -57,6 +57,15 @@ test.each([
['null', 'null'],
['?string', 'null'],
['?boolean', 'void'],
['() => boolean', '() => ?boolean'],
['() => ?boolean', '() => boolean'],
['() => true', '() => boolean'],
['(test: ?boolean) => true', '(test: boolean) => true'],
['(test?: string) => void', '() => void'],
['(test: string) => void', '() => void'],
['() => void', '(test?: string) => void'],
['() => void', '(test?: string, test2: number) => void'],
['(test?: boolean) => true', '(test?: string) => true'],
])('comparing basic types', (left, right) => {
const result = compareTypeAnnotation(
getTypeFromCode(left),
Expand Down
2 changes: 2 additions & 0 deletions packages/metro-react-native-interop-tools/src/ast-helpers.js
Expand Up @@ -169,7 +169,9 @@ export function getFunctionTypeParameter(
param: BabelNodeFunctionTypeParam,
): FunctionTypeParam {
return {
loc: getNodeLoc(param.loc),
name: param.name?.name,
optional: param.optional,
typeAnnotation: getTypeAnnotation(param.typeAnnotation),
};
}
Expand Down
Expand Up @@ -37,7 +37,9 @@ export type VoidTypeAnnotation = $ReadOnly<{
}>;

export type FunctionTypeParam = $ReadOnly<{|
loc: ?SourceLocation,
name: ?string,
optional: ?boolean,
typeAnnotation: AnyTypeAnnotation,
|}>;

Expand Down
117 changes: 117 additions & 0 deletions packages/metro-react-native-interop-tools/src/type-comparison.js
Expand Up @@ -13,6 +13,9 @@ import type {
AnyTypeAnnotation,
NullableTypeAnnotation,
LiteralTypeAnnotation,
FunctionTypeAnnotation,
FunctionTypeParam,
ObjectTypeProperty,
} from './type-annotation.js';
import type {SourceLocation} from '@babel/types';

Expand Down Expand Up @@ -49,6 +52,8 @@ function getSimplifiedType(annotation: AnyTypeAnnotation): string {
return 'null';
case 'NullableTypeAnnotation':
return `nullable ${getSimplifiedType(removeNullable(annotation))}`;
case 'FunctionTypeAnnotation':
return 'function';
}
throw new Error(annotation.type + ' is not supported');
}
Expand Down Expand Up @@ -122,11 +127,123 @@ export function compareTypeAnnotation(
right,
isInFunctionReturn,
);
case 'FunctionTypeAnnotation':
if (right.type === 'FunctionTypeAnnotation') {
return compareFunctionType(left, right, isInFunctionReturn);
}
return [basicError(left, right, isInFunctionReturn)];
default:
throw new Error(left.type + 'is not supported');
}
}

function compareFunctionType(
left: FunctionTypeAnnotation,
right: FunctionTypeAnnotation,
isInFunctionReturn: boolean,
): Array<ComparisonResult> {
/*
* For the returned type comparison the comparison should be made
* other way around, because it will return from native something
* instead of native to be called from js
*/
const finalResult = [];
finalResult.push(
...compareTypeAnnotation(
right.returnTypeAnnotation,
left.returnTypeAnnotation,
true,
),
);
let minimumLength = right.params.length;
if (left.params.length < right.params.length) {
minimumLength = left.params.length;
for (let index = left.params.length; index < right.params.length; ++index) {
if (right.params[index].optional !== true) {
finalResult.push(
addedRequiredParamError(left, right.params[index].typeAnnotation),
);
}
}
}
for (let index = 0; index < minimumLength; ++index) {
finalResult.push(
...compareTypeAnnotation(
left.params[index].typeAnnotation,
right.params[index].typeAnnotation,
isInFunctionReturn,
),
);
if (
left.params[index].optional === false &&
right.params[index].optional === true
) {
finalResult.push(
optionalError(
left.params[index],
right.params[index],
isInFunctionReturn,
),
);
}
}
for (let index = right.params.length; index < left.params.length; ++index) {
if (left.params[index].optional !== true) {
finalResult.push(
removedRequiredParamError(left.params[index].typeAnnotation, right),
);
}
}
return finalResult;
}

function addedRequiredParamError(
oldType: FunctionTypeAnnotation,
newType: AnyTypeAnnotation,
): ComparisonResult {
const addedType = getSimplifiedType(newType);
return {
message: `Error: cannot add new required parameter ${addedType} because native will not provide it.`,
oldTypeLoc: oldType.loc,
newTypeLoc: newType.loc,
};
}

function removedRequiredParamError(
oldType: AnyTypeAnnotation,
newType: FunctionTypeAnnotation,
) {
const removedType = getSimplifiedType(oldType);
return {
message: `Error: cannot remove required parameter ${removedType} because native code will break when js calls it.`,
oldTypeLoc: oldType.loc,
newTypeLoc: newType.loc,
};
}

function optionalError(
left: FunctionTypeParam | ObjectTypeProperty,
right: FunctionTypeParam | ObjectTypeProperty,
isInFunctionReturn: boolean,
): ComparisonResult {
const newAnnotationType = isInFunctionReturn ? left : right;
const oldAnnotationType = isInFunctionReturn ? right : left;
const newOptionality =
newAnnotationType.optional === true ? 'optional' : 'required';
const oldOptionality =
oldAnnotationType.optional === true ? 'optional' : 'required';
const newType = getSimplifiedType(newAnnotationType.typeAnnotation);
const oldType = getSimplifiedType(oldAnnotationType.typeAnnotation);
const reason = isInFunctionReturn
? 'is incompatible with what the native code returns'
: 'native code will break when js calls it';
return {
message: `Error: cannot change ${oldOptionality} ${oldType} to ${newOptionality} ${newType} because ${reason}.`,
newTypeLoc: newAnnotationType.loc,
oldTypeLoc: oldAnnotationType.loc,
};
}

function basicError(
left: AnyTypeAnnotation,
right: AnyTypeAnnotation,
Expand Down

0 comments on commit 26f7ec7

Please sign in to comment.