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

Update 'implement interface' to forward explicit members to implicit ones #76044

Merged
merged 10 commits into from
Nov 26, 2024
Merged

Update 'implement interface' to forward explicit members to implicit ones #76044

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ba0cc81
Update 'implement interface' to forward explicit members to implicit …
CyrusNajmabadi Nov 22, 2024
7784412
Add tests
CyrusNajmabadi Nov 22, 2024
2f20031
Fix
CyrusNajmabadi Nov 22, 2024
2e13dfe
Fix
CyrusNajmabadi Nov 23, 2024
5b2a04a
Merge remote-tracking branch 'upstream/main' into implementInterfaceF…
CyrusNajmabadi Nov 23, 2024
a0ed252
lint
CyrusNajmabadi Nov 23, 2024
0052955
Fix tests
CyrusNajmabadi Nov 23, 2024
029bcfa
Merge remote-tracking branch 'upstream/main' into implementInterfaceF…
CyrusNajmabadi Nov 25, 2024
3f80de7
Simpliufy
CyrusNajmabadi Nov 25, 2024
32b4614
Update tests
CyrusNajmabadi Nov 25, 2024
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
177 changes: 165 additions & 12 deletions src/Analyzers/CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Diagnostics.CodeAnalysis;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.CodeStyle;
Expand All @@ -21,12 +22,12 @@ namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.ImplementInterface;
CSharpImplementInterfaceCodeFixProvider>;

