Add a warning for cases in which SwiftPM generates a module map for u…

…nsupported header layouts

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 for 5.3 is to keep the current behavior but emit a warning asking package authors to add a custom module map.  We also 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.

Suggesting to use a custom module map for the cases in which the package target doesn't conform to the documented layout seems like the right thing even for packages that currently happen to work.

rdar://65692136

Documentation/Usage.md

@@ -724,9 +724,10 @@ 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.
+  include directory, and the include directory contains no header files, then
+  `include/Foo/Foo.h` becomes the umbrella header.
 
-* If `include/Foo.h` exists and `include` contains no other subdirectory then
+* 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

Fixtures/CFamilyTargets/ModuleMapGenerationCases/Package.swift

@@ -6,15 +6,18 @@ let package = Package(
     targets: [
 		.target(
             name: "Baz",
-            dependencies: ["FlatInclude", "UmbrellaHeader", "UmbellaModuleNameInclude", "UmbrellaHeaderFlat"]),
+            dependencies: ["FlatInclude", "NonModuleDirectoryInclude", "UmbrellaHeader", "UmbrellaDirectoryInclude", "UmbrellaHeaderFlat"]),
 		.target(
             name: "FlatInclude",
             dependencies: []),
 		.target(
             name: "NoIncludeDir",
             dependencies: []),
 		.target(
-            name: "UmbellaModuleNameInclude",
+            name: "NonModuleDirectoryInclude",
+            dependencies: []),
+		.target(
+            name: "UmbrellaDirectoryInclude",
             dependencies: []),
 		.target(
             name: "UmbrellaHeader",

Fixtures/CFamilyTargets/ModuleMapGenerationCases/Sources/Baz/main.swift

@@ -1,4 +1,4 @@
-import UmbellaModuleNameInclude
+import UmbrellaDirectoryInclude
 import FlatInclude
 import UmbrellaHeader
 import UmbrellaHeaderFlat

Fixtures/CFamilyTargets/ModuleMapGenerationCases/Sources/NonModuleDirectoryInclude/Maz.c

@@ -0,0 +1,8 @@
+#include "NonModuleDirectoryInclude/Maz.h"
+
+int maz() {
+    int a = 6;
+    int b = a;
+    a = b;
+    return a;
+}

Fixtures/CFamilyTargets/ModuleMapGenerationCases/Sources/UmbellaModuleNameInclude/Jaz.c → Fixtures/CFamilyTargets/ModuleMapGenerationCases/Sources/UmbrellaDirectoryInclude/Jaz.c

@@ -1,4 +1,4 @@
-#include "UmbellaModuleNameInclude/Paz.h"
+#include "Paz.h"
 
 int jaz() {
     int a = 6;

Fixtures/CFamilyTargets/ModuleMapGenerationCases/Sources/UmbrellaDirectoryInclude/include/Paz.h

@@ -0,0 +1 @@
+int jaz();

Sources/PackageLoading/ModuleMapGenerator.swift

@@ -1,7 +1,7 @@
 /*
  This source file is part of the Swift.org open source project
  
- Copyright (c) 2014 - 2017 Apple Inc. and the Swift project authors
+ Copyright (c) 2014 - 2020 Apple Inc. and the Swift project authors
  Licensed under Apache License v2.0 with Runtime Library Exception
  
  See http://swift.org/LICENSE.txt for license information
@@ -44,11 +44,11 @@ extension ClangTarget: ModuleMapProtocol {
 ///
 /// Modulemap is generated under the following rules provided it is not already present in include directory:
 ///
-/// * "include/foo/foo.h" exists and `foo` is the only directory under include directory.
+/// * "include/foo/foo.h" exists and `foo` is the only directory under the "include" directory, and the "include" directory contains no header files:
 ///    Generates: `umbrella header "/path/to/include/foo/foo.h"`
-/// * "include/foo.h" exists and include contains no other directory.
+/// * "include/foo.h" exists and "include" contains no other subdirectory:
 ///    Generates: `umbrella header "/path/to/include/foo.h"`
-/// *  Otherwise in all other cases.
+/// *  Otherwise, if the "include" directory only contains header files and no other subdirectory:
 ///    Generates: `umbrella "path/to/include"`
 public struct ModuleMapGenerator {
 
@@ -81,8 +81,7 @@ public struct ModuleMapGenerator {
         }
     }
 
-    /// Create the synthesized modulemap, if necessary.
-    /// Note: modulemap is not generated for test targets.
+    /// Generates a modulemap based on the layout of the target's public headers.  This is only valid for library targets.
     public mutating func generateModuleMap(inDir wd: AbsolutePath) throws {
         assert(target.type == .library)
 
@@ -105,8 +104,10 @@ public struct ModuleMapGenerator {
         let files = walked.filter({ fileSystem.isFile($0) && $0.suffix == ".h" })
         let dirs = walked.filter({ fileSystem.isDirectory($0) })
 
+        // If 'include/ModuleName.h' exists, then use it as the umbrella header (this is case 2 at https://github.com/apple/swift-package-manager/blob/master/Documentation/Usage.md#creating-c-language-targets).
         let umbrellaHeaderFlat = includeDir.appending(component: target.c99name + ".h")
         if fileSystem.isFile(umbrellaHeaderFlat) {
+            // In this case, 'include' is expected to contain no subdirectories.
             guard dirs.isEmpty else {
                 throw ModuleMapError.unsupportedIncludeLayoutForModule(
                     target.name,
@@ -117,8 +118,10 @@ public struct ModuleMapGenerator {
         }
         diagnoseInvalidUmbrellaHeader(includeDir)
 
+        // If 'include/ModuleName/ModuleName.h' exists, then use it as the umbrella header (this is case 1 at Documentation/Usage.md#creating-c-language-targets).
         let umbrellaHeader = includeDir.appending(components: target.c99name, target.c99name + ".h")
         if fileSystem.isFile(umbrellaHeader) {
+            // In this case, 'include' is expected to contain no subdirectories other than 'ModuleName', and no header files.
             guard dirs.count == 1 else {
                 throw ModuleMapError.unsupportedIncludeLayoutForModule(
                     target.name,
@@ -134,6 +137,17 @@ public struct ModuleMapGenerator {
         }
         diagnoseInvalidUmbrellaHeader(includeDir.appending(component: target.c99name))
 
+        // Otherwise, if 'include' contains only header files and no subdirectories, use it as the umbrella directory (this is case 3 at https://github.com/apple/swift-package-manager/blob/master/Documentation/Usage.md#creating-c-language-targets).
+        if files.count == walked.count {
+            try createModuleMap(inDir: wd, type: .directory(includeDir))
+            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).
+        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.flush()
         try createModuleMap(inDir: wd, type: .directory(includeDir))
     }
 

Tests/PackageLoadingTests/ModuleMapGenerationTests.swift

@@ -1,7 +1,7 @@
 /*
  This source file is part of the Swift.org open source project
 
- Copyright (c) 2014 - 2017 Apple Inc. and the Swift project authors
+ Copyright (c) 2014 - 2020 Apple Inc. and the Swift project authors
  Licensed under Apache License v2.0 with Runtime Library Exception
 
  See http://swift.org/LICENSE.txt for license information
@@ -77,13 +77,6 @@ class ModuleMapGeneration: XCTestCase {
             "/Foo.c")
         checkExpected()
 
-        // FIXME: Should this be allowed?
-        fs = InMemoryFileSystem(emptyFiles:
-            "/include/Baz/Foo.h",
-            "/include/Bar/Bar.h",
-            "/Foo.c")
-        checkExpected()
-
         fs = InMemoryFileSystem(emptyFiles:
             "/include/Baz.h",
             "/include/Bar.h",
@@ -111,6 +104,20 @@ class ModuleMapGeneration: XCTestCase {
             result.check(value: expected.bytes)
             result.checkDiagnostics("warning: /include/F-o-o.h should be renamed to /include/F_o_o.h to be used as an umbrella header")
         }
+
+        fs = InMemoryFileSystem(emptyFiles:
+            "/include/Baz/Foo.h",
+            "/include/Bar/Bar.h",
+            "/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")
+        }
     }
 
     func testUnsupportedLayouts() throws {