[Sema] Return primary merged decl as canonical for concepts

[ilya-biryukov]

Summary

Otherwise we get invalid results for ODR checks. See changed test for an
example: despite the fact that we merge the first concept, its uses
were considered different by Profile, leading to redefinition errors.

After this change, canonical decl for a concept can come from a
different module and may not be visible. This behavior looks suspicious,
but does not break any tests. We might want to add a mechanism to make
the canonical concept declaration visible if we find code that relies on
this invariant.

Also change the order of includes in the test. Importing a moduralized
header before its textual part causes the include guard macro to be
exported and the corresponding #include becomes a no-op.

[ilya-biryukov]

I wonder whether the right fix for this should generalize to Mergeable<T>.
The fact that Mergeable::getFirstDecl checks if the decl is coming from the AST file before looking up its primary merged decl is probably a performance optimization, not a deliberate semantic.

[ChuanqiXu]

In D130585#3680189, @ilya-biryukov wrote:

I wonder whether the right fix for this should generalize to Mergeable<T>.
The fact that Mergeable::getFirstDecl checks if the decl is coming from the AST file before looking up its primary merged decl is probably a performance optimization, not a deliberate semantic.

Agreed. I think we could omit the isFromASTFile part. But if we want to generalize to Mergeable<T>, we need to add much more tests. I think we could make it in future patches.

[ChuanqiXu]

I've added a C++20 Named Modules tests for this (and some trivial change) in D130614.

[ilya-biryukov]

• set canonical decl as primary merged, add tests for the relevant failure

[ilya-biryukov]

In D130585#3681397, @ChuanqiXu wrote:

In D130585#3680189, @ilya-biryukov wrote:

I wonder whether the right fix for this should generalize to Mergeable<T>.
The fact that Mergeable::getFirstDecl checks if the decl is coming from the AST file before looking up its primary merged decl is probably a performance optimization, not a deliberate semantic.

Agreed. I think we could omit the isFromASTFile part. But if we want to generalize to Mergeable<T>, we need to add much more tests. I think we could make it in future patches.

Totally agree, the Mergable change needs to be tested more thoroughly.
I have incorporated the getCanonicalDecl() call from your patch here and added test for the corresponding failure to merge-concepts.cpp too.
Otherwise my patch did not fix the failure I was encountering in practice (after I double-checked).

clang/include/clang/AST/DeclTemplate.h

SourceRange getSourceRange() const override LLVM_READONLY {
     return SourceRange(getTemplateParameters()->getTemplateLoc(),
                        ConstraintExpr->getEndLoc());
   }
 
   bool isTypeConcept() const {
     return isa<TemplateTypeParmDecl>(getTemplateParameters()->getParam(0));
   }
 
-  ConceptDecl *getCanonicalDecl() override { return getFirstDecl(); }
-  const ConceptDecl *getCanonicalDecl() const { return getFirstDecl(); }
+  ConceptDecl *getCanonicalDecl() override {
+    return cast<ConceptDecl>(getPrimaryMergedDecl(this));
+  }
+  const ConceptDecl *getCanonicalDecl() const {
+    return const_cast<ConceptDecl *>(this)->getCanonicalDecl();
+  }
 
   // Implement isa/cast/dyncast/etc.
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
   static bool classofKind(Kind K) { return K == Concept; }
 
   friend class ASTReader;
   friend class ASTDeclReader;
   friend class ASTDeclWriter;

clang/lib/Sema/SemaTemplate.cpp

if (hasReachableDefinition(OldConcept)) {
     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);
+  // We unwrap canonical decl late to check for module visibility.
+  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.cpp

 
 //--- modules.map
 module "library" {
   export *
   module "concepts" {
     export *
     header "concepts.h"
   }
+  module "compare" {
+    export *
+    header "compare.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);
+concept same_as_impl = __is_same(T, U);
 
+template <class T, class U>
+concept same_as = same_as_impl<T, U> && same_as_impl<U, T>;
 #endif // SAME_AS_H
 
+
+//--- compare.h
+#ifndef COMPARE_H
+#define COMPARE_H
+
+#include "same_as.h"
+#include "concepts.h"
+
+template <class T> void foo()
+  requires same_as<T, int>
+{}
+#endif // COMPARE_H
+
 //--- format.h
 #ifndef FORMAT_H
 #define FORMAT_H
 
-#include "concepts.h"
 #include "same_as.h"
+#include "concepts.h"
 
-template <class T> void foo()
+template <class T> void bar()
   requires same_as<T, int>
 {}
 
-#endif // FORMAT_H
+#endif // FORMAT_H