Skip to content
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

[BUG] forward return passing style produces bad cpp1 return type #248

Closed
filipsajdak opened this issue Jan 31, 2023 · 26 comments
Closed

[BUG] forward return passing style produces bad cpp1 return type #248

filipsajdak opened this issue Jan 31, 2023 · 26 comments
Assignees
Labels
bug Something isn't working

Comments

@filipsajdak
Copy link
Contributor

In the current implementation of cppfront, we can use a forward return passing style to return a reference from a function or use decltype(auto) to return an exact cv-qualified type.

Unfortunately, in some cases, it produces the wrong cpp1 code. E.g., in the following code:

fun4: (i : int) -> forward int = {
    return i;
}

fun4_in: (in i : int) -> forward int = {
    return i;
}

fun4_copy: (copy i : int) -> forward int = {
    return i;
}

fun4_move: (move i : int) -> forward int = {
    return i;
}

fun4_forward: (forward i : int) -> forward int = {
    return i;
}

Generates (skipping boilerplate):

[[nodiscard]] auto fun4(cpp2::in<int> i) -> int&{ // should be `const int&`
    return i; 
}

[[nodiscard]] auto fun4_in(cpp2::in<int> i) -> int&{ // should be `const int&`
    return i; 
}

[[nodiscard]] auto fun4_copy(int i) -> int&{  // non-const reference bind to temporary (maybe decltype(auto)?)
    return std::move(i); 
}

[[nodiscard]] auto fun4_move(int&& i) -> int&{  // non-const reference bind to temporary (maybe decltype(auto)?)
    return std::move(i); 
}

[[nodiscard]] auto fun4_forward(auto&& i) -> int& // should be decltype(auto) to forward type
requires std::is_same_v<CPP2_TYPEOF(i), int>
#line 18 "tests/bug_forward_return.cpp2"
{   return CPP2_FORWARD(i); 
}
@filipsajdak filipsajdak added the bug Something isn't working label Jan 31, 2023
@filipsajdak
Copy link
Contributor Author

The issue found during discussion here: #247

@AbhinavK00
Copy link

This is offtopic, but copy passing convention seems... out of place. It does not convey intent like the other 5 conventions do and I think copy should be an annotation at call site rather than in the function signature. It should be made so that user calls method/function if he wants to pass the copy of an object to function. Something like,
func(v.copy());
and then the function can take the rvalue created as a move parameter.
Also, copy was not in the parameter passing paper, or was it?

@leejy12
Copy link

leejy12 commented Feb 1, 2023

[[nodiscard]] auto fun4(cpp2::in<int> i) -> int&{ // should be `const int&`
    return i; 
}

[[nodiscard]] auto fun4_in(cpp2::in<int> i) -> int&{ // should be `const int&`
    return i; 
}

Are these functions intentionally returning references to local variables? Shouldn't they just be int because int is a "trivial" type?
Or maybe forward returning a local variable is ill-formed by nature and cppfront should just reject this kind of function. Can someone clarify this?

@filipsajdak
Copy link
Contributor Author

