[BUG] move from last use break code where variable is passed to function with `inout` argument passing

[filipsajdak]

In the current implementation of cppfront, the following code:

f2: (inout x) -> _ = {
    x *= 2;
    return x;
}

main: () -> int = {
    x := 21;
    std::cout << f2(x) << std::endl;
}

Passes cppfront:

tests/bug_inout_argument.cpp2... ok (all Cpp2, passes safety checks)

But failed to compile on the cpp1 side with an error:

tests/bug_inout_argument.cpp2:7:18: error: no matching function for call to 'f2'
    std::cout << f2(std::move(x)) << std::endl;
                 ^~
tests/bug_inout_argument.cpp2:1:20: note: candidate function [with x:auto = int] not viable: expects an lvalue for 1st argument
[[nodiscard]] auto f2(auto& x) -> auto{
                   ^
1 error generated.

When cppfront moves x on its last use it breaks the requirements of the f2 function that requires an lvalue reference but gets an rvalue reference.

Expectations

cppfront should take into consideration the context in which the variable is used to avoid breaking the code on the cpp1 side.

[hsutter]

Thanks!

One way to suppress this could be to require a mutating argument to be qualified with inout, which I've thought of and @jbatez suggested in #198.

I see there's a related issue in #230 which might also be solved with an inout argument (call-site) requirement...

I'll consider #230 and #231 together...

[hsutter]

Thanks for picking this up again in #294.

tldr

After reconsidering the examples, I think the status quo is a feature, not a bug, in Cpp2. I think the combination of parameter passing + move from definite last use is (elegantly? certainly naturally) exposing real user code bugs that were silent in Cpp1. This is very pleasing.

That said, I agree that an argument qualifier is the right answer. But understanding why the status quo is actually a feature is important because it will:

• help us name the qualifier,
• show where else the qualifier should be allowed,
• show why the qualifier will be needed rarely, and
• show why when the qualifier is needed it adds value (and a "programmer doing something odd here" flag to focus on during code review).

Why a feature: Diagnosing an unused output, like [[nodiscard]]

There are two features interacting here:

(1) Declaring parameter passing intent: This states the direction of data flow (in, inout, etc.).

(2) Move from definite last use: When we know the variable won't be used again, of course it's safe to move from so it seems this should be automatic and default.

Both features let the programmer declare their intent in a way that helps expose program bugs. Specifically:

(1) An inout or out (or Cpp1 non-const &) parameter is declaring that one of the function's outputs is via that argument, just as declaring a non-void return type is declaring that one of the function's outputs is via the return value. Those are the outputs, and ignoring an output is usually bad (but of course not always, see bottom). Just as Cpp2 makes [[nodiscard]] the default for return values, what you're encountering here is that it is naturally doing the same thing for inout arguments too, treating both declared output data flows similarly.

(2) A last use argument is diagnosing that the variable will no longer be used. If the last use is to an inout or out parameter, then not looking at it afterward is just the same as calling a function with a non-void return and never looking at the returned value (which is diagnosed in Cpp2 because of the enforced [[nodiscard]]).

So we are doing the user a favor by diagnosing this, just the same as if the user were ignoring a [[nodiscard]] return value.

And that's why I think that we should consider naming the opt-out for "unused out result" and "unused return value" with the same name, if there's a good name. They are the same case. (Sure, you sometimes want an opt-out, but only in rarer cases where you're relying on other side effects being performed by the function and really don't need the value, in which case the code should say so by writing discard or something.)

Example 1: Just return

Let's consider the two versions of the code... First, consider the version of the code you used in #294:

f2: (inout x) -> _ = {
    return x * 2;
}

main: () = {
    x := 21;
    std::cout << f2(x) << std::endl;
}

Compiling this with cppfront and then a Cpp1 compiler calls out f2(x) as invalid. But why? The compilers tell us it's because x is an rvalue, and the argument must be an lvalue. This is great, because it's true. There's something fishy.

What's fishy? It's f2... it declares its parameter as inout, but never writes to it. As you know, I aim eventually (not now) to emit a diagnostic for failure to have a non-const use of an inout parameter on at least one path somewhere in the function... when I implement that, the error will be flagged even sooner within the callee. Right now, the error is being flagged at the call site, which I expect to be usually still caught early at f2 unit test time because it will be common for even f2's initial toy test cases to do this... pass a last use, which exposes the bug in f2.

What's the solution? In this case, f2 should change its parameter to be in, and then everything compiles and runs.

Example 2: Also modify parameter

But what if f2 actually modifies its parameter? That brings us to the other version of your code, above...

f2: (inout x) -> _ = {
    x *= 2;
    return x;
}

main: () = {
    x := 21;
    std::cout << f2(x) << std::endl;
}

Again we get the error flagged, but this time the problem is at the call site, f2 is okay.

Consider why f2 is okay: Even though f2 is a little odd for redundantly emitting the same output value in two different output return paths (the inout argument and the return value), that's not wrong per se, and might be useful for chaining or whatever. So f2 is fine this time, in the sense that it's doing what it declared it would do... it's writing to its input argument, and it's returning a value.

But now the call site is definitely suspicious because it's making a call that is declared to modify its argument, but then never looks at the argument again. Ignoring an output is usually bad, at least by default.

Naming the opt-out

So I view this as a great feature of Cpp2... by:

• declaring the parameter passing direction, and
• moving from definite last use,

we naturally and automatically diagnosed failure to use an output. I like that a lot.

Furthermore, this is just like [[nodiscard]]. In both cases, we want an opt-out. But what's the right name? Given:

inout_func: ( inout x ) = { /*...*/ }
returning_func: () -> _ = { /*...*/ } 

Then consider this call site, where we want an explicit opt-out, and ideally the same word of power in both places since they're opting out of conceptually the same thing:

{
    x := 42;
    inout_func( SOMETHING x );
    (SOMETHING returning_func());
}

I want to think about the naming some more, but as a start I'm not sure inout works well for both:

//  What if "SOMETHING" were "inout"? Doesn't feel quite right...
{
    x := 42;
    inout_func( inout x );     // inout works pretty well here
    (inout returning_func());  // but not so well here
}

On the other hand, "discard" gives a nice first impression, and is symmetric with [[nodiscard]] and could connote "don't do anything special with, including don't move its guts along" as well as "discard this thing's value, I'm not going to use it from here onward":

//  What if "SOMETHING" were "discard"? I think I like it... "discard this value, I'm not going to use it after here"
{
    x := 42;
    inout_func( discard x );     // that word is a big red code review flag (good)
    (discard returning_func());  // and here with a clear meaning
}

It seems right to use the same opt-out word for unused inout/out arguments and unused return values. Getting the name right is important, though. This is something I want to sleep on further, but there's my brain dump for today. Thanks again.

[filipsajdak]

@hsutter Thank you for this summary - I think you synthesize it very well.

I agree that this is a similar thing as [[nodiscard]], and not using the inout or out argument is suspicious at the minimum.

I like the

discard returning_func();

But using it next to the function argument looks suspicious:

inout_func( discard x );

My first impression is that we want to discard the x variable - unfortunately, it is on the call side before it gets to the function. It could be misinterpreted as something will happen to x before a call to inout_func... or maybe it is just me.

Maybe we can add a passing style to clarify:

inout_func( discard inout x ); // maybe `discard out x` to emphasize that we discard output of the x

Another keyword to consider is unused:

x := 42;
inout_func( unused x );
(unused returning_func());

But still I would prefer to add a passing style:

x := 42;
inout_func( unused inout x ); // or unused out x
(unused returning_func());

[SebastianTroy]

Totally agree with all of this, discard does indeed feel like a good choice given the nodiscard symmetry. I have two(three) questions, Why not use [[discard]] instead of adding a new keyword? (I'm not against using just discard, merely curious, in fact is cpp2 avoiding the [[ xyz ]] syntax altogether?) Would it be reasonable to decorate a parameter with multiple passing intentions, i.e. in_or_inout_func: ( in|inout x ) = { /*.. Suggesting that the parameter's side effect is not mandatory and therefore not worth warning when the user doesn't use it? On 26 March 2023 22:06:18 Herb Sutter ***@***.***> wrote: Thanks for picking this up again in #294<#294>. tldr After reconsidering the examples, I think the status quo is a feature, not a bug, in Cpp2. I think the combination of parameter passing + move from definite last use is (elegantly? certainly naturally) exposing real user code bugs that were silent in Cpp1. This is very pleasing. That said, I agree that an argument qualifier is the right answer. But understanding why the status quo is actually a feature is important because it will: * help us name the qualifier, * show why the qualifier will be needed rarely, and * show why when the qualifier is needed it adds value (and a "programmer doing something odd here" flag to focus on during code review). Why a feature: Diagnosing an unused side effect, like [[nodiscard]] There are two features interacting here: (1) Intentional parameter passing: This states the direction of data flow (in, inout, etc.). (2) Definite move from last use: When we know the variable won't be used again, of course it's safe to move from so it seems this should be automatic and default. Both features let the programmer declare their intent in a way that helps expose program bugs. Specifically: (1) An inout (or Cpp1 non-const &) parameter is declaring that one of the function's outputs is via that argument, just as declaring a non-void return type is declaring that one of the function's outputs is via the return value. Those are the side effects, and ignoring a side effect is usually bad Just as Cpp2 makes [[nodiscard]] the default for return values, it is effectively doing the same thing for inout arguments too, treating both declared side effects similarly. (2) A last use argument is diagnosing that the variable will no longer be used. If the last use is to an inout or out parameter, then not looking at it afterward is just the same as calling a function with a non-void return and never looking at the returned value (which is diagnosed in Cpp2 because of the enforced [[nodiscard]]). So we are doing the user a favor by diagnosing this, just the same as if the user were ignoring a [[nodiscard]] return value. And that's why I think that we should consider naming the opt-out for "unused out result" and "unused return value" with the same name, if there's a good name. They are the same case. (Sure, you sometimes want an opt-out, but only in rarer cases where you're relying on other side effects being performed by the function and really don't need the value, in which case the code should say so by writing discard or something.) Example 1: Just return Let's consider the two versions of the code... First, consider the version of the code you used in #294<#294>: f2: (inout x) -> _ = { return x * 2; } main: () -> int = { x := 21; std::cout << f2(x) << std::endl; } Compiling this with cppfront and then a Cpp1 compiler calls out f2(x) as invalid. But why? The compilers tell us it's because x is an rvalue, and the argument must be an lvalue. This is great, because it's true. There's something fishy. What's fishy? It's f2... it declares its parameter as inout, but never writes to it. As you know, I aim eventually (not now) to emit a diagnostic for failure to have a non-const use of an inout parameter on at least one path somewhere in the function... when I implement that, the error will be flagged even sooner within the callee. Right now, the error is being flagged at the call site, which I expect to be usually still caught early at f2 unit test time because it will be common for even f2's initial toy test cases to do this... pass a last use, which exposes the bug in f2. What's the solution? In this case, f2 should change its parameter to be in. But what if f2 actually modifies its parameter? That brings us to the other version of your code, above... Example 2: Also modify parameter Second, consider the variation you posted above: f2: (inout x) -> _ = { x *= 2; return x; } main: () -> int = { x := 21; std::cout << f2(x) << std::endl; } Again we get the error flagged, but this time the problem is at the call site, f2 is okay. Consider why f2 is okay: Even though f2 is a little odd for redundantly emitting the same output value in two different output return paths (the inout argument and the return value), that's not wrong per se, and might be useful for chaining or whatever. So f2 is fine this time, it's doing what it declared it would do... it's writing to its input argument, and it's returning a value. But now the call site is definitely suspicious because it's making a call that is declared to modify its argument, but then never looks at the argument again. Ignoring a side effect is usually bad. Naming the opt-out So I view this as a great feature of Cpp2... by: * declaring the parameter passing direction, and * moving from definite last use, we naturally and automatically diagnosed failure to use a side effect. I like that a lot. Furthermore, this is just like [[nodiscard]]. In both cases, we want an opt-out. But what's the right name? Consider, given: inout_func: ( inout x ) = { /*...*/ } returning_func: () -> T = { /*...*/ } Then we have this call site: { x := 42; inout_func( SOMETHING x ); SOMETHING returning_func(); } I want to think about that some more, but I'm not sure inout works well for both: // What if "SOMETHING" were "inout"? Doesn't feel quite right... { x := 42; inout_func( inout x ); // inout works pretty well here inout returning_func(); // but not so well here } On the other hand, "discard" gives a nice first impression, and is symmetric with [[nodiscard]] and could connote "don't do anything special with, including don't move its guts along" as well as "discard this thing's value, I'm not going to use it from here onward": // What if "SOMETHING" were "discard"? I think I like it... "discard this value, I'm not going to use it after here" { x := 42; inout_func( discard x ); // that word is a big red code review flag (good) discard returning_func(); // and here with a clear meaning } It seems right to use the same opt-out word for unused inout/out arguments and unused return values. Getting the name right is important, though. This is something I want to sleep on further, but there's my brain dump for today. Thanks again. — Reply to this email directly, view it on GitHub<#231 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AALUZQIDQWDARPE6W44CL23W6CVURANCNFSM6AAAAAAT5JDQ2Y>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[AbhinavK00]

Agreed with Herb's analysis on why this is actually great. Small things I want to point out from @SebastianTroy's comment:

Why not use [[discard]] instead of adding a new keyword?

I have the same question, an attribute seems like the right fit for this job instead of "some new keyword popping out of nowhere".

Would it be reasonable to decorate a parameter with multiple passing intentions, i.e.
in_or_inout_func: ( in|inout x ) = { /*..

I have a question related to this, will we able to mark certain parameters in the function body as discard, this could be another way of doing the same thing. So, our example would become

f2: ([[discard]] inout x) -> _ = {
    x *= 2;
    return x;
}

main: () = {
    x := 21;
    std::cout << f2(x) << std::endl;
}

This would be a way to signify that the mutations it makes to x could be discarded, so cppfront would simply do a static cast of any rvalue before passing to such functions or not emit std::move at all.

[gregmarr]

I like the idea but also don't think discard makes sense as a parameter decoration, as you aren't discarding the entire thing, just the returned information, so maybe something like discard_return or discard_result. I think that would apply just as well to the return value of the function.

[hsutter]

Another keyword to consider is unused:

x := 42;
inout_func( unused x );
(unused returning_func());

That's a good candidate. Thoughts:

• Primarily, it has a connection with "definite last 'use'". The idea we want to convey is that there would be more "use" of x later in the scope, but the annotation on this use is that that later last use explicitly being omitted... "unused" might connote that, but maybe less strongly than "discard" or "discard_result"?
• Also, it has a symmetry with Cpp1 [[maybe_unused]].

Why not use [[discard]] instead of adding a new keyword? (I'm not against using just discard, merely curious, in fact is cpp2 avoiding the [[ xyz ]] syntax altogether?)

Cpp2 is currently using [[ ]] only for contracts, and I might change that too. In Cpp1 we spell some things as attributes, in part for syntax compatibility constraints which don't apply in Cpp2.

Would it be reasonable to decorate a parameter with multiple passing intentions, i.e. in_or_inout_func: ( in|inout x ) = { /*..
Suggesting that the parameter's side effect is not mandatory and therefore not worth warning when the user doesn't use it?

If we want to express that an output (parameter out data flow, or non-void return value) is discardable, we should have a consistent way to say that and again I would like to use the same word in both places.

For example:

inout_func_with_ignorable_result: ( ~SOMETHING inout x ) = { /*...*/ }
returning_func_with_ignorable_result: () -> ~SOMETHING _ = { /*...*/ }

I tag this as ~SOMETHING because it probably wants to be the inverse of the above SOMETHING.

Putting it together:

inout_func: ( inout x ) = { /*...*/ }
returning_func: () -> _ = { /*...*/ } 
inout_func_with_ignorable_result: ( ~SOMETHING inout x ) = { /*...*/ }
returning_func_with_ignorable_result: () -> ~SOMETHING _ = { /*...*/ }

{  // call site
    x := 42;
    inout_func( SOMETHING x );
    (SOMETHING returning_func());
}

Trying out @gregmarr's discard_result suggestion:

inout_func: ( inout x ) = { /*...*/ }
returning_func: () -> _ = { /*...*/ } 
inout_func_with_ignorable_result: ( discardable_result inout x ) = { /*...*/ }
returning_func_with_ignorable_result: () -> discardable_result _ = { /*...*/ }

{  // call site
    x := 42;
    inout_func( discard_result x );
    (discard_result returning_func());
}

That looks fairly decent at first blush. Clear, and a little verbose which is a good thing for an explicitly lossy escape hatch that we want to stand out. (Syntax colorizer writers, feel free to make it red... :) )

Trying out @filipsajdak's unused suggestion:

inout_func: ( inout x ) = { /*...*/ }
returning_func: () -> _ = { /*...*/ } 
inout_func_with_ignorable_result: ( maybe_unused inout x ) = { /*...*/ }
returning_func_with_ignorable_result: () -> maybe_unused _ = { /*...*/ }

{  // call site
    x := 42;
    inout_func( unused x );
    (unused returning_func());
}

This looks nice on the call site, but I worry that on the parameter it could imply that the name is not used in the callee body, which is what Cpp1 [[maybe_unused]] does.

Trying out a merger of the two, even more verbose on the declarations but again this is a case where verbosity can be a plus:

inout_func: ( inout x ) = { /*...*/ }
returning_func: () -> _ = { /*...*/ } 
inout_func_with_ignorable_result: ( maybe_unused_result inout x ) = { /*...*/ }
returning_func_with_ignorable_result: () -> maybe_unused_result _ = { /*...*/ }

{  // call site
    x := 42;
    inout_func( unused_result x );
    (unused_result returning_func());
}

Will think some more...

[filipsajdak]

@hsutter I like the way you make a synthesis of the proposed ideas.

Looking at the last one:

inout_func: ( inout x ) = { /*...*/ }
returning_func: () -> _ = { /*...*/ } 
inout_func_with_ignorable_result: ( maybe_unused_result inout x ) = { /*...*/ }
returning_func_with_ignorable_result: () -> maybe_unused_result _ = { /*...*/ }

{  // call site
    x := 42;
    inout_func( unused_result x );
    (unused_result returning_func());
}

How will it interact with move-of-last-use?

    x := 42;
    inout_func( unused_result x ); // will it just suppress the move?

And when we would define a function with maybe_unused_result inout:

    x := 42;
    inout_func_with_ignorable_result( x ); // will it just suppress the move?

Will it change the function's signature or add unused_result on the call side by default? Is this good or bad?

I like the focus on the intention and would like to know if we shall support defining functions in that way. I feel comfortable with -> maybe_unused_result _ on the return side, but having that on the inout argument feels like trying to fix some wrong design decision. Is there a use case where we use such an approach in the current cpp1 code?

But now the call site is definitely suspicious because it's making a call that is declared to modify its argument but then never looks at the argument again. Ignoring an output is usually bad, at least by default.

I like the above way of thinking, and for sure, I need to fix some cpp2 code just because cppfront complains about ignoring the return value from a function. Please note that I use the term FIX as, after second thought, my code was just somehow broken. I don't know if providing an easy way to opt out of this rule on the definition side is a good thing.

I like the idea of being explicit when something odd is going on. Ignoring output from a function is an odd thing that you might want to do, which is why you should have the possibility to add unused_result on the call side. That will focus the attention of the code reader.

Having the same thing on the definition side and not requiring anything on the call side will make things (from that perspective) worse, as when you read code, you don't check function definitions all the time - that might mislead the reader.