From 50ebf26c30b297dbea9c20d993415b13f27b0300 Mon Sep 17 00:00:00 2001 From: Bruno Cardoso Lopes Date: Mon, 25 Jun 2018 22:24:17 +0000 Subject: [PATCH] Warning for framework include violation from Headers to PrivateHeaders 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 , 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: ./A.framework/Headers/A.h:1:10: warning: public framework header includes private framework header 'A/APriv.h' #include ^ rdar://problem/38712182 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@335542 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/DiagnosticGroups.td | 2 + include/clang/Basic/DiagnosticLexKinds.td | 3 ++ lib/Lex/HeaderSearch.cpp | 25 ++++++++--- .../A.framework/Headers/A.h | 4 ++ .../A.framework/Modules/module.modulemap | 4 ++ .../Modules/module.private.modulemap | 4 ++ .../A.framework/PrivateHeaders/APriv.h | 1 + .../A.framework/PrivateHeaders/APriv2.h | 1 + .../a.hmap.json | 8 ++++ .../flat-header-path/Z.h | 2 + .../flat-header-path/Z.modulemap | 4 ++ .../flat-header-path/Z.private.modulemap | 4 ++ .../flat-header-path/ZPriv.h | 1 + .../z.hmap.json | 7 +++ .../framework-public-includes-private/z.yaml | 45 +++++++++++++++++++ .../framework-public-includes-private.m | 37 +++++++++++++++ 16 files changed, 147 insertions(+), 5 deletions(-) create mode 100644 test/Modules/Inputs/framework-public-includes-private/A.framework/Headers/A.h create mode 100644 test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.modulemap create mode 100644 test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.private.modulemap create mode 100644 test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv.h create mode 100644 test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv2.h create mode 100644 test/Modules/Inputs/framework-public-includes-private/a.hmap.json create mode 100644 test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.h create mode 100644 test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.modulemap create mode 100644 test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.private.modulemap create mode 100644 test/Modules/Inputs/framework-public-includes-private/flat-header-path/ZPriv.h create mode 100644 test/Modules/Inputs/framework-public-includes-private/z.hmap.json create mode 100644 test/Modules/Inputs/framework-public-includes-private/z.yaml create mode 100644 test/Modules/framework-public-includes-private.m diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td index 7cc9967738..56384db124 100644 --- a/include/clang/Basic/DiagnosticGroups.td +++ b/include/clang/Basic/DiagnosticGroups.td @@ -32,6 +32,8 @@ 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">; diff --git a/include/clang/Basic/DiagnosticLexKinds.td b/include/clang/Basic/DiagnosticLexKinds.td index de6bffcdca..5d1e149eab 100644 --- a/include/clang/Basic/DiagnosticLexKinds.td +++ b/include/clang/Basic/DiagnosticLexKinds.td @@ -718,6 +718,9 @@ def warn_quoted_include_in_framework_header : Warning< "double-quoted include \"%0\" in framework header, " "expected angle-bracketed instead" >, InGroup, DefaultIgnore; +def warn_framework_include_private_from_public : Warning< + "public framework header includes private framework header '%0'" + >, InGroup; def warn_auto_module_import : Warning< "treating #%select{include|import|include_next|__include_macros}0 as an " diff --git a/lib/Lex/HeaderSearch.cpp b/lib/Lex/HeaderSearch.cpp index 757cd097e4..312bd2d061 100644 --- a/lib/Lex/HeaderSearch.cpp +++ b/lib/Lex/HeaderSearch.cpp @@ -621,11 +621,12 @@ static const char *copyString(StringRef Str, llvm::BumpPtrAllocator &Alloc) { return CopyStr; } -static bool isFrameworkStylePath(StringRef Path, +static bool isFrameworkStylePath(StringRef Path, bool &IsPrivateHeader, SmallVectorImpl &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: // @@ -637,12 +638,16 @@ static bool isFrameworkStylePath(StringRef 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; } @@ -654,11 +659,13 @@ diagnoseFrameworkInclude(DiagnosticsEngine &Diags, SourceLocation IncludeLoc, StringRef Includer, StringRef IncludeFilename, const FileEntry *IncludeFE, bool isAngled = false, bool FoundByHeaderMap = false) { + bool IsIncluderPrivateHeader = false; SmallString<128> FromFramework, ToFramework; - if (!isFrameworkStylePath(Includer, FromFramework)) + if (!isFrameworkStylePath(Includer, IsIncluderPrivateHeader, FromFramework)) return; - bool IsIncludeeInFramework = - isFrameworkStylePath(IncludeFE->getName(), ToFramework); + bool IsIncludeePrivateHeader = false; + bool IsIncludeeInFramework = isFrameworkStylePath( + IncludeFE->getName(), IsIncludeePrivateHeader, ToFramework); if (!isAngled && !FoundByHeaderMap) { SmallString<128> NewInclude("<"); @@ -672,6 +679,14 @@ diagnoseFrameworkInclude(DiagnosticsEngine &Diags, SourceLocation IncludeLoc, << 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 \ reference, look up the indicated file, diff --git a/test/Modules/Inputs/framework-public-includes-private/A.framework/Headers/A.h b/test/Modules/Inputs/framework-public-includes-private/A.framework/Headers/A.h new file mode 100644 index 0000000000..03bd90b494 --- /dev/null +++ b/test/Modules/Inputs/framework-public-includes-private/A.framework/Headers/A.h @@ -0,0 +1,4 @@ +#include +#include "APriv2.h" +#include +int foo(); diff --git a/test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.modulemap b/test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.modulemap new file mode 100644 index 0000000000..09639b2b50 --- /dev/null +++ b/test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.modulemap @@ -0,0 +1,4 @@ +// framework-public-includes-private/A.framework/Modules/module.modulemap +framework module A { + header "A.h" +} diff --git a/test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.private.modulemap b/test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.private.modulemap new file mode 100644 index 0000000000..8ede0b0d6e --- /dev/null +++ b/test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.private.modulemap @@ -0,0 +1,4 @@ +// framework-public-includes-private/A.framework/Modules/module.private.modulemap +framework module A_Private { + header "APriv.h" +} diff --git a/test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv.h b/test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv.h new file mode 100644 index 0000000000..34cc847512 --- /dev/null +++ b/test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv.h @@ -0,0 +1 @@ +// framework-public-includes-private/A.framework/PrivateHeaders/APriv.h diff --git a/test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv2.h b/test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv2.h new file mode 100644 index 0000000000..8ffeb41837 --- /dev/null +++ b/test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv2.h @@ -0,0 +1 @@ +// framework-public-includes-private/A.framework/PrivateHeaders/APriv2.h diff --git a/test/Modules/Inputs/framework-public-includes-private/a.hmap.json b/test/Modules/Inputs/framework-public-includes-private/a.hmap.json new file mode 100644 index 0000000000..42aed4551c --- /dev/null +++ b/test/Modules/Inputs/framework-public-includes-private/a.hmap.json @@ -0,0 +1,8 @@ + +{ + "mappings" : + { + "A.h" : "A/A.h", + "APriv2.h" : "A/APriv2.h" + } +} diff --git a/test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.h b/test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.h new file mode 100644 index 0000000000..157c6daa76 --- /dev/null +++ b/test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.h @@ -0,0 +1,2 @@ +// framework-public-includes-private/flat-header-path/Z.h +#import "ZPriv.h" diff --git a/test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.modulemap b/test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.modulemap new file mode 100644 index 0000000000..0a1a9710c4 --- /dev/null +++ b/test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.modulemap @@ -0,0 +1,4 @@ +// framework-public-includes-private/flat-header-path/Z.modulemap +framework module Z { + header "Z.h" +} diff --git a/test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.private.modulemap b/test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.private.modulemap new file mode 100644 index 0000000000..f409af8a15 --- /dev/null +++ b/test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.private.modulemap @@ -0,0 +1,4 @@ +// framework-public-includes-private/flat-header-path/Z.private.modulemap +framework module Z_Private { + header "ZPriv.h" +} diff --git a/test/Modules/Inputs/framework-public-includes-private/flat-header-path/ZPriv.h b/test/Modules/Inputs/framework-public-includes-private/flat-header-path/ZPriv.h new file mode 100644 index 0000000000..08532fedbf --- /dev/null +++ b/test/Modules/Inputs/framework-public-includes-private/flat-header-path/ZPriv.h @@ -0,0 +1 @@ +// framework-public-includes-private/flat-header-path/ZPriv.h diff --git a/test/Modules/Inputs/framework-public-includes-private/z.hmap.json b/test/Modules/Inputs/framework-public-includes-private/z.hmap.json new file mode 100644 index 0000000000..206b25ec73 --- /dev/null +++ b/test/Modules/Inputs/framework-public-includes-private/z.hmap.json @@ -0,0 +1,7 @@ + +{ + "mappings" : + { + "ZPriv.h" : "Z/ZPriv.h" + } +} diff --git a/test/Modules/Inputs/framework-public-includes-private/z.yaml b/test/Modules/Inputs/framework-public-includes-private/z.yaml new file mode 100644 index 0000000000..5793c1ff1f --- /dev/null +++ b/test/Modules/Inputs/framework-public-includes-private/z.yaml @@ -0,0 +1,45 @@ +{ + '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" + } + ] + } + ] +} diff --git a/test/Modules/framework-public-includes-private.m b/test/Modules/framework-public-includes-private.m new file mode 100644 index 0000000000..e58492091e --- /dev/null +++ b/test/Modules/framework-public-includes-private.m @@ -0,0 +1,37 @@ +// 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:1{{public framework header includes private framework header 'A/APriv.h'}} +// expected-warning@Inputs/framework-public-includes-private/A.framework/Headers/A.h:2{{public framework header includes private framework header 'A/APriv2.h'}} +// expected-warning@Inputs/framework-public-includes-private/flat-header-path/Z.h:2{{public framework header includes private framework header 'Z/ZPriv.h'}} -- 2.40.0