Warning for framework headers using double quote includes

Introduce -Wquoted-include-in-framework-header, which should fire a warning
whenever a quote include appears in a framework header and suggest a fix-it.
For instance, for header A.h added in the tests, this is how the warning looks
like:

./A.framework/Headers/A.h:2:10: warning: double-quoted include "A0.h" in framework header, expected angle-bracketed instead [-Wquoted-include-in-framework-header]
#include "A0.h"
         ^~~~~~
         <A/A0.h>
./A.framework/Headers/A.h:3:10: warning: double-quoted include "B.h" in framework header, expected angle-bracketed instead [-Wquoted-include-in-framework-header]
#include "B.h"
         ^~~~~
         <B.h>

This helps users to prevent frameworks from using local headers when in fact
they should be targetting system level ones.

The warning is off by default.

Differential Revision: https://reviews.llvm.org/D47157

rdar://problem/37077034

llvm-svn: 335184

clang/include/clang/Basic/DiagnosticGroups.td

@@ -31,6 +31,7 @@ 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 CXX14BinaryLiteral : DiagGroup<"c++14-binary-literal">;
 def CXXPre14CompatBinaryLiteral : DiagGroup<"c++98-c++11-compat-binary-literal">;
 def GNUBinaryLiteral : DiagGroup<"gnu-binary-literal">;

clang/include/clang/Basic/DiagnosticLexKinds.td

@@ -714,6 +714,11 @@ def warn_mmap_redundant_export_as : Warning<
 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_auto_module_import : Warning<
   "treating #%select{include|import|include_next|__include_macros}0 as an "
   "import of module '%1'">, InGroup<AutoImport>, DefaultIgnore;

clang/lib/Lex/HeaderSearch.cpp

@@ -621,6 +621,59 @@ static const char *copyString(StringRef Str, llvm::BumpPtrAllocator &Alloc) {
   return CopyStr;
 }
 
+static bool isFrameworkStylePath(StringRef Path,
+                                 SmallVectorImpl<char> &FrameworkName) {
+  using namespace llvm::sys;
+  path::const_iterator I = path::begin(Path);
+  path::const_iterator E = path::end(Path);
+
+  // 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->endswith(".framework")) {
+      FrameworkName.append(I->begin(), I->end());
+      ++FoundComp;
+    }
+    if (*I == "Headers" || *I == "PrivateHeaders")
+      ++FoundComp;
+    ++I;
+  }
+
+  return FoundComp >= 2;
+}
+
+static void
+diagnoseFrameworkInclude(DiagnosticsEngine &Diags, SourceLocation IncludeLoc,
+                         StringRef Includer, StringRef IncludeFilename,
+                         const FileEntry *IncludeFE, bool isAngled = false,
+                         bool FoundByHeaderMap = false) {
+  SmallString<128> FromFramework, ToFramework;
+  if (!isFrameworkStylePath(Includer, FromFramework))
+    return;
+  bool IsIncludeeInFramework =
+      isFrameworkStylePath(IncludeFE->getName(), ToFramework);
+
+  if (!isAngled && !FoundByHeaderMap) {
+    SmallString<128> NewInclude("<");
+    if (IsIncludeeInFramework) {
+      NewInclude += StringRef(ToFramework).drop_back(10); // drop .framework
+      NewInclude += "/";
+    }
+    NewInclude += IncludeFilename;
+    NewInclude += ">";
+    Diags.Report(IncludeLoc, diag::warn_quoted_include_in_framework_header)
+        << IncludeFilename
+        << FixItHint::CreateReplacement(IncludeLoc, NewInclude);
+  }
+}
+
 /// 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
@@ -722,8 +775,12 @@ const FileEntry *HeaderSearch::LookupFile(
           RelativePath->clear();
           RelativePath->append(Filename.begin(), Filename.end());
         }
-        if (First)
+        if (First) {
+          diagnoseFrameworkInclude(Diags, IncludeLoc,
+                                   IncluderAndDir.second->getName(), Filename,
+                                   FE);
           return FE;
+        }
 
         // Otherwise, we found the path via MSVC header search rules.  If
         // -Wmsvc-include is enabled, we have to keep searching to see if we
@@ -834,6 +891,12 @@ const FileEntry *HeaderSearch::LookupFile(
       return MSFE;
     }
 
+    bool FoundByHeaderMap = !IsMapped ? false : *IsMapped;
+    if (!Includers.empty())
+      diagnoseFrameworkInclude(Diags, IncludeLoc,
+                               Includers.front().second->getName(), Filename,
+                               FE, isAngled, FoundByHeaderMap);
+
     // Remember this location for the next lookup we do.
     CacheLookup.HitIdx = i;
     return FE;

