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

[ilya-biryukov]

Summary

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

[erichkeane]

I don't see anything to be concerned about here, but I'd like a modules person to take a look. @ChuanqiXu any chance you can confirm here? (or @iains ?)

[erichkeane]

I don't see anything to be concerned about here, but I'd like a modules person to take a look. @ChuanqiXu any chance you can confirm here? (or @iains ?)

Also, would like to see a release note on this too!

[ilya-biryukov]

@erichkeane please let me know if you're the right person to review this.

clang/lib/Sema/SemaTemplate.cpp

8661–8662

Not sure if reporting the redefinition if isSameEntity returns false is the right approach here.
This leads to errors on definitions of something like:

// foo.h
template <class T> concept A = false;

// bar.h
template <class T, class U> concept A = true;

#include "foo.h"
#include "bar.h" // <-- redefinition error shown here

The alternative is the "ambiguous reference" error on the use of the concept. I will check what happens for variables and typedefs in that case and follow the same pattern.

[ilya-biryukov]

• Update code to match how typedefs behave

[ilya-biryukov]

I haven't seen your reply before posting my initial comments. Many thanks for a very quick turnaroud on this!
Will wait for module folks to approve.

clang/lib/Sema/SemaTemplate.cpp

8661–8662

Clang tends to do "ambiguous reference" instead of the behavior we had in this patch.
I have updated it to be consistent with what typedefs and other symbols produce.

[ilya-biryukov]

• remove leftover test from previous version

[ChuanqiXu]

Thanks for filing that issue. I wanted to add that myself before. It should be better to change isVisible and hasVisibleDefinition to isReachable and hasReachableDefinition. The definition of reachability comes from C++20 Modules and it shouldn't affect Clang Modules.

And would you like to add an example for C++20 Modules? It should be something like:

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

Also I feel it would be better to rewrite the current tests into the above style by split-file.

clang/lib/Sema/SemaTemplate.cpp

8679

8689

[ilya-biryukov]

• Update code to match how typedefs behave
• remove leftover test from previous version
• Add test for C++20 modules, rewrite original test with split-file
• use isReachable, do not call makeMergedDefinitionVisible

[ilya-biryukov]

Many thanks! I didn't know about split-file, it's much nicer indeed!

Also incorporating feedback from @rsmith and removing the call to makeMergedDefinitionVisible. Keeping just setPrimaryMergedDecl is enough here.
Richard's reply from the email exchange:

I don't think that makeMergedDefinitionVisible is quite right; that's intended to solve a different problem. Specifically, when we have a repeated class definition in a single parse:

// in module X
class A { void f(); };

// in module Y
class A { void f(); };

Here, we first see and parse the class definition of A in module X. That then becomes "the" definition of A, for the purposes of this compilation. Then when we see another definition of the class, we choose to skip it because we already have a definition, and Clang wants there to only be one definition of each class. So we end up behaving as if we parsed this:

// in module X
class A { void f(); };

// in module Y
class A;

... but we still need to track that class A has a complete definition if only Y is imported, so that A::f can be used for example. And that's what makeMergedDefinitionVisible does: it says that A should be considered as having a complete definition when Y is imported, even though the definition actually lives in X.

For concepts I think the situation is different: we're not going to skip the "body" of the concept and treat

// module X
template<typename T> concept bool C = true;

// module Y
template<typename T> concept bool C = true;

as if it were


// module X
template<typename T> concept bool C = true;

// module Y
template<typename T> concept bool C; // forward declaration?

... because there's no such thing as a forward declaration of a concept. Instead, I think we should just be calling ASTContext::setPrimaryMergedDecl to say that the C in module Y is a redeclaration of the C in module X, so that a name lookup that finds both is not ambiguous. It shouldn't matter which one we actually pick, because they are the same.

[ilya-biryukov]

PS I will make sure to look at the patches you sent my way this week.
Wanted to do it earlier, but have been having some personal emergencies I needed to take care of.

[ilya-biryukov]

• Always call PushToScopeChains. This is the right behavior after we stopped calling makeMergedDefinitionVisible

[rsmith]

