mizvekov created this revision.
mizvekov published this revision for review.
mizvekov added a reviewer: rsmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
See PR50864.
This fixes error caused by substitution failures on template arguments
for concept specializations. With this patch, in such cases, the concept
specialization will evaluate to false instead.
As a bonus, we:
- Remove some memory leak as an std::string was being stored in the AST.
- Remove some duplicate implementation of serialization (as string) of
substitution diagnostics for concepts.
- Fix some cases where we would try to take a SFINAE diagnostic without
checking if there was one first.
Signed-off-by: Matheus Izvekov <mizve...@gmail.com>
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D107595
Files:
clang/include/clang/AST/ASTConcept.h
clang/include/clang/AST/ExprConcepts.h
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Sema/Sema.h
clang/lib/AST/ASTConcept.cpp
clang/lib/Sema/SemaConcept.cpp
clang/lib/Sema/SemaTemplateInstantiate.cpp
clang/lib/Sema/TreeTransform.h
clang/lib/Serialization/ASTWriterStmt.cpp
clang/test/SemaTemplate/concepts.cpp
Index: clang/test/SemaTemplate/concepts.cpp
===================================================================
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -169,3 +169,24 @@
template<C T, typename U> void f(T, U) = delete;
void g() { f(0, 0); }
}
+
+namespace PR50864 {
+ template <class T> concept Valid = T::valid; // expected-note {{evaluated to false}}
+ template <class T> struct A {
+ template <class U> void f(U) requires Valid<typename T::type>;
+ // expected-note@-1 {{candidate template ignored: constraints not satisfied [with U = int]}}
+ // expected-note@-2 {{because 'typename T::type' does not satisfy 'Valid'}}
+ // expected-note@-3 {{because substituted constraint expression is ill-formed: type 'int' cannot be used prior to '::' because it has no members}}
+ template <class U> void f(U) requires Valid<T>;
+ // expected-note@-1 {{candidate template ignored: constraints not satisfied [with U = int]}}
+ // expected-note@-2 {{does not satisfy 'Valid'}}
+ };
+
+ template<bool V> struct X { static constexpr bool valid = V; };
+
+ struct T1 : X<false> {};
+ void t1() { A<T1>().f(1); } // expected-error {{no matching member function for call to 'f'}}
+
+ struct T2 : X<true> {};
+ void t2() { A<T2>().f(1); }
+}
Index: clang/lib/Serialization/ASTWriterStmt.cpp
===================================================================
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -409,10 +409,9 @@
if (E)
Record.AddStmt(E);
else {
- auto *Diag = DetailRecord.second.get<std::pair<SourceLocation,
- StringRef> *>();
- Record.AddSourceLocation(Diag->first);
- Record.AddString(Diag->second);
+ auto *Diag = DetailRecord.second.get<SubstitutionDiagnosticDiag *>();
+ Record.AddSourceLocation(Diag->Loc);
+ Record.AddString(Diag->Msg);
}
}
}
@@ -423,8 +422,8 @@
ASTRecordWriter &Record,
const concepts::Requirement::SubstitutionDiagnostic *D) {
Record.AddString(D->SubstitutedEntity);
- Record.AddSourceLocation(D->DiagLoc);
- Record.AddString(D->DiagMessage);
+ Record.AddSourceLocation(D->Diag.Loc);
+ Record.AddString(D->Diag.Msg);
}
void ASTStmtWriter::VisitConceptSpecializationExpr(
Index: clang/lib/Sema/TreeTransform.h
===================================================================
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -3357,25 +3357,6 @@
return getSema().BuildSourceLocExpr(Kind, BuiltinLoc, RPLoc, ParentContext);
}
- /// Build a new Objective-C boxed expression.
- ///
- /// By default, performs semantic analysis to build the new expression.
- /// Subclasses may override this routine to provide different behavior.
- ExprResult RebuildConceptSpecializationExpr(NestedNameSpecifierLoc NNS,
- SourceLocation TemplateKWLoc, DeclarationNameInfo ConceptNameInfo,
- NamedDecl *FoundDecl, ConceptDecl *NamedConcept,
- TemplateArgumentListInfo *TALI) {
- CXXScopeSpec SS;
- SS.Adopt(NNS);
- ExprResult Result = getSema().CheckConceptTemplateId(SS, TemplateKWLoc,
- ConceptNameInfo,
- FoundDecl,
- NamedConcept, TALI);
- if (Result.isInvalid())
- return ExprError();
- return Result;
- }
-
/// \brief Build a new requires expression.
///
/// By default, performs semantic analysis to build the new expression.
@@ -12271,20 +12252,10 @@
E->getEndLoc());
}
-template<typename Derived>
-ExprResult
-TreeTransform<Derived>::TransformConceptSpecializationExpr(
- ConceptSpecializationExpr *E) {
- const ASTTemplateArgumentListInfo *Old = E->getTemplateArgsAsWritten();
- TemplateArgumentListInfo TransArgs(Old->LAngleLoc, Old->RAngleLoc);
- if (getDerived().TransformTemplateArguments(Old->getTemplateArgs(),
- Old->NumTemplateArgs, TransArgs))
- return ExprError();
-
- return getDerived().RebuildConceptSpecializationExpr(
- E->getNestedNameSpecifierLoc(), E->getTemplateKWLoc(),
- E->getConceptNameInfo(), E->getFoundDecl(), E->getNamedConcept(),
- &TransArgs);
+template <typename Derived>
+ExprResult TreeTransform<Derived>::TransformConceptSpecializationExpr(
+ ConceptSpecializationExpr *E) {
+ return E;
}
template<typename Derived>
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===================================================================
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -1185,6 +1185,8 @@
concepts::NestedRequirement *
TransformNestedRequirement(concepts::NestedRequirement *Req);
+ ExprResult TransformConceptSpecializationExpr(ConceptSpecializationExpr *E);
+
private:
ExprResult transformNonTypeTemplateParmRef(NonTypeTemplateParmDecl *parm,
SourceLocation loc,
@@ -1878,27 +1880,12 @@
template<typename EntityPrinter>
static concepts::Requirement::SubstitutionDiagnostic *
createSubstDiag(Sema &S, TemplateDeductionInfo &Info, 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())};
+ StringRef(Entity).copy(S.Context.getAllocator()),
+ S.getSubstitutionDiagnosticDiag(Info)};
}
concepts::TypeRequirement *
@@ -2030,6 +2017,46 @@
return RebuildNestedRequirement(TransConstraint.get());
}
+ExprResult TemplateInstantiator::TransformConceptSpecializationExpr(
+ ConceptSpecializationExpr *E) {
+
+ Sema::SFINAETrap Trap(SemaRef);
+ TemplateDeductionInfo Info(E->getTemplateKWLoc());
+ Sema::InstantiatingTemplate Inst(
+ SemaRef, E->getBeginLoc(),
+ Sema::InstantiatingTemplate::ConstraintSubstitution{},
+ E->getNamedConcept(), Info, E->getSourceRange());
+ if (Inst.isInvalid())
+ return ExprError();
+
+ CXXScopeSpec SS;
+ SS.Adopt(E->getNestedNameSpecifierLoc());
+
+ const ASTTemplateArgumentListInfo *Old = E->getTemplateArgsAsWritten();
+ TemplateArgumentListInfo TransArgs(Old->LAngleLoc, Old->RAngleLoc);
+ if (TransformTemplateArguments(Old->getTemplateArgs(), Old->NumTemplateArgs,
+ TransArgs) ||
+ Trap.hasErrorOccurred()) {
+ ConstraintSatisfaction Satisfaction;
+ Satisfaction.Details.emplace_back(
+ nullptr,
+ new (SemaRef.Context) ConstraintSatisfaction::SubstitutionDiagnostic(
+ SemaRef.getSubstitutionDiagnosticDiag(Info)));
+ return ConceptSpecializationExpr::Create(
+ SemaRef.Context,
+ SS.isSet() ? SS.getWithLocInContext(SemaRef.Context)
+ : NestedNameSpecifierLoc{},
+ E->getTemplateKWLoc(), E->getConceptNameInfo(), E->getFoundDecl(),
+ E->getNamedConcept(), Old, ArrayRef<TemplateArgument>{}, &Satisfaction);
+ }
+
+ ExprResult Result = SemaRef.CheckConceptTemplateId(
+ SS, E->getTemplateKWLoc(), E->getConceptNameInfo(), E->getFoundDecl(),
+ E->getNamedConcept(), &TransArgs);
+ if (Result.isInvalid())
+ return ExprError();
+ return Result;
+}
/// Perform substitution on the type T with a given set of template
/// arguments.
Index: clang/lib/Sema/SemaConcept.cpp
===================================================================
--- clang/lib/Sema/SemaConcept.cpp
+++ clang/lib/Sema/SemaConcept.cpp
@@ -123,6 +123,25 @@
return true;
}
+// FIXME: Concepts: This is an unfortunate consequence of there
+// being no serialization code for PartialDiagnostics and the fact
+// that serializing them would likely take a lot more storage than
+// just storing them as strings. We would still like, in the
+// future, to serialize the proper PartialDiagnostic as serializing
+// it as a string defeats the purpose of the diagnostic mechanism.
+SubstitutionDiagnosticDiag
+Sema::getSubstitutionDiagnosticDiag(TemplateDeductionInfo &TDI) {
+ if (!TDI.hasSFINAEDiagnostic())
+ return {TDI.getLocation(), StringRef()};
+
+ PartialDiagnosticAt SubstDiag{SourceLocation(),
+ PartialDiagnostic::NullDiagnostic()};
+ TDI.takeSFINAEDiagnostic(SubstDiag);
+ SmallString<128> DiagString;
+ SubstDiag.second.EmitToString(getDiagnostics(), DiagString);
+ return {SubstDiag.first, StringRef(DiagString).copy(Context.getAllocator())};
+}
+
template <typename AtomicEvaluator>
static bool
calculateConstraintSatisfaction(Sema &S, const Expr *ConstraintExpr,
@@ -230,37 +249,21 @@
if (!SubstitutedExpression.isInvalid())
SubstitutedExpression =
S.PerformContextuallyConvertToBool(SubstitutedExpression.get());
- if (SubstitutedExpression.isInvalid() || Trap.hasErrorOccurred()) {
+ if (Trap.hasErrorOccurred()) {
// C++2a [temp.constr.atomic]p1
// ...If substitution results in an invalid type or expression, the
// constraint is not satisfied.
- if (!Trap.hasErrorOccurred())
- // A non-SFINAE error has occured as a result of this
- // substitution.
- return ExprError();
-
- PartialDiagnosticAt SubstDiag{SourceLocation(),
- PartialDiagnostic::NullDiagnostic()};
- Info.takeSFINAEDiagnostic(SubstDiag);
- // FIXME: Concepts: This is an unfortunate consequence of there
- // being no serialization code for PartialDiagnostics and the fact
- // that serializing them would likely take a lot more storage than
- // just storing them as strings. We would still like, in the
- // future, to serialize the proper PartialDiagnostic as serializing
- // it as a string defeats the purpose of the diagnostic mechanism.
- SmallString<128> DiagString;
- DiagString = ": ";
- SubstDiag.second.EmitToString(S.getDiagnostics(), DiagString);
- unsigned MessageSize = DiagString.size();
- char *Mem = new (S.Context) char[MessageSize];
- memcpy(Mem, DiagString.c_str(), MessageSize);
Satisfaction.Details.emplace_back(
AtomicExpr,
- new (S.Context) ConstraintSatisfaction::SubstitutionDiagnostic{
- SubstDiag.first, StringRef(Mem, MessageSize)});
+ new (S.Context) ConstraintSatisfaction::SubstitutionDiagnostic(
+ S.getSubstitutionDiagnosticDiag(Info)));
Satisfaction.IsSatisfied = false;
return ExprEmpty();
}
+ if (SubstitutedExpression.isInvalid())
+ // A non-SFINAE error has occured as a result of this
+ // substitution.
+ return ExprError();
}
if (!S.CheckConstraintExpression(SubstitutedExpression.get()))
@@ -422,13 +425,13 @@
break;
case concepts::ExprRequirement::SS_ExprSubstitutionFailure: {
auto *SubstDiag = Req->getExprSubstitutionDiagnostic();
- if (!SubstDiag->DiagMessage.empty())
- S.Diag(SubstDiag->DiagLoc,
+ if (!SubstDiag->Diag.Msg.empty())
+ S.Diag(SubstDiag->Diag.Loc,
diag::note_expr_requirement_expr_substitution_error)
- << (int)First << SubstDiag->SubstitutedEntity
- << SubstDiag->DiagMessage;
+ << (int)First << SubstDiag->SubstitutedEntity
+ << SubstDiag->Diag.Msg;
else
- S.Diag(SubstDiag->DiagLoc,
+ S.Diag(SubstDiag->Diag.Loc,
diag::note_expr_requirement_expr_unknown_substitution_error)
<< (int)First << SubstDiag->SubstitutedEntity;
break;
@@ -441,14 +444,16 @@
case concepts::ExprRequirement::SS_TypeRequirementSubstitutionFailure: {
auto *SubstDiag =
Req->getReturnTypeRequirement().getSubstitutionDiagnostic();
- if (!SubstDiag->DiagMessage.empty())
- S.Diag(SubstDiag->DiagLoc,
+ if (!SubstDiag->Diag.Msg.empty())
+ S.Diag(SubstDiag->Diag.Loc,
diag::note_expr_requirement_type_requirement_substitution_error)
<< (int)First << SubstDiag->SubstitutedEntity
- << SubstDiag->DiagMessage;
+ << SubstDiag->Diag.Msg;
else
- S.Diag(SubstDiag->DiagLoc,
- diag::note_expr_requirement_type_requirement_unknown_substitution_error)
+ S.Diag(
+ SubstDiag->Diag.Loc,
+ diag::
+ note_expr_requirement_type_requirement_unknown_substitution_error)
<< (int)First << SubstDiag->SubstitutedEntity;
break;
}
@@ -487,12 +492,12 @@
return;
case concepts::TypeRequirement::SS_SubstitutionFailure: {
auto *SubstDiag = Req->getSubstitutionDiagnostic();
- if (!SubstDiag->DiagMessage.empty())
- S.Diag(SubstDiag->DiagLoc,
- diag::note_type_requirement_substitution_error) << (int)First
- << SubstDiag->SubstitutedEntity << SubstDiag->DiagMessage;
+ if (!SubstDiag->Diag.Msg.empty())
+ S.Diag(SubstDiag->Diag.Loc,
+ diag::note_type_requirement_substitution_error)
+ << (int)First << SubstDiag->SubstitutedEntity << SubstDiag->Diag.Msg;
else
- S.Diag(SubstDiag->DiagLoc,
+ S.Diag(SubstDiag->Diag.Loc,
diag::note_type_requirement_unknown_substitution_error)
<< (int)First << SubstDiag->SubstitutedEntity;
return;
@@ -509,13 +514,12 @@
if (Req->isSubstitutionFailure()) {
concepts::Requirement::SubstitutionDiagnostic *SubstDiag =
Req->getSubstitutionDiagnostic();
- if (!SubstDiag->DiagMessage.empty())
- S.Diag(SubstDiag->DiagLoc,
+ if (!SubstDiag->Diag.Msg.empty())
+ S.Diag(SubstDiag->Diag.Loc,
diag::note_nested_requirement_substitution_error)
- << (int)First << SubstDiag->SubstitutedEntity
- << SubstDiag->DiagMessage;
+ << (int)First << SubstDiag->SubstitutedEntity << SubstDiag->Diag.Msg;
else
- S.Diag(SubstDiag->DiagLoc,
+ S.Diag(SubstDiag->Diag.Loc,
diag::note_nested_requirement_unknown_substitution_error)
<< (int)First << SubstDiag->SubstitutedEntity;
return;
@@ -631,8 +635,8 @@
const llvm::PointerUnion<Expr *, SubstitutionDiagnostic *> &Record,
bool First = true) {
if (auto *Diag = Record.template dyn_cast<SubstitutionDiagnostic *>()){
- S.Diag(Diag->first, diag::note_substituted_constraint_expr_is_ill_formed)
- << Diag->second;
+ S.Diag(Diag->Loc, diag::note_substituted_constraint_expr_is_ill_formed)
+ << Diag->Msg.empty() << Diag->Msg;
return;
}
Index: clang/lib/AST/ASTConcept.cpp
===================================================================
--- clang/lib/AST/ASTConcept.cpp
+++ clang/lib/AST/ASTConcept.cpp
@@ -33,12 +33,10 @@
Detail.second.get<Expr *>())};
else {
auto &SubstitutionDiagnostic =
- *Detail.second.get<std::pair<SourceLocation, StringRef> *>();
- unsigned MessageSize = SubstitutionDiagnostic.second.size();
- char *Mem = new (C) char[MessageSize];
- memcpy(Mem, SubstitutionDiagnostic.second.data(), MessageSize);
- auto *NewSubstDiag = new (C) std::pair<SourceLocation, StringRef>(
- SubstitutionDiagnostic.first, StringRef(Mem, MessageSize));
+ *Detail.second.get<SubstitutionDiagnosticDiag *>();
+ auto *NewSubstDiag = new (C) SubstitutionDiagnosticDiag{
+ SubstitutionDiagnostic.Loc,
+ SubstitutionDiagnostic.Msg.copy(C.getAllocator())};
new (getTrailingObjects<UnsatisfiedConstraintRecord>() + I)
UnsatisfiedConstraintRecord{Detail.first,
UnsatisfiedConstraintRecord::second_type(
Index: clang/include/clang/Sema/Sema.h
===================================================================
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -6841,6 +6841,9 @@
bool MaybeEmitAmbiguousAtomicConstraintsDiagnostic(NamedDecl *D1,
ArrayRef<const Expr *> AC1, NamedDecl *D2, ArrayRef<const Expr *> AC2);
+ SubstitutionDiagnosticDiag
+ getSubstitutionDiagnosticDiag(sema::TemplateDeductionInfo &TDI);
+
/// \brief Check whether the given list of constraint expressions are
/// satisfied (as if in a 'conjunction') given template arguments.
/// \param Template the template-like entity that triggered the constraints
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2785,7 +2785,7 @@
"constraints not satisfied for %select{class template|function template|variable template|alias template|"
"template template parameter|template}0 %1%2">;
def note_substituted_constraint_expr_is_ill_formed : Note<
- "because substituted constraint expression is ill-formed%0">;
+ "because substituted constraint expression is ill-formed%select{: %1|}0">;
def note_atomic_constraint_evaluated_to_false : Note<
"%select{and|because}0 '%1' evaluated to false">;
def note_concept_specialization_constraint_evaluated_to_false : Note<
Index: clang/include/clang/AST/ExprConcepts.h
===================================================================
--- clang/include/clang/AST/ExprConcepts.h
+++ clang/include/clang/AST/ExprConcepts.h
@@ -43,7 +43,7 @@
friend class ASTStmtReader;
friend TrailingObjects;
public:
- using SubstitutionDiagnostic = std::pair<SourceLocation, std::string>;
+ using SubstitutionDiagnostic = SubstitutionDiagnosticDiag;
protected:
/// \brief The number of template arguments in the tail-allocated list of
@@ -160,11 +160,7 @@
public:
struct SubstitutionDiagnostic {
StringRef SubstitutedEntity;
- // FIXME: Store diagnostics semantically and not as prerendered strings.
- // Fixing this probably requires serialization of PartialDiagnostic
- // objects.
- SourceLocation DiagLoc;
- StringRef DiagMessage;
+ SubstitutionDiagnosticDiag Diag;
};
Requirement(RequirementKind Kind, bool IsDependent,
Index: clang/include/clang/AST/ASTConcept.h
===================================================================
--- clang/include/clang/AST/ASTConcept.h
+++ clang/include/clang/AST/ASTConcept.h
@@ -25,6 +25,14 @@
class ConceptDecl;
class ConceptSpecializationExpr;
+struct SubstitutionDiagnosticDiag {
+ SourceLocation Loc;
+ // FIXME: Store diagnostics semantically and not as prerendered strings.
+ // Fixing this probably requires serialization of PartialDiagnostic
+ // objects.
+ StringRef Msg;
+};
+
/// The result of a constraint satisfaction check, containing the necessary
/// information to diagnose an unsatisfied constraint.
class ConstraintSatisfaction : public llvm::FoldingSetNode {
@@ -42,7 +50,7 @@
ConstraintOwner(ConstraintOwner), TemplateArgs(TemplateArgs.begin(),
TemplateArgs.end()) { }
- using SubstitutionDiagnostic = std::pair<SourceLocation, StringRef>;
+ using SubstitutionDiagnostic = SubstitutionDiagnosticDiag;
using Detail = llvm::PointerUnion<Expr *, SubstitutionDiagnostic *>;
bool IsSatisfied = false;
@@ -68,8 +76,7 @@
/// an invalid expression.
using UnsatisfiedConstraintRecord =
std::pair<const Expr *,
- llvm::PointerUnion<Expr *,
- std::pair<SourceLocation, StringRef> *>>;
+ llvm::PointerUnion<Expr *, SubstitutionDiagnosticDiag *>>;
/// \brief The result of a constraint satisfaction check, containing the
/// necessary information to diagnose an unsatisfied constraint.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
