<rdar://63848226> [SR-12758] C wrapper library requires custom module map in Swift 5.2

The problem here is actually that the logic in SwiftPM always creates a module map with either an umbrella header or an umbrella directory for every C target, even though the documentation says that a module map will only be created for a C target under particular conditions.  According to https://github.com/apple/swift-package-manager/blob/master/Documentation/Usage.md#creating-c-language-targets, here are the rules:

> Swift Package Manager will automatically generate a modulemap for each
> C language library target for these 3 cases:
>
> • If “include/Foo/Foo.h” exists and “Foo” is the only directory under the
> “include” directory, then “include/Foo/Foo.h” becomes the umbrella header.
>
> • If “include/Foo.h” exists and “include” contains no other subdirectory then
> “include/Foo.h” becomes the umbrella header.
>
> • Otherwise, if the “include” directory only contains header files and no
> other subdirectory, it becomes the umbrella directory.
>
> In case of complicated include layouts, a custom module.modulemap can be
> provided inside “include”.  SwiftPM will error out if it can not generate
> a modulemap with respect to the above rules.

But the problem is that the third of those bullet points is not how the logic works.  Any header layout that doesn't match one of the first two cases automatically became an umbrella directory in the generated module map.

This bug was masked because module map files weren’t passed from one C target to another until https://bugs.swift.org/browse/SR-10707 was fixed, at which point this started breaking.  For C targets whose module maps were passed to Swift targets, it was always broken (but only surfaced as errors from the compiler as a result of trying to build a module from the non-modularized headers, not surfaced as errors from SwiftPM about an incorrect header layout).

We don’t want to undo the fix for SR-10707, nor is it right to tie this behavior to particular Swift tools versions, since that only postpones the problem.  It’s perfectly legitimate to want to wrap an unmodified C or C++ library (with non-modularized header layout) in an authored C wrapper target that then presents a Swift-friendly modular interface to Swift targets, so non-modular C-to-C target imports should continue to be supported.  The code also alludes to this, at BuildPlan.swift:409 in current main branch:

> // FIXME: We should probably only warn if we're unable to generate the
> // modulemap because the clang target is still a valid, it just can't be
> // imported from Swift targets.

This is above the line that does a `try` that ends up passing on any errors if there’s a problem with the layout.

After having gone through the code, and discussing this in some detail, and trying a couple of different approaches, I think the right thing to do is:

1.  Fix the logic to only create an umbrella directory module map in the cases that are spelled out in the third bullet of the documentation (i.e. match the documented behavior).

2.  But emit a warning, not an error, suggesting that the target add a custom module map.  If we emitted an error as the documentation said, we would break a lot of packages.

3.  Update the documentation to account for one other case that’s already being checked in the logic for case 1, which is that there are no headers next to the umbrella directory.

Instead of not generating a module map at all, we generate an empty module map (along with emitting the warning) in the cases in which an incorrect umbrella directory would previously have been generated into the module map.  This has the least impact on the downstream logic.

Author: abertelrud

Sources/PackageLoading/ModuleMapGenerator.swift

@@ -143,12 +143,12 @@ public struct ModuleMapGenerator {
             return
         }
 
-        // Otherwise, the target's public headers are considered to be incompatible with modules.  Other C targets can still import them, but Swift won't be able to see them.  This is documented as an error, but because SwiftPM has previously allowed it (creating module maps that then cause errors when used), we instead emit a warning and for now, continue to emit what SwiftPM has historically emitted (an umbrella directory include).
+        // Otherwise, the target's public headers are considered to be incompatible with modules.  Other C targets can still import them, but Swift won't be able to see them.  This is documented as an error, but because SwiftPM has previously allowed it (creating module maps that then cause errors when used), we instead emit a warning.
         warningStream <<< "warning: the include directory of target '\(target.name)' has "
-        warningStream <<< "a layout that is incompatible with modules; consider adding a "
-        warningStream <<< "custom module map to the target"
+        warningStream <<< "a layout that is incompatible with modules; no module map will "
+        warningStream <<< "be generated; consider adding a custom module map to the target"
         warningStream.flush()
-        try createModuleMap(inDir: wd, type: .directory(includeDir))
+        try createModuleMap(inDir: wd, type: .empty)
     }
 
     /// Warn user if in case target name and c99name are different and there is a
@@ -164,6 +164,7 @@ public struct ModuleMapGenerator {
     }
 
     private enum UmbrellaType {
+        case empty
         case header(AbsolutePath)
         case directory(AbsolutePath)
     }
@@ -172,6 +173,8 @@ public struct ModuleMapGenerator {
         let stream = BufferedOutputByteStream()
         stream <<< "module \(target.c99name) {\n"
         switch type {
+        case .empty:
+            break
         case .header(let header):
             stream <<< "    umbrella header \"\(header.pathString)\"\n"
         case .directory(let path):

Tests/PackageLoadingTests/ModuleMapGenerationTests.swift

@@ -111,12 +111,11 @@ class ModuleMapGeneration: XCTestCase {
             "/Foo.c")
         let expected2 = BufferedOutputByteStream()
         expected2 <<< "module Foo {\n"
-        expected2 <<< "    umbrella \"/include\"\n"
         expected2 <<< "    export *\n"
         expected2 <<< "}\n"
         ModuleMapTester("Foo", in: fs) { result in
             result.check(value: expected2.bytes)
-            result.checkDiagnostics("warning: the include directory of target \'Foo\' has a layout that is incompatible with modules; consider adding a custom module map to the target")
+            result.checkDiagnostics("warning: the include directory of target \'Foo\' has a layout that is incompatible with modules; no module map will be generated; consider adding a custom module map to the target")
         }
     }