[C++20] [Modules] Warn for duplicated decls in mutliple module units

[ChuanqiXu9]

It is a long standing issue that the duplicated declarations in multiple module units would cause the compilation performance to get slowed down. And there are many questions or issue reports. So I think it is better to add a warning for it.

And given this is not because the users' code violates the language specification or any best practices, the warning is disabled by default even if -Wall is specified. The users need to specify the warning explcitly or use Weverything.

The documentation will add separately.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-modules

Author: Chuanqi Xu (ChuanqiXu9)

Changes

It is a long standing issue that the duplicated declarations in multiple module units would cause the compilation performance to get slowed down. And there are many questions or issue reports. So I think it is better to add a warning for it.

And given this is not because the users' code violates the language specification or any best practices, the warning is disabled by default even if -Wall is specified. The users need to specify the warning explcitly or use Weverything.

The documentation will add separately.

---

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

5 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticSerializationKinds.td (+6)
• (modified) clang/include/clang/Serialization/ASTReader.h (+6)
• (modified) clang/lib/Serialization/ASTReader.cpp (+39)
• (modified) clang/lib/Serialization/ASTReaderDecl.cpp (+18-3)
• (added) clang/test/Modules/warn-duplicated-decls-in-module-units.cppm (+83)

diff --git a/clang/include/clang/Basic/DiagnosticSerializationKinds.td b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
index 9854972cbfe7e4..253a955431997b 100644
--- a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
@@ -134,6 +134,12 @@ def warn_module_system_bit_conflict : Warning<
   "as a non-system module; any difference in diagnostic options will be ignored">,
   InGroup<ModuleConflict>;
 
+def warn_decls_in_multiple_modules : Warning<
+  "declaration %0 is detected to be defined in multiple module units, first is from '%1' and second is from '%2'; "
+  "the compiler may not be good at merging the definitions. ">,
+  InGroup<DiagGroup<"decls-in-multiple-modules">>,
+  DefaultIgnore;
+
 def err_failed_to_find_module_file : Error<
   "failed to find module file for module '%0'">;
 } // let CategoryName
diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index 4593213c5f43ce..2d8952ddbd71df 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -648,6 +648,12 @@ class ASTReader
   /// performed deduplication.
   llvm::SetVector<NamedDecl *> PendingMergedDefinitionsToDeduplicate;
 
