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

Incorrect if constexpr evaluation in nested generic lambda #58872

Open
cschreib opened this issue Nov 8, 2022 · 23 comments
Open

Incorrect if constexpr evaluation in nested generic lambda #58872

cschreib opened this issue Nov 8, 2022 · 23 comments
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"

Comments

@cschreib
Copy link

cschreib commented Nov 8, 2022

The test code below does not compile (in C++20 mode) with any version of clang I could test in Compiler Explorer (up to and including clang 15):

#include <type_traits>

// Generic type holder, for types that cannot be instanciated.
template<typename T>
struct type_holder {};

// Generic type to create a function with a given signature.
template<typename T>
struct foo;

template<typename R, typename ... Args>
struct foo<R(Args...)> {
    static R bar(Args...) {}
};

// Offending code.
template<typename T, typename ... IArgs>
auto test(IArgs... inputs) {
    [&]<typename R, typename... Args>(type_holder<R(Args...)>, Args... values) {
        // This always works.
        foo<T>::bar(values...);

        // This does not always work.
        if constexpr (std::is_same_v<R, void>) {
            if constexpr (sizeof...(Args) > 0) {
                foo<T>::bar(values...);
            } else {
                foo<T>::bar();
            }
        } else {
            int return_value = 0;
            if constexpr (sizeof...(Args) > 0) {
                return_value = foo<T>::bar(values...);
            } else {
                return_value = foo<T>::bar();
            }
        }
    }(type_holder<T>{}, inputs...);
}

int main() {
    // <source>:35:32: error: assigning to 'int' from incompatible type 'void'
    //                 return_value = foo<T>::bar();
    //                                ^~~~~~~~~~~~~
    test<void()>();

    //<source>:28:29: error: too few arguments to function call, expected 1, have 0
    //                foo<T>::bar();
    //                ~~~~~~~~~~~ ^
    test<void(int)>(1);

    // works!
    test<int()>();

    //<source>:28:29: error: too few arguments to function call, expected 1, have 0
    //                foo<T>::bar();
    //                ~~~~~~~~~~~ ^
    test<int(int)>(1);

    return 0;
}

Test cases:

  1. As you can see from the errors reported, test<void()>() generates an error inside an if constexpr branch that should not be entered. std::is_same_v<R, void> should be true, but it took (or at least, also tried to compile) the false branch.
  2. Similar story with test<void(int)>(1). There it took the correct branch for the return type, but then took the wrong branch for the arguments. sizeof...(Args) > 0 should be true, but it took (or at least, also tried to compile) the false branch.
  3. test<int()>() works fine, for some reason (it is the case that corresponds to the else branch for all if constexpr checks).
  4. test<int(int)>(1) fails again and takes the wrong branch for the return value and the arguments.

Extra information:

  • The code compiles if using foo<R(Args...)> instead of foo<T>, which should be equivalent. Weirdly enough, as this seems orthogonal to the if constexpr issue.
  • The code always failed to compile with clang since generic lambda with explicit template parameters were added (clang 9 with -std=c++2a).
  • The code compiles with GCC 8 (-std=c++2a) and above.
  • The code compiles with MSVC 19.30 and above.
@EugeneZelenko EugeneZelenko added clang:frontend Language frontend issues, e.g. anything involving "Sema" and removed new issue labels Nov 8, 2022
@llvmbot
Copy link
Member

llvmbot commented Nov 8, 2022

@llvm/issue-subscribers-clang-frontend

@shafik
Copy link
Collaborator

shafik commented Nov 9, 2022

If the condition is not value-dependent then the discarded statement should not be instantiated stmt.if p2. I believe std::is_same_v<R, void> is not value-dependent.

Maybe @erichkeane might have a better idea

@erichkeane
Copy link
Collaborator

I think the problem is that std::is_same_v<R,void> IS dependent during the instantiation of 'test'. That is, the first thing we do is substitute T/IArgs into the entire body of test. SO, the definition of the dependent lambda in the instantiation IS dependent, and thus we have to evaluate the discarded statement.

It is only when the call to the lambda happens ((type_holder<T>{}, inputs...);) that std::is_same_v<R, void> is no longer dependent.

@cschreib
Copy link
Author

cschreib commented Nov 12, 2022

I'm not a language lawyer, so forgive me if I get this completely wrong :) But the linked standardese above talks about "value-dependent" expressions. The expressions here only involves types, hence this is never value-dependent?

Edit: reading some more about it, value-dependent doesn't mean what I think it means.

Still, it seems to me the body of the inner lambda, which is a template, shouldn't be instanciated until called, regardless of whether it refers to T or not.

