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

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

Merged
merged 16 commits into from
Jun 29, 2021
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
9f0ba2f
Adjust implement interface service to work with static abstract opera…
Youssef1313 Jun 8, 2021
9ce92bd
Fix typo
Youssef1313 Jun 8, 2021
3ac631d
Update src/Features/Core/Portable/ImplementInterface/AbstractImplemen…
Youssef1313 Jun 8, 2021
d9bd43b
Fix implementation
Youssef1313 Jun 9, 2021
38a4605
add tests and minor fixes
Youssef1313 Jun 17, 2021
6fece0f
Merge remote-tracking branch 'upstream/features/StaticAbstractMembers…
Youssef1313 Jun 17, 2021
f6def2b
Fix test
Youssef1313 Jun 17, 2021
9c698bb
Fixes
Youssef1313 Jun 17, 2021
c0c96c8
Fix generate abstractly
Youssef1313 Jun 17, 2021
ce3d776
More test case
Youssef1313 Jun 17, 2021
b23f164
Clean
Youssef1313 Jun 17, 2021
17b736e
Link spec
Youssef1313 Jun 17, 2021
35d1be4
Update CodeGenerationTests.Shared.cs
Youssef1313 Jun 17, 2021
7eca18a
Address feedback
Youssef1313 Jun 22, 2021
00364cb
Fix conflicts
Youssef1313 Jun 23, 2021
7a83274
Use FixedCode instead of FixedState when appropriate
Youssef1313 Jun 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 117 additions & 0 deletions src/EditorFeatures/CSharpTest/ImplementInterface/ImplementInterfaceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8984,5 +8984,122 @@ public int Foo
}
", parseOptions: CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.CSharp5));
}

[WorkItem(53927, "https://github.com/dotnet/roslyn/issues/53927")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsImplementInterface)]
public async Task TestStaticAbstractInterfaceOperator_OnlyExplicitlyImplementable()
{
await TestInRegularAndScriptAsync(@"
Copy link
Member

Choose a reason for hiding this comment

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

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 :)

Copy link
Member

Choose a reason for hiding this comment

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

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

interface ITest
{
static abstract int operator -(ITest x);
}

class C : [|ITest|]
{
}
",
@"
interface ITest
{
static abstract int operator -(ITest x);
}

class C : ITest
{
static int ITest.operator -(ITest x)
{
throw new System.NotImplementedException();
}
}
", parseOptions: CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.Preview), index: 0, title: FeaturesResources.Implement_all_members_explicitly);
}

[WorkItem(53927, "https://github.com/dotnet/roslyn/issues/53927")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsImplementInterface)]
public async Task TestStaticAbstractInterfaceOperator_ImplementImplicitly()
{
await TestInRegularAndScriptAsync(@"
interface ITest<T> where T : ITest<T>
{
static abstract int operator -(T x);
}

class C : [|ITest<C>|]
{
}
",
@"
interface ITest<T> where T : ITest<T>
{
static abstract int operator -(T x);
}

class C : ITest<C>
{
public static int operator -(C x)
{
throw new System.NotImplementedException();
}
}
", parseOptions: CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.Preview), index: 0, title: FeaturesResources.Implement_interface);
}

[WorkItem(53927, "https://github.com/dotnet/roslyn/issues/53927")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsImplementInterface)]
public async Task TestStaticAbstractInterfaceOperator_ImplementExplicitly()
{
await TestInRegularAndScriptAsync(@"
interface ITest<T> where T : ITest<T>
{
static abstract int operator -(T x);
}

class C : [|ITest<C>|]
{
}
",
@"
interface ITest<T> where T : ITest<T>
{
static abstract int operator -(T x);
}

class C : ITest<C>
{
static int ITest<C>.operator -(C x)
{
throw new System.NotImplementedException();
}
}
", parseOptions: CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.Preview), index: 1, title: FeaturesResources.Implement_all_members_explicitly);
}

[WorkItem(53927, "https://github.com/dotnet/roslyn/issues/53927")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsImplementInterface)]
public async Task TestStaticAbstractInterfaceOperator_ImplementAbstractly()
{
await TestInRegularAndScriptAsync(@"
interface ITest<T> where T : ITest<T>
{
static abstract int operator -(T x);
}

abstract class C : [|ITest<C>|]
{
}
",
@"
interface ITest<T> where T : ITest<T>
{
static abstract int operator -(T x);
}

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

Choose a reason for hiding this comment

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

i'm curious. is this legal?

Copy link
Contributor

Choose a reason for hiding this comment

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

Abstract statics are allowed only in interfaces

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

}
", parseOptions: CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.Preview), index: 1, title: FeaturesResources.Implement_interface_abstractly);
}
}
}
4 changes: 1 addition & 3 deletions ...s/Core/Portable/ImplementInterface/AbstractImplementInterfaceService.CodeAction_Method.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#nullable disable

