Add function call inout passing style - closes #231

[filipsajdak]

In the current implementation of cppfront, the following code

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

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

Generates:

[[nodiscard]] auto f2(auto& x) -> auto{
    return x * 2; 
}

[[nodiscard]] auto main() -> int{
    auto x {21}; 
    std::cout << f2(std::move(x)) << std::endl;
}

and fail to compile as move from last use make the call to f2 incompatible with requirements of the f2 function (function requires lvalue reference and receives rvalue reference).

This change introduces the possibility of adding an inout passing style to function call to emphasize that it has to be passed without the move.
After this change, the original code can be fixed to:

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

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

Which will generate:

[[nodiscard]] auto f2(auto& x) -> auto{
    return x * 2; 
}

[[nodiscard]] auto main() -> int{
    auto x {21}; 
    std::cout << f2(      x) << std::endl;
}

Closes #231 - all regression tests pass.

[filipsajdak]

@hsutter, one side note. You use adjust_remaining_token_columns_on_this_line_visitor here:

cppfront/source/cppfront.cpp

Lines 3250 to 3252 in fbf55ad

	adjust_remaining_token_columns_on_this_line_visitor v(x.expr->position(), offset);
	current_args.push_back( {x.pass} );
	x.expr->visit(v, 0);

It should adjust the position of the argument (x in my case), which should remove space created by the lack of the inout keyword. All its functionality is in start(token& n, int) method:

cppfront/source/parse.h

Lines 5609 to 5618 in fbf55ad

	auto start(token& n, int) -> void
	{
	if (
	n.position().lineno == line_to_adjust_pos.lineno
	&& n.position().colno >= line_to_adjust_pos.colno
	)
	{
	n.position_col_shift(col_offset);
	}
	}

Unfortunately, in the case of function argument x (that is unqualified_id_node), this method is never called by calling x.expr->visit(v, 0); here

cppfront/source/cppfront.cpp

Line 3252 in fbf55ad

x.expr->visit(v, 0);

The issue is that the unqualified_id_node::visit(auto& v, int depth) method calls:

cppfront/source/parse.h

Line 589 in fbf55ad

v.start(*identifier, depth+1);

it almost matches adjust_remaining_token_columns_on_this_line_visitor::start method except first argument will be const, and adjust_remaining_token_columns_on_this_line_visitor expects token&.

Should I report it as a separate issue?

[JohelEGP]

Closes #231

Does it? It still doesn't match the set expectations:

[...]
cppfront should take into consideration the context in which the variable is used to avoid breaking the code on the cpp1 side.
-- #231 (comment)

To be able to do that, "move from last use" should inspect that the call would be well-formed with a std::move. There's complexity issues when there are multiple arguments, just like d0708 suggests.

[filipsajdak]

