Adjust implement interface service to work with static abstract operators #53941

Youssef1313 · 2021-06-08T11:41:25Z

Fixes #53927
Fixes #53925
Fixes #54328
Fixes #54327

…tors

...ures/Core/Portable/ImplementInterface/AbstractImplementInterfaceService.CodeAction_Method.cs

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

Youssef1313 · 2021-06-09T14:01:58Z

@CyrusNajmabadi This is ready for another look :)

CyrusNajmabadi · 2021-06-09T14:16:25Z

src/EditorFeatures/CSharpTest/ImplementInterface/ImplementInterfaceTests.cs

+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsImplementInterface)]
+        public async Task TestStaticAbstractInterfaceOperator_OnlyExplicitlyImplementable()
+        {
+            await TestInRegularAndScriptAsync(@"


in the future, if your'e interested, it would be good to update these to be sdk style tests. that way we can validate if our fix produces errors :)

📝 I'm working on this change for ImplementInterfaceTests in a separate branch.

CyrusNajmabadi · 2021-06-09T14:17:12Z

src/EditorFeatures/CSharpTest/ImplementInterface/ImplementInterfaceTests.cs

+
+abstract class C : ITest<C>
+{
+    public abstract static int operator -(C x);


i'm curious. is this legal?

Abstract statics are allowed only in interfaces

Thanks for noting @CyrusNajmabadi @AlekseyTs
I'm curious whether the "Implement interface abstractly" should do nothing for the operator, or generate it without the "abstract" keyword?

implement abstractly needs to take the code from uncompilable state to compilable, attempting for each member it needs to generate to make that abstract. If the member doesn't need to be generated, we should not generate it.

@CyrusNajmabadi The member needs to be generated to be in a compilable state, but it cannot be generated abstractly.

I do not believe there is away to implement a static member abstractly. So, if there are static abstract members in the interface, the option probably shouldn't be offered.

The member needs to be generated to be in a compilable state, but it cannot be generated abstractly.

'GEnerate abstractly' means 'generate abstractly if possible'. If not possible, generate legally. This already occurs today where normal instance methods might cause errors if generated abstractly. In that case i believe we normally just generate explicitly.

End goal of this feature is always compilable code. The different user facing choices are about how we should generate when we have such flexibility while still ensuring compilable. If we are constrained, then we generate in whatever manner the constraints force.

src/Workspaces/CSharp/Portable/CodeGeneration/OperatorGenerator.cs

...rkspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/INamedTypeSymbolExtensions.cs

CyrusNajmabadi

Overall this looks good. Just some tiny questions/cleanup/requests.

…InInterfaces' into impl-interface-operators

AlekseyTs · 2021-06-17T14:22:15Z

Note that https://github.com/dotnet/roslyn/tree/features/StaticAbstractMembersInInterfaces has been merged to main, all related PRs should target main at this point.

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

src/Features/Core/Portable/ImplementInterface/AbstractImplementInterfaceService.CodeAction.cs

@@ -309,7 +309,7 @@ private string DetermineMemberName(ISymbol member, ArrayBuilder<ISymbol> impleme
                var generateInvisibleMember = GenerateInvisibleMember(member, memberName);
                memberName = generateInvisibleMember ? member.Name : memberName;

-                var generateAbstractly = !generateInvisibleMember && Abstractly;
+                var generateAbstractly = !member.IsStatic && !generateInvisibleMember && Abstractly;


possibly explicitly comment that the language doesn't allow static abstract impls of interface methods.

Added comment.

CyrusNajmabadi · 2021-06-21T19:14:56Z

src/Workspaces/CSharp/Portable/CodeGeneration/OperatorGenerator.cs

-            return SyntaxFactory.TokenList(
-                SyntaxFactory.Token(SyntaxKind.PublicKeyword),
-                SyntaxFactory.Token(SyntaxKind.StaticKeyword));
+            var tokens = ArrayBuilder<SyntaxToken>.GetInstance();


Suggested change

var tokens = ArrayBuilder<SyntaxToken>.GetInstance();

using var tokens = TemporaryArray<SyntaxToken>.Empty;

TemporaryArray is nice when we expect 4 or less items commonly :)

Nice to know about TemporaryArray! Thanks. I applied the change.

Youssef1313 · 2021-06-23T10:27:25Z

CI failure is unrelated and was fixed in main. Closing and reopening for new build.

CyrusNajmabadi · 2021-06-29T19:24:11Z

Thanks!

Adjust implement interface service to work with static abstract opera…

9f0ba2f

…tors

Youssef1313 requested a review from a team as a code owner June 8, 2021 11:41

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

Youssef1313 marked this pull request as draft June 8, 2021 11:51

CyrusNajmabadi reviewed Jun 8, 2021

View reviewed changes

...ures/Core/Portable/ImplementInterface/AbstractImplementInterfaceService.CodeAction_Method.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jun 8, 2021

View reviewed changes

...ures/Core/Portable/ImplementInterface/AbstractImplementInterfaceService.CodeAction_Method.cs Outdated Show resolved Hide resolved

Fix typo

9ce92bd

CyrusNajmabadi reviewed Jun 8, 2021

View reviewed changes

...ures/Core/Portable/ImplementInterface/AbstractImplementInterfaceService.CodeAction_Method.cs Outdated Show resolved Hide resolved

Youssef1313 and others added 2 commits June 8, 2021 16:08

Update src/Features/Core/Portable/ImplementInterface/AbstractImplemen…

3ac631d

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

Fix implementation

d9bd43b

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

CyrusNajmabadi reviewed Jun 9, 2021

View reviewed changes

src/Workspaces/CSharp/Portable/CodeGeneration/OperatorGenerator.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jun 9, 2021

View reviewed changes

...rkspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/INamedTypeSymbolExtensions.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jun 9, 2021

View reviewed changes

...rkspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/INamedTypeSymbolExtensions.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Jun 9, 2021

View reviewed changes

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

jinujoseph assigned CyrusNajmabadi Jun 9, 2021

add tests and minor fixes

38a4605

Youssef1313 mentioned this pull request Jun 17, 2021

Implement interface fixer doesn't spit out static modifier implementations of static members #53925

Closed

Youssef1313 added 3 commits June 17, 2021 13:53

Merge remote-tracking branch 'upstream/features/StaticAbstractMembers…

6fece0f

…InInterfaces' into impl-interface-operators

Fix test

f6def2b

Fixes

9c698bb

Youssef1313 marked this pull request as draft June 17, 2021 12:09

Youssef1313 marked this pull request as ready for review June 17, 2021 12:14

Youssef1313 added 5 commits June 17, 2021 17:11

Fix generate abstractly

c0c96c8

More test case

ce3d776

Clean

b23f164

Link spec

17b736e

Update CodeGenerationTests.Shared.cs

35d1be4

Youssef1313 requested review from sharwell, CyrusNajmabadi and AlekseyTs June 21, 2021 09:59

CyrusNajmabadi reviewed Jun 21, 2021

View reviewed changes

CyrusNajmabadi approved these changes Jun 21, 2021

View reviewed changes

Address feedback

7eca18a

Youssef1313 closed this Jun 23, 2021

Youssef1313 reopened this Jun 23, 2021

Youssef1313 mentioned this pull request Jun 23, 2021

Implement interface abstractly should not apply to static members #54328

Closed

Youssef1313 added 2 commits June 23, 2021 18:20

Fix conflicts

00364cb

Use FixedCode instead of FixedState when appropriate

7a83274

CyrusNajmabadi merged commit f881f21 into dotnet:main Jun 29, 2021

ghost added this to the Next milestone Jun 29, 2021

Youssef1313 deleted the impl-interface-operators branch June 29, 2021 19:48

RikkiGibson modified the milestones: Next, 17.0.P2 Jun 29, 2021

sharwell mentioned this pull request Dec 6, 2021

"Implement all members explicitly" action generates wrong code for static interface properties #58136

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust implement interface service to work with static abstract operators #53941

Adjust implement interface service to work with static abstract operators #53941

Youssef1313 commented Jun 8, 2021 •

edited

Loading

Youssef1313 commented Jun 9, 2021

CyrusNajmabadi Jun 9, 2021

sharwell Jun 15, 2021

CyrusNajmabadi Jun 9, 2021

AlekseyTs Jun 15, 2021

Youssef1313 Jun 17, 2021

CyrusNajmabadi Jun 17, 2021

Youssef1313 Jun 17, 2021

AlekseyTs Jun 17, 2021

CyrusNajmabadi Jun 17, 2021

CyrusNajmabadi left a comment

AlekseyTs commented Jun 17, 2021

CyrusNajmabadi Jun 21, 2021

Youssef1313 Jun 22, 2021

CyrusNajmabadi Jun 21, 2021

CyrusNajmabadi Jun 21, 2021

Youssef1313 Jun 22, 2021

Youssef1313 commented Jun 23, 2021

CyrusNajmabadi commented Jun 29, 2021

	var tokens = ArrayBuilder<SyntaxToken>.GetInstance();
	using var tokens = TemporaryArray<SyntaxToken>.Empty;

Adjust implement interface service to work with static abstract operators #53941

Adjust implement interface service to work with static abstract operators #53941

Conversation

Youssef1313 commented Jun 8, 2021 • edited Loading

Youssef1313 commented Jun 9, 2021

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

AlekseyTs commented Jun 17, 2021

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

Youssef1313 commented Jun 23, 2021

CyrusNajmabadi commented Jun 29, 2021

Youssef1313 commented Jun 8, 2021 •

edited

Loading