Warning for framework include violation from Headers to PrivateHeaders

[bruno]

Summary

Framework vendors usually layout their framework headers in the following way:

Foo.framework/Headers -> "public" headers
Foo.framework/PrivateHeader -> "private" headers

Since both headers in both directories can be found with #import <Foo/some-header.h>, it's easy to make mistakes and include headers in Foo.framework/PrivateHeader from headers in Foo.framework/Headers, which usually configures a layering violation on Darwin ecosystems. One of the problem this causes is dep cycles when modules are used, since it's very common for "private" modules to include from the "public" ones; adding an edge the other way around will trigger cycles.

Add a warning to catch those cases such that:

In file included from test.m:1:
./A.framework/Headers/A.h:2:10: warning: public framework header includes private framework header 'A/APriv.h' [-Wframework-include-private-from-public]

^

This only works for the layering violation within a framework.

rdar://problem/38712182

Depends on D47157

[bruno]

Update to a more recent version of the patch.

[vsapsai]

include/clang/Basic/DiagnosticGroups.td

34–37

It might be convenient for users to have a warning group that will cover different framework warnings, something like -Wframework-hygiene. But it's out of scope for this review, more as an idea for future improvements.

lib/Lex/HeaderSearch.cpp

625

In this function we accept some paths that aren't valid framework paths. Need to think more if it is OK or if we want to be stricter.

920

s/api/API/

922

My opinion on readability is personal, so take it with a grain of salt. What if you make variable names more consistent, like

• IsIncluderPrivateHeader
• IsIncluderFromFramework
• IsIncludeePrivateHeader

test/Modules/framework-public-includes-private.m

34

I'm not entirely sure it's not covered by existing test. It might be worth testing private header including public header and private header including another private header.

[bruno]

Update patch after changes to D47157. Also address some of Volodymyr feedback.

[bruno]

include/clang/Basic/DiagnosticGroups.td

34–37

Yep, that should come once we land all other bits.

lib/Lex/HeaderSearch.cpp

625

It should be ok at this point, otherwise the framework style path would have failed before finding this header.

test/Modules/framework-public-includes-private.m

34

The warning is on by default and clang already have the scenario you described in other module tests - those would fail if there's a bug in the logic here.

[vsapsai]

lib/Lex/HeaderSearch.cpp

683

Please capitalize "api". The rest looks good.

[bruno]

Address Volodymyr's comments

[bruno]

Ping!

[vsapsai]

Looks good to me. The only problem is I'm not entirely sure this warning will interact with real code the way I expect it to. We'll need to keep an eye on it and tweak if necessary.

[bruno]

Committed r335542

include/clang/Basic/DiagnosticGroups.td

 def GNUAnonymousStruct : DiagGroup<"gnu-anonymous-struct">;
 def GNUAutoType : DiagGroup<"gnu-auto-type">;
 def ArrayBounds : DiagGroup<"array-bounds">;
 def ArrayBoundsPointerArithmetic : DiagGroup<"array-bounds-pointer-arithmetic">;
 def AutoDisableVptrSanitizer : DiagGroup<"auto-disable-vptr-sanitizer">;
 def Availability : DiagGroup<"availability">;
 def Section : DiagGroup<"section">;
 def AutoImport : DiagGroup<"auto-import">;
 def FrameworkHdrQuotedInclude : DiagGroup<"quoted-include-in-framework-header">;
+def FrameworkIncludePrivateFromPublic :
+  DiagGroup<"framework-include-private-from-public">;
 def CXX14BinaryLiteral : DiagGroup<"c++14-binary-literal">;
 def CXXPre14CompatBinaryLiteral : DiagGroup<"c++98-c++11-compat-binary-literal">;
 def GNUBinaryLiteral : DiagGroup<"gnu-binary-literal">;
 def BinaryLiteral : DiagGroup<"binary-literal", [CXX14BinaryLiteral,
                                                  CXXPre14CompatBinaryLiteral,
                                                  GNUBinaryLiteral]>;
 def GNUCompoundLiteralInitializer : DiagGroup<"gnu-compound-literal-initializer">;
 def BitFieldConstantConversion : DiagGroup<"bitfield-constant-conversion">;
 def BitFieldEnumConversion : DiagGroup<"bitfield-enum-conversion">;

include/clang/Basic/DiagnosticLexKinds.td

def warn_mmap_redundant_export_as : Warning<
   InGroup<PrivateModule>;
 def err_mmap_submodule_export_as : Error<
   "only top-level modules can be re-exported as public">;
 
 def warn_quoted_include_in_framework_header : Warning<
   "double-quoted include \"%0\" in framework header, "
   "expected angle-bracketed instead"
   >, InGroup<FrameworkHdrQuotedInclude>, DefaultIgnore;
