Skip to content

[BUG] short function body syntax generates bad code for named functions #257

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
filipsajdak opened this issue Feb 10, 2023 · 9 comments
Closed
Assignees
Labels
question - further information requested Further information is requested

Comments

@filipsajdak
Copy link
Contributor

The current implementation of the cppfront compiles the following code:

f    :(x) = x + 2;
l := :(x) = x + 2;

to

auto f(auto const& x) -> void { x + 2; } // notice missing return
auto l {[](auto const& x) { return x + 2; }}; 

Expectation

I expect that the named and unnamed function defined with short function body syntax generates the same code that uses implicit return.

@filipsajdak filipsajdak added the bug Something isn't working label Feb 10, 2023
@hsutter
Copy link
Owner

hsutter commented Mar 1, 2023

Thanks! You may have come across an example where my current choice of different defaults for named and unnamed functions may be a problem.

Here's why this is currently by-design:

  • Named functions default to -> void, not a deduced type, because named functions want to be order-independent, deduced return types inhibit full order-independence, and so we don't want that to be the default for most functions.
  • Unnamed functions default to -> _, because it seemed to me that unnamed functions were frequently written in places where by default you want a deduced return type.

But now when you additionally throw a single-expression body into the mix, you get the effect you see above.

What do you think... is this too surprising, so that (probably the better resolution) unnamed functions would also default to no return, and you'd have to write -> _ a little more often on unnamed functions?

@hsutter hsutter self-assigned this Mar 1, 2023
@hsutter hsutter added question - further information requested Further information is requested and removed bug Something isn't working labels Mar 1, 2023
@gregmarr
Copy link
Contributor

gregmarr commented Mar 1, 2023

Brainstorming:

  • Functions that are single-expression bodies are defaulted to -> _.
  • Functions with a return <something>; are defaulted to -> _.
  • Functions otherwise default to -> void.

Okay, it just hit me (after writing the below) that what I wrote here is exactly how -> decltype(auto) works. If the function doesn't have a return <something>, it's void. If it does, then it's the type of the expression. So maybe -> _ is a perfectly good default for everything.

deduced return types inhibit full order-independence

Is this only when the type of the return is based on the type of one or more arguments? Is this a fundamental limitation, or just a limitation of the conversion to C++1?

What do you think... is this too surprising, so that (probably the better resolution) unnamed functions would also default to no return, and you'd have to write -> _ a little more often on unnamed functions?

I think defaulting to -> void at all is surprising, as I think non-void functions are going to be far more likely than void functions.

I'm thinking this might fall into don't pay for what you don't use. If you don't need order independence, then you can use defaulted return types. If you want order independence for your functions, then you have to pay for it by having explicit return types for them. The question then becomes, can we easily determine at the point of a function call that if a function had an explicit return type, then we could call it, and we can tell the user this?

@filipsajdak
Copy link
Contributor Author

What do you think... is this too surprising, so that (probably the better resolution) unnamed functions would also default to no return, and you'd have to write -> _ a little more often on unnamed functions?

I am aware that deduced return types inhibit full order-independence.

What I started to like about cppfront is that it is consistent regarding the syntax. The more exceptions to the rules, the more we need to learn how it works. That is worth fighting for. Maybe this is one of the alpha limitations.

@hsutter
Copy link
Owner

hsutter commented Mar 8, 2023

@filipsajdak I hear you... I'll likely switch this to making -> void the consistent default everywhere, and try that path to see what the experience feels like.

@gregmarr I hear you too, arguing in favor of the consistent default being -> _, but I think the argument that currently sways me is that in Cpp2 I've tried to follow "explicit is better than implicit" in cases where "additional stuff is happening," and returning a value is something additional (it's not just potentially giving up order independence). But we'll see how the UX is, and if it's annoying we can revisit this.

Thanks for the input!

@gregmarr
Copy link
Contributor

gregmarr commented Mar 8, 2023

