From 73d4cf2ee5b4705992e18873f658656f023b653b Mon Sep 17 00:00:00 2001 From: Bruno Cardoso Lopes Date: Fri, 1 Jun 2018 01:26:18 +0000 Subject: [PATCH] [Modules] Warning for module declarations lacking 'framework' qualifier When a module declaration for a framework lacks the 'framework' qualifier, the listed headers aren't found (because there's no trigger for the special framework style path lookup) and the module is silently not built. This leads to frameworks not being modularized by accident, which is pretty bad. Add a warning and suggest the user to add the 'framework' qualifier when we can prove that it's the case. rdar://problem/39193062 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@333718 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/DiagnosticGroups.td | 2 + include/clang/Basic/DiagnosticLexKinds.td | 5 ++ include/clang/Lex/ModuleMap.h | 23 +++++- lib/Lex/ModuleMap.cpp | 82 +++++++++++++------ .../Foo.framework/Headers/Foo.h | 1 + .../Foo.framework/Headers/FooB.h | 1 + .../Foo.framework/Modules/module.modulemap | 4 + test/Modules/incomplete-framework-module.m | 11 +++ 8 files changed, 100 insertions(+), 29 deletions(-) create mode 100644 test/Modules/Inputs/incomplete-framework-module/Foo.framework/Headers/Foo.h create mode 100644 test/Modules/Inputs/incomplete-framework-module/Foo.framework/Headers/FooB.h create mode 100644 test/Modules/Inputs/incomplete-framework-module/Foo.framework/Modules/module.modulemap create mode 100644 test/Modules/incomplete-framework-module.m diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td index 604b2ebdd0..643c178743 100644 --- a/include/clang/Basic/DiagnosticGroups.td +++ b/include/clang/Basic/DiagnosticGroups.td @@ -285,6 +285,8 @@ def IncompatiblePointerTypes [IncompatiblePointerTypesDiscardsQualifiers, IncompatibleFunctionPointerTypes]>; def IncompleteUmbrella : DiagGroup<"incomplete-umbrella">; +def IncompleteFrameworkModuleDeclaration + : DiagGroup<"incomplete-framework-module-declaration">; def NonModularIncludeInFrameworkModule : DiagGroup<"non-modular-include-in-framework-module">; def NonModularIncludeInModule : DiagGroup<"non-modular-include-in-module", diff --git a/include/clang/Basic/DiagnosticLexKinds.td b/include/clang/Basic/DiagnosticLexKinds.td index bf13b5baef..9b7f7e6777 100644 --- a/include/clang/Basic/DiagnosticLexKinds.td +++ b/include/clang/Basic/DiagnosticLexKinds.td @@ -694,6 +694,11 @@ def warn_mmap_mismatched_private_module_name : Warning< InGroup; def note_mmap_rename_top_level_private_module : Note< "rename '%0' to ensure it can be found by name">; +def warn_mmap_incomplete_framework_module_declaration : Warning< + "skipping '%0' because module declaration of '%1' lacks the 'framework' qualifier">, + InGroup; +def note_mmap_add_framework_keyword : Note< + "use 'framework module' to declare module '%0'">; def err_mmap_duplicate_header_attribute : Error< "header attribute '%0' specified multiple times">; diff --git a/include/clang/Lex/ModuleMap.h b/include/clang/Lex/ModuleMap.h index 406bac5c12..577f400897 100644 --- a/include/clang/Lex/ModuleMap.h +++ b/include/clang/Lex/ModuleMap.h @@ -303,8 +303,15 @@ private: Module *resolveModuleId(const ModuleId &Id, Module *Mod, bool Complain) const; /// Add an unresolved header to a module. + /// + /// \param Mod The module in which we're adding the unresolved header + /// directive. + /// \param Header The unresolved header directive. + /// \param NeedsFramework If Mod is not a framework but a missing header would + /// be found in case Mod was, set it to true. False otherwise. void addUnresolvedHeader(Module *Mod, - Module::UnresolvedHeaderDirective Header); + Module::UnresolvedHeaderDirective Header, + bool &NeedsFramework); /// Look up the given header directive to find an actual header file. /// @@ -312,14 +319,22 @@ private: /// \param Header The header directive to resolve. /// \param RelativePathName Filled in with the relative path name from the /// module to the resolved header. + /// \param NeedsFramework If M is not a framework but a missing header would + /// be found in case M was, set it to true. False otherwise. /// \return The resolved file, if any. const FileEntry *findHeader(Module *M, const Module::UnresolvedHeaderDirective &Header, - SmallVectorImpl &RelativePathName); + SmallVectorImpl &RelativePathName, + bool &NeedsFramework); /// Resolve the given header directive. - void resolveHeader(Module *M, - const Module::UnresolvedHeaderDirective &Header); + /// + /// \param M The module in which we're resolving the header directive. + /// \param Header The header directive to resolve. + /// \param NeedsFramework If M is not a framework but a missing header would + /// be found in case M was, set it to true. False otherwise. + void resolveHeader(Module *M, const Module::UnresolvedHeaderDirective &Header, + bool &NeedsFramework); /// Attempt to resolve the specified header directive as naming a builtin /// header. diff --git a/lib/Lex/ModuleMap.cpp b/lib/Lex/ModuleMap.cpp index 8343c8bb6b..6a1731ef6f 100644 --- a/lib/Lex/ModuleMap.cpp +++ b/lib/Lex/ModuleMap.cpp @@ -170,10 +170,13 @@ static void appendSubframeworkPaths(Module *Mod, llvm::sys::path::append(Path, "Frameworks", Paths[I-1] + ".framework"); } -const FileEntry * -ModuleMap::findHeader(Module *M, - const Module::UnresolvedHeaderDirective &Header, - SmallVectorImpl &RelativePathName) { +const FileEntry *ModuleMap::findHeader( + Module *M, const Module::UnresolvedHeaderDirective &Header, + SmallVectorImpl &RelativePathName, bool &NeedsFramework) { + // Search for the header file within the module's home directory. + auto *Directory = M->Directory; + SmallString<128> FullPathName(Directory->getName()); + auto GetFile = [&](StringRef Filename) -> const FileEntry * { auto *File = SourceMgr.getFileManager().getFile(Filename); if (!File || @@ -183,18 +186,8 @@ ModuleMap::findHeader(Module *M, return File; }; - if (llvm::sys::path::is_absolute(Header.FileName)) { - RelativePathName.clear(); - RelativePathName.append(Header.FileName.begin(), Header.FileName.end()); - return GetFile(Header.FileName); - } - - // Search for the header file within the module's home directory. - auto *Directory = M->Directory; - SmallString<128> FullPathName(Directory->getName()); - unsigned FullPathLength = FullPathName.size(); - - if (M->isPartOfFramework()) { + auto GetFrameworkFile = [&]() -> const FileEntry * { + unsigned FullPathLength = FullPathName.size(); appendSubframeworkPaths(M, RelativePathName); unsigned RelativePathLength = RelativePathName.size(); @@ -219,18 +212,46 @@ ModuleMap::findHeader(Module *M, Header.FileName); llvm::sys::path::append(FullPathName, RelativePathName); return GetFile(FullPathName); + }; + + if (llvm::sys::path::is_absolute(Header.FileName)) { + RelativePathName.clear(); + RelativePathName.append(Header.FileName.begin(), Header.FileName.end()); + return GetFile(Header.FileName); } + if (M->isPartOfFramework()) + return GetFrameworkFile(); + // Lookup for normal headers. llvm::sys::path::append(RelativePathName, Header.FileName); llvm::sys::path::append(FullPathName, RelativePathName); - return GetFile(FullPathName); + auto *NormalHdrFile = GetFile(FullPathName); + + if (M && !NormalHdrFile && Directory->getName().endswith(".framework")) { + // The lack of 'framework' keyword in a module declaration it's a simple + // mistake we can diagnose when the header exists within the proper + // framework style path. + FullPathName.assign(Directory->getName()); + RelativePathName.clear(); + if (auto *FrameworkHdr = GetFrameworkFile()) { + Diags.Report(Header.FileNameLoc, + diag::warn_mmap_incomplete_framework_module_declaration) + << Header.FileName << M->getFullModuleName(); + NeedsFramework = true; + } + return nullptr; + } + + return NormalHdrFile; } void ModuleMap::resolveHeader(Module *Mod, - const Module::UnresolvedHeaderDirective &Header) { + const Module::UnresolvedHeaderDirective &Header, + bool &NeedsFramework) { SmallString<128> RelativePathName; - if (const FileEntry *File = findHeader(Mod, Header, RelativePathName)) { + if (const FileEntry *File = + findHeader(Mod, Header, RelativePathName, NeedsFramework)) { if (Header.IsUmbrella) { const DirectoryEntry *UmbrellaDir = File->getDir(); if (Module *UmbrellaMod = UmbrellaDirs[UmbrellaDir]) @@ -1056,7 +1077,8 @@ void ModuleMap::setUmbrellaDir(Module *Mod, const DirectoryEntry *UmbrellaDir, } void ModuleMap::addUnresolvedHeader(Module *Mod, - Module::UnresolvedHeaderDirective Header) { + Module::UnresolvedHeaderDirective Header, + bool &NeedsFramework) { // If there is a builtin counterpart to this file, add it now so it can // wrap the system header. if (resolveAsBuiltinHeader(Mod, Header)) { @@ -1087,7 +1109,7 @@ void ModuleMap::addUnresolvedHeader(Module *Mod, // We don't have stat information or can't defer looking this file up. // Perform the lookup now. - resolveHeader(Mod, Header); + resolveHeader(Mod, Header, NeedsFramework); } void ModuleMap::resolveHeaderDirectives(const FileEntry *File) const { @@ -1107,10 +1129,11 @@ void ModuleMap::resolveHeaderDirectives(const FileEntry *File) const { } void ModuleMap::resolveHeaderDirectives(Module *Mod) const { + bool NeedsFramework = false; for (auto &Header : Mod->UnresolvedHeaders) // This operation is logically const; we're just changing how we represent // the header information for this file. - const_cast(this)->resolveHeader(Mod, Header); + const_cast(this)->resolveHeader(Mod, Header, NeedsFramework); Mod->UnresolvedHeaders.clear(); } @@ -1323,7 +1346,10 @@ namespace clang { /// The current module map file. const FileEntry *ModuleMapFile; - + + /// Source location of most recent parsed module declaration + SourceLocation CurrModuleDeclLoc; + /// The directory that file names in this module map file should /// be resolved relative to. const DirectoryEntry *Directory; @@ -1743,7 +1769,7 @@ void ModuleMapParser::parseModuleDecl() { HadError = true; return; } - consumeToken(); // 'module' keyword + CurrModuleDeclLoc = consumeToken(); // 'module' keyword // If we have a wildcard for the module name, this is an inferred submodule. // Parse it. @@ -2267,7 +2293,13 @@ void ModuleMapParser::parseHeaderDecl(MMToken::TokenKind LeadingToken, } } - Map.addUnresolvedHeader(ActiveModule, std::move(Header)); + bool NeedsFramework = false; + Map.addUnresolvedHeader(ActiveModule, std::move(Header), NeedsFramework); + + if (NeedsFramework && ActiveModule) + Diags.Report(CurrModuleDeclLoc, diag::note_mmap_add_framework_keyword) + << ActiveModule->getFullModuleName() + << FixItHint::CreateReplacement(CurrModuleDeclLoc, "framework module"); } static int compareModuleHeaders(const Module::Header *A, diff --git a/test/Modules/Inputs/incomplete-framework-module/Foo.framework/Headers/Foo.h b/test/Modules/Inputs/incomplete-framework-module/Foo.framework/Headers/Foo.h new file mode 100644 index 0000000000..547f1bc6b0 --- /dev/null +++ b/test/Modules/Inputs/incomplete-framework-module/Foo.framework/Headers/Foo.h @@ -0,0 +1 @@ +// Inputs/incomplete-framework-module/Foo.framework/Headers/Foo.h diff --git a/test/Modules/Inputs/incomplete-framework-module/Foo.framework/Headers/FooB.h b/test/Modules/Inputs/incomplete-framework-module/Foo.framework/Headers/FooB.h new file mode 100644 index 0000000000..eda7c17070 --- /dev/null +++ b/test/Modules/Inputs/incomplete-framework-module/Foo.framework/Headers/FooB.h @@ -0,0 +1 @@ +// FooB.h diff --git a/test/Modules/Inputs/incomplete-framework-module/Foo.framework/Modules/module.modulemap b/test/Modules/Inputs/incomplete-framework-module/Foo.framework/Modules/module.modulemap new file mode 100644 index 0000000000..d928a53efc --- /dev/null +++ b/test/Modules/Inputs/incomplete-framework-module/Foo.framework/Modules/module.modulemap @@ -0,0 +1,4 @@ +module Foo { + umbrella header "Foo.h" + header "FooB.h" +} diff --git a/test/Modules/incomplete-framework-module.m b/test/Modules/incomplete-framework-module.m new file mode 100644 index 0000000000..9034c1aab9 --- /dev/null +++ b/test/Modules/incomplete-framework-module.m @@ -0,0 +1,11 @@ +// RUN: rm -rf %t +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t \ +// RUN: -F%S/Inputs/incomplete-framework-module \ +// RUN: -fsyntax-only %s -verify + +#import + +// expected-warning@Inputs/incomplete-framework-module/Foo.framework/Modules/module.modulemap:2{{skipping 'Foo.h' because module declaration}} +// expected-warning@Inputs/incomplete-framework-module/Foo.framework/Modules/module.modulemap:3{{skipping 'FooB.h' because module declaration}} +// expected-note@Inputs/incomplete-framework-module/Foo.framework/Modules/module.modulemap:1{{use 'framework module' to declare module 'Foo'}} +// expected-note@Inputs/incomplete-framework-module/Foo.framework/Modules/module.modulemap:1{{use 'framework module' to declare module 'Foo'}} -- 2.40.0