@erichkeane
Copy link
Collaborator

erichkeane commented Nov 12, 2022

See here: https://eel.is/c++draft/temp.dep.constexpr#2.2

 An id-expression is value-dependent if:
...
it is type-dependent,

As far as the instantiation: The BODY of the lambda gets partially instantiated as a part of instantiating test here. I can't find the prose at the moment, but I seem to remember that what you're doing above is Ill-Formed, No Diagnostic Required, and Clang's instantiation of test there is diagnosing (despite not being required). Would love it if an actual core expert like @hubert-reinterpretcast could confirm for me.

@cschreib
Copy link
Author

cschreib commented Nov 13, 2022

I have refactored the original code to not use explicit template parameters in the inner lambda, and the issue still occurs. This confirms this isn't specific to lambdas with explicit template parameters, and applies to any nested generic lambda.

Modified code without explicit template parameters in the lambda.
#include <type_traits>

// Generic type holder, for types that cannot be instanciated.
template<typename T>
struct type_holder {};

// Generic type to create a function with a given signature.
template<typename T>
struct foo;

template<typename R, typename ... Args>
struct foo<R(Args...)> {
    static R bar(Args...) {}
};

// Generic type trait to extract the return type of a function signature.
template<typename T>
struct return_type_of_t;

template<typename R, typename ... Args>
struct return_type_of_t<type_holder<R(Args...)>> {
    using type = R;
};

template<typename T>
using return_type_of = typename return_type_of_t<T>::type;

// Offending code.
template<typename T, typename ... IArgs>
auto test(IArgs... inputs) {
    [&](auto holder, auto... values) {
        // This always works.
        foo<T>::bar(values...);

        // This does not always work.
        using R = return_type_of<decltype(holder)>;
        if constexpr (std::is_same_v<R, void>) {
            if constexpr (sizeof...(values) > 0) {
                foo<T>::bar(values...);
            } else {
                foo<T>::bar();
            }
        } else {
            int return_value = 0;
            if constexpr (sizeof...(values) > 0) {
                return_value = foo<T>::bar(values...);
            } else {
                return_value = foo<T>::bar();
            }
        }
    }(type_holder<T>{}, inputs...);
}

int main() {
    test<void()>(); // complains on line 35 'assigning to 'int' from incompatible type 'void''
    test<void(int)>(1); // complains on line 28 'too few arguments to function call, expected 1, have 0'
    test<int()>(); // works!
    test<int(int)>(1); // complains on line 28 'too few arguments to function call, expected 1, have 0'
    return 0;
}

That being said, I don't think the problem is about std::is_same_v<R, void>, because if I replace all instances of foo<T>::* by other functions that do not depend on T, the code compiles fine even though the incorrect if constexpr branches would still fail to compile if instantiated.

Modified code to not use a class dependent on T inside the constexpr if.
#include <type_traits>

// Generic type holder, for types that cannot be instanciated.
template<typename T>
struct type_holder {};

// Set of non-template free functions to call later.
void free_bar() {}
void free_bar_with_param(int) {}
int free_bar_with_return() { return 0; }
int free_bar_with_return_and_param(int) { return 0; }

// Generic type trait to extract the return type of a function signature.
template<typename T>
struct return_type_of_t;

template<typename R, typename ... Args>
struct return_type_of_t<type_holder<R(Args...)>> {
    using type = R;
};

template<typename T>
using return_type_of = typename return_type_of_t<T>::type;

// This now compiles fine
template<typename T, typename ... IArgs>
auto test(IArgs... inputs) {
    [&](auto holder, auto... values) {
        using R = return_type_of<decltype(holder)>;
        if constexpr (std::is_same_v<R, void>) {
            if constexpr (sizeof...(values) > 0) {
                free_bar_with_param(values...);
            } else {
                free_bar();
            }
        } else {
            int return_value = 0;
            if constexpr (sizeof...(values) > 0) {
                return_value = free_bar_with_return_and_param(values...);
            } else {
                return_value = free_bar_with_return();
            }
        }
    }(type_holder<T>{}, inputs...);
}

int main() {
    test<void()>(); // works!
    test<void(int)>(1); // works!
    test<int()>(); // works!
    test<int(int)>(1); // works!
    return 0;
}

This suggests to me that the if constexpr branches get instantiated or not depending on their body, and not about the tested expression in the if constexpr. That does not seem right.

@cschreib cschreib changed the title Incorrect if constexpr evaluation in nested generic lambda with explicit template parameters Incorrect if constexpr evaluation in nested generic lambda Nov 13, 2022
@cschreib
Copy link
Author

