[Sema] Merge C++20 concept definitions from different modules in same TU

Currently the C++20 concepts are only merged in `ASTReader`, i.e. when
coming from different TU. This can causes ambiguious reference errors when
trying to access the same concept that should otherwise be merged.

Please see the added test for an example.

Note that we currently use `ASTContext::isSameEntity` to check for ODR
violations. However, it will not check that concept requirements match.
The same issue holds for mering concepts from different TUs, I added a
FIXME and filed a GH issue to track this:
https://github.com/llvm/llvm-project/issues/56310

Reviewed By: ChuanqiXu

Differential Revision: https://reviews.llvm.org/D128921

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index c2cabcb..6ff5b8d 100644 (file)
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5774,6 +5774,8 @@ def warn_forward_class_redefinition : Warning<
 def err_redefinition_different_typedef : Error<
   "%select{typedef|type alias|type alias template}0 "
   "redefinition with different types%diff{ ($ vs $)|}1,2">;
+def err_redefinition_different_concept : Error<
+  "redefinition of concept %0 with different template parameters or requirements">;
 def err_tag_reference_non_tag : Error<
   "%select{non-struct type|non-class type|non-union type|non-enum "
   "type|typedef|type alias|template|type alias template|template "

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index de9bde6..b149c24 100644 (file)
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -8260,6 +8260,9 @@ public:
       Scope *S, MultiTemplateParamsArg TemplateParameterLists,
       IdentifierInfo *Name, SourceLocation NameLoc, Expr *ConstraintExpr);
 
+  void CheckConceptRedefinition(ConceptDecl *NewDecl, LookupResult &Previous,
+                                bool &AddToScope);
+
   RequiresExprBodyDecl *
   ActOnStartRequiresExpr(SourceLocation RequiresKWLoc,
                          ArrayRef<ParmVarDecl *> LocalParameters,

diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 95c83eb..1542a07 100644 (file)
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -20,6 +20,7 @@
 #include "clang/AST/TemplateName.h"
 #include "clang/AST/TypeVisitor.h"
 #include "clang/Basic/Builtins.h"
+#include "clang/Basic/DiagnosticSema.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/PartialDiagnostic.h"
 #include "clang/Basic/Stack.h"
@@ -8707,23 +8708,59 @@ Decl *Sema::ActOnConceptDefinition(Scope *S,
   // Check for conflicting previous declaration.
   DeclarationNameInfo NameInfo(NewDecl->getDeclName(), NameLoc);
   LookupResult Previous(*this, NameInfo, LookupOrdinaryName,
-                        ForVisibleRedeclaration);
+                        forRedeclarationInCurContext());
   LookupName(Previous, S);
-
   FilterLookupForScope(Previous, DC, S, /*ConsiderLinkage=*/false,
                        /*AllowInlineNamespace*/false);
-  if (!Previous.empty()) {
-    auto *Old = Previous.getRepresentativeDecl();
-    Diag(NameLoc, isa<ConceptDecl>(Old) ? diag::err_redefinition :
-         diag::err_redefinition_different_kind) << NewDecl->getDeclName();
-    Diag(Old->getLocation(), diag::note_previous_definition);
-  }
+  bool AddToScope = true;
+  CheckConceptRedefinition(NewDecl, Previous, AddToScope);
 
   ActOnDocumentableDecl(NewDecl);
-  PushOnScopeChains(NewDecl, S);
+  if (AddToScope)
+    PushOnScopeChains(NewDecl, S);
   return NewDecl;
 }
 