[Trait(Traits.Feature, Traits.Features.CodeActionsImplementInterface)]
public class ImplementInterfaceTests
public sealed class ImplementInterfaceTests
{
private readonly NamingStylesTestOptionSets _options = new NamingStylesTestOptionSets(LanguageNames.CSharp);

private static OptionsCollection AllOptionsOff
=> new OptionsCollection(LanguageNames.CSharp)
=> new(LanguageNames.CSharp)
{
{ CSharpCodeStyleOptions.PreferExpressionBodiedMethods, CSharpCodeStyleOptions.NeverWithSilentEnforcement },
{ CSharpCodeStyleOptions.PreferExpressionBodiedConstructors, CSharpCodeStyleOptions.NeverWithSilentEnforcement },
Expand All @@ -37,7 +38,7 @@ private static OptionsCollection AllOptionsOff
};

private static OptionsCollection AllOptionsOn
=> new OptionsCollection(LanguageNames.CSharp)
=> new(LanguageNames.CSharp)
{
{ CSharpCodeStyleOptions.PreferExpressionBodiedMethods, CSharpCodeStyleOptions.WhenPossibleWithSilentEnforcement },
{ CSharpCodeStyleOptions.PreferExpressionBodiedConstructors, CSharpCodeStyleOptions.WhenPossibleWithSilentEnforcement },
Expand All @@ -48,7 +49,7 @@ private static OptionsCollection AllOptionsOn
};

private static OptionsCollection AccessorOptionsOn
=> new OptionsCollection(LanguageNames.CSharp)
=> new(LanguageNames.CSharp)
{
{ CSharpCodeStyleOptions.PreferExpressionBodiedMethods, CSharpCodeStyleOptions.NeverWithSilentEnforcement },
{ CSharpCodeStyleOptions.PreferExpressionBodiedConstructors, CSharpCodeStyleOptions.NeverWithSilentEnforcement },
Expand All @@ -59,7 +60,8 @@ private static OptionsCollection AccessorOptionsOn
};

internal static async Task TestWithAllCodeStyleOptionsOffAsync(
string initialMarkup, string expectedMarkup,
[StringSyntax(PredefinedEmbeddedLanguageNames.CSharpTest)] string initialMarkup,
[StringSyntax(PredefinedEmbeddedLanguageNames.CSharpTest)] string expectedMarkup,
(string equivalenceKey, int index)? codeAction = null)
{
await new VerifyCS.Test
Expand All @@ -73,7 +75,9 @@ internal static async Task TestWithAllCodeStyleOptionsOffAsync(
}.RunAsync();
}

internal static async Task TestWithAllCodeStyleOptionsOnAsync(string initialMarkup, string expectedMarkup)
internal static async Task TestWithAllCodeStyleOptionsOnAsync(
[StringSyntax(PredefinedEmbeddedLanguageNames.CSharpTest)] string initialMarkup,
[StringSyntax(PredefinedEmbeddedLanguageNames.CSharpTest)] string expectedMarkup)
{
await new VerifyCS.Test
{
Expand All @@ -83,7 +87,9 @@ internal static async Task TestWithAllCodeStyleOptionsOnAsync(string initialMark
}.RunAsync();
}

internal static async Task TestWithAccessorCodeStyleOptionsOnAsync(string initialMarkup, string expectedMarkup)
internal static async Task TestWithAccessorCodeStyleOptionsOnAsync(
[StringSyntax(PredefinedEmbeddedLanguageNames.CSharpTest)] string initialMarkup,
[StringSyntax(PredefinedEmbeddedLanguageNames.CSharpTest)] string expectedMarkup)
{
await new VerifyCS.Test
{
Expand All @@ -94,8 +100,8 @@ internal static async Task TestWithAccessorCodeStyleOptionsOnAsync(string initia
}

private static async Task TestInRegularAndScriptAsync(
string initialMarkup,
string expectedMarkup,
[StringSyntax(PredefinedEmbeddedLanguageNames.CSharpTest)] string initialMarkup,
[StringSyntax(PredefinedEmbeddedLanguageNames.CSharpTest)] string expectedMarkup,
(string equivalenceKey, int index)? codeAction = null)
{
await new VerifyCS.Test
Expand Down Expand Up @@ -1443,12 +1449,12 @@ int I1.Prop
{
get
{
throw new System.NotImplementedException();
return Prop;
}

set
{
throw new System.NotImplementedException();
Prop = value;
}
}
}
Expand All @@ -1460,6 +1466,35 @@ interface I1
""");
}

[Fact]
public async Task TestConflictingProperties2()
{
await TestWithAllCodeStyleOptionsOnAsync(
"""
class Test : {|CS0737:I1|}
{
int Prop { get; set; }
}

interface I1
{
int Prop { get; set; }
}
""",
"""
class Test : I1
{
int Prop { get; set; }
int I1.Prop { get => Prop; set => Prop = value; }
}

interface I1
{
int Prop { get; set; }
}
""");
}

[Fact, WorkItem("http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/539043")]
public async Task TestExplicitProperties()
{
Expand Down Expand Up @@ -11895,7 +11930,7 @@ public IEnumerator<A> GetEnumerator()

IEnumerator IEnumerable.GetEnumerator()
{
throw new System.NotImplementedException();
return GetEnumerator();
}
}
""",
Expand Down Expand Up @@ -11976,4 +12011,122 @@ static explicit I11<C11>.operator int(C11 x)
CodeActionVerifier = (codeAction, verifier) => verifier.Equal(CodeFixesResources.Implement_all_members_explicitly, codeAction.Title),
}.RunAsync();
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/67023")]
public async Task TestIEnumerable1()
{
await TestWithAllCodeStyleOptionsOffAsync(
"""
using System;
using System.Collections;
using System.Collections.Generic;

class Class : {|CS0535:{|CS0535:IEnumerable<int>|}|}
{
}
""",
"""
using System;
using System.Collections;
using System.Collections.Generic;

class Class : IEnumerable<int>
{
public IEnumerator<int> GetEnumerator()
{
throw new NotImplementedException();
}

IEnumerator IEnumerable.GetEnumerator()
{
return GetEnumerator();
}
}
""");
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/67023")]
public async Task TestIEnumerable2()
{
await TestWithAllCodeStyleOptionsOffAsync(
"""
using System;
using System.Collections;
using System.Collections.Generic;

class Class : {|CS0535:{|CS0535:{|CS0535:{|CS0535:{|CS0535:IEnumerator<int>|}|}|}|}|}
{
}
""",
"""
using System;
using System.Collections;
using System.Collections.Generic;

class Class : IEnumerator<int>
{
public int Current
{
get
{
throw new NotImplementedException();
}
}

object IEnumerator.Current
{
get
{
return Current;
Copy link
Contributor

Choose a reason for hiding this comment

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

return Current;

Is this desired? The boxing might not be necessary depending on how the Current method is implemented.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is "fine". it's the legacy apis calling to the modern ones.

}
}

public void Dispose()
{
throw new NotImplementedException();
}

public bool MoveNext()
{
throw new NotImplementedException();
}

public void Reset()
{
throw new NotImplementedException();
}
}
""");
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/67023")]
public async Task TestIEnumerable3()
{
await TestWithAllCodeStyleOptionsOnAsync(
"""
using System;
using System.Collections;
using System.Collections.Generic;

class Class : {|CS0535:{|CS0535:{|CS0535:{|CS0535:{|CS0535:IEnumerator<int>|}|}|}|}|}
{
}
""",
"""
using System;
using System.Collections;
using System.Collections.Generic;

class Class : IEnumerator<int>
{
public int Current => throw new NotImplementedException();

object IEnumerator.Current => Current;

public void Dispose() => throw new NotImplementedException();
public bool MoveNext() => throw new NotImplementedException();
public void Reset() => throw new NotImplementedException();
}
""");
}
}
4 changes: 2 additions & 2 deletions src/Analyzers/Core/CodeFixes/ImplementAbstractClass/ImplementAbstractClassData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ private IPropertySymbol GenerateProperty(
attributes: default,
accessibility: property.GetMethod.ComputeResultantAccessibility(ClassType),
statements: generator.GetGetAccessorStatements(
compilation, property, throughMember, preferAutoProperties))
compilation, property, conflictingProperty: null, throughMember, preferAutoProperties))
: null;

var setMethod = ShouldGenerateAccessor(property.SetMethod)
Expand All @@ -224,7 +224,7 @@ private IPropertySymbol GenerateProperty(
attributes: default,
accessibility: property.SetMethod.ComputeResultantAccessibility(ClassType),
statements: generator.GetSetAccessorStatements(
compilation, property, throughMember, preferAutoProperties))
compilation, property, conflictingProperty: null, throughMember, preferAutoProperties))
: null;

