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

[ChuanqiXu]

Summary

I found this one when I try to add a C++20 Modules example for D130585. Then I found there are something to improve:
(1) When the declaration is shadowed by using decls, we could try to find its targeted decl. This is fine since we would check Previous.isSingleResult() before we process anything.
(2) When we complain about the redefinition, we need to check for the existence of global module fragment. GMF is designed to make C++20 Named modules to be compatible with headers.

[ilya-biryukov]

Thanks! Mostly questions to better understand the spec and clang. Please excuse me if they sound too basic.
I have read the modules section of the standard, but I couldn't find where it specifies whether these redefinition errors should be present or not. Any guidance of where to look at?

Could we add a few examples where legitimate redefinition errors do happen?
(private module fragment?

clang/lib/Sema/SemaTemplate.cpp

8727

This follows what getFoundDecl() does, but still allows to show errors when lookup results contain multiple entities.

A question too: is true that concept is always reachable if it has a reachable using decl?
I wonder whether hasReachableDefinition should be called on UsingShadowDecl instead?

8748

Would we allow redefinitions inside the same global module fragment?

module;

template<class T> concept ok = true;
template<class T> concept ok = true; // should be an error.

could we add a test for that?
or do we only mark declarations coming from actual pcm files as having as owning module fragment?

[ChuanqiXu]

• Address comment.
• Add Sema::CheckRedefinitionInModule

[ChuanqiXu]

In D130614#3681881, @ilya-biryukov wrote:

Thanks! Mostly questions to better understand the spec and clang. Please excuse me if they sound too basic.
I have read the modules section of the standard, but I couldn't find where it specifies whether these redefinition errors should be present or not. Any guidance of where to look at?

This is primarily at: http://eel.is/c++draft/basic.def.odr#14:

[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, ...

... (following off is about the ODR checks)

So except the ODR checking, the redefinition errors should be present if:

• The redefinitions lives in one TU. or,
• Any of the redefinitions get attached to a named modules.

So we can get some examples:

---

// foo.cpp
template <class T, class U>
concept same_as = __is_same(T, U);
template <class T, class U>
concept same_as = __is_same(T, U);

This is disallowed. Since there are definitions of same_as in the same TU for foo.cpp. Note that headers won't present a TU. So the following one is disallowed too:

// foo.h
template <class T, class U>
concept same_as = __is_same(T, U);
// foo.cpp
#include "foo.h"
#include "foo.h"

The definitions lives in the same TU of foo.cpp too. So this is problematic.

---

Note that a named module unit (*.cppm files) presents a TU. So the following one is disallowed:

C++
// foo.h
template <class T, class U>
concept same_as = __is_same(T, U);
// foo.cppm
module;
#include "foo.h"
#include "foo.h"
export module foo;

Since there are two definitions in the same TU of foo.cppm.

---

And the following one should be allowed:

C++
// foo.h
template <class T, class U>
concept same_as = __is_same(T, U);
// foo.cppm
module;
#include "foo.h"
export module foo;
export using :: same_as;
// use.cpp
#include "foo.h"
import foo;
template <class T> void foo()
  requires same_as<T, int>
{}

This is valid since the two definitions lives in two TUs (use.cpp and foo.cppm) and neither of them lives in named modules (the same_as definition in foo.cppm lives in global module instead of a named module).

---

And this one is disallowed

C++
// foo.h
template <class T, class U>
concept same_as = __is_same(T, U);
// foo.cppm
export module foo;
export template <class T, class U>
concept same_as = __is_same(T, U);
// use.cpp
#include "foo.h"
import foo;
template <class T> void foo()
  requires same_as<T, int>
{}

Although the two definitions live in 2 TUs, but one of them is attached to the named module foo. So it is problematic.

---

Overall, I think the design intuition as follows:
the biggest different of C++20 Named Modules between clang modules (or the standardized one, header units) is that the C++ Named Modules owns a unique TU, while the design of clang header modules (or header units) tries to mimic headers as much as they can. However, there are full of legacy codes using headers. The C++20 Named modules are hard to use in real projects if they can't be compatible with existing headers. So here is the global module fragment, which is designed for compatibility.
It might be easier to understand the rules now.

I implemented Sema::CheckRedefinitionInModule by the way to mimic the rules. Although there is a FIXME, I guess the FIXME itself is not too bad. At least it won't make things worse : )

Could we add a few examples where legitimate redefinition errors do happen?

Yeah, added.

(private module fragment?

In the above wording, there is a requirement reachable. But the declarations in the private module fragment is not reachable to the users of the module. So it won't be a legitimate redefinition error.

clang/lib/Sema/SemaTemplate.cpp

8727

This follows what getFoundDecl() does, but still allows to show errors when lookup results contain multiple entities.

Yeah, this is much better.

is true that concept is always reachable if it has a reachable using decl?

Yes.

I wonder whether hasReachableDefinition should be called on UsingShadowDecl instead?

We don't need too.

8748

I've added this as D.cppm in the tests.

[ChuanqiXu]

BTW, I've sent a NFC test to show the current behavior for functions, classes and variables are basically fine: https://github.com/llvm/llvm-project/commit/fe1887da36c63f64903de112f2a8e88f973318fa

[ilya-biryukov]

Thanks for the thorough explanation and testing.
LGTM.

clang/lib/Sema/SemaDecl.cpp

1738

NIT: maybe rename to IsRedefinitionInModule?
I would expect Check... to produce a diagnostic itself. Having Is... in the name also removes any confusion about whether the true as a return value indicates an error or not.

1747

NIT: maybe accept the modules directly?

clang/test/Modules/merge-concepts.cppm

23

NIT on wording

[ChuanqiXu]

Address comments.

[ChuanqiXu]

In D130614#3685142, @ilya-biryukov wrote:

Thanks for the thorough explanation and testing.
LGTM.

Thanks for reviewing!

clang/lib/Sema/SemaDecl.cpp

1747

I feel like the current style saves more typing.

[ChuanqiXu]

This is not automated closed by https://github.com/llvm/llvm-project/commit/4d9251bd780d20eebbcb124608b36a69787d5575 due to a typo.

clang/include/clang/Basic/Module.h

public:
   /// could not be set during module creation.
   void setParent(Module *M) {
     assert(!Parent);
     Parent = M;
     Parent->SubModuleIndex[Name] = Parent->SubModules.size();
     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 ||
            Kind == ModulePartitionImplementation;
   }
 
   /// Is this module a header unit.
   bool isHeaderUnit() const { return Kind == ModuleHeaderUnit; }

clang/include/clang/Sema/Sema.h

void FindAssociatedClassesAndNamespaces(SourceLocation InstantiationLoc,
                                    AssociatedClassSet &AssociatedClasses);
 
   void FilterLookupForScope(LookupResult &R, DeclContext *Ctx, Scope *S,
                             bool ConsiderLinkage, bool AllowInlineNamespace);
 
   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);
   //@}
 
   /// Attempts to produce a RecoveryExpr after some AST node cannot be created.
   ExprResult CreateRecoveryExpr(SourceLocation Begin, SourceLocation End,
                                 ArrayRef<Expr *> SubExprs,
                                 QualType T = QualType());

clang/lib/Sema/SemaDecl.cpp

if (CheckRedeclarationModuleOwnership(New, Old))
     return true;
 
   if (CheckRedeclarationExported(New, Old))
     return true;
 
   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) ||
          isa<UnresolvedUsingValueDecl>(D);
 }
 
 /// Removes using shadow declarations from the lookup results.
 static void RemoveUsingDecls(LookupResult &R) {

clang/lib/Sema/SemaTemplate.cpp

 
 void Sema::CheckConceptRedefinition(ConceptDecl *NewDecl,
                                     LookupResult &Previous, bool &AddToScope) {
   AddToScope = true;
 
   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)
         << NewDecl->getDeclName();
     notePreviousDefinition(Old, NewDecl->getLocation());
     AddToScope = false;
     return;
   }
   // Check if we can merge with a concept declaration.
   bool IsSame = Context.isSameEntity(NewDecl, OldConcept);
   if (!IsSame) {
     Diag(NewDecl->getLocation(), diag::err_redefinition_different_concept)
         << NewDecl->getDeclName();
     notePreviousDefinition(OldConcept, NewDecl->getLocation());
     AddToScope = false;
     return;
   }
-  if (hasReachableDefinition(OldConcept)) {
+  if (hasReachableDefinition(OldConcept) &&
+      IsRedefinitionInModule(NewDecl, OldConcept)) {
     Diag(NewDecl->getLocation(), diag::err_redefinition)
         << NewDecl->getDeclName();
     notePreviousDefinition(OldConcept, NewDecl->getLocation());
     AddToScope = false;
     return;
   }
   if (!Previous.isSingleResult()) {
     // FIXME: we should produce an error in case of ambig and failed lookups.
     //        Other decls (e.g. namespaces) also have this shortcoming.
     return;

clang/test/Modules/merge-concepts.cppm

+// 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>
+{}