don't include implicit/explicit keywords when completing explicit interface operator #53938

Youssef1313 · 2021-06-08T09:52:23Z

~~This adds a public API :(~~
Another solution would be to handle this case entirely in the completion provider, i.e, do displayText.Replace("implicit operator", "").Replace("explicit operator", "");, but it looks a bit ugly. I'm certainly open to any other ideas.

Test plan: #52221

…erface operator

src/EditorFeatures/TestUtilities/Completion/AbstractCompletionProviderTests.cs

333fred · 2021-06-08T22:26:47Z

src/Compilers/Core/Portable/SymbolDisplay/SymbolDisplayMiscellaneousOptions.cs

+        /// ]]></code>
+        /// instead of:
+        /// <code><![CDATA[
+        /// static implicit I2<Test2>.implicit operator int(Test2 x)


I don't see why we would ever format this text. That just feels like a bug.

i am a little confused how symboldisplay plays into this scenario. i would not expect completion to be using symboldisplay here. that feels like a core bug on its own to me.

roslyn/src/Features/CSharp/Portable/Completion/CompletionProviders/ExplicitInterfaceMemberCompletionProvider.cs

Line 101 in bcd1dab

var memberString = member.ToMinimalDisplayString(semanticModel, namePosition, s_signatureDisplayFormat);

:(

@CyrusNajmabadi @333fred Any suggestion on how to best approach this?

kill me

replace that code with code that just builds the right syntax for what we're generating. it seems not that hard right? it's just member name, type parameters, parameters, constraints.

@CyrusNajmabadi

replace that code with code that just builds the right syntax for what we're generating

I did that. The code isn't large as I was initially expecting. The only risk I see with this approach is regressing some edge cases, but I'll add more tests.

.../CSharp/Portable/Completion/CompletionProviders/ExplicitInterfaceMemberCompletionProvider.cs

...ion/CompletionProviders/ExplicitInterfaceMemberCompletionProvider.CompletionSymbolDisplay.cs

CyrusNajmabadi · 2021-06-21T19:17:04Z

...ion/CompletionProviders/ExplicitInterfaceMemberCompletionProvider.CompletionSymbolDisplay.cs

+                    }
+
+                    first = false;
+                    AddType(parameter.Type, builder);


any concern about ref-kind?

@CyrusNajmabadi Yes. This was a bug. Fixed and added a test.

any need to add params?

any need to add constraints? i know it's an explicit interface, so normally no. but is there that funky case where we need to know if T? is unconstrained or not?

any need to add params?

Yes it definitely should be added. It was a working scenario. Fixed in latest commit.

any need to add constraints? i know it's an explicit interface, so normally no. but is there that funky case where we need to know if T? is unconstrained or not?

This is a scenario that doesn't work well on latest preview already.

The existing behavior is: no ? is added, and no "default" constraint is added.

I'll probably just add the ? for this PR and leave any potential improvements to address separately.

...ion/CompletionProviders/ExplicitInterfaceMemberCompletionProvider.CompletionSymbolDisplay.cs

CyrusNajmabadi

Request changes
Submit feedback that must be addressed before merging.

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

…lay.cs

Youssef1313 · 2022-01-17T17:27:01Z

@CyrusNajmabadi Can you review please? Thanks!

Youssef1313 · 2022-01-19T11:22:00Z

Ping @CyrusNajmabadi
Can you take a look please? Thanks!

CyrusNajmabadi · 2022-01-19T19:35:08Z

...ion/CompletionProviders/ExplicitInterfaceMemberCompletionProvider.CompletionSymbolDisplay.cs

+
+                        first = false;
+                        builder.Append(typeArgument.Name);
+                    }


i would be 100% ok with a stringbuilder extension that takes a TEnumerable and a separator and does this logic :)

@CyrusNajmabadi A private extension just for this class, or somewhere else for wider usage?

somewhere else :)

CyrusNajmabadi · 2022-01-19T20:49:05Z

I have a few suggestions. If you do not want to take them, just LMK.

Youssef1313 · 2022-01-20T21:24:32Z

@CyrusNajmabadi Thanks. I fixed those you pointed to in addition to other few bugs.

Youssef1313 · 2022-01-20T21:41:18Z

@CyrusNajmabadi This is ready for another look

...ion/CompletionProviders/ExplicitInterfaceMemberCompletionProvider.CompletionSymbolDisplay.cs

CyrusNajmabadi · 2022-01-20T22:12:57Z

Thanks!

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

Youssef1313 · 2022-01-21T08:00:15Z

@CyrusNajmabadi Auto-merge was disabled as I pushed commits. Can you re-enable? Thanks!

CyrusNajmabadi · 2022-01-21T18:42:45Z

Thanks!

don't include implicit/explicit keywords when completing explicit int…

44c7684

…erface operator

Youssef1313 requested review from a team as code owners June 8, 2021 09:52

dotnet-issue-labeler bot added the Area-Compilers label Jun 8, 2021

Youssef1313 commented Jun 8, 2021

View reviewed changes

src/EditorFeatures/TestUtilities/Completion/AbstractCompletionProviderTests.cs Show resolved Hide resolved

333fred reviewed Jun 8, 2021

View reviewed changes

Revert compiler changes

ca184bb

Youssef1313 marked this pull request as draft June 9, 2021 07:15

Youssef1313 force-pushed the explicit-interface-impl-completion branch from 55dfaf7 to cd11a40 Compare June 9, 2021 07:42

Partial implementation

