[clang] Construct ExprRequirement with SubstitutionDiagnostic on SubstFailure

[zyounan]

Summary

We're expecting a SubstitutionDiagnostic in diagnoseUnsatisfiedRequirement
if the status of ExprRequirement is SubstFailure. Previously, the Requirement
was created with Expr on SubstFailure by mistake, which could lead to the
assertion failure in the subsequent diagnosis.

Fixes https://github.com/clangd/clangd/issues/1726
Fixes https://github.com/llvm/llvm-project/issues/64723
Fixes https://github.com/llvm/llvm-project/issues/64172

In addition, this patch also fixes an invalid test from D129499.

[zyounan]

Rebase and format

[zyounan]

Adapt to D157554

[zyounan]

clang/lib/Sema/SemaExprCXX.cpp

9079

I'm adding another assertion here following up on D157554: If IDC is nullptr somehow, we will fall into the cast statement in the else block and crash anyway.

[zyounan]

Add release note

[zyounan]

Rebase and Remove the stray status

[zyounan]

Gently ping~

[erichkeane]

Hi, sorry for the delay in reviewing, I'm just coming back from an extended vacation. This looks alright, except for the test.

clang/test/SemaCXX/concept-crash-on-diagnostic.cpp

27

Please don't do this, actually write out the remaining diagnostics. Same with the notes above.

Additionally, please use the 'bookmark' feature of verify-consumer to make sure the diagnostics/notes are in proper order (which makes it easier to figure out when reviewing).

[zyounan]

Expand diagnostics in the test.

[zyounan]

Thanks for the review!

clang/test/SemaCXX/concept-crash-on-diagnostic.cpp

27

Thanks for reminding me. Will do that.

[zyounan]

Rebase to main

[erichkeane]

Clean up the test layout a little as a part of the commit, otherwise, LGTM!

clang/test/SemaCXX/concept-crash-on-diagnostic.cpp

29

So I typically don't do the 'error' with a bookmark, just put the 'error' next to the line that causes the error, and bookmark the note. It makes it easier to read.

[zyounan]

Thanks again for the review!

clang/test/SemaCXX/concept-crash-on-diagnostic.cpp

29

Good suggestion!

[zyounan]

Address one last nit comment.

clang/include/clang/AST/ExprConcepts.h

 //===- ExprConcepts.h - C++2a Concepts expressions --------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 /// \file
 /// Defines Expressions and AST nodes for C++2a concepts.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_AST_EXPRCONCEPTS_H
 #define LLVM_CLANG_AST_EXPRCONCEPTS_H
 
-#include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTConcept.h"
+#include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
-#include "clang/AST/DeclarationName.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/DeclarationName.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/STLFunctionalExtras.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/TrailingObjects.h"
-#include <utility>
 #include <string>
+#include <utility>
 
 namespace clang {
 class ASTStmtReader;
 class ASTStmtWriter;
 
 /// \brief Represents the specialization of a concept - evaluates to a prvalue
 /// of type bool.
 ///
const ASTConstraintSatisfaction &getConstraintSatisfaction() const {
     return *Satisfaction;
   }
 
   static bool classof(const Requirement *R) {
     return R->getKind() == RK_Nested;
   }
 };
 
+using EntityPrinter = llvm::function_ref<void(llvm::raw_ostream &)>;
+
+/// \brief create a Requirement::SubstitutionDiagnostic with only a
+/// SubstitutedEntity and DiagLoc using Sema's allocator.
+Requirement::SubstitutionDiagnostic *
+createSubstDiagAt(Sema &S, SourceLocation Location, EntityPrinter Printer);
+
 } // namespace concepts
 
 /// C++2a [expr.prim.req]:
 ///     A requires-expression provides a concise way to express requirements on
 ///     template arguments. A requirement is one that can be checked by name
 ///     lookup (6.4) or by checking properties of types and expressions.
 ///     [...]
 ///     A requires-expression is a prvalue of type bool [...]

