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] Incorrect error on capture list + lambda. #52720

Closed
tcanabrava opened this issue Dec 15, 2021 · 7 comments
Closed

[Clang] Incorrect error on capture list + lambda. #52720

tcanabrava opened this issue Dec 15, 2021 · 7 comments
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"

Comments

@tcanabrava
Copy link

tcanabrava commented Dec 15, 2021

~ ⌚ 11:24:54
$ clang --version
clang version 13.0.0
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

The following toy code generates the following error.

#include <iostream>

int main() {
    for (auto [test, enum_thing] : std::initializer_list<std::pair<int, int>> { {1,2}}) {
        [test, enum_thing] {
            std::cout << test << " " << enum_thing << std::endl;
        } ();
    }
}
~ ⌚ 11:23:47
$ clang++ -o test test.cpp -std=c++17
test.cpp:5:10: error: 'test' in capture list does not name a variable
        [test, enum_thing] {
         ^
test.cpp:5:16: error: 'enum_thing' in capture list does not name a variable
        [test, enum_thing] {
               ^
test.cpp:6:26: error: reference to local binding 'test' declared in enclosing function 'main'
            std::cout << test << " " << enum_thing << std::endl;
                         ^
test.cpp:4:16: note: 'test' declared here
    for (auto [test, enum_thing] : std::initializer_list<std::pair<int, int>> { {1,2}}) {
               ^
test.cpp:6:41: error: reference to local binding 'enum_thing' declared in enclosing function 'main'
            std::cout << test << " " << enum_thing << std::endl;
                                        ^
test.cpp:4:22: note: 'enum_thing' declared here
    for (auto [test, enum_thing] : std::initializer_list<std::pair<int, int>> { {1,2}}) {
                     ^
4 errors generated.

This is something that I use constantly with Qt based code.
Testing with g++ 11.0 and the code compiles correctly

@luizffgv
Copy link

The same behavior happens with -std=c++20, in which structured bindings should be able to be captured by lambdas.

Also happens when capturing outside a loop:

auto [_, enum_] = std::pair<int, int>{1, 2};
[_, enum_] { std::cout << _ << " " << enum_ << std::endl; }();

@zero9178
Copy link
Member

Last I recall, the above behaviour is actually correct for C++17 and GCC and MSVC simply implement it as a language extension, sadly leading to confusion. Specifically, Section 8.4.5.2 paragraph 8 states: "If a lambda-expression explicitly captures
an entity that is not odr-usable or captures a structured binding (explicitly or implicitly), the program is
ill-formed. " [1]

The workaround for your case in particular is to use an init list. Your lambda would be written as:

[test = test, enum_thing = enum_thing] {
    std::cout << test << " " << enum_thing << std::endl;
} ();

If you'd want to capture it by reference instead, writing &test = test would do the trick.

As @luizffgv notes however, it is definitely allowed in C++20 and that is indeed a missing feature/bug in clang.

[1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/n4713.pdf

@tcanabrava
Copy link
Author

Then, I'd argue that the bug is still valid.
instead of:

test.cpp:5:16: error: 'enum_thing' in capture list does not name a variable
        [test, enum_thing] {

The error should be

test.cpp:5:16: error: 'enum_thing' in capture list can't be captured, as it's an initializer list variable. those are not allowed in C++17, please try with c++20.
        [test, enum_thing] {

since the error is does not name a variable, while it does, makes it a bug on the compiler.

@luizffgv
Copy link

luizffgv commented Dec 15, 2021

I'd argue that enum_thing does not name a variable (if n4892 has no significant changes from ISO/IEC 14882:2020).

Section 9.6 paragraph 4 of n4892 states that, for a non-array type E (in this case std::pair<int, int>) such that std::tuple_size<E> names a complete class type with a member value , the identifiers introduced in the structured binding name lvalues that refer to objects bound to temporary variables, not the variables themselves.

@zero9178
Copy link
Member

One could still improve the error message however. Its probably not ideal that one has to know specifically that a structured binding is not considered a variable. Simply stating that a structured binding can't be captured in C++17 would be better eg. (most likely it will just be a warning/error in the future that capturing a structured binding is a C++20 extension, but that remains to be seen)

@ecatmur
Copy link

ecatmur commented Jan 19, 2022

See also #48582

@EugeneZelenko EugeneZelenko added clang:frontend Language frontend issues, e.g. anything involving "Sema" and removed new issue labels Aug 3, 2022
@llvmbot
Copy link
Collaborator

llvmbot commented Aug 3, 2022

@llvm/issue-subscribers-clang-frontend

cor3ntin added a commit that referenced this issue Aug 4, 2022
This completes the implementation of P1091R3 and P1381R1.

This patch allow the capture of structured bindings
both for C++20+ and C++17, with extension/compat warning.

In addition, capturing an anonymous union member,
a bitfield, or a structured binding thereof now has a
better diagnostic.

We only support structured bindings - as opposed to other kinds
of structured statements/blocks. We still emit an error for those.

In addition, support for structured bindings capture is entirely disabled in
OpenMP mode as this needs more investigation - a specific diagnostic indicate the feature is not yet supported there.

Note that the rest of P1091R3 (static/thread_local structured bindings) was already implemented.

at the request of @shafik, i can confirm the correct behavior of lldb wit this change.

Fixes #54300
Fixes #54300
Fixes #52720

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D122768
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

6 participants