0b92d20

Youssef1313 force-pushed the explicit-interface-impl-completion branch from cd11a40 to 0b92d20 Compare June 9, 2021 07:53

Youssef1313 commented Jun 9, 2021

View reviewed changes

.../CSharp/Portable/Completion/CompletionProviders/ExplicitInterfaceMemberCompletionProvider.cs Outdated Show resolved Hide resolved

Youssef1313 added 3 commits June 9, 2021 10:07

Small more work

278864e

Final fixes

a6c5d52

Move to new file

429cff9

Youssef1313 requested review from CyrusNajmabadi and 333fred and removed request for a team June 9, 2021 08:40

Youssef1313 added 2 commits June 9, 2021 10:47

Remove empty line

a418a74

Add more tests

493c72d

Youssef1313 marked this pull request as ready for review June 9, 2021 09:09

Fix test

d731f8f

jcouv added Area-IDE New Feature - StaticAbstractMembersInInterfaces and removed Area-Compilers labels Jun 10, 2021

CyrusNajmabadi reviewed Jun 21, 2021

View reviewed changes

...ion/CompletionProviders/ExplicitInterfaceMemberCompletionProvider.CompletionSymbolDisplay.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jun 21, 2021

View reviewed changes

...ion/CompletionProviders/ExplicitInterfaceMemberCompletionProvider.CompletionSymbolDisplay.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jun 21, 2021

View reviewed changes

CyrusNajmabadi approved these changes Jun 21, 2021

View reviewed changes

Youssef1313 changed the base branch from features/StaticAbstractMembersInInterfaces to main June 21, 2021 19:25

CyrusNajmabadi reviewed Jul 19, 2021

View reviewed changes

...ion/CompletionProviders/ExplicitInterfaceMemberCompletionProvider.CompletionSymbolDisplay.cs Outdated Show resolved Hide resolved

CyrusNajmabadi requested changes Jul 19, 2021

View reviewed changes

Youssef1313 and others added 2 commits July 19, 2021 20:22

Apply suggestions from code review

80b60fb

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

Fix build error

4eaeffa

Youssef1313 requested a review from CyrusNajmabadi January 4, 2022 13:45

Update ExplicitInterfaceMemberCompletionProvider.CompletionSymbolDisp…

91543a1

…lay.cs

CyrusNajmabadi reviewed Jan 19, 2022

View reviewed changes

Youssef1313 added 2 commits January 20, 2022 23:06

Few fixes

586b8d8

Escape type arguments

fb38eab

Add extension method

689cc1f

Youssef1313 requested a review from CyrusNajmabadi January 20, 2022 21:41

CyrusNajmabadi reviewed Jan 20, 2022

View reviewed changes

...ion/CompletionProviders/ExplicitInterfaceMemberCompletionProvider.CompletionSymbolDisplay.cs Outdated Show resolved Hide resolved

CyrusNajmabadi approved these changes Jan 20, 2022

View reviewed changes

CyrusNajmabadi enabled auto-merge January 20, 2022 22:12

auto-merge was automatically disabled January 20, 2022 22:20
Head branch was pushed to by a user without write access

Youssef1313 and others added 2 commits January 21, 2022 00:20

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

4f98a2a

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

Add missing header

4055a8c

CyrusNajmabadi merged commit ae23cba into dotnet:main Jan 21, 2022

ghost added this to the Next milestone Jan 21, 2022

Youssef1313 deleted the explicit-interface-impl-completion branch January 21, 2022 18:48

RikkiGibson modified the milestones: Next, 17.2.P1 Feb 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't include implicit/explicit keywords when completing explicit interface operator #53938

don't include implicit/explicit keywords when completing explicit interface operator #53938

Youssef1313 commented Jun 8, 2021 •

edited by jcouv

333fred Jun 8, 2021

CyrusNajmabadi Jun 8, 2021

333fred Jun 8, 2021

Youssef1313 Jun 9, 2021

CyrusNajmabadi Jun 9, 2021

Youssef1313 Jun 9, 2021

CyrusNajmabadi Jun 21, 2021

Youssef1313 Jun 23, 2021

CyrusNajmabadi Jan 19, 2022

CyrusNajmabadi Jan 19, 2022

Youssef1313 Jan 20, 2022

CyrusNajmabadi left a comment

Youssef1313 commented Jan 17, 2022

Youssef1313 commented Jan 19, 2022

CyrusNajmabadi Jan 19, 2022

Youssef1313 Jan 20, 2022

CyrusNajmabadi Jan 20, 2022

CyrusNajmabadi commented Jan 19, 2022

Youssef1313 commented Jan 20, 2022

Youssef1313 commented Jan 20, 2022

CyrusNajmabadi commented Jan 20, 2022

Youssef1313 commented Jan 21, 2022

CyrusNajmabadi commented Jan 21, 2022

don't include implicit/explicit keywords when completing explicit interface operator #53938

don't include implicit/explicit keywords when completing explicit interface operator #53938

Conversation

Youssef1313 commented Jun 8, 2021 • edited by jcouv

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Youssef1313 commented Jan 17, 2022

Youssef1313 commented Jan 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jan 19, 2022

Youssef1313 commented Jan 20, 2022

Youssef1313 commented Jan 20, 2022

CyrusNajmabadi commented Jan 20, 2022

Youssef1313 commented Jan 21, 2022

CyrusNajmabadi commented Jan 21, 2022

Youssef1313 commented Jun 8, 2021 •

edited by jcouv