Skip to content

[BUG] Returning a tuple with unnamed members #325

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
ntrel opened this issue Apr 7, 2023 · 18 comments
Closed

[BUG] Returning a tuple with unnamed members #325

ntrel opened this issue Apr 7, 2023 · 18 comments
Labels
bug Something isn't working

Comments

@ntrel
Copy link
Contributor

ntrel commented Apr 7, 2023

Possibly this should be an enhancement request. Some of these errors may be because the feature is not implemented? Perhaps I'm using the wrong syntax? If so, better diagnostics would be nice, especially for f (see 4).

To Reproduce

d: () -> (int, int) = (1,2);
e: () -> (int, int) = { return (1,2); }
f: () -> _ = (5, "hi");
g: () -> (int, std::string) = (5, "hi");

main: () -> int = {
    auto [a,b] = f();
    assert(a == 5);
    assert(b == "hi");
}
  1. d can't be initialized by an expression:
rettup.cpp2(3,1): error: a function with named return value(s) must have a full { } body

But d doesn't used named return values. Also f can be initialized by a tuple expression without needing {}.

  1. Commenting out d, spurious compile error for e:
rettup.cpp2(1,16): error: int - variable must be initialized on every branch path

There is no declared variable.

  1. Commenting out e, spurious compile error for g:
rettup.cpp2(3,19): error: expected , in parameter list (at '::')
rettup.cpp2(3,19): error: missing function return after -> (at '::')

Probably it's parsing std as an identifier.

  1. Commenting out g, f compiles OK but doesn't declare or use a tuple struct so the c++ compiler errors:
#line 2 "old/rettup.cpp2"
[[nodiscard]] auto f() -> auto { return 5, "hi";  }

