[clang] check deduction consistency when partial ordering function templates #100692
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang Author: Matheus Izvekov (mizvekov) ChangesThis makes partial ordering of function templates consistent with other entities. Fixes #18291 Patch is 43.09 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/100692.diff 10 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 5dddd8f1c5af5..6098101a73a62 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -153,6 +153,8 @@ Bug Fixes to C++ Support
- Fixed a crash when an expression with a dependent ``__typeof__`` type is used as the operand of a unary operator. (#GH97646)
- Fixed a failed assertion when checking invalid delete operator declaration. (#GH96191)
+- When performing partial ordering of function templates, clang now checks that
+ the deduction was consistent. Fixes (#GH18291).
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 558e20ed3e423..2512cd575fbdd 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -5346,7 +5346,6 @@ static EvalStmtResult EvaluateStmt(StmtResult &Result, EvalInfo &Info,
const Expr *RetExpr = cast<ReturnStmt>(S)->getRetValue();
FullExpressionRAII Scope(Info);
if (RetExpr && RetExpr->isValueDependent()) {
- EvaluateDependentExpr(RetExpr, Info);
// We know we returned, but we don't know what the value is.
return ESR_Failed;
}
diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp
index b7b857ebf804b..c848f1ef83b42 100644
--- a/clang/lib/Sema/SemaTemplateDeduction.cpp
+++ b/clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -837,9 +837,11 @@ class PackDeductionScope {
PackDeductionScope(Sema &S, TemplateParameterList *TemplateParams,
SmallVectorImpl<DeducedTemplateArgument> &Deduced,
TemplateDeductionInfo &Info, TemplateArgument Pattern,
- bool DeducePackIfNotAlreadyDeduced = false)
+ bool DeducePackIfNotAlreadyDeduced = false,
+ bool FinishingDeduction = false)
: S(S), TemplateParams(TemplateParams), Deduced(Deduced), Info(Info),
- DeducePackIfNotAlreadyDeduced(DeducePackIfNotAlreadyDeduced){
+ DeducePackIfNotAlreadyDeduced(DeducePackIfNotAlreadyDeduced),
+ FinishingDeduction(FinishingDeduction) {
unsigned NumNamedPacks = addPacks(Pattern);
finishConstruction(NumNamedPacks);
}
@@ -859,8 +861,10 @@ class PackDeductionScope {
// by this pack expansion, then clear out the deduction.
DeducedFromEarlierParameter = !Deduced[Index].isNull();
DeducedPack Pack(Index);
- Pack.Saved = Deduced[Index];
- Deduced[Index] = TemplateArgument();
+ if (!FinishingDeduction) {
+ Pack.Saved = Deduced[Index];
+ Deduced[Index] = TemplateArgument();
+ }
// FIXME: What if we encounter multiple packs with different numbers of
// pre-expanded expansions? (This should already have been diagnosed
@@ -1024,18 +1028,20 @@ class PackDeductionScope {
// Capture the deduced template arguments for each parameter pack expanded
// by this pack expansion, add them to the list of arguments we've deduced
// for that pack, then clear out the deduced argument.
- for (auto &Pack : Packs) {
- DeducedTemplateArgument &DeducedArg = Deduced[Pack.Index];
- if (!Pack.New.empty() || !DeducedArg.isNull()) {
- while (Pack.New.size() < PackElements)
- Pack.New.push_back(DeducedTemplateArgument());
- if (Pack.New.size() == PackElements)
- Pack.New.push_back(DeducedArg);
- else
- Pack.New[PackElements] = DeducedArg;
- DeducedArg = Pack.New.size() > PackElements + 1
- ? Pack.New[PackElements + 1]
- : DeducedTemplateArgument();
+ if (!FinishingDeduction) {
+ for (auto &Pack : Packs) {
+ DeducedTemplateArgument &DeducedArg = Deduced[Pack.Index];
+ if (!Pack.New.empty() || !DeducedArg.isNull()) {
+ while (Pack.New.size() < PackElements)
+ Pack.New.push_back(DeducedTemplateArgument());
+ if (Pack.New.size() == PackElements)
+ Pack.New.push_back(DeducedArg);
+ else
+ Pack.New[PackElements] = DeducedArg;
+ DeducedArg = Pack.New.size() > PackElements + 1
+ ? Pack.New[PackElements + 1]
+ : DeducedTemplateArgument();
+ }
}
}
++PackElements;
@@ -1045,11 +1051,14 @@ class PackDeductionScope {
/// producing the argument packs and checking for consistency with prior
/// deductions.
TemplateDeductionResult finish() {
+ if (FinishingDeduction)
+ return TemplateDeductionResult::Success;
// Build argument packs for each of the parameter packs expanded by this
// pack expansion.
for (auto &Pack : Packs) {
// Put back the old value for this pack.
- Deduced[Pack.Index] = Pack.Saved;
+ if (!FinishingDeduction)
+ Deduced[Pack.Index] = Pack.Saved;
// Always make sure the size of this pack is correct, even if we didn't
// deduce any values for it.
@@ -1149,6 +1158,7 @@ class PackDeductionScope {
bool IsPartiallyExpanded = false;
bool DeducePackIfNotAlreadyDeduced = false;
bool DeducedFromEarlierParameter = false;
+ bool FinishingDeduction = false;
/// The number of expansions, if we have a fully-expanded pack in this scope.
std::optional<unsigned> FixedNumExpansions;
@@ -1157,43 +1167,13 @@ class PackDeductionScope {
} // namespace
-/// Deduce the template arguments by comparing the list of parameter
-/// types to the list of argument types, as in the parameter-type-lists of
-/// function types (C++ [temp.deduct.type]p10).
-///
-/// \param S The semantic analysis object within which we are deducing
-///
-/// \param TemplateParams The template parameters that we are deducing
-///
-/// \param Params The list of parameter types
-///
-/// \param NumParams The number of types in \c Params
-///
-/// \param Args The list of argument types
-///
-/// \param NumArgs The number of types in \c Args
-///
-/// \param Info information about the template argument deduction itself
-///
-/// \param Deduced the deduced template arguments
-///
-/// \param TDF bitwise OR of the TemplateDeductionFlags bits that describe
-/// how template argument deduction is performed.
-///
-/// \param PartialOrdering If true, we are performing template argument
-/// deduction for during partial ordering for a call
-/// (C++0x [temp.deduct.partial]).
-///
-/// \returns the result of template argument deduction so far. Note that a
-/// "success" result means that template argument deduction has not yet failed,
-/// but it may still fail, later, for other reasons.
-static TemplateDeductionResult
-DeduceTemplateArguments(Sema &S, TemplateParameterList *TemplateParams,
- const QualType *Params, unsigned NumParams,
- const QualType *Args, unsigned NumArgs,
- TemplateDeductionInfo &Info,
- SmallVectorImpl<DeducedTemplateArgument> &Deduced,
- unsigned TDF, bool PartialOrdering = false) {
+template <class T>
+static TemplateDeductionResult DeduceForEachType(
+ Sema &S, TemplateParameterList *TemplateParams, const QualType *Params,
+ unsigned NumParams, const QualType *Args, unsigned NumArgs,
+ TemplateDeductionInfo &Info,
+ SmallVectorImpl<DeducedTemplateArgument> &Deduced, bool PartialOrdering,
+ bool FinishingDeduction, T &&DeductFunc) {
// C++0x [temp.deduct.type]p10:
// Similarly, if P has a form that contains (T), then each parameter type
// Pi of the respective parameter-type- list of P is compared with the
@@ -1219,11 +1199,10 @@ DeduceTemplateArguments(Sema &S, TemplateParameterList *TemplateParams,
return TemplateDeductionResult::MiscellaneousDeductionFailure;
}
- if (TemplateDeductionResult Result = DeduceTemplateArgumentsByTypeMatch(
- S, TemplateParams, Params[ParamIdx].getUnqualifiedType(),
- Args[ArgIdx].getUnqualifiedType(), Info, Deduced, TDF,
- PartialOrdering,
- /*DeducedFromArrayBound=*/false);
+ if (TemplateDeductionResult Result = DeductFunc(
+ S, TemplateParams, ArgIdx, Params[ParamIdx].getUnqualifiedType(),
+ Args[ArgIdx].getUnqualifiedType(), Info, Deduced,
+ PartialOrdering);
Result != TemplateDeductionResult::Success)
return Result;
@@ -1239,20 +1218,21 @@ DeduceTemplateArguments(Sema &S, TemplateParameterList *TemplateParams,
// template parameter packs expanded by the function parameter pack.
QualType Pattern = Expansion->getPattern();
- PackDeductionScope PackScope(S, TemplateParams, Deduced, Info, Pattern);
+ PackDeductionScope PackScope(S, TemplateParams, Deduced, Info, Pattern,
+ /*DeducePackIfNotAlreadyDeduced=*/false,
+ FinishingDeduction);
// A pack scope with fixed arity is not really a pack any more, so is not
// a non-deduced context.
if (ParamIdx + 1 == NumParams || PackScope.hasFixedArity()) {
for (; ArgIdx < NumArgs && PackScope.hasNextElement(); ++ArgIdx) {
// Deduce template arguments from the pattern.
- if (TemplateDeductionResult Result = DeduceTemplateArgumentsByTypeMatch(
- S, TemplateParams, Pattern.getUnqualifiedType(),
- Args[ArgIdx].getUnqualifiedType(), Info, Deduced, TDF,
- PartialOrdering, /*DeducedFromArrayBound=*/false);
+ if (TemplateDeductionResult Result = DeductFunc(
+ S, TemplateParams, ArgIdx, Pattern.getUnqualifiedType(),
+ Args[ArgIdx].getUnqualifiedType(), Info, Deduced,
+ PartialOrdering);
Result != TemplateDeductionResult::Success)
return Result;
-
PackScope.nextPackElement();
}
} else {
@@ -1305,6 +1285,56 @@ DeduceTemplateArguments(Sema &S, TemplateParameterList *TemplateParams,
return TemplateDeductionResult::Success;
}
+/// Deduce the template arguments by comparing the list of parameter
+/// types to the list of argument types, as in the parameter-type-lists of
+/// function types (C++ [temp.deduct.type]p10).
+///
+/// \param S The semantic analysis object within which we are deducing
+///
+/// \param TemplateParams The template parameters that we are deducing
+///
+/// \param Params The list of parameter types
+///
+/// \param NumParams The number of types in \c Params
+///
+/// \param Args The list of argument types
+///
+/// \param NumArgs The number of types in \c Args
+///
+/// \param Info information about the template argument deduction itself
+///
+/// \param Deduced the deduced template arguments
+///
+/// \param TDF bitwise OR of the TemplateDeductionFlags bits that describe
+/// how template argument deduction is performed.
+///
+/// \param PartialOrdering If true, we are performing template argument
+/// deduction for during partial ordering for a call
+/// (C++0x [temp.deduct.partial]).
+///
+/// \returns the result of template argument deduction so far. Note that a
+/// "success" result means that template argument deduction has not yet failed,
+/// but it may still fail, later, for other reasons.
+static TemplateDeductionResult
+DeduceTemplateArguments(Sema &S, TemplateParameterList *TemplateParams,
+ const QualType *Params, unsigned NumParams,
+ const QualType *Args, unsigned NumArgs,
+ TemplateDeductionInfo &Info,
+ SmallVectorImpl<DeducedTemplateArgument> &Deduced,
+ unsigned TDF, bool PartialOrdering = false) {
+ return ::DeduceForEachType(
+ S, TemplateParams, Params, NumParams, Args, NumArgs, Info, Deduced,
+ PartialOrdering, /*FinishingDeduction=*/false,
+ [TDF](Sema &S, TemplateParameterList *TemplateParams, int, QualType P,
+ QualType A, TemplateDeductionInfo &Info,
+ SmallVectorImpl<DeducedTemplateArgument> &Deduced,
+ bool PartialOrdering) {
+ return DeduceTemplateArgumentsByTypeMatch(
+ S, TemplateParams, P, A, Info, Deduced, TDF, PartialOrdering,
+ /*DeducedFromArrayBound=*/false);
+ });
+}
+
/// Determine whether the parameter has qualifiers that the argument
/// lacks. Put another way, determine whether there is no way to add
/// a deduced set of qualifiers to the ParamType that would result in
@@ -2922,7 +2952,7 @@ static TemplateDeductionResult ConvertDeducedTemplateArguments(
SmallVectorImpl<TemplateArgument> &SugaredBuilder,
SmallVectorImpl<TemplateArgument> &CanonicalBuilder,
LocalInstantiationScope *CurrentInstantiationScope = nullptr,
- unsigned NumAlreadyConverted = 0, bool PartialOverloading = false) {
+ unsigned NumAlreadyConverted = 0, bool *IsIncomplete = nullptr) {
TemplateParameterList *TemplateParams = Template->getTemplateParameters();
for (unsigned I = 0, N = TemplateParams->size(); I != N; ++I) {
@@ -3008,11 +3038,17 @@ static TemplateDeductionResult ConvertDeducedTemplateArguments(
// If there was no default argument, deduction is incomplete.
if (DefArg.getArgument().isNull()) {
+ if (IsIncomplete) {
+ *IsIncomplete = true;
+ SugaredBuilder.push_back({});
+ CanonicalBuilder.push_back({});
+ continue;
+ }
+
Info.Param = makeTemplateParameter(
const_cast<NamedDecl *>(TemplateParams->getParam(I)));
Info.reset(TemplateArgumentList::CreateCopy(S.Context, SugaredBuilder),
TemplateArgumentList::CreateCopy(S.Context, CanonicalBuilder));
- if (PartialOverloading) break;
return HasDefaultArg ? TemplateDeductionResult::SubstitutionFailure
: TemplateDeductionResult::Incomplete;
@@ -3227,7 +3263,7 @@ static TemplateDeductionResult FinishTemplateArgumentDeduction(
S, Template, /*IsDeduced*/ PartialOrdering, Deduced, Info,
SugaredBuilder, CanonicalBuilder,
/*CurrentInstantiationScope=*/nullptr,
- /*NumAlreadyConverted=*/0U, /*PartialOverloading=*/false);
+ /*NumAlreadyConverted=*/0U);
Result != TemplateDeductionResult::Success)
return Result;
@@ -3831,11 +3867,12 @@ TemplateDeductionResult Sema::FinishTemplateArgumentDeduction(
// C++ [temp.deduct.type]p2:
// [...] or if any template argument remains neither deduced nor
// explicitly specified, template argument deduction fails.
+ bool IsIncomplete = false;
SmallVector<TemplateArgument, 4> SugaredBuilder, CanonicalBuilder;
if (auto Result = ConvertDeducedTemplateArguments(
*this, FunctionTemplate, /*IsDeduced*/ true, Deduced, Info,
SugaredBuilder, CanonicalBuilder, CurrentInstantiationScope,
- NumExplicitlySpecified, PartialOverloading);
+ NumExplicitlySpecified, PartialOverloading ? &IsIncomplete : nullptr);
Result != TemplateDeductionResult::Success)
return Result;
@@ -3914,9 +3951,7 @@ TemplateDeductionResult Sema::FinishTemplateArgumentDeduction(
// ([temp.constr.decl]), those constraints are checked for satisfaction
// ([temp.constr.constr]). If the constraints are not satisfied, type
// deduction fails.
- if (!PartialOverloading ||
- (CanonicalBuilder.size() ==
- FunctionTemplate->getTemplateParameters()->size())) {
+ if (!IsIncomplete) {
if (CheckInstantiatedFunctionTemplateConstraints(
Info.getLocation(), Specialization, CanonicalBuilder,
Info.AssociatedConstraintsSatisfaction))
@@ -5399,11 +5434,85 @@ static QualType GetImplicitObjectParameterType(ASTContext &Context,
return Context.getLValueReferenceType(RawType);
}
+static TemplateDeductionResult FinishTemplateArgumentDeduction(
+ Sema &S, FunctionTemplateDecl *FTD, int ArgIdx, QualType P, QualType A,
+ SmallVectorImpl<DeducedTemplateArgument> &Deduced,
+ TemplateDeductionInfo &Info) {
+ // Unevaluated SFINAE context.
+ EnterExpressionEvaluationContext Unevaluated(
+ S, Sema::ExpressionEvaluationContext::Unevaluated);
+ Sema::SFINAETrap Trap(S);
+
+ Sema::ContextRAII SavedContext(S, getAsDeclContextOrEnclosing(FTD));
+
+ // C++ [temp.deduct.type]p2:
+ // [...] or if any template argument remains neither deduced nor
+ // explicitly specified, template argument deduction fails.
+ bool IsIncomplete = false;
+ SmallVector<TemplateArgument, 4> SugaredBuilder, CanonicalBuilder;
+ if (auto Result = ConvertDeducedTemplateArguments(
+ S, FTD, /*IsDeduced=*/true, Deduced, Info, SugaredBuilder,
+ CanonicalBuilder, /*CurrentInstantiationScope=*/nullptr,
+ /*NumAlreadyConverted=*/0, &IsIncomplete);
+ Result != TemplateDeductionResult::Success)
+ return Result;
+
+ // Form the template argument list from the deduced template arguments.
+ TemplateArgumentList *SugaredDeducedArgumentList =
+ TemplateArgumentList::CreateCopy(S.Context, SugaredBuilder);
+ TemplateArgumentList *CanonicalDeducedArgumentList =
+ TemplateArgumentList::CreateCopy(S.Context, CanonicalBuilder);
+
+ Info.reset(SugaredDeducedArgumentList, CanonicalDeducedArgumentList);
+
+ // Substitute the deduced template arguments into the argument
+ // and verify that the instantiated argument is both valid
+ // and equivalent to the parameter.
+ LocalInstantiationScope InstScope(S);
+
+ QualType InstP;
+ {
+ MultiLevelTemplateArgumentList MLTAL(FTD, SugaredBuilder,
+ /*Final=*/true);
+ if (ArgIdx != -1)
+ if (auto *MD = dyn_cast<CXXMethodDecl>(FTD->getTemplatedDecl());
+ MD && MD->isImplicitObjectMemberFunction())
+ ArgIdx -= 1;
+ Sema::ArgumentPackSubstitutionIndexRAII PackIndex(
+ S, ArgIdx != -1 ? ::getPackIndexForParam(S, FTD, MLTAL, ArgIdx) : -1);
+ InstP = S.SubstType(P, MLTAL, FTD->getLocation(), FTD->getDeclName());
+ if (InstP.isNull())
+ return TemplateDeductionResult::SubstitutionFailure;
+ }
+
+ if (auto *PA = dyn_cast<PackExpansionType>(A);
+ PA && !isa<PackExpansionType>(InstP))
+ A = PA->getPattern();
+ if (!S.Context.hasSameType(
+ S.Context.getUnqualifiedArrayType(InstP.getNonReferenceType()),
+ S.Context.getUnqualifiedArrayType(A.getNonReferenceType())))
+ return TemplateDeductionResult::NonDeducedMismatch;
+
+ // C++20 [temp.deduct]p5 - Only check constraints when all parameters have
+ // been deduced.
+ if (!IsIncomplete) {
+ if (auto Result = CheckDeducedArgumentConstraints(S, FTD, SugaredBuilder,
+ CanonicalBuilder, Info);
+ Result != TemplateDeductionResult::Success)
+ return Result;
+ }
+
+ if (Trap.hasErrorOccurred())
+ return TemplateDeductionResult::SubstitutionFailure;
+
+ return TemplateDeductionResult::Success;
+}
+
/// Determine whether the function template \p FT1 is at least as
/// specialized as \p FT2.
static bool isAtLeastAsSpecializedAs(Sema &S, SourceLocation Loc,
- const FunctionTemplateDecl *FT1,
- const FunctionTemplateDecl *FT2,
+ FunctionTemplateDecl *FT1,
+ FunctionTemplateDecl *FT2,
TemplatePartialOrderingContext TPOC,
bool Reversed,
const SmallVector<QualType> &Args1,
@@ -5425,34 +5534,112 @@ static bool isAtLeastAsSpecializedAs(Sema &S, SourceLocation Loc,
// the partial ordering is done:
TemplateDeductionInfo Info(Loc);
switch (TPOC) {
- case TPOC_Call:
+ case TPOC_Call: {
if (DeduceTemplateArguments(S, TemplateParams, Args2.data(), Args2.size(),
Args1.data(), Args1.size(), Info, Deduced,
TDF_None, /*PartialOrdering=*/true) !=
TemplateDeductionResult::Success)
return false;
- break;
+ SmallVector<TemplateArgument, 4> DeducedArgs(Deduced.begin(),
+ Deduced.end());
+ Sema::InstantiatingTemplate Inst(
+ S, Info.getLocation(), FT2, DeducedArgs,
+ Sema::CodeSynthesisContext::DeducedTemplateArgumentSubstitution, Info);
...
[truncated]
|
mizvekov
force-pushed
the
users/mizvekov/clang-fix-GH18291
branch
2 times, most recently
from
497d0c6 to
04cbc86
Compare
mizvekov
force-pushed
the
users/mizvekov/clang-fix-GH18291
branch
from
04cbc86 to
c7ef00e
Compare
clang-tools-extra/test/clang-tidy/checkers/bugprone/stringview-nullptr.cpp
Outdated
Show resolved
Hide resolved
| if (!S.Context.hasSameType( | ||
| S.Context.getUnqualifiedArrayType(InstP.getNonReferenceType()), | ||
| S.Context.getUnqualifiedArrayType(A.getNonReferenceType()))) | ||
| return TemplateDeductionResult::NonDeducedMismatch; |
There was a problem hiding this comment.
A cross-reference to [temp.deduct.call]/4 might be useful here. Do we need to handle the other differences allowed there? I'm thinking of cases like:
template<typename T> struct A {};
struct B : A<int> {};
template<typename U, typename V> void f(U, A<V>&); // #1
template<typename W> void f(W, B&); // #2Is #2 more specialized than #1, by deducing V = int? (Maybe this doesn't matter because they'll never be equally good candidates in overload resolution, so we'll never try to partially order them?)
There was a problem hiding this comment.
We could consider both candidates for this example:
void g() {
B b;
f(0, b);
}But in this case, we order the function template specializations, instead of the function templates, which this patch does not change.
I guess this is what you mean by them being not equally good during overload resolution.
There was a problem hiding this comment.
One thing I have been worried about this check, is that since we need to allow incomplete deductions,
we can have cases where the type we are transforming depends on template parameters we have not deduced at all.
I don't think this check would be guaranteed to fail in that case, as we would be comparing completely unrelated types that could match by happenstance.
So it seems we do need to check all the template parameters the type references are actually deduced.
One way I have been thinking, is that we could extend the transform with an additional boolean result, that would be set in case we tried to use any template arguments which were null.
| // C++20 [temp.deduct]p5 - Only check constraints when all parameters have | ||
| // been deduced. | ||
| if (!IsIncomplete) { | ||
| if (auto Result = CheckDeducedArgumentConstraints(S, FTD, SugaredBuilder, | ||
| CanonicalBuilder, Info); | ||
| Result != TemplateDeductionResult::Success) | ||
| return Result; | ||
| } |
There was a problem hiding this comment.
Is this the right moment to do this? I would have expected we'd check constraints after we form the final template argument list and before we substitute into any parameter types.
There was a problem hiding this comment.
We seem to consistently follow the order I implemented here.
One reason I can think of, is that it would probably be too confusing to produce constraints check failure diagnostics before checking one thing even implies the other.
There was a problem hiding this comment.
According to our side discussions, you think [temp.deduct]p5 should not apply during partial ordering in any case.
But clang follows the opposite consistently.
I think we should leave this as is, and then make a separate patch to fix everywhere at the same time.
Please let me know if you would prefer to remove on this patch.
There was a problem hiding this comment.
Yes, there are orthogonal bugs with when we check constraints, not doing that here is reasonable
There was a problem hiding this comment.
I presume this happens before substituting into the function declaration, right?
I have a concern about CheckDeducedArgumentConstraints() that it would check the function's associated constraints that might refer to uninstantiated function parameters. We didn't set up a scope for that because this function was just used for class/variable templates, which wasn't necessary previously.
There was a problem hiding this comment.
Here on partial ordering of function templates, most of the times we are going to be deducing dependent arguments anyway, so in very few cases we would actually even try to check the constraints.
Since [temp.deduct]p5 does not apply to partial ordering, maybe that's an argument to not do this here anyway.
| bool AtLeastAsSpecialized; | ||
| S.runWithSufficientStackSpace(Info.getLocation(), [&] { | ||
| AtLeastAsSpecialized = ::FinishTemplateArgumentDeduction( | ||
| S, FT2, /*ArgIdx=*/-1, Proto2->getReturnType(), | ||
| Proto1->getReturnType(), Deduced, | ||
| Info) == TemplateDeductionResult::Success; | ||
| if (!AtLeastAsSpecialized) | ||
| return; | ||
| S.runWithSufficientStackSpace(Info.getLocation(), [&] { | ||
| AtLeastAsSpecialized = | ||
| ::DeduceForEachType( | ||
| S, TemplateParams, Proto2->getParamTypes().data(), | ||
| Proto2->getParamTypes().size(), Proto1->getParamTypes().data(), | ||
| Proto1->getParamTypes().size(), Info, Deduced, | ||
| /*PartialOrdering=*/true, /*FinishingDeduction=*/true, | ||
| [&FT2](Sema &S, TemplateParameterList *, int ArgIdx, QualType P, | ||
| QualType A, TemplateDeductionInfo &Info, | ||
| SmallVectorImpl<DeducedTemplateArgument> &Deduced, | ||
| bool PartialOrdering) { | ||
| return ::FinishTemplateArgumentDeduction(S, FT2, ArgIdx, P, A, | ||
| Deduced, Info); | ||
| }) == TemplateDeductionResult::Success; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Do we need to separately check the return type and the parameter types here, rather than checking the entire function type as a whole? (This is not quite the same: for example, checking the whole function type would also consider the exception specification.)
There was a problem hiding this comment.
Since the deduction itself ignores the exception specification (TDF_AllowCompatibleFunctionType), it would seem strange to check exception spec compatibility now, when we could have done that during deduction.
Otherwise, this would fail the first example in ExplicitArgs from CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.partial/p3.cpp:
namespace ExplicitArgs {
template<typename T, typename U>
constexpr int f(U) noexcept(noexcept(T())) {
return 0;
}
template<typename T>
constexpr int f(T*) noexcept {
return 1;
}
template<>
constexpr int f<int>(int*) noexcept {
return 2;
}
static_assert(f<int>(1) == 0);
static_assert(f<short>(y) == 1);
static_assert(f<int>(x) == 2);But also otherwise, we would not handle packs correctly.
See temp_deduct_type_example3 in CXX/CXX/drs/cwg6xx.cpp
template<class T, class... U> void f(T*, U...){}
template<class T> void f(T){}
template void f(int*);We would fail the consistency check as we would get A = void (T), InstP = void (T, U...) if I remember right.
mizvekov
force-pushed
the
users/mizvekov/clang-fix-GH18291
branch
from
c7ef00e to
1aa99ba
Compare
|
Building abseil-cpp with the new Clang runs into errors. Are they expected? git clone https://github.com/abseil/abseil-cpp/
cd abseil-cpp
cmake -GNinja -S. -Bout/release -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_COMPILER=/tmp/Rel/bin/clang++ -DCMAKE_C_COMPILER=/tmp/Rel/bin/clang
ninja -C out/release |
Did you test the version I just pushed 25 minutes ago? I believe these particular breakages should be fixed there. We just implemented a rule change to account for the string_view failures, but in any case I see this patch still fails libc++ tests, so we might be missing more stuff. |
Nevermind, that's a different failure. Still investigating. |
mizvekov
force-pushed
the
users/mizvekov/clang-fix-GH18291
branch
from
1aa99ba to
89e0168
Compare
|
@MaskRay the latest version fixes libc++ regressions. |
mizvekov
force-pushed
the
users/mizvekov/clang-fix-GH18291
branch
from
89e0168 to
89e5db4
Compare
mizvekov
force-pushed
the
users/mizvekov/clang-fix-GH18291
branch
from
89e5db4 to
3771ffa
Compare
|
@zygoloid ping, this is ready for another round of review. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
mizvekov
force-pushed
the
users/mizvekov/clang-fix-GH18291
branch
from
3771ffa to
561c912
Compare
|
@zygoloid ping, this is ready for another look. |
mizvekov
force-pushed
the
users/mizvekov/clang-fix-GH18291
branch
from
561c912 to
14b0812
Compare
It should work without any flags now, can you confirm? |
I can confirm I'm able to build SPEC without the flag now. |
|
@mizvekov another distinct compilation error after this commit (and not fixed by #107972 and other follow-up commits): https://gcc.godbolt.org/z/zMG5nsda3 (reduced from https://github.com/hlslibs/ac_math/blob/20bbf040834c5815b01a9ed8e523e4b0dabecf23/tests/rtest_ac_matrixmul.cpp) We have a few more cases that still generate this compilation error ( |
|
@mizvekov any news here? I think, I'll come up with two more reduced tests later today. |
|
Now I think these should cover all remaining issues we see after this commit (and up to the current ToT):
|
Thanks. I am looking into it. |
When checking deduction consistency, a substitution can be incomplete such that only sugar parts refer to non-deduced template parameters. This would not otherwise lead to an inconsistent deduction, so this patch makes it so we canonicalize the types before substitution in order to avoid that possibility, for now. When we are able to produce substitution failure diagnostics for partial ordering, we might want to improve the TemplateInstantiator so that it does not fail in that case. This fixes a regression on top of #100692, which was reported on the PR. This was never released, so there are no release notes.
|
Regarding test case 2, that further reduces to: template <class, int Rows_, int = (Rows_)> class Q {};
template <int rows> void F(Q<double, rows, rows>);
template <class Scalar, int Rows> void F(Q<Scalar, Rows>) = delete;
void test() {
F(Q<double, 1>());
}The problem is that we consider the parenthesized expression significant in terms of forming a distinct type. I believe the parenthesis should not be significant in this case, and there is also some divergence between GCC and Clang in this area. |
|
I looked at the original code and found a much more involved expression there (https://gitlab.com/libeigen/eigen/-/blob/master/Eigen/src/Core/util/ForwardDeclarations.h?ref_type=heads#L66), so the parenthesized expression is merely an artifact of reduction and there doesn't seem to be an easy workaround, IIUC. |
|
An example, which is a bit closer to the original: https://gcc.godbolt.org/z/5WMb78434. |
|
Our internal releases have been blocked on this issue, since even a local revert is not an option at the moment due to the number of dependent commits and problems in other parts of LLVM. We would appreciate if we could get a resolution in the next day or two. If there's no clear path forward yet or more delays are expected, should we consider reverting this temporarily and relanding with the necessary adjustments? I must say I appreciate the overall responsiveness and readiness to resolve the issues. I just see that there seem to be objective complexities here which may require a bit more time to figure out. |
|
I think that if we don't have a fix in the next 24-48 hours, and no clarity on the issue - a revert should be considered (note that I think it will be disruptive for llvm too as we are, I think, 3 patches deep, but i agree that keeping a regression on trunk for too long is undesirable) BTW, we really appreciate your help finding reductions and test cases for these issues However, I suspect that the parentheses reduction and the one with a ternary are actually the same issue - In you last example, M*2 and the conditional with M+N after the substitution of the default argument aren't equivalent despite having the same value after instantiation. And just looking at the code the error makes sense to me, there isn't a better match (intuitively) here. So I don't think it's a bug with the implementation per say but rather a disruptive changed caused by the fact no one implemented that bit of the standard until now. Then again, the fact your are hitting the issue and the fact it's emmaning from Eigen might motivate some sort of workaround |
|
It's one thing that we might tweak the rules to account for redundant parenthesis. Note that we hadn't so far implemented these rules for partial ordering of function templates, but the same issue should already exist for all implementations when you convert this example to class templates partial specializations, so the approach Eigen is taking here is not standing on solid ground. |
|
Thanks for the clarification! The fact that there's code around that isn't quite following the standard, is not news, but the regular way to approach tightening the compiler is to provide an opt-out mechanism for some time (one release cycle?) in the shape of a flag or otherwise. Do you think it could be a good option here? Alternatively, do you see a standard-compliant way to rewrite this code? And did you have a chance to look at the third test case? It would be nice to know if Clang is acting according to the standard there. |
Actually, I'm not sure the code I provided as "a bit closer to the original" is actually close enough to retain the relevant properties. I'll try to look at the original code to see if it actually is IFNDR according to https://eel.is/c++draft/temp.over.link#5. It's going to take some time though. |
|
It was faster than I thought. The original code for case 2 is indeed invalid. Having looked carefully at the two definitions of PrintTo, I clearly see they are violating ODR. And both are originating in the internal code, so nothing specific to Eigen. Thanks again for the good explanation! |
…#109065) When checking deduction consistency, a substitution can be incomplete such that only sugar parts refer to non-deduced template parameters. This would not otherwise lead to an inconsistent deduction, so this patch makes it so we canonicalize the types before substitution in order to avoid that possibility, for now. When we are able to produce substitution failure diagnostics for partial ordering, we might want to improve the TemplateInstantiator so that it does not fail in that case. This fixes a regression on top of #100692, which was reported on the PR. This was never released, so there are no release notes.
|
@alexfh Your third test case reduces to: template <unsigned> struct InlinedVector {};
template <int TotalNumArgs>
void PackArgsHelper(InlinedVector<TotalNumArgs>, unsigned char);
template <int TotalNumArgs, typename T0, typename... TRest>
void PackArgsHelper(InlinedVector<TotalNumArgs> packed_args, T0,
TRest... args_rest) {
PackArgsHelper<TotalNumArgs>(packed_args, args_rest...);
}
template void PackArgsHelper<2>(InlinedVector<2>, int, unsigned char);The problem here is the mismatch between If you replace Does that solution translate to the unreduced problem? Does that come from Eigen, or is that internal to google? It's a shame that we produce these diagnostics internally, but we don't have them wired to show up when these happen in partial ordering. |
|
Wow, I would never have thought it was a problem. Making these the same type removes the compilation error. Can you explain why this is a problem from the C++ standard PoV? |
That's https://eel.is/c++draft/temp.deduct.type#20 In deduced contexts for an NTTP, the type of the argument and the parameter must match exactly. |
|
I see. A clear diagnostic wouldn't hurt indeed. Thanks a lot for the analysis! No more concerns from our side at this point. |
…llvm#109065) When checking deduction consistency, a substitution can be incomplete such that only sugar parts refer to non-deduced template parameters. This would not otherwise lead to an inconsistent deduction, so this patch makes it so we canonicalize the types before substitution in order to avoid that possibility, for now. When we are able to produce substitution failure diagnostics for partial ordering, we might want to improve the TemplateInstantiator so that it does not fail in that case. This fixes a regression on top of llvm#100692, which was reported on the PR. This was never released, so there are no release notes.
|
There is a case that becomes invalid with this patch, is it expected behaviour? |
|
@mizvekov Gentle ping... |
|
Hello, sorry I missed your earlier notification. Your test case further reduces to: https://godbolt.org/z/q6ndebaeW template <char> struct Traits { using Type = Traits; };
template <char... ArgType> void FillVRegs(typename Traits<ArgType>::Type...);
template <char FirstArgType> void FillVRegs(typename Traits<FirstArgType>::Type);
void foo() { FillVRegs<'A'>(Traits<'A'>()); }I believe Clang and GCC are correct to reject this, we can't partial order |
|
Thanks for the clarification! |
The two specializations with <char FirstArgType, char...ArgType> and <char ...ArgType> are ambiguous after llvm/llvm-project#100692. For e.g., MaterializeVRegs<'L', 'L'> can match both specializations: ArgType = <'L', 'L'>, and FirstArgType = 'L', ArgType = <'L'> The first (now-deleted) specialization can match calls with any number of template parameters, including zero. The second specialization matches calls with one or more parameters. To avoid the ambiguity, delete the first specialization and explicitly avoid calls with zero template parameters. Bug: http://b/363682086 Test: mmma art with ToT clang; and presubmit Change-Id: I4a3737b4098180faa1f889c2bba475846acc8a51
This makes partial ordering of function templates consistent with other entities, by implementing [temp.deduct.type]p1 in that case.
Fixes #18291