clang/lib/Sema/SemaExprCXX.cpp

 #include "TreeTransform.h"
 #include "TypeLocBuilder.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTLambda.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/CharUnits.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/ExprCXX.h"
+#include "clang/AST/ExprConcepts.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/AlignedAllocation.h"
 #include "clang/Basic/DiagnosticSema.h"
 #include "clang/Basic/PartialDiagnostic.h"
 #include "clang/Basic/TargetInfo.h"
else if (ReturnTypeRequirement.isTypeConstraint()) {
     auto *Param = cast<TemplateTypeParmDecl>(TPL->getParam(0));
 
     TemplateArgumentList TAL(TemplateArgumentList::OnStack, Args);
     MultiLevelTemplateArgumentList MLTAL(Param, TAL.asArray(),
                                          /*Final=*/false);
     MLTAL.addOuterRetainedLevels(TPL->getDepth());
     const TypeConstraint *TC = Param->getTypeConstraint();
     assert(TC && "Type Constraint cannot be null here");
-    ExprResult Constraint =
-        SubstExpr(TC->getImmediatelyDeclaredConstraint(), MLTAL);
+    auto *IDC = TC->getImmediatelyDeclaredConstraint();
+    assert(IDC && "ImmediatelyDeclaredConstraint can't be null here.");
+    ExprResult Constraint = SubstExpr(IDC, MLTAL);
     if (Constraint.isInvalid()) {
       Status = concepts::ExprRequirement::SS_ExprSubstitutionFailure;
-    } else {
-      SubstitutedConstraintExpr =
-          cast<ConceptSpecializationExpr>(Constraint.get());
-      if (!SubstitutedConstraintExpr->isSatisfied())
-        Status = concepts::ExprRequirement::SS_ConstraintsNotSatisfied;
-    }
-  }
+      return new (Context) concepts::ExprRequirement(
+          concepts::createSubstDiagAt(*this, IDC->getExprLoc(),
+                                      [&](llvm::raw_ostream &OS) {
+                                        IDC->printPretty(OS, /*Helper=*/nullptr,
+                                                         getPrintingPolicy());
+                                      }),
+          IsSimple, NoexceptLoc, ReturnTypeRequirement);
+    }
   return new (Context) concepts::ExprRequirement(E, IsSimple, NoexceptLoc,
                                                  ReturnTypeRequirement, Status,
                                                  SubstitutedConstraintExpr);
 }
 
 concepts::ExprRequirement *
 Sema::BuildExprRequirement(
     concepts::Requirement::SubstitutionDiagnostic *ExprSubstitutionDiagnostic,

clang/lib/Sema/SemaTemplateInstantiate.cpp

QualType TemplateInstantiator::TransformSubstTemplateTypeParmPackType(
 
   TemplateArgument Pack = T->getArgumentPack();
   TemplateArgument Arg = getPackSubstitutedTemplateArgument(getSema(), Pack);
   return BuildSubstTemplateTypeParmType(
       TLB, SuppressObjCLifetime, T->getFinal(), NewReplaced, T->getIndex(),
       getPackIndex(Pack), Arg, TL.getNameLoc());
 }
 
-template<typename EntityPrinter>
 static concepts::Requirement::SubstitutionDiagnostic *
-createSubstDiag(Sema &S, TemplateDeductionInfo &Info, EntityPrinter Printer) {
+createSubstDiag(Sema &S, TemplateDeductionInfo &Info,
+                concepts::EntityPrinter Printer) {
   SmallString<128> Message;
   SourceLocation ErrorLoc;
   if (Info.hasSFINAEDiagnostic()) {
     PartialDiagnosticAt PDA(SourceLocation(),
                             PartialDiagnostic::NullDiagnostic{});
     Info.takeSFINAEDiagnostic(PDA);
     PDA.second.EmitToString(S.getDiagnostics(), Message);
     ErrorLoc = PDA.first;
   } else {
     ErrorLoc = Info.getLocation();
   }
   char *MessageBuf = new (S.Context) char[Message.size()];
   std::copy(Message.begin(), Message.end(), MessageBuf);
   SmallString<128> Entity;
   llvm::raw_svector_ostream OS(Entity);
   Printer(OS);
   char *EntityBuf = new (S.Context) char[Entity.size()];
   std::copy(Entity.begin(), Entity.end(), EntityBuf);
   return new (S.Context) concepts::Requirement::SubstitutionDiagnostic{
       StringRef(EntityBuf, Entity.size()), ErrorLoc,
       StringRef(MessageBuf, Message.size())};
 }
 
+concepts::Requirement::SubstitutionDiagnostic *
+concepts::createSubstDiagAt(Sema &S, SourceLocation Location,
+                            EntityPrinter Printer) {
+  SmallString<128> Entity;
+  llvm::raw_svector_ostream OS(Entity);
+  Printer(OS);
+  char *EntityBuf = new (S.Context) char[Entity.size()];
+  llvm::copy(Entity, EntityBuf);
+  return new (S.Context) concepts::Requirement::SubstitutionDiagnostic{
+      /*SubstitutedEntity=*/StringRef(EntityBuf, Entity.size()),
+      /*DiagLoc=*/Location, /*DiagMessage=*/StringRef()};
+}
+
 ExprResult TemplateInstantiator::TransformRequiresTypeParams(
     SourceLocation KWLoc, SourceLocation RBraceLoc, const RequiresExpr *RE,
     RequiresExprBodyDecl *Body, ArrayRef<ParmVarDecl *> Params,
     SmallVectorImpl<QualType> &PTypes,
     SmallVectorImpl<ParmVarDecl *> &TransParams,
     Sema::ExtParameterInfoBuilder &PInfos) {
 
   TemplateDeductionInfo Info(KWLoc);

clang/test/SemaCXX/concept-crash-on-diagnostic.cpp

+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify %s
+
+template <typename _Iterator> class normal_iterator {};
+
+template <typename From, typename To> struct is_convertible {};
+
+template <typename From, typename To>
+inline constexpr bool is_convertible_v = is_convertible<From, To>::value; // #1
+
+// expected-error@#1 {{no member named 'value' in 'is_convertible<bool, bool>'}}
+
+template <typename From, typename To>
+concept convertible_to = is_convertible_v<From, To>; // expected-note 0+{{}}
+template <typename IteratorL, typename IteratorR>
+  requires requires(IteratorL lhs, IteratorR rhs) { // expected-note 0+{{}}
+    { lhs == rhs } -> convertible_to<bool>; // #2
+  }
+constexpr bool compare(normal_iterator<IteratorL> lhs, normal_iterator<IteratorR> rhs) {
+  return false;
+}
+
+// We don't know exactly the substituted type for `lhs == rhs`, thus a placeholder 'expr-type' is emitted.
+// expected-note@#2 {{'convertible_to<expr-type, bool>'}}
+
+// Consume remaining notes/errors.
+// expected-note@* 0+{{}}
+// expected-error@* 0+{{}}
+class Object;
+
+void function() {
+  normal_iterator<Object *> begin, end;
+  compare(begin, end);
+}

clang/test/SemaCXX/concept-fatal-error.cpp

-// RUN: not %clang_cc1 -fsyntax-only -std=c++20 -ferror-limit 1 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -ferror-limit 1 -verify %s
 
 template <class>
 concept f = requires { 42; };
 struct h {
   // The missing semicolon will trigger an error and -ferror-limit=1 will make it fatal
   // We test that we do not crash in such cases (#55401)
   int i = requires { { i } f } // expected-error {{expected ';' at end of declaration list}}
-                               // expected-error@* {{too many errros emitted}}
+                               // expected-error@* {{too many errors emitted}}
 };