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

Wrong evaluation of a if constexpr requires #63845

Closed
Fedr opened this issue Jul 13, 2023 · 9 comments · Fixed by #93206
Closed

Wrong evaluation of a if constexpr requires #63845

Fedr opened this issue Jul 13, 2023 · 9 comments · Fixed by #93206
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" concepts C++20 concepts

Comments

@Fedr
Copy link

Fedr commented Jul 13, 2023

The following program

#include <array>
#include <vector>

template <typename T>
constexpr std::size_t get_length(const T &t) {
    return t.size();
}

template <typename T, std::size_t N>
constexpr auto get_length(const std::array<T, N> &) {
    return std::integral_constant<std::size_t, N>();
}

static_assert( get_length(std::array<int, 3>())() == 3 );
static_assert( requires { typename std::array<int, get_length(std::array<int, 3>())>; } );

auto foo(auto length) {
    // If length could be evaluated at compile time,
    if constexpr (requires { typename std::array<int, length>; })
        return std::array<int, length>{};
    // Otherwise,
    else
        return std::vector<int>(length);
}

int main() {
    auto x = foo(get_length(std::vector<int>(5)));
    x.resize(1); // array does not have resize
    auto y = foo(get_length(std::array<int, 3>()));
    static_assert( y.size() == 3 ); // in Clang y is vector?
}

is accepted by GCC and MSVC. But in Clang y is a std::vector for some reason. Online demo: https://gcc.godbolt.org/z/h53zc3bjv

Related discussion: https://stackoverflow.com/a/76673819/7325599

Could you please check?

@EugeneZelenko EugeneZelenko added clang:frontend Language frontend issues, e.g. anything involving "Sema" consteval C++20 consteval and removed new issue labels Jul 13, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 13, 2023

@llvm/issue-subscribers-clang-frontend

@shafik
Copy link
Collaborator

shafik commented Jul 13, 2023

I believe this should be ok, size() is marked w/ _LIBCPP_CONSTEXPR_SINCE_CXX20. It is not obvious what the problem is, it would require some debugging.

@cor3ntin
Copy link
Contributor

In foo, length is not a constant expression (https://gcc.godbolt.org/z/orr6ndfj4), and cannot be used
as template argument of typename std::array<int, length> - from there, the requires is false and clang returns a vector.
Neither foo() nor y are constexpr, but even if they were, we can't return a vector from a constexpr function because C++ has no support for transient allocation.

Ultimately, the comment "If length could be evaluated at compile time" is what does not work here, function parameters are never constant expressions.

Now, the thing I'm not quite sure about is whether we should diagnose L19 in the requires or not? Presumably not, and if so I don't think there is a bug there.

I'm not quite sure what GCC and MSVC are doing, am I missing something?

@shafik
Copy link
Collaborator

shafik commented Jul 14, 2023

Yeah, I believe @cor3ntin is correct we can't return the std::vector (I completely missed that aspect) but if we make foo a constexpr function and use it directly in the static_assert it works fine: https://gcc.godbolt.org/z/rqePad5ex

int main() {
    auto x = foo(get_length(std::vector<int>(5)));
    x.resize(1); // array does not have resize
    //constexpr auto y = foo(get_length(std::array<int, 3>()));
    static_assert( foo(get_length(std::array<int, 3>())).size() == 3 ); // in Clang y is vector?
}

I think clang is correct here although the diagnostic could be way more informative.

CC @tbaederr

@cor3ntin
Copy link
Contributor

@shafik the salient difference https://gcc.godbolt.org/z/vcdr7nea7
But I don't see how GCC and MSVC get to return an array here..... right?

@efriedma-quic
Copy link
Collaborator

Consider the following, which is simplified to just the essentials (https://gcc.godbolt.org/z/774n7svhn):

#include <array>

auto foo() {
    std::integral_constant<std::size_t, 3> length;
    if constexpr (requires { typename std::array<int, length.value>; })
        return 0;
    else
        return 0LL;
}
static_assert(sizeof foo() == 4);

template<int x> auto bar() {
    std::integral_constant<std::size_t, 3> length;
    if constexpr (requires { typename std::array<int, length.value>; })
        return 0;
    else
        return 0LL;
}
static_assert(sizeof bar<0>() == 4);

template<typename T> auto baz() {
    T length;
    if constexpr (requires { typename std::array<int, length.value>; })
        return 0;
    else
        return 0LL;
}
static_assert(sizeof baz<std::integral_constant<std::size_t, 3>>() == 4);

clang disagrees with gcc/msvc about whether the first example is valid. For the second and third examples, clang agrees with gcc/msvc that an error shouldn't be printed.

Comparing the second and third examples, though, shows that clang isn't consistent with itself. Somehow, whether a local variable is type-dependent changes the result of the "if constexpr", which is pretty clearly wrong.

@Fedr
Copy link
Author

Fedr commented Aug 29, 2023

Even more simplified code:

emplate <bool>
struct A{};

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

static_assert( foo( std::true_type{} ) ); // fails in Clang

Online demo: https://gcc.godbolt.org/z/j6Mnqjvbz

Another simplified example without function template:

template <bool>
struct A{};

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

static_assert( foo() );

Online demo: https://gcc.godbolt.org/z/zGfo9a45P

Related discussion: https://stackoverflow.com/q/76697099/7325599

@cor3ntin cor3ntin added concepts C++20 concepts and removed consteval C++20 consteval labels Aug 29, 2023
@cor3ntin
Copy link
Contributor

@erichkeane you have an opinion on this?

@erichkeane
Copy link
Collaborator

I think Clang is wrong here, see the answer in @Fedr 's last stack overflow example. I think we're being overly aggressive with the 'local variable' check.

zyn0217 added a commit that referenced this issue Jun 4, 2024
This patch picks up #78598 with the hope that we can address such
crashes in `tryCaptureVariable()` for unevaluated lambdas.

In addition to `tryCaptureVariable()`, this also contains several other
fixes on e.g. lambda parsing/dependencies.

Fixes #63845
Fixes #67260
Fixes #69307
Fixes #88081
Fixes #89496
Fixes #90669
Fixes #91633
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" concepts C++20 concepts
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants