Skip to content

[BUG] Last use of member function cannot be inout this #999

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

Closed
ProgrammingRainbow opened this issue Feb 26, 2024 · 6 comments
Closed

[BUG] Last use of member function cannot be inout this #999

ProgrammingRainbow opened this issue Feb 26, 2024 · 6 comments
Labels
bug Something isn't working

Comments

@ProgrammingRainbow
Copy link

ProgrammingRainbow commented Feb 26, 2024

I think this is similar to this bug #888 because i believe it's to do with the implicit move.

If the second to last member function is (inout this) the code runs as expected. But if you comment out or remove the "x.print();" making "x.inc();" that is an (inout this) the last use of x. then it falls apart.

myclass : type = {
    data: int = 42;
    more: std::string = std::to_string(42);

    // method
    print: (this) = {
        std::cout << "data: " << data << ", more: " << more << std::endl;
    }

    // non-const method
    inc: (inout this) = {
        data++;
    }
}

main: () = {
    x: myclass = ();
    x.inc();
    x.print();
}
❯ ../cppfront test3.cpp2
test3.cpp2... ok (all Cpp2, passes safety checks)

❯ g++ -I../include -std=c++23 test3.cpp -o test3

❯ ./test3
data: 43, more: 42
myclass : type = {
    data: int = 42;
    more: std::string = std::to_string(42);

    // method
    print: (this) = {
        std::cout << "data: " << data << ", more: " << more << std::endl;
    }

    // non-const method
    inc: (inout this) = {
        data++;
    }
}

main: () = {
    x: myclass = ();
    x.inc();
}
❯ ../cppfront test3.cpp2
test3.cpp2... ok (all Cpp2, passes safety checks)

❯ g++ -I../include -std=c++23 test3.cpp -o test3
test3.cpp2: In function ‘int main()’:
test3.cpp2:18:19: error: no match for call to ‘(main()::<lambda(Obj&&, Params&& ...)>) (std::remove_reference<myclass&>::type)’
   18 |     x.inc();
In file included from test3.cpp:6:
../include/cpp2util.h:927:1: note: candidate: ‘template<class Obj, class ... Params, bool IsNothrow> main()::<lambda(Obj&&, Params&& ...)>’
  927 | [LAMBDADEFCAPT]< \
      | ^
../include/cpp2util.h:942:59: note: in expansion of macro ‘CPP2_UFCS_’
  942 | #define CPP2_UFCS(...)                                    CPP2_UFCS_(&,CPP2_UFCS_IDENTITY,(),,__VA_ARGS__)
      |                                                           ^~~~~~~~~~
test3.cpp2:18:5: note: in expansion of macro ‘CPP2_UFCS’
   18 |     x.inc();
      |     ^~~~~~~~ 
../include/cpp2util.h:927:1: note:   template argument deduction/substitution failed:
  927 | [LAMBDADEFCAPT]< \
      | ^
../include/cpp2util.h:942:59: note: in expansion of macro ‘CPP2_UFCS_’
  942 | #define CPP2_UFCS(...)                                    CPP2_UFCS_(&,CPP2_UFCS_IDENTITY,(),,__VA_ARGS__)
      |                                                           ^~~~~~~~~~
test3.cpp2:18:5: note: in expansion of macro ‘CPP2_UFCS’
   18 |     x.inc();
      |     ^~~~~~~~ 
../include/cpp2util.h:927:1: note: constraints not satisfied
  927 | [LAMBDADEFCAPT]< \
      | ^
../include/cpp2util.h:942:59: note: in expansion of macro ‘CPP2_UFCS_’
  942 | #define CPP2_UFCS(...)                                    CPP2_UFCS_(&,CPP2_UFCS_IDENTITY,(),,__VA_ARGS__)
      |                                                           ^~~~~~~~~~
test3.cpp2:18:5: note: in expansion of macro ‘CPP2_UFCS’
   18 |     x.inc();
      |     ^~~~~~~~ 
