Add ios_assume_nonnull flag to react native codegen library (#31543)

Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/31543 Changelog: [iOS][Added] - Description When compiling iOS apps with flag `-Wnullability-completeness` (like Lightspeed app and soon Instagram), Objective-C headers are required to either have full *explicit* nullability annotations on all members of its public API, or none at all; partially annotated headers will fail to build that module. RN native modules are currently generated with *partial* annotations. This works today because most apps are not compiled with `-Wnullability-completeness` turned on. But when we flip the switch for Instagram, the app doesn't build due to importing these RN partially annotated modules. JavsScript Flow types are implied nonnull, and the current RN codegen translates Flow's [maybe/optional](https://flow.org/en/docs/types/maybe/) type to Obj-C `_Nullable` annotation, and everything else without an explicit Obj-C annotation. However this creates a mismatch with the Obj-C type system, where the implied default is *unannotated*, which is handled differently from nonnull when built with the nullability compiler flags. There is a simple Obj-C macro that automatically adds *explicit nonnull* annotations to all members in a header: `NS_ASSUME_NONNULL_BEGIN` / `NS_ASSUME_NONNULL_END`. If we add this to *all* RN-generated headers, however, we run into issues: 1) We may erroneously assume any previously-unannotated header was meant to be nonnull and cause future bugs 2) Another compiler flag (`-Wnullable-to-nonnull-conversion`) statically analyzes Obj-C implementation code to prevent us from ever passing null to one of these headers. Much existing Obj-C code will break here, and it's ambiguous if these are true or false positives because of the first point. Instead, in this diff we add a new BUCK flag `ios_assume_nonnull` to let module authors opt into automatic nonnull for unannotated members so that Obj-C headers are generated correctly in alignment with Flow's type system. We can migrate all libraries individually as needed and eventually make this the RN native codegen default. Reviewed By: RSNara Differential Revision: D28396446 fbshipit-source-id: ad3a3a97ab19183df4ef504b1c3140596c8f69ca
2025-11-01 09:14:26 +00:00 · 2021-05-20 10:07:37 -07:00
parent d670381fac
commit fa4045e4dd
27 changed files with 1246 additions and 10 deletions
@@ -12,6 +12,7 @@ rn_codegen(
    name = "FBReactNativeSpec",
    android_package_name = "com.facebook.fbreact.specs",
    codegen_modules = True,
+    ios_assume_nonnull = False,
    library_labels = ["supermodule:xplat/default/public.react_native.infra"],
    native_module_spec_name = "FBReactNativeSpec",
 )
@@ -20,5 +21,6 @@ rn_codegen(
 rn_codegen(
    name = "FBReactNativeComponentSpec",
    codegen_components = True,
+    ios_assume_nonnull = False,
    library_labels = ["supermodule:xplat/default/public.react_native.infra"],
 )
@@ -29,6 +29,7 @@ rn_codegen_components(
 rn_codegen_modules(
    name = "FBReactNativeTestSpec",
    android_package_name = "com.facebook.fbreact.specs",
+    ios_assume_nonnull = False,
    schema_target = ":codegen_tests_schema",
 )

@@ -105,6 +105,7 @@ def rn_codegen_cli():
 def rn_codegen_modules(
        name,
        android_package_name,
+        ios_assume_nonnull,
        library_labels = [],
        schema_target = ""):
    generate_fixtures_rule_name = "{}-codegen-modules".format(name)
@@ -118,11 +119,12 @@ def rn_codegen_modules(
    fb_native.genrule(
        name = generate_fixtures_rule_name,
        srcs = native.glob(["src/generators/**/*.js"]),
-        cmd = "$(exe {generator_script}) $(location {schema_target}) {library_name} $OUT {android_package_name}".format(
+        cmd = "$(exe {generator_script}) $(location {schema_target}) {library_name} $OUT {android_package_name} {ios_assume_nonnull}".format(
            generator_script = react_native_root_target("packages/react-native-codegen:generate_all_from_schema"),
            schema_target = schema_target,
            library_name = name,
            android_package_name = android_package_name,
+            ios_assume_nonnull = ios_assume_nonnull,
        ),
        out = "codegenfiles-{}".format(name),
        labels = ["codegen_rule"],
@@ -38,13 +38,19 @@ function getModules(): SchemaType {
 describe('GenerateModuleObjCpp', () => {
  it('can generate a header file NativeModule specs', () => {
    const libName = 'RNCodegenModuleFixtures';
-    const output = generator.generate(libName, getModules());
+    const output = generator.generate(libName, getModules(), undefined, false);
+    expect(output.get(libName + '.h')).toMatchSnapshot();
+  });
+
+  it('can generate a header file NativeModule specs with assume nonnull enabled', () => {
+    const libName = 'RNCodegenModuleFixtures';
+    const output = generator.generate(libName, getModules(), undefined, true);
    expect(output.get(libName + '.h')).toMatchSnapshot();
  });

  it('can generate an implementation file NativeModule specs', () => {
    const libName = 'RNCodegenModuleFixtures';
-    const output = generator.generate(libName, getModules());
+    const output = generator.generate(libName, getModules(), undefined, false);
    expect(output.get(libName + '-generated.mm')).toMatchSnapshot();
  });
 });
@@ -31,6 +31,7 @@ const schemaPath = args[0];
 const libraryName = args[1];
 const outputDirectory = args[2];
 const packageName = args[3];
+const assumeNonnull = args[4] === 'true' || args[4] === 'True';

 const schemaText = fs.readFileSync(schemaPath, 'utf-8');

@@ -48,7 +49,7 @@ try {
 }

 RNCodegen.generate(
-  {libraryName, schema, outputDirectory, packageName},
+  {libraryName, schema, outputDirectory, packageName, assumeNonnull},
  {
    generators: [
      'descriptors',
@@ -45,6 +45,7 @@ type Options = $ReadOnly<{
  schema: SchemaType,
  outputDirectory: string,
  packageName?: string, // Some platforms have a notion of package, which should be configurable.
+  assumeNonnull: boolean,
 }>;

 type Generators =
@@ -152,7 +153,7 @@ function checkFilesForChanges(

 module.exports = {
  generate(
-    {libraryName, schema, outputDirectory, packageName}: Options,
+    {libraryName, schema, outputDirectory, packageName, assumeNonnull}: Options,
    {generators, test}: Config,
  ): boolean {
    schemaValidator.validate(schema);
@@ -160,7 +161,9 @@ module.exports = {
    const generatedFiles = [];
    for (const name of generators) {
      for (const generator of GENERATORS[name]) {
-        generatedFiles.push(...generator(libraryName, schema, packageName));
+        generatedFiles.push(
+          ...generator(libraryName, schema, packageName, assumeNonnull),
+        );
      }
    }

@@ -48,6 +48,7 @@ module.exports = {
    libraryName: string,
    schema: SchemaType,
    packageName?: string,
+    assumeNonnull: boolean = false,
  ): FilesOutput {
    const fileName = 'ComponentDescriptors.h';

@@ -324,6 +324,7 @@ module.exports = {
    libraryName: string,
    schema: SchemaType,
    packageName?: string,
+    assumeNonnull: boolean = false,
  ): FilesOutput {
    const fileName = 'RCTComponentViewHelpers.h';

@@ -190,6 +190,7 @@ module.exports = {
    libraryName: string,
    schema: SchemaType,
    packageName?: string,
+    assumeNonnull: boolean = false,
  ): FilesOutput {
    const moduleComponents: ComponentCollection = Object.keys(schema.modules)
      .map(moduleName => {
@@ -239,6 +239,7 @@ module.exports = {
    libraryName: string,
    schema: SchemaType,
    packageName?: string,
+    assumeNonnull: boolean = false,
  ): FilesOutput {
    const moduleComponents: ComponentCollection = Object.keys(schema.modules)
      .map(moduleName => {
@@ -86,6 +86,7 @@ module.exports = {
    libraryName: string,
    schema: SchemaType,
    packageName?: string,
+    assumeNonnull: boolean = false,
  ): FilesOutput {
    const fileName = 'Props.cpp';
    const allImports: Set<string> = new Set([
@@ -752,6 +752,7 @@ module.exports = {
    libraryName: string,
    schema: SchemaType,
    packageName?: string,
+    assumeNonnull: boolean = false,
  ): FilesOutput {
    const fileName = 'Props.h';

@@ -265,6 +265,7 @@ module.exports = {
    libraryName: string,
    schema: SchemaType,
    packageName?: string,
+    assumeNonnull: boolean = false,
  ): FilesOutput {
    // TODO: This doesn't support custom package name yet.
    const normalizedPackageName = 'com.facebook.react.viewmanagers';
@@ -213,6 +213,7 @@ module.exports = {
    libraryName: string,
    schema: SchemaType,
    packageName?: string,
+    assumeNonnull: boolean = false,
  ): FilesOutput {
    // TODO: This doesn't support custom package name yet.
    const normalizedPackageName = 'com.facebook.react.viewmanagers';
@@ -45,6 +45,7 @@ module.exports = {
    libraryName: string,
    schema: SchemaType,
    packageName?: string,
+    assumeNonnull: boolean = false,
  ): FilesOutput {
    const fileName = 'ShadowNodes.cpp';

@@ -55,6 +55,7 @@ module.exports = {
    libraryName: string,
    schema: SchemaType,
    packageName?: string,
+    assumeNonnull: boolean = false,
  ): FilesOutput {
    const fileName = 'ShadowNodes.h';

@@ -140,6 +140,7 @@ module.exports = {
    libraryName: string,
    schema: SchemaType,
    packageName?: string,
+    assumeNonnull: boolean = false,
  ): FilesOutput {
    const fileName = 'Tests.cpp';
    const allImports = new Set([
@@ -185,6 +185,7 @@ module.exports = {
    libraryName: string,
    schema: SchemaType,
    packageName?: string,
+    assumeNonnull: boolean = false,
  ): FilesOutput {
    const nativeModules = getModules(schema);

@@ -124,6 +124,7 @@ module.exports = {
    libraryName: string,
    schema: SchemaType,
    packageName?: string,
+    assumeNonnull: boolean = false,
  ): FilesOutput {
    const nativeModules = getModules(schema);

@@ -369,6 +369,7 @@ module.exports = {
    libraryName: string,
    schema: SchemaType,
    packageName?: string,
+    assumeNonnull: boolean = false,
  ): FilesOutput {
    const files = new Map();
    const nativeModules = getModules(schema);
@@ -358,6 +358,7 @@ module.exports = {
    libraryName: string,
    schema: SchemaType,
    packageName?: string,
+    assumeNonnull: boolean = false,
  ): FilesOutput {
    const nativeModules = getModules(schema);

@@ -98,6 +98,7 @@ module.exports = {
    libraryName: string,
    schema: SchemaType,
    packageName?: string,
+    assumeNonnull: boolean = false,
  ): FilesOutput {
    const nativeModules = getModules(schema);
    const modules = Object.keys(nativeModules)
@@ -51,10 +51,13 @@ namespace facebook {
 const HeaderFileTemplate = ({
  moduleDeclarations,
  structInlineMethods,
+  assumeNonnull,
 }: $ReadOnly<{
  moduleDeclarations: string,
  structInlineMethods: string,
-}>) => `/**
+  assumeNonnull: boolean,
+}>) =>
+  `/**
 * ${'C'}opyright (c) Facebook, Inc. and its affiliates.
 *
 * This source code is licensed under the MIT license found in the
@@ -81,9 +84,12 @@ const HeaderFileTemplate = ({
 #import <folly/Optional.h>
 #import <vector>

-${moduleDeclarations}
-${structInlineMethods}
-`;
+` +
+  (assumeNonnull ? '\nNS_ASSUME_NONNULL_BEGIN\n' : '') +
+  moduleDeclarations +
+  '\n' +
+  structInlineMethods +
+  (assumeNonnull ? '\nNS_ASSUME_NONNULL_END\n' : '\n');

 const SourceFileTemplate = ({
  headerFileName,
@@ -114,6 +120,7 @@ module.exports = {
    libraryName: string,
    schema: SchemaType,
    packageName?: string,
+    assumeNonnull: boolean,
  ): FilesOutput {
    const nativeModules = getModules(schema);

@@ -194,6 +201,7 @@ module.exports = {
    const headerFile = HeaderFileTemplate({
      moduleDeclarations: moduleDeclarations.join('\n'),
      structInlineMethods: structInlineMethods.join('\n'),
+      assumeNonnull,
    });

    const sourceFileName = `${libraryName}-generated.mm`;
@@ -25,6 +25,7 @@ describe('GenerateModuleHObjCpp', () => {
          fixtureName,
          fixture,
          'com.facebook.fbreact.specs',
+          false,
        );
        expect(
          new Map([[`${fixtureName}.h`, output.get(`${fixtureName}.h`)]]),
@@ -25,6 +25,7 @@ describe('GenerateModuleMm', () => {
          fixtureName,
          fixture,
          'com.facebook.fbreact.specs',
+          false,
        );
        expect(
          new Map([
@@ -22,6 +22,7 @@ rn_codegen_modules = _rn_codegen_modules

 def rn_codegen(
        name,
+        ios_assume_nonnull,
        native_module_spec_name = None,
        android_package_name = None,
        codegen_components = False,
@@ -57,6 +58,7 @@ def rn_codegen(
        rn_codegen_modules(
            name = native_module_spec_name,
            android_package_name = android_package_name,
+            ios_assume_nonnull = ios_assume_nonnull,
            schema_target = ":{}".format(module_schema_target),
            library_labels = library_labels,
        )