[clang] [clang][modules] Make -fmodules-decluse work on the public/private pair of modules (PR #192585)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Matt (matts1)

<details>
<summary>Changes</summary>

Previously, it would only check the main module.
Now, if the main module is a private module (foo_Private), it will also
check the public module.

---
Full diff: https://github.com/llvm/llvm-project/pull/192585.diff


3 Files Affected:

- (modified) clang/include/clang/Basic/DiagnosticLexKinds.td (+2) 
- (modified) clang/lib/Lex/ModuleMap.cpp (+37-5) 
- (added) clang/test/Modules/declare-use-private-textual.cpp (+69) 


``````````diff
diff --git a/clang/include/clang/Basic/DiagnosticLexKinds.td b/clang/include/clang/Basic/DiagnosticLexKinds.td
index 2846db8320275..85fa290de6fd9 100644
--- a/clang/include/clang/Basic/DiagnosticLexKinds.td
+++ b/clang/include/clang/Basic/DiagnosticLexKinds.td
@@ -999,6 +999,8 @@ def err_undeclared_use_of_module : Error<
   "module %0 does not depend on a module exporting '%1'">;
 def err_undeclared_use_of_module_indirect : Error<
   "module %0 does not directly depend on a module exporting '%1', which is part of indirectly-used module %2">;
+def err_undeclared_use_of_module_private : Error<
+  "module %0 does not depend on a module exporting '%1', which is part of module %2, although its private module does">;
 def warn_non_modular_include_in_framework_module : Warning<
   "include of non-modular header inside framework module '%0': '%1'">,
   InGroup<NonModularIncludeInFrameworkModule>, DefaultIgnore;
diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index 56ae51ada7148..a61154ab53c58 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -46,6 +46,8 @@
 
 using namespace clang;
 
+static constexpr llvm::StringRef kPrivateModuleSuffix = "_Private";
+
 void ModuleMapCallbacks::anchor() {}
 
 void ModuleMap::resolveLinkAsDependencies(Module *Mod) {
@@ -498,10 +500,31 @@ void ModuleMap::diagnoseHeaderInclusion(Module *RequestingModule,
 
   // No errors for indirect modules. This may be a bit of a problem for modules
   // with no source files.
-  if (getTopLevelOrNull(RequestingModule) != getTopLevelOrNull(SourceModule))
-    return;
+  Module *TopLevelRequestingModule = getTopLevelOrNull(RequestingModule);
+  Module *TopLevelSourceModule = getTopLevelOrNull(SourceModule);
+  bool IsPublicForMainPrivateModule = false;
+  if (TopLevelRequestingModule != TopLevelSourceModule) {
+    // Suppose we have a pair of files foo.cpp / foo.h.
+    // Our build system may want to verify that foo.cpp only uses things
+    // declared in the implementation_deps of foo, while foo.h only uses things
+    // declared in interface_deps. This requires them to be two seperate
+    // modules, foo_Private and foo. This check is required to ensure that foo.h
+    // is still checked. Otherwise, foo.h would never be checked, since it will
+    // never be the top-level module.
+    if (TopLevelRequestingModule && TopLevelSourceModule &&
+        llvm::StringRef(TopLevelSourceModule->Name)
+            .ends_with(kPrivateModuleSuffix) &&
+        llvm::StringRef(TopLevelSourceModule->Name)
+                .drop_back(kPrivateModuleSuffix.size()) ==
+            TopLevelRequestingModule->Name) {
+      IsPublicForMainPrivateModule = true;
+    } else {
+      return;
+    }
+  }
 
   bool Excluded = false;
+  bool UsedByPrivateModule = false;
   Module *Private = nullptr;
   Module *NotUsed = nullptr;
 
@@ -524,6 +547,9 @@ void ModuleMap::diagnoseHeaderInclusion(Module *RequestingModule,
       if (RequestingModule && LangOpts.ModulesDeclUse &&
           !RequestingModule->directlyUses(Header.getModule())) {
         NotUsed = Header.getModule();
+        if (IsPublicForMainPrivateModule) {
+          UsedByPrivateModule = SourceModule->directlyUses(Header.getModule());
+        }
         continue;
       }
 
@@ -543,9 +569,15 @@ void ModuleMap::diagnoseHeaderInclusion(Module *RequestingModule,
 
   // We have found a module, but we don't use it.
   if (NotUsed) {
-    Diags.Report(FilenameLoc, diag::err_undeclared_use_of_module_indirect)
-        << RequestingModule->getTopLevelModule()->Name << Filename
-        << NotUsed->Name;
+    if (UsedByPrivateModule) {
+      Diags.Report(FilenameLoc, diag::err_undeclared_use_of_module_private)
+          << RequestingModule->getTopLevelModule()->Name << Filename
+          << NotUsed->Name;
+    } else {
+      Diags.Report(FilenameLoc, diag::err_undeclared_use_of_module_indirect)
+          << RequestingModule->getTopLevelModule()->Name << Filename
+          << NotUsed->Name;
+    }
     return;
   }
 
diff --git a/clang/test/Modules/declare-use-private-textual.cpp b/clang/test/Modules/declare-use-private-textual.cpp
new file mode 100644
index 0000000000000..840c0c24a538b
--- /dev/null
+++ b/clang/test/Modules/declare-use-private-textual.cpp
@@ -0,0 +1,69 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fimplicit-module-maps -fmodules-cache-path=%t -fmodules-strict-decluse -fmodule-name=lib_Private -I %t %t/lib.cpp -verify
+
+//--- module.modulemap
+// This simulates a modulemap that might reasonably be generated by a build system for something resembling the following.
+// This can be used to perform strict dependency checking to ensure that your build graph is correctly declared.
+// cc_library(
+//   name = "lib",
+//   # .cpp file in sources -> generate a compilation action with -fmodule-name=lib_Private
+//   # .h file in sources -> "textual header" in private modulemap
+//   sources = ["lib.cpp"],
+//   # headers -> "textual header" in public modulemap
+//   headers = ["lib.h"],
+//   # implementation_dep -> "use" in private modulemap
+//   implementation_deps = [":impl_dep"]
+//   # dep -> "use" in private and public modulemap
+//   deps = [":direct"],
+// )
+// cc_library(name = "direct", headers = ["direct.h"], deps = [":indirect"], ...)
+// cc_library(name = "indirect", headers = ["indirect.h"], ..)
+// cc_library(name = "impl_dep", headers = ["impl_dep.h"], ...)
+module lib_Private {
+  use impl_dep
+  use lib
+  use direct
+}
+
+module lib {
+  textual header "lib.h"
+  use direct
+}
+
+
+module impl_dep {
+  textual header "impl_dep.h"
+}
+
+module direct {
+  textual header "direct.h"
+  use indirect
+}
+
+module indirect {
+  textual header "indirect.h"
+}
+
+//--- impl_dep.h
+void impl_dep();
+//--- direct.h
+#include "impl_dep.h" // OK (it should only verify the main module).
+void direct();
+//--- indirect.h
+void indirect();
+//--- not_module.h
+void not_module();
+
+//--- lib.cpp
+#include "lib.h"
+#include "direct.h" // OK
+#include "indirect.h" // expected-error {{module lib_Private does not directly depend on a module exporting 'indirect.h', which is part of indirectly-used module indirect}}
+#include "impl_dep.h" // OK
+#include "not_module.h" // expected-error {{module lib_Private does not depend on a module exporting 'not_module.h'}}
+
+//--- lib.h
+#include "direct.h" // OK
+#include "indirect.h"  // expected-error {{module lib does not directly depend on a module exporting 'indirect.h', which is part of indirectly-used module indirect}}
+#include "impl_dep.h"  // expected-error {{module lib does not depend on a module exporting 'impl_dep.h', which is part of module impl_dep, although its private module does}}
+#include "not_module.h" // expected-error {{module lib does not depend on a module exporting 'not_module.h'}}

``````````

</details>


https://github.com/llvm/llvm-project/pull/192585