Skip to content

Commit

Permalink
Fix generated props CPP default values
Browse files Browse the repository at this point in the history
Summary:
This one is fun.

# What?

Previously, the codegen'd constructor for a prop value in CPP was defined like so: `value(convertRawProp(rawProps, "value", sourceProps.value, value))`.

The fourth argument there is the default value of the result of `convertRawProps`. What that is saying is: the default value of `value` is `value` - the default value is itself.

The assumption was that because value is defined as `T value{someDefaultValue}` in the struct, in other words, because it has a default value in the `.h` file, that it will /start out/ being equal to `someDefaultValue`, and then be overridden later optionally, or be set to itself, which should be `someDefaultValue`.

However, that is not how initialization of class members in C++ constructors work. If a class member is in the member initializer list, [then the default value is not set](https://en.cppreference.com/w/cpp/language/initializer_list). That means that if the `defaultValue` as passed is /ever/ used, we incur undefined behavior.

# When is the defaultValue used?

The defaultValue is only used when no prop or a null value is sent from JS to C++: https://our.intern.facebook.com/intern/diffusion/FBS/browse/master/xplat/js/react-native-github/ReactCommon/fabric/core/propsConversions.h?commit=becfded106d706e6028e705d7883483051061e9f&lines=60

In most cases, the `sourceProps.value` is actually used as a fallback (the previous props value). The first time props are ever constructed, `sourceProps` should have valid values since it goes through a different path where only the defaultValues from props.h are used.

# Why wasn't this crashing before?

Most codegen'd properties are ints, floats, doubles, and bools. They might get wacky values, but they won't crash. I wouldn't be surprised if this diff solves some subtle visual or layout bugs, but I have no evidence of this yet.

# How do non-codegen'd props.cpp initialize default values?

Same as this diff does: defaultValue should be explicit everywhere: https://our.intern.facebook.com/intern/diffusion/FBS/browse/master/xplat/js/react-native-github/ReactCommon/fabric/components/scrollview/ScrollViewProps.cpp?commit=2813789c292dfdf1220b88f203af6b33ba9e42de&lines=51

# So... what have we learned?

C++ is tricky!

Reviewed By: yungsters, shergin

Differential Revision: D16955421

fbshipit-source-id: 75bb3f22822299e17df1c36abecdb6ce49012406
  • Loading branch information
JoshuaGross authored and facebook-github-bot committed Aug 22, 2019
1 parent 2f7732b commit cafbf67
Show file tree
Hide file tree
Showing 4 changed files with 134 additions and 124 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,88 @@ function generateStructName(
return `${componentName}${additional}Struct`;
}

function getEnumName(componentName: string, propName: string): string {
const uppercasedPropName = toSafeCppString(propName);
return `${componentName}${uppercasedPropName}`;
}

function getEnumMaskName(enumName: string): string {
return `${enumName}Mask`;
}

function convertDefaultTypeToString(
componentName: string,
prop: PropTypeShape,
): string {
const typeAnnotation = prop.typeAnnotation;
switch (typeAnnotation.type) {
case 'BooleanTypeAnnotation':
return String(typeAnnotation.default);
case 'StringTypeAnnotation':
if (typeAnnotation.default == null) {
return '';
}
return `"${typeAnnotation.default}"`;
case 'Int32TypeAnnotation':
return String(typeAnnotation.default);
case 'DoubleTypeAnnotation':
const defaultDoubleVal = typeAnnotation.default;
return parseInt(defaultDoubleVal, 10) === defaultDoubleVal
? typeAnnotation.default.toFixed(1)
: String(typeAnnotation.default);
case 'FloatTypeAnnotation':
const defaultFloatVal = typeAnnotation.default;
return parseInt(defaultFloatVal, 10) === defaultFloatVal
? typeAnnotation.default.toFixed(1)
: String(typeAnnotation.default);
case 'NativePrimitiveTypeAnnotation':
switch (typeAnnotation.name) {
case 'ColorPrimitive':
return '';
case 'ImageSourcePrimitive':
return '';
case 'PointPrimitive':
return '';
default:
(typeAnnotation.name: empty);
throw new Error('Received unknown NativePrimitiveTypeAnnotation');
}
case 'ArrayTypeAnnotation': {
switch (typeAnnotation.elementType.type) {
case 'StringEnumTypeAnnotation':
if (typeAnnotation.elementType.default == null) {
throw new Error(
'A default is required for array StringEnumTypeAnnotation',
);
}
const enumName = getEnumName(componentName, prop.name);
const enumMaskName = getEnumMaskName(enumName);
const defaultValue = `${enumName}::${toSafeCppString(
typeAnnotation.elementType.default || '',
)}`;
return `static_cast<${enumMaskName}>(${defaultValue})`;
default:
return '';
}
}
case 'ObjectTypeAnnotation': {
return '';
}
case 'StringEnumTypeAnnotation':
return `${getEnumName(componentName, prop.name)}::${toSafeCppString(
typeAnnotation.default,
)}`;
default:
(typeAnnotation: empty);
throw new Error('Received invalid typeAnnotation');
}
}

module.exports = {
convertDefaultTypeToString,
getCppTypeForAnnotation,
getEnumName,
getEnumMaskName,
getImports,
toSafeCppString,
generateStructName,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
'use strict';

import type {ComponentShape, SchemaType} from '../../CodegenSchema';
const {getImports} = require('./CppHelpers');
const {convertDefaultTypeToString, getImports} = require('./CppHelpers');

// File path -> contents
type FilesOutput = Map<string, string>;
Expand Down Expand Up @@ -45,12 +45,13 @@ const componentTemplate = `
{}
`.trim();

function generatePropsString(component: ComponentShape) {
function generatePropsString(componentName: string, component: ComponentShape) {
return component.props
.map(prop => {
const defaultValue = convertDefaultTypeToString(componentName, prop);
return `${prop.name}(convertRawProp(rawProps, "${
prop.name
}", sourceProps.${prop.name}, ${prop.name}))`;
}", sourceProps.${prop.name}, {${defaultValue}}))`;
})
.join(',\n' + ' ');
}
Expand Down Expand Up @@ -104,7 +105,7 @@ module.exports = {
const component = components[componentName];
const newName = `${componentName}Props`;

const propsString = generatePropsString(component);
const propsString = generatePropsString(componentName, component);
const extendString = getClassExtendString(component);

const imports = getImports(component.props);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@
'use strict';

const {
convertDefaultTypeToString,
getCppTypeForAnnotation,
getEnumMaskName,
getEnumName,
toSafeCppString,
generateStructName,
getImports,
Expand Down Expand Up @@ -239,80 +242,6 @@ function getNativeTypeFromAnnotation(
}
}

function convertDefaultTypeToString(componentName: string, prop): string {
const typeAnnotation = prop.typeAnnotation;
switch (typeAnnotation.type) {
case 'BooleanTypeAnnotation':
return String(typeAnnotation.default);
case 'StringTypeAnnotation':
if (typeAnnotation.default == null) {
return '';
}
return `"${typeAnnotation.default}"`;
case 'Int32TypeAnnotation':
return String(typeAnnotation.default);
case 'DoubleTypeAnnotation':
const defaultDoubleVal = typeAnnotation.default;
return parseInt(defaultDoubleVal, 10) === defaultDoubleVal
? typeAnnotation.default.toFixed(1)
: String(typeAnnotation.default);
case 'FloatTypeAnnotation':
const defaultFloatVal = typeAnnotation.default;
return parseInt(defaultFloatVal, 10) === defaultFloatVal
? typeAnnotation.default.toFixed(1)
: String(typeAnnotation.default);
case 'NativePrimitiveTypeAnnotation':
switch (typeAnnotation.name) {
case 'ColorPrimitive':
return '';
case 'ImageSourcePrimitive':
return '';
case 'PointPrimitive':
return '';
default:
(typeAnnotation.name: empty);
throw new Error('Received unknown NativePrimitiveTypeAnnotation');
}
case 'ArrayTypeAnnotation': {
switch (typeAnnotation.elementType.type) {
case 'StringEnumTypeAnnotation':
if (typeAnnotation.elementType.default == null) {
throw new Error(
'A default is required for array StringEnumTypeAnnotation',
);
}
const enumName = getEnumName(componentName, prop.name);
const enumMaskName = getEnumMaskName(enumName);
const defaultValue = `${enumName}::${toSafeCppString(
typeAnnotation.elementType.default || '',
)}`;
return `static_cast<${enumMaskName}>(${defaultValue})`;
default:
return '';
}
}
case 'ObjectTypeAnnotation': {
return '';
}
case 'StringEnumTypeAnnotation':
return `${getEnumName(componentName, prop.name)}::${toSafeCppString(
typeAnnotation.default,
)}`;
default:
(typeAnnotation: empty);
throw new Error('Received invalid typeAnnotation');
}
}

function getEnumName(componentName, propName): string {
const uppercasedPropName = toSafeCppString(propName);
return `${componentName}${uppercasedPropName}`;
}

function getEnumMaskName(enumName: string): string {
return `${enumName}Mask`;
}

function convertValueToEnumOption(value: string): string {
return toSafeCppString(value);
}
Expand Down
Loading

0 comments on commit cafbf67

Please sign in to comment.