return CodeGenerationSymbolFactory.CreatePropertySymbol(
Expand Down
17 changes: 11 additions & 6 deletions src/Analyzers/Core/CodeFixes/ImplementInterface/ImplementInterfaceGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,9 +157,12 @@ private bool IsReservedName(string name)
State.ClassOrStructType.TypeParameters.Any(static (t, arg) => arg.self.IdentifiersMatch(t.Name, arg.name), (self: this, name));
}

private string DetermineMemberName(ISymbol member, ArrayBuilder<ISymbol> implementedVisibleMembers)
private string DetermineMemberName(ISymbol member, ArrayBuilder<ISymbol> implementedVisibleMembers, out ISymbol? conflictingMember)
{
if (HasConflictingMember(member, implementedVisibleMembers))
conflictingMember = null;

if (IsReservedName(member.Name) ||
HasConflictingMember(member, implementedVisibleMembers, out conflictingMember))
{
var memberNames = State.ClassOrStructType.GetAccessibleMembersInThisAndBaseTypes<ISymbol>(State.ClassOrStructType).Select(m => m.Name);

Expand Down Expand Up @@ -194,7 +197,7 @@ private string DetermineMemberName(ISymbol member, ArrayBuilder<ISymbol> impleme
if (HasMatchingMember(implementedVisibleMembers, member))
return [];

var memberName = DetermineMemberName(member, implementedVisibleMembers);
var memberName = DetermineMemberName(member, implementedVisibleMembers, out var conflictingMember);

// See if we need to generate an invisible member. If we do, then reset the name
// back to what then member wants it to be.
Expand All @@ -215,7 +218,7 @@ private string DetermineMemberName(ISymbol member, ArrayBuilder<ISymbol> impleme
var addUnsafe = member.RequiresUnsafeModifier() && !syntaxFacts.IsUnsafeContext(State.InterfaceNode);

return GenerateMembers(
compilation, member, memberName, generateInvisibleMember, generateAbstractly,
compilation, member, conflictingMember, memberName, generateInvisibleMember, generateAbstractly,
addNew, addUnsafe, propertyGenerationBehavior);
}

Expand Down Expand Up @@ -277,6 +280,7 @@ private static bool IsUnexpressibleTypeParameter(
private IEnumerable<ISymbol?> GenerateMembers(
Compilation compilation,
ISymbol member,
ISymbol? conflictingMember,
string memberName,
bool generateInvisibly,
bool generateAbstractly,
Expand All @@ -294,11 +298,12 @@ private static bool IsUnexpressibleTypeParameter(

if (member is IMethodSymbol method)
{
yield return GenerateMethod(compilation, method, accessibility, modifiers, generateAbstractly, useExplicitInterfaceSymbol, memberName);
yield return GenerateMethod(
compilation, method, conflictingMember as IMethodSymbol, accessibility, modifiers, generateAbstractly, useExplicitInterfaceSymbol, memberName);
}
else if (member is IPropertySymbol property)
{
foreach (var generated in GeneratePropertyMembers(compilation, property, accessibility, modifiers, generateAbstractly, useExplicitInterfaceSymbol, memberName, propertyGenerationBehavior))
foreach (var generated in GeneratePropertyMembers(compilation, property, conflictingMember as IPropertySymbol, accessibility, modifiers, generateAbstractly, useExplicitInterfaceSymbol, memberName, propertyGenerationBehavior))
yield return generated;
}
else if (member is IEventSymbol @event)
Expand Down
9 changes: 4 additions & 5 deletions src/Analyzers/Core/CodeFixes/ImplementInterface/ImplementInterfaceGenerator_Conflicts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Diagnostics.CodeAnalysis;
using System.Linq;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.PooledObjects;
Expand All @@ -21,18 +22,16 @@ internal abstract partial class AbstractImplementInterfaceService
{
private sealed partial class ImplementInterfaceGenerator
{
private bool HasConflictingMember(ISymbol member, ArrayBuilder<ISymbol> implementedVisibleMembers)
private bool HasConflictingMember(ISymbol member, ArrayBuilder<ISymbol> implementedVisibleMembers, [NotNullWhen(true)] out ISymbol? conflictingMember)
{
// Checks if this member conflicts with an existing member in classOrStructType or with
// a method we've already implemented. If so, we'll need to implement this one
// explicitly.

var allMembers = State.ClassOrStructType.GetAccessibleMembersInThisAndBaseTypes<ISymbol>(State.ClassOrStructType).Concat(implementedVisibleMembers);

var conflict1 = allMembers.Any(m => HasConflict(m, member));
var conflict2 = IsReservedName(member.Name);

return conflict1 || conflict2;
conflictingMember = allMembers.FirstOrDefault(m => HasConflict(m, member));
return conflictingMember != null;
}

private bool HasConflict(ISymbol member1, ISymbol member2)
Expand Down
Loading
Loading