Cleanup overload resolution code used in signature help #71329
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| @@ -5,297 +5,252 @@ | |||
| using System; | |||
There was a problem hiding this comment.
View with whitespace off.
| protected readonly struct LightweightOverloadResolution( | ||
| SemanticModel semanticModel, | ||
| int position, | ||
| SeparatedSyntaxList<ArgumentSyntax> arguments) |
There was a problem hiding this comment.
made this a helper struct. moved up common, readonly, state to the primary constructor.
| { | ||
| internal static class LightweightOverloadResolution | ||
| public (IMethodSymbol? method, int parameterIndex) RefineOverloadAndPickParameter(SymbolInfo symbolInfo, ImmutableArray<IMethodSymbol> candidates) |
There was a problem hiding this comment.
made this method take a symbolinfo + candidates. taht way we could move common caller method into it.
| { | ||
| // map the arguments to their corresponding parameters | ||
| var argumentCount = arguments.Count; | ||
| using var _ = ArrayBuilder<int>.GetInstance(argumentCount, fillWithValue: -1, out var argToParamMap); |
There was a problem hiding this comment.
a common problem was using fillWithValue, but then not ensuring the Count of hte array matches the count requested. unless .Add is actually called the right number of times, that won't be the case. I changed this to just get empty arrays and add hte number of fill-with-value values manually.
| // parameter, we will highlight the first unspecified parameter. | ||
| static int FirstUnspecifiedParameter(ArrayBuilder<int> argToParamMap, int argumentCount) | ||
| { | ||
| using var _ = ArrayBuilder<bool>.GetInstance(argumentCount, false, out var specified); |
There was a problem hiding this comment.
this logic was very suspect. it made an array filled with 'false', but which had a count of 0. so this would only work properly if the argToParameterIndex happened to be in order. if, say, there was an out of order arg/param, this would index improperly.
| var type = parameter.Type; | ||
| if (parameter.IsParams | ||
| && type is IArrayTypeSymbol arrayType | ||
| && semanticModel.ClassifyConversion(argument.Expression, arrayType.ElementType).IsImplicit) |
There was a problem hiding this comment.
inlined this method.
| } | ||
| // A by-value argument matches an `in` parameter | ||
| if (parameterRefKind == RefKind.In && argumentRefKind == RefKind.None) | ||
| return true; |
There was a problem hiding this comment.
swapped order for clarity.
| } | ||
| // We count the empty argument as a used position | ||
| if (!seenOutOfPositionArgument) | ||
| AddArgumentToParameterMapping(argumentIndex, currentParameterIndex, ref argumentToParameterMap); | ||
| } |
There was a problem hiding this comment.
always used with AddArgToParamMapping. so just inlined there.
| // `$$,` points to the argument before the separator | ||
| // but `,$$` points to the argument following the separator | ||
| if (position <= arguments.GetSeparator(i).Span.Start) | ||
| return i; | ||
| } |
There was a problem hiding this comment.
inlined to single callsite.
| var arguments = constructorInitializer.ArgumentList.Arguments; | ||
| var candidates = semanticModel.GetSymbolInfo(constructorInitializer, cancellationToken).Symbol is IMethodSymbol exactSymbol | ||
| ? ImmutableArray.Create(exactSymbol) | ||
| : constructors; |
There was a problem hiding this comment.
pushed this into RefineOverloadAndPickParameter.
| /// </summary> | ||
| private bool TryPrepareArgumentToParameterMap(IMethodSymbol method, ref TemporaryArray<int> argumentToParameterMap) | ||
| { | ||
| Contract.ThrowIfTrue(argumentToParameterMap.Count != arguments.Count); |
There was a problem hiding this comment.
Instead of passing in the array as a ref, could it be initialized here and returned as an out parameter?
There was a problem hiding this comment.
not while using it in a using scope properly.
| Debug.Assert(parameterIndex >= 0); | ||
| Debug.Assert(parameterIndex < method.Parameters.Length); | ||
|
|
||
| inParams |= method.Parameters[parameterIndex].IsParams; |
There was a problem hiding this comment.
no, i think this means "user is currently in the params parameters).
src/Features/CSharp/Portable/SignatureHelp/LightweightOverloadResolution.cs
Outdated
Show resolved
Hide resolved
…Resolution.cs Co-authored-by: David Barbet <[email protected]>
ghost
added this to the 
We've seen some crashes here, so i rewrote things to simplify and remove certain issues.