clang/lib/Sema/SemaTemplate.cpp

8679

I don't think that's right: the C++20 modules rule is that at most one definition is permitted per translation unit, and the Clang header modules rule is that a definition is only permissible if no definition is visible (Clang header modules effectively behave as if they're part of the same translation unit for the purposes of this check). For now, checking for a visible definition seems better than checking for a reachable one, until we implement the C++20 rule in general.

8695–8697

It'd be nice to have a third diagnostic for the case where there's a known declaration that doesn't match. That is:

• If the name is used for something that's not a concept, diagnose with err_redefinition_different_kind.
• If the name is used for a visible and matching concept, diagnose with err_redefinition.
• If the name is used for a non-matching concept, regardless of whether it's visible or even reachable, produce some new mismatch error.
• Otherwise, if the name is used for a matching concept that's not visible, merge.

[ChuanqiXu]

clang/lib/Sema/SemaTemplate.cpp

8679

For clang modules, the reachability should be the visibility: https://github.com/llvm/llvm-project/blob/9cb00e7133ea3da6a49ade95e3708240c2aaae39/clang/lib/Sema/SemaLookup.cpp#L1903-L1905. So I think it is better to check for a reachable definition here. Otherwise we might not be able to handle the following case:

import M;
export template <class T>
concept ConflictingConcept = true; // There is another reachable but not visible definition for ConflictingConcept.

[ChuanqiXu]

LGTM if the comments about diagnostic get addressed.

[ilya-biryukov]

• Update diagnostics per Richard's suggestions, add tests

[ilya-biryukov]

Sorry, was out again due to an emergency. So I wanted to give a chance for any last comments despite having an LGTM
I plan to land this on Monday.

clang/lib/Sema/SemaTemplate.cpp

8679

Keeping the reachability for now. Happy to change if we run into trouble later.

8695–8697

Done. PTAL at the wording for the new error.

regardless of whether it's visible or even reachable, produce some new mismatch error.

I have removed the check visibility of conflicting declarations completely.
I assume it does not make sense to distinguish between symbols of the same kind or different kinds as both are ODR violations, right?

There is a corner case where we don't report a conflict now: lookup with >1 result and a matching non-visible concept as a representative decl. I did not find a good error for this case, and there are other symbols that have this problem, e.g. namespaces.

It's not easy to trigger this, but I bet it's possible.

clang/include/clang/Sema/Sema.h

 
   //===--------------------------------------------------------------------===//
   // C++ Concepts
   //===--------------------------------------------------------------------===//
   Decl *ActOnConceptDefinition(
       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,
                          Scope *BodyScope);
   void ActOnFinishRequiresExpr();
   concepts::Requirement *ActOnSimpleRequirement(Expr *E);
   concepts::Requirement *ActOnTypeRequirement(
       SourceLocation TypenameKWLoc, CXXScopeSpec &SS, SourceLocation NameLoc,

clang/lib/AST/ASTContext.cpp

if (const auto *NamespaceX = dyn_cast<NamespaceDecl>(X)) {
     const auto *NamespaceY = cast<NamespaceDecl>(Y);
     return NamespaceX->isInline() == NamespaceY->isInline();
   }
 
   // Identical template names and kinds match if their template parameter lists
   // and patterns match.
   if (const auto *TemplateX = dyn_cast<TemplateDecl>(X)) {
     const auto *TemplateY = cast<TemplateDecl>(Y);
+    // FIXME: for C++20 concepts, check their requirements are the same.
     return isSameEntity(TemplateX->getTemplatedDecl(),
                         TemplateY->getTemplatedDecl()) &&
            isSameTemplateParameterList(TemplateX->getTemplateParameters(),
                                        TemplateY->getTemplateParameters());
   }
 
   // Fields with the same name and the same type match.
   if (const auto *FDX = dyn_cast<FieldDecl>(X)) {

clang/lib/Sema/SemaTemplate.cpp

if (NewDecl->hasAssociatedConstraints()) {
     // A concept shall not have associated constraints.
     Diag(NameLoc, diag::err_concept_no_associated_constraints);
     NewDecl->setInvalidDecl();
   }
 
   // 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);
+  if (AddToScope)
     PushOnScopeChains(NewDecl, S);
   return NewDecl;
 }
 
+void Sema::CheckConceptRedefinition(ConceptDecl *NewDecl,
+                                    LookupResult &Previous, bool &AddToScope) {
+  AddToScope = true;
+  if (Previous.empty())
+    return;
+  // Check if there is a concept declaration we can merge with.
+  auto *PrevDef =
+      Previous.isSingleResult() ? Previous.getAsSingle<ConceptDecl>() : nullptr;
+  if (PrevDef && !hasVisibleDefinition(PrevDef) &&
+      Context.isSameEntity(NewDecl, PrevDef)) {
+    Context.setPrimaryMergedDecl(NewDecl, PrevDef);
+    makeMergedDefinitionVisible(PrevDef);
+    AddToScope = false;
+    return;
+  }
+  // Filter out non-visible declaration to avoid spurious redefinition errors.
+  auto F = Previous.makeFilter();
+  while (F.hasNext()) {
+    if (!isVisible(F.next())) {
+      F.erase();
+    }
+  }
+  F.done();
+  // We report redefinition if the lookup is not empty after filters.
+  if (Previous.empty())
+    return;
+  auto *Old = Previous.getRepresentativeDecl();
+  Diag(NewDecl->getLocation(), isa<ConceptDecl>(Old)
+                                   ? diag::err_redefinition
+                                   : diag::err_redefinition_different_kind)
+      << NewDecl->getDeclName();
+  Diag(Old->getLocation(), diag::note_previous_definition);
+}
+
 /// \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>();
 
   if (FunctionDecl *FD = dyn_cast<FunctionDecl>(D))
     FD->setInlineSpecified(false);

clang/test/Modules/Inputs/merge-concepts/concepts.h

+#ifndef SAMEAS_CONCEPTS_H_
+#define SAMEAS_CONCEPTS_H_
+
+#include "same_as.h"
+
+template <class T>
+concept ConflictingConcept = true;
+
+#endif // SAMEAS_CONCEPTS_H

clang/test/Modules/Inputs/merge-concepts/conflicting.h

+#ifndef CONFLICTING_H
+#define CONFLICTING_H
+
+template <class T, class U = int>
+concept ConflictingConcept = true;
+
+#endif // CONFLICTING_H

clang/test/Modules/Inputs/merge-concepts/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

clang/test/Modules/Inputs/merge-concepts/modules.map

+module "library" {
+  export *
+  module "concepts" {
+    export *
+    header "concepts.h"
+  }
+  module "format" {
+    export *
+    header "format.h"
+  }
+  module "conflicting" {
+    export *
+    header "conflicting.h"
+  }
+}

clang/test/Modules/Inputs/merge-concepts/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

clang/test/Modules/merge-concepts.cpp

+// RUN: rm -rf %t.dir
+// RUN: mkdir %t.dir
+//
+// RUN: %clang_cc1 -xc++ -std=c++20 -fmodules -fmodule-name=library \
+// RUN:     -emit-module %S/Inputs/merge-concepts/modules.map \
+// RUN:     -o %t.dir/module.pcm
+//
+//
+// RUN: %clang_cc1 -xc++ -std=c++20 -fmodules -fmodule-file=%t.dir/module.pcm  \
+// RUN:     -fmodule-map-file=%S/Inputs/merge-concepts/modules.map \
+// RUN:     -I%S/Inputs/merge-concepts \
+// RUN:     -fsyntax-only -verify %s
+
+#include "concepts.h"
+#include "conflicting.h"
+#include "format.h"
+
+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}}

clang/test/Modules/merge-concepts.m

+// RUN: rm -rf %t.dir
+// RUN: mkdir %t.dir
+//
+// RUN: %clang_cc1 -xc++ -std=c++20 -fmodules -fmodule-name=library \
+// RUN:     -emit-module %S/Inputs/merge-concepts/modules.map \
+// RUN:     -o %t.dir/module.pcm
+//
+// Note that this file merely checks the module compiles. The contents of this
+// file are never read by the compiler.