Skip to content

[QUESTION] How to properly capture this pointer in lambda #281

Closed
Closed
[QUESTION] How to properly capture this pointer in lambda#281
Assignees
hsutter
Labels
should be fixed now - please check

Description

@filipsajdak
filipsajdak
opened on Mar 14, 2023

In the current implementation of cppfront (4c52d2d) I am trying to capture this pointer to lambda:

html: namespace = {
    element: type = {
        children: html::children = ();
        parent: *element;

        set_parent:  (inout this, p : *element) = parent = p;

        operator=: (out this, forward cs : html::children) = {
            children = cs;
            parent = nullptr;
            for children do :(inout v) = {
                std::visit(:(inout c) = {
                    c.set_parent(this&$); // <-- trying to capture a pointer to this
                }, v);
            }
        }
    }
}

that produces (skipping boilerplate_:

for ( auto&& cpp2_range = children;  auto& v : cpp2_range ) {
    std::visit([_0 = (&(*this))](auto& c) -> void{
        CPP2_UFCS(set_parent, c, _0);
    }, v);
}

and fail to compile with an error:

[build] html/html.h2:115:31: error: 'this' cannot be implicitly captured in this context
[build]                     CPP2_UFCS(set_parent, c, _0);
[build]                               ^
[build] html/html.h2:115:21: note: explicitly capture 'this'
[build]                     CPP2_UFCS(set_parent, c, _0);
[build]                     ^

When capturing value before lambda creation like the following:

self := this&;
for children do :(inout v) = {
    std::visit(:(inout c) = {
        c.set_parent(self$);
    }, v);
}

Generates:

auto self {&(*this)}; 
for ( auto&& cpp2_range = children;  auto& v : cpp2_range ) {
    std::visit([_0 = self](auto& c) -> void{
        CPP2_UFCS(set_parent, c, _0);
    }, v);
}

That also ends with an error:

[build] html/html.h2:115:31: error: 'this' cannot be implicitly captured in this context
[build]                     CPP2_UFCS(set_parent, c, _0);
[build]                               ^
[build] html/html.h2:115:21: note: explicitly capture 'this'
[build]                     CPP2_UFCS(set_parent, c, _0);
[build]                     ^

When trying another syntax:

for children do :(inout v) = {
    std::visit(:(inout c) = {
        c.set_parent(this$&);
    }, v);
}

That worked and produced:

for ( auto&& cpp2_range = children;  auto& v : cpp2_range ) {
    std::visit([_0 = (*this)](auto& c) -> void{
        CPP2_UFCS(set_parent, c, &_0);
    }, v);
}

Unfortunately, this solution makes a copy of *this and calls set_parent with the address to the copy.

Question

Could you tell me how to capture a pointer to this in lambda?

Activity

  1. filipsajdak
    added
    suggestion
    on Mar 14, 2023
  2. hsutter
    self-assigned this
    on Mar 20, 2023

  3. hsutter commented on Mar 20, 2023

    @hsutter
    hsutter
    on Mar 20, 2023
    Owner

    Thanks, I'll take a look. I thought the this capture was only deprecated, not banned, as of C++20, but even so I should make sure we capture it the right way.

    Also: This example found a bug in the code I just pushed, because it has a lambda inside an operator=... I'll take a look at what's failing there too.

  4. hsutter commented on Mar 21, 2023

    @hsutter
    hsutter
    on Mar 21, 2023
    Owner

    Interim update: I can't repro your full example because I don't have all the types, but here's what I know so far...

    Capturing a pointer to this actually works fine, just like you did it. Here's a simpler example that works and does what you'd expect:

    test: type = {
    operator=: (out this) = { }

    f: (this) = {
        lambda := :()={ std::cout << this&$; };
        lambda();
    }
}

main: () = {
    t: test = ();
    t.f();
}

    The problem seems to be when a member function has a lambda that does a UFCS call to a member function (that's a mouthful), because that causes the member function name to appear unadorned. In this example the lambda contains the expression c.set_parent(this&$); which turns into CPP2_UFCS(set_parent, c, _0);. I suspect that if you change the code to c.set_parent(blah); you might find the same problem (i.e., it's not actually about capturing this), because that would generate CPP2_UFCS(set_parent, c, blah); which is a non-this-qualified use of set_parent.

    Here's a distilled repro:

    element: type = {
    set_parent:  (inout this, x: int) = { }

    f: (inout this, xx: int) = {
        lambda := :(inout c) = {
                c.set_parent(xx$);
            };
        lambda();
    }
}

    That's what I have so far...

  5. hsutter commented on Mar 22, 2023

    @hsutter
    hsutter
    on Mar 22, 2023
    Owner

    (Again, the answer to your original question "how to capture this pointer" is: Yes, the syntax you tried this&$ works fine. See above.)

    I think I've found the problem that your code hit, and it seems to be a bug in Clang and MSVC. What's happening is that Clang and MSVC are not properly discarding the not-taken if constexpr branch in the UFCS macro's lambda -- here is a minimal distilled case: https://godbolt.org/z/M47zzMsoT

    class test {
public: 
    auto f() { std::cout << "called f\n"; }

    auto g() {
        [](auto& obj) {
            if constexpr (requires{ obj.f(); }) {  obj.f();  }
            else {  f(obj);  }  // *** BUG: this branch is never taken, but Clang and MSVC consider it
                                // WORKAROUNDS: 1) comment out this not-taken branch
                                //          OR: 2) change lambda capture from [] to [&]
        }(*this);
    }
};

    I don't have access to your entire test case, but I'm about to push a commit that I've verified does fix a slightly-distilled version of your test case. @filipsajdak please check to see whether it fixes your whole test case.

    Thanks again for reporting, this was a subtle issue where (if I'm not mistaken) two of the three major commercial C++ compilers had the same bug! That doesn't happen every day.

  6. hsutter
    added a commit that references this issue on Mar 22, 2023

    Fix generation when `operator=` nests a lambda, and work around Clang…

    f9ca050
  7. hsutter
    added
    should be fixed now - please check
    and removed
    suggestion
    on Mar 22, 2023

  8. filipsajdak commented on Mar 22, 2023

    @filipsajdak
    filipsajdak
    on Mar 22, 2023
    ContributorAuthor

    Hi @hsutter,

    Thank you for working on that issue, and I am sorry I did not help with it so far - I am stuck with other work.

    I have been investigating this issue. I have found at least one issue (that still manifests in the current main (de25e0e). Also, I have simplified the code not to contain unknown types:

    element: type = {
    children: std::vector<std::variant<std::unique_ptr<element>>> = ();
    parent: *element;

    get_children: (this) -> forward _ = children;
    get_children: (inout this) -> forward _ = children;

    get_parent:  (this) -> forward _ = parent;
    get_parent:  (inout this) -> forward _ = parent;
    set_parent:  (inout this, p : *element) = parent = p;

    operator=: (out this, forward t : _ ) = {
        parent = nullptr;
    }

    operator=: (out this, forward cs : std::vector<std::unique_ptr<element>>) = {
        children = cs;
        parent = nullptr;
        for children do :(inout c) = {
            c*.set_parent(this&); // case 1: cpp2 works, cpp1 fail
        }
    }

    operator=: (out this, forward cs : std::vector<std::variant<std::unique_ptr<element>>>) = {
        children = cs;
        parent = nullptr;
        for children do :(inout v) = {
            std::visit(:(inout c) = {
                c*.set_parent(this&$); // case 2: cpp2 fail (ICE)
            }, v);
        }
    }
}

    There are two cases here.

    Case 1 is the original one. It fails even without using std::variant and std::visit.

    Case 2 causes ICE in cppfront - it looks like build_capture_lambda_intro_for() function is called two times that cause cap.str and cap.str_suppressed_move contains duplicated symbol inside - the dirty fix is below patch:

    diff --git a/source/cppfront.cpp b/source/cppfront.cpp
index c1744eb..14b43f7 100644
--- a/source/cppfront.cpp
+++ b/source/cppfront.cpp
@@ -2009,8 +2009,10 @@ public:
         for (auto& cap : captures.members)
         {
             assert(cap.capture_expr->cap_grp == &captures);
+            cap.str.clear();
             print_to_string(&cap.str, *cap.capture_expr, true);
             suppress_move_from_last_use = true;
+            cap.str_suppressed_move.clear();
             print_to_string(&cap.str_suppressed_move, *cap.capture_expr, true);
             suppress_move_from_last_use = false;
         }

    Case 2 still fail on cpp1 side.

  9. filipsajdak commented on Mar 22, 2023

    @filipsajdak
    filipsajdak
    on Mar 22, 2023
    ContributorAuthor

    @hsutter I am checking your latest change.

  10. filipsajdak commented on Mar 22, 2023

    @filipsajdak
    filipsajdak
    on Mar 22, 2023
    ContributorAuthor

    @hsutter what I can quickly check is that the following code:

    element: type = {
    children: std::vector<std::variant<std::unique_ptr<element>>> = ();
    parent: *element;

    get_children: (this) -> forward _ = children;
    get_children: (inout this) -> forward _ = children;

    get_parent:  (this) -> forward _ = parent;
    get_parent:  (inout this) -> forward _ = parent;
    set_parent:  (inout this, p : *element) = parent = p;

    operator=: (out this, forward t : _ ) = {
        parent = nullptr;
    }

    operator=: (out this, forward cs : std::vector<std::variant<std::unique_ptr<element>>>) = {
        children = cs;
        parent = nullptr;
        for children do :(inout v) = {
            std::visit(:(inout c) = {
                c*.set_parent(this&$); // cpp2 fail (ICE)
            }, v);
        }
    }
}

    with the patch:

    diff --git a/source/cppfront.cpp b/source/cppfront.cpp
index c1744eb..14b43f7 100644
--- a/source/cppfront.cpp
+++ b/source/cppfront.cpp
@@ -2009,8 +2009,10 @@ public:
         for (auto& cap : captures.members)
         {
             assert(cap.capture_expr->cap_grp == &captures);
+            cap.str.clear();
             print_to_string(&cap.str, *cap.capture_expr, true);
             suppress_move_from_last_use = true;
+            cap.str_suppressed_move.clear();
             print_to_string(&cap.str_suppressed_move, *cap.capture_expr, true);
             suppress_move_from_last_use = false;
         }

    Generates:

    #define CPP2_USE_MODULES         Yes

#include "cpp2util.h"


#line 1 "../tests/capture_this.cpp2"
class element;

//=== Cpp2 definitions ==========================================================

#line 1 "../tests/capture_this.cpp2"
class element  {
    private: std::vector<std::variant<std::unique_ptr<element>>> children {}; 
    private: element* parent; 

    public: [[nodiscard]] auto get_children() const -> auto&& { return children; }
    public: [[nodiscard]] auto get_children() -> auto&& { return children; }

    public: [[nodiscard]] auto get_parent() const -> auto&& { return parent; }
    public: [[nodiscard]] auto get_parent() -> auto&& { return parent; }
    public: auto set_parent(cpp2::in<element*> p) -> void { parent = p; }

    public: explicit element(auto&& t)
        : parent{ nullptr }
#line 13 "../tests/capture_this.cpp2"
{
    }
#line 12 "../tests/capture_this.cpp2"
    public: auto operator=(auto&& t) -> element& {
        children = ;
        parent = nullptr;
#line 13 "../tests/capture_this.cpp2"
        
        return *this;
#line 14 "../tests/capture_this.cpp2"
    }

    public: explicit element(auto&& cs)
requires std::is_same_v<CPP2_TYPEOF(cs), std::vector<std::variant<std::unique_ptr<element>>>>
#line 17 "../tests/capture_this.cpp2"
        
        : children{ CPP2_FORWARD(cs) }
        , parent{ nullptr }
#line 18 "../tests/capture_this.cpp2"
{
        for ( auto&& cpp2_range = children;  auto& v : cpp2_range ) {
            std::visit([&, _1 = (&(*this))](auto& c) -> void{
                CPP2_UFCS(set_parent, (*cpp2::assert_not_null(c)), _1);// cpp2 fail (ICE)
            }, v);
        }
    }
#line 16 "../tests/capture_this.cpp2"
    public: auto operator=(auto&& cs) -> element& 
requires std::is_same_v<CPP2_TYPEOF(cs), std::vector<std::variant<std::unique_ptr<element>>>>
#line 17 "../tests/capture_this.cpp2"
{       
        children = CPP2_FORWARD(cs);
        parent = nullptr;
#line 18 "../tests/capture_this.cpp2"

        for ( auto&& cpp2_range = children;  auto& v : cpp2_range ) {
            std::visit([&, _1 = (&(*this))](auto& c) -> void{
                CPP2_UFCS(set_parent, (*cpp2::assert_not_null(c)), _1);
            }, v);
        }
        return *this;
#line 24 "../tests/capture_this.cpp2"
    }
};

    It contains an issue in the implementation of auto operator=(auto&& t) -> element& - nothing is assigned to the children member:

        public: auto operator=(auto&& t) -> element& {
        children = ; // <<<< nothing is assigned to children
        parent = nullptr;
#line 13 "../tests/capture_this.cpp2"
        
        return *this;
#line 14 "../tests/capture_this.cpp2"
    }

    After correcting it manually, it compiles fine.

    That is all I can do now - running original test cases requires me to do the rebase of the branch with raw string literals, which I will do later.

    Thank you again for investigating it, and sorry for not helping more with it.

  11. filipsajdak commented on Mar 22, 2023

    @filipsajdak
    filipsajdak
    on Mar 22, 2023
    ContributorAuthor

    I have rebased my other branch, and I am not getting the previous errors, but I have plenty of new ones:

    I will investigate and get back to you with more content - it may be in the new issue.

            public: explicit spec(cpp2::in<std::string> n)
            : name{ n }
#line 27 "/Users/filipsajdak/dev/execspec/libs/execspec/include/execspec/spec.h2"
{
        }
#line 26 "/Users/filipsajdak/dev/execspec/libs/execspec/include/execspec/spec.h2"
        public: auto operator=(cpp2::in<std::string> n) -> spec& 
            , description{ "" }
            , config{  }
            , stages{  }
#line 27 "/Users/filipsajdak/dev/execspec/libs/execspec/include/execspec/spec.h2"
{           
            name = n;
#line 28 "/Users/filipsajdak/dev/execspec/libs/execspec/include/execspec/spec.h2"
            
            return *this;
#line 29 "/Users/filipsajdak/dev/execspec/libs/execspec/include/execspec/spec.h2"
}

    I see that there is an issue with generating: auto operator=(cpp2::in<std::string> n) -> spec& - cppfront generates something that looks like a constructors initialization list, but in assignment operator:

            public: auto operator=(cpp2::in<std::string> n) -> spec& 
            , description{ "" }
            , config{  }
            , stages{  }

  12. filipsajdak commented on Mar 24, 2023

    @filipsajdak
    filipsajdak
    on Mar 24, 2023
    ContributorAuthor

    @hsutter I have distilled three issues that I have reported as separate issues: #290, #291, and #292

  13. filipsajdak commented on Mar 27, 2023

    @filipsajdak
    filipsajdak
    on Mar 27, 2023
    ContributorAuthor

    @hsutter as this is captured properly and I have created three other issues for tracking. We can close this one.

  14. filipsajdak
    closed this as completedon Mar 27, 2023
  15. JohelEGP
    added a commit that references this issue on Nov 23, 2023

    Emit anonymous functions as `mutable` lambdas

    4bd0c04
  16. zaucy
    added a commit that references this issue on Dec 5, 2023

    Fix generation when `operator=` nests a lambda, and work around Clang…

    978be9a

  17. JohelEGP commented on Dec 6, 2023

    @JohelEGP
    JohelEGP
    on Dec 6, 2023
    Contributor

    Is there an MSVC bug report for #281 (comment)?
    I suppose the LLVM issue linked at #555 (comment) works for Clang.
    It's too bad that a function expression in a this function doesn't convert to a function pointer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

should be fixed now - please check

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions