Fix build errors when a native module includes Swift

[tsapeta]

Summary:

Similar to #34527, but covers much more Objective-C++ headers that are now included in the autogenerated React-Core-umbrella.h.

Changelog:

[IOS] [FIXED] - Fixed build errors when a native module includes Swift

Test Plan:

RNTester builds without errors after adding a Swift file to https://github.com/facebook/react-native/tree/main/packages/rn-tester/NativeModuleExample and running USE_FRAMEWORKS=static pod install.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,892,878	-1
android	hermes	armeabi-v7a	7,941,614	-1
android	hermes	x86	9,290,634	-3
android	hermes	x86_64	9,192,292	-3
android	jsc	arm64-v8a	9,485,475	-2
android	jsc	armeabi-v7a	8,427,159	-4
android	jsc	x86	9,469,397	-1
android	jsc	x86_64	9,783,772	-2

Base commit: 589aea0
Branch: main

[cipolleschi]

I'm a bit confused, because these errors should not happen: a .h file is not build unless a .m or .mm file imports it.

A .h files imported by .m files must not contains C++ code/headers or they fail with the error you are seeing. But, if we rename the .m to .mm the error should go away.

I feel that this change fixes the symptom but not the root cause: instead of adding #ifdef __cplusplus all over the place, can we try to look for the .m that is making the build fail, and rename that file, instead?

The maintenance of the codebase would be better with that change instead of this one.

[tsapeta]

if we rename the .m to .mm the error should go away.

