Static local functions #32067

cston · 2018-12-31T16:32:55Z

Allow static modifier for local functions in C#8; report errors for static local functions that reference this or a variable in an outer scope; and allow shadowing of local and parameter names inside local functions.

[jcouv update:]
Championed issue: dotnet/csharplang#1565
Last LDM notes: https://github.com/dotnet/csharplang/blob/master/meetings/2018/LDM-2018-09-10.md
Test plan: #32069

CyrusNajmabadi · 2018-12-31T17:25:30Z

@jcouv Can you validate the IDE side? I think the only thing that would matter would be that 'this' doesn't appear in a 'static' function.
Also, it might be nice to have an analyzer detect and suggest changing to a static function if that is possible.
It might be nice to have a refactoring that converts a capturing-local-function to a non-capturing-static local function. It would have to update callsites to pass in the values as parameters.

jcouv · 2018-12-31T17:29:59Z

src/Compilers/CSharp/Test/Semantic/Semantics/LocalFunctionTests.cs

+}";
+            var comp = CreateCompilation(source);
+            comp.VerifyEmitDiagnostics();
+        }


Since you're merging straight to master, some test ideas:

test IOperation

test the semantic model (there is likely a bug there, as regular local functions used to report IsStatic=true, issue LocalFunctionSymbol.IsStatic always returns true #27719)

add a test using a local function with a delegate type (delegate probably still won't get cached at this point though)

can you verify that the IDE recommends "static"? File an issue if it doesn't.

In my mind, fixing any of those is not blocking this PR though.

Consider testing static static local() ... #Closed

Another test idea: a static local function should not allow a this parameter (extension method). #Resolved

Logged #32174 for static in IDE.

jcouv · 2018-12-31T17:37:42Z

src/Compilers/CSharp/Portable/Binder/Binder_NameConflicts.cs

@@ -51,7 +52,7 @@ private static Location GetLocation(Symbol symbol)
                    {
                        // Type parameter declaration name conflicts are detected elsewhere
                    }
-                    else
+                    else if (!isLocalFunction)


I found this surprising. Should we just change the behavior for static local functions? #Closed

I believe the expected behavior is to allow shadowing for all local functions.

In reply to: 244605104 [](ancestors = 244605104)

Part of the motivation for the shadowing decisions was that I should be able to take a normal function and copy + paste it as a local function and it should just compile. No hassle. Given that I would expect all forms of shadowing to work.

This is my memory of the meeting at least. #Closed

~~notes were mentioning lambdas too, would it make more sense to do this for both at the same time?~~ nevermind, it's been separated out. (#32069 (comment)) #Closed

jcouv · 2018-12-31T17:45:26Z

src/Compilers/CSharp/Portable/Binder/Binder_NameConflicts.cs

@@ -111,13 +112,28 @@ protected bool ValidateNameConflictsInScope(Symbol symbol, Location location, st
                    break;
                }

+                // symbols inside local function shadow names in scopes outside local function


I thought this new behavior would be only for static local functions too.

Do you have some tests with nesting: a local function inside a static local function that shadows type parameters or parameters?

void M(int x) { static void local() { void local2(int x) { } // allowed or not? } } ``` #Closed

Added tests.

In reply to: 244605531 [](ancestors = 244605531)

jcouv · 2018-12-31T17:50:22Z

src/Compilers/CSharp/Portable/Binder/Binder_NameConflicts.cs

+                            Debug.Assert(!error); // If the assert fails, add a corresponding test.
+                            goto End;
+                    }
+                }


Do we have a test where the name of the local function is in conflict with a variable in scope?

public void M(int local, int local2) { // both should be errors void local() { } static void local2() { } } ``` #Closed

jaredpar · 2018-12-31T17:53:35Z

Test Plan Link: #32069

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

jaredpar · 2018-12-31T18:30:01Z

src/Compilers/CSharp/Test/Semantic/Semantics/LocalFunctionTests.cs

+            comp.VerifyDiagnostics(
+                // (11,26): error CS0136: A local or parameter named 'x' cannot be declared in this scope because that name is used in an enclosing local scope to define a local or parameter
+                //         void F3() { void x() { } } // method
+                Diagnostic(ErrorCode.ERR_LocalIllegallyOverrides, "x").WithArguments("x").WithLocation(11, 26),


This should be allowed correct? #Resolved

jaredpar · 2018-12-31T18:30:15Z

src/Compilers/CSharp/Test/Semantic/Semantics/LocalFunctionTests.cs

+                Diagnostic(ErrorCode.ERR_LocalIllegallyOverrides, "x").WithArguments("x").WithLocation(11, 26),
+                // (13,30): error CS1931: The range variable 'x' conflicts with a previous declaration of 'x'
+                //         void F5() { _ = from x in new[] { 1, 2, 3 } select x; } // range variable
+                Diagnostic(ErrorCode.ERR_QueryRangeVariableOverrides, "x").WithArguments("x").WithLocation(13, 30));


This should be allowed correct? #Resolved

jcouv

Done with review pass (iteration 1).
We need to clarify the expected behavior (are we changing shadowing rules for regular local functions). Also a few test ideas.

jcouv · 2019-01-02T18:53:23Z

src/Compilers/CSharp/Test/Syntax/Parsing/LocalFunctionParsingTests.cs

+
+            UsingDeclaration(text, options: TestOptions.Regular7_3,
+                // (5,9): error CS8421: To use the 'static' modifier for local functions, please use language version 8.0 or greater.
+                //         static static void F1() { }


static static [](start = 27, length = 13)

nit: the error recovery seems worse than on regular methods. I'd expect a single diagnostic: "error CS1004: Duplicate 'static' modifier".
Consider filing an issue.

Example #Closed

Logged #32106

In reply to: 244819913 [](ancestors = 244819913)

jcouv · 2019-01-02T18:57:50Z

        compilation.VerifyDiagnostics(

~~Seems like a compat break (upgrading to new language version introduces new diagnostics)~~
Sorry, my bad. The diagnostics below are split into two lists and 8.0 is a subset of 7.3 diagnostics, which is correct. #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/OutVarTests.cs:4417 in 3118ed4. [](commit_id = 3118ed4ffe76cc6f9580d6c367d36da939cbc2fa, deletion_comment = False)

jcouv · 2019-01-02T21:30:21Z

src/Compilers/CSharp/Portable/Binder/InMethodBinder.cs

                    return true;
                }

+                if (newSymbolKind == SymbolKind.TypeParameter)


if [](start = 16, length = 2)

nit: Consider using a switch on newSymbolKind for the whole block. #Closed

AlekseyTs · 2019-01-03T00:31:39Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

@@ -8568,6 +8568,7 @@ private static bool IsAdditionalLocalFunctionModifier(SyntaxKind kind)
                case SyntaxKind.InternalKeyword:
                case SyntaxKind.ProtectedKeyword:
                case SyntaxKind.PrivateKeyword:
+                case SyntaxKind.StaticKeyword:


case SyntaxKind.StaticKeyword: [](start = 16, length = 30)

Should this be in the bucket of "valid" modifiers at the beginning of this switch? #Closed

AlekseyTs · 2019-01-03T00:41:20Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

                        }
-                        badBuilder[i] = this.AddError(modifiers[i], ErrorCode.ERR_BadMemberFlag, ((SyntaxToken)modifiers[i]).Text);
+                        modifier = this.AddError(modifier, ErrorCode.ERR_StaticLocalFunctionModifier, new CSharpRequiredLanguageVersion(MessageID.IDS_FeatureStaticLocalFunctions.RequiredVersion()));


modifier = this.AddError(modifier, ErrorCode.ERR_StaticLocalFunctionModifier, new CSharpRequiredLanguageVersion(MessageID.IDS_FeatureStaticLocalFunctions.RequiredVersion())); [](start = 24, length = 174)

This doesn't feel right. It looks like this is going to have effect on syntax nodes that goes beyond just an additional diagnostics. For example, an async/unsafe modifiers are likely to be dropped. It is probably worth adjusting the logic so it would never drop original modifiers from the list, only add errors to them at most. #Closed

The loop should be dropping modifiers, only modifying modifiers that have errors.

In reply to: 244893785 [](ancestors = 244893785)

AlekseyTs · 2019-01-03T00:46:09Z

src/Compilers/CSharp/Portable/Symbols/Source/LocalFunctionSymbol.cs

@@ -286,6 +286,8 @@ public override bool IsExtensionMethod
            }
        }

+        internal bool IsStaticLocalFunction => _syntax.Modifiers.Any(SyntaxKind.StaticKeyword);


internal bool IsStaticLocalFunction => _syntax.Modifiers.Any(SyntaxKind.StaticKeyword); [](start = 8, length = 87)

Would IsStatic property work? #Closed

IsStatic currently returns true always. See #27719. We should replace this property when that issue is resolved.

In reply to: 244894319 [](ancestors = 244894319)

What would be the public API to answer the same question? It feels like we should have one at the time we ship this feature.

In reply to: 245043955 [](ancestors = 245043955,244894319)

The public API is ISymbol.IsStatic which is implemented by LocalFunctionSymbol.IsStatic. In short, the public API relies on fixing 27719.

In reply to: 245086567 [](ancestors = 245086567,245043955,244894319)

AlekseyTs · 2019-01-03T01:03:47Z

src/Compilers/CSharp/Portable/Binder/LocalScopeBinder.cs

@@ -469,25 +469,32 @@ private bool ReportConflictWithLocal(Symbol local, Symbol newSymbol, string name
                return true;
            }

-            if (newSymbolKind == SymbolKind.Local || newSymbolKind == SymbolKind.Parameter || newSymbolKind == SymbolKind.Method || newSymbolKind == SymbolKind.TypeParameter)


if (newSymbolKind == SymbolKind.Local || newSymbolKind == SymbolKind.Parameter || newSymbolKind == SymbolKind.Method || newSymbolKind == SymbolKind.TypeParameter) [](start = 12, length = 162)

Instead of making changes to this function would it be simpler to make changes to ValidateNameConflictsInScope method to get out of the loop once we are done with a body of a local function and the language version is right? #Closed

AlekseyTs · 2019-01-03T01:10:25Z

    internal static bool ReportConflictWithParameter(Symbol parameter, Symbol newSymbol, string name, Location newLocation, DiagnosticBag diagnostics)

Similar comment as for ReportConflictWithLocal. Otherwise we are likely missing changes in WithLambdaParametersBinder #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/InMethodBinder.cs:257 in da7cd13. [](commit_id = da7cd13b29df6cc832fa782bf2b3916de7e0e682, deletion_comment = False)

AlekseyTs · 2019-01-03T18:59:27Z

src/Compilers/CSharp/Portable/Binder/Binder_NameConflicts.cs

+            // avoid checking for conflicts outside of the local function.
+            var stopAtLocalFunction =
+                symbol?.DeclaringCompilation.IsFeatureEnabled(MessageID.IDS_FeatureStaticLocalFunctions) == true ?
+                symbol.ContainingSymbol as LocalFunctionSymbol :


symbol.ContainingSymbol as LocalFunctionSymbol [](start = 16, length = 46)

Is this condition really necessary? For example, what if the symbol is a local or parameter in a lambda within a local function? #Closed

Good catch, thanks.

In reply to: 245098932 [](ancestors = 245098932)

AlekseyTs · 2019-01-03T19:12:26Z

src/Compilers/CSharp/Portable/Binder/Binder_NameConflicts.cs

@@ -53,7 +48,7 @@ private static Location GetLocation(Symbol symbol)
                    }
                    else
                    {
-                        ValidateDeclarationNameConflictsInScope(tp, diagnostics);
+                        ValidateDeclarationNameConflictsInScope(tp, diagnostics, outsideContainingSymbol: true);


outsideContainingSymbol: true [](start = 81, length = 29)

It looks like there is only one call site for the ValidateParameterNameConflicts function, it feels like it would be simpler to pass a flag to suppress this call altogether rather than adding a parameter to ValidateDeclarationNameConflictsInScope and to ValidateNameConflictsInScope to deal with this special case there. #Closed

AlekseyTs · 2019-01-03T19:13:20Z

src/Compilers/CSharp/Portable/Binder/Binder_NameConflicts.cs

@@ -83,7 +78,7 @@ private static Location GetLocation(Symbol symbol)
                    }
                    else
                    {
-                        ValidateDeclarationNameConflictsInScope(p, diagnostics);
+                        ValidateDeclarationNameConflictsInScope(p, diagnostics, outsideContainingSymbol: true);


outsideContainingSymbol: true [](start = 80, length = 29)

Same comment as for the previous ValidateDeclarationNameConflictsInScope call #Closed

jcouv · 2019-01-03T19:30:36Z

I marked the PR as "personal" for now, since you're addressing blocking issues from design review. Please ping when ready for review. Thanks #Closed

AlekseyTs · 2019-01-03T20:11:05Z

src/Compilers/CSharp/Test/Semantic/Semantics/LocalFunctionTests.cs

+                //         void F5() { _ = from x in new[] { 1, 2, 3 } select x; } // range variable
+                Diagnostic(ErrorCode.ERR_QueryRangeVariableOverrides, "x").WithArguments("x").WithLocation(12, 30));
+
+            comp = CreateCompilation(source, parseOptions: TestOptions.Regular8);


