release/19.x: [C++20] [Modules] Don't diagnose duplicated implicit decl in multiple named modules (#102423)

[llvmbot]

Backport e72d956

Requested by: @ChuanqiXu9

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-modules

Author: None (llvmbot)

Changes

Backport e72d956

Requested by: @ChuanqiXu9

---

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

3 Files Affected:

• (modified) clang/lib/Serialization/ASTReaderDecl.cpp (+49-16)
• (added) clang/test/Modules/pr102349.cppm (+52)
• (added) clang/test/Modules/pr102360.cppm (+53)

diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 31ab6c651d59f4..ccc97f65526d6c 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -3684,6 +3684,54 @@ 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) {
+  Module *M = Previous->getOwningModule();
+
+  // We only care about the case in named modules.
+  if (!M || !M->isNamedModule())
+    return;
+
+  // If it is previous implcitly introduced, it is not meaningful to
+  // diagnose it.
+  if (Previous->isImplicit())
+    return;
+
+  // FIXME: Get rid of the enumeration of decl types once we have an appropriate
+  // abstract for decls of an entity. e.g., the namespace decl and using decl
+  // doesn't introduce an entity.
+  if (!isa<VarDecl, FunctionDecl, TagDecl, RedeclarableTemplateDecl>(Previous))
+    return;
+
+  // Skip implicit instantiations since it may give false positive diagnostic
+  // messages.
+  // FIXME: Maybe this shows the implicit instantiations may have incorrect
+  // module owner ships. But given we've finished the compilation of a module,
+  // how can we add new entities to that module?
+  if (auto *VTSD = dyn_cast<VarTemplateSpecializationDecl>(Previous);
+      VTSD && !VTSD->isExplicitSpecialization())
+    return;
+  if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(Previous);
+      CTSD && !CTSD->isExplicitSpecialization())
+    return;
+  if (auto *Func = dyn_cast<FunctionDecl>(Previous))
+    if (auto *FTSI = Func->getTemplateSpecializationInfo();
+        FTSI && !FTSI->isExplicitSpecialization())
+      return;
+
+  // It is fine if they are in the same module.
+  if (Reader.getContext().isInSameModule(M, D->getOwningModule()))
+    return;
+
+  Reader.Diag(Previous->getLocation(),
+              diag::err_multiple_decl_in_different_modules)
+      << cast<NamedDecl>(Previous) << M->Name;
+  Reader.Diag(D->getLocation(), diag::note_also_found);
+}
+
 void ASTDeclReader::attachPreviousDecl(ASTReader &Reader, Decl *D,
                                        Decl *Previous, Decl *Canon) {
   assert(D && Previous);
@@ -3697,22 +3745,7 @@ void ASTDeclReader::attachPreviousDecl(ASTReader &Reader, Decl *D,
 #include "clang/AST/DeclNodes.inc"
   }
 
-  // [basic.link]/p10:
-  //    If two declarations of an entity are attached to different modules,
-  //    the program is ill-formed;
-  //
-  // FIXME: Get rid of the enumeration of decl types once we have an appropriate
-  // abstract for decls of an entity. e.g., the namespace decl and using decl
-  // doesn't introduce an entity.
-  if (Module *M = Previous->getOwningModule();
-      M && M->isNamedModule() &&
-      isa<VarDecl, FunctionDecl, TagDecl, RedeclarableTemplateDecl>(Previous) &&
-      !Reader.getContext().isInSameModule(M, D->getOwningModule())) {
-    Reader.Diag(Previous->getLocation(),
-                diag::err_multiple_decl_in_different_modules)
-        << cast<NamedDecl>(Previous) << M->Name;
-    Reader.Diag(D->getLocation(), diag::note_also_found);
-  }
+  checkMultipleDefinitionInNamedModules(Reader, D, Previous);
 
   // If the declaration was visible in one module, a redeclaration of it in
   // another module remains visible even if it wouldn't be visible by itself.
diff --git a/clang/test/Modules/pr102349.cppm b/clang/test/Modules/pr102349.cppm
new file mode 100644
index 00000000000000..2d166c9e93fcfc
--- /dev/null
+++ b/clang/test/Modules/pr102349.cppm
@@ -0,0 +1,52 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/a.cppm -emit-module-interface -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 %t/b.cppm -emit-module-interface -o %t/b.pcm \
+// RUN:   -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/c.cppm -emit-module-interface -o %t/c.pcm \
+// RUN:   -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/d.cpp -fsyntax-only -verify \
+// RUN:   -fprebuilt-module-path=%t
+
+//--- a.cppm
+export module a;
+
+export template<typename>
+struct a;
+
+template<typename>
+struct a {
+};
+
+export template<typename T>
+constexpr auto aa = a<T>();
+
+//--- b.cppm
+export module b;
+
+import a;
+
+static void b() {
+	static_cast<void>(a<void>());
+}
+
+//--- c.cppm
+export module c;
+
+import a;
+
+static void c() {
+	static_cast<void>(aa<void>);
+}
+
+//--- d.cpp
+// expected-no-diagnostics
+import a;
+import b;
+import c;
+
+static void d() {
+	static_cast<void>(a<void>());
+}
diff --git a/clang/test/Modules/pr102360.cppm b/clang/test/Modules/pr102360.cppm
new file mode 100644
index 00000000000000..e0dab1a0318015
--- /dev/null
+++ b/clang/test/Modules/pr102360.cppm
@@ -0,0 +1,53 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/a.cppm -emit-module-interface -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 %t/b.cppm -emit-module-interface -o %t/b.pcm \
+// RUN:   -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/c.cppm -emit-module-interface -o %t/c.pcm \
+// RUN:   -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/d.cpp -fsyntax-only -verify \
+// RUN:   -fprebuilt-module-path=%t
+
+//--- a.cppm
+export module a;
+
+template<typename>
+constexpr auto impl = true;
+
+export template<typename T>
+void a() {
+}
+
+export template<typename T> requires impl<T>
+void a() {
+}
+
+//--- b.cppm
+export module b;
+
+import a;
+
+static void b() {
+	a<void>();
+}
+
+//--- c.cppm
+export module c;
+
+import a;
+
+static void c() {
+	a<void>();
+}
+
+//--- d.cpp
+// expected-no-diagnostics
+import a;
+import b;
+import c;
+
+static void d() {
+	a<void>();
+}

[tru]

@ChuanqiXu9 who can review this?

[ChuanqiXu9]

I landed this directly as the owner of serialization. I feel this change is not riskful as it adds more conditions to generate a diagnose message we didn't do in 18.x and before. So nothing will be worse.

[github-actions]

@ChuanqiXu9 (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.