+def warn_framework_include_private_from_public : Warning<
+  "public framework header includes private framework header '%0'"
+  >, InGroup<FrameworkIncludePrivateFromPublic>;
 
 def warn_auto_module_import : Warning<
   "treating #%select{include|import|include_next|__include_macros}0 as an "
   "import of module '%1'">, InGroup<AutoImport>, DefaultIgnore;
 def note_implicit_top_level_module_import_here : Note<
   "submodule of top-level module '%0' implicitly imported here">;
 def warn_uncovered_module_header : Warning<
   "umbrella header for module '%0' does not include header '%1'">, 

lib/Lex/HeaderSearch.cpp

 static const char *copyString(StringRef Str, llvm::BumpPtrAllocator &Alloc) {
   assert(!Str.empty());
   char *CopyStr = Alloc.Allocate<char>(Str.size()+1);
   std::copy(Str.begin(), Str.end(), CopyStr);
   CopyStr[Str.size()] = '\0';
   return CopyStr;
 }
 
-static bool isFrameworkStylePath(StringRef Path,
+static bool isFrameworkStylePath(StringRef Path, bool &IsPrivateHeader,
                                  SmallVectorImpl<char> &FrameworkName) {
   using namespace llvm::sys;
   path::const_iterator I = path::begin(Path);
   path::const_iterator E = path::end(Path);
+  IsPrivateHeader = false;
 
   // Detect different types of framework style paths:
   //
   //   ...Foo.framework/{Headers,PrivateHeaders}
   //   ...Foo.framework/Versions/{A,Current}/{Headers,PrivateHeaders}
   //   ...Foo.framework/Frameworks/Nested.framework/{Headers,PrivateHeaders}
   //   ...<other variations with 'Versions' like in the above path>
   //
   // and some other variations among these lines.
   int FoundComp = 0;
   while (I != E) {
+    if (*I == "Headers")
+      ++FoundComp;
     if (I->endswith(".framework")) {
       FrameworkName.append(I->begin(), I->end());
       ++FoundComp;
     }
-    if (*I == "Headers" || *I == "PrivateHeaders")
+    if (*I == "PrivateHeaders") {
       ++FoundComp;
+      IsPrivateHeader = true;
+    }
     ++I;
   }
 
   return FoundComp >= 2;
 }
 
 static void
 diagnoseFrameworkInclude(DiagnosticsEngine &Diags, SourceLocation IncludeLoc,
if (IsIncludeeInFramework) {
       NewInclude += "/";
     }
     NewInclude += IncludeFilename;
     NewInclude += ">";
     Diags.Report(IncludeLoc, diag::warn_quoted_include_in_framework_header)
         << IncludeFilename
         << FixItHint::CreateReplacement(IncludeLoc, NewInclude);
   }
+
+  // Headers in Foo.framework/Headers should not include headers
+  // from Foo.framework/PrivateHeaders, since this violates public/private
+  // api boundaries and can cause modular dependency cycles.
+  if (!IsIncluderPrivateHeader && IsIncludeeInFramework &&
+      IsIncludeePrivateHeader && FromFramework == ToFramework)
+    Diags.Report(IncludeLoc, diag::warn_framework_include_private_from_public)
+        << IncludeFilename;
 }
 
 /// LookupFile - Given a "foo" or \<foo> reference, look up the indicated file,
 /// return null on failure.  isAngled indicates whether the file reference is
 /// for system \#include's or not (i.e. using <> instead of ""). Includers, if
 /// non-empty, indicates where the \#including file(s) are, in case a relative
 /// search is needed. Microsoft mode will pass all \#including files.
 const FileEntry *HeaderSearch::LookupFile(
for (; i != SearchDirs.size(); ++i) {
 
     // Remember this location for the next lookup we do.
     CacheLookup.HitIdx = i;
     return FE;
   }
 
   // If we are including a file with a quoted include "foo.h" from inside
   // a header in a framework that is currently being built, and we couldn't
   // resolve "foo.h" any other way, change the include to <Foo/foo.h>, where
   // "Foo" is the name of the framework in which the including header was found.
   if (!Includers.empty() && Includers.front().first && !isAngled &&
       Filename.find('/') == StringRef::npos) {
     HeaderFileInfo &IncludingHFI = getFileInfo(Includers.front().first);
     if (IncludingHFI.IndexHeaderMapHeader) {
       SmallString<128> ScratchFilename;
       ScratchFilename += IncludingHFI.Framework;
       ScratchFilename += '/';
       ScratchFilename += Filename;
 

test/Modules/Inputs/framework-public-includes-private/A.framework/Headers/A.h

+
+#include <A/APriv.h>
+#include "APriv2.h"
+#include <Z/Z.h>
+int foo();

test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.modulemap

+
+framework module A {
+  header "A.h"
+}

test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.private.modulemap

+framework module A_Private {
+  header "APriv.h"
+}

test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv.h

+// APriv.h

test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv2.h

+// APriv.h

test/Modules/Inputs/framework-public-includes-private/a.hmap.json

+
+{
+  "mappings" :
+    {
+     "A.h" : "A/A.h",
+     "APriv2.h" : "A/APriv2.h"
+    }
+}

test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.h

+#import "ZPriv.h"

test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.modulemap

+framework module Z {
+  header "Z.h"
+}

test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.private.modulemap

+framework module Z_Private {
+  header "ZPriv.h"
+}

test/Modules/Inputs/framework-public-includes-private/flat-header-path/ZPriv.h

+// ZPriv.h

test/Modules/Inputs/framework-public-includes-private/z.hmap.json

+
+{
+  "mappings" :
+    {
+     "ZPriv.h" : "Z/ZPriv.h"
+    }
+}

test/Modules/Inputs/framework-public-includes-private/z.yaml

+{
+  'version': 0,
+  'case-sensitive': 'false',
+  'use-external-names' : 'false',
+  'roots': [
+    {
+      'type': 'directory',
+      'name': "TEST_DIR/Z.framework/Headers",
+      'contents': [
+        {
+          'type': 'file',
+          'name': "Z.h",
+          'external-contents': "TEST_DIR/flat-header-path/Z.h"
+        }
+      ]
+    },
+    {
+      'type': 'directory',
+      'name': "TEST_DIR/Z.framework/PrivateHeaders",
+      'contents': [
+        {
+          'type': 'file',
+          'name': "ZPriv.h",
+          'external-contents': "TEST_DIR/flat-header-path/ZPriv.h"
+        }
+      ]
+    },
+    {
+      'type': 'directory',
+      'name': "TEST_DIR/Z.framework/Modules",
+      'contents': [
+        {
+          'type': 'file',
+          'name': "module.modulemap",
+          'external-contents': "TEST_DIR/flat-header-path/Z.modulemap"
+        },
+        {
+          'type': 'file',
+          'name': "module.private.modulemap",
+          'external-contents': "TEST_DIR/flat-header-path/Z.private.modulemap"
+        }
+      ]
+    }
+  ]
+}

test/Modules/framework-public-includes-private.m

+// REQUIRES: shell
+
+// RUN: rm -rf %t
+// RUN: mkdir %t
+
+// RUN: hmaptool write %S/Inputs/framework-public-includes-private/a.hmap.json %t/a.hmap
+// RUN: hmaptool write %S/Inputs/framework-public-includes-private/z.hmap.json %t/z.hmap
+
+// RUN: sed -e "s:TEST_DIR:%S/Inputs/framework-public-includes-private:g" \
+// RUN:   %S/Inputs/framework-public-includes-private/z.yaml > %t/z.yaml
+
+// The output with and without modules should be the same, without modules first.
+// RUN: %clang_cc1 \
+// RUN:   -iquote %t/z.hmap -iquote %t/a.hmap -ivfsoverlay %t/z.yaml \
+// RUN:   -F%S/Inputs/framework-public-includes-private \
+// RUN:   -fsyntax-only %s -verify
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache \
+// RUN:   -iquote %t/z.hmap -iquote %t/a.hmap -ivfsoverlay %t/z.yaml \
+// RUN:   -F%S/Inputs/framework-public-includes-private \
+// RUN:   -fsyntax-only %s \
+// RUN:   2>%t/stderr
+
+// The same warnings show up when modules is on but -verify doesn't get it
+// because they only show up under the module A building context.
+// RUN: FileCheck --input-file=%t/stderr %s
+// CHECK: public framework header includes private framework header 'A/APriv.h'
+// CHECK: public framework header includes private framework header 'A/APriv2.h'
+// CHECK: public framework header includes private framework header 'Z/ZPriv.h'
+
+#import "A.h"
+
+int bar() { return foo(); }
+
+// expected-warning@Inputs/framework-public-includes-private/A.framework/Headers/A.h:2{{public framework header includes private framework header 'A/APriv.h'}}
+// expected-warning@Inputs/framework-public-includes-private/A.framework/Headers/A.h:3{{public framework header includes private framework header 'A/APriv2.h'}}
+// expected-warning@Inputs/framework-public-includes-private/flat-header-path/Z.h:1{{public framework header includes private framework header 'Z/ZPriv.h'}}