clang/test/Modules/Inputs/double-quotes/A.framework/Headers/A.h

@@ -0,0 +1,6 @@
+#include "A0.h"
+#include "B.h"
+
+#include "X.h"
+
+int foo();

clang/test/Modules/Inputs/double-quotes/A.framework/Headers/A0.h

@@ -0,0 +1 @@
+// double-quotes/A.framework/Headers/A0.h

clang/test/Modules/Inputs/double-quotes/A.framework/Modules/module.modulemap

@@ -0,0 +1,5 @@
+// double-quotes/A.framework/Modules/module.modulemap
+framework module A {
+  header "A.h"
+  header "A0.h"
+}

clang/test/Modules/Inputs/double-quotes/B.h

@@ -0,0 +1 @@
+// double-quotes/B.h

clang/test/Modules/Inputs/double-quotes/X.framework/Headers/X.h

@@ -0,0 +1 @@
+// double-quotes/X.framework/Headers/X.h

clang/test/Modules/Inputs/double-quotes/X.framework/Modules/module.modulemap

@@ -0,0 +1,4 @@
+// double-quotes/X.framework/Modules/module.modulemap
+framework module X {
+  header "X.h"
+}

clang/test/Modules/Inputs/double-quotes/a.hmap.json

@@ -0,0 +1,6 @@
+{
+  "mappings" :
+    {
+     "A.h" : "A/A.h"
+    }
+}

clang/test/Modules/Inputs/double-quotes/flat-header-path/Z.h

@@ -0,0 +1 @@
+#import "B.h" // Included from Z.h & A.h

clang/test/Modules/Inputs/double-quotes/flat-header-path/Z.modulemap

@@ -0,0 +1,4 @@
+// double-quotes/flat-header-path/Z.modulemap
+framework module Z {
+  header "Z.h"
+}

clang/test/Modules/Inputs/double-quotes/x.hmap.json

@@ -0,0 +1,7 @@
+
+{
+  "mappings" :
+    {
+     "X.h" : "X/X.h"
+    }
+}

clang/test/Modules/Inputs/double-quotes/z.yaml

@@ -0,0 +1,28 @@
+{
+  'version': 0,
+  'case-sensitive': '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/Modules",
+      'contents': [
+        {
+          'type': 'file',
+          'name': "module.modulemap",
+          'external-contents': "TEST_DIR/flat-header-path/Z.modulemap"
+        }
+      ]
+    }
+  ]
+}

clang/test/Modules/double-quotes.m

@@ -0,0 +1,39 @@
+// REQUIRES: shell
+
+// RUN: rm -rf %t
+// RUN: mkdir %t
+
+// RUN: hmaptool write %S/Inputs/double-quotes/a.hmap.json %t/a.hmap
+// RUN: hmaptool write %S/Inputs/double-quotes/x.hmap.json %t/x.hmap
+
+// RUN: sed -e "s:TEST_DIR:%S/Inputs/double-quotes:g" \
+// RUN:   %S/Inputs/double-quotes/z.yaml > %t/z.yaml
+
+// The output with and without modules should be the same
+
+// RUN: %clang_cc1 \
+// RUN:   -I %t/x.hmap -iquote %t/a.hmap -ivfsoverlay %t/z.yaml \
+// RUN:   -F%S/Inputs/double-quotes -I%S/Inputs/double-quotes \
+// RUN:   -Wquoted-include-in-framework-header -fsyntax-only %s -verify
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache \
+// RUN:   -I %t/x.hmap -iquote %t/a.hmap -ivfsoverlay %t/z.yaml \
+// RUN:   -F%S/Inputs/double-quotes -I%S/Inputs/double-quotes \
+// RUN:   -Wquoted-include-in-framework-header -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: double-quoted include "A0.h" in framework header, expected angle-bracketed instead
+// CHECK: double-quoted include "B.h" in framework header, expected angle-bracketed instead
+// CHECK: double-quoted include "B.h" in framework header, expected angle-bracketed instead
+
+#import "A.h"
+#import <Z/Z.h>
+
+int bar() { return foo(); }
+
+// expected-warning@Inputs/double-quotes/A.framework/Headers/A.h:1{{double-quoted include "A0.h" in framework header, expected angle-bracketed instead}}
+// expected-warning@Inputs/double-quotes/A.framework/Headers/A.h:2{{double-quoted include "B.h" in framework header, expected angle-bracketed instead}}
+// expected-warning@Inputs/double-quotes/flat-header-path/Z.h:1{{double-quoted include "B.h" in framework header, expected angle-bracketed instead}}