Warning for framework headers using double quote includes

[bruno]

Summary

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.

rdar://problem/37077034

[bruno]

Depends on D46485

[Eugene.Zelenko]

See also PR22165.

[aaron.ballman]

The warning is off by default.

We typically do not add off-by-default warnings because experience has shown the rarely get enabled in practice. Can you explain a bit more about why this one is off by default?

[bruno]

See also PR22165.

Nice, seems related to this indeed. Are you aware of any development along those lines in clang-tidy? We would like this to be part of clang and be used as part of the normal compiler workflow, it can surely be as well part of any clang-tidy related include guidelines in the future.

The warning is off by default.

We typically do not add off-by-default warnings because experience has shown the rarely get enabled in practice. Can you explain a bit more about why this one is off by default?

Right. I believe this is going to be used in practice, the reason I'm adding it involves some user demand for such warning. Such quoted include use in frameworks happen often and we would like a smooth transition to happen here (e.g. do not initially affect -Werror users). If it proves worth it, we can migrate to on by default in the future. It wouldn't be a problem if we have it on by default on open source and disable by default downstream, but I rather be consistent.

[Eugene.Zelenko]

The warning is off by default.

We typically do not add off-by-default warnings because experience has shown the rarely get enabled in practice. Can you explain a bit more about why this one is off by default?

Right. I believe this is going to be used in practice, the reason I'm adding it involves some user demand for such warning. Such quoted include use in frameworks happen often and we would like a smooth transition to happen here (e.g. do not initially affect -Werror users). If it proves worth it, we can migrate to on by default in the future. It wouldn't be a problem if we have it on by default on open source and disable by default downstream, but I rather be consistent.

You could just not include new warning into -Wall and -Wextra. Those who will want to check #include statements correctness could enable it explicitly.

[bruno]

Hi Eugene,

You could just not include new warning into -Wall and -Wextra. Those who will want to check #include statements correctness could enable it explicitly.

This is exactly what's happening in the patch, the new warning isn't part of -Wall or -Wextra, and is marked DefaultIgnore, which means that will fire only when -Wquoted-include-in-framework-header is passed to the driver. Am I missing something from your explanation?

Thanks,

[Eugene.Zelenko]

In D47157#1110430, @bruno wrote:

Hi Eugene,

You could just not include new warning into -Wall and -Wextra. Those who will want to check #include statements correctness could enable it explicitly.

This is exactly what's happening in the patch, the new warning isn't part of -Wall or -Wextra, and is marked DefaultIgnore, which means that will fire only when -Wquoted-include-in-framework-header is passed to the driver. Am I missing something from your explanation?

Thanks,

Thank you for clarification! Sorry, I didn't know TableGen syntax well enough to deduce this from source.

[aaron.ballman]

In D47157#1107210, @bruno wrote:

See also PR22165.

Nice, seems related to this indeed. Are you aware of any development along those lines in clang-tidy? We would like this to be part of clang and be used as part of the normal compiler workflow, it can surely be as well part of any clang-tidy related include guidelines in the future.

The warning is off by default.

We typically do not add off-by-default warnings because experience has shown the rarely get enabled in practice. Can you explain a bit more about why this one is off by default?

Right. I believe this is going to be used in practice, the reason I'm adding it involves some user demand for such warning. Such quoted include use in frameworks happen often and we would like a smooth transition to happen here (e.g. do not initially affect -Werror users). If it proves worth it, we can migrate to on by default in the future. It wouldn't be a problem if we have it on by default on open source and disable by default downstream, but I rather be consistent.

Consistency would be nice, but at the same time, I don't see a good metric for when we'd know it's time to switch it to being on by default. I'm worried that it'll remain off by default forever simply because no one thinks to go turn it on (because it's silent by default). Perhaps on-by-default here and off-by-default downstream would be the better approach, or do you think this would be too disruptive to enable by default anywhere?

include/clang/Basic/DiagnosticLexKinds.td

713

80-col limit?

Also, I'd probably drop "system style" and reword slightly to:

"double-quoted include \"%0\" in framework header, expected angle-bracketed include <%0> instead"

lib/Lex/HeaderSearch.cpp

781–782

This seems like a good place for a fix-it to switch the include style. Is there a reason to not do that work for the user?

899–900

Similar here.

[bruno]

