[C++20] [Modules] Warn for importing implementation partition unit in interface units

[ChuanqiXu9]

Recently, there are multiple false positive issue reports about the reachability of implementation partition units:

• [C++20] [Modules] clang should find CNTTP type from partition even if not exported #105882
• [Clang][Module] Miss definition of inline function in implementation partition units #101348
• https://lists.isocpp.org/core/2024/08/16232.php

And according to our use experience for modules, we find it is a pretty good practice to not import implementation partition units in the interface units. It can help developers to have a pretty good mental model for when to use an implementation partition unit: that any unit in the module but not in the module interfaces can be in the implementation partition unit.

So I think it is good to add the diagnostics.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-modules

Author: Chuanqi Xu (ChuanqiXu9)

Changes

Recently, there are multiple false positive issue reports about the reachability of implementation partition units:

• [C++20] [Modules] clang should find CNTTP type from partition even if not exported #105882
• [Clang][Module] Miss definition of inline function in implementation partition units #101348
• https://lists.isocpp.org/core/2024/08/16232.php

And according to our use experience for modules, we find it is a pretty good practice to not import implementation partition units in the interface units. It can help developers to have a pretty good mental model for when to use an implementation partition unit: that any unit in the module but not in the module interfaces can be in the implementation partition unit.

So I think it is good to add the diagnostics.

---

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

4 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+3)
• (modified) clang/lib/Sema/SemaModule.cpp (+5)
• (modified) clang/test/CXX/module/module.import/p2.cpp (+1-2)
• (modified) clang/test/Modules/cxx20-10-3-ex1.cpp (+1)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index efdc058edca56d..7545a06123789d 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -439,6 +439,9 @@ def warn_deprecated_literal_operator_id: Warning<
   "is deprecated">, InGroup<DeprecatedLiteralOperator>, DefaultIgnore;
 def warn_reserved_module_name : Warning<
   "%0 is a reserved name for a module">, InGroup<ReservedModuleIdentifier>;
+def warn_import_implementation_partition_unit_in_interface_unit : Warning<
+  "it is not suggested to import implementation partition unit in interface unit">,
+  InGroup<DiagGroup<"import-implementation-partition-unit-in-interface-unit">>;
 
 def warn_parameter_size: Warning<
   "%0 is a large (%1 bytes) pass-by-value argument; "
diff --git a/clang/lib/Sema/SemaModule.cpp b/clang/lib/Sema/SemaModule.cpp
index 3b84e7bd4277fd..5748906748a44e 100644
--- a/clang/lib/Sema/SemaModule.cpp
+++ b/clang/lib/Sema/SemaModule.cpp
@@ -650,6 +650,11 @@ DeclResult Sema::ActOnModuleImport(SourceLocation StartLoc,
   else
     VisibleModules.setVisible(Mod, ImportLoc);
 
+  assert((!Mod->isModulePartitionImplementation() || getCurrentModule()) &&
+         "We can only import a partition unit in a named module.");
+  if (Mod->isModulePartitionImplementation() && getCurrentModule()->isModuleInterfaceUnit())
+    Diag(ImportLoc, diag::warn_import_implementation_partition_unit_in_interface_unit);
+
   checkModuleImportContext(*this, Mod, ImportLoc, CurContext);
 
   // FIXME: we should support importing a submodule within a different submodule
diff --git a/clang/test/CXX/module/module.import/p2.cpp b/clang/test/CXX/module/module.import/p2.cpp
index ef6006811e7763..3ac76856c7cfc3 100644
--- a/clang/test/CXX/module/module.import/p2.cpp
+++ b/clang/test/CXX/module/module.import/p2.cpp
@@ -30,9 +30,8 @@ void test() {
 }
 
 //--- UseInPartA.cppm
-// expected-no-diagnostics
 export module M:partA;
-import :impl;
+import :impl; // expected-warning {{it is not suggested to import implementation partition unit in interface unit}}
 void test() {
   A a;
 }
diff --git a/clang/test/Modules/cxx20-10-3-ex1.cpp b/clang/test/Modules/cxx20-10-3-ex1.cpp
index 99b88c7e442ffd..dcdc92340366ae 100644
--- a/clang/test/Modules/cxx20-10-3-ex1.cpp
+++ b/clang/test/Modules/cxx20-10-3-ex1.cpp
@@ -37,6 +37,7 @@ module M:PartImpl;
 export module M;
                      // error: exported partition :Part is an implementation unit
 export import :PartImpl; // expected-error {{module partition implementations cannot be exported}}
+                         // expected-warning@-1 {{it is not suggested to import implementation partition unit in interface unit}}
 
 //--- std10-3-ex1-tu3.cpp
 export module M:Part;

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[cor3ntin]

Suggested change

	"it is not suggested to import implementation partition unit in interface unit">,
	"importing an implementation partition unit in a module interface is not recommended. Symbols exported by %0 may not be reachable">,

Maybe something like that ?

[ChuanqiXu9]

Done. But I changed Symbols exported to Names in since the term name is more precise and we won't export things in implementation partition unit.

[AaronBallman]

This should probably come with a release note so that users know about the improved diagnostic behavior.

[AaronBallman]

Suggested change

	"importing an implementation partition unit in a module interface is not recommended. "
	"Names in by %0 may not be reachable">,
	"importing an implementation partition unit in a module interface is not recommended; "
	"names from '%0' may not be reachable">,

[ChuanqiXu9]

Done

[AaronBallman]

Not for this PR, but at some point it might make sense to have an overload for operator<< so we can pass a const Module * directly and it automatically quotes the name, similar to how NamedDecl works.

[ChuanqiXu9]

Got it. I'll try to make it next time

[ChuanqiXu9]

This should probably come with a release note so that users know about the improved diagnostic behavior.

Done

[cor3ntin]

lgtm