@AbhinavK00, there are other issues regarding passing style (see: #231 (comment))

@hsutter mentioned the solution here: #198 (comment)

I'm currently supporting out, move, and forward arguments only. I'm thinking about copy or inout arguments in the future, but I don't plan to implement them now because I'm still thinking them through, and they probably require more work.

If I understand correctly, the idea is to emphasize intention on the call side by adding a passing style keyword:

// what is supported at the moment
fun(out x);     // x will be initialized (modified before use) in the fun
fun(move x);    // x moved to fun
fun(forward x); // x forward to fun

// what is planned
fun(copy x);    // x copied to fun
fun(inout x);   // x passed as inout parameter and cannot be rvalue

@AbhinavK00
Copy link

AbhinavK00 commented Feb 1, 2023

@filipsajdak I see, what I was saying is that I don't think there is a need for a copy passing convention, we already have five. We can always have user pass a copy by calling a method/function and take it in as a move parameter.
As for the annotations on callsite, there was this post on reddit which proposed the following

func(a)  //no annotation for in parameter 
func (b+)  //annotate with + to mark mutation, this encompasses both out and inout parameters 
func(c-)  //annotate with - to mark release, this encompasses both move and forward

You should check out full post for more info:
https://www.reddit.com/r/cppfront/comments/yt4cye/cppfront_callsite_parameter_specifiers/

@filipsajdak
Copy link
Contributor Author

Are these functions intentionally returning references to local variables?

@leejy12 I missed your comment. Currently, the cppfront is returning a reference to temporary, which is an error (it will not compile on the cpp1 compiler).

Returning const ref was my proposal, but of course, it is my mistake as it still is a reference to temporary - it will compile on the cpp1 side but with warnings. It should be just a copy for these cases.

We can do the proper thing for generic function as the following:

fun: (i : int) -> forward _ = {
    return i;
}

It will generate:

[[nodiscard]] auto fun(cpp2::in<int> i) -> decltype(auto){
    return i; 
}

And the return type will be deduced correctly. There is an issue when we want to specify the forward return type, e.g. int. A similar issue is on the argument side when you want to forward an argument of a specific type. There is a solution by having a template that limits the type with a requires clause:

fun3_forward: (forward i : int) -> forward _ = {
    return i;
}

Generates:

[[nodiscard]] auto fun3_forward(auto&& i) -> decltype(auto) // template or auto is needed to allow perfect forwarding
  requires std::is_same_v<CPP2_TYPEOF(i), int>              // requires limits its match to only int
{   
  return CPP2_FORWARD(i);
}

@hsutter
Copy link
Owner

hsutter commented Feb 1, 2023

Thanks. forward return of a local (including parameter) isn't intended, and I should issue a diagnostic for these examples.

The purpose of forward return is to let the caller use the result to modify some data that is owned by the function but outlives it, such as /*vector or basic_string*/<T>::operator[] /*const/non-const*/ which would return forward /*const/non-const*/ T.

@hsutter hsutter self-assigned this Feb 1, 2023
@filipsajdak
Copy link
Contributor Author

@hsutter Take a look at the rest test cases I have made - maybe it will bring you more cases.

fun1: (i) -> forward _ = {
    return i;
}

fun2: (i : _) -> forward _ = {
    return i;
}

fun3: (i : int) -> forward _ = {
    return i;
}

fun4: (i : int) -> forward int = {
    return i;
}

fun1_in: (in i) -> forward _ = {
    return i;
}

fun2_in: (in i : _) -> forward _ = {
    return i;
}

fun3_in: (in i : int) -> forward _ = {
    return i;
}

fun4_in: (in i : int) -> forward int = {
    return i;
}

fun1_inout: (inout i) -> forward _ = {
    return i;
}

fun2_inout: (inout i : _) -> forward _ = {
    return i;
}

fun3_inout: (inout i : int) -> forward _ = {
    return i;
}

fun4_inout: (inout i : int) -> forward int = {
    return i;
}

fun1_copy: (copy i) -> forward _ = {
    return i;
}

fun2_copy: (copy i : _) -> forward _ = {
    return i;
}

fun3_copy: (copy i : int) -> forward _ = {
    return i;
}

fun4_copy: (copy i : int) -> forward int = {
    return i;
}

fun1_move: (move i) -> forward _ = {
    return i;
}

fun2_move: (move i : _) -> forward _ = {
    return i;
}

fun3_move: (move i : int) -> forward _ = {
    return i;
}

fun4_move: (move i : int) -> forward int = {
    return i;
}

fun1_forward: (forward i) -> forward _ = {
    return i;
}

fun2_forward: (forward i : _) -> forward _ = {
    return i;
}

fun3_forward: (forward i : int) -> forward _ = {
    return i;
}

fun4_forward: (forward i : int) -> forward int = {
    return i;
}

// fun1_out: (out i) -> forward _ = { // error: (temporary alpha limitation) an 'out' parameter cannot be a deduced type (at ')')
//     return i;
// }

// fun2_out: (out i : _) -> forward _ = { // error: (temporary alpha limitation) an 'out' parameter cannot be a deduced type (at ')')
//     return i;
// }

fun3_out: (out i : int) -> forward _ = {
    i = 42;
    return i;
}

fun4_out: (out i : int) -> forward int = {
    return fun4_out_int(out i);
}

fun4_out_int: (out i : int) -> forward int = { 
    i = 42;
    return i;
}

Generates:

[[nodiscard]] auto fun1(auto const& i) -> decltype(auto){
    return i; 
}

[[nodiscard]] auto fun2(auto const& i) -> decltype(auto){
    return i; 
}

[[nodiscard]] auto fun3(cpp2::in<int> i) -> decltype(auto){
    return i; 
}

[[nodiscard]] auto fun4(cpp2::in<int> i) -> int&{
    return i; 
}

[[nodiscard]] auto fun1_in(auto const& i) -> decltype(auto){
    return i; 
}

[[nodiscard]] auto fun2_in(auto const& i) -> decltype(auto){
    return i; 
}

[[nodiscard]] auto fun3_in(cpp2::in<int> i) -> decltype(auto){
    return i; 
}

[[nodiscard]] auto fun4_in(cpp2::in<int> i) -> int&{
    return i; 
}

[[nodiscard]] auto fun1_inout(auto& i) -> decltype(auto){
    return i; 
}

[[nodiscard]] auto fun2_inout(auto& i) -> decltype(auto){
    return i; 
}

[[nodiscard]] auto fun3_inout(int& i) -> decltype(auto){
    return i; 
}

[[nodiscard]] auto fun4_inout(int& i) -> int&{
    return i; 
}

[[nodiscard]] auto fun1_copy(auto i) -> decltype(auto){
    return std::move(i); 
}

[[nodiscard]] auto fun2_copy(auto i) -> decltype(auto){
    return std::move(i); 
}

[[nodiscard]] auto fun3_copy(int i) -> decltype(auto){
    return std::move(i); 
}

[[nodiscard]] auto fun4_copy(int i) -> int&{
    return std::move(i); 
}

[[nodiscard]] auto fun1_move(auto&& i) -> decltype(auto){
    return std::move(i); 
}

[[nodiscard]] auto fun2_move(auto&& i) -> decltype(auto){
    return std::move(i); 
}

[[nodiscard]] auto fun3_move(int&& i) -> decltype(auto){
    return std::move(i); 
}

[[nodiscard]] auto fun4_move(int&& i) -> int&{
    return std::move(i); 
}

[[nodiscard]] auto fun1_forward(auto&& i) -> decltype(auto){
    return CPP2_FORWARD(i); 
}

[[nodiscard]] auto fun2_forward(auto&& i) -> decltype(auto){
    return CPP2_FORWARD(i); 
}

[[nodiscard]] auto fun3_forward(auto&& i) -> decltype(auto)
requires std::is_same_v<CPP2_TYPEOF(i), int>
#line 90 "tests/bug_forward_return_type.cpp2"
{   return CPP2_FORWARD(i); 
}

[[nodiscard]] auto fun4_forward(auto&& i) -> int&
requires std::is_same_v<CPP2_TYPEOF(i), int>
#line 94 "tests/bug_forward_return_type.cpp2"
{   return CPP2_FORWARD(i); 
}

// fun1_out: (out i) -> forward _ = { // error: (temporary alpha limitation) an 'out' parameter cannot be a deduced type (at ')')
//     return i;
// }

// fun2_out: (out i : _) -> forward _ = { // error: (temporary alpha limitation) an 'out' parameter cannot be a deduced type (at ')')
//     return i;
// }

[[nodiscard]] auto fun3_out(cpp2::out<int> i) -> decltype(auto){
    i.construct(42);
    return i.value(); 
}

[[nodiscard]] auto fun4_out(cpp2::out<int> i) -> int&{
    return fun4_out_int(&   i); 
}

[[nodiscard]] auto fun4_out_int(cpp2::out<int> i) -> int&{
    i.construct(42);
    return i.value(); 
}

@leejy12
Copy link

leejy12 commented Feb 6, 2023

Thanks. forward return of a local (including parameter) isn't intended, and I should issue a diagnostic for these examples.

Thanks for the answer. in, out, inout, move parameters can't be forward returned, but does it make sense to forward return forward parameters @hsutter?

The purpose of forward return is to let the caller use the result to modify some data that is owned by the function but outlives it, such as /vector or basic_string/::operator[] /const/non-const/ which would return forward /const/non-const/ T.

Cool. A cpp2 implementation could probably look something like this:

export template <typename T>
vector: class = {
private:
    data: *T;
	
public:
    operator[]: (pos: size_type) -> forward T = {
        return (data + pos)*;
    }

    operator[]: const (pos: size_type) -> forward const T = {
        return (data + pos)*;
    }
}

which will transpile to

export template <typename T>
class vector {
private:
    T* data;
	
public:
    T& operator[](size_type pos) {
        return (data + pos)*;
    }

    const T& operator[](size_type pos) const {
        return (data + pos)*;
    }
};

@hsutter
Copy link
Owner

hsutter commented Feb 6, 2023

Quick ack, since this was in my inbox this morning and I have a minute over morning coffee jot down notes for myself:

in, out, inout, move parameters can't be forward returned,

Briefly: in and move don't seem natural. But inout accepts only initialized lvalues, so forward-returning seems natural. And out similarly is guaranteed to be an initialized lvalue by the time you return.

does it make sense to forward return forward parameters

I think so. Examples of its usefulness today would be all functions that take a forwarding reference parameter and then return it with a decltype(auto) return type.

A cpp2 implementation could probably look something like this:

Pretty close, the syntax I'm contemplating is more like this which avoids special class/member grammar (quick notes: data members private by default and functions public by default, and explicit this which like all parameters is in by default which implies const, and defaults to the current type's name so there's no need to write the redundant : vector on the parameter type, and since they're single-return bodies we can use the existing short syntax for that):

vector: <T: type> class = {
    data: *T;
    size: size_type;
    // ...
    operator[]: (inout this, pos: size_type) -> forward T = std::span(data,size)[pos];
    operator[]: (this, pos: size_type) -> forward const T = std::span(data,size)[pos];
}

... something like that.

which will transpile to

Pretty much, but also with automatic bounds checking by default so you'll see the body emitted as { return cpp2::assert_in_bounds(std::span(data, size), pos); }, unless you disable bounds check injection with -s-.

@filipsajdak
Copy link
Contributor Author

filipsajdak commented Feb 12, 2023

@hsutter What about the following code

print: (inout o : std::ostream, x : sometype) -> forward std::ostream = {
    return o << "# (x.name)$\n";
}

The intention is to return a reference to allow chaining:

o.print(x1).print(x2).print(x3);

In the current cppfront implementation, the code mentioned generates the following cpp1 code:

[[nodiscard]] auto print(std::ostream& o, cpp2::in<sometype> x) -> std::ostream&{
    return o << "# " + cpp2::to_string(x.name) + "\n"; 
}

forward return of a local (including parameter) isn't intended, and I should issue a diagnostic for these examples.

The purpose of forward return is to let the caller use the result to modify some data that is owned by the function but outlives it, such as /vector or basic_string/::operator[] /const/non-const/ which would return forward /const/non-const/ T.

Does it mean that my code is illegal? Maybe there is other way to achieve that?

@hsutter
Copy link
Owner

hsutter commented Feb 12, 2023

@filipsajdak, yes this is the kind of case I meant with the later addition that "inout accepts only initialized lvalues, so forward-returning seems natural. And out similarly is guaranteed to be an initialized lvalue by the time you return." I think that's sensible as you wrote it, and translates to the expected Cpp1 code. It's in and move that don't seem reasonable to forward-return.

@AbhinavK00
Copy link

AbhinavK00 commented Feb 14, 2023

It's in and move that don't seem reasonable to forward-return.

in parameters DO make sense to be forward returned, think of std::min or related.
Or am I missing something?

Edit: #250 shows my point.

Edit 2: Maybe the template parameters can default to typename so we can write <T> instead of <T : type>.
and can mention the type/concept like
<size : int> if needed. I also got something to say about short template syntax for functions to express relation between types, should I write it?

@hsutter
Copy link
Owner

hsutter commented Mar 12, 2023

Re the earlier problem of returning a local via a -> forward return leading to a dangling reference, I think the above commit addresses that by diagnosing trying to return a local.

@hsutter
Copy link
Owner

hsutter commented Mar 23, 2023

I think the main issues in this thread have been addressed now, so I'll close it.

If I missed something, please reopen and point it out! Thanks again.

@hsutter hsutter closed this as completed Mar 23, 2023
@filipsajdak
Copy link
Contributor Author

filipsajdak commented Mar 24, 2023

@hsutter The following cases:

fun3: (i : int) -> forward _ = {
    return i;
}

fun4: (i : int) -> forward int = {
    return i;
}

fun3_in: (in i : int) -> forward _ = {
    return i;
}

fun4_in: (in i : int) -> forward int = {
    return i;
}

Generate:

[[nodiscard]] auto fun3(cpp2::in<int> i) -> auto&&{
    return i; 
}

[[nodiscard]] auto fun4(cpp2::in<int> i) -> int&{
    return i; 
}

[[nodiscard]] auto fun3_in(cpp2::in<int> i) -> auto&&{
    return i; 
}

[[nodiscard]] auto fun4_in(cpp2::in<int> i) -> int&{
    return i; 
}

All return reference to the local variable.

@hsutter
Copy link
Owner

hsutter commented Apr 1, 2023

@filipsajdak Catching up on this last comment, I've now pushed a commit that disallows forward-return of an in or move parameter. All four of your cases are now rejected and will not return a referenc to the parameter. Thanks!

@AbhinavK00
Copy link

Why in parameters? The above examples make perfect sense if we function takes something with size more than 2 pointers (I think it is any UDT now).

I think the right diagnosis isn't to ban forward return of in arguments but to pass the argument by reference everytime the function forward returns it. How would we write functions such as std::min etc. with this?

@filipsajdak
Copy link
Contributor Author

@AbhinavK00 scalars are passed by value when used in passing style. They cannot be forwarded as references.

JohelEGP referenced this issue Apr 1, 2023
See the wiki documentation page: https://github.com/hsutter/cppfront/wiki/Cpp2:-operator=,-this-&-that

See also the new `pure2-types-smf*.cpp2` test cases for examples.

Made Cpp1 assignment "return *this;", closes #277

Changed in<T> to pass fundamental types by value. Only those are known to be always complete. Partly addresses #270.

Also partly improves #282.
@JohelEGP
Copy link
Contributor

JohelEGP commented Apr 1, 2023

How would we write functions such as std::min etc. with this?

Like today's C++1, with inout parameters and decltype(auto) return type.

@AbhinavK00
Copy link

AbhinavK00 commented Apr 1, 2023

Why inout, we're not modifying the arguments. That'd be an edge-case.
And doesn't std::min take parameters by const ref? That would correspond to in.

Edit: @filipsajdak , I know they're passed by value but we can do an exception for when the function forward returns it.
Edit 2: Take a look at this comment

@JohelEGP
Copy link
Contributor

JohelEGP commented Apr 1, 2023

Edit: @filipsajdak , I know they're passed by value but we can do an exception for when the function forward returns it.

So you want to write a concrete min? Meaning, it's not a template and so an in parameter's type might be passed by value and not by reference?

I quickly changed in to inout before replying as I realized in might result in a copy. But as the comment you linked to reminded me, templates can't take advantage of that.

@AbhinavK00
Copy link

AbhinavK00 commented Apr 1, 2023

So you want to write a concrete min? Meaning, it's not a template and so an in parameter's type might be passed by value and not by reference?

Not really, my point is that the current fix disallows writing the templated function even though it's fully correct.

Edit: The solution descirbed by you using inout will be an exception to expressing intent.

@JohelEGP
Copy link
Contributor

JohelEGP commented Apr 1, 2023

Edit: The solution described by you using inout will be an exception to expressing intent.

That was a mistake. My intent was to reimplement std::min.

@leejy12
Copy link

leejy12 commented Apr 2, 2023

I think the right diagnosis isn't to ban forward return of in arguments but to pass the argument by reference everytime the function forward returns it.

I think using the above rule and templates is the correct way to implement std::min.

min: <T> (a: T, b: T) -> forward T = {
  if a < b {
    return a;
  }
  else {
    return b;
  }
}

Since there's no way of explicitly defining a parameter to be const reference in Cpp2, I think some special rules are needed for forward returning in parameters.

Also, I wonder if we could improve the semantics of in, maybe by building an actual Cpp2 compiler. Right now, templated arguments are always passed by const T&, but I wonder if we could choose between const T and const T& on Cpp2's side by type deducing T.

@JohelEGP
Copy link
Contributor

I think the right diagnosis isn't to ban forward return of in arguments but to pass the argument by reference everytime the function forward returns it.

I think using the above rule and templates is the correct way to implement std::min.

min: <T> (a: T, b: T) -> forward T = {
  if a < b {
    return a;
  }
  else {
    return b;
  }
}

Since there's no way of explicitly defining a parameter to be const reference in Cpp2, I think some special rules are needed for forward returning in parameters.

Also, I wonder if we could improve the semantics of in, maybe by building an actual Cpp2 compiler. Right now, templated arguments are always passed by const T&, but I wonder if we could choose between const T and const T& on Cpp2's side by type deducing T.

Just like #250 (comment), this no longer works: https://godbolt.org/z/EGPYevc16.

zaucy pushed a commit to zaucy/cppfront that referenced this issue Dec 5, 2023
zaucy pushed a commit to zaucy/cppfront that referenced this issue Dec 5, 2023
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

5 participants