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

C# v7.x: Local functions #104

Merged
merged 27 commits into from
Oct 11, 2022
Merged

C# v7.x: Local functions #104

merged 27 commits into from
Oct 11, 2022

Conversation

RexJaeschke
Copy link
Contributor

@RexJaeschke RexJaeschke commented Dec 27, 2020

No description provided.

@RexJaeschke RexJaeschke marked this pull request as draft December 27, 2020 19:28
@RexJaeschke RexJaeschke removed the request for review from BillWagner December 27, 2020 19:28
@RexJaeschke RexJaeschke added this to the C# 7.x milestone Jan 26, 2021
@RexJaeschke RexJaeschke marked this pull request as ready for review April 19, 2021 15:26
@RexJaeschke RexJaeschke added the Review: pending Proposal is available for review label Sep 16, 2021
@RexJaeschke RexJaeschke marked this pull request as draft December 4, 2021 18:09
@BillWagner BillWagner force-pushed the Rex-v7-local-functions branch 2 times, most recently from 3baa9e8 to 21e116a Compare April 2, 2022 20:58
@BillWagner BillWagner self-assigned this Apr 18, 2022
Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

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

This looks great @RexJaeschke Should we mark this ready for review?

I've got one note for how local functions interacts with generalized async return types.

I've also got one second question that won't need to be addressed until C# 8.

standard/statements.md Outdated Show resolved Hide resolved
standard/statements.md Outdated Show resolved Hide resolved
Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

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

Add notes from committee meeting.

standard/statements.md Outdated Show resolved Hide resolved
standard/statements.md Outdated Show resolved Hide resolved
BillWagner added a commit that referenced this pull request May 12, 2022
Addresses comments in committee meeting:

- #104 (comment)
- #104 (comment)
@BillWagner
Copy link
Member

BillWagner commented May 12, 2022

This is now ready for final review.

@jskeet Should I tag others for review?

@BillWagner BillWagner marked this pull request as ready for review May 12, 2022 15:08
@gafter
Copy link
Member

gafter commented May 13, 2022

A lot of the work on this feature was done by @agocke. It would be good to get his feedback.

@agocke
Copy link
Member

agocke commented May 13, 2022

I'll try to take a look. It looks like the definite assignment section needs work. Unlike lambdas, local functions use a more precise definite assignment when not converted to delegates. I'll try to write up some appropriate spec language for this. Overview of the differences is at https://commentout.com/localfunc.html

@RexJaeschke
Copy link
Contributor Author

@BillWagner I think the header and modifiers rules needs tweaking:

We have

local_function_header
    : local_function_modifiers? return_type identifier type_parameter_list?
        ( formal_parameter_list? ) type_parameter_constraints_clause*
    ;
local_function_modifiers
    : 'async'
    | 'unsafe'
    ;

That allows a local function to have no modifiers, async, or unsafe, but it does not allow async unsafe, which I think is a valid combination. [In V8, we'll add static, and in V9, extern.]

Following the corresponding pattern for modifiers on methods (14.16.1), the rule should be renamed local_function_modifier (singular), with 0 or more occurrences allowed, with both rules changed, as follows:

local_function_header
    : local_function_modifier* return_type identifier type_parameter_list?
        ( formal_parameter_list? ) type_parameter_constraints_clause*
    ;
local_function_modifier
    : 'async'
    | 'unsafe'
    ;

BillWagner and others added 17 commits October 2, 2022 17:07
Co-authored-by: KalleOlaviNiemitalo <kon@iki.fi>
Addresses comments in committee meeting:

- #104 (comment)
- #104 (comment)
In this commit, create the new clause for the definite assignment rules for local functions. Move the existing example to that clause. Add text from Andy's comments and notes.
Incorporate the rules for definite assignment for captured variables in local functions. This is both for local function calls and for delegate conversion.
Co-authored-by: Nigel-Ecma <perryresearch@zoot.net.nz>
Co-authored-by: Jon Skeet <jonskeet@google.com>
Co-authored-by: Neal Gafter <neal@gafter.com>
@BillWagner BillWagner force-pushed the Rex-v7-local-functions branch from 28f95fe to 3a4fc3d Compare October 2, 2022 21:07
We resolved these two discussions during the committee meeting. Edits reflect those decisions.
@BillWagner
Copy link
Member

@jskeet Check the final commit, and when you're ready, let's :shipit:

Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Just a few nits left; happy for it to be merged after that.

standard/statements.md Outdated Show resolved Hide resolved
standard/statements.md Outdated Show resolved Hide resolved
standard/statements.md Outdated Show resolved Hide resolved
@BillWagner BillWagner merged commit 912b5b2 into draft-v7 Oct 11, 2022
@BillWagner BillWagner deleted the Rex-v7-local-functions branch October 11, 2022 11:51
BillWagner added a commit to RexJaeschke/csharpstandard that referenced this pull request Feb 17, 2023
BillWagner added a commit to RexJaeschke/csharpstandard that referenced this pull request Apr 14, 2023
brendasmith8 added a commit to brendasmith8/csharp-standard that referenced this pull request Apr 24, 2023
* Update statements.md

* Update unsafe-code.md

* Add semantics and example for local functions v7.0 feature

* removed local function unsafe grammar extension

With the consolidation of the unsafe grammar in PR #233, this text is no longer needed.

* include support for local functions

* add support for local functions

* fix build issues

* Apply suggestions from code review

Co-authored-by: KalleOlaviNiemitalo <kon@iki.fi>

* clarify definite assignment, add examples

Addresses comments in committee meeting:

- dotnet/csharpstandard#104 (comment)
- dotnet/csharpstandard#104 (comment)

* respond to feedback.

* grammar updates

* Incorporate Andy's text

In this commit, create the new clause for the definite assignment rules for local functions. Move the existing example to that clause. Add text from Andy's comments and notes.

* finish local functions and definite assignment.

Incorporate the rules for definite assignment for captured variables in local functions. This is both for local function calls and for delegate conversion.

* typo

* Apply suggestions from code review

Co-authored-by: Nigel-Ecma <perryresearch@zoot.net.nz>

* Apply suggestions from code review

* Update standard/statements.md

Co-authored-by: Jon Skeet <jonskeet@google.com>

* Update standard/statements.md

* Update standard/statements.md

Co-authored-by: Neal Gafter <neal@gafter.com>

* Update standard/statements.md

* Update standard/statements.md

* edits based on September committee meeting

* fix link text

* fix section renumbering issue

* Final edits

We resolved these two discussions during the committee meeting. Edits reflect those decisions.

* updates from code review.

* one more bit of feedback.

Co-authored-by: Bill Wagner <wiwagn@microsoft.com>
Co-authored-by: KalleOlaviNiemitalo <kon@iki.fi>
Co-authored-by: Nigel-Ecma <perryresearch@zoot.net.nz>
Co-authored-by: Jon Skeet <jonskeet@google.com>
Co-authored-by: Neal Gafter <neal@gafter.com>
jskeet pushed a commit that referenced this pull request May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meeting: discuss This issue should be discussed at the next TC49-TG2 meeting Review: pending Proposal is available for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants