Local functions not available in intellisense/autocomplete

[miloush]

Version Used: 16..3.0 Preview 2.0

Steps to Reproduce:

namespace ConsoleApp1
{
    class Program
    {
        static void Main(string[] args)
        {
            Loca|

            void Local() { }
        }
    }
}

Expected Behavior: Local function offered in autocomplete list and provided with intellisense support when typing arguments.

Actual Behavior: Local function not in the list and gets colorized only after arguments match the signature.

[gafter]

I'm not sure that the parser would recognize that there is a local function in your example, preceded as it is by a garbage token.

[miloush]

static void Main()
{
    Loca

    switch (Console.BackgroundColor)
    {
        default:
            break;
    }

    Main();
            
    void Local() { }
}

This does not help either.

Even in code with no syntax errors,

static void Main()
{
    Local(4, 2);

    void Local(int a, int b) { }
}

The Display parameter info command does not show any intellisense at the callsite.

[gafter]

Hey IDE folks, is there some API not behaving as expected here, or is this just not implemented in the IDE?

[sharwell]

Regression in 16.3 Preview 2 relative to 15.9. Also works in 16.2.2.

[sharwell]

/cc @ivanbasov

[svick]

As far as I can tell, this regression was introduced by #35822, which probably explains why the issue happens in static methods, but not in instance methods.

[ivanbasov]

@gafter the issue is totally on the compiler side. The semantic model does not return local functions defined inside static members. I debugged it and found that local functions are filtered out inside static scopes if is no static modifier provided for it.

I would consider this line: https://github.com/dotnet/roslyn/blob/master/src/Compilers/CSharp/Portable/Binder/Binder_Lookup.cs#L1468

Maybe it is worth to change it to
return symbol.RequiresInstanceReceiver();

[svick]

I have submitted #38117, which fixes the completion issue using the approach suggested by @ivanbasov.

But that still leaves the Display parameter info issue. In other words, the following test will still fail, even if that PR is merged:

roslyn/src/EditorFeatures/CSharpTest/SignatureHelp/InvocationExpressionSignatureHelpProviderTests.cs

Lines 2421 to 2437 in 62918fc

	public async Task TestLocalFunctionInStaticMethod()
	{
	var markup = @"
	class C
	{
	static void M()
	{
	void Local() { }
	Local($$);
	}
	}";
	var expectedOrderedItems = new List<SignatureHelpTestItem> { new SignatureHelpTestItem("void Local()") };
	await TestAsync(markup, expectedOrderedItems);
	}

I suspect the problem is here:

roslyn/src/Features/CSharp/Portable/SignatureHelp/InvocationExpressionSignatureHelpProvider_MethodGroup.cs

Lines 56 to 60 in 5551fad

	else if (invocationExpression.Expression is SimpleNameSyntax &&
	invocationExpression.IsInStaticContext())
	{
	methodGroup = methodGroup.Where(m => m.IsStatic);
	}

I think this could be resolved by using RequiresInstanceReceiver too, except that RequiresInstanceReceiver is internal, so it can't be used here. What would be the right fix here? Would it make sense to add RequiresInstanceReceiver to ISymbol? (It's currently only defined for C# Symbol.)

[gafter]

@333fred What do you think is the right approach to address this?

[ivanbasov]

Thank you for your help, @svick ! The change to InvocationExpressionSignatureHelpProvider_MethodGroup.cs seems to be reasonable for me. If the compiler team is OK with the approach in general (@333fred to review), please go ahead and add it to #38117 as well

[333fred]

I like the idea of adding the API, though I'm not sure of the naming. RequiresInstanceReceiver was fine when it was an internal API, but it doesn't fit with the rest of the properties on ISymbol as a public API. Perhaps IsInstanceMember? @gafter, do you have other thoughts on naming?

[333fred]

Additionally, the docs will need to be clear where the potential differences between these two properties lies, as a naive first glance makes these properties appear identical.

[333fred]

@svick @ivanbasov, is there a reason that the IDE can't just additionally filter based on IMethodSymbol.MethodKind? The filter could be Where(m => m.IsStatic || m is IMethodSymbol { MethodKind: MethodKind.LocalFunction }), right? Am I missing something here that warrants adding a new API here?

[ivanbasov]

@333fred not sure I understand you. Do you mean that you want to keep the binder code as is and correct IDE only by explicit adding items with || m is IMethodSymbol { MethodKind: MethodKind.LocalFunction } to the list? If so, is the binder code correct in the place for other scenarios?

[333fred]

That's correct.

[CyrusNajmabadi]

@333fred @ivanbasov i'm a little confused. It appears as if the issue is:

I debugged it and found that local functions are filtered out inside static scopes if is no static modifier provided for it.

Could you be explicit about where that filtering is happening? Is it in IDE or code or in compiler code?

If it's in IDE code, we could certainly fixup teh filtering. If it's compiler code, it's unclear to me what IDE could do as the filtering is happening before we ever see the symbol.

[333fred]

@CyrusNajmabadi #38074 (comment). This is in the IDE layer, not the compiler. I'm not sure what RequiresInstanceReceiver would even do for you here. The IDE just needs to include all local functions that are in scope.

[CyrusNajmabadi]

The IDE just needs to include all local functions that are in scope.

Ok... :) But the IDE doesn't have logic for that. All we call is LookupSymbols to ask for the "all [...] that are in scope".

I think the problem is that the compiler is considering this not in scope, when we really want it to be for intellisense purposes in this scenario.

IDE def doesn't want to reimpleemnt the "include all XXX in scope" logic. That's basically reimplementing the complex compiler rules that exist here and one of the reason we don't do that and instead use LookupSymbols instead.

Does that help clear things up?

[CyrusNajmabadi]

I misunderstood the repro. A simpler repro is:

    static void Foo()
    {
        void Bar()
        {

        }

        Bar$$ // Bar should appear here but doesn't.
    }

[CyrusNajmabadi]

Ok. So i grossly misunderstood things. I think there are two feasible solutions. HOwever, i personally think it's totally ok to just do the expeditious thing here. i.e. we should just update

roslyn/src/Features/CSharp/Portable/SignatureHelp/InvocationExpressionSignatureHelpProvider_MethodGroup.cs

Line 59 in 5551fad

methodGroup = methodGroup.Where(m => m.IsStatic);

so that it checks:

m.IsStatic || m is IMethodSymbol { MethodKind: MethodKind.LocalFunction }

I'm totally amenable to the idea that .RequiresInstanceReceiver is "more right". But i think the value is so narrow, and the cost of hte public APi so high, that we should just take the expeditious route.

[vatsalyaagrawal]

https://devdiv.visualstudio.com/DevDiv/_workitems/edit/985838

[ivanbasov]

Fixed with #38117 (16.4 P1) and #38811 (16.4 P2). I do not see open issues with this. Please let me know if there are.