I'll also point out that the standardese for if constexpr says only:

During the instantiation of an enclosing templated entity ([temp.pre]), if the condition is not value-dependent after its instantiation, the discarded substatement (if any) is not instantiated.

It does not say that "if the condition is value-dependent, the discarded substatement is instantiated." That would defeat the whole purpose of the feature.

Here is a counter-example. Inside foo<T>::bar<U>(), instantiating the else branch of the if constexpr will always fail to compile if T is void, regardless of U; this could be determined at the point where foo<void> is instantiated. Yet the compilation error only occurs when actually calling foo<void>::bar<int>, which instantiates the offending code:

#include <type_traits>

template<typename T>
struct foo {
    template<typename U>
    U bar() {
        if constexpr (std::is_same_v<U, void>) {
            // do nothing
        } else {
            return sizeof(T);
        }
    }
};

int main() {
    foo<void> f1; // instantiates 'foo' but not 'bar'. OK.
    f1.bar<void>(); // instantiates 'bar'; takes the 'if' branch, OK.
    f1.bar<int>(); // instantiates 'bar'; takes the 'else' branch, does no compile.
    return 0;
}

This works as I would expect. However, doing the same thing with a generic lambda nested inside a function, even when the lambda is never instantiated, generates an error in clang (but not GCC/MSVC):

#include <type_traits>

template<typename T>
void foo() {
    auto bar = []<typename U>() {
        if constexpr (std::is_same_v<U, void>) {
            // do nothing
        } else {
            return sizeof(T);
        }
    };
}

int main() {
    foo<void>(); // error, even though `decltype(bar)::operator()<U>` is never instantiated.
    return 0;
}

Is there something special about generic lambdas nested inside a template function, that they should get more aggressively instantiated?

@cschreib
Copy link
Author

I attempted to look at the code of clang to see what logic was implemented. It appears that, when an if constexpr is being transformed by template instantiation of the enclosing template (here foo being instantiated), the code doesn't handle differently the cases "not constexpr" and "constexpr but no known value". In both cases, ConstexprConditionValue (see below) is a null optional. Both the "then" and "else" branches are instantiated, and there is no apparent logic to tell the rest of the code "this is in an if constexpr and we don't know yet which branch will be taken, so hold off creating diagnostics".

// If this is a constexpr if, determine which arm we should instantiate.
llvm::Optional<bool> ConstexprConditionValue;
if (S->isConstexpr())
ConstexprConditionValue = Cond.getKnownValue();
// Transform the "then" branch.
StmtResult Then;
if (!ConstexprConditionValue || *ConstexprConditionValue) {
Then = getDerived().TransformStmt(S->getThen());
if (Then.isInvalid())
return StmtError();
} else {
Then = new (getSema().Context) NullStmt(S->getThen()->getBeginLoc());
}
// Transform the "else" branch.
StmtResult Else;
if (!ConstexprConditionValue || !*ConstexprConditionValue) {
Else = getDerived().TransformStmt(S->getElse());
if (Else.isInvalid())
return StmtError();
}

@erichkeane
Copy link
Collaborator

I have refactored the original code to not use explicit template parameters in the inner lambda, and the issue still occurs. This confirms this isn't specific to lambdas with explicit template parameters, and applies to any nested generic lambda.
Modified code without explicit template parameters in the lambda.

Right, the problem is the definition of the lambda being in the definition of 'test', the problem is only relevant to you because the lambda itself is generic in some way.

That being said, I don't think the problem is about std::is_same_v<R, void>, because if I replace all instances of foo<T>::* by other functions that do not depend on T, the code compiles fine even though the incorrect if constexpr branches would still fail to compile if instantiated.
Modified code to not use a class dependent on T inside the constexpr if.

I don't see what you mean? If they don't depend on T, they are going to not fail the instantiation with template parameter T

.

This suggests to me that the if constexpr branches get instantiated or not depending on their body, and not about the tested expression in the if constexpr. That does not seem right.

I'll also point out that the standardese for if constexpr says only:

During the instantiation of an enclosing templated entity ([temp.pre]), if the condition is not value-dependent after its instantiation, the discarded substatement (if any) is not instantiated.

It does not say that "if the condition is value-dependent, the discarded substatement is instantiated." That would defeat the whole purpose of the feature.

THAT phrase is an exception to the normal instantiation rules. So it doesn't have to say that, it is already true. It doesn't defeat the purpose of the feature, it is in fact part of how the feature is designed.

Here is a counter-example. Inside foo<T>::bar<U>(), instantiating the else branch of the if constexpr will always fail to compile if T is void, regardless of U; this could be determined at the point where foo<void> is instantiated. Yet the compilation error only occurs when actually calling foo<void>::bar<int>, which instantiates the offending code:

#include <type_traits>

template<typename T>
struct foo {
    template<typename U>
    U bar() {
        if constexpr (std::is_same_v<U, void>) {
            // do nothing
        } else {
            return sizeof(T);
        }
    }
};

int main() {
    foo<void> f1; // instantiates 'foo' but not 'bar'. OK.
    f1.bar<void>(); // instantiates 'bar'; takes the 'if' branch, OK.
    f1.bar<int>(); // instantiates 'bar'; takes the 'else' branch, does no compile.
    return 0;
}

This works as I would expect. However, doing the same thing with a generic lambda nested inside a function, even when the lambda is never instantiated, generates an error in clang (but not GCC/MSVC):

The difference HERE is that function templates, while part of their class-template, don't have their body instantiated until they are called. Else the use of members wouldn't work right in the body of hte function, this is to be expected.

#include <type_traits>

template<typename T>
void foo() {
    auto bar = []<typename U>() {
        if constexpr (std::is_same_v<U, void>) {
            // do nothing
        } else {
            return sizeof(T);
        }
    };
}

int main() {
    foo<void>(); // error, even though `decltype(bar)::operator()<U>` is never instantiated.
    return 0;
}

Is there something special about generic lambdas nested inside a template function, that they should get more aggressively instantiated?

The 'special' thing about lambdas is that their definitions exist inside of a function. Thus, we need to instantiate them as we instantiate the containing function template. However, with generic lambdas, we end up creating the 'new' generic version for the function template instantiation.

I attempted to look at the code of clang to see what logic was implemented. It appears that, when an if constexpr is being transformed by template instantiation of the enclosing template (here foo being instantiated), the code doesn't handle differently the cases "not constexpr" and "constexpr but no known value". In both cases, ConstexprConditionValue (see below) is a null optional. Both the "then" and "else" branches are instantiated, and there is no apparent logic to tell the rest of the code "this is in an if constexpr and we don't know yet which branch will be taken, so hold off creating diagnostics".

// If this is a constexpr if, determine which arm we should instantiate.
llvm::Optional<bool> ConstexprConditionValue;
if (S->isConstexpr())
ConstexprConditionValue = Cond.getKnownValue();
// Transform the "then" branch.
StmtResult Then;
if (!ConstexprConditionValue || *ConstexprConditionValue) {
Then = getDerived().TransformStmt(S->getThen());
if (Then.isInvalid())
return StmtError();
} else {
Then = new (getSema().Context) NullStmt(S->getThen()->getBeginLoc());
}
// Transform the "else" branch.
StmtResult Else;
if (!ConstexprConditionValue || !*ConstexprConditionValue) {
Else = getDerived().TransformStmt(S->getElse());
if (Else.isInvalid())
return StmtError();
}

Yep, this looks perfectly correct to me. I don't see a bug here. This is simply a case of Clang diagnosing an IFNDR that the other compilers don't.

@erichkeane
Copy link
Collaborator

I don't see any defect here still, this is working exactly how template instantiation and if-constexpr is supposed to work, so closing as 'not a defect'.

@EugeneZelenko EugeneZelenko closed this as not planned Won't fix, can't repro, duplicate, stale Nov 14, 2022
@EugeneZelenko EugeneZelenko added the invalid Resolved as invalid, i.e. not a bug label Nov 14, 2022
@cschreib
Copy link
Author

cschreib commented Nov 14, 2022

The closure type for a lambda-expression has a public inline function call operator (for a non-generic lambda) or function call operator template (for a generic lambda) ([over.call]) whose parameters and return type are described by the lambda-expression's parameter-declaration-clause and trailing-return-type respectively, and whose template-parameter-list consists of the specified template-parameter-list, if any.

The standard says that lambdas are function objects, implemented as a closure type with a function call operator. The body of the lambda is therefore that of a function, I don't see anything in the standard that says this body should be instanciated differently than other function templates.

The 'special' thing about lambdas is that their definitions exist inside of a function. Thus, we need to instantiate them as we instantiate the containing function template.

I think this is where our understanding differ. Why do you need to instanciate it? To creat the object bar in the example above, all you need is to instanciate the type of the closure object. Not the function call operator.

@cschreib
Copy link
Author

cschreib commented Nov 14, 2022

In fact:

An implementation shall not implicitly instantiate a function template, a variable template, a member template, a non-virtual member function, a member class or static data member of a templated class, or a substatement of a constexpr if statement ([stmt.if]), unless such instantiation is required.
[Note 5: The instantiation of a generic lambda does not require instantiation of substatements of a constexpr if statement within its compound-statement unless the call operator template is instantiated.
— end note]

Could this be reopened please?

@erichkeane
Copy link
Collaborator

In fact:

An implementation shall not implicitly instantiate a function template, a variable template, a member template, a non-virtual member function, a member class or static data member of a templated class, or a substatement of a constexpr if statement ([stmt.if]), unless such instantiation is required.
[Note 5: The instantiation of a generic lambda does not require instantiation of substatements of a constexpr if statement within its compound-statement unless the call operator template is instantiated.
— end note]

Could this be reopened please?

My understanding is that historically our belief was that the first part of this doesn't apply to a lambda body, as they're special in that regard.

That said, that note is one I've not noticed before, and I'd very like to make sure we get this right, so I'm hopeful one of my core experts can come along and clarify that we're wrong here. @hubert-reinterpretcast is our standards expert here.

As far as implementation, we'd have to stop instantiating-into the body of a lambda during normal tree-transform and delay said instantiation like we do with a member function.

@erichkeane
Copy link
Collaborator

That note is particularly confusing, because we AREN'T implicitly instantiating a the lambda, we're instantiating the containing definition, of which it is a a part. By my reading that is not allowing us to do the full instantiation here until we hit the call operator. BUT, that makes it somewhat of a non-sequitur here...

@hubert-reinterpretcast
Copy link
Collaborator

My understanding is that historically our belief was that the first part of this doesn't apply to a lambda body, as they're special in that regard.

My guess is that any such historical belief stemmed from the old determination of lambda captures, which involved determining odr-use. This was changed between C++17 and C++20.

@hubert-reinterpretcast
Copy link
Collaborator

My guess is that any such historical belief stemmed from the old determination of lambda captures, which involved determining odr-use. This was changed between C++17 and C++20.

Confirmed: See P0588R1 (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0588r1.html), adopted as DR: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/n4709.pdf

@cschreib
Copy link
Author

Thank you for the pointers! This issue is then a duplicate of #44851, so it should probably stay closed.

Out of curiosity, is the status of this DR tracked somewhere? I may be able to help implementing it if no one is working on it yet.

@erichkeane
Copy link
Collaborator

Thanks Hubert! That's exactly what I was looking for.

DR status is tracked here : https://clang.llvm.org/cxx_dr_status.html

So it would be tracked there as 1632 and 1913.

From the looks of it, implementation will likely quickly get into a "thar be dragons" part of the compiler. We need to suppress instantiation of the body of dependent lambdas (the easy part), but getInstantiationArgs likely needs updating for it (which is a function that had some serious problems) to properly handle this and other cases.

Ive been messing with it recently for other issues, so perhaps this is another I have to make dependent on that.

@erichkeane
Copy link
Collaborator

Note: I did a few hrs of poking at this, and got myself closer to 'fixed' than I thought with a lot less effort than i thought. Unfortunately, this is a fairly significant breaking change, so at one poitn we'll have to update a ton of tests: https://reviews.llvm.org/D138148

We ALSO have a ton of crashes. that come out of this, but I don't know if that is just me missing a hack somewhere, or putting the lambda-body-transformation work in teh wrong place. @cschreib : you had interest in helping on this, so feel free to poke at this further and update that review if you make progress.

@cschreib
Copy link
Author

Sorry, I thought I had replied to that last comment. I had a look at the proposed changes back then, and I think it flies way over my head. I could probably have helped if it was an easy change, but this isn't.

@erichkeane
Copy link
Collaborator

Sorry, I thought I had replied to that last comment. I had a look at the proposed changes back then, and I think it flies way over my head. I could probably have helped if it was an easy change, but this isn't.

Ah, no worries! I'd not seen anything, but got distracted with a ton of other things anyway, hopefully I'll have time to poke this again.

@JohelEGP

This comment was marked as duplicate.

@JohelEGP
Copy link

JohelEGP commented Oct 25, 2023

Is this the same issue, or should I open a new one (https://compiler-explorer.com/z/oh4M5xKW3):

struct t {
  void f();
  static void g(auto x) {
    [](auto obj) {
      if constexpr (requires { obj.f(); }) obj.f();
      else f(obj);
    }(x);
  }
};
<source>:6:12: error: call to non-static member function without an object argument
    6 |       else f(obj);
      |            ^
1 error generated.
Compiler returned: 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"
Projects
None yet
Development

No branches or pull requests

7 participants