Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang] check deduction consistency when partial ordering function templates #100692

Merged
merged 5 commits into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,8 @@ Bug Fixes to C++ Support
- Clang now properly handles the order of attributes in `extern` blocks. (#GH101990).
- Fixed an assertion failure by preventing null explicit object arguments from being deduced. (#GH102025).
- Correctly check constraints of explicit instantiations of member functions. (#GH46029)
- When performing partial ordering of function templates, clang now checks that
the deduction was consistent. Fixes (#GH18291).

Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
7 changes: 6 additions & 1 deletion clang/include/clang/Sema/Sema.h
Original file line number Diff line number Diff line change
Expand Up @@ -13273,6 +13273,10 @@ class Sema final : public SemaBase {
/// \param AllowDeducedTST Whether a DeducedTemplateSpecializationType is
/// acceptable as the top level type of the result.
///
/// \param IsIncompleteSubstitution If provided, the pointee will be set
/// whenever substitution would perform a replacement with a null or
/// non-existent template argument.
///
/// \returns If the instantiation succeeds, the instantiated
/// type. Otherwise, produces diagnostics and returns a NULL type.
TypeSourceInfo *SubstType(TypeSourceInfo *T,
Expand All @@ -13282,7 +13286,8 @@ class Sema final : public SemaBase {

QualType SubstType(QualType T,
const MultiLevelTemplateArgumentList &TemplateArgs,
SourceLocation Loc, DeclarationName Entity);
SourceLocation Loc, DeclarationName Entity,
bool *IsIncompleteSubstitution = nullptr);

TypeSourceInfo *SubstType(TypeLoc TL,
const MultiLevelTemplateArgumentList &TemplateArgs,
Expand Down
1 change: 0 additions & 1 deletion clang/lib/AST/ExprConstant.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5399,7 +5399,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);
mizvekov marked this conversation as resolved.
Show resolved Hide resolved
// We know we returned, but we don't know what the value is.
return ESR_Failed;
}
Expand Down
827 changes: 570 additions & 257 deletions clang/lib/Sema/SemaTemplateDeduction.cpp

Large diffs are not rendered by default.

29 changes: 24 additions & 5 deletions clang/lib/Sema/SemaTemplateInstantiate.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1343,6 +1343,9 @@ namespace {
DeclarationName Entity;
// Whether to evaluate the C++20 constraints or simply substitute into them.
bool EvaluateConstraints = true;
// Whether Substitution was Incomplete, that is, we tried to substitute in
// any user provided template arguments which were null.
bool IsIncomplete = false;

public:
typedef TreeTransform<TemplateInstantiator> inherited;
Expand Down Expand Up @@ -1373,6 +1376,9 @@ namespace {
/// Returns the name of the entity being instantiated, if any.
DeclarationName getBaseEntity() { return Entity; }

/// Returns whether any substitution so far was incomplete.
bool getIsIncomplete() const { return IsIncomplete; }

/// Sets the "base" location and entity when that
/// information is known based on another transformation.
void setBase(SourceLocation Loc, DeclarationName Entity) {
Expand Down Expand Up @@ -1419,6 +1425,8 @@ namespace {
if (TemplateArgs.hasTemplateArgument(Depth, Index)) {
Result = TemplateArgs(Depth, Index);
TemplateArgs.setArgument(Depth, Index, TemplateArgument());
} else {
IsIncomplete = true;
}
}

Expand Down Expand Up @@ -1814,8 +1822,10 @@ Decl *TemplateInstantiator::TransformDecl(SourceLocation Loc, Decl *D) {
// template arguments in a function template, but there were some
// arguments left unspecified.
if (!TemplateArgs.hasTemplateArgument(TTP->getDepth(),
TTP->getPosition()))
TTP->getPosition())) {
IsIncomplete = true;
return D;
}

TemplateArgument Arg = TemplateArgs(TTP->getDepth(), TTP->getPosition());

Expand Down Expand Up @@ -1961,8 +1971,10 @@ TemplateName TemplateInstantiator::TransformTemplateName(
// template arguments in a function template, but there were some
// arguments left unspecified.
if (!TemplateArgs.hasTemplateArgument(TTP->getDepth(),
TTP->getPosition()))
TTP->getPosition())) {
IsIncomplete = true;
return Name;
}

TemplateArgument Arg = TemplateArgs(TTP->getDepth(), TTP->getPosition());

Expand Down Expand Up @@ -2044,8 +2056,10 @@ TemplateInstantiator::TransformTemplateParmRefExpr(DeclRefExpr *E,
// template arguments in a function template, but there were some
// arguments left unspecified.
if (!TemplateArgs.hasTemplateArgument(NTTP->getDepth(),
NTTP->getPosition()))
NTTP->getPosition())) {
IsIncomplete = true;
return E;
}

TemplateArgument Arg = TemplateArgs(NTTP->getDepth(), NTTP->getPosition());

Expand Down Expand Up @@ -2464,6 +2478,7 @@ TemplateInstantiator::TransformTemplateTypeParmType(TypeLocBuilder &TLB,
// template arguments in a function template class, but there were some
// arguments left unspecified.
if (!TemplateArgs.hasTemplateArgument(T->getDepth(), T->getIndex())) {
IsIncomplete = true;
TemplateTypeParmTypeLoc NewTL
= TLB.push<TemplateTypeParmTypeLoc>(TL.getType());
NewTL.setNameLoc(TL.getNameLoc());
Expand Down Expand Up @@ -2835,7 +2850,8 @@ TypeSourceInfo *Sema::SubstType(TypeLoc TL,
/// Deprecated form of the above.
QualType Sema::SubstType(QualType T,
const MultiLevelTemplateArgumentList &TemplateArgs,
SourceLocation Loc, DeclarationName Entity) {
SourceLocation Loc, DeclarationName Entity,
bool *IsIncompleteSubstitution) {
assert(!CodeSynthesisContexts.empty() &&
"Cannot perform an instantiation without some context on the "
"instantiation stack");
Expand All @@ -2846,7 +2862,10 @@ QualType Sema::SubstType(QualType T,
return T;

TemplateInstantiator Instantiator(*this, TemplateArgs, Loc, Entity);
return Instantiator.TransformType(T);
QualType QT = Instantiator.TransformType(T);
if (IsIncompleteSubstitution && Instantiator.getIsIncomplete())
*IsIncompleteSubstitution = true;
return QT;
}

static bool NeedsInstantiationAsFunctionType(TypeSourceInfo *T) {
Expand Down
2 changes: 1 addition & 1 deletion clang/test/CodeCompletion/variadic-template.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ void f() {
// The important thing is that we provide OVERLOAD signature in all those cases.
//
// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:%(line-5):7 %s -o - | FileCheck --check-prefix=CHECK-1 %s
// CHECK-1: OVERLOAD: [#void#]fun(<#T x#>, Args args...)
// CHECK-1: OVERLOAD: [#void#]fun(<#T x#>)
// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:%(line-7):10 %s -o - | FileCheck --check-prefix=CHECK-2 %s
// CHECK-2: OVERLOAD: [#void#]fun(int x)
// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:%(line-9):13 %s -o - | FileCheck --check-prefix=CHECK-3 %s
Expand Down
5 changes: 3 additions & 2 deletions clang/test/Index/complete-call.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,8 @@ struct Bar2Template : public BarTemplates {
// CHECK-CC22-NEXT: Objective-C interface

// RUN: c-index-test -code-completion-at=%s:64:10 %s | FileCheck -check-prefix=CHECK-CC23 %s
// CHECK-CC23: OverloadCandidate:{ResultType void}{Text foo_12}{LeftParen (}{CurrentParameter int}{Comma , }{Placeholder int}{RightParen )} (1)

// CHECK-CC23: OverloadCandidate:{ResultType void}{Text foo_12}{LeftParen (}{CurrentParameter T}{Comma , }{Placeholder T}{RightParen )} (1)
// CHECK-CC23: Completion contexts:
// CHECK-CC23-NEXT: Any type
// CHECK-CC23-NEXT: Any value
Expand Down Expand Up @@ -702,7 +703,7 @@ struct Bar2Template : public BarTemplates {
// CHECK-CC46-NEXT: Objective-C interface

// RUN: c-index-test -code-completion-at=%s:84:12 %s | FileCheck -check-prefix=CHECK-CC47 %s
// CHECK-CC47: OverloadCandidate:{ResultType void}{Text foo_12}{LeftParen (}{CurrentParameter int}{Comma , }{Placeholder int}{RightParen )} (1)
// CHECK-CC47: OverloadCandidate:{ResultType void}{Text foo_12}{LeftParen (}{CurrentParameter T}{Comma , }{Placeholder T}{RightParen )} (1)
// CHECK-CC47: Completion contexts:
// CHECK-CC47-NEXT: Any type
// CHECK-CC47-NEXT: Any value
Expand Down
43 changes: 43 additions & 0 deletions clang/test/SemaCXX/cxx2b-deducing-this.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1012,6 +1012,49 @@ static_assert(__is_same(X<B{0}>, X<B{0}>));
static_assert(!__is_same(X<B{0}>, X<B{1}>));
} // namespace defaulted_compare

namespace static_overloaded_operator {
struct A {
template<auto N>
static void operator()(const char (&)[N]);
void operator()(this auto &&, auto &&);

void implicit_this() {
operator()("123");
}
};

struct B {
template<auto N>
void operator()(this auto &&, const char (&)[N]);
static void operator()(auto &&);

void implicit_this() {
operator()("123");
}
};

struct C {
template<auto N>
static void operator[](const char (&)[N]);
void operator[](this auto &&, auto &&);

void implicit_this() {
operator[]("123");
}
};

struct D {
template<auto N>
void operator[](this auto &&, const char (&)[N]);
static void operator[](auto &&);

void implicit_this() {
operator[]("123");
}
};

} // namespace static_overloaded_operator

namespace GH102025 {
struct Foo {
template <class T>
Expand Down
32 changes: 32 additions & 0 deletions clang/test/SemaTemplate/GH18291.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// RUN: %clang_cc1 -std=c++23 -verify %s

namespace t1 {
template<bool> struct enable_if { typedef void type; };
template <class T> class Foo {};
template <class X> constexpr bool check() { return true; }
template <class X, class Enable = void> struct Bar {};

template<class X> void func(Bar<X, typename enable_if<check<X>()>::type>) {}
// expected-note@-1 {{candidate function}}

template<class T> void func(Bar<Foo<T>>) {}
// expected-note@-1 {{candidate function}}

void g() {
func(Bar<Foo<int>>()); // expected-error {{call to 'func' is ambiguous}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly everyone accepts that https://godbolt.org/z/KG9cx4c6f
I think that this is the expected behavior as I am not aware of a disambiguation rule but it's worth double checking @AaronBallman @zygoloid

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a similar tests where the overload are constrained (both with subsumption, and with either one having a constraint nit satisfied)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not seeing wording that would allow us to accept this, but it is interesting that we all consistently implement the same rules and accept anyway. Should this be a Core issue in order to avoid breaking working code?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, I think this is contrived enough that it's unlikely to affect users.
And if it does we probably want to break them anyway... I'm not even sure what the exact behavior that determines the order is here: Which do you think should be more specialized?
Breaking code seems like a least worst options that making arbitrary ordering decisions.
In general there are tons of bugs pertaining to default arguments and ordering and we should fix them.
Adding more special cases to partial ordering seem like a bad idea.

If users find issues with the direction we can reconsider or divide a transition strategy (like we did with template template parameter ordering)

And as explain in #18291 - we are not self consistent

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion, but when 100% of implementations all do the same thing (which we do), it seems rather silly to expect implementations and users to change instead of asking whether the standard should reflect reality.

https://godbolt.org/z/johT7fva8

I don't see a reason to break user code except for conformance, but nobody conforms so...

Copy link
Contributor

@cor3ntin cor3ntin Aug 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related core issue CWG2160

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, after playing around a bit more... I don't think I can explain what our behavior is in practice, so maybe this is the correct approach. Certainly the code seems ambiguous to me, so I think the changes are defensible. I'm curious if @zygoloid has opinions here, but if not, I think it's fine to move forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a similar tests where the overload are constrained (both with subsumption, and with either one having a constraint nit satisfied)

Subsumption is not checked on deduction, that's only used later during overload resolution, which we are not changing, so I am not sure that test would be relevant to this change.

Relatedly, in a callback to another discussion with @zygoloid , we currently check if deduction would produce arguments which would not satisfy the constraints, but according to the specification this should not apply to partial ordering, which we fail to do so before this patch, and this patch introduces another case where we fail to follow the rules.

}
} // namespace t1

namespace t2 {
template <bool> struct enable_if;
template <> struct enable_if<true> {
typedef int type;
};
struct pair {
template <int = 0> pair(int);
template <class _U2, enable_if<__is_constructible(int &, _U2)>::type = 0>
pair(_U2 &&);
};
int test_test_i;
void test() { pair{test_test_i}; }
} // namespace t2
14 changes: 14 additions & 0 deletions clang/test/SemaTemplate/cwg2398.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,20 @@ namespace class_template {
// new-error@-1 {{ambiguous partial specialization}}
} // namespace class_template

namespace class_template_func {
template <class T1, class T2 = float> struct A {};

template <template <class T4> class TT1, class T5> void f(TT1<T5>);
// new-note@-1 {{candidate function}}

template <class T6, class T7> void f(A<T6, T7>) {};
// new-note@-1 {{candidate function}}

void g() {
f(A<int>()); // new-error {{call to 'f' is ambiguous}}
}
} // namespace class_template_func

namespace type_pack1 {
template<class T2> struct A;
template<template<class ...T3s> class TT1, class T4> struct A<TT1<T4>> ;
Expand Down
14 changes: 5 additions & 9 deletions clang/test/SemaTemplate/temp_arg_nontype.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -458,17 +458,13 @@ namespace dependent_nested_partial_specialization {
namespace nondependent_default_arg_ordering {
int n, m;
template<typename A, A B = &n> struct X {};
template<typename A> void f(X<A>); // expected-note {{candidate}}
template<typename A> void f(X<A, &m>); // expected-note {{candidate}}
template<typename A, A B> void f(X<A, B>); // expected-note 2{{candidate}}
template<typename A> void f(X<A>);
template<typename A> void f(X<A, &m>);
template<typename A, A B> void f(X<A, B>);
template<template<typename U, U> class T, typename A, int *B> void f(T<A, B>);
void g() {
// FIXME: The first and second function templates above should be
// considered more specialized than the third, but during partial
// ordering we fail to check that we actually deduced template arguments
// that make the deduced A identical to A.
X<int *, &n> x; f(x); // expected-error {{ambiguous}}
X<int *, &m> y; f(y); // expected-error {{ambiguous}}
X<int *, &n> x; f(x);
X<int *, &m> y; f(y);
}
}

Expand Down
7 changes: 3 additions & 4 deletions clang/test/SemaTemplate/temp_arg_type.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,10 @@ namespace deduce_noexcept {
void noexcept_function() noexcept;
void throwing_function();

template<typename T, bool B> float &deduce_function(T(*)() noexcept(B)); // expected-note {{candidate}}
template<typename T> int &deduce_function(T(*)() noexcept); // expected-note {{candidate}}
template<typename T, bool B> float &deduce_function(T(*)() noexcept(B));
template<typename T> int &deduce_function(T(*)() noexcept);
void test_function_deduction() {
// FIXME: This should probably unambiguously select the second overload.
int &r = deduce_function(noexcept_function); // expected-error {{ambiguous}}
int &r = deduce_function(noexcept_function);
float &s = deduce_function(throwing_function);
}

Expand Down
Loading
Loading