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][Sema] Tweak tryCaptureVariable for unevaluated lambdas #93206

Merged
merged 4 commits into from
Jun 4, 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
3 changes: 3 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -710,6 +710,9 @@ Bug Fixes to C++ Support
- Correctly treat the compound statement of an ``if consteval`` as an immediate context. Fixes (#GH91509).
- When partial ordering alias templates against template template parameters,
allow pack expansions when the alias has a fixed-size parameter list. Fixes (#GH62529).
- Fixes several bugs in capturing variables within unevaluated contexts. Fixes (#GH63845), (#GH67260), (#GH69307), (#GH88081),
(#GH89496), (#GH90669) and (#GH91633).


Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
4 changes: 4 additions & 0 deletions clang/include/clang/AST/DeclBase.h
Original file line number Diff line number Diff line change
Expand Up @@ -2148,6 +2148,10 @@ class DeclContext {
getDeclKind() <= Decl::lastRecord;
}

bool isRequiresExprBody() const {
return getDeclKind() == Decl::RequiresExprBody;
}

bool isNamespace() const { return getDeclKind() == Decl::Namespace; }

bool isStdNamespace() const;
Expand Down
5 changes: 4 additions & 1 deletion clang/lib/Parse/ParseExprCXX.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1576,7 +1576,10 @@ ExprResult Parser::ParseLambdaExpressionAfterIntroducer(
TrailingReturnTypeLoc, &DS),
std::move(Attributes), DeclEndLoc);

Actions.ActOnLambdaClosureQualifiers(Intro, MutableLoc);
// We have called ActOnLambdaClosureQualifiers for parentheses-less cases
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any good reason to do this in 2 places, rather than just delay the above one to here? @cor3ntin ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, looking into ActOnLambdaClosureQualifiers, I don't see anything that (currently) depends on lambda parameters. I guess we can combine these two calls? I'd love to open a separate NFC PR for doing so, before landing this patch. :)

@erichkeane I retrospected your comment in #78598 (comment) and I appreciate the diagnostics for non-ODR uses - however that's much like a QoI issue to me and would it be OK to leave it as a follow-up? Let me know what you think and I'll add a FIXME then.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I'd be ok with this as a followup. But @cor3ntin is the one who knows the most about lambda stuff (particularly parsing, etc), so I'd like to see him approve this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we need to set whether the lambda has a mutable keyword as soon as the parameters, if present, are parsed, as that affect the meaning of subsequent id-expressions

When there are no parens, there can be a noexcept clause for example, and parsing that does requires knowing whether the lambda is mutable, so that we can adjust the type of any reference-to-capture.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we need to set whether the lambda has a mutable keyword as soon as the parameters, if present, are parsed, as that affect the meaning of subsequent id-expressions
When there are no parens, there can be a noexcept clause for example, and parsing that does requires knowing whether the lambda is mutable, so that we can adjust the type of any reference-to-capture.

Thanks, that makes sense! We currently set LSI->Mutable in ActOnLambdaClosureQualifiers() , and while I think it's possible to move it out of the function, I'm not opposed to leaving it as-is.

// above.
if (HasParentheses)
Actions.ActOnLambdaClosureQualifiers(Intro, MutableLoc);

if (HasParentheses && Tok.is(tok::kw_requires))
ParseTrailingRequiresClause(D);
Expand Down
10 changes: 10 additions & 0 deletions clang/lib/Sema/SemaExpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18831,13 +18831,23 @@ bool Sema::tryCaptureVariable(
DeclContext *VarDC = Var->getDeclContext();
DeclContext *DC = CurContext;

// Skip past RequiresExprBodys because they don't constitute function scopes.
while (DC->isRequiresExprBody())
DC = DC->getParent();

// tryCaptureVariable is called every time a DeclRef is formed,
// it can therefore have non-negigible impact on performances.
// For local variables and when there is no capturing scope,
// we can bailout early.
if (CapturingFunctionScopes == 0 && (!BuildAndDiagnose || VarDC == DC))
return true;

// Exception: Function parameters are not tied to the function's DeclContext
// until we enter the function definition. Capturing them anyway would result
// in an out-of-bounds error while traversing DC and its parents.
if (isa<ParmVarDecl>(Var) && !VarDC->isFunctionOrMethod())
return true;

const auto *VD = dyn_cast<VarDecl>(Var);
if (VD) {
if (VD->isInitCapture())
Expand Down
20 changes: 20 additions & 0 deletions clang/lib/Sema/SemaLambda.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1036,6 +1036,7 @@ void Sema::ActOnLambdaExpressionAfterIntroducer(LambdaIntroducer &Intro,
// be dependent, because there are template parameters in scope.
CXXRecordDecl::LambdaDependencyKind LambdaDependencyKind =
CXXRecordDecl::LDK_Unknown;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a blank line so I can comment here: I found it interesting that we asserted in line 1033 that LSI->NumExplicitTemplateParams is always 0... so the lines in the first branch of the if below are useless?
@cor3ntin

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like dead code from a past refactor, we should remove it
(template parameters are set later, in ActOnLambdaExplicitTemplateParameterList

if (LSI->NumExplicitTemplateParams > 0) {
Scope *TemplateParamScope = CurScope->getTemplateParamParent();
assert(TemplateParamScope &&
Expand All @@ -1046,6 +1047,25 @@ void Sema::ActOnLambdaExpressionAfterIntroducer(LambdaIntroducer &Intro,
LambdaDependencyKind = CXXRecordDecl::LDK_AlwaysDependent;
} else if (CurScope->getTemplateParamParent() != nullptr) {
LambdaDependencyKind = CXXRecordDecl::LDK_AlwaysDependent;
} else if (Scope *P = CurScope->getParent()) {
// Given a lambda defined inside a requires expression,
//
// struct S {
// S(auto var) requires requires { [&] -> decltype(var) { }; }
// {}
// };
//
// The parameter var is not injected into the function Decl at the point of
// parsing lambda. In such scenarios, perceiving it as dependent could
// result in the constraint being evaluated, which matches what GCC does.
while (P->getEntity() && P->getEntity()->isRequiresExprBody())
P = P->getParent();
if (P->isFunctionDeclarationScope() &&
llvm::any_of(P->decls(), [](Decl *D) {
return isa<ParmVarDecl>(D) &&
cast<ParmVarDecl>(D)->getType()->isTemplateTypeParmType();
}))
LambdaDependencyKind = CXXRecordDecl::LDK_AlwaysDependent;
}

CXXRecordDecl *Class = createLambdaClosureType(
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Sema/TreeTransform.h
Original file line number Diff line number Diff line change
Expand Up @@ -14232,7 +14232,7 @@ TreeTransform<Derived>::TransformLambdaExpr(LambdaExpr *E) {
// will be deemed as dependent even if there are no dependent template
// arguments.
// (A ClassTemplateSpecializationDecl is always a dependent context.)
while (DC->getDeclKind() == Decl::Kind::RequiresExprBody)
while (DC->isRequiresExprBody())
DC = DC->getParent();
if ((getSema().isUnevaluatedContext() ||
getSema().isConstantEvaluatedContext()) &&
Expand Down
164 changes: 164 additions & 0 deletions clang/test/SemaCXX/lambda-unevaluated.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -189,3 +189,167 @@ void recursive() {

}
}

namespace GH63845 {

template <bool> struct A {};

struct true_type {
constexpr operator bool() noexcept { return true; }
};

constexpr bool foo() {
true_type x{};
return requires { typename A<x>; }; // fails in Clang
}

constexpr bool foo(auto b) {
return requires { typename A<b>; };
}

static_assert(foo(true_type{}));
static_assert(foo());

}

namespace GH88081 {

// Test that ActOnLambdaClosureQualifiers is called only once.
void foo(auto value)
requires requires { [&] -> decltype(value) {}; }
// expected-error@-1 {{non-local lambda expression cannot have a capture-default}}
{}

struct S { //#S
S(auto value) //#S-ctor
requires requires { [&] -> decltype(value) { return 2; }; } {} // #S-requires

static auto foo(auto value) -> decltype([&]() -> decltype(value) {}()) { return {}; } // #S-foo

static auto bar(auto value) -> decltype([&] { return value; }()) {
return "a"; // #bar-body
}
};

S s("a"); // #use
// expected-error@#S-requires {{cannot initialize return object of type 'decltype(value)' (aka 'const char *') with an rvalue of type 'int'}}
// expected-error@#use {{no matching constructor}}
// expected-note@#S-requires {{substituting into a lambda expression here}}
// expected-note@#S-requires {{substituting template arguments into constraint expression here}}
// expected-note@#S-requires {{in instantiation of requirement here}}
// expected-note@#use {{checking constraint satisfaction for template 'S<const char *>' required here}}
// expected-note@#use {{requested here}}
// expected-note-re@#S 2{{candidate constructor {{.*}} not viable}}
// expected-note@#S-ctor {{constraints not satisfied}}
// expected-note-re@#S-requires {{because {{.*}} would be invalid}}

void func() {
S::foo(42);
S::bar("str");
S::bar(0.618);
// expected-error-re@#bar-body {{cannot initialize return object of type {{.*}} (aka 'double') with an lvalue of type 'const char[2]'}}
// expected-note@-2 {{requested here}}
}

} // namespace GH88081

namespace GH69307 {

constexpr auto ICE() {
constexpr auto b = 1;
return [=](auto c) -> int
requires requires { b + c; }
{ return 1; };
};

constexpr auto Ret = ICE()(1);

constexpr auto ICE(auto a) { // template function, not lambda
return [=]()
requires requires { a; }
{ return 1; };
};

} // namespace GH69307

namespace GH91633 {

struct __normal_iterator {};

template <typename _Iterator>
void operator==(_Iterator __lhs, _Iterator) // expected-note {{declared here}}
requires requires { __lhs; };

__normal_iterator finder();

template <typename >
void findDetail() {
auto makeResult = [&](auto foo) -> void {
finder() != foo;
// expected-error@-1 {{function for rewritten '!=' comparison is not 'bool'}}
};
makeResult(__normal_iterator{}); // expected-note {{requested here}}
}

void find() {
findDetail<void>(); // expected-note {{requested here}}
}
} // namespace GH91633

namespace GH90669 {

struct __normal_iterator {};

struct vector {
__normal_iterator begin(); // #begin
int end();
};

template <typename _IteratorR>
bool operator==(_IteratorR __lhs, int)
requires requires { __lhs; }
{}

template <typename PrepareFunc> void queued_for_each(PrepareFunc prep) {
prep(vector{}); //#prep
}

void scan() {
queued_for_each([&](auto ino) -> int { // #queued-for-each
for (auto e : ino) { // #for-each
// expected-error@#for-each {{cannot increment value of type '__normal_iterator'}}
// expected-note-re@#prep {{instantiation {{.*}} requested here}}
// expected-note-re@#queued-for-each {{instantiation {{.*}} requested here}}
// expected-note@#for-each {{implicit call to 'operator++'}}
// expected-note@#begin {{selected 'begin' function}}
};
});
}
} // namespace GH90669

namespace GH89496 {
template <class Iter> struct RevIter {
Iter current;
constexpr explicit RevIter(Iter x) : current(x) {}
inline constexpr bool operator==(const RevIter<Iter> &other) const
requires requires {
// { current == other.current } -> std::convertible_to<bool>;
{ other };
}
{
return true;
}
};
struct Foo {
Foo() {};
};
void CrashFunc() {
auto lambda = [&](auto from, auto to) -> Foo {
(void)(from == to);
return Foo();
};
auto from = RevIter<int *>(nullptr);
auto to = RevIter<int *>(nullptr);
lambda(from, to);
}
} // namespace pr89496
Loading