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

Function return type inference #826

Merged
merged 20 commits into from
Sep 23, 2021
Merged

Function return type inference #826

merged 20 commits into from
Sep 23, 2021

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Sep 13, 2021

Allow auto as a return type for functions. Only support functions with one return statement for now (open question).

This explicitly suggests removing the executable semantics fn name(args) => expression syntax, and is motivated by reconciling executable semantics with approved Carbon state. example This aspect is a decision that may be affected by lambda syntax, but we might also choose to keep lambda syntax and function syntax separate -- I don't think there's enough benefit to providing the alternate function syntax right now.

@jonmeow jonmeow added the proposal A proposal label Sep 13, 2021
@jonmeow jonmeow requested a review from a team September 13, 2021 22:28
@google-cla google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Sep 13, 2021
@jonmeow jonmeow changed the title fn returning auto Function return type inference Sep 14, 2021
@jonmeow jonmeow marked this pull request as ready for review September 14, 2021 17:51
@github-actions github-actions bot added the proposal rfc Proposal with request-for-comment sent out label Sep 14, 2021
proposals/p0826.md Outdated Show resolved Hide resolved
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally makes sense. Would like to specifically know if @zygoloid has anything we should ensure to avoid here and whether this seems reasonable to go in at this stage.

Comment on lines 81 to 86
For example, this is valid because `CallAdd` can see the `Add` definition:

```
fn Add(x: i32, y: i32) -> auto;

fn Add(x: i32, y: i32) -> auto { return x + y; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should even allow this. While the caller is fine, we can't type check the first declaration. I'd suggest requiring a definition to accompany any declaration with auto.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think disallowing this makes sense. A forward declaration involving auto is usually not useful -- you can't actually call it, so it doesn't break any cycles, and you can't use it to move the implementation out of an api file. Perhaps it's useful to allow you to declare a member function inside a class and then define it outside, though I'm not sure how valuable that is. If we're deducing a return type, I'd expect the function body to be small.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated proposal and added alternative corresponding.

Comment on lines 81 to 86
For example, this is valid because `CallAdd` can see the `Add` definition:

```
fn Add(x: i32, y: i32) -> auto;

fn Add(x: i32, y: i32) -> auto { return x + y; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think disallowing this makes sense. A forward declaration involving auto is usually not useful -- you can't actually call it, so it doesn't break any cycles, and you can't use it to move the implementation out of an api file. Perhaps it's useful to allow you to declare a member function inside a class and then define it outside, though I'm not sure how valuable that is. If we're deducing a return type, I'd expect the function body to be small.


## Proposal

Support automatic type inference in Carbon with `auto`, as in:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I think this is our first allowance for the use of auto anywhere. I don't think that's a problem in and of itself, but it may be worth saying that auto will be a keyword.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added notes to the background, and a very brief note here. (TBH, I had thought #339 included it, but obviously not -- maybe it's worth a proposal for a starting point)

Copy link
Contributor Author

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressing comments


## Proposal

Support automatic type inference in Carbon with `auto`, as in:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added notes to the background, and a very brief note here. (TBH, I had thought #339 included it, but obviously not -- maybe it's worth a proposal for a starting point)

Comment on lines 81 to 86
For example, this is valid because `CallAdd` can see the `Add` definition:

```
fn Add(x: i32, y: i32) -> auto;

fn Add(x: i32, y: i32) -> auto { return x + y; }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated proposal and added alternative corresponding.

@zygoloid
Copy link
Contributor

zygoloid commented Sep 21, 2021

Generally makes sense. Would like to specifically know if @zygoloid has anything we should ensure to avoid here and whether this seems reasonable to go in at this stage.

I think this proposal has bounded the scope enough to avoid most problems here: we require exactly one return statement, so there's no need to consider how to merge multiple return types, and we're not allowing auto as a component of a more complex type so there's no need to specify how that works.

I think there are two more issues from C++ that we should consider, though I think at least one of them is out of scope for now:

  • What happens if an attempt is made to call the function
    i. before itsreturn statement,
    ii. within its return statement, or
    iii. after its return statement but before the end of its definition?
    I think (ii) is easy: we basically must reject, because the return type is defined in terms of itself. I think we should reject in cases (i) and (iii) too, even though we could in principle support (iii) and the subset of (i) where the expression in the return statement doesn't depend on the return type. This would diverge from C++, which permits recursive calls after the (first) return statement (ie, C++ permits (iii) but not (i) nor (ii)).
  • What happens if the return value is const qualified (or more generally is of some facet type)? In C++, we initially said that return type deduction for lambdas just copied the type of the return expression, as we do here, and this preserved const qualifiers on the return type. Then with generalized return type deduction we switched to using the auto rules, which are defined in terms of template argument deduction and discard const qualifiers, resulting in some subtle behavior changes. I think this is probably out of scope for now, given that we haven't decided on exactly how to model const yet and in particular I don't think we have reason to believe that making a copy would drop constness in Carbon like it does in C++, but this seems like something we should keep in mind.

In terms of what to do with the above in this document, I think adding something saying we don't permit recursive calls in functions with a deduced return type at least for now (and that this intentionally diverges from C++) would be sufficient.

@chandlerc
Copy link
Contributor

Generally makes sense. Would like to specifically know if @zygoloid has anything we should ensure to avoid here and whether this seems reasonable to go in at this stage.

I think this proposal has bounded the scope enough to avoid most problems here: we require exactly one return statement, so there's no need to consider how to merge multiple return types, and we're not allowing auto as a component of a more complex type so there's no need to specify how that works.

I think there are two more issues from C++ that we should consider, though I think at least one of them is out of scope for now:

  • What happens if an attempt is made to call the function
    i. before itsreturn statement,
    ii. within its return statement, or
    iii. after its return statement but before the end of its definition?
    I think (ii) is easy: we basically must reject, because the return type is defined in terms of itself. I think we should reject in cases (i) and (iii) too, even though we could in principle support (iii) and the subset of (i) where the expression in the return statement doesn't depend on the return type. This would diverge from C++, which permits recursive calls after the (first) return statement (ie, C++ permits (iii) but not (i) nor (ii)).

Strongly agree with just rejecting all recursion here. Seems much simpler and more obvious.

  • What happens if the return value is const qualified (or more generally is of some facet type)? In C++, we initially said that return type deduction for lambdas just copied the type of the return expression, as we do here, and this preserved const qualifiers on the return type. Then with generalized return type deduction we switched to using the auto rules, which are defined in terms of template argument deduction and discard const qualifiers, resulting in some subtle behavior changes. I think this is probably out of scope for now, given that we haven't decided on exactly how to model const yet and in particular I don't think we have reason to believe that making a copy would drop constness in Carbon like it does in C++, but this seems like something we should keep in mind.

Yeah, I agree this is a bit out of scope for now. I'd revisit this (and any other auto we add) when we are getting much more precise with rules around deduction for templates.

@jonmeow
Copy link
Contributor Author

jonmeow commented Sep 22, 2021

Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor wording suggestions below to just make things read a bit easier for me. Generally, LGTM. Feel free to merge with these or similar. Also happy to look if the wording suggestions aren't making sense or are changing anything.

proposals/p0826.md Outdated Show resolved Hide resolved
proposals/p0826.md Outdated Show resolved Hide resolved
Comment on lines 287 to 289
However, this would be invalid because `CallAdd` can only see the `Add`
declaration (even though the definition may be in the same file), and it would
need the definition to determine the return type:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just trying to make "this" a bit less ambiguous:

Suggested change
However, this would be invalid because `CallAdd` can only see the `Add`
declaration (even though the definition may be in the same file), and it would
need the definition to determine the return type:
However, the example below would be invalid because `CallAdd` can only see the `Add`
declaration (even though the definition may be in the same file), and it would
need the definition to determine the return type:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI re: below, more specific words are preferred: https://developers.google.com/style/word-list#letter-b

I'll probably do something like "following example"

@github-actions github-actions bot added proposal accepted Decision made, proposal accepted and removed proposal rfc Proposal with request-for-comment sent out labels Sep 23, 2021
@jonmeow jonmeow merged commit 0eb78c7 into carbon-language:trunk Sep 23, 2021
@jonmeow jonmeow deleted the proposal-fn-returning-auto branch September 23, 2021 17:10
jonmeow added a commit that referenced this pull request Sep 27, 2021
This implements #826, I think covering everything important there.

Regarding ReturnTypeContext, I broke that out because it started feeling like a significant number of args to be passing around, and I think this makes the association inside type checking clearer.

Co-authored-by: Geoff Romer <[email protected]>
jonmeow added a commit that referenced this pull request Jan 28, 2022
I was doing this for #851 initially, but I think #438 and #826 hadn't made it in (possibly intentionally due to #851? I don't recall). 

Co-authored-by: Chandler Carruth <[email protected]>
Co-authored-by: Richard Smith <[email protected]>
chandlerc added a commit that referenced this pull request Jun 28, 2022
Allow `auto` as a return type for functions. Only support functions with one `return` statement for now (open question).

This explicitly suggests removing the executable semantics `fn name(args) => expression` syntax, and is motivated by reconciling executable semantics with approved Carbon state. [example](https://github.com/carbon-language/carbon-lang/blob/3d1716f6c692a840d8b4b513ddfb8119432a5150/executable_semantics/testdata/fun_named_params.carbon) This aspect is a decision that may be affected by lambda syntax, but we might also choose to keep lambda syntax and function syntax separate -- I don't think there's enough benefit to providing the alternate function syntax right now.

Co-authored-by: Geoff Romer <[email protected]>
Co-authored-by: Chandler Carruth <[email protected]>
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
This implements #826, I think covering everything important there.

Regarding ReturnTypeContext, I broke that out because it started feeling like a significant number of args to be passing around, and I think this makes the association inside type checking clearer.

Co-authored-by: Geoff Romer <[email protected]>
chandlerc added a commit that referenced this pull request Jun 28, 2022
I was doing this for #851 initially, but I think #438 and #826 hadn't made it in (possibly intentionally due to #851? I don't recall). 

Co-authored-by: Chandler Carruth <[email protected]>
Co-authored-by: Richard Smith <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes PR meets CLA requirements according to bot. proposal accepted Decision made, proposal accepted proposal A proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants