Inline Hints: More logic around handling named arguments

[akhera99]

Fixes: #59317

1. Iterate through the arguments first
2. Create a listing of ParameterSymbols that map back to named arguments
3. Go through the arguments again
4. Get the parameter symbol the argument maps back to
5. If it is a parameter symbol contained in the list, don't add a hint for it

[ryzngard]

Aren't we already skipping here because of line 80?

if (argument.NameColon != null)
    continue;

[akhera99]

This is for this case:

In which DetermineParameter maps the argument "true" to the parameter "d". So GetParametersOfNamedArguments goes through and gets the parameters in those mappings and makes sure to skip them. The argument.NameColon is necessary for the named argument "d:" since we still want to skip it.

[ryzngard]

ahhhh.... I get it now. Thanks for the explanation

[ryzngard]

I'm confused by this fix so maybe I'm missing something. It looks like all the work you do in GetParametersOfNamedArguments is duplicate continue logic in the for. What is causing this to be different?

[akhera99]

I'm confused by this fix so maybe I'm missing something. It looks like all the work you do in GetParametersOfNamedArguments is duplicate continue logic in the for. What is causing this to be different?

Previously, if there was a named argument that wasn't at the exact location as the parameter, then it was still being shown. This is to combat that case.

[Youssef1313]

Why not update DetermineParameter to not return a symbol if there is an overload resolution error?

More specifically, remove this if entirely:

roslyn/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/ArgumentSyntaxExtensions.cs

Lines 53 to 56 in 8192820

	if (symbol == null && symbolInfo.CandidateSymbols.Length == 1)
	{
	symbol = symbolInfo.CandidateSymbols[0];
	}

[akhera99]

Why not update DetermineParameter to not return a symbol if there is an overload resolution error?

More specifically, remove this if entirely:

roslyn/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/ArgumentSyntaxExtensions.cs

Lines 53 to 56 in 8192820

	if (symbol == null && symbolInfo.CandidateSymbols.Length == 1)
	{
	symbol = symbolInfo.CandidateSymbols[0];
	}

I remember adding that bit of code back when I first implemented this feature, it had something to do with an overload case. I need to see if it can just be removed.

Edit:
I remember now, in incomplete function calls we still want to try and add a hint if we can.
For instance:

class A
{
    int testMethod(int x, object y)
    {
        return x;
    }
    void Main() 
    {
        testMethod(-(int)5.5,);
    }
}

Should still show a hint for the argument in testMethod and it doesn't if we don't try to use the candidate symbol.

[Youssef1313]

@akhera99 Does disabling inline parameter hints completely when there are more arguments than the parameter count make sense? I don't think we can give any good result for this case.

[akhera99]

@akhera99 Does disabling inline parameter hints completely when there are more arguments than the parameter count make sense? I don't think we can give any good result for this case.

Yeah, I have no qualms with that, though I think that is orthogonal to this issue about named arguments.

[Youssef1313]

@akhera99 The issue with named arguments seems to be reproducible only when the given arguments are greater than the number of parameters. If this assumption is correct, then the suggestion will result in a less complex fix.

An alternative is to somehow tweak DetermineParameter such that we guarantee it doesn't return the same parameter for different arguments. Here:

It returns the symbol for parameter d for both the true and the named argument, I think.