From ba0cc813e88595d653c795408313b73f2ba311c2 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 22 Nov 2024 15:45:32 -0800 Subject: [PATCH 1/8] Update 'implement interface' to forward explicit members to implicit ones --- .../ImplementInterfaceTests.cs | 146 ++++++++++++++++-- .../ImplementAbstractClassData.cs | 4 +- .../ImplementInterfaceGenerator.cs | 17 +- .../ImplementInterfaceGenerator_Conflicts.cs | 9 +- .../ImplementInterfaceGenerator_Method.cs | 55 ++++++- .../ImplementInterfaceGenerator_Property.cs | 19 ++- .../ImplementInterfaceTests.vb | 20 +-- .../CodeGeneration/PropertyGenerator.cs | 22 ++- .../CodeGenerationSymbolFactory.cs | 11 +- .../Extensions/SyntaxGeneratorExtensions.cs | 44 +++++- 10 files changed, 273 insertions(+), 74 deletions(-) diff --git a/src/Analyzers/CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs b/src/Analyzers/CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs index ef616c40e1044..393f4b351e78e 100644 --- a/src/Analyzers/CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs +++ b/src/Analyzers/CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs @@ -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.CodeActions; using Microsoft.CodeAnalysis.CSharp; @@ -22,12 +23,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 }, @@ -38,7 +39,7 @@ private static OptionsCollection AllOptionsOff }; private static OptionsCollection AllOptionsOn - => new OptionsCollection(LanguageNames.CSharp) + => new(LanguageNames.CSharp) { { CSharpCodeStyleOptions.PreferExpressionBodiedMethods, CSharpCodeStyleOptions.WhenPossibleWithSilentEnforcement }, { CSharpCodeStyleOptions.PreferExpressionBodiedConstructors, CSharpCodeStyleOptions.WhenPossibleWithSilentEnforcement }, @@ -49,7 +50,7 @@ private static OptionsCollection AllOptionsOn }; private static OptionsCollection AccessorOptionsOn - => new OptionsCollection(LanguageNames.CSharp) + => new(LanguageNames.CSharp) { { CSharpCodeStyleOptions.PreferExpressionBodiedMethods, CSharpCodeStyleOptions.NeverWithSilentEnforcement }, { CSharpCodeStyleOptions.PreferExpressionBodiedConstructors, CSharpCodeStyleOptions.NeverWithSilentEnforcement }, @@ -60,7 +61,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 @@ -74,7 +76,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 { @@ -84,7 +88,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 { @@ -95,8 +101,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 @@ -1444,7 +1450,7 @@ int I1.Prop { get { - throw new System.NotImplementedException(); + return Prop; } set @@ -11896,7 +11902,7 @@ public IEnumerator GetEnumerator() IEnumerator IEnumerable.GetEnumerator() { - throw new System.NotImplementedException(); + return GetEnumerator(); } } """, @@ -11933,4 +11939,122 @@ event System.EventHandler I.Click } }.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|}|} + { + } + """, + """ + using System; + using System.Collections; + using System.Collections.Generic; + + class Class : IEnumerable + { + public IEnumerator 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|}|}|}|}|} + { + } + """, + """ + using System; + using System.Collections; + using System.Collections.Generic; + + class Class : IEnumerator + { + public int Current + { + get + { + throw new NotImplementedException(); + } + } + + object IEnumerator.Current + { + get + { + return Current; + } + } + + 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|}|}|}|}|} + { + } + """, + """ + using System; + using System.Collections; + using System.Collections.Generic; + + class Class : IEnumerator + { + 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(); + } + """); + } } diff --git a/src/Analyzers/Core/CodeFixes/ImplementAbstractClass/ImplementAbstractClassData.cs b/src/Analyzers/Core/CodeFixes/ImplementAbstractClass/ImplementAbstractClassData.cs index b1e7588ff6615..69f72936d8ecb 100644 --- a/src/Analyzers/Core/CodeFixes/ImplementAbstractClass/ImplementAbstractClassData.cs +++ b/src/Analyzers/Core/CodeFixes/ImplementAbstractClass/ImplementAbstractClassData.cs @@ -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) @@ -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( diff --git a/src/Analyzers/Core/CodeFixes/ImplementInterface/ImplementInterfaceGenerator.cs b/src/Analyzers/Core/CodeFixes/ImplementInterface/ImplementInterfaceGenerator.cs index c950b2e54a02a..f6b5c50d4e79f 100644 --- a/src/Analyzers/Core/CodeFixes/ImplementInterface/ImplementInterfaceGenerator.cs +++ b/src/Analyzers/Core/CodeFixes/ImplementInterface/ImplementInterfaceGenerator.cs @@ -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 implementedVisibleMembers) + private string DetermineMemberName(ISymbol member, ArrayBuilder implementedVisibleMembers, out ISymbol? conflictingMember) { - if (HasConflictingMember(member, implementedVisibleMembers)) + conflictingMember = null; + + if (IsReservedName(member.Name) || + HasConflictingMember(member, implementedVisibleMembers, out conflictingMember)) { var memberNames = State.ClassOrStructType.GetAccessibleMembersInThisAndBaseTypes(State.ClassOrStructType).Select(m => m.Name); @@ -194,7 +197,7 @@ private string DetermineMemberName(ISymbol member, ArrayBuilder 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. @@ -215,7 +218,7 @@ private string DetermineMemberName(ISymbol member, ArrayBuilder 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); } @@ -277,6 +280,7 @@ private static bool IsUnexpressibleTypeParameter( private IEnumerable GenerateMembers( Compilation compilation, ISymbol member, + ISymbol? conflictingMember, string memberName, bool generateInvisibly, bool generateAbstractly, @@ -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) diff --git a/src/Analyzers/Core/CodeFixes/ImplementInterface/ImplementInterfaceGenerator_Conflicts.cs b/src/Analyzers/Core/CodeFixes/ImplementInterface/ImplementInterfaceGenerator_Conflicts.cs index fa6c44be8c163..0d3489a6fe31f 100644 --- a/src/Analyzers/Core/CodeFixes/ImplementInterface/ImplementInterfaceGenerator_Conflicts.cs +++ b/src/Analyzers/Core/CodeFixes/ImplementInterface/ImplementInterfaceGenerator_Conflicts.cs @@ -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; @@ -21,7 +22,7 @@ internal abstract partial class AbstractImplementInterfaceService { private sealed partial class ImplementInterfaceGenerator { - private bool HasConflictingMember(ISymbol member, ArrayBuilder implementedVisibleMembers) + private bool HasConflictingMember(ISymbol member, ArrayBuilder 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 @@ -29,10 +30,8 @@ private bool HasConflictingMember(ISymbol member, ArrayBuilder implemen var allMembers = State.ClassOrStructType.GetAccessibleMembersInThisAndBaseTypes(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) diff --git a/src/Analyzers/Core/CodeFixes/ImplementInterface/ImplementInterfaceGenerator_Method.cs b/src/Analyzers/Core/CodeFixes/ImplementInterface/ImplementInterfaceGenerator_Method.cs index 8ba8cf420dfa8..8ac028421aefd 100644 --- a/src/Analyzers/Core/CodeFixes/ImplementInterface/ImplementInterfaceGenerator_Method.cs +++ b/src/Analyzers/Core/CodeFixes/ImplementInterface/ImplementInterfaceGenerator_Method.cs @@ -2,6 +2,8 @@ // 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.CodeGeneration; using Microsoft.CodeAnalysis.Editing; using Microsoft.CodeAnalysis.LanguageService; @@ -22,6 +24,7 @@ private sealed partial class ImplementInterfaceGenerator private ISymbol GenerateMethod( Compilation compilation, IMethodSymbol method, + IMethodSymbol? conflictingMethod, Accessibility accessibility, DeclarationModifiers modifiers, bool generateAbstractly, @@ -42,17 +45,55 @@ private ISymbol GenerateMethod( modifiers: modifiers, explicitInterfaceImplementations: useExplicitInterfaceSymbol ? [updatedMethod] : default, name: memberName, - statements: generateAbstractly - ? default - : [CreateStatement(compilation, updatedMethod)]); + statements: generateAbstractly ? default : [GenerateStatement(compilation, updatedMethod, conflictingMethod)]); } - private SyntaxNode CreateStatement(Compilation compilation, IMethodSymbol method) + private SyntaxNode GenerateStatement( + Compilation compilation, IMethodSymbol updatedMethod, IMethodSymbol? conflictingMethod) { var factory = Document.GetRequiredLanguageService(); - return ThroughMember == null - ? factory.CreateThrowNotImplementedStatement(compilation) - : factory.GenerateDelegateThroughMemberStatement(method, ThroughMember); + if (ThroughMember != null) + return factory.GenerateDelegateThroughMemberStatement(updatedMethod, ThroughMember); + + // Forward from the explicit method we're creating to the existing method it conflicts with if possible. + if (CanForwardToConflictingMethod(compilation, updatedMethod, conflictingMethod)) + { + var invocation = factory.InvocationExpression( + factory.MemberAccessExpression( + factory.ThisExpression(), + conflictingMethod.Name), + factory.CreateArguments(updatedMethod.Parameters)); + + return updatedMethod.ReturnsVoid + ? factory.ExpressionStatement(invocation) + : factory.ReturnStatement(invocation); + } + + return factory.CreateThrowNotImplementedStatement(compilation); + } + + private static bool CanForwardToConflictingMethod( + Compilation compilation, IMethodSymbol method, [NotNullWhen(true)] IMethodSymbol? conflictingMethod) + { + if (conflictingMethod is null) + return false; + + if (method.Parameters.Length != conflictingMethod.Parameters.Length) + return false; + + if (method.ReturnsVoid != conflictingMethod.ReturnsVoid) + return false; + + if (method.Parameters.Zip(conflictingMethod.Parameters, (p1, p2) => (p1, p2)).Any( + t => compilation.ClassifyCommonConversion(t.p1.Type, t.p2.Type) is not { IsImplicit: true, Exists: true })) + { + return false; + } + + if (method.ReturnsVoid) + return true; + + return compilation.ClassifyCommonConversion(conflictingMethod.ReturnType, method.ReturnType) is { IsImplicit: true, Exists: true }; } } } diff --git a/src/Analyzers/Core/CodeFixes/ImplementInterface/ImplementInterfaceGenerator_Property.cs b/src/Analyzers/Core/CodeFixes/ImplementInterface/ImplementInterfaceGenerator_Property.cs index 7e6fc368666be..c2d10d30e9d36 100644 --- a/src/Analyzers/Core/CodeFixes/ImplementInterface/ImplementInterfaceGenerator_Property.cs +++ b/src/Analyzers/Core/CodeFixes/ImplementInterface/ImplementInterfaceGenerator_Property.cs @@ -28,6 +28,7 @@ private sealed partial class ImplementInterfaceGenerator private IEnumerable GeneratePropertyMembers( Compilation compilation, IPropertySymbol property, + IPropertySymbol? conflictingProperty, Accessibility accessibility, DeclarationModifiers modifiers, bool generateAbstractly, @@ -39,11 +40,11 @@ private sealed partial class ImplementInterfaceGenerator var attributesToRemove = AttributesToRemove(compilation); var getAccessor = GenerateGetAccessor( - compilation, property, accessibility, generateAbstractly, useExplicitInterfaceSymbol, + compilation, property, conflictingProperty, accessibility, generateAbstractly, useExplicitInterfaceSymbol, propertyGenerationBehavior, attributesToRemove); var setAccessor = GenerateSetAccessor( - compilation, property, accessibility, generateAbstractly, useExplicitInterfaceSymbol, + compilation, property, conflictingProperty, accessibility, generateAbstractly, useExplicitInterfaceSymbol, propertyGenerationBehavior, attributesToRemove); var syntaxFacts = Document.GetRequiredLanguageService(); @@ -90,6 +91,7 @@ private static INamedTypeSymbol[] AttributesToRemove(Compilation compilation) private IMethodSymbol? GenerateSetAccessor( Compilation compilation, IPropertySymbol property, + IPropertySymbol? conflictingProperty, Accessibility accessibility, bool generateAbstractly, bool useExplicitInterfaceSymbol, @@ -117,12 +119,13 @@ private static INamedTypeSymbol[] AttributesToRemove(Compilation compilation) accessibility: accessibility, explicitInterfaceImplementations: useExplicitInterfaceSymbol ? [property.SetMethod] : default, statements: GetSetAccessorStatements( - compilation, property, generateAbstractly, propertyGenerationBehavior)); + compilation, property, conflictingProperty, generateAbstractly, propertyGenerationBehavior)); } private IMethodSymbol? GenerateGetAccessor( Compilation compilation, IPropertySymbol property, + IPropertySymbol? conflictingProperty, Accessibility accessibility, bool generateAbstractly, bool useExplicitInterfaceSymbol, @@ -130,9 +133,7 @@ private static INamedTypeSymbol[] AttributesToRemove(Compilation compilation) INamedTypeSymbol[] attributesToRemove) { if (property.GetMethod == null) - { return null; - } var getMethod = property.GetMethod.RemoveInaccessibleAttributesAndAttributesOfTypes( State.ClassOrStructType, @@ -144,12 +145,13 @@ private static INamedTypeSymbol[] AttributesToRemove(Compilation compilation) accessibility: accessibility, explicitInterfaceImplementations: useExplicitInterfaceSymbol ? [property.GetMethod] : default, statements: GetGetAccessorStatements( - compilation, property, generateAbstractly, propertyGenerationBehavior)); + compilation, property, conflictingProperty, generateAbstractly, propertyGenerationBehavior)); } private ImmutableArray GetSetAccessorStatements( Compilation compilation, IPropertySymbol property, + IPropertySymbol? conflictingProperty, bool generateAbstractly, ImplementTypePropertyGenerationBehavior propertyGenerationBehavior) { @@ -157,13 +159,14 @@ private ImmutableArray GetSetAccessorStatements( return default; var generator = Document.GetRequiredLanguageService(); - return generator.GetSetAccessorStatements(compilation, property, ThroughMember, + return generator.GetSetAccessorStatements(compilation, property, conflictingProperty, ThroughMember, propertyGenerationBehavior == ImplementTypePropertyGenerationBehavior.PreferAutoProperties); } private ImmutableArray GetGetAccessorStatements( Compilation compilation, IPropertySymbol property, + IPropertySymbol? conflictingProperty, bool generateAbstractly, ImplementTypePropertyGenerationBehavior propertyGenerationBehavior) { @@ -171,7 +174,7 @@ private ImmutableArray GetGetAccessorStatements( return default; var generator = Document.GetRequiredLanguageService(); - return generator.GetGetAccessorStatements(compilation, property, ThroughMember, + return generator.GetGetAccessorStatements(compilation, property, conflictingProperty, ThroughMember, propertyGenerationBehavior == ImplementTypePropertyGenerationBehavior.PreferAutoProperties); } } diff --git a/src/Analyzers/VisualBasic/Tests/ImplementInterface/ImplementInterfaceTests.vb b/src/Analyzers/VisualBasic/Tests/ImplementInterface/ImplementInterfaceTests.vb index 8658148219004..d1d431a8250d3 100644 --- a/src/Analyzers/VisualBasic/Tests/ImplementInterface/ImplementInterfaceTests.vb +++ b/src/Analyzers/VisualBasic/Tests/ImplementInterface/ImplementInterfaceTests.vb @@ -109,7 +109,7 @@ Class C End Sub Private Sub IGoo_Bar() Implements IGoo.Bar - Throw New System.NotImplementedException() + Bar() End Sub End Class") End Function @@ -156,10 +156,10 @@ Class C Private Property I_M As Integer Implements I.M Get - Throw New System.NotImplementedException() + Return M End Get Set(value As Integer) - Throw New System.NotImplementedException() + M = value End Set End Property End Class") @@ -196,10 +196,10 @@ Class C Private Property I_M As Integer Implements I.M Get - Throw New System.NotImplementedException() + Return M End Get Set(value As Integer) - Throw New System.NotImplementedException() + M = value End Set End Property End Class") @@ -231,7 +231,7 @@ Class C Implements I Private Sub I_M() Implements I.M - Throw New System.NotImplementedException() + M() End Sub End Class") End Function @@ -1683,10 +1683,10 @@ Class C Private Property I1_Bar As Integer Implements I1.Bar Get - Throw New System.NotImplementedException() + Return Bar End Get Set(value As Integer) - Throw New System.NotImplementedException() + Bar = value End Set End Property End Class") @@ -1730,10 +1730,10 @@ Class C Private Property I1_Bar As Integer Implements I1.Bar Get - Throw New System.NotImplementedException() + Return Bar End Get Set(value As Integer) - Throw New System.NotImplementedException() + Bar = value End Set End Property diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/CodeGeneration/PropertyGenerator.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/CodeGeneration/PropertyGenerator.cs index fb5130ab4ee32..974e84987e529 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/CodeGeneration/PropertyGenerator.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/CodeGeneration/PropertyGenerator.cs @@ -107,7 +107,7 @@ private static MemberDeclarationSyntax GenerateIndexerDeclaration( AddAnnotationsTo(property, declaration)); } - private static MemberDeclarationSyntax GeneratePropertyDeclaration( + private static PropertyDeclarationSyntax GeneratePropertyDeclaration( IPropertySymbol property, CodeGenerationDestination destination, CSharpCodeGenerationContextInfo info, CancellationToken cancellationToken) { @@ -179,19 +179,15 @@ private static bool TryGetExpressionBody( private static PropertyDeclarationSyntax UseExpressionBodyIfDesired( CSharpCodeGenerationContextInfo info, PropertyDeclarationSyntax declaration, CancellationToken cancellationToken) { - if (declaration.ExpressionBody == null) + if (declaration.ExpressionBody == null && + declaration.Initializer == null && + TryGetExpressionBody( + declaration, info.LanguageVersion, info.Options.PreferExpressionBodiedProperties.Value, cancellationToken, + out var expressionBody, out var semicolonToken)) { - if (declaration.Initializer == null) - { - if (TryGetExpressionBody( - declaration, info.LanguageVersion, info.Options.PreferExpressionBodiedProperties.Value, cancellationToken, - out var expressionBody, out var semicolonToken)) - { - declaration = declaration.WithAccessorList(null) - .WithExpressionBody(expressionBody) - .WithSemicolonToken(semicolonToken); - } - } + declaration = declaration.WithAccessorList(null) + .WithExpressionBody(expressionBody) + .WithSemicolonToken(semicolonToken); } return declaration; diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/CodeGeneration/CodeGenerationSymbolFactory.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/CodeGeneration/CodeGenerationSymbolFactory.cs index c0e38b5abedbb..d6e7d4d702e6f 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/CodeGeneration/CodeGenerationSymbolFactory.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/CodeGeneration/CodeGenerationSymbolFactory.cs @@ -83,7 +83,7 @@ public static IPropertySymbol CreatePropertySymbol( ImmutableArray attributes, Accessibility accessibility, DeclarationModifiers modifiers, ITypeSymbol type, RefKind refKind, ImmutableArray explicitInterfaceImplementations, string name, ImmutableArray parameters, IMethodSymbol? getMethod, IMethodSymbol? setMethod, - bool isIndexer = false) + bool isIndexer = false, SyntaxNode? initializer = null) { return CreatePropertySymbol( containingType: null, @@ -97,7 +97,8 @@ public static IPropertySymbol CreatePropertySymbol( parameters: parameters, getMethod: getMethod, setMethod: setMethod, - isIndexer: isIndexer); + isIndexer: isIndexer, + initializer: initializer); } /// @@ -551,7 +552,8 @@ internal static IPropertySymbol CreatePropertySymbol( string? name = null, bool? isIndexer = null, IMethodSymbol? getMethod = null, - IMethodSymbol? setMethod = null) + IMethodSymbol? setMethod = null, + SyntaxNode? initializer = null) { return CreatePropertySymbol( attributes, @@ -564,7 +566,8 @@ internal static IPropertySymbol CreatePropertySymbol( parameters ?? property.Parameters, getMethod, setMethod, - isIndexer ?? property.IsIndexer); + isIndexer ?? property.IsIndexer, + initializer: initializer); } internal static IEventSymbol CreateEventSymbol( diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/Extensions/SyntaxGeneratorExtensions.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/Extensions/SyntaxGeneratorExtensions.cs index e1dd24c448120..ba65465f275c4 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/Extensions/SyntaxGeneratorExtensions.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/Extensions/SyntaxGeneratorExtensions.cs @@ -189,8 +189,12 @@ static SyntaxNode GenerateContainerName(SyntaxGenerator factory, ISymbol through } public static ImmutableArray GetGetAccessorStatements( - this SyntaxGenerator generator, Compilation compilation, - IPropertySymbol property, ISymbol? throughMember, bool preferAutoProperties) + this SyntaxGenerator generator, + Compilation compilation, + IPropertySymbol property, + IPropertySymbol? conflictingProperty, + ISymbol? throughMember, + bool preferAutoProperties) { if (throughMember != null) { @@ -209,12 +213,27 @@ public static ImmutableArray GetGetAccessorStatements( return [generator.ReturnStatement(expression)]; } - return preferAutoProperties ? default : generator.CreateThrowNotImplementedStatementBlock(compilation); + if (preferAutoProperties) + return default; + + // Forward from the explicit property we're creating to the existing property it conflicts with if possible. + if (conflictingProperty is { GetMethod: not null, Parameters.Length: 0 } && + property is { GetMethod: not null, Parameters.Length: 0 }) + { + if (compilation.ClassifyCommonConversion(conflictingProperty.Type, property.Type) is { Exists: true, IsImplicit: true }) + return [generator.ReturnStatement(generator.MemberAccessExpression(generator.ThisExpression(), property.Name))]; + } + + return generator.CreateThrowNotImplementedStatementBlock(compilation); } public static ImmutableArray GetSetAccessorStatements( - this SyntaxGenerator generator, Compilation compilation, - IPropertySymbol property, ISymbol? throughMember, bool preferAutoProperties) + this SyntaxGenerator generator, + Compilation compilation, + IPropertySymbol property, + IPropertySymbol? conflictingProperty, + ISymbol? throughMember, + bool preferAutoProperties) { if (throughMember != null) { @@ -235,9 +254,18 @@ public static ImmutableArray GetSetAccessorStatements( return [generator.ExpressionStatement(expression)]; } - return preferAutoProperties - ? default - : generator.CreateThrowNotImplementedStatementBlock(compilation); + if (preferAutoProperties) + return default; + + // Forward from the explicit property we're creating to the existing property it conflicts with if possible. + if (conflictingProperty is { SetMethod: not null, Parameters.Length: 0 } && + property is { SetMethod: not null, Parameters.Length: 0 }) + { + if (compilation.ClassifyCommonConversion(property.Type, conflictingProperty.Type) is { Exists: true, IsImplicit: true }) + return [generator.ExpressionStatement(generator.AssignmentStatement(generator.MemberAccessExpression(generator.ThisExpression(), property.Name), generator.IdentifierName("value")))]; + } + + return generator.CreateThrowNotImplementedStatementBlock(compilation); } private static bool TryGetValue(IDictionary? dictionary, string key, [NotNullWhen(true)] out string? value) From 7784412b69dc345392767a3b705fd52b9113bdfa Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 22 Nov 2024 15:48:51 -0800 Subject: [PATCH 2/8] Add tests --- .../ImplementInterfaceTests.cs | 37 ++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/src/Analyzers/CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs b/src/Analyzers/CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs index 393f4b351e78e..f228b8bc0c2ba 100644 --- a/src/Analyzers/CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs +++ b/src/Analyzers/CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs @@ -1455,7 +1455,7 @@ int I1.Prop set { - throw new System.NotImplementedException(); + Prop = value; } } } @@ -1467,6 +1467,41 @@ 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() { From 2f20031454e23dfa367abb0579bfe42ee10dcb72 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 22 Nov 2024 15:50:52 -0800 Subject: [PATCH 3/8] Fix git --- .../Tests/ImplementInterface/ImplementInterfaceTests.cs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/Analyzers/CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs b/src/Analyzers/CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs index f228b8bc0c2ba..91ad98879f369 100644 --- a/src/Analyzers/CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs +++ b/src/Analyzers/CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs @@ -1487,12 +1487,7 @@ class Test : I1 { int Prop { get; set; } - int I1.Prop - { - get => Prop; - - set => Prop = value; - } + int I1.Prop { get => Prop; set => Prop = value; } } interface I1 From 2e13dfe44605959e440627fffdbfe052fca343b9 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 22 Nov 2024 16:26:34 -0800 Subject: [PATCH 4/8] Fix git --- .../CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Analyzers/CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs b/src/Analyzers/CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs index 91ad98879f369..9a70157b01b48 100644 --- a/src/Analyzers/CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs +++ b/src/Analyzers/CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs @@ -1486,7 +1486,6 @@ interface I1 class Test : I1 { int Prop { get; set; } - int I1.Prop { get => Prop; set => Prop = value; } } From a0ed2527965a8e7f924ce9164cdc1bfa4680ceae Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 22 Nov 2024 19:07:19 -0800 Subject: [PATCH 5/8] lint --- .../CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Analyzers/CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs b/src/Analyzers/CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs index 3f07f6699e2bc..27e93dea536b0 100644 --- a/src/Analyzers/CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs +++ b/src/Analyzers/CSharp/Tests/ImplementInterface/ImplementInterfaceTests.cs @@ -12012,7 +12012,7 @@ static explicit I11.operator int(C11 x) }.RunAsync(); } - [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/67023")] + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/67023")] public async Task TestIEnumerable1() { await TestWithAllCodeStyleOptionsOffAsync( From 0052955e6dacbee57171b3010dc89bfa0db40a32 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 22 Nov 2024 19:14:58 -0800 Subject: [PATCH 6/8] Fix tests --- .../ImplementInterfaceCommandHandlerTests.vb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/EditorFeatures/VisualBasicTest/ImplementInterface/ImplementInterfaceCommandHandlerTests.vb b/src/EditorFeatures/VisualBasicTest/ImplementInterface/ImplementInterfaceCommandHandlerTests.vb index b763aeee6b18d..577cc4cda56e7 100644 --- a/src/EditorFeatures/VisualBasicTest/ImplementInterface/ImplementInterfaceCommandHandlerTests.vb +++ b/src/EditorFeatures/VisualBasicTest/ImplementInterface/ImplementInterfaceCommandHandlerTests.vb @@ -118,7 +118,7 @@ End Class End Sub Private Sub IBar_Goo() Implements IBar.Goo - Throw New NotImplementedException() + Goo() End Sub Test(code, @@ -484,7 +484,7 @@ Class C End Sub Private Sub IB_Goo() Implements IB.Goo - Throw New NotImplementedException() + goo() End Sub End Class From 3f80de7e59abb15e8cf6a7b3741e9399905c2bf0 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Mon, 25 Nov 2024 15:37:32 -0800 Subject: [PATCH 7/8] Simpliufy --- .../Workspace/Core/Extensions/SyntaxGeneratorExtensions.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/Extensions/SyntaxGeneratorExtensions.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/Extensions/SyntaxGeneratorExtensions.cs index ba65465f275c4..6375e384bd4d3 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/Extensions/SyntaxGeneratorExtensions.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/Extensions/SyntaxGeneratorExtensions.cs @@ -258,11 +258,11 @@ public static ImmutableArray GetSetAccessorStatements( return default; // Forward from the explicit property we're creating to the existing property it conflicts with if possible. - if (conflictingProperty is { SetMethod: not null, Parameters.Length: 0 } && - property is { SetMethod: not null, Parameters.Length: 0 }) + if (conflictingProperty is { SetMethod.Parameters.Length: 1 } && + property is { SetMethod.Parameters: [var parameter] }) { if (compilation.ClassifyCommonConversion(property.Type, conflictingProperty.Type) is { Exists: true, IsImplicit: true }) - return [generator.ExpressionStatement(generator.AssignmentStatement(generator.MemberAccessExpression(generator.ThisExpression(), property.Name), generator.IdentifierName("value")))]; + return [generator.ExpressionStatement(generator.AssignmentStatement(generator.MemberAccessExpression(generator.ThisExpression(), property.Name), generator.IdentifierName(parameter.Name)))]; } return generator.CreateThrowNotImplementedStatementBlock(compilation); From 32b4614ab694db543a166c25d70498f7dc6c11f9 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Mon, 25 Nov 2024 15:47:23 -0800 Subject: [PATCH 8/8] Update tests --- .../Tests/ImplementInterface/ImplementInterfaceTests.vb | 8 ++++---- .../Recommendations/CSharpRecommendationServiceRunner.cs | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Analyzers/VisualBasic/Tests/ImplementInterface/ImplementInterfaceTests.vb b/src/Analyzers/VisualBasic/Tests/ImplementInterface/ImplementInterfaceTests.vb index d1d431a8250d3..a3db069eff8ad 100644 --- a/src/Analyzers/VisualBasic/Tests/ImplementInterface/ImplementInterfaceTests.vb +++ b/src/Analyzers/VisualBasic/Tests/ImplementInterface/ImplementInterfaceTests.vb @@ -159,7 +159,7 @@ Class C Return M End Get Set(value As Integer) - M = value + M = Value End Set End Property End Class") @@ -199,7 +199,7 @@ Class C Return M End Get Set(value As Integer) - M = value + M = Value End Set End Property End Class") @@ -1686,7 +1686,7 @@ Class C Return Bar End Get Set(value As Integer) - Bar = value + Bar = Value End Set End Property End Class") @@ -1733,7 +1733,7 @@ Class C Return Bar End Get Set(value As Integer) - Bar = value + Bar = Value End Set End Property diff --git a/src/Workspaces/CSharp/Portable/Recommendations/CSharpRecommendationServiceRunner.cs b/src/Workspaces/CSharp/Portable/Recommendations/CSharpRecommendationServiceRunner.cs index 74c4fc0a55d99..159493a34e1a8 100644 --- a/src/Workspaces/CSharp/Portable/Recommendations/CSharpRecommendationServiceRunner.cs +++ b/src/Workspaces/CSharp/Portable/Recommendations/CSharpRecommendationServiceRunner.cs @@ -362,7 +362,7 @@ private ImmutableArray GetSymbolsForExpressionOrStatementContext() ? null : semanticModel.GetSymbolInfo(enclosingMethodOrLambdaNode).GetAnySymbol() ?? semanticModel.GetDeclaredSymbol(enclosingMethodOrLambdaNode); if (enclosingMethodOrLambda is IMethodSymbol method) - symbols = [.. symbols.Concat(method.Parameters)]; + symbols = [.. symbols, .. method.Parameters]; } else {