Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SignatureHelp: guess best overload for incomplete invocations #31843

Merged
merged 44 commits into from
Feb 2, 2022
Merged

SignatureHelp: guess best overload for incomplete invocations #31843

merged 44 commits into from
Feb 2, 2022

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Dec 16, 2018 •
edited
Loading

The current behavior is that a provider will recommend a selection only if the compiler definitely identified the symbol. But when overload resolution failed, the compiler doesn't say which overload was the best match.
I implemented a simple heuristic (poor man's overload resolution), which just checks if the arguments are compatible with the parameters.

Fixes #6713

Tagging @CyrusNajmabadi @dpoeschl @sharwell

@jcouv jcouv added the Area-IDE label Dec 16, 2018
@jcouv jcouv added this to the 16.0.P2 milestone Dec 16, 2018
@jcouv jcouv self-assigned this Dec 16, 2018
@jcouv jcouv requested a review from a team as a code owner December 16, 2018 19:07
CyrusNajmabadi
CyrusNajmabadi reviewed Dec 16, 2018
View reviewed changes
src/Workspaces/VisualBasic/Portable/LanguageServices/VisualBasicSemanticFactsService.vb Outdated

Public Function CanConvert(semanticModel As SemanticModel, node As SyntaxNode, type As ITypeSymbol) As Boolean Implements ISemanticFactsService.CanConvert
Dim conversion = semanticModel.ClassifyConversion(DirectCast(node, ExpressionSyntax), type)
Return conversion.Exists
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Dec 16, 2018 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not .isWidening for VB to match .IsImplicit for C#? #Resolved

Copy link
Member Author

@jcouv jcouv Dec 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was meant to be minimal. I'll fix when I do the VB implementation/testing. #Resolved

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Dec 16, 2018 •
edited by jcouv
Loading

I implemented a simple heuristic (poor man's overload resolution), which just checks if the arguments are compatible with the parameters. Argument names are not taken into account at this point.

Seems reasonable. Though i'm curious about the costs of arg name checking.

Humorously enough doesn't sig-help already use provided arg-names to filter down the list? Or, at least, to figure out which param you're on to show the appropriate sig help docs for that param? #Resolved

@jcouv
Copy link
Member Author

jcouv commented Dec 16, 2018 •
edited
Loading

Though i'm curious about the costs of arg name checking.
I don't know either. I may take a stab.

Yes, sig-help filters out the list based on named arguments. But that logic is simple (it doesn't need to understand non-trailing named arguments).

PS: This reminds me, I should probably handle params too... #Closed

@jcouv jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Dec 18, 2018
@jcouv jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Dec 25, 2018
@jcouv
Copy link
Member Author

jcouv commented Dec 25, 2018 •
edited
Loading

Added handling for named arguments, empty arguments and params. #Closed

sharwell
sharwell previously requested changes Jan 2, 2019
View reviewed changes
Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was only able to partially complete this review so far. Will go through it again.

src/Features/CSharp/Portable/SignatureHelp/AbstractCSharpSignatureHelpProvider.cs Outdated Show resolved Hide resolved
src/Features/CSharp/Portable/SignatureHelp/AbstractCSharpSignatureHelpProvider.cs Outdated Show resolved Hide resolved
...tures/CSharp/Portable/SignatureHelp/InvocationExpressionSignatureHelpProvider_MethodGroup.cs Outdated Show resolved Hide resolved
CyrusNajmabadi
CyrusNajmabadi reviewed Jan 3, 2019
View reviewed changes
src/Features/CSharp/Portable/SignatureHelp/AbstractCSharpSignatureHelpProvider.cs Outdated
/// </summary>
protected static bool IsInacceptable(SeparatedSyntaxList<ArgumentSyntax> arguments, IMethodSymbol method)
protected static bool IsUnacceptable(SeparatedSyntaxList<ArgumentSyntax> arguments, IMethodSymbol method)
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jan 3, 2019 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of having IsUnacceptable which tehn has to eb used like => !IsUnacceptable, consider just having => IsAcceptable. #Resolved

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this cmment still applies, unless you strongly like the negative form here.

CyrusNajmabadi
CyrusNajmabadi reviewed Jan 26, 2022
View reviewed changes
...s/CSharp/Portable/SignatureHelp/InvocationExpressionSignatureHelpProviderBase_MethodGroup.cs Outdated
CancellationToken cancellationToken)
{
return Task.FromResult(
(accessibleMethods.SelectAsArray(m => ConvertMethodGroupMethod(document, m, invocationExpression.SpanStart, semanticModel)),
TryGetSelectedIndex(accessibleMethods, currentSymbol.Symbol)));
TryGetSelectedIndex(accessibleMethods, currentSymbol)));
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jan 26, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the prior spacing was intentional fwiw :) #Pending

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

specifically, teh tuple element values were aligned.

CyrusNajmabadi
CyrusNajmabadi reviewed Jan 26, 2022
View reviewed changes
src/Features/Core/Portable/SignatureHelp/AbstractSignatureHelpProvider.cs Outdated
if (selectedItem == -1)
{
selectedItem = null;
}
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jan 26, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this something you encountered? #Pending

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably did at some point, but seems this can now be removed. Thanks

CyrusNajmabadi
CyrusNajmabadi reviewed Jan 29, 2022
View reviewed changes
src/Features/CSharp/Portable/SignatureHelp/ObjectCreationExpressionSignatureHelpProvider.cs Outdated
{
result.ArgumentIndex = parameterIndex;
}
return result;
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jan 29, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you extract a helper in the base type that these guys can all share? #Resolved

@jcouv jcouv enabled auto-merge (squash) January 29, 2022 01:27
@jcouv jcouv disabled auto-merge January 29, 2022 01:28
@jcouv jcouv enabled auto-merge (squash) January 29, 2022 05:30
@jcouv jcouv disabled auto-merge February 2, 2022 18:38
@jcouv jcouv merged commit 278a27f into dotnet:main Feb 2, 2022
@jcouv jcouv deleted the sig-help branch February 2, 2022 18:38
@ghost ghost modified the milestones: Backlog, Next Feb 2, 2022
@RikkiGibson
Copy link
Contributor

I'm anticipating being able to rest the fingers that hit the up-down keys to search through overloads 🎉

@jcouv
Copy link
Member Author

jcouv commented Feb 2, 2022

Yay! Thanks @CyrusNajmabadi, @sharwell. It feels good to get this 2018 PR finally merged :-) That'll teach me not to let PRs go so stale...

@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P1 Feb 4, 2022
@commit-lint commit-lint bot mentioned this pull request Feb 9, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@333fred 333fred 333fred left review comments

@sharwell sharwell sharwell left review comments

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

Assignees

@sharwell sharwell

@jcouv jcouv

Labels
Area-IDE Feature Request
Projects
None yet
Milestone
17.2.P1
Development

Successfully merging this pull request may close these issues.

Use argument types to guide Signature Help
6 participants
@jcouv @CyrusNajmabadi @sharwell @RikkiGibson @333fred @jinujoseph