I did a dirty hack and I used const_cast (please don't tell anyone - I did it only to check if it will work when matched; UB might affect it as well). I have changed unqualified_id_node::visit(auto& v, int depth) to call:

        v.start(*const_cast<token*>(identifier), depth+1);

The adjust_remaining_token_columns_on_this_line_visitor::start(token&, int) was called but it did not change the generated file - there was still space left by lack of inout keyword (I set an offset to -6).

Something is not working.

[filipsajdak]

@JohelEGP solving #231 is about suppressing move-from-last-use by adding an explicit inout keyword on the function call side.

It is not about adding std::move but about NOT adding std::move.

[AbhinavK00]

Its not like we don't need std::move in this case, it's just that it's generated at the wrong place. The following code works just fine

[[nodiscard]] auto f2(auto& x) -> auto{
    return x * 2; 
}

[[nodiscard]] auto main() -> int{
    auto x {21}; 
    std::cout << std::move(f2(x)) << std::endl;
}

Last use of our variable is not when we pass it onto the function but when we actually print the result.
Two things I'd like to point out:
inout parameters should never be moved because if the user wants to change the parameter, he'll probably use it again. So, cppfront should be conservative and possibly refrain from moving inout parameters.
Second, let's first be sure about syntax we intend to use for marking function arguments because the way proposed here is incompatible (not really but kind of) with UFCS. Possibly open a new issue with a mention of all issues where it has come up?

[JohelEGP]

Last use of our variable is not when we pass it onto the function but when we actually print the result.

An inout parameter doesn't imply that the function returns a reference to the argument it was initialized with. The example returns an int by value. inout are like today's & input parameter that's read and modified.

[AbhinavK00]

An inout parameter doesn't imply that the function returns a reference to the argument it was initialized with. The example returns an int by value. inout are like today's & input parameter that's read and modified.

Ah yes, sorry for the mistake so in this case we should probably get the suggestion to change the parameter passing convention to in as the argument is not modified, is that right?

[JohelEGP]

No, the problem still exists. It's just that the function's body is irrelevant, as the problem is on the call side.

@JohelEGP solving #231 is about suppressing move-from-last-use by adding an explicit inout keyword on the function call side.

It is not about adding std::move but about NOT adding std::move.

You're right, but the linked issue hints at a more general problem.

#198, which Herb refers to in his reply, suggests that more would be needed to support inout argument passing.

As it stands, the inout argument passing style of this PR seems like a workaround of "move from last", which wouldn't be a problem if x were used further on the call site. It is OK for this to be its use case?

[filipsajdak]

@AbhinavK00, you are correct about that it should be in, and cppfront will be, in that case, report the diagnostic as described in d0708:

If the function is nonvirtual, at least one path must contain a non-const use of x (otherwise, the parameter should be in)

This issue is about something else. The correct version of this code is following:

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

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

To suppress the move on the last use of x I have followed the discussion of @jbatez & @hsutter here: #198, and I have proposed the use of the explicit inout keyword on the call side.

I propose to use inout as that was the least invasive change that enabled testing this approach and finding issues where it does not work as expected.

[AbhinavK00]

I propose to use inout as that was the least invasive change that enabled testing this approach and finding issues where it does not work as expected.

As @JohelEGP pointed out, this change does seem like a work-around and we probably need a more general solution for the more general problem. I would propose the the following change, what do you think about it

inout parameters should never be moved-from because if the user wants to change the parameter, he'll probably use it again. So, cppfront should be conservative and possibly refrain from moving inout parameters.

Yes, this would lose performance on some use cases but there are not really cases where you don't use the value after modifying it, I mean why modify it in first place if you won't use it again, that's just bad code. Should've been some form of nodiscard to detect it but it's not detected on cpp side so I guess not.

[filipsajdak]

After sleeping over this topic, I see some of the issues here. E.g., I would expect that call:

fun(inout x);

Will NOT match the following functions:

fun_in:   (in   x) -> _  {/* ... */} // now matches
fun_copy: (copy x) -> _  {/* ... */} // now matches
fun_move: (move x) -> _  {/* ... */} // now does not match
fun_out:  (out  x) -> _  {/* ... */} // now does not match

And SHOULD match the following:

fun_inout:   (inout   x) -> _  {/* ... */} // now matches
fun_forward: (forward x) -> _  {/* ... */} // now matches

I thought about it a little more and thought, what if we will require explicit argument passing on the call side (except for in, which will be the default)?

1. We will have a symmetry between a declaration and calling function:

fun_out(out x); // similar to above declaration
fun_copy(copy x); // symmetry
fun_in(x); // also symmetry as function can be declared as fun_in:(x) = ...

2. We will avoid accidental matches to functions that will make copies - the general rule will be if you explicitly say what you expect from the function and the function is doing something else, then it is not a match.

3. It will create a rule that arguments by default are passed with in argument passing - that will also make a symmetry with a function declaration.

I draw a matrix on potential matches between explicit argument passing on the call site vs. match to function declaration:

Call site \ Function declaration	in	copy	inout	move	forward	out
in	✅	❌	❌	❌	✅	❌
copy	❌	✅	❌	✅	✅	❌
inout	❌	❌	✅	❌	✅	❌?
move	❌	✅	❌	✅	✅	❌
forward	✅	✅	✅	✅	✅	✅
out	❌	❌	❌	❌	✅	✅

What do you think?

[AbhinavK00]

copy and move should match every parameter signature, no? In both cases, we're giving the function an rvalue and saying "I don't care what function does with this". And inout arguments should match out parameters. And we don't want forward to be at callsites, since it's about expressing intent and the caller doesn't care if side-effects of his call are directly by the function he calls or through some wrapped function.

Edit: About copy parameters, what are some use cases of a function saying "I want an prvalue no matter what"? I don't see the point of copy parameters and I think it should be a callsite only thing.

[filipsajdak]

@AbhinavK00 if you ask about function declaration and why it uses copy argument passing, please check this: d0708

[AbhinavK00]

@filipsajdak , the paper only talks about 5 passing conventions mainly

[filipsajdak]

@AbhinavK00 Oh, sorry. I was sure that it was described in the paper as well...

I quickly searched what was discussed with @hsutter in the issues/PRs. Please take a look here: #76 (comment)

Sorry for misleading you.

[AbhinavK00]

Ok, so Herb tried to keep in and copy parameters unified but couldn't do, so here's something I would like y'all's opinion on how to do away with copy parameters:

The user can always pass in a copy if he wants to any kind of function, so he'll just do

 func(a.copy());

when he wants to pass a copy.
On the callee side, the function can always be a in function and take the copy inside the function body rather than having copy parameters.
My main concern is that we have too many parameter passing conventions so one less would definitely help.

[hsutter]

Thanks for picking this up again. I'm going to reply on the original thread for #231... see you over there.

[AbhinavK00]

With this feature being on verge on getting merged, I think I'll start the talk about syntax here. I know Herb says not to worry about syntax but I think once features make it into the language, its hard to bring changes in them.

So here it is
in arguments: No need to mark them. As in is the default convention on the callee side, it would be the default on the caller side too.

out and inout parameters: These two could be unified because the caller does not care as much about the initialization of the value as much as the callee (it's the callee who has to worry about reading the value). An out function can also accept an inout argument, so both can use the same syntax like this:

func_call(arg1@, arg2@);

We put a suffix symbol after the argument name to symbolise "modification". It could be any symbol, I just used @ here. I will also put my preference down below.

move arguments: Like out and inout, we again use a symbol but a different one because moving is fundamentally different from modification. Here's an example with two options:

move_func_call(arg1!@);
move_func_call(arg1@@);

You may notice that both of the above options have the user using two symbols, it's because moving should be more "visible" than modification as it is an operation to be careful about.
As with the two options, one with ! preceding the symbol used for showing modification or the one with two of those symbols, any could be chosen but I prefer the first one (can't really explain but I think the user is less likely to make mistakes with that one).
Now this convention could be used anywhere where we use std::move today if it is implemented.

forward arguments: This one is special. For forward, the caller does not worry about whether the effect on his passed value is through a wrapped function or not, so it'll NOT have it's own convention. Think about it, the standard library may have many functions which just wrap other functions which the users don't know today (because they don't need to) so why should they be supposed to know it now? As a caller, I'm only worried about expressing my intention about what effect happens on my arguments and not about function implementation so there's no need of forward argument passing syntax.

foward_func_call_out(arg@);  //forwarding to an out function
forward_func_call_inout(arg@);  //forwarding to an inout function
forward_func_call_move(arg!@);  //forwarding to a move function
//no worries about implementation, we just express our intent

copy arguments: Not a fan of this one, I think this should not even exist but still I think the best way would be like this:

func(arg.copy());

We just copy our object and pass it along.

Reason
The simple reason is that, function calls exist in largely greater amount than function definitions, you define a function once but use it indefinitely. So, the syntax for function calls should be more terse and possibly as short as possible.

Symbols considered for using
It's not really important which symbol we choose but I think & looks good.
But wait, what about the adress of operator?
That could be changed to a function named addr so we would write

b := obj.addr(); //similar to ssize, huh
c := addr(obj); //less elegant

While both ways would be legal, the first way (present thanks to UFCS) is more similar to how we write it today but with some more keystrokes.

With that, the syntax would look like this:

func_in(arg);
func_out(arg&);
func_inout(arg&);
func_move_1(arg!&); // or 
func_move_2(arg&&);
func_copy(arg.copy());
//nothing special for forward
//BONUS
v := a!&; // similar to cpp's auto v = std::move(a);
//anymore you could think of

With &, the syntax is similar to how we pass references today but since there are no references in cpp2, we could say that we just took that syntax.
I'm not concerned about symbol but Herb rightly pointed out in this article that we only get so many symbols to use and therefore should be careful with deciding to use the newer ones.
BTW, I do support using ^ for pointers, can I open an issue suggesting how we could go about it? pls tell

Also, this is NOT similar to @filipsajdak 's suggestion where marking and not marking arguments do different things. Here, we HAVE to mark arguments in all cases as specified above. Not marking the arguments (if it is not in) would always result in an compile-time error.

[gregmarr]

out and inout parameters: These two could be unified because the caller does not care as much about the initialization of the value as much as the callee (it's the callee who has to worry about reading the value). An out function can also accept an inout argument, so both can use the same syntax like this:

@AbhinavK00 Just to be sure, by "these two could be unified", are you referring just to the potential decoration syntax on the caller side, and not merging the two on the function declaration/definition?

[AbhinavK00]

@gregmarr , yes just on the caller side. That comment proposes for no change on the callee side that we already have in cpp2.

[gregmarr]

Ok, so Herb tried to keep in and copy parameters unified but couldn't do

This discussion sounds vaguely familiar but I can't find it. Do you have a reference?

[AbhinavK00]

Here, thanks to @filipsajdak

[filipsajdak]

I need to work more on that to include comments from the issue: #231