test3.cpp2: In substitution of ‘template<class Obj, class ... Params, bool IsNothrow> main()::<lambda(Obj&&, Params&& ...)> [with Obj = myclass; Params = {}; bool IsNothrow = false]’:
test3.cpp2:18:19:   required from here
test3.cpp2:18:5:   required by the constraints of ‘template<class Obj, class ... Params, bool IsNothrow> main()::<lambda(Obj&&, Params&& ...)>’
../include/cpp2util.h:916:1: note: no operand of the disjunction is satisfied
  915 |    requires { CPP2_FORWARD(obj).CPP2_UFCS_REMPARENS QUALID TEMPKW __VA_ARGS__(CPP2_FORWARD(params)...); } \
      |    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  916 | || requires { MVFWD(CPP2_UFCS_REMPARENS QUALID __VA_ARGS__)(CPP2_FORWARD(obj), CPP2_FORWARD(params)...); }
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../include/cpp2util.h:934:14: note: in expansion of macro ‘CPP2_UFCS_CONSTRAINT_ARG’
  934 |     requires CPP2_UFCS_CONSTRAINT_ARG(MVFWD,QUALID,TEMPKW,__VA_ARGS__) { \
      |              ^~~~~~~~~~~~~~~~~~~~~~~~
../include/cpp2util.h:942:59: note: in expansion of macro ‘CPP2_UFCS_’
  942 | #define CPP2_UFCS(...)                                    CPP2_UFCS_(&,CPP2_UFCS_IDENTITY,(),,__VA_ARGS__)
      |                                                           ^~~~~~~~~~
test3.cpp2:18:5: note: in expansion of macro ‘CPP2_UFCS’
   18 |     x.inc();
      |     ^~~~~~~~ 
cc1plus: note: set ‘-fconcepts-diagnostics-depth=’ to at least 2 for more detail
@hsutter
Copy link
Owner

hsutter commented Mar 18, 2024

Thanks! I think this is resolved by 7041994, by making the error message better.

It's actually intentional that this is an error. Briefly, the reason is that if a definite last use of a local object modifies that object, then the code is not looking at the object again (because by definition it's a last use) and therefore trying to implicitly discard the new value. For a complete discussion, please see Design note: Explicit discard.

However, you are correct that the error message above is terrible. Fortunately, I just fixed that in commit 7041994... now for the latter program above, the diagnostic is much better, it starts with this:

demo.cpp2(18): error C2338: static_assert failed: 'this function call syntax tries 'obj.func(...)', then 'func(obj,...);' - both failed, but the first would have succeeded if obj were an lvalue - the likely problem is that this is a definite last use of the object (which automatically treats it as an rvalue / move candidate), and the function cannot accept an rvalue'

Followed by semi-useful information (showing MSVC output here):

demo.cpp2(18): note: the template instantiation context (the oldest one first) is
demo.cpp2(18): note: see reference to function template instantiation 'decltype(auto) main::<lambda_1>::operator ()<myclass,,false>(Obj &&) noexcept(false) const' being compiled
        with
        [
            Obj=myclass
        ]
demo.cpp2(18): error C2662: 'void myclass::inc(void) &': cannot convert 'this' pointer from 'Obj' to 'myclass &'
        with
        [
            Obj=myclass
        ]
demo.cpp2(18): note: A non-const reference may only be bound to an lvalue
demo.cpp2(11): note: see declaration of 'myclass::inc'
demo.cpp2(18): note: while trying to match the argument list '()'
demo.cpp2(18): error C3861: 'inc': identifier not found
demo.cpp2(18): note: 'inc': function was not declared in the template definition context and can be found only via argument-dependent lookup in the instantiation context

@hsutter
Copy link
Owner

hsutter commented Mar 18, 2024

However, based on this feedback I think changing the first error message as follows would be clearer about the actual problem, and advise how to fix it:

demo.cpp2(18): error C2338: static_assert failed: 'error: implicit discard of an object's modified value is not allowed - this function call modifies 'obj', but 'obj' is never used again in the function so the new value is never used - if that's what you intended, add another line '_ = obj;' afterward to explicitly discard the new value of the object'

Thanks!

@bluetarpmedia
Copy link
Contributor

bluetarpmedia commented Mar 18, 2024

I'm not convinced about the semantics of explicit discard in this situation.

I'm thinking about RAII-style types, e.g.:

raii: type = {
    operator=: (out this) = {
        f = nullptr;
        // Pretend this is: fopen(...);
    }
    operator=: (move this) = {
        // Pretend this is: fclose(...);
    }
    write: (inout this, something: int) = {
        // Pretend this is: fwrite(...);
    }
    f: *FILE;
}

main: () -> int = {
    
    {
        file: raii = ();
        file.write(55);
    }

    return 0;
}

I'd find it awkward to have to write _ = file to produce a valid program -- coming from the Cpp1 perspective where RAII behaviour is well established.

@hsutter hsutter reopened this Mar 18, 2024
@hsutter
Copy link
Owner

hsutter commented Mar 18, 2024

OK, reopened. See also my comment on #1002 which also came down to "this is about guard objects I think"...

@hsutter
Copy link
Owner

hsutter commented Mar 19, 2024

Thanks! See #1030, this should now be fixed in the above commit.

Both code examples above will now work.

@hsutter
Copy link
Owner

hsutter commented Nov 1, 2024

Brief update re Neil's raii type example: On reviewing these changes in a recent commit discussion, I think I'm going to switch gears and not move from last use for object whose names start with guard. I think that expresses the first intent that Greg summarized, non-intrusively at each use site in a way that's self-documenting.

While this specific example isn't quite a "guard" in the usual sense, I want to try out the path of guard prefixes to see if it addresses enough of the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants