[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

8731

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?

8755

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

8731

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.

8755

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 ↗

(On Diff #448246)

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 ↗

(On Diff #448246)

NIT: maybe accept the modules directly?

clang/test/Modules/merge-concepts.cppm

22 ↗

(On Diff #448246)

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 ↗

(On Diff #448246)

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/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());
+  NamedDecl *Old = Previous.getRepresentativeDecl();
+  if (auto *USD = dyn_cast<UsingShadowDecl>(Old))
+    Old = USD->getTargetDecl();
+
+  auto *OldConcept = dyn_cast<ConceptDecl>(Old);
   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) &&
+      // When the old decl lives in the GMF and the new decl is same with the
+      // old one, we should merge them instead of complaining.
+      !(OldConcept->getOwningModule() &&
+        OldConcept->getOwningModule()->isGlobalModule())) {
     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;
   }
-  Context.setPrimaryMergedDecl(NewDecl, OldConcept);
+  Context.setPrimaryMergedDecl(NewDecl, OldConcept->getCanonicalDecl());
 }
 
 /// \brief Strips various properties off an implicit instantiation
 /// that has just been explicitly specialized.
 static void StripImplicitInstantiation(NamedDecl *D) {
   D->dropAttr<DLLImportAttr>();
   D->dropAttr<DLLExportAttr>();
 

clang/test/Modules/merge-concepts-in-GMF.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 -emit-module-interface -fprebuilt-module-path=%t %t/Use.cpp -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -fprebuilt-module-path=%t %t/Use2.cpp -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -fprebuilt-module-path=%t %t/Use3.cpp -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -fprebuilt-module-path=%t %t/Use4.cpp -verify -fsyntax-only
+
+//--- foo.h
+template <class T, class U>
+concept same_as = __is_same(T, U);
+
+//--- 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>
+{}