[C++20] [Modules] Merge same concept decls in global module fragment

ChuanqiXu9 · ChuanqiXu9 · commit 4d9251bd780d · 2022-07-29T10:50:27.000+08:00
According to [basic.def.odr]p14, the same redeclarations in different TU but not attached to a named module are allowed. But we didn't take care of concept decl for this condition. This patch tries to fix this problem. Reviewed By: ilya-biryukov Differention Revision: https://reviews.llvm.org/D130614
diff --git a/clang/include/clang/Basic/Module.h b/clang/include/clang/Basic/Module.h
@@ -525,6 +525,11 @@ class Module {
     Parent->SubModules.push_back(this);
   }
 
+  /// Is this module have similar semantics as headers.
+  bool isHeaderLikeModule() const {
+    return isModuleMapModule() || isHeaderUnit();
+  }
+
   /// Is this a module partition.
   bool isModulePartition() const {
     return Kind == ModulePartitionInterface ||
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
@@ -4479,6 +4479,8 @@ class Sema final {
   bool CheckRedeclarationModuleOwnership(NamedDecl *New, NamedDecl *Old);
   bool CheckRedeclarationExported(NamedDecl *New, NamedDecl *Old);
   bool CheckRedeclarationInModule(NamedDecl *New, NamedDecl *Old);
+  bool IsRedefinitionInModule(const NamedDecl *New,
+                                 const NamedDecl *Old) const;
 
   void DiagnoseAmbiguousLookup(LookupResult &Result);
   //@}
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
@@ -1719,6 +1719,80 @@ bool Sema::CheckRedeclarationInModule(NamedDecl *New, NamedDecl *Old) {
   return false;
 }
 
+// Check the redefinition in C++20 Modules.
+//
+// [basic.def.odr]p14:
+// For any definable item D with definitions in multiple translation units,
+// - if D is a non-inline non-templated function or variable, or
+// - if the definitions in different translation units do not satisfy the
+// following requirements,
+//   the program is ill-formed; a diagnostic is required only if the definable
+//   item is attached to a named module and a prior definition is reachable at
+//   the point where a later definition occurs.
+// - Each such definition shall not be attached to a named module
+// ([module.unit]).
+// - Each such definition shall consist of the same sequence of tokens, ...
+// ...
+//
+// Return true if the redefinition is not allowed. Return false otherwise.
+bool Sema::IsRedefinitionInModule(const NamedDecl *New,
+                                     const NamedDecl *Old) const {
+  assert(getASTContext().isSameEntity(New, Old) &&
+         "New and Old are not the same definition, we should diagnostic it "
+         "immediately instead of checking it.");
+  assert(const_cast<Sema *>(this)->isReachable(New) &&
+         const_cast<Sema *>(this)->isReachable(Old) &&
+         "We shouldn't see unreachable definitions here.");
+
+  Module *NewM = New->getOwningModule();
+  Module *OldM = Old->getOwningModule();
+
+  // We only checks for named modules here. The header like modules is skipped.
+  // FIXME: This is not right if we import the header like modules in the module
+  // purview.
+  //
+  // For example, assuming "header.h" provides definition for `D`.
+  // ```C++
+  // //--- M.cppm
+  // export module M;
+  // import "header.h"; // or #include "header.h" but import it by clang modules
+  // actually.
+  //
+  // //--- Use.cpp
+  // import M;
+  // import "header.h"; // or uses clang modules.
+  // ```
+  //
+  // In this case, `D` has multiple definitions in multiple TU (M.cppm and
+  // Use.cpp) and `D` is attached to a named module `M`. The compiler should
+  // reject it. But the current implementation couldn't detect the case since we
+  // don't record the information about the importee modules.
+  //
+  // But this might not be painful in practice. Since the design of C++20 Named
+  // Modules suggests us to use headers in global module fragment instead of
+  // module purview.
+  if (NewM && NewM->isHeaderLikeModule())
+    NewM = nullptr;
+  if (OldM && OldM->isHeaderLikeModule())
+    OldM = nullptr;
+
+  if (!NewM && !OldM)
+    return true;
+
+  // [basic.def.odr]p14.3
+  // Each such definition shall not be attached to a named module
+  // ([module.unit]).
+  if ((NewM && NewM->isModulePurview()) || (OldM && OldM->isModulePurview()))
+    return true;
+
+  // Then New and Old lives in the same TU if their share one same module unit.
+  if (NewM)
+    NewM = NewM->getTopLevelModule();
+  if (OldM)
+    OldM = OldM->getTopLevelModule();
+  return OldM == NewM;
+}
+
 static bool isUsingDecl(NamedDecl *D) {
   return isa<UsingShadowDecl>(D) ||
          isa<UnresolvedUsingTypenameDecl>(D) ||
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
@@ -8724,7 +8724,7 @@ void Sema::CheckConceptRedefinition(ConceptDecl *NewDecl,
   if (Previous.empty())
     return;
 
-  auto *OldConcept = dyn_cast<ConceptDecl>(Previous.getRepresentativeDecl());
+  auto *OldConcept = dyn_cast<ConceptDecl>(Previous.getRepresentativeDecl()->getUnderlyingDecl());
   if (!OldConcept) {
     auto *Old = Previous.getRepresentativeDecl();
     Diag(NewDecl->getLocation(), diag::err_redefinition_different_kind)
@@ -8742,7 +8742,8 @@ void Sema::CheckConceptRedefinition(ConceptDecl *NewDecl,
     AddToScope = false;
     return;
   }
-  if (hasReachableDefinition(OldConcept)) {
+  if (hasReachableDefinition(OldConcept) &&
+      IsRedefinitionInModule(NewDecl, OldConcept)) {
     Diag(NewDecl->getLocation(), diag::err_redefinition)
         << NewDecl->getDeclName();
     notePreviousDefinition(OldConcept, NewDecl->getLocation());
diff --git a/clang/test/Modules/merge-concepts.cppm b/clang/test/Modules/merge-concepts.cppm
@@ -0,0 +1,185 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -I%t %t/A.cppm -o %t/A.pcm
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -I%t %t/B.cppm -o %t/B.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/Use.cpp -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/Use2.cpp -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/Use3.cpp -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/Use4.cpp -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/C.cppm -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -I%t %t/D.cppm -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -I%t %t/E.cppm -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -I%t %t/F.cppm -verify -fsyntax-only
+//
+// Testing header units for coverity.
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header %t/foo.h -o %t/foo.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t -fmodule-file=%t/foo.pcm %t/Use5.cpp -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t -fmodule-file=%t/foo.pcm %t/Use6.cpp -verify -fsyntax-only
+//
+// Testing with module map modules. It is unclear about the relation ship between Clang modules and
+// C++20 Named Modules. Will they coexist? Or will they be mutually exclusive?
+// The test here is for primarily coverity.
+//
+// RUN: rm -f %t/foo.pcm
+// RUN: %clang_cc1 -std=c++20 -fmodules -fmodules-cache-path=%t -fprebuilt-module-path=%t \
+// RUN:   -fmodule-map-file=%t/module.map %t/Use7.cpp -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -fmodules -fmodules-cache-path=%t -fprebuilt-module-path=%t \
+// RUN:   -fmodule-map-file=%t/module.map %t/Use7.cpp -verify -fsyntax-only
+// Testing module map modules with named modules.
+// RUN: %clang_cc1 -std=c++20 -fmodules -fmodules-cache-path=%t -fmodule-map-file=%t/module.map \
+// RUN:   %t/A.cppm -o %t/A.pcm
+// RUN: %clang_cc1 -std=c++20 -fmodules -fmodules-cache-path=%t -fprebuilt-module-path=%t \
+// RUN:   -fmodule-map-file=%t/module.map %t/Use7.cpp -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -fmodules -fmodules-cache-path=%t -fprebuilt-module-path=%t \
+// RUN:   -fmodule-map-file=%t/module.map %t/Use7.cpp -verify -fsyntax-only
+
+//
+//--- foo.h
+#ifndef FOO_H
+#define FOO_H
+template <class T, class U>
+concept same_as = __is_same(T, U);
+#endif
+
+// The compiler would warn if we include foo_h twice without guard.
+//--- redecl.h
+#ifndef REDECL_H
+#define REDECL_H
+template <class T, class U>
+concept same_as = __is_same(T, U);
+#endif
+
+//--- A.cppm
+module;
+#include "foo.h"
+export module A;
+export using ::same_as;
+
+//--- B.cppm
+module;
+#include "foo.h"
+export module B;
+export using ::same_as;
+
+//--- Use.cpp
+// expected-no-diagnostics
+import A;
+import B;
+
+template <class T> void foo()
+  requires same_as<T, int>
+{}
+
+//--- Use2.cpp
+// expected-no-diagnostics
+#include "foo.h"
+import A;
+
+template <class T> void foo()
+  requires same_as<T, int>
+{}
+
+//--- Use3.cpp
+// expected-no-diagnostics
+import A;
+#include "foo.h"
+
+template <class T> void foo()
+  requires same_as<T, int>
+{}
+
+//--- Use4.cpp
+// expected-no-diagnostics
+import A;
+import B;
+#include "foo.h"
+
+template <class T> void foo()
+  requires same_as<T, int>
+{}
+
+//--- C.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module C;
+import A;
+import B;
+
+template <class T> void foo()
+  requires same_as<T, int>
+{}
+
+//--- D.cppm
+module;
+#include "foo.h"
+#include "redecl.h"
+export module D;
+export using ::same_as;
+
+// expected-error@* {{redefinition of 'same_as'}}
+// expected-note@* 1+{{previous definition is here}}
+
+//--- E.cppm
+module;
+#include "foo.h"
+export module E;
+export template <class T, class U>
+concept same_as = __is_same(T, U);
+
+// expected-error@* {{redefinition of 'same_as'}}
+// expected-note@* 1+{{previous definition is here}}
+
+//--- F.cppm
+export module F;
+template <class T, class U>
+concept same_as = __is_same(T, U);
+template <class T, class U>
+concept same_as = __is_same(T, U);
+
+// expected-error@* {{redefinition of 'same_as'}}
+// expected-note@* 1+{{previous definition is here}}
+
+//--- Use5.cpp
+// expected-no-diagnostics
+import "foo.h";
+import A;
+
+template <class T> void foo()
+  requires same_as<T, int>
+{}
+
+//--- Use6.cpp
+// expected-no-diagnostics
+import A;
+import "foo.h";
+
+template <class T> void foo()
+  requires same_as<T, int>
+{}
+
+//--- module.map
+module "foo" {
+  export * 
+  header "foo.h"
+}
+
+//--- Use7.cpp
+// expected-no-diagnostics
+#include "foo.h"
+import A;
+
+template <class T> void foo()
+  requires same_as<T, int>
+{}
+
+//--- Use8.cpp
+// expected-no-diagnostics
+import A;
+#include "foo.h"
+
+template <class T> void foo()
+  requires same_as<T, int>
+{}