+void Sema::CheckConceptRedefinition(ConceptDecl *NewDecl,
+                                    LookupResult &Previous, bool &AddToScope) {
+  AddToScope = true;
+
+  if (Previous.empty())
+    return;
+
+  auto *OldConcept = dyn_cast<ConceptDecl>(Previous.getRepresentativeDecl());
+  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)) {
+    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);
+}
+
 /// \brief Strips various properties off an implicit instantiation
 /// that has just been explicitly specialized.
 static void StripImplicitInstantiation(NamedDecl *D) {

diff --git a/clang/test/Modules/merge-concepts-cxx-modules.cpp b/clang/test/Modules/merge-concepts-cxx-modules.cpp
new file mode 100644 (file)
index 0000000..3d4f843
--- /dev/null
+++ b/clang/test/Modules/merge-concepts-cxx-modules.cpp
@@ -0,0 +1,46 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/same_as.cppm -o %t/same_as.pcm
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -fprebuilt-module-path=%t %t/concepts.cppm -o %t/concepts.pcm
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -fprebuilt-module-path=%t %t/format.cppm -o %t/format.pcm
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/conflicting.cppm -o %t/conflicting.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/Use.cppm -fsyntax-only -verify
+
+//--- same_as.cppm
+export module same_as;
+export template <class T, class U>
+concept same_as = __is_same(T, U);
+
+//--- concepts.cppm
+export module concepts;
+export import same_as;
+
+export template <class T>
+concept ConflictingConcept = true;
+
+//--- format.cppm
+
+export module format;
+export import concepts;
+export import same_as;
+
+export template <class T> void foo()
+  requires same_as<T, int>
+{}
+
+//--- conflicting.cppm
+export module conflicting;
+export template <class T, class U = int>
+concept ConflictingConcept = true;
+
+//--- Use.cppm
+import format;
+import conflicting;
+
+template <class T> void foo()
+  requires same_as<T, T>
+{}
+ConflictingConcept auto x = 10; // expected-error {{reference to 'ConflictingConcept' is ambiguous}}
+                                // expected-note@* 2 {{candidate}}

diff --git a/clang/test/Modules/merge-concepts-redefinition-error.cpp b/clang/test/Modules/merge-concepts-redefinition-error.cpp
new file mode 100644 (file)
index 0000000..844a483
--- /dev/null
+++ b/clang/test/Modules/merge-concepts-redefinition-error.cpp
@@ -0,0 +1,57 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -xc++ -std=c++20 -fmodules -fmodule-name=library \
+// RUN:     -emit-module %t/modules.map \
+// RUN:     -o %t/module.pcm \
+// RUN:     -verify
+//
+//--- modules.map
+module "library" {
+       export *
+       module "concepts" {
+               export *
+               header "concepts.h"
+       }
+       module "conflicting" {
+               export *
+               header "conflicting.h"
+       }
+}
+
+//--- concepts.h
+#ifndef CONCEPTS_H_
+#define CONCEPTS_H_
+
+template <class T>
+concept ConflictingConcept = true;
+
+template <class T, class U>
+concept same_as = __is_same(T, U);
+
+template<class T> concept truec = true;
+
+int var;
+
+#endif // SAMEAS_CONCEPTS_H
+
+//--- conflicting.h
+#ifndef CONFLICTING_H
+#define CONFLICTING_H
+
+#include "concepts.h"
+
+template <class T, class U = int>
+concept ConflictingConcept = true; // expected-error {{redefinition of concept 'ConflictingConcept' with different template}}
+                                   // expected-note@* {{previous definition}}
+
+int same_as; // expected-error {{redefinition of 'same_as' as different kind of symbol}}
+             // expected-note@* {{previous definition}}
+
+template<class T> concept var = false; // expected-error {{redefinition of 'var' as different kind of symbol}}
+                                       // expected-note@* {{previous definition}}
+
+template<class T> concept truec = true; // expected-error {{redefinition of 'truec'}}
+                                        // expected-note@* {{previous definition}}
+#endif // CONFLICTING_H

diff --git a/clang/test/Modules/merge-concepts.cpp b/clang/test/Modules/merge-concepts.cpp
new file mode 100644 (file)
index 0000000..9242f27
--- /dev/null
+++ b/clang/test/Modules/merge-concepts.cpp
@@ -0,0 +1,65 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -xc++ -std=c++20 -fmodules -fmodule-name=library \
+// RUN:     -emit-module %t/modules.map \
+// RUN:     -o %t/module.pcm
+//
+//
+// RUN: %clang_cc1 -xc++ -std=c++20 -fmodules -fmodule-file=%t/module.pcm  \
+// RUN:     -fmodule-map-file=%t/modules.map \
+// RUN:     -fsyntax-only -verify %t/use.cpp
+//
+//--- use.cpp
+// expected-no-diagnostics
+
+#include "concepts.h"
+#include "format.h"
+
+template <class T> void foo()
+  requires same_as<T, T>
+{}
+
+//--- modules.map
+module "library" {
+       export *
+       module "concepts" {
+               export *
+               header "concepts.h"
+       }
+       module "format" {
+               export *
+               header "format.h"
+       }
+}
+
+//--- concepts.h
+#ifndef SAMEAS_CONCEPTS_H_
+#define SAMEAS_CONCEPTS_H_
+
+#include "same_as.h"
+
+#endif // SAMEAS_CONCEPTS_H
+
+//--- same_as.h
+#ifndef SAME_AS_H
+#define SAME_AS_H
+
+template <class T, class U>
+concept same_as = __is_same(T, U);
+
+#endif // SAME_AS_H
+
+//--- format.h
+#ifndef FORMAT_H
+#define FORMAT_H
+
+#include "concepts.h"
+#include "same_as.h"
+
+template <class T> void foo()
+  requires same_as<T, int>
+{}
+
+#endif // FORMAT_H
\ No newline at end of file