[clang] Propagate requires-clause from constructor template to implicit deduction guide

[nridge]

Summary

Fixes https://github.com/clangd/clangd/issues/890

[nridge]

Revise fix approach

[nridge]

Adding some potential reviewers.

I'm open to testing this some way other than through clangd, but note that, while the issue is not in AST serialization code, it only trips an assertion after a serialization round-trip.

[kadircet]

As for testing strategy, after this change the AST should look different. You can probably modify clang/test/SemaTemplate/deduction-guide.cpp to check for existence of requires clauses in the implicitly generated function templates for guides.

[nridge]

Now with clang test

[sammccall]

Fantastic, thank you!

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

691

Having solved and tested this in the AST, this test is arguably redundant (it's not like there's anything interesting going on in de/serialization).

Happy if you want to keep it as a regression test, maybe leave a comment that we used to crash here or linking to the bug?

clang/test/SemaTemplate/deduction-guide.cpp

219 ↗

(On Diff #416827)

Before your patch, clang requires this explicit deduction guide in order to accept the code.
Without it, it says the two implicit deduction guides from the constructor are ambiguous. But this is precisely because they lack the constraints!

After your patch, clang accepts this code without the explicit deduction guide.
So I think omitting the guide (and having no expected-error...) makes it a better test - in addition to verifying the guides can be dumped, it checks they are used.

(Kadir and I spent some time poking at variations of this example to understand how CTAD works in the AST, so thanks for this!)

[nridge]

Remove clangd test, remove explicit deduction guide from clang test

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

Annotations Test(R"cpp(
     [[Define]](main) // error-ok
   )cpp");
   auto TU = TestTU::withCode(Test.code());
   EXPECT_THAT(*TU.build().getDiagnostics(),
               ElementsAre(AllOf(Diag(Test.range(), "'main' must return 'int'"),
                                 Not(WithFix(_)))));
 }
 
+TEST(DiagnosticsTest, Issue890) {
+  Annotations Header(R"cpp(
+template<char = 'x'> struct S;
+
+template<char> struct S {
+    template<typename U> requires(false) S(U);
+    template<typename U> requires(true) S(U);
+};
+
+template<typename T> S(T) -> S<>;
+  )cpp");
+  Annotations Main(R"cpp(
+    #include "a.h"
+    void foo() {
+     S(0); // Simply ::x; is enough to crash!
+    }
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.ExtraArgs.push_back("-std=c++20");
+  TU.AdditionalFiles = {{"a.h", std::string(Header.code())}};
+  EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
+}
+
 TEST(ClangdTest, MSAsm) {
   // Parsing MS assembly tries to use the target MCAsmInfo, which we don't link.
   // We used to crash here. Now clang emits a diagnostic, which we filter out.
   llvm::InitializeAllTargetInfos(); // As in ClangdMain
   auto TU = TestTU::withCode("void fn() { __asm { cmp cl,64 } }");
   TU.ExtraArgs = {"-fms-extensions"};
   EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
 }

clang/lib/Serialization/ASTReaderDecl.cpp

static void inheritDefaultTemplateArguments(ASTContext &Context,
   auto *FromTP = From->getTemplateParameters();
   auto *ToTP = To->getTemplateParameters();
   assert(FromTP->size() == ToTP->size() && "merged mismatched templates?");
 
   for (unsigned I = 0, N = FromTP->size(); I != N; ++I) {
     NamedDecl *FromParam = FromTP->getParam(I);
     NamedDecl *ToParam = ToTP->getParam(I);
 
+    if (FromParam == ToParam)
+      continue;
+
     if (auto *FTTP = dyn_cast<TemplateTypeParmDecl>(FromParam))
       inheritDefaultTemplateArgument(Context, FTTP, ToParam);
     else if (auto *FNTTP = dyn_cast<NonTypeTemplateParmDecl>(FromParam))
       inheritDefaultTemplateArgument(Context, FNTTP, ToParam);
     else
       inheritDefaultTemplateArgument(
               Context, cast<TemplateTemplateParmDecl>(FromParam), ToParam);
   }