This is a solution, but only for Objective-C. If any .m file imports one of those headers, you would see compilation errors even without adding any Swift file, thus there is nothing to rename (those headers are already imported only from .mm files).
Swift relies on the generated umbrella header which basically contains all public headers, including these that contain C++ code (the umbrella header generator doesn't know if the header is actually Objective-C or C++). For Swift, the C++ specific code needs to be stripped down from headers.

I feel that this change fixes the symptom but not the root cause

The root cause is exactly what I described in this PR. To integrate well with Swift, all public headers must not contain C++ code.

The maintenance of the codebase would be better

I think the maintenance would be easier if we have a better separation between Objective-C and C++ code, instead of trying to mix them in headers.

Moreover, I think that #37275 also introduced some regressions in Swift integration. In particular, it adds glog as a dependency of native modules, which will not work as glog cannot define a module. I was going to fix it in a separate PR.

EDIT: I closed the PR by accident, I'm re-opening it in the next comment 😅

[tsapeta]

I'm also going to add a dummy Swift file to the native module example so we can see issues like this immediately. Sadly, missing #ifdef __cplusplus and the direct dependency on glog are not the only problems currently on main.

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[NickGerleman]

I don't like the number of times I've seen this pattern work its way into C++ headers inside of RN. It is quite fragile, and any new header could add breakage.

Is it possible to use an explicitly defined umbrella header or module map? Here is a recent example where someone have a modulemap that SwiftPM uses to make sure the umbrella header only includes the public C APIs. https://github.com/facebook/yoga/blob/6f958afd3b10d2845a08ca0ad2917a87b09a7867/yoga/module.modulemap

[tsapeta]

I don't like the number of times I've seen this pattern work its way into C++ headers inside of RN. It is quite fragile, and any new header could add breakage

I agree that this probably isn't perfect, but it's as common as extern "C" wrapped by #ifdef __cplusplus and for similar purpose. To prevent accidental breakages in the future, I'm going to set up the repo properly so that CI fails when someone forgets about adding this directive. My plan was to do the following:

• Add a dummy Swift file to the native module example used by RNTester
• Apply some necessary fixes (mostly in podspecs) to make React compilable with Swift libraries without the need to use use_frameworks! :linkage => :static or add :modular_headers => true for some pods in the Podfile.

Is it possible to use an explicitly defined umbrella header or module map?

It is possible, but I don't think it solves the problem. You can achieve the same result by just adding those files to private_header_files in the podspec (only public headers are included in the umbrella header). However, if any of the public header imports a private header that contains C++ code, you will get exactly the same compilation errors. Please notice that even in yoga, there are dozens of these directives, also to remove C++ includes. https://github.com/facebook/yoga/blob/c18e2566a0718384869c798e6b6ebebc3b30087a/yoga/YGMacros.h#L10-L12

[NickGerleman]

Please notice that even in yoga, there are dozens of these directives, also to remove C++ includes. https://github.com/facebook/yoga/blob/c18e2566a0718384869c798e6b6ebebc3b30087a/yoga/YGMacros.h#L10-L12

This is not quite correct. The ifdefsin Yoga are to provide additional safety or standardized function to C++ users of the C ABI. E.g. we use constexpr C++ values for quiet NaN, or define flag operators on bitset enums for C++ operator semantics.

I went through the effort of removing ifdefs Expo added to Yoga a while back in 33ebb5d

[Kudo]

IIRC, CocoaPods with custom module_map will break in use_frameworks mode. does yoga use the custom module_map for CocoaPods as well or SPM only?

[tsapeta]

@Kudo It looks like it's not used for CocoaPods (module_map spec property is not specified in Yoga.podspec), but these headers are instead specified as public, so the umbrella header imports only those ones (and descendants).
The problem with this solution that I see here is that RN would have to export much more headers as public, which I think increases the maintenance cost much more than ifdefs, at least for now because it could break many libraries.

[Kudo]

besides custom module_map, i feel like the swift build errors happen many times from the past. @tsapeta has an idea to create a pr and add a swift file to rn-tester, so that we could find swift integration error as early as possible.
do you folks feel whether it s a good idea to add the swift file to rn-tester also in this pr? then it will be the best test plan to verify this pr's change.

[NickGerleman]

@Kudo It looks like it's not used for CocoaPods (module_map spec property is not specified in Yoga.podspec), but these headers are instead specified as public, so the umbrella header imports only those ones (and descendants).
The problem with this solution that I see here is that RN would have to export much more headers as public, which I think increases the maintenance cost much more than ifdefs, at least for now because it could break many libraries.

In Cocoapods for Yoga, we include a small set of public headers which forms the transitive closure of importing “<yoga/Yoga.h>”. It is fairly minimal, and C compatible. So, modular headers get this smaller set of headers.

Fabric C++ has been importing private C++ implementation details from Yoga. E.g. YGStyle I’ve been working towards RN only using public Yoga APIs, and cleaning up the directory structure, but in the meantime, the C++ build for Fabric has private Yoga headers added to its header search path for C++ users.

In Yoga there is an intentional header structure for the C API, that avoids importing any C++ headers by using a PIMPL-like pattern (all refs in the headers are opaque pointers).

If we were designing an explicit C/ObjC extension point for these RN bits, what would it look like? And then how to we create a thin header graph that can keep explicitly being C++ free.

[Kudo]

If we were designing an explicit C/ObjC extension point for these RN bits, what would it look like? And then how to we create a thin header graph that can keep explicitly being C++ free.

that would be huge effort for RN but would benefit ABI safety. the scope is far more than this pr.

[NickGerleman]

What headers are you using from Swift, where these C++ headers got added to your include chain?

[Kudo]

since swift doesn't support importing a c/c++ header file, we could just import React in swift. this is to import the React-Core clang module and all headers in the generated React-Core-umbrella.h will be included. an example of React-Core-umbrella.h from react-native 0.72 is like this one

[NickGerleman]

The React-core clang module has quite a bit of surface that seems like it would be unneeded for Swift modules.

That seems like space for a solution, where we split out a module with smaller surface that we can keep explicitly ObjC headers only, and explicitly able to be imported by Swift.

I’m also curious, if the React Core module was previously all ObjC headers, what change introduced the new C++ headers? That might be a sign of organization we can improve.

[cipolleschi]

closing as it has been superseded by #38993

[Kudo]

I’m also curious, if the React Core module was previously all ObjC headers, what change introduced the new C++ headers? That might be a sign of organization we can improve.

that is a great question. i took some time to check the difference between 0.72 and the git history, 42d67452eb9a#diff-226ff5f87f146abfebd14a69eeb7d95c358d53da30533321e3ae9281c8acc6f0L102 could be the root cause.

i'm creating #38993 as a replacement for this pr.