Consistency would be nice, but at the same time, I don't see a good metric for when we'd know it's time to switch it to being on by default. I'm worried that it'll remain off by default forever simply because no one thinks to go turn it on (because it's silent by default). Perhaps on-by-default here and off-by-default downstream would be the better approach, or do you think this would be too disruptive to enable by default anywhere?

I believe this could be too disruptive to enable now, since it's still very common to find quoted includes in framework headers. This is very important for Modules to properly work with frameworks (quoted headers are usually considered non-modular when modules builds kick in) and is actually very compelling for us to turn it on by default on Darwin as soon as we can, but we need to educate users first.

include/clang/Basic/DiagnosticLexKinds.td

713

Unfortunately this won't work because for framework style includes we use the angled-bracketed with the framework name. For example, if one wants to include Foo.h from Foo.framework, one should use #include <Foo/Foo.h>, although on disk you actually have Foo.framework/Headers/Foo.h. Framework header lookup does this magic and other similar ones.

Since we don't know which framework the quoted header could be part of, it was not included in the warning (doing so would require extra header searches - which could be expensive for this specific warning). However it seems that I can do better to indicate that the framework name is desired here, perhaps:

"double-quoted include \"%0\" in framework header, expected angle-bracketed include <FrameworkName/%0> instead"

How does that sound to you? Other suggestions?

lib/Lex/HeaderSearch.cpp

781–782

Like I explained above, we don't know which framework the header could be part of, so a fix-it could be misleading.

[dexonsmith]

lib/Lex/HeaderSearch.cpp

781–782

Clang supports editor placeholders, which we use in some refactoring-style fix-its. I think this would be spelled <#framework-name#>, or #include <<#framework-name#>/Foo.h>

[aaron.ballman]

In D47157#1115445, @bruno wrote:

Consistency would be nice, but at the same time, I don't see a good metric for when we'd know it's time to switch it to being on by default. I'm worried that it'll remain off by default forever simply because no one thinks to go turn it on (because it's silent by default). Perhaps on-by-default here and off-by-default downstream would be the better approach, or do you think this would be too disruptive to enable by default anywhere?

I believe this could be too disruptive to enable now, since it's still very common to find quoted includes in framework headers. This is very important for Modules to properly work with frameworks (quoted headers are usually considered non-modular when modules builds kick in) and is actually very compelling for us to turn it on by default on Darwin as soon as we can, but we need to educate users first.

That sounds like good justification for it being off by default then, thank you.

include/clang/Basic/DiagnosticLexKinds.td

713

Thank you for the explanation!

I think your suggested text sounds good, though I do wonder how expensive is "expensive" in finding the intended header? Not only would it provide a better diagnostic, it would also let you use a fixit that doesn't use editor placeholders.

[bruno]

lib/Lex/HeaderSearch.cpp

781–782

My current understanding (after chatting with @arphaman) is that editor placeholders isn't a great fit here:

• For non IDE uses of this, it will just be weird to output something like #include <<#framework-name#>/Foo.h>. Even if we wanted to emit this only for IDE use, clang currently has no way to make that distinction (editor placeholder related compiler flags only make sense when actually making the special token sequence lexable, not when generating it)
• Fixits are (with some known exceptions) meant to be applied to code and subsequently allow compilation to succeed, this wouldn't be the case here.

[dexonsmith]

include/clang/Basic/DiagnosticLexKinds.td

713

I'm also interested in just how expensive it would be, because I think users will be frustrated that the compiler knows it's a framework include "so it obviously knows which framework".

I'd be fine if the fix-it came in a follow-up commit though (not sure how Aaron feels).

lib/Lex/HeaderSearch.cpp

781–782

Okay, sounds good.

[bruno]

include/clang/Basic/DiagnosticLexKinds.td

713

I haven't measured, but for each quoted include we would have to:

• Start a fresh header search.
• Look for Foo.h in all possible frameworks in the path (just on the Darwin macOS SDK path that would be around 140 frameworks).
• If it's only found in once place, we are mostly safe to say we found a matching framework, otherwise we can't emit a reliable fixit.
• Header maps and VFS might add extra level of searches (this is very common in Xcode based clang invocations).

[dexonsmith]

include/clang/Basic/DiagnosticLexKinds.td

713

Can we just check if it's a header in the *same* framework?

[bruno]

include/clang/Basic/DiagnosticLexKinds.td

713

For some pretty obvious cases we can probably assume that this is what the user wants, but even so it might be misleading. For example, if you're in ABC/H1.h, you include H2.h and the framework ABC has an ABC/H2.h. It could be that #include "H2.h" is mapped via header maps to $SOURCE/H2.h instead of using the installed headers in the framework style build products.

This is likely a mistake, but what if it's intentional? I would prefer if the user rethink it instead of just buying potential misleading clues. OTOH, I share the concern that we don't need to be perfect here and only emit the fixit for really obvious cases, and not for the others. Will update the patch to include a fixit to the very straightforward scenario: H2.h was found in the same framework style path as the includer.

[aaron.ballman]

include/clang/Basic/DiagnosticLexKinds.td

713

Will update the patch to include a fixit to the very straightforward scenario: H2.h was found in the same framework style path as the includer.

Thanks, I think users will appreciate the compiler helping them out like that.

lib/Lex/HeaderSearch.cpp

781–782

That makes sense to me.

[bruno]

Updated the patch after Duncan and Aaron reviews. I actually went a bit more aggressive with the fixits, since I realized the conditions for the warning are already strict enough and we should take the chance to be more clear. For the attached testcase, the output now is:

./A.framework/Headers/A.h:2:10: warning: double-quoted include "A0.h" in framework header, expected angle-bracketed include <A/A0.h> instead
#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 include <B.h> instead
#include "B.h"
         ^~~~~
         <B.h>

[dexonsmith]

test/Modules/double-quotes.m

25–26

Would using an explicit module build make this any easier?

28–30

When there's a fixit, you don't need to list it in the warning text (the fix-it itself is sufficient). I also feel like "quoted include" is as clear as "double-quoted include" (but more succinct). So I think these would be better as:

warning: quoted include "..." in framework header, expected angle-bracketed include instead

[aaron.ballman]

test/Modules/double-quotes.m

28–30

Some other lexer diagnostics use "double-quoted" when they want to distinguish with "angle-bracketed" (see warn_pragma_include_alias_mismatch_angle and warn_pragma_include_alias_mismatch_quote as examples). I don't have a strong opinion on what form we use, but I'd prefer for it to be consistent exposition.

[dexonsmith]

test/Modules/double-quotes.m

28–30

I agree we should be consistent. No reason to change it here then.

[aaron.ballman]

LGTM

[bruno]

Update after Duncan's review: remove header name from the warning message (since it's already in the fixit)

test/Modules/double-quotes.m

25–26

---

Not really =T

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 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">;

include/clang/Basic/DiagnosticLexKinds.td

 def err_mmap_conflicting_export_as : Error<
   "conflicting re-export of module '%0' as '%1' or '%2'">;
 def warn_mmap_redundant_export_as : Warning<
   "module '%0' already re-exported as '%1'">,
   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_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'">, 
   InGroup<IncompleteUmbrella>;

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,
+                                 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
 /// 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(
     StringRef Filename, SourceLocation IncludeLoc, bool isAngled,
     const DirectoryLookup *FromDir, const DirectoryLookup *&CurDir,
for (const auto &IncluderAndDir : Includers) {
           StringRef SearchPathRef(IncluderAndDir.second->getName());
           SearchPath->clear();
           SearchPath->append(SearchPathRef.begin(), SearchPathRef.end());
         }
         if (RelativePath) {
           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
         // would've found this header in -I or -isystem directories.
         if (Diags.isIgnored(diag::ext_pp_include_search_ms, IncludeLoc)) {
           return FE;
         } else {
           MSFE = FE;
for (; i != SearchDirs.size(); ++i) {
     }
 
     if (checkMSVCHeaderSearch(Diags, MSFE, FE, IncludeLoc)) {
       if (SuggestedModule)
         *SuggestedModule = MSSuggestedModule;
       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;
   }
 
   // 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.

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

+#include "A0.h"
+#include "B.h"
+
+#include "X.h"
+
+int foo();

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

+// A0.h

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

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

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

+// A0.h

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

+// X.h

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

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

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

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

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

+#import "B.h" // Included from Z.h & A.h

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

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

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

+
+{
+  "mappings" :
+    {
+     "X.h" : "X/X.h"
+    }
+}

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

+{
+  '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"
+        }
+      ]
+    }
+  ]
+}

test/Modules/double-quotes.m

+// 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}}