[Modules] Add ODR Check for concepts

[ChuanqiXu]

Summary

Closing https://github.com/llvm/llvm-project/issues/56310

Previously we don't check the contents of concept so it might merge inconsistent results.

Important Note: this patch might break existing code but the behavior might be right.

[ChuanqiXu]

Undo the format test. It breaks the testing.

[erichkeane]

clang/lib/AST/ASTContext.cpp

6569

This isn't necessarily valid here, you did a dyn_cast above.

6570

[ChuanqiXu]

Address comments.

clang/lib/AST/ASTContext.cpp

6569

Oh, my bad. I should use cast. We could find the style at line 6525-6526 and line 6519-6520.

[ChuanqiXu]

clang/lib/AST/ASTContext.cpp

6570

The assertion here should be fine since the '=' part is guaranteed by std: https://eel.is/c++draft/temp.concept#1 and clang would reject it too: https://godbolt.org/z/Tfe493xK7

[ChuanqiXu]

@erichkeane @ilya-biryukov gentle ping~

[erichkeane]

LGTM, thanks!

[erichkeane]

Should have a release note on commit!

[ChuanqiXu]

Thanks for reviewing and noting!

[ChuanqiXu]

Add ReleaseNotes.

[erichkeane]

clang/docs/ReleaseNotes.rst

186

187

[ChuanqiXu]

Address comments.

[ChuanqiXu]

clang/docs/ReleaseNotes.rst

187

Although as a non-native speaker, the current wording looks like the new clang is going to break existing code and what I want to say it is only possible.

I followed your suggestion since I feel it might not be meaningful to argue English with a native speaker : )

[erichkeane]

clang/docs/ReleaseNotes.rst

187

Hmm, you're right about that being a subtlety that I didn't pick up.

We could consider changing 'expected to' to 'likely to', or replace 'is expected to' could be 'may' (or even 'may possibly') depending on your confidence :)

[ChuanqiXu]

Address comments.

clang/docs/ReleaseNotes.rst

- Fixed memory leak due to ``VarTemplateSpecializationDecl`` using
   ``TemplateArgumentListInfo`` instead of ``ASTTemplateArgumentListInfo``.
 - An initializer for a static variable declaration, which is nested
   inside a statement expression in an aggregate initializer, is now
   emitted as a dynamic initializer. Previously the variable would
   incorrectly be zero-initialized. In contexts where a dynamic
   initializer is not allowed this is now diagnosed as an error.
 - Clang now correctly emits symbols for implicitly instantiated constexpr
   template function. Fixes `Issue 55560 <https://github.com/llvm/llvm-project/issues/55560>`_.
+- Clang now checks ODR violations when merging concepts from different modules.
+  Note that this may possibly break existing code, and is done so intentionally.
+  Fixes `Issue 56310 <https://github.com/llvm/llvm-project/issues/56310>`_.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - ``-Wliteral-range`` will warn on floating-point equality comparisons with
   constants that are not representable in a casted value. For example,
   ``(float) f == 0.1`` is always false.
 - ``-Winline-namespace-reopened-noninline`` now takes into account that the
   ``inline`` keyword must appear on the original but not necessarily all

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);
+
+    // ConceptDecl wouldn't be the same if their constraint expression differs.
+    if (const auto *ConceptX = dyn_cast<ConceptDecl>(X)) {
+      const auto *ConceptY = cast<ConceptDecl>(Y);
+      const Expr *XCE = ConceptX->getConstraintExpr();
+      const Expr *YCE = ConceptY->getConstraintExpr();
+      assert(XCE && YCE && "ConceptDecl without constraint expression?");
+      llvm::FoldingSetNodeID XID, YID;
+      XCE->Profile(XID, *this, /*Canonical=*/true);
+      YCE->Profile(YID, *this, /*Canonical=*/true);
+      if (XID != YID)
+        return false;
+    }
+
     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/test/Modules/concept_differ.cpp

+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -x c++ -std=c++20 -fmodules -fmodules-cache-path=%t -fmodule-map-file=%t/module.map %t/foo.cpp -verify
+
+//--- module.map
+module "foo" {
+  export * 
+  header "foo.h"
+}
+module "bar" {
+  export * 
+  header "bar.h"
+}
+
+//--- foo.h
+template <class T>
+concept A = true;
+
+//--- bar.h
+template <class T>
+concept A = false;
+
+//--- foo.cpp
+#include "bar.h"
+#include "foo.h"
+
+template <class T> void foo() requires A<T> {}  // expected-error 1+{{reference to 'A' is ambiguous}}
+                                                // expected-note@* 1+{{candidate found by name lookup}}
+
+int main() {
+  foo<int>();
+  return 0;
+}

clang/test/Modules/concept_differ.cppm

+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -x c++ -std=c++20 %t/A.cppm -I%t -emit-module-interface -o %t/A.pcm
+// RUN: %clang_cc1 -x c++ -std=c++20 %t/B.cppm -I%t -emit-module-interface -o %t/B.pcm
+// RUN: %clang_cc1 -x c++ -std=c++20 -fprebuilt-module-path=%t %t/foo.cpp -verify
+
+//--- foo.h
+template <class T>
+concept A = true;
+
+//--- bar.h
+template <class T>
+concept A = false;
+
+//--- A.cppm
+module;
+#include "foo.h"
+export module A;
+export using ::A;
+
+//--- B.cppm
+module;
+#include "bar.h"
+export module B;
+export using ::A;
+
+//--- foo.cpp
+import A;
+import B;
+
+template <class T> void foo() requires A<T> {}  // expected-error 1+{{reference to 'A' is ambiguous}}
+                                                // expected-note@* 1+{{candidate found by name lookup}}
+
+int main() {
+  foo<int>();
+  return 0;
+}