parseOptions: TestOptions.Regular8 [](start = 45, length = 34)

We should also always test with the latest version. Consider relying on implicit value instead. #Closed

AlekseyTs · 2019-01-03T20:16:26Z

src/Compilers/CSharp/Test/Semantic/Semantics/LocalFunctionTests.cs

+        void F1()
+        {
+            object x = null;
+            Action a1 = () => { int x = 0; };


() => { int x = 0; }; [](start = 23, length = 22)

It looks like we are missing a test for a scenario when a lambda parameter or a local inside a lambda shadow something from outside of the local function. #Closed

AlekseyTs · 2019-01-03T20:18:18Z

src/Compilers/CSharp/Test/Semantic/Semantics/LocalFunctionTests.cs

+                Diagnostic(ErrorCode.ERR_StaticLocalFunctionCannotCaptureThis, "_f").WithLocation(18, 30));
+
+            comp = CreateCompilation(source, parseOptions: TestOptions.Regular8);
+            // PROTOTYPE: Should we report the same errors when "MyDefine" is not set?


// PROTOTYPE: [](start = 12, length = 13)

Does this PR target a feature branch? #Closed

jcouv · 2019-01-04T23:46:34Z

Yay! 🎉

cston requested a review from a team as a code owner December 31, 2018 16:32

jcouv added the Area-Compilers label Dec 31, 2018

jcouv reviewed Dec 31, 2018

View reviewed changes

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Outdated Show resolved Hide resolved

jaredpar reviewed Dec 31, 2018

View reviewed changes

jcouv reviewed Dec 31, 2018

View reviewed changes

jcouv self-assigned this Jan 2, 2019

jcouv reviewed Jan 2, 2019

View reviewed changes

AlekseyTs reviewed Jan 3, 2019

View reviewed changes

jcouv added this to the 16.0.P2 milestone Jan 3, 2019

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jan 3, 2019

AlekseyTs reviewed Jan 3, 2019

View reviewed changes

cston added 25 commits January 4, 2019 13:57

Static local functions

af69391

Misc.

7b34664

PR feedback

4ac2ae3

Misc.

c341f4d

Fix tests

7437b13

Report name conflict for local function type parameters

5a80b91

PR feedback

4edad9d

Misc.

d1a1e14

PR feedback

bd26f17

More tests

7ae5081

Fix tests

2780986

IOperation tests

58b8476

PR feedback

0f75bb6

Use latest version

4e6331b

Misc.

c823d3f

Misc.

b85cee4

Misc.

0fc4cb1

PR feedback

cd5d4ab

Misc.

6f436c4