#line 5 "old/rettup.cpp2"
[[nodiscard]] auto main() -> int{
    cpp2::assert_in_bounds(auto, a, b) = f();
@ntrel ntrel added the bug Something isn't working label Apr 7, 2023
@JohelEGP
Copy link
Contributor

JohelEGP commented Apr 7, 2023

https://github.com/hsutter/cppfront/blob/main/regression-tests/mixed-multiple-return-values.cpp2 doesn't demonstrate it, but f: () -> (i: int, s: std::string) returns a structure with data members i and s, just like in the parameter list.

@ntrel
Copy link
Contributor Author

ntrel commented Apr 7, 2023

@JohelEGP Yes, this is about unnamed members. BTW is there a pure cpp2 way to write auto [a,b] = f();?

@JohelEGP
Copy link
Contributor

JohelEGP commented Apr 7, 2023

@JohelEGP Yes, this is about unnamed members.

Ah, I see. IIRC, you're right here.

Possibly this should be an enhancement request.

BTW is there a pure cpp2 way to write auto [a,b] = f();?

Not yet, it seems.

@filipsajdak
Copy link
Contributor

filipsajdak commented Apr 7, 2023

A couple of issues here.

  1. auto [a,b] = f(); is not a valid cpp2 syntax - the cpp2 rule is when you declare something there need to be : (colon) after the name - I assume it can look like the following: [a,b]:=f();. Also, structured binding still needs to be supported.

  2. you need to provide the type that will be returned (if there is no -> _ or -> type the return void is assumed): f: () -> _ = (5, "hi");. You use generic type _ but you did not provide the type on the return side so compiler cannot deduce it, and that is an easiest fix e.g.:

f: () -> _ = std::tuple(5, "hi");

main: () -> int = {
    t := f();
    assert(t.get<0>() == 5);
    assert(t.get<1>() == std::string("hi"));
}

Generates:

[[nodiscard]] auto f() -> auto { return std::tuple(5, "hi");  }

[[nodiscard]] auto main() -> int{
    auto t {f()}; 
    assert(CPP2_UFCS_TEMPLATE_0(get, (<0>), t)==5);
    assert(CPP2_UFCS_TEMPLATE_0(get, (<1>), std::move(t))==std::string("hi"));
}
  1. When you are using multivalue return you need to provide name and you need to initialize named return variables:
f: () -> (a:int, b:int) = { a = 5; b = 7; }

main: () -> int = {
    t := f();
    assert(t.a == 5);
    assert(t.b == 7);
}

Generates:

[[nodiscard]] auto f() -> f__ret{
                                cpp2::deferred_init<int> a;
                                cpp2::deferred_init<int> b;
#line 1 "../tests/tuple_return.cpp2"
a.construct(5); b.construct(7);
return  { std::move(a.value()), std::move(b.value()) }; }
[[nodiscard]] auto main() -> int{
    auto t {f()}; 
    assert(t.a==5);
    assert(std::move(t).b==7);
}

@ntrel
Copy link
Contributor Author

ntrel commented Apr 7, 2023

auto [a,b] = f(); is not a valid cpp2 syntax

Mixed C++ and C++2 is OK though AFAIU, it's done in regression-tests/mixed-multiple-return-values.cpp2.

I assume it can look like the following: [a,b]:=f();

Or (a, b) := f(); perhaps, for consistency with tuple syntax.

you need to provide the type that will be returned

OK, though it would be good to error when trying to infer the type of e.g. (5, "hi") if it's not allowed there. Also:

f: () -> _ = std::tuple(5, "hi");
main: () -> int = {
    auto [a,b] = f();
}
  1. The destructuring in main doesn't work, it generates:
    cpp2::assert_in_bounds(auto, a, b) = f();

When you are using multivalue return you need to provide name

OK, but if we have tuple types, it would be nice if you could use a tuple type as the return type and not have to declare member names.

@JohelEGP
Copy link
Contributor

JohelEGP commented Apr 7, 2023

OK, but if we have tuple types, it would be nice if you could use a tuple type as the return type and not have to declare member names.

That's why I replied:

@JohelEGP Yes, this is about unnamed members.

Ah, I see. IIRC, you're right here.

Possibly this should be an enhancement request.

IOW, this is a feature request.

It's not the problem solved by this feature, but sounds like a reasonable extension.

The destructuring in main doesn't work, it generates:

Cpp2 is distinguished from C++1 at the top-level declarations. Once you're in a C++1 top-level declaration, you stay in C++1. Same for Cpp2.

namespace x {
  // Only C++1 here.
}
x: namespace = {
  // Only Cpp2 here.
}

@filipsajdak
Copy link
Contributor

OK, but if we have tuple types, it would be nice if you could use a tuple type as the return type and not have to declare member names.

@ntrel you can return a tuple type:

f1: () -> _ = std::tuple(5, "hi"); // using deduced return type

f2: () -> std::tuple<int, std::string> = std::tuple(5, "hi"); // using explicit return type

@ntrel
Copy link
Contributor Author

ntrel commented Apr 9, 2023

Cpp2 is distinguished from C++1 at the top-level declarations. Once you're in a C++1 top-level declaration, you stay in C++1. Same for Cpp2.

OK, thanks. So deducing an expression constructing a std::tuple does work but the destructuring has to be done in a C++1 declaration.

I think these should be diagnosed (better) in a C++2 function:

f: () = {
    ...
    auto x = 1; // causes ill-formed initializer error but line is first line of `f`, not this line
    auto [x,y] = (1,2); // passes cppfront but errors in C++1
}

@ntrel
Copy link
Contributor Author

ntrel commented Apr 9, 2023

I think these should be diagnosed (better) in a C++2 function:

Implemented in #342.

@orent
Copy link

orent commented Apr 11, 2023

@JohelEGP Yes, this is about unnamed members. BTW is there a pure cpp2 way to write auto [a,b] = f();?

IIUC, comma expressions are gone from cpp2 so it could be just

a, b := f();

Can we have general destructuring assignment to any existing lvalues without necessarily defining new variables?

@filipsajdak
Copy link
Contributor

@orent Regarding using comma:

(Have I mentioned Cpp2 has no comma operator? 😁 Or at least it's trying very hard not to have one, even accidentally.)

Originally posted by @hsutter in #331 (comment)

@ntrel
Copy link
Contributor Author

ntrel commented Apr 15, 2023

Triage:

d: () -> (int, int) = (1,2);
e: () -> (int, int) = { return (1,2); }

  1. d can't be initialized by an expression:

The error should be that an identifier was expected, not int. I have a fix for that.
If this was to be supported the syntax would probably be d: () -> (:int, :int) = .

g: () -> (int, std::string) = (5, "hi");
rettup.cpp2(3,19): error: expected , in parameter list (at '::')

I think for that parameter the error is correct as std can be an identifier and the parser expects an unqualified identifier.

f: () -> _ = (5, "hi");
Commenting out g, f compiles OK but doesn't declare or use a tuple struct so the c++ compiler errors:

Type inference + initializer list could be diagnosed with an error.

There is also another bug:

b: () -> (a, b:_) = (1,2);

This should error that no type is declared for a and b. I have a fix for that.

ntrel added a commit to ntrel/cppfront that referenced this issue Apr 15, 2023
Note: type cannot be `_`.
Fixes most of hsutter#325.
@filipsajdak
Copy link
Contributor

I see that you treat (1,2) as a tuple (like Python does). Cpp2 does not have such syntax for the tuple.

(1,2) is used as a call to constructor:

t : std::tuple = (1,2); // creates tuple
v : std::vector = (1,2); // creates a vector with 1 and 2

@filipsajdak
Copy link
Contributor

Or in function calls:

f(1, 2); // call callable f with 1 and 2
:(a, b) = {
  std::cout << a << b << std::end;
}(1, 2); // call the unnamed function with 1 and 2

@ntrel
Copy link
Contributor Author

ntrel commented Apr 15, 2023

I see that you treat (1,2) as a tuple (like Python does). Cpp2 does not have such syntax for the tuple.

Yes I understand it's a std::initializer_list (which can be passed to a function as one argument too). I thought it would be nice to make it a std::tuple implicitly, though that may be outside the remit of Cpp2. I'm happy to close this issue once the diagnostics have been improved (I've implemented 2/3).

f(1, 2); // call callable f with 1 and 2

Yes it's nice Cpp2 seems to unify them syntactically.

@filipsajdak
Copy link
Contributor

Actually, it is not std::initializer_list it is just replaced with { initializer } which might construct std::initializer_list or any other object.

@hsutter
Copy link
Owner

hsutter commented Apr 20, 2023

d: () -> (int, int) = (1,2);
e: () -> (int, int) = { return (1,2); }

  1. d can't be initialized by an expression:

The error should be that an identifier was expected, not int. I have a fix for that. If this was to be supported the syntax would probably be d: () -> (:int, :int) = .

And named returns, which you initialize by name rather than put in an anonymous return expression. So this would be:

d: () -> (x:int, y:int) = { x = 1; y = 2; }

main: () = {
    std::cout << "(d().x)$ and (d().y)$\n";    // prints: 1 and 2
}

@hsutter
Copy link
Owner

hsutter commented Apr 20, 2023

I think this thread has answered some of the misunderstanding of Cpp2 syntax and what is supported, but I did think that this case should work:

// Cpp2 functions
f: () -> std::pair<int, std::string> = (5, "hi");
g: () -> std::pair<int, std::string> = { return (5, "hi"); }

// Cpp1 code using structured bindings
auto main() -> int {
    auto [a,b] = f();
    auto [c,d] = g();
}
~~~

I'm about to push a commit that makes those work. Thanks!

hsutter pushed a commit that referenced this issue Apr 23, 2023
* Check return parameter has identifier and type

Note: type cannot be `_`.
Fixes most of #325.

* Tweak error messages; add tests

* Move checks to parameter_declaration

* Address review

* Fix wrong check for missing type
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
* Check return parameter has identifier and type

Note: type cannot be `_`.
Fixes most of hsutter#325.

* Tweak error messages; add tests

* Move checks to parameter_declaration

* Address review

* Fix wrong check for missing type
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