using System.Collections.Immutable;
using Microsoft.CodeAnalysis.CodeGeneration;
using Microsoft.CodeAnalysis.Editing;
Expand All @@ -25,7 +23,7 @@ private ISymbol GenerateMethod(
bool useExplicitInterfaceSymbol,
string memberName)
{
var syntaxFacts = Document.GetLanguageService<ISyntaxFactsService>();
var syntaxFacts = Document.GetRequiredLanguageService<ISyntaxFactsService>();

var updatedMethod = method.EnsureNonConflictingNames(State.ClassOrStructType, syntaxFacts);

Expand Down
28 changes: 21 additions & 7 deletions src/Workspaces/CSharp/Portable/CodeGeneration/OperatorGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
using Microsoft.CodeAnalysis.CSharp.CodeStyle;
using Microsoft.CodeAnalysis.CSharp.Extensions;
using Microsoft.CodeAnalysis.CSharp.Syntax;

using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Shared.Extensions;
using static Microsoft.CodeAnalysis.CodeGeneration.CodeGenerationHelpers;
using static Microsoft.CodeAnalysis.CSharp.CodeGeneration.CSharpCodeGenerationHelpers;

Expand Down Expand Up @@ -74,7 +75,7 @@ private static OperatorDeclarationSyntax GenerateOperatorDeclarationWorker(
CodeGenerationOptions options,
ParseOptions parseOptions)
{
var hasNoBody = !options.GenerateMethodBodies || method.IsExtern;
var hasNoBody = !options.GenerateMethodBodies || method.IsExtern || method.IsAbstract;

var operatorSyntaxKind = SyntaxFacts.GetOperatorKind(method.MetadataName);
if (operatorSyntaxKind == SyntaxKind.None)
Expand All @@ -86,23 +87,36 @@ private static OperatorDeclarationSyntax GenerateOperatorDeclarationWorker(

var operatorDecl = SyntaxFactory.OperatorDeclaration(
attributeLists: AttributeGenerator.GenerateAttributeLists(method.GetAttributes(), options),
modifiers: GenerateModifiers(),
modifiers: GenerateModifiers(method),
returnType: method.ReturnType.GenerateTypeSyntax(),
explicitInterfaceSpecifier: GenerateExplicitInterfaceSpecifier(method.ExplicitInterfaceImplementations),
operatorKeyword: SyntaxFactory.Token(SyntaxKind.OperatorKeyword),
operatorToken: operatorToken,
parameterList: ParameterGenerator.GenerateParameterList(method.Parameters, isExplicit: false, options: options),
body: hasNoBody ? null : StatementGenerator.GenerateBlock(method),
expressionBody: null,
semicolonToken: hasNoBody ? SyntaxFactory.Token(SyntaxKind.SemicolonToken) : new SyntaxToken());

operatorDecl = UseExpressionBodyIfDesired(options, operatorDecl, parseOptions);
return operatorDecl;
}

private static SyntaxTokenList GenerateModifiers()
private static SyntaxTokenList GenerateModifiers(IMethodSymbol method)
{
return SyntaxFactory.TokenList(
SyntaxFactory.Token(SyntaxKind.PublicKeyword),
SyntaxFactory.Token(SyntaxKind.StaticKeyword));
var tokens = ArrayBuilder<SyntaxToken>.GetInstance();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


if (method.ExplicitInterfaceImplementations.Length == 0)
{
tokens.Add(SyntaxFactory.Token(SyntaxKind.PublicKeyword));
}

if (method.IsAbstract)
{
tokens.Add(SyntaxFactory.Token(SyntaxKind.AbstractKeyword));
}

tokens.Add(SyntaxFactory.Token(SyntaxKind.StaticKeyword));
return tokens.ToSyntaxTokenListAndFree();
}
}
}
19 changes: 17 additions & 2 deletions ...paces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/INamedTypeSymbolExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,8 @@ static ImmutableArray<ISymbol> GetImplicitlyImplementableMembers(INamedTypeSymbo
{
return type.GetMembers().WhereAsArray(m => m.DeclaredAccessibility == Accessibility.Public &&
m.Kind != SymbolKind.NamedType && IsImplementable(m) &&
!IsPropertyWithNonPublicImplementableAccessor(m));
!IsPropertyWithNonPublicImplementableAccessor(m) &&
IsImplicitlyImplementable(m, within));
}

return type.GetMembers();
Expand All @@ -226,6 +227,20 @@ static bool IsNonPublicImplementableAccessor(IMethodSymbol? accessor)
{
return accessor != null && IsImplementable(accessor) && accessor.DeclaredAccessibility != Accessibility.Public;
}

static bool IsImplicitlyImplementable(ISymbol member, ISymbol within)
{
if (member is IMethodSymbol { IsStatic: true, IsAbstract: true } method)
{
// For example, the following is not implementable implicitly.
// interface I { static abstract int operator -(I x); }
// But the following is implementable:
// interface I<T> where T : I<T> { static abstract int operator -(T x); }
return method.Parameters.Any(p => p.Type.Equals(within, SymbolEqualityComparer.Default));
}

return true;
}
}

private static bool IsImplementable(ISymbol m)
Expand Down Expand Up @@ -396,7 +411,7 @@ private static ImmutableArray<ISymbol> GetUnimplementedMembers(
{
var q = from m in interfaceMemberGetter(interfaceType, classOrStructType)
where m.Kind != SymbolKind.NamedType
where m.Kind != SymbolKind.Method || ((IMethodSymbol)m).MethodKind == MethodKind.Ordinary
where m.Kind != SymbolKind.Method || ((IMethodSymbol)m).MethodKind is MethodKind.Ordinary or MethodKind.UserDefinedOperator
where m.Kind != SymbolKind.Property || ((IPropertySymbol)m).IsIndexer || ((IPropertySymbol)m).CanBeReferencedByName
where m.Kind != SymbolKind.Event || ((IEventSymbol)m).CanBeReferencedByName
where !isImplemented(classOrStructType, m, isValidImplementation, cancellationToken)
Expand Down