returning a value is something additional (it's not just potentially giving up order independence)

An interesting observation is that I probably would never have even suggested this if I hadn't started using more C# and Typescript recently and gotten used to () => x; means the same as () => { return x; }, so I didn't even really think of "returning a value" as "something additional" here. By now, that's just the way bare expressions as function declarations work.

Also, in the end, I wasn't as much arguing for -> _ as default, as that the default is equivalent to the current -> decltype(auto) and thus based on the function body.

@hsutter
Copy link
Owner

hsutter commented Mar 8, 2023

Yes, my experience with lambdas in other languages is what made me default to -> _ for lambdas (unnamed functions). Note that those other languages also have a difference between named functions and lambdas... in C#, named functions must always explicitly write their return type (including void), but lambdas have deduced return types... and the experience in those languages seems to be that that's just fine.

FWIW, here in cppfront, the one small usage-based data point I have so far about making -> void the consistent default is was that as I re-ran the regression tests:

  • There was only one place where I had to add -> _. It did take me a few seconds to realize what the problem was, because I was so used to expression-functions defaulting to that before this. I'll have to see whether I get used to it.
  • In several tests, the code gen changed to -> void, and it was actually appropriate because it was cases like anonymous functions that just did std::cout << something;. For these cases the change to -> void default for all functions seemed to be an improvement.

I'll keep an eye on it...

@gregmarr
Copy link
Contributor

gregmarr commented Mar 8, 2023

in C#, named functions must always explicitly write their return type (including void), but lambdas have deduced return types... and the experience in those languages seems to be that that's just fine.

I don't think I ever consciously noticed that, so that's a good thing. I do have under 2 years regular experience with C# now, so I'm still learning.

FWIW, here in cppfront, the one small usage-based data point I have so far about making -> void the consistent default is was that as I re-ran the regression tests:

I would be interested to see what those same changes would be with -> decltype(auto).

There was only one place where I had to add -> _. It did take me a few seconds to realize what the problem was, because I was so used to expression-functions defaulting to that before this. I'll have to see whether I get used to it.

Was this found because you had an expression function that returned something, and then assigned that return value to a variable? I assume then that any other expression functions that you had that returned something had an explicit return type, including -> _.

it was actually appropriate because it was cases like anonymous functions that just did std::cout << something;. For these cases the change to -> void default for all functions seemed to be an improvement.

I could see this being something that -> decltype(auto) can't handle, if you used an expression function, as it's perfectly reasonable to return std::cout << something; and if we do default to no discard as someone had mentioned somewhere recently, then you'd have to add -> void to prevent the error. That's definitely a point in favor of -> void.

@hsutter
Copy link
Owner

hsutter commented Mar 8, 2023

It was this:

less_than: (value) -> _ =
    :(x:_) = x < value$;  // now needs to add "-> _"

@gregmarr
Copy link
Contributor

gregmarr commented Mar 8, 2023

Oh, interesting, mismatched function pointer types, that's not what I would have expected.

@hsutter hsutter closed this as completed in fb4a9fc Mar 9, 2023
zaucy pushed a commit to zaucy/cppfront that referenced this issue Dec 5, 2023
This is an omnibus commit of the last few evenings' changes. Primarily it was to start laying the groundwork for constructors, but it includes other fixes and closes a few issues.

Details:

- Begin infrastructure preparation for constructors

- Started creating navigation APIs to replace the low-level graph node chasing; this makes cppfront's own code cleaner and the tree easier to change if needed, but it's also a step toward a reflection API

- Extended `main:(args)` to require the name "args" for the single-parameter `main`, and to support `args.argc` and `args.argv`, further on hsutter#262 (see comment thread)

- Changed default return type for unnamed functions to be `-> void`, same as named functions, closes hsutter#257

- Disallow single-expression function body to be just `return`, closes hsutter#267

- Make `make_args` inline, closes hsutter#268

- Bug fix: emit prolog also for single-expression function body. Specifically, this enables main:(args)=expression; to work correctly. Generally, this also corrects the code gen for examples that were trying (but failing) to inject prologs in single-expression functions... in the regression tests this corrected the code gen for `pure2-forward-return.cpp2` which was missing the contract check before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question - further information requested Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants