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

Add support for static local functions #869

Merged
merged 20 commits into from
Mar 27, 2024

Conversation

RexJaeschke
Copy link
Contributor

@RexJaeschke RexJaeschke commented Jul 22, 2023

Note 1:

In researching this V8 feature, I discovered that while V7 stated, “It is a compile-time error for a [non-static] local function to declare a parameter or local variable with the same name as one declared in the enclosing scope.”, based on the implementation’s behavior when compiling with LangVersion 8, that restriction was removed in V8 for non-static local functions, and also does not apply to static local functions either.

I note this here to make sure this was the intent, as it was not so stated in the MS proposal for this feature.

Note 2:

The spec for 13.6.4 contains a pointer to 9.4.4.33, “Rules for variables in local functions,” part of which talks about capturing variables, which, clearly, static local functions can’t do. As such, a knowledgeable reader of that referred-to reference might be expected to understand that it can’t apply to static local functions. As such, I did not say that explicitly in that section.

Reviewers might want to add an explicit note about that.


Associated WorkItem - 209387

@RexJaeschke RexJaeschke added the type: feature This issue describes a new feature label Jul 22, 2023
@RexJaeschke RexJaeschke added this to the C# 8.0 milestone Jul 22, 2023
@RexJaeschke RexJaeschke marked this pull request as draft July 22, 2023 22:29
@BillWagner
Copy link
Member

rebased on updated draft-v8 branch on 09/25/2023.

standard/statements.md Outdated Show resolved Hide resolved
@ericlippert ericlippert self-assigned this Oct 4, 2023
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.

I made a suggested change for the "enclosing scope" sections.

In the suggestion, I removed the sentence saying that async doesn't apply if the enclosing method is async. I don't think it's needed with the new wording.

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

@RexJaeschke Should we mark this one ready for review?

@RexJaeschke RexJaeschke added the Review: pending Proposal is available for review label Oct 13, 2023
@RexJaeschke RexJaeschke marked this pull request as ready for review January 11, 2024 14:13
@jskeet jskeet added the meeting: discuss This issue should be discussed at the next TC49-TG2 meeting label Feb 7, 2024
@BillWagner BillWagner self-assigned this Feb 8, 2024
@sequestor sequestor bot added seQUESTered and removed reQUEST labels Feb 9, 2024
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.

I do wonder whether we need a note about the purpose of making a local function static. At the moment we say that removing the modifier from valid code doesn't change the meaning - but we don't say why you'd want to keep it anyway.

standard/statements.md Outdated Show resolved Hide resolved
standard/statements.md Outdated Show resolved Hide resolved
standard/statements.md Outdated Show resolved Hide resolved
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.

I have a few comments to address, along with Jon's nits.

After that, this one is ready to merge. I don't think any rules on static local functions are missing.

standard/statements.md Outdated Show resolved Hide resolved
standard/statements.md Outdated Show resolved Hide resolved
standard/statements.md Outdated Show resolved Hide resolved
standard/statements.md Outdated Show resolved Hide resolved
@Nigel-Ecma
Copy link
Contributor

I'm not sure it's clear that a static local function can call another static local function (but not a non-static local function), and that includes a static local function defined in an enclosing local function.

@jskeet wrote:

I do wonder whether we need a note about the purpose of making a local function static. At the moment we say that removing the modifier from valid code doesn't change the meaning - but we don't say why you'd want to keep it anyway.

A note might be worth it as using the keyword static to mean "limited scope non-capturing pure function" may not leap off the page ;-)

jskeet and others added 2 commits March 27, 2024 20:22
Co-authored-by: Bill Wagner <wiwagn@microsoft.com>
Co-authored-by: Bill Wagner <wiwagn@microsoft.com>
standard/statements.md Outdated Show resolved Hide resolved
standard/statements.md Outdated Show resolved Hide resolved
standard/statements.md Outdated Show resolved Hide resolved
standard/statements.md Outdated Show resolved Hide resolved
standard/statements.md Outdated Show resolved Hide resolved
standard/statements.md Outdated Show resolved Hide resolved
standard/statements.md Outdated Show resolved Hide resolved
standard/statements.md Outdated Show resolved Hide resolved
Co-authored-by: Neal Gafter <neal@gafter.com>
standard/statements.md Outdated Show resolved Hide resolved
standard/statements.md Outdated Show resolved Hide resolved
standard/statements.md Outdated Show resolved Hide resolved
@jskeet jskeet merged commit 0467ef9 into dotnet:standard-v7 Mar 27, 2024
6 of 7 checks passed
@BillWagner BillWagner deleted the Rex-add-static-local-functions branch March 27, 2024 21:30
BillWagner pushed a commit to BillWagner/csharpstandard that referenced this pull request Mar 28, 2024
BillWagner added a commit to BillWagner/csharpstandard that referenced this pull request Mar 28, 2024
BillWagner added a commit that referenced this pull request Mar 28, 2024
Co-authored-by: Rex Jaeschke <rex@RexJaeschke.com>
BillWagner added a commit that referenced this pull request Mar 28, 2024
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 type: feature This issue describes a new feature
Projects
No open projects
Status: ✅ Done
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

8 participants