hsutter / cppfront

A personal experimental C++ Syntax 2 -> Syntax 1 compiler
Other
5.39k stars 232 forks source link

[BUG] calling a method on a class may do std::move on instance. #900

Closed tomFlowee closed 8 months ago

tomFlowee commented 8 months ago

Take this example code:

A: type = {
    connect: (inout this, s: std::string) = {
        std::cout << s;
    }
    foo: (inout this) = {}
}

main: (args) -> int = {
    a: A = ();
    a.connect("bla");
    // a.foo();
}

On gcc 13.2.1 this does not compile. If you uncomment the 'a.foo()', the error changes.

Looks like the generated code: CPP2_UFCS(connect)(std::move(a), "bla"); adds a std::move there that should not be there when calling a method on that same object. The error changes at uncomment because the 'std::move' is only generated for the last usage of the variable.

The final error is: note: no operand of the disjunction is satisfied

JohelEGP commented 8 months ago

Local variables are moved on their last use. In this case, you want to discard the final use. Like this (https://cpp2.godbolt.org/z/EP64Wc9z3):

main: (args) -> int = {
    a: A = ();
    a.connect("bla");
    a.foo();
    _ = a;
}

That moves the std::move in the final use to a discarded statement.

JohelEGP commented 8 months ago

See also #792.

hsutter commented 8 months ago

The current behavior is intentional as @JohelEGP mentioned. However, I agree that the failure mode is harder to figure out and it would be nice to have the error message (whether in cppfront or the Cpp1 compiler) be something closer to "can't call connect with an rvalue a, and because this is the last use of a in scope it is treated as an rvalue"...

hsutter commented 8 months ago

I'll close this as "by design," but I would welcome a PR that would improve the error message.

Thanks!

tomFlowee commented 8 months ago

Thanks for the work around. I added the dummy call on the class which avoids the compiler error too.

To give a big of background;

this is a simple case of create some instances in main, call a method to start multithreaded networking and wait for it to finish.

cpp: main: QCoreApplication app; Client client; client.connect(hostname, post); return app.exec();

So your idea of "can't call connect with an rvalue a, and because this is the last use of a in scope it is treated as an rvalue"...

substantially changes the concepts of C++ and requires learning a new way of thinking and working. I would argue that 90% of the C++ devs do not know lvalue rvalue. It just never comes up.

So, I would like to make the case that for calling methods on a class this design doesn't make sense. Your suggestion to learn the cppfont behavior (which should be hidden as per your 2022 presentation) in order to work around some side-effects with the assignement at the end is making it a pain instead of a boon.

The usage of std::move when calling a method on that same thing doesn't ever make sense. Does it? Which is what this bugreport is about.

JohelEGP commented 8 months ago

The usage of std::move when calling a method on that same thing doesn't ever make sense. Does it? Which is what this bugreport is about.

Here's a counter-example (https://cpp2.godbolt.org/z/KEdq38frP):

main: () -> int = {
  x: std::optional = new<int>(0);
  return std::unique_ptr<int>(x*)*;
  // _ = x; // This would make it error.
}
JohelEGP commented 8 months ago

The usage of std::move when calling a method on that same thing doesn't ever make sense. Does it? Which is what this bugreport is about.

I think we'd benefit more if you opened a dedicated discussion for your arguments on this part of the experiment.

Others could chime in, and maybe someday you could comment that you warmed up to the idea. Or come down more strongly that you feel it's failing you.

tomFlowee commented 8 months ago

@JohelEGP

Here's a counter-example

The case I'm making is one where we call a method on a variable.

A a;
a.foo();

So maybe the more wordy point is, std::move can be used for the last usage, unless it is about calling a method on that variable.

Because as far as I now it is never useful for the compiler to std::move a class instance to another scope in order to call simply call a method on that variable. Too much magic behavior.

JohelEGP commented 8 months ago

Sorry, here's the same, using .value(): https://cpp2.godbolt.org/z/3ajbne7vo.

tomFlowee commented 8 months ago

Again, not the usecase.

The assumption that 'doing something' with a variable is the determining factor for the lifetime of that variable is false.

Please refer to the background post I wrote to see a real life usecase of this. A class that holds application state, gets its 'start' method called and then is expected to live until the end of main().

Below that 'start()' call the application-state keeps on being useful. It owns threads, for instance. Having an on-stack variable defined in main is a VERY common usecase to define lifetime of such data. The idea that I should "discard" it with the underscore operator misses the fact that I don't want to discard it.

gregmarr commented 8 months ago

So this is one of those cases where the timing of the destructor call is important, like with a scope_guard, and the object needs to not be moved prior to the end of the scope. See #839

tomFlowee commented 8 months ago

Lets continue here; https://github.com/hsutter/cppfront/discussions/901