Suggest parameter names from matching overloads by Youssef1313 · Pull Request #52801 · dotnet/roslyn

Youssef1313 · 2021-04-21T15:06:40Z

Youssef1313 · 2021-04-21T15:41:09Z

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs

+            foreach (var sibling in siblingMethods)
+            {
+                foreach (var siblingParameter in sibling.ParameterList.Parameters)


@sharwell Any performance concerns?

Not really. There are other completion scenarios that are more expensive without problems.

Maybe try running this in a class with a large number of methods all with different names, just to make sure the outer loop isn't too slow in huge types?

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs

Youssef1313 · 2021-04-26T16:53:55Z

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs

+                    var partialSemanticModel = await document.GetPartialSemanticModelAsync(cancellationToken).ConfigureAwait(false);
+                    if (partialSemanticModel is not null)
+                        AddNamesFromExistingOverloads(context, partialSemanticModel, result, cancellationToken);


Caller already has a semantic model, but it's not a partial semantic model.

var semanticModel = await document.ReuseExistingSpeculativeModelAsync(position, cancellationToken).ConfigureAwait(false);

I used partial semantic model per your suggestion @sharwell.

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs

Youssef1313 · 2021-05-29T12:31:05Z

@sharwell @CyrusNajmabadi Anything left here? Thanks!

davidwengier

This LGTM, but would be good to get a tick from @CyrusNajmabadi or @sharwell for any existing perf concerns.

In particular whilst there might be de-duping code in completion itself, I don't know if this is something we want to rely on. Duplicates in overloads seem like they would be very common, and it would be very simple to filter them out here (add them to the hashset this is already checking).

CyrusNajmabadi · 2021-07-28T03:19:20Z

I can look tomorrow

CyrusNajmabadi · 2021-08-02T21:43:42Z

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs

+            if (parameterSyntax is not { Type: { } parameterType, Parent: { Parent: BaseMethodDeclarationSyntax baseMethod } })
+                return;
+
+            var overloads = GetOverloads(baseMethod, namedType);


my preference is to swap these parameters so we always go from 'wider to narrow' scope for them.

CyrusNajmabadi · 2021-08-02T21:43:59Z

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs

+            if (!overloads.Any())
+                return;
+
+            var methodParameterType = semanticModel.GetTypeInfo(parameterType, cancellationToken).Type;


consider doing this first before botherin gto collect overloads.

CyrusNajmabadi · 2021-08-02T21:45:35Z

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs

+
+            var overloads = GetOverloads(baseMethod, namedType);
+
+            if (!overloads.Any())


this will causde double enumeration of an enumerable.

Youssef1313 · 2021-12-03T16:17:50Z

I fixed the conflicts. If CI failed, I will get to that in a week after finishing my exams.

Youssef1313 · 2021-12-12T18:16:11Z

@CyrusNajmabadi This should be ready for another look :)

CyrusNajmabadi · 2021-12-12T21:30:33Z

...Features/CSharpTest/Completion/CompletionProviders/DeclarationNameCompletionProviderTests.cs

+}
+";
+            await VerifyItemExistsAsync(markup, "myTok", glyph: (int)Glyph.Parameter);
+            await VerifyItemExistsAsync(markup, "cancellationToken", glyph: (int)Glyph.Parameter);


can you add an assert about what is selected?

@CyrusNajmabadi What's the API for doing so? Or does it need an integration test?

i think you'll need to add a test to CompletionCommandHandlerTests

Added the test.

...Features/CSharpTest/Completion/CompletionProviders/DeclarationNameCompletionProviderTests.cs

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs

CyrusNajmabadi · 2021-12-25T19:27:30Z

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs

+            {
+                return baseMethod switch
+                {
+                    MethodDeclarationSyntax method => namedType.GetMembers(method.Identifier.ValueText).OfType<IMethodSymbol>().ToImmutableArray(),


do you want to collect from teh base type as well?

No strong opinion. I think we can keep it simple for now instead of walking through base types.

CyrusNajmabadi

approving. but if you want to add another test, i would like that.

Youssef1313 · 2022-01-23T16:53:45Z

@CyrusNajmabadi Sorry for the delay. I added the test you requested.

CyrusNajmabadi · 2022-02-06T00:50:08Z

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs

+
+            var parameterSyntax = context.LeftToken.GetAncestor(n => n.IsKind(SyntaxKind.Parameter)) as ParameterSyntax;
+            if (parameterSyntax is not { Type: { } parameterType, Parent: { Parent: BaseMethodDeclarationSyntax baseMethod } })
+                return;


can you check that the left token is hte last token of the parameter type. i'm actually worried about you offering these names inside the parameter type. Can you add tests that shows this doesn't happen (for example, in the type arg of a generic type).

@CyrusNajmabadi Likely already handled in GetDeclarationInfoAsync. I'm not sure about the exact test case you're referring to. Something like this?

void M<T>($$) { }

No. more like: void M(List<$$> the left token will be inside a parameter here.

CyrusNajmabadi · 2022-02-06T00:52:05Z

overall, i like this. but i do have the concern about this showing up not inside more than just the parameter name.

CyrusNajmabadi · 2022-02-06T00:52:50Z

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs

+                return;
+
+            var parameterSyntax = context.LeftToken.GetAncestor(n => n.IsKind(SyntaxKind.Parameter)) as ParameterSyntax;
+            if (parameterSyntax is not { Type: { } parameterType, Parent: { Parent: BaseMethodDeclarationSyntax baseMethod } })


Suggested change

if (parameterSyntax is not { Type: { } parameterType, Parent: { Parent: BaseMethodDeclarationSyntax baseMethod } })

if (parameterSyntax is not { Type: { } parameterType, Parent.Parent: BaseMethodDeclarationSyntax baseMethod })

also, we don't want this in the optional initializer value of a parameter either.

also, we don't want this in the optional initializer value of a parameter either.

I'm not sure what you mean.

// I think this is not handled by declaration name completion at all? public void M(string s = $$) { }

basically, i'm saying that this test doesn't guarantee that (even if some other check does). So it would be good to have a test here for those cases, just to show there's no issue.

@CyrusNajmabadi Yeah sure adding the test is good for coverage anyway even if already handled. I was trying to make sure I'm properly understanding the scenario you're referring to.

yup. that's the scenario. the core issue i have with this code is that all it does is check if it's contained within a parameter.

Now, it sounds like there's other code that ensures that we're typing the parameter name. If so, that's great. I just want tests to ensure that works properly. thanks!

…clarationNameCompletionProvider.cs Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>

Youssef1313 · 2022-02-06T09:35:02Z

@CyrusNajmabadi Anything left?

CyrusNajmabadi · 2022-02-06T17:33:11Z

Thanks!

Suggest parameter names from matching overloads

d609159

Youssef1313 requested a review from a team as a code owner April 21, 2021 15:06

ghost added the Area-IDE label Apr 21, 2021

Youssef1313 commented Apr 21, 2021

View reviewed changes

jinujoseph assigned sharwell Apr 21, 2021

jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Apr 21, 2021

MaStr11 reviewed Apr 25, 2021

View reviewed changes

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs Outdated Show resolved Hide resolved

sharwell reviewed Apr 25, 2021

View reviewed changes

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs Outdated Show resolved Hide resolved

sharwell reviewed Apr 25, 2021

View reviewed changes

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs Outdated Show resolved Hide resolved

Youssef1313 added 3 commits April 26, 2021 18:48

Use semantic model

d7c046e

Be consistent

72369ac

Be consistent

3d1ab93

Youssef1313 commented Apr 26, 2021

View reviewed changes

Youssef1313 requested a review from sharwell April 27, 2021 06:13

CyrusNajmabadi reviewed Apr 27, 2021

View reviewed changes

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Apr 27, 2021

View reviewed changes

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Apr 27, 2021

View reviewed changes

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Apr 27, 2021

View reviewed changes

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Youssef1313 marked this pull request as draft April 28, 2021 08:45

Youssef1313 added 2 commits April 28, 2021 11:21

Address feedback and few fixes

43ef597

Add test for constructors

405b120

Youssef1313 marked this pull request as ready for review April 28, 2021 09:26

Youssef1313 requested a review from CyrusNajmabadi May 3, 2021 06:46

davidwengier approved these changes Jul 28, 2021

View reviewed changes

CyrusNajmabadi reviewed Aug 2, 2021

View reviewed changes

runfoapp bot mentioned this pull request Dec 3, 2021

Microsoft.CodeAnalysis.CSharp.CommandLine.UnitTests.CommandLineTests.ArgumentParsing is flaky #58077

Open

Fix exception and compile error due to signature change in another PR

e4fb0e2

CyrusNajmabadi reviewed Dec 12, 2021

View reviewed changes

...Features/CSharpTest/Completion/CompletionProviders/DeclarationNameCompletionProviderTests.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Dec 12, 2021

View reviewed changes

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs Outdated Show resolved Hide resolved

Simplify check

a7d00c2

Youssef1313 requested a review from CyrusNajmabadi December 25, 2021 18:15

CyrusNajmabadi reviewed Dec 25, 2021

View reviewed changes

CyrusNajmabadi approved these changes Dec 25, 2021

View reviewed changes

jinujoseph assigned CyrusNajmabadi and unassigned sharwell Jan 10, 2022

Add test

39d625d

runfoapp bot mentioned this pull request Jan 24, 2022

Test failure in Microsoft.CodeAnalysis.CSharp.Emit.UnitTests.dll #57560

Closed

Youssef1313 requested a review from CyrusNajmabadi February 5, 2022 20:23

CyrusNajmabadi reviewed Feb 6, 2022

View reviewed changes

Youssef1313 and others added 2 commits February 6, 2022 09:45

Update src/Features/CSharp/Portable/Completion/CompletionProviders/De…

9695265

…clarationNameCompletionProvider.cs Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>

Add requested tests

38906af

CyrusNajmabadi merged commit be221d5 into dotnet:main Feb 6, 2022

ghost added this to the Next milestone Feb 6, 2022

Youssef1313 deleted the intellisense-completion-overloads branch February 6, 2022 17:38

runfoapp bot mentioned this pull request Feb 7, 2022

[Flaky Test] VisualBasic.BasicAddMissingReference.InvokeSomeFixesInVisualBasicThenVerifyReferences #57980

Closed

RikkiGibson modified the milestones: Next, 17.2.P2 Mar 1, 2022


		var overloads = GetOverloads(baseMethod, namedType);

		if (!overloads.Any())

	if (parameterSyntax is not { Type: { } parameterType, Parent: { Parent: BaseMethodDeclarationSyntax baseMethod } })
	if (parameterSyntax is not { Type: { } parameterType, Parent.Parent: BaseMethodDeclarationSyntax baseMethod })

Conversation

Youssef1313 commented Apr 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Youssef1313 commented May 29, 2021

Uh oh!

davidwengier left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Jul 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Youssef1313 commented Dec 3, 2021

Uh oh!

Youssef1313 commented Dec 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

Youssef1313 commented Jan 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Feb 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Youssef1313 commented Feb 6, 2022

Uh oh!

CyrusNajmabadi commented Feb 6, 2022

Uh oh!

Reviewers

Youssef1313 Dec 13, 2021 •

edited

Loading