LocalFunctionSymbol.IsStatic always returns true

[jcouv]

Current constructor always sets an implicit declaration modifier:

        public LocalFunctionSymbol(
            Binder binder,
            Symbol containingSymbol,
            LocalFunctionStatementSyntax syntax)
        {
            _declarationModifiers =
                DeclarationModifiers.Private |
                DeclarationModifiers.Static |
                syntax.Modifiers.ToDeclarationModifiers(diagnostics: _declarationDiagnostics);

IsStatic checks the declaration modifiers:

        public override bool IsStatic => (_declarationModifiers & DeclarationModifiers.Static) != 0;

From discussion with @agocke and @AlekseyTs, IsStatic should always return false. Some overload resolution logic may need to be adjusted (to have special handling for local functions).

To save some typing, here's a test I had.

        [Fact]
        [WorkItem(27028, "https://github.com/dotnet/roslyn/issues/27028")]
        public void LocalFunction_IsStatic()
        {
            const string text = @"
class C
{
    void M()
    {
        void local1() { }
        void outer() { void local2() { } }
    }
    static void M2()
    {
        void local3() { }
        void outer() { void local4() { }
    }
}";
            var comp = CreateCompilation(text);
            var tree = comp.SyntaxTrees.Single();
            var model = comp.GetSemanticModel(tree);

            var local1 = tree.GetRoot().DescendantNodes().OfType<LocalFunctionStatementSyntax>().ElementAt(0);
            Assert.Equal("void local1() { }", local1.ToString());
            var local1Symbol = (IMethodSymbol)model.GetDeclaredSymbol(local1);
            verify(local1Symbol);

            var local2 = tree.GetRoot().DescendantNodes().OfType<LocalFunctionStatementSyntax>().ElementAt(2);
            Assert.Equal("void local2() { }", local2.ToString());
            var local2Symbol = (IMethodSymbol)model.GetDeclaredSymbol(local2);
            verify(local2Symbol);

            var m2 = tree.GetRoot().DescendantNodes().OfType<MethodDeclarationSyntax>().ElementAt(1);
            Assert.Equal("M2", m2.Identifier.ToString());
            var m2Symbol = model.GetDeclaredSymbol(m2);
            Assert.True(m2Symbol.IsStatic);

            var local3 = tree.GetRoot().DescendantNodes().OfType<LocalFunctionStatementSyntax>().ElementAt(3);
            Assert.Equal("void local3() { }", local3.ToString());
            var local3Symbol = (IMethodSymbol)model.GetDeclaredSymbol(local3);
            verify(local3Symbol);

            var local4 = tree.GetRoot().DescendantNodes().OfType<LocalFunctionStatementSyntax>().ElementAt(5);
            Assert.Equal("void local4() { }", local4.ToString());
            var local4Symbol = (IMethodSymbol)model.GetDeclaredSymbol(local4);
            verify(local4Symbol);

            void verify(IMethodSymbol localSymbol)
            {
                Assert.False(localSymbol.IsAbstract);
                Assert.False(localSymbol.IsAsync);
                Assert.True(localSymbol.IsDefinition);
                Assert.False(localSymbol.IsExtensionMethod);
                Assert.False(localSymbol.IsExtern);
                Assert.False(localSymbol.IsGenericMethod);
                Assert.False(localSymbol.IsImplicitlyDeclared);
                Assert.False(localSymbol.IsOverride);
                Assert.False(localSymbol.IsSealed);
                Assert.False(localSymbol.IsStatic);
                Assert.False(localSymbol.IsVirtual);
            }
        }

Relates to #27028

Note: in the implementation of static local functions, an internal property IsStaticLocalFunction was added. It should be removed when this issue is fixed.

[gafter]

We classify "methods" into instance methods, which are invoked off an enclosing instance, and static methods, which are not. It is under this classification that local functions are considered static methods.

[agocke]

@gafter I see. Do you know where "static" is described in the spec? Our public API is useless here, it says "Gets a value indicating whether the symbol is static".

@jcouv If we've defined static in this context to mean "does not have a receiver", this may actually be the correct behavior. I still think messing with the declaration modifiers was a bad idea though.

[gafter]

@agocke The C# language spec won't say anything about this. It is an artifact of Roslyn modeling local functions as methods. Roslyn's engineers had to make up their own definition that somehow mediates that mushed-together model, and this is what we (you?) came up with.

There is a proposal to give meaning to "static" on local functions (that do not capture anything), and if we do that we'll need to represent that concept somehow; the "static" modifier seems the best thing for that purpose. If and when we do that, we'd have to make regular local functions (that can capture stuff) not have the static modifier set. Should we preemptively make that change now? I don't know whether or not that is the best approach.

[agocke]

I was referring to the meaning of static on members. I thought it may be useful to compare the description there with whatever we choose for local functions.

The spec says per https://github.com/dotnet/csharplang/blob/master/spec/classes.md#static-and-instance-members

A static function member (method, property, event, operator, or constructor) does not operate on a specific instance, and it is a compile-time error to refer to this in such a function member.

That statement conveniently allows for multiple interpretations. Either a local function could be always static because it doesn't operate on a specific instance, or it could be static only when it can refer to this, or it could be never static as being nonsensical on a local function.

Looking at the full definition I think @jcouv's definition of a local function being static if it cannot access this is quite reasonable. The overlap with "no-capture local functions" is interesting. The curious thing is that a "no-capture" local function will necessary always be "static" because it can't access this. The opposite is not true. A local function in a static method may be capturing, but it still can't capture this (because there is no this in context). I'm not sure if this is an argument for or against using IsStatic to represent that information.

[gafter]

Local functions are not members, so we can justify whatever we want to do 😉

[jaredpar]

We should just clean this up as we actually implement static local functions.

[YairHalberstadt]

I gave this a quick try - I made local functions non- static unless they were static local functions, and added another method RequiresInstanceReciever

My conclusion: the vast majority of calls to IsStatic would have to be replaced by RequiresInstanceReciever.

IMO this is error prone, and has little benefit.

Instead, I would suggest that IsStatic stays as it is, and we explicitly document this meaning.

We then move IsStaticLocalFunction to MethodSymbol, and expose it publicly.

[jcouv]

@agocke Can this issue be closed now? If not, it probably can be moved to Compiler.Next milestone.

[agocke]

Yup, this is can be closed out