Fixing schema types for component command params of Arrays

[elicwhite]

Summary:
Command param array types were generating invalid schemas due to untyped parser code. The invalid schemas occurred for any alias type, including custom objects and basics like Int32. This was also inconsistent between Flow and TypeScript.

We already had one component utilizing this issue, so this just codifies that support into the schema so it reflects reality. This is only a partial solution. The more full solution would be to fully encode the custom types in the schemas like we do for Native Modules.

More Information:

Tl;dr, DebuggingOverlay is abusing a FlowFixMe in codegen commands.

The problem:

The CodegenSchema should be the source of truth for anything that can be in the schema. If something is in the schema that isn't allowed by the types, that's a bug. We have a bug. I'm adding compat-check support for components and it's blowing up on prod schemas because DebuggingOverlay causes an invalid schema.

The details:

Support for Arrays as arguments in commands was added to the Codegen in D51866557. Code Pointer

The intention appears to be to support arrays of primitives. There is a TODO for supporting complex types.

interface NativeCommands {
+addOverlays: (
viewRef: React.ElementRef<NativeType>,
overlayColorsReadOnly: $ReadOnlyArray<string>,
)
}

This support was added to TypeScript in D52046165 where it types the allowed Array arguments to be only

{
readonly type: 'ArrayTypeAnnotation';
readonly elementType:
| Int32TypeAnnotation
| DoubleTypeAnnotation
| FloatTypeAnnotation
| BooleanTypeAnnotation
| StringTypeAnnotation
};

However, because the Parsers are treating the input type as any, it isn't safe to pass through an input value into the schema as Flow won't catch mismatches.

The Flow parser just passes it through:

{
type: 'ArrayTypeAnnotation',
elementType: {
// TODO: T172453752 support complex type annotation for array element
type: paramValue.typeParameters.params[0].type,
}

Whereas the TypeScript parser has the more correct behavior of validating the inputs and returning specific outputs. Unfortunately, the return type is also typed here as $FlowFixMe, losing most of the benefits.

function getPrimitiveTypeAnnotation(type: string): $FlowFixMe {
switch (type) {
case 'Int32':
return {
type: 'Int32TypeAnnotation',
};
case 'Double':
return {
type: 'DoubleTypeAnnotation',
};
case 'Float':
return {
type: 'FloatTypeAnnotation',
};
case 'TSBooleanKeyword':
return {
type: 'BooleanTypeAnnotation',
};
case 'Stringish':
case 'TSStringKeyword':
return {
type: 'StringTypeAnnotation',
};
default:
throw new Error(`Unknown primitive type "${type}"`);
}
}

DebuggingOverlay is abusing this gap in the Flow parser by sticking an Array of Objects in.

export type ElementRectangle = {
x: number,
y: number,
width: number,
height: number,
};
...
+highlightElements: (
viewRef: React.ElementRef<DebuggingOverlayNativeComponentType>,
elements: $ReadOnlyArray<ElementRectangle>,
) => void;
...

This isn't allowed in the schema, but it seems to fall through the holes of the flow parser and generators.

The resulting schema from Flow is this. Note the GenericTypeAnnotation which isn't allowed to be in the schema.

{
"name": "highlightElements",
"optional": false,
"typeAnnotation": {
"type": "FunctionTypeAnnotation",
"params": [
{
"name": "elements",
"optional": false,
"typeAnnotation": {
"type": "ArrayTypeAnnotation",
"elementType": {
"type": "GenericTypeAnnotation"
}
}
}
],
"returnTypeAnnotation": {
"type": "VoidTypeAnnotation"
}
},

The TypeScript parser fails with Error: Unsupported type annotation: GenericTypeAnnotation.

The generators don't seem to check beyond the ArrayTypeAnnotation so they fall through to generating generic arrays.

// ios
elements:(const NSArray *)elements
// android
ReadableArray locations

So how do I fix this?

I think there are a couple of different options here. The key problem is that the Schema types need to represent reality of what can be in the schema.

1. We revert DebuggingOverlay to not use features that aren't supported (I assume nobody would be happy with this, but the change shouldn't have been made in the first place)
2. (This is the approach taken in this diff) We add MixedTypeAnnotation to the allowed types in Command arrays and have it generate that and add official support for that to the TypeScript parser as well. That is probably the quickest and easiest approach. It leaves the same type unsafety we have today on the native side.
3. NativeModules seem to have a lot more complex type safety here. They persist the alias type in the schema so that the CompatCheck can check them on changes. And then in C++ they generate structs and RCTConvert functions although for Java and ObjC it looks like they just use the same untyped native code. The matching approach here would be to add aliasMap and the whole data to the schema for commands, use that for the compat check, and still generate the same unsafe native code.

export type ObjectAlias = {|
x: number,
y: number,
|};
export interface Spec extends TurboModule {
+getAlias: (a: ObjectAlias) => string;
}

stores the ObjectAlias in the schema

{
"aliasMap": {
"ObjectAlias": {
"type": "ObjectTypeAnnotation",
"properties": [
{
"name": "x",
"optional": false,
"typeAnnotation": {
"type": "NumberTypeAnnotation"
}
},
{
"name": "y",
"optional": false,
"typeAnnotation": {
"type": "NumberTypeAnnotation"
}
},
]
}
},
"spec": {
"methods": [
{
"name": "getAlias",
"optional": false,
"typeAnnotation": {
"type": "FunctionTypeAnnotation",
"returnTypeAnnotation": {
"type": "StringTypeAnnotation"
},
"params": [
{
"name": "a",
"optional": false,
"typeAnnotation": {
"type": "TypeAliasTypeAnnotation",
"name": "ObjectAlias"
}
}
]
}
}
]
}
}

and then generates the appropriate structs on the native side and generates this

Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request has been merged in 25c673e.

[elicwhite]

Summary:
Command param array types were generating invalid schemas due to untyped parser code. The invalid schemas occurred for any alias type, including custom objects and basics like Int32. This was also inconsistent between Flow and TypeScript.

We already had one component utilizing this issue, so this just codifies that support into the schema so it reflects reality. This is only a partial solution. The more full solution would be to fully encode the custom types in the schemas like we do for Native Modules.

More Information:

Tl;dr, DebuggingOverlay is abusing a FlowFixMe in codegen commands.

The problem:

The CodegenSchema should be the source of truth for anything that can be in the schema. If something is in the schema that isn't allowed by the types, that's a bug. We have a bug. I'm adding compat-check support for components and it's blowing up on prod schemas because DebuggingOverlay causes an invalid schema.

The details:

Support for Arrays as arguments in commands was added to the Codegen in D51866557. Code Pointer

The intention appears to be to support arrays of primitives. There is a TODO for supporting complex types.

interface NativeCommands {
+addOverlays: (
viewRef: React.ElementRef<NativeType>,
overlayColorsReadOnly: $ReadOnlyArray<string>,
)
}

This support was added to TypeScript in D52046165 where it types the allowed Array arguments to be only

{
readonly type: 'ArrayTypeAnnotation';
readonly elementType:
| Int32TypeAnnotation
| DoubleTypeAnnotation
| FloatTypeAnnotation
| BooleanTypeAnnotation
| StringTypeAnnotation
};

However, because the Parsers are treating the input type as any, it isn't safe to pass through an input value into the schema as Flow won't catch mismatches.

The Flow parser just passes it through:

{
type: 'ArrayTypeAnnotation',
elementType: {
// TODO: T172453752 support complex type annotation for array element
type: paramValue.typeParameters.params[0].type,
}

Whereas the TypeScript parser has the more correct behavior of validating the inputs and returning specific outputs. Unfortunately, the return type is also typed here as $FlowFixMe, losing most of the benefits.

function getPrimitiveTypeAnnotation(type: string): $FlowFixMe {
switch (type) {
case 'Int32':
return {
type: 'Int32TypeAnnotation',
};
case 'Double':
return {
type: 'DoubleTypeAnnotation',
};
case 'Float':
return {
type: 'FloatTypeAnnotation',
};
case 'TSBooleanKeyword':
return {
type: 'BooleanTypeAnnotation',
};
case 'Stringish':
case 'TSStringKeyword':
return {
type: 'StringTypeAnnotation',
};
default:
throw new Error(`Unknown primitive type "${type}"`);
}
}

DebuggingOverlay is abusing this gap in the Flow parser by sticking an Array of Objects in.

export type ElementRectangle = {
x: number,
y: number,
width: number,
height: number,
};
...
+highlightElements: (
viewRef: React.ElementRef<DebuggingOverlayNativeComponentType>,
elements: $ReadOnlyArray<ElementRectangle>,
) => void;
...

This isn't allowed in the schema, but it seems to fall through the holes of the flow parser and generators.

The resulting schema from Flow is this. Note the GenericTypeAnnotation which isn't allowed to be in the schema.

{
"name": "highlightElements",
"optional": false,
"typeAnnotation": {
"type": "FunctionTypeAnnotation",
"params": [
{
"name": "elements",
"optional": false,
"typeAnnotation": {
"type": "ArrayTypeAnnotation",
"elementType": {
"type": "GenericTypeAnnotation"
}
}
}
],
"returnTypeAnnotation": {
"type": "VoidTypeAnnotation"
}
},

The TypeScript parser fails with Error: Unsupported type annotation: GenericTypeAnnotation.

The generators don't seem to check beyond the ArrayTypeAnnotation so they fall through to generating generic arrays.

// ios
elements:(const NSArray *)elements
// android
ReadableArray locations

So how do I fix this?

I think there are a couple of different options here. The key problem is that the Schema types need to represent reality of what can be in the schema.

1. We revert DebuggingOverlay to not use features that aren't supported (I assume nobody would be happy with this, but the change shouldn't have been made in the first place)
2. (This is the approach taken in this diff) We add MixedTypeAnnotation to the allowed types in Command arrays and have it generate that and add official support for that to the TypeScript parser as well. That is probably the quickest and easiest approach. It leaves the same type unsafety we have today on the native side.
3. NativeModules seem to have a lot more complex type safety here. They persist the alias type in the schema so that the CompatCheck can check them on changes. And then in C++ they generate structs and RCTConvert functions although for Java and ObjC it looks like they just use the same untyped native code. The matching approach here would be to add aliasMap and the whole data to the schema for commands, use that for the compat check, and still generate the same unsafe native code.

export type ObjectAlias = {|
x: number,
y: number,
|};
export interface Spec extends TurboModule {
+getAlias: (a: ObjectAlias) => string;
}

stores the ObjectAlias in the schema

{
"aliasMap": {
"ObjectAlias": {
"type": "ObjectTypeAnnotation",
"properties": [
{
"name": "x",
"optional": false,
"typeAnnotation": {
"type": "NumberTypeAnnotation"
}
},
{
"name": "y",
"optional": false,
"typeAnnotation": {
"type": "NumberTypeAnnotation"
}
},
]
}
},
"spec": {
"methods": [
{
"name": "getAlias",
"optional": false,
"typeAnnotation": {
"type": "FunctionTypeAnnotation",
"returnTypeAnnotation": {
"type": "StringTypeAnnotation"
},
"params": [
{
"name": "a",
"optional": false,
"typeAnnotation": {
"type": "TypeAliasTypeAnnotation",
"name": "ObjectAlias"
}
}
]
}
}
]
}
}

and then generates the appropriate structs on the native side and generates this

Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request has been merged in 25c673e.

[elicwhite]

Summary:
Command param array types were generating invalid schemas due to untyped parser code. The invalid schemas occurred for any alias type, including custom objects and basics like Int32. This was also inconsistent between Flow and TypeScript.

We already had one component utilizing this issue, so this just codifies that support into the schema so it reflects reality. This is only a partial solution. The more full solution would be to fully encode the custom types in the schemas like we do for Native Modules.

More Information:

Tl;dr, DebuggingOverlay is abusing a FlowFixMe in codegen commands.

The problem:

The CodegenSchema should be the source of truth for anything that can be in the schema. If something is in the schema that isn't allowed by the types, that's a bug. We have a bug. I'm adding compat-check support for components and it's blowing up on prod schemas because DebuggingOverlay causes an invalid schema.

The details:

Support for Arrays as arguments in commands was added to the Codegen in D51866557. Code Pointer

The intention appears to be to support arrays of primitives. There is a TODO for supporting complex types.

interface NativeCommands {
+addOverlays: (
viewRef: React.ElementRef<NativeType>,
overlayColorsReadOnly: $ReadOnlyArray<string>,
)
}

This support was added to TypeScript in D52046165 where it types the allowed Array arguments to be only

{
readonly type: 'ArrayTypeAnnotation';
readonly elementType:
| Int32TypeAnnotation
| DoubleTypeAnnotation
| FloatTypeAnnotation
| BooleanTypeAnnotation
| StringTypeAnnotation
};

However, because the Parsers are treating the input type as any, it isn't safe to pass through an input value into the schema as Flow won't catch mismatches.

The Flow parser just passes it through:

{
type: 'ArrayTypeAnnotation',
elementType: {
// TODO: T172453752 support complex type annotation for array element
type: paramValue.typeParameters.params[0].type,
}

Whereas the TypeScript parser has the more correct behavior of validating the inputs and returning specific outputs. Unfortunately, the return type is also typed here as $FlowFixMe, losing most of the benefits.

function getPrimitiveTypeAnnotation(type: string): $FlowFixMe {
switch (type) {
case 'Int32':
return {
type: 'Int32TypeAnnotation',
};
case 'Double':
return {
type: 'DoubleTypeAnnotation',
};
case 'Float':
return {
type: 'FloatTypeAnnotation',
};
case 'TSBooleanKeyword':
return {
type: 'BooleanTypeAnnotation',
};
case 'Stringish':
case 'TSStringKeyword':
return {
type: 'StringTypeAnnotation',
};
default:
throw new Error(`Unknown primitive type "${type}"`);
}
}

DebuggingOverlay is abusing this gap in the Flow parser by sticking an Array of Objects in.

export type ElementRectangle = {
x: number,
y: number,
width: number,
height: number,
};
...
+highlightElements: (
viewRef: React.ElementRef<DebuggingOverlayNativeComponentType>,
elements: $ReadOnlyArray<ElementRectangle>,
) => void;
...

This isn't allowed in the schema, but it seems to fall through the holes of the flow parser and generators.

The resulting schema from Flow is this. Note the GenericTypeAnnotation which isn't allowed to be in the schema.

{
"name": "highlightElements",
"optional": false,
"typeAnnotation": {
"type": "FunctionTypeAnnotation",
"params": [
{
"name": "elements",
"optional": false,
"typeAnnotation": {
"type": "ArrayTypeAnnotation",
"elementType": {
"type": "GenericTypeAnnotation"
}
}
}
],
"returnTypeAnnotation": {
"type": "VoidTypeAnnotation"
}
},

The TypeScript parser fails with Error: Unsupported type annotation: GenericTypeAnnotation.

The generators don't seem to check beyond the ArrayTypeAnnotation so they fall through to generating generic arrays.

// ios
elements:(const NSArray *)elements
// android
ReadableArray locations

So how do I fix this?

I think there are a couple of different options here. The key problem is that the Schema types need to represent reality of what can be in the schema.

1. We revert DebuggingOverlay to not use features that aren't supported (I assume nobody would be happy with this, but the change shouldn't have been made in the first place)
2. (This is the approach taken in this diff) We add MixedTypeAnnotation to the allowed types in Command arrays and have it generate that and add official support for that to the TypeScript parser as well. That is probably the quickest and easiest approach. It leaves the same type unsafety we have today on the native side.
3. NativeModules seem to have a lot more complex type safety here. They persist the alias type in the schema so that the CompatCheck can check them on changes. And then in C++ they generate structs and RCTConvert functions although for Java and ObjC it looks like they just use the same untyped native code. The matching approach here would be to add aliasMap and the whole data to the schema for commands, use that for the compat check, and still generate the same unsafe native code.

export type ObjectAlias = {|
x: number,
y: number,
|};
export interface Spec extends TurboModule {
+getAlias: (a: ObjectAlias) => string;
}

stores the ObjectAlias in the schema

{
"aliasMap": {
"ObjectAlias": {
"type": "ObjectTypeAnnotation",
"properties": [
{
"name": "x",
"optional": false,
"typeAnnotation": {
"type": "NumberTypeAnnotation"
}
},
{
"name": "y",
"optional": false,
"typeAnnotation": {
"type": "NumberTypeAnnotation"
}
},
]
}
},
"spec": {
"methods": [
{
"name": "getAlias",
"optional": false,
"typeAnnotation": {
"type": "FunctionTypeAnnotation",
"returnTypeAnnotation": {
"type": "StringTypeAnnotation"
},
"params": [
{
"name": "a",
"optional": false,
"typeAnnotation": {
"type": "TypeAliasTypeAnnotation",
"name": "ObjectAlias"
}
}
]
}
}
]
}
}

and then generates the appropriate structs on the native side and generates this

Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request has been merged in 25c673e.

[elicwhite]

Summary:
Command param array types were generating invalid schemas due to untyped parser code. The invalid schemas occurred for any alias type, including custom objects and basics like Int32. This was also inconsistent between Flow and TypeScript.

We already had one component utilizing this issue, so this just codifies that support into the schema so it reflects reality. This is only a partial solution. The more full solution would be to fully encode the custom types in the schemas like we do for Native Modules.

More Information:

Tl;dr, DebuggingOverlay is abusing a FlowFixMe in codegen commands.

The problem:

The CodegenSchema should be the source of truth for anything that can be in the schema. If something is in the schema that isn't allowed by the types, that's a bug. We have a bug. I'm adding compat-check support for components and it's blowing up on prod schemas because DebuggingOverlay causes an invalid schema.

The details:

Support for Arrays as arguments in commands was added to the Codegen in D51866557. Code Pointer

The intention appears to be to support arrays of primitives. There is a TODO for supporting complex types.

interface NativeCommands {
+addOverlays: (
viewRef: React.ElementRef<NativeType>,
overlayColorsReadOnly: $ReadOnlyArray<string>,
)
}

This support was added to TypeScript in D52046165 where it types the allowed Array arguments to be only

{
readonly type: 'ArrayTypeAnnotation';
readonly elementType:
| Int32TypeAnnotation
| DoubleTypeAnnotation
| FloatTypeAnnotation
| BooleanTypeAnnotation
| StringTypeAnnotation
};

However, because the Parsers are treating the input type as any, it isn't safe to pass through an input value into the schema as Flow won't catch mismatches.

The Flow parser just passes it through:

{
type: 'ArrayTypeAnnotation',
elementType: {
// TODO: T172453752 support complex type annotation for array element
type: paramValue.typeParameters.params[0].type,
}

Whereas the TypeScript parser has the more correct behavior of validating the inputs and returning specific outputs. Unfortunately, the return type is also typed here as $FlowFixMe, losing most of the benefits.

function getPrimitiveTypeAnnotation(type: string): $FlowFixMe {
switch (type) {
case 'Int32':
return {
type: 'Int32TypeAnnotation',
};
case 'Double':
return {
type: 'DoubleTypeAnnotation',
};
case 'Float':
return {
type: 'FloatTypeAnnotation',
};
case 'TSBooleanKeyword':
return {
type: 'BooleanTypeAnnotation',
};
case 'Stringish':
case 'TSStringKeyword':
return {
type: 'StringTypeAnnotation',
};
default:
throw new Error(`Unknown primitive type "${type}"`);
}
}

DebuggingOverlay is abusing this gap in the Flow parser by sticking an Array of Objects in.

export type ElementRectangle = {
x: number,
y: number,
width: number,
height: number,
};
...
+highlightElements: (
viewRef: React.ElementRef<DebuggingOverlayNativeComponentType>,
elements: $ReadOnlyArray<ElementRectangle>,
) => void;
...

This isn't allowed in the schema, but it seems to fall through the holes of the flow parser and generators.

The resulting schema from Flow is this. Note the GenericTypeAnnotation which isn't allowed to be in the schema.

{
"name": "highlightElements",
"optional": false,
"typeAnnotation": {
"type": "FunctionTypeAnnotation",
"params": [
{
"name": "elements",
"optional": false,
"typeAnnotation": {
"type": "ArrayTypeAnnotation",
"elementType": {
"type": "GenericTypeAnnotation"
}
}
}
],
"returnTypeAnnotation": {
"type": "VoidTypeAnnotation"
}
},

The TypeScript parser fails with Error: Unsupported type annotation: GenericTypeAnnotation.

The generators don't seem to check beyond the ArrayTypeAnnotation so they fall through to generating generic arrays.

// ios
elements:(const NSArray *)elements
// android
ReadableArray locations

So how do I fix this?

I think there are a couple of different options here. The key problem is that the Schema types need to represent reality of what can be in the schema.

1. We revert DebuggingOverlay to not use features that aren't supported (I assume nobody would be happy with this, but the change shouldn't have been made in the first place)
2. (This is the approach taken in this diff) We add MixedTypeAnnotation to the allowed types in Command arrays and have it generate that and add official support for that to the TypeScript parser as well. That is probably the quickest and easiest approach. It leaves the same type unsafety we have today on the native side.
3. NativeModules seem to have a lot more complex type safety here. They persist the alias type in the schema so that the CompatCheck can check them on changes. And then in C++ they generate structs and RCTConvert functions although for Java and ObjC it looks like they just use the same untyped native code. The matching approach here would be to add aliasMap and the whole data to the schema for commands, use that for the compat check, and still generate the same unsafe native code.

export type ObjectAlias = {|
x: number,
y: number,
|};
export interface Spec extends TurboModule {
+getAlias: (a: ObjectAlias) => string;
}

stores the ObjectAlias in the schema

{
"aliasMap": {
"ObjectAlias": {
"type": "ObjectTypeAnnotation",
"properties": [
{
"name": "x",
"optional": false,
"typeAnnotation": {
"type": "NumberTypeAnnotation"
}
},
{
"name": "y",
"optional": false,
"typeAnnotation": {
"type": "NumberTypeAnnotation"
}
},
]
}
},
"spec": {
"methods": [
{
"name": "getAlias",
"optional": false,
"typeAnnotation": {
"type": "FunctionTypeAnnotation",
"returnTypeAnnotation": {
"type": "StringTypeAnnotation"
},
"params": [
{
"name": "a",
"optional": false,
"typeAnnotation": {
"type": "TypeAliasTypeAnnotation",
"name": "ObjectAlias"
}
}
]
}
}
]
}
}

and then generates the appropriate structs on the native side and generates this

Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request has been merged in 25c673e.

[elicwhite]

Summary:
Command param array types were generating invalid schemas due to untyped parser code. The invalid schemas occurred for any alias type, including custom objects and basics like Int32. This was also inconsistent between Flow and TypeScript.

We already had one component utilizing this issue, so this just codifies that support into the schema so it reflects reality. This is only a partial solution. The more full solution would be to fully encode the custom types in the schemas like we do for Native Modules.

More Information:

Tl;dr, DebuggingOverlay is abusing a FlowFixMe in codegen commands.

The problem:

The CodegenSchema should be the source of truth for anything that can be in the schema. If something is in the schema that isn't allowed by the types, that's a bug. We have a bug. I'm adding compat-check support for components and it's blowing up on prod schemas because DebuggingOverlay causes an invalid schema.

The details:

Support for Arrays as arguments in commands was added to the Codegen in D51866557. Code Pointer

The intention appears to be to support arrays of primitives. There is a TODO for supporting complex types.

interface NativeCommands {
+addOverlays: (
viewRef: React.ElementRef<NativeType>,
overlayColorsReadOnly: $ReadOnlyArray<string>,
)
}

This support was added to TypeScript in D52046165 where it types the allowed Array arguments to be only

{
readonly type: 'ArrayTypeAnnotation';
readonly elementType:
| Int32TypeAnnotation
| DoubleTypeAnnotation
| FloatTypeAnnotation
| BooleanTypeAnnotation
| StringTypeAnnotation
};

However, because the Parsers are treating the input type as any, it isn't safe to pass through an input value into the schema as Flow won't catch mismatches.

The Flow parser just passes it through:

{
type: 'ArrayTypeAnnotation',
elementType: {
// TODO: T172453752 support complex type annotation for array element
type: paramValue.typeParameters.params[0].type,
}

Whereas the TypeScript parser has the more correct behavior of validating the inputs and returning specific outputs. Unfortunately, the return type is also typed here as $FlowFixMe, losing most of the benefits.

function getPrimitiveTypeAnnotation(type: string): $FlowFixMe {
switch (type) {
case 'Int32':
return {
type: 'Int32TypeAnnotation',
};
case 'Double':
return {
type: 'DoubleTypeAnnotation',
};
case 'Float':
return {
type: 'FloatTypeAnnotation',
};
case 'TSBooleanKeyword':
return {
type: 'BooleanTypeAnnotation',
};
case 'Stringish':
case 'TSStringKeyword':
return {
type: 'StringTypeAnnotation',
};
default:
throw new Error(`Unknown primitive type "${type}"`);
}
}

DebuggingOverlay is abusing this gap in the Flow parser by sticking an Array of Objects in.

export type ElementRectangle = {
x: number,
y: number,
width: number,
height: number,
};
...
+highlightElements: (
viewRef: React.ElementRef<DebuggingOverlayNativeComponentType>,
elements: $ReadOnlyArray<ElementRectangle>,
) => void;
...

This isn't allowed in the schema, but it seems to fall through the holes of the flow parser and generators.

The resulting schema from Flow is this. Note the GenericTypeAnnotation which isn't allowed to be in the schema.

{
"name": "highlightElements",
"optional": false,
"typeAnnotation": {
"type": "FunctionTypeAnnotation",
"params": [
{
"name": "elements",
"optional": false,
"typeAnnotation": {
"type": "ArrayTypeAnnotation",
"elementType": {
"type": "GenericTypeAnnotation"
}
}
}
],
"returnTypeAnnotation": {
"type": "VoidTypeAnnotation"
}
},

The TypeScript parser fails with Error: Unsupported type annotation: GenericTypeAnnotation.

The generators don't seem to check beyond the ArrayTypeAnnotation so they fall through to generating generic arrays.

// ios
elements:(const NSArray *)elements
// android
ReadableArray locations

So how do I fix this?

I think there are a couple of different options here. The key problem is that the Schema types need to represent reality of what can be in the schema.

1. We revert DebuggingOverlay to not use features that aren't supported (I assume nobody would be happy with this, but the change shouldn't have been made in the first place)
2. (This is the approach taken in this diff) We add MixedTypeAnnotation to the allowed types in Command arrays and have it generate that and add official support for that to the TypeScript parser as well. That is probably the quickest and easiest approach. It leaves the same type unsafety we have today on the native side.
3. NativeModules seem to have a lot more complex type safety here. They persist the alias type in the schema so that the CompatCheck can check them on changes. And then in C++ they generate structs and RCTConvert functions although for Java and ObjC it looks like they just use the same untyped native code. The matching approach here would be to add aliasMap and the whole data to the schema for commands, use that for the compat check, and still generate the same unsafe native code.

export type ObjectAlias = {|
x: number,
y: number,
|};
export interface Spec extends TurboModule {
+getAlias: (a: ObjectAlias) => string;
}

stores the ObjectAlias in the schema

{
"aliasMap": {
"ObjectAlias": {
"type": "ObjectTypeAnnotation",
"properties": [
{
"name": "x",
"optional": false,
"typeAnnotation": {
"type": "NumberTypeAnnotation"
}
},
{
"name": "y",
"optional": false,
"typeAnnotation": {
"type": "NumberTypeAnnotation"
}
},
]
}
},
"spec": {
"methods": [
{
"name": "getAlias",
"optional": false,
"typeAnnotation": {
"type": "FunctionTypeAnnotation",
"returnTypeAnnotation": {
"type": "StringTypeAnnotation"
},
"params": [
{
"name": "a",
"optional": false,
"typeAnnotation": {
"type": "TypeAliasTypeAnnotation",
"name": "ObjectAlias"
}
}
]
}
}
]
}
}

and then generates the appropriate structs on the native side and generates this

Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request has been merged in 25c673e.

[elicwhite]

Summary:
Command param array types were generating invalid schemas due to untyped parser code. The invalid schemas occurred for any alias type, including custom objects and basics like Int32. This was also inconsistent between Flow and TypeScript.

We already had one component utilizing this issue, so this just codifies that support into the schema so it reflects reality. This is only a partial solution. The more full solution would be to fully encode the custom types in the schemas like we do for Native Modules.

More Information:

Tl;dr, DebuggingOverlay is abusing a FlowFixMe in codegen commands.

The problem:

The CodegenSchema should be the source of truth for anything that can be in the schema. If something is in the schema that isn't allowed by the types, that's a bug. We have a bug. I'm adding compat-check support for components and it's blowing up on prod schemas because DebuggingOverlay causes an invalid schema.

The details:

Support for Arrays as arguments in commands was added to the Codegen in D51866557. Code Pointer

The intention appears to be to support arrays of primitives. There is a TODO for supporting complex types.

interface NativeCommands {
+addOverlays: (
viewRef: React.ElementRef<NativeType>,
overlayColorsReadOnly: $ReadOnlyArray<string>,
)
}

This support was added to TypeScript in D52046165 where it types the allowed Array arguments to be only

{
readonly type: 'ArrayTypeAnnotation';
readonly elementType:
| Int32TypeAnnotation
| DoubleTypeAnnotation
| FloatTypeAnnotation
| BooleanTypeAnnotation
| StringTypeAnnotation
};

However, because the Parsers are treating the input type as any, it isn't safe to pass through an input value into the schema as Flow won't catch mismatches.

The Flow parser just passes it through:

{
type: 'ArrayTypeAnnotation',
elementType: {
// TODO: T172453752 support complex type annotation for array element
type: paramValue.typeParameters.params[0].type,
}

Whereas the TypeScript parser has the more correct behavior of validating the inputs and returning specific outputs. Unfortunately, the return type is also typed here as $FlowFixMe, losing most of the benefits.

function getPrimitiveTypeAnnotation(type: string): $FlowFixMe {
switch (type) {
case 'Int32':
return {
type: 'Int32TypeAnnotation',
};
case 'Double':
return {
type: 'DoubleTypeAnnotation',
};
case 'Float':
return {
type: 'FloatTypeAnnotation',
};
case 'TSBooleanKeyword':
return {
type: 'BooleanTypeAnnotation',
};
case 'Stringish':
case 'TSStringKeyword':
return {
type: 'StringTypeAnnotation',
};
default:
throw new Error(`Unknown primitive type "${type}"`);
}
}

DebuggingOverlay is abusing this gap in the Flow parser by sticking an Array of Objects in.

export type ElementRectangle = {
x: number,
y: number,
width: number,
height: number,
};
...
+highlightElements: (
viewRef: React.ElementRef<DebuggingOverlayNativeComponentType>,
elements: $ReadOnlyArray<ElementRectangle>,
) => void;
...

This isn't allowed in the schema, but it seems to fall through the holes of the flow parser and generators.

The resulting schema from Flow is this. Note the GenericTypeAnnotation which isn't allowed to be in the schema.

{
"name": "highlightElements",
"optional": false,
"typeAnnotation": {
"type": "FunctionTypeAnnotation",
"params": [
{
"name": "elements",
"optional": false,
"typeAnnotation": {
"type": "ArrayTypeAnnotation",
"elementType": {
"type": "GenericTypeAnnotation"
}
}
}
],
"returnTypeAnnotation": {
"type": "VoidTypeAnnotation"
}
},

The TypeScript parser fails with Error: Unsupported type annotation: GenericTypeAnnotation.

The generators don't seem to check beyond the ArrayTypeAnnotation so they fall through to generating generic arrays.

// ios
elements:(const NSArray *)elements
// android
ReadableArray locations

So how do I fix this?

I think there are a couple of different options here. The key problem is that the Schema types need to represent reality of what can be in the schema.

1. We revert DebuggingOverlay to not use features that aren't supported (I assume nobody would be happy with this, but the change shouldn't have been made in the first place)
2. (This is the approach taken in this diff) We add MixedTypeAnnotation to the allowed types in Command arrays and have it generate that and add official support for that to the TypeScript parser as well. That is probably the quickest and easiest approach. It leaves the same type unsafety we have today on the native side.
3. NativeModules seem to have a lot more complex type safety here. They persist the alias type in the schema so that the CompatCheck can check them on changes. And then in C++ they generate structs and RCTConvert functions although for Java and ObjC it looks like they just use the same untyped native code. The matching approach here would be to add aliasMap and the whole data to the schema for commands, use that for the compat check, and still generate the same unsafe native code.

export type ObjectAlias = {|
x: number,
y: number,
|};
export interface Spec extends TurboModule {
+getAlias: (a: ObjectAlias) => string;
}

stores the ObjectAlias in the schema

{
"aliasMap": {
"ObjectAlias": {
"type": "ObjectTypeAnnotation",
"properties": [
{
"name": "x",
"optional": false,
"typeAnnotation": {
"type": "NumberTypeAnnotation"
}
},
{
"name": "y",
"optional": false,
"typeAnnotation": {
"type": "NumberTypeAnnotation"
}
},
]
}
},
"spec": {
"methods": [
{
"name": "getAlias",
"optional": false,
"typeAnnotation": {
"type": "FunctionTypeAnnotation",
"returnTypeAnnotation": {
"type": "StringTypeAnnotation"
},
"params": [
{
"name": "a",
"optional": false,
"typeAnnotation": {
"type": "TypeAliasTypeAnnotation",
"name": "ObjectAlias"
}
}
]
}
}
]
}
}

and then generates the appropriate structs on the native side and generates this

Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request has been merged in 25c673e.

[elicwhite]

Summary:
Command param array types were generating invalid schemas due to untyped parser code. The invalid schemas occurred for any alias type, including custom objects and basics like Int32. This was also inconsistent between Flow and TypeScript.

We already had one component utilizing this issue, so this just codifies that support into the schema so it reflects reality. This is only a partial solution. The more full solution would be to fully encode the custom types in the schemas like we do for Native Modules.

More Information:

Tl;dr, DebuggingOverlay is abusing a FlowFixMe in codegen commands.

The problem:

The CodegenSchema should be the source of truth for anything that can be in the schema. If something is in the schema that isn't allowed by the types, that's a bug. We have a bug. I'm adding compat-check support for components and it's blowing up on prod schemas because DebuggingOverlay causes an invalid schema.

The details:

Support for Arrays as arguments in commands was added to the Codegen in D51866557. Code Pointer

The intention appears to be to support arrays of primitives. There is a TODO for supporting complex types.

interface NativeCommands {
+addOverlays: (
viewRef: React.ElementRef<NativeType>,
overlayColorsReadOnly: $ReadOnlyArray<string>,
)
}

This support was added to TypeScript in D52046165 where it types the allowed Array arguments to be only

{
readonly type: 'ArrayTypeAnnotation';
readonly elementType:
| Int32TypeAnnotation
| DoubleTypeAnnotation
| FloatTypeAnnotation
| BooleanTypeAnnotation
| StringTypeAnnotation
};

However, because the Parsers are treating the input type as any, it isn't safe to pass through an input value into the schema as Flow won't catch mismatches.

The Flow parser just passes it through:

{
type: 'ArrayTypeAnnotation',
elementType: {
// TODO: T172453752 support complex type annotation for array element
type: paramValue.typeParameters.params[0].type,
}

Whereas the TypeScript parser has the more correct behavior of validating the inputs and returning specific outputs. Unfortunately, the return type is also typed here as $FlowFixMe, losing most of the benefits.

function getPrimitiveTypeAnnotation(type: string): $FlowFixMe {
switch (type) {
case 'Int32':
return {
type: 'Int32TypeAnnotation',
};
case 'Double':
return {
type: 'DoubleTypeAnnotation',
};
case 'Float':
return {
type: 'FloatTypeAnnotation',
};
case 'TSBooleanKeyword':
return {
type: 'BooleanTypeAnnotation',
};
case 'Stringish':
case 'TSStringKeyword':
return {
type: 'StringTypeAnnotation',
};
default:
throw new Error(`Unknown primitive type "${type}"`);
}
}

DebuggingOverlay is abusing this gap in the Flow parser by sticking an Array of Objects in.

export type ElementRectangle = {
x: number,
y: number,
width: number,
height: number,
};
...
+highlightElements: (
viewRef: React.ElementRef<DebuggingOverlayNativeComponentType>,
elements: $ReadOnlyArray<ElementRectangle>,
) => void;
...

This isn't allowed in the schema, but it seems to fall through the holes of the flow parser and generators.

The resulting schema from Flow is this. Note the GenericTypeAnnotation which isn't allowed to be in the schema.

{
"name": "highlightElements",
"optional": false,
"typeAnnotation": {
"type": "FunctionTypeAnnotation",
"params": [
{
"name": "elements",
"optional": false,
"typeAnnotation": {
"type": "ArrayTypeAnnotation",
"elementType": {
"type": "GenericTypeAnnotation"
}
}
}
],
"returnTypeAnnotation": {
"type": "VoidTypeAnnotation"
}
},

The TypeScript parser fails with Error: Unsupported type annotation: GenericTypeAnnotation.

The generators don't seem to check beyond the ArrayTypeAnnotation so they fall through to generating generic arrays.

// ios
elements:(const NSArray *)elements
// android
ReadableArray locations

So how do I fix this?

I think there are a couple of different options here. The key problem is that the Schema types need to represent reality of what can be in the schema.

1. We revert DebuggingOverlay to not use features that aren't supported (I assume nobody would be happy with this, but the change shouldn't have been made in the first place)
2. (This is the approach taken in this diff) We add MixedTypeAnnotation to the allowed types in Command arrays and have it generate that and add official support for that to the TypeScript parser as well. That is probably the quickest and easiest approach. It leaves the same type unsafety we have today on the native side.
3. NativeModules seem to have a lot more complex type safety here. They persist the alias type in the schema so that the CompatCheck can check them on changes. And then in C++ they generate structs and RCTConvert functions although for Java and ObjC it looks like they just use the same untyped native code. The matching approach here would be to add aliasMap and the whole data to the schema for commands, use that for the compat check, and still generate the same unsafe native code.

export type ObjectAlias = {|
x: number,
y: number,
|};
export interface Spec extends TurboModule {
+getAlias: (a: ObjectAlias) => string;
}

stores the ObjectAlias in the schema

{
"aliasMap": {
"ObjectAlias": {
"type": "ObjectTypeAnnotation",
"properties": [
{
"name": "x",
"optional": false,
"typeAnnotation": {
"type": "NumberTypeAnnotation"
}
},
{
"name": "y",
"optional": false,
"typeAnnotation": {
"type": "NumberTypeAnnotation"
}
},
]
}
},
"spec": {
"methods": [
{
"name": "getAlias",
"optional": false,
"typeAnnotation": {
"type": "FunctionTypeAnnotation",
"returnTypeAnnotation": {
"type": "StringTypeAnnotation"
},
"params": [
{
"name": "a",
"optional": false,
"typeAnnotation": {
"type": "TypeAliasTypeAnnotation",
"name": "ObjectAlias"
}
}
]
}
}
]
}
}

and then generates the appropriate structs on the native side and generates this

Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request has been merged in 25c673e.

[elicwhite]

Summary:
Command param array types were generating invalid schemas due to untyped parser code. The invalid schemas occurred for any alias type, including custom objects and basics like Int32. This was also inconsistent between Flow and TypeScript.

We already had one component utilizing this issue, so this just codifies that support into the schema so it reflects reality. This is only a partial solution. The more full solution would be to fully encode the custom types in the schemas like we do for Native Modules.

More Information:

Tl;dr, DebuggingOverlay is abusing a FlowFixMe in codegen commands.

The problem:

The CodegenSchema should be the source of truth for anything that can be in the schema. If something is in the schema that isn't allowed by the types, that's a bug. We have a bug. I'm adding compat-check support for components and it's blowing up on prod schemas because DebuggingOverlay causes an invalid schema.

The details:

Support for Arrays as arguments in commands was added to the Codegen in D51866557. Code Pointer

The intention appears to be to support arrays of primitives. There is a TODO for supporting complex types.

interface NativeCommands {
+addOverlays: (
viewRef: React.ElementRef<NativeType>,
overlayColorsReadOnly: $ReadOnlyArray<string>,
)
}

This support was added to TypeScript in D52046165 where it types the allowed Array arguments to be only

{
readonly type: 'ArrayTypeAnnotation';
readonly elementType:
| Int32TypeAnnotation
| DoubleTypeAnnotation
| FloatTypeAnnotation
| BooleanTypeAnnotation
| StringTypeAnnotation
};

However, because the Parsers are treating the input type as any, it isn't safe to pass through an input value into the schema as Flow won't catch mismatches.

The Flow parser just passes it through:

{
type: 'ArrayTypeAnnotation',
elementType: {
// TODO: T172453752 support complex type annotation for array element
type: paramValue.typeParameters.params[0].type,
}

Whereas the TypeScript parser has the more correct behavior of validating the inputs and returning specific outputs. Unfortunately, the return type is also typed here as $FlowFixMe, losing most of the benefits.

function getPrimitiveTypeAnnotation(type: string): $FlowFixMe {
switch (type) {
case 'Int32':
return {
type: 'Int32TypeAnnotation',
};
case 'Double':
return {
type: 'DoubleTypeAnnotation',
};
case 'Float':
return {
type: 'FloatTypeAnnotation',
};
case 'TSBooleanKeyword':
return {
type: 'BooleanTypeAnnotation',
};
case 'Stringish':
case 'TSStringKeyword':
return {
type: 'StringTypeAnnotation',
};
default:
throw new Error(`Unknown primitive type "${type}"`);
}
}

DebuggingOverlay is abusing this gap in the Flow parser by sticking an Array of Objects in.

export type ElementRectangle = {
x: number,
y: number,
width: number,
height: number,
};
...
+highlightElements: (
viewRef: React.ElementRef<DebuggingOverlayNativeComponentType>,
elements: $ReadOnlyArray<ElementRectangle>,
) => void;
...

This isn't allowed in the schema, but it seems to fall through the holes of the flow parser and generators.

The resulting schema from Flow is this. Note the GenericTypeAnnotation which isn't allowed to be in the schema.

{
"name": "highlightElements",
"optional": false,
"typeAnnotation": {
"type": "FunctionTypeAnnotation",
"params": [
{
"name": "elements",
"optional": false,
"typeAnnotation": {
"type": "ArrayTypeAnnotation",
"elementType": {
"type": "GenericTypeAnnotation"
}
}
}
],
"returnTypeAnnotation": {
"type": "VoidTypeAnnotation"
}
},

The TypeScript parser fails with Error: Unsupported type annotation: GenericTypeAnnotation.

The generators don't seem to check beyond the ArrayTypeAnnotation so they fall through to generating generic arrays.

// ios
elements:(const NSArray *)elements
// android
ReadableArray locations

So how do I fix this?

I think there are a couple of different options here. The key problem is that the Schema types need to represent reality of what can be in the schema.

1. We revert DebuggingOverlay to not use features that aren't supported (I assume nobody would be happy with this, but the change shouldn't have been made in the first place)
2. (This is the approach taken in this diff) We add MixedTypeAnnotation to the allowed types in Command arrays and have it generate that and add official support for that to the TypeScript parser as well. That is probably the quickest and easiest approach. It leaves the same type unsafety we have today on the native side.
3. NativeModules seem to have a lot more complex type safety here. They persist the alias type in the schema so that the CompatCheck can check them on changes. And then in C++ they generate structs and RCTConvert functions although for Java and ObjC it looks like they just use the same untyped native code. The matching approach here would be to add aliasMap and the whole data to the schema for commands, use that for the compat check, and still generate the same unsafe native code.

export type ObjectAlias = {|
x: number,
y: number,
|};
export interface Spec extends TurboModule {
+getAlias: (a: ObjectAlias) => string;
}

stores the ObjectAlias in the schema

{
"aliasMap": {
"ObjectAlias": {
"type": "ObjectTypeAnnotation",
"properties": [
{
"name": "x",
"optional": false,
"typeAnnotation": {
"type": "NumberTypeAnnotation"
}
},
{
"name": "y",
"optional": false,
"typeAnnotation": {
"type": "NumberTypeAnnotation"
}
},
]
}
},
"spec": {
"methods": [
{
"name": "getAlias",
"optional": false,
"typeAnnotation": {
"type": "FunctionTypeAnnotation",
"returnTypeAnnotation": {
"type": "StringTypeAnnotation"
},
"params": [
{
"name": "a",
"optional": false,
"typeAnnotation": {
"type": "TypeAliasTypeAnnotation",
"name": "ObjectAlias"
}
}
]
}
}
]
}
}

and then generates the appropriate structs on the native side and generates this

Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D67806838

[facebook-github-bot]

This pull request has been merged in 25c673e.