+  /// The duplicated definitions in module units which are pending to be warned.
+  /// We need to delay it to wait for the loading of definitions since we don't
+  /// want to warn for forward declarations.
+  llvm::SmallVector<std::pair<Decl *, Decl *>>
+      PendingWarningForDuplicatedDefsInModuleUnits;
+
   /// Read the record that describes the lexical contents of a DC.
   bool ReadLexicalDeclContextStorage(ModuleFile &M,
                                      llvm::BitstreamCursor &Cursor,
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index fa9b815239dbb6..be83805f1e92b9 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -9955,6 +9955,45 @@ void ASTReader::finishPendingActions() {
   }
   PendingDefinitions.clear();
 
+  for (auto [D, Previous] : PendingWarningForDuplicatedDefsInModuleUnits) {
+    auto hasDefinitionImpl = [this](Decl *D, auto hasDefinitionImpl) {
+      if (auto *VD = dyn_cast<VarDecl>(D))
+        return VD->isThisDeclarationADefinition() ||
+               VD->isThisDeclarationADemotedDefinition();
+
+      if (auto *TD = dyn_cast<TagDecl>(D))
+        return TD->isThisDeclarationADefinition() ||
+               TD->isThisDeclarationADemotedDefinition();
+
+      if (auto *FD = dyn_cast<FunctionDecl>(D))
+        return FD->isThisDeclarationADefinition() || PendingBodies.count(FD);
+
+      if (auto *RTD = dyn_cast<RedeclarableTemplateDecl>(D))
+        return hasDefinitionImpl(RTD->getTemplatedDecl(), hasDefinitionImpl);
+
+      // Conservatively return false here.
+      return false;
+    };
+
+    auto hasDefinition = [this, &hasDefinitionImpl](Decl *D) {
+      return hasDefinitionImpl(D, hasDefinitionImpl);
+    };
+
+    // It is not good to prevent multiple declarations since the forward
+    // declaration is common. Let's try to avoid duplicated definitions
+    // only.
+    if (!hasDefinition(D) || !hasDefinition(Previous))
+      continue;
+
+    Module *PM = Previous->getOwningModule();
+    Module *DM = D->getOwningModule();
+    Diag(D->getLocation(), diag::warn_decls_in_multiple_modules)
+        << cast<NamedDecl>(Previous) << PM->getTopLevelModuleName()
+        << (DM ? DM->getTopLevelModuleName() : "global module");
+    Diag(Previous->getLocation(), diag::note_also_found);
+  }
+  PendingWarningForDuplicatedDefsInModuleUnits.clear();
+
   // Load the bodies of any functions or methods we've encountered. We do
   // this now (delayed) so that we can be sure that the declaration chains
   // have been fully wired up (hasBody relies on this).
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 4d9d024796716e..d1b77358d0cde4 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -320,6 +320,9 @@ class ASTDeclReader : public DeclVisitor<ASTDeclReader, void> {
   static void attachPreviousDecl(ASTReader &Reader, Decl *D, Decl *Previous,
                                  Decl *Canon);
 
+  static void checkMultipleDefinitionInNamedModules(ASTReader &Reader, Decl *D,
+                                                    Decl *Previous);
+
   template <typename DeclT>
   static void attachLatestDeclImpl(Redeclarable<DeclT> *D, Decl *Latest);
   static void attachLatestDeclImpl(...);
@@ -3690,8 +3693,9 @@ static void inheritDefaultTemplateArguments(ASTContext &Context,
 // [basic.link]/p10:
 //    If two declarations of an entity are attached to different modules,
 //    the program is ill-formed;
-static void checkMultipleDefinitionInNamedModules(ASTReader &Reader, Decl *D,
-                                                  Decl *Previous) {
+void ASTDeclReader::checkMultipleDefinitionInNamedModules(ASTReader &Reader,
+                                                          Decl *D,
+                                                          Decl *Previous) {
   // If it is previous implcitly introduced, it is not meaningful to
   // diagnose it.
   if (Previous->isImplicit())
@@ -3721,8 +3725,19 @@ static void checkMultipleDefinitionInNamedModules(ASTReader &Reader, Decl *D,
     return;
 
   // We only forbids merging decls within named modules.
-  if (!M->isNamedModule())
+  if (!M->isNamedModule()) {
+    // Try to warn the case that we merged decls from global module.
+    if (!M->isGlobalModule())
+      return;
+
+    if (D->getOwningModule() &&
+        M->getTopLevelModule() == D->getOwningModule()->getTopLevelModule())
+      return;
+
+    Reader.PendingWarningForDuplicatedDefsInModuleUnits.push_back(
+        {D, Previous});
     return;
+  }
 
   // It is fine if they are in the same module.
   if (Reader.getContext().isInSameModule(M, D->getOwningModule()))
diff --git a/clang/test/Modules/warn-duplicated-decls-in-module-units.cppm b/clang/test/Modules/warn-duplicated-decls-in-module-units.cppm
new file mode 100644
index 00000000000000..f9156497bc6b17
--- /dev/null
+++ b/clang/test/Modules/warn-duplicated-decls-in-module-units.cppm
@@ -0,0 +1,83 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/m1.cppm -emit-module-interface -o %t/m1.pcm
+// RUN: %clang_cc1 -std=c++20 %t/m2.cppm -emit-module-interface -o %t/m2.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/use.cc -fsyntax-only \
+// RUN:     -verify
+//
+// RUN: %clang_cc1 -std=c++20 %t/m1.cppm -Wall -emit-module-interface -o %t/m1.pcm
+// RUN: %clang_cc1 -std=c++20 %t/m2.cppm -Wall -emit-module-interface -o %t/m2.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/use.cc -fsyntax-only \
+// RUN:     -verify -Wall
+//
+// RUN: %clang_cc1 -std=c++20 %t/m1.cppm -Wdecls-in-multiple-modules -emit-module-interface -o %t/m1.pcm
+// RUN: %clang_cc1 -std=c++20 %t/m2.cppm -Wdecls-in-multiple-modules -emit-module-interface -o %t/m2.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/use.cc -fsyntax-only \
+// RUN:     -verify -Wdecls-in-multiple-modules -DWARNING
+
+//--- foo.h
+#ifndef FOO_H
+#define FOO_H
+
+enum E { E1, E2 };
+
+int a = 43;
+
+class foo {
+public:
+    void consume(E, int);
+};
+
+inline void func() {}
+
+void fwd_decl();
+
+#endif 
+
+//--- m1.cppm
+module;
+#include "foo.h"
+export module m1;
+export {
+    using ::foo;
+    using ::a;
+    using ::func;
+    using ::fwd_decl;
+    using ::E;
+}
+
+//--- m2.cppm
+module;
+#include "foo.h"
+export module m2;
+export {
+    using ::foo;
+    using ::a;
+    using ::func;
+    using ::fwd_decl;
+    using ::E;
+}
+
+//--- use.cc
+import m1;
+import m2;
+void use();
+void use() {
+    E e = E1;
+    foo f;
+    f.consume(e, a);
+    func();
+    fwd_decl();
+}
+
+#ifndef WARNING
+// expected-no-diagnostics
+#else
+// expected-warning@* {{declaration 'E' is detected to be defined in multiple module units}}
+// expected-warning@* {{declaration 'foo' is detected to be defined in multiple module units}}
+// expected-warning@* {{declaration 'a' is detected to be defined in multiple module units}}
+// expected-warning@* {{declaration 'func' is detected to be defined in multiple module units}}
+// expected-note@* 1+ {{}}
+#endif

[mikaelholmen]

I get a clang warning here:

../../clang/lib/Serialization/ASTReader.cpp:9978:27: error: lambda capture 'this' is not used [-Werror,-Wunused-lambda-capture]
    auto hasDefinition = [this, &hasDefinitionImpl](Decl *D) {
                          ^~~~~
1 error generated.

[ChuanqiXu9]

Thanks and this looks to be fixed by someoneelse. Thanks again!