[clang] 82034ac - [C++20] [Modules] Warn for importing implementation partition unit in interface units (#108493)

[via cfe-commits]

Author: Chuanqi Xu
Date: 2024-09-14T14:45:50+08:00
New Revision: 82034aca30ad8b08aadfe6b6b9048f5cdfa1d3ff

URL: https://github.com/llvm/llvm-project/commit/82034aca30ad8b08aadfe6b6b9048f5cdfa1d3ff
DIFF: https://github.com/llvm/llvm-project/commit/82034aca30ad8b08aadfe6b6b9048f5cdfa1d3ff.diff

LOG: [C++20] [Modules] Warn for importing implementation partition unit in interface units (#108493)

Recently, there are multiple false positive issue reports about the
reachability of implementation partition units:
- https://github.com/llvm/llvm-project/issues/105882
- https://github.com/llvm/llvm-project/issues/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.

Added: 
    

Modified: 
    clang/docs/ReleaseNotes.rst
    clang/include/clang/Basic/DiagnosticSemaKinds.td
    clang/lib/Sema/SemaModule.cpp
    clang/test/CXX/module/module.import/p2.cpp
    clang/test/Modules/cxx20-10-3-ex1.cpp

Removed: 
    


################################################################################
diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 3929a9fb599259..79b154ef1aef5e 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -303,6 +303,8 @@ Improvements to Clang's diagnostics
 
 - Clang now warns for u8 character literals used in C23 with ``-Wpre-c23-compat`` instead of ``-Wpre-c++17-compat``.
 
+- Clang now diagnose when importing module implementation partition units in module interface units.
+
 Improvements to Clang's time-trace
 ----------------------------------
 

diff  --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 50f27eec0ef2c4..e506811b034745 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -442,6 +442,10 @@ 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<
+  "importing an implementation partition unit in a module interface is not recommended. "
+  "Names from %0 may not be reachable">,
+  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..d6ebc382dad472 100644
--- a/clang/lib/Sema/SemaModule.cpp
+++ b/clang/lib/Sema/SemaModule.cpp
@@ -650,6 +650,14 @@ 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)
+        << Mod->Name;
+
   checkModuleImportContext(*this, Mod, ImportLoc, CurContext);
 
   // FIXME: we should support importing a submodule within a 
diff erent submodule

diff  --git a/clang/test/CXX/module/module.import/p2.cpp b/clang/test/CXX/module/module.import/p2.cpp
index ef6006811e7763..6b8e32f746b628 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 {{importing an implementation partition unit in a module interface is not recommended.}}
 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..82ecb40df910fa 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 at -1 {{importing an implementation partition unit in a module interface is not recommended.}}
 
 //--- std10-3-ex1-tu3.cpp
 export module M:Part;