More tests

532ea30

Remove custom feature error

4162940

Move 'cannot capture' errors to initial binding

9a80f24

Fix tests

618fbe6

PR feedback

2f7c97e

Misc.

ea78b74

cston force-pushed the local-fns branch from fdba7dc to ea78b74 Compare January 4, 2019 22:00

cston merged commit 5f3e87a into dotnet:master Jan 4, 2019

cston deleted the local-fns branch January 4, 2019 23:30

Youssef1313 mentioned this pull request Nov 2, 2019

Documentation incorrectly specifies that local functions cannot be static dotnet/docs#15654

Closed

Static local functions #32067

Static local functions #32067

Conversation

cston commented Dec 31, 2018 • edited by jcouv Loading

CyrusNajmabadi commented Dec 31, 2018

jcouv Dec 31, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Dec 31, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Dec 31, 2018 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Dec 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar Dec 31, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

alrz Jan 1, 2019 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Dec 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Dec 31, 2018 • edited Loading

Choose a reason for hiding this comment

jaredpar commented Dec 31, 2018

jaredpar Dec 31, 2018 • edited by cston Loading

Choose a reason for hiding this comment

jaredpar Dec 31, 2018 • edited by cston Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv Jan 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Jan 2, 2019 • edited Loading

jcouv Jan 2, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Jan 3, 2019 • edited Loading

AlekseyTs Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

jcouv commented Jan 3, 2019 • edited Loading

AlekseyTs Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

jcouv commented Jan 4, 2019

cston commented Dec 31, 2018 •

edited by jcouv

Loading

jcouv Dec 31, 2018 •

edited

Loading

jcouv Dec 31, 2018 •

edited

Loading

jcouv Dec 31, 2018 •

edited by cston

Loading

jcouv Dec 31, 2018 •

edited

Loading

jaredpar Dec 31, 2018 •

edited by jcouv

Loading

alrz Jan 1, 2019 •

edited by jcouv

Loading

jcouv Dec 31, 2018 •

edited

Loading

jcouv Dec 31, 2018 •

edited

Loading

jaredpar Dec 31, 2018 •

edited by cston

Loading

jaredpar Dec 31, 2018 •

edited by cston

Loading

jcouv Jan 2, 2019 •

edited

Loading

jcouv commented Jan 2, 2019 •

edited

Loading

jcouv Jan 2, 2019 •

edited

Loading

AlekseyTs Jan 3, 2019 •

edited

Loading

AlekseyTs Jan 3, 2019 •

edited

Loading

AlekseyTs Jan 3, 2019 •

edited

Loading

AlekseyTs Jan 3, 2019 •

edited

Loading

AlekseyTs commented Jan 3, 2019 •

edited

Loading

AlekseyTs Jan 3, 2019 •

edited

Loading

AlekseyTs Jan 3, 2019 •

edited

Loading

AlekseyTs Jan 3, 2019 •

edited

Loading

jcouv commented Jan 3, 2019 •

edited

Loading

AlekseyTs Jan 3, 2019 •

edited

Loading

AlekseyTs Jan 3, 2019 •

edited

Loading

AlekseyTs Jan 3, 2019 •

edited

Loading