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.

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

Enhance explicit interface implementation completion provider #75568

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
219a780
Offer completion without return type
Rekkonnect Oct 19, 2024
dfb7cfb
Implement as a member inserting provider
Rekkonnect Oct 20, 2024
c6bdfec
Adjust commit behavior tests
Rekkonnect Oct 20, 2024
8b3936c
Share implementation with overload completion
Rekkonnect Oct 20, 2024
7b1a029
Fix member name splitting
Rekkonnect Oct 20, 2024
c80b651
Fix completion provider
Rekkonnect Oct 20, 2024
b7705b7
Fix trivia and formatting
Rekkonnect Oct 20, 2024
5afac24
Only remove tokens when node extends beyond the line
Rekkonnect Oct 20, 2024
1740ae0
Fix generation location
Rekkonnect Oct 21, 2024
167adf2
Fix removed line span
Rekkonnect Oct 21, 2024
0f0027c
Handle conversion operators
Rekkonnect Oct 21, 2024
18aae6b
Fix item commit tests
Rekkonnect Oct 21, 2024
bb2eaa2
Fix properties and events
Rekkonnect Oct 21, 2024
597408a
Add more tests
Rekkonnect Oct 21, 2024
1d6eb9d
Fix nullability warning
Rekkonnect Oct 21, 2024
5c3132c
Add skipped failing test for #70458
Rekkonnect Oct 21, 2024
2dc4ecf
Fix failing test
Rekkonnect Oct 21, 2024
6717df7
Remove spell fix suggestions on explicit interface implementations
Rekkonnect Oct 21, 2024
40cd75c
Only remove CS0539 on spell fix provider
Rekkonnect Oct 21, 2024
3cd8104
Revive spell fix tests and assert missing spell fixes
Rekkonnect Oct 21, 2024
d68cd2b
Add tests for nested interfaces
Rekkonnect Oct 21, 2024
f3f028d
Fix test regression
Rekkonnect Oct 22, 2024
3ab46c5
Fix explicit implementation parameter attributes
Rekkonnect Oct 22, 2024
e94f2cf
Remove leftover parameter
Rekkonnect Oct 22, 2024
062b705
Fix broken tests
Rekkonnect Oct 23, 2024
07a8599
Add extra trailing trivia test
Rekkonnect Oct 23, 2024
88b726e
Fix tests
Rekkonnect Oct 23, 2024
cc673cb
Remove default parameter value from explicit impl
Rekkonnect Nov 7, 2024
0bce3fd
Fix failing test
Rekkonnect Nov 7, 2024
1ae949f
Review comments
Rekkonnect Nov 8, 2024
7b0f668
Handle scoped
Rekkonnect Nov 8, 2024
de4e2c8
Merge branch 'main' into feature/75435-explicit-interface-impl-sugges…
Rekkonnect Nov 8, 2024
5bab752
Fix formatting
Rekkonnect Nov 8, 2024
381b125
Prefer using service interface
Rekkonnect Nov 8, 2024
58e7e86
Add docs + improve code
Rekkonnect Nov 8, 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
Original file line number Diff line number Diff line change
Expand Up @@ -4663,81 +4663,87 @@ public async Task TestOptionalDateTime1()
await new VerifyCS.Test
{
TestCode = """
using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

interface IGoo
{
void Goo([Optional][DateTimeConstant(100)] DateTime x);
}
interface IGoo
{
void Goo([Optional][DateTimeConstant(100)] DateTime x);
}

public class C : {|CS0535:IGoo|}
{
}
""",
public class C : {|CS0535:IGoo|}
{
}
""",
FixedCode = """
using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

interface IGoo
{
void Goo([Optional][DateTimeConstant(100)] DateTime x);
}
interface IGoo
{
void Goo([Optional][DateTimeConstant(100)] DateTime x);
}

public class C : IGoo
{
public void Goo([DateTimeConstant(100), Optional] DateTime x)
public class C : IGoo
{
throw new NotImplementedException();
public void Goo([DateTimeConstant(100), Optional] DateTime x)
{
throw new NotImplementedException();
}
}
}
""",
""",
Options = { AllOptionsOff },

// 🐛 one value is generated with 0L instead of 0
// 🐛 one value is generated with 100L instead of 100L
CodeActionValidationMode = CodeActionValidationMode.None,
}.RunAsync();
}

[Fact, WorkItem("http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/545476")]
public async Task TestOptionalDateTime2()
{
await TestWithAllCodeStyleOptionsOffAsync(
"""
using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
await new VerifyCS.Test
{
TestCode = """
using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

interface IGoo
{
void Goo([Optional][DateTimeConstant(100)] DateTime x);
}
interface IGoo
{
void Goo([Optional][DateTimeConstant(100)] DateTime x);
}

public class C : {|CS0535:IGoo|}
{
}
""",
"""
using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
public class C : {|CS0535:IGoo|}
{
}
""",
FixedCode = """
using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

interface IGoo
{
void Goo([Optional][DateTimeConstant(100)] DateTime x);
}
interface IGoo
{
void Goo([Optional][DateTimeConstant(100)] DateTime x);
}

public class C : IGoo
{
void IGoo.Goo(DateTime x)
public class C : IGoo
{
throw new NotImplementedException();
void IGoo.Goo([DateTimeConstant(100), Optional] DateTime x)
{
throw new NotImplementedException();
}
}
}
""",
codeAction: ("True;False;False:global::IGoo;Microsoft.CodeAnalysis.ImplementInterface.AbstractImplementInterfaceService+ImplementInterfaceCodeAction;", 1));
""",
Options = { AllOptionsOff },

CodeActionEquivalenceKey = "True;False;False:global::IGoo;Microsoft.CodeAnalysis.ImplementInterface.AbstractImplementInterfaceService+ImplementInterfaceCodeAction;",
// 🐛 one value is generated with 0L instead of 0
Copy link
Member

Choose a reason for hiding this comment

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

not sure what this means.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See also TestIntegralAndFloatLiterals; when specifying DateTimeConstant(100), the semantic structure equivalence check does not forgive the mismatching literal types, despite the available conversion. Removing the CodeActionValidationMode causes the test to fail saying that "100" does not equal "100", where the one is int and the other is long. Apparently the generated argument is perceived as being int when the attribute's argument expects a long. I have not dug up the real cause for this problem, so I just copied the note from the other test to indicate the problem.

CodeActionValidationMode = CodeActionValidationMode.None,
}.RunAsync();
}

[Fact, WorkItem("http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/545477")]
Expand Down Expand Up @@ -4813,12 +4819,12 @@ interface IGoo

public class C : IGoo
{
void IGoo.Goo1(object x)
void IGoo.Goo1([IUnknownConstant, Optional] object x)
{
throw new System.NotImplementedException();
}

void IGoo.Goo2(object x)
void IGoo.Goo2([IDispatchConstant, Optional] object x)
{
throw new System.NotImplementedException();
}
Expand Down Expand Up @@ -6726,7 +6732,8 @@ interface I

class C : I
{
bool I.Goo(bool x)
[return: MarshalAs(UnmanagedType.U1)]
bool I.Goo([MarshalAs(UnmanagedType.U1)] bool x)
{
throw new System.NotImplementedException();
}
Expand Down Expand Up @@ -7622,6 +7629,29 @@ public List<UU> M<TT, UU>(Dictionary<TT, List<UU>> a, TT b, UU c) where UU : TT
[Fact, WorkItem("http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/994328")]
public async Task TestDisposePatternWhenAdditionalUsingsAreIntroduced2()
{
const string equatableParameter =
#if NET9_0_OR_GREATER
"[AllowNull] int other"
Copy link
Member

Choose a reason for hiding this comment

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

this seems... undesirable. why have AllowNull on something known to be a value type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll crosscheck with the explicit interface implementation refactoring to see whether this is the existing behavior

Copy link
Member

Choose a reason for hiding this comment

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

waiting on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check the test GenericInterfaceNotNull1 in ImplementInterfaceTests. It generates [AllowNull] int bar, with the type of the parameter in the interface method being a generic type parameter, which is a similar case to this. I don't think it's that big of an issue.

#else
"int other"
#endif
;

const string resultUsings =
#if NET9_0_OR_GREATER
"""
using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
"""
#else
"""
using System;
using System.Collections.Generic;
"""
#endif
;

await TestWithAllCodeStyleOptionsOffAsync(
"""
interface I<T, U> : System.IDisposable, System.IEquatable<int> where U : T
Expand All @@ -7639,8 +7669,7 @@ partial class C
}
""",
$$"""
using System;
using System.Collections.Generic;
{{resultUsings}}

interface I<T, U> : System.IDisposable, System.IEquatable<int> where U : T
{
Expand All @@ -7652,7 +7681,7 @@ partial class C : I<System.Exception, System.AggregateException>, System.IDispos
{
private bool disposedValue;

bool IEquatable<int>.Equals(int other)
bool IEquatable<int>.Equals({{equatableParameter}})
{
throw new NotImplementedException();
}
Expand Down Expand Up @@ -11538,12 +11567,12 @@ class C3 : I1<C3>
throw new System.NotImplementedException();
}

public static explicit operator checked string(C3 x)
static explicit I1<C3>.operator checked string(C3 x)
{
throw new System.NotImplementedException();
}

public static explicit operator string(C3 x)
static explicit I1<C3>.operator string(C3 x)
{
throw new System.NotImplementedException();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@
// 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;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Editing;
Expand All @@ -11,6 +15,7 @@
using Microsoft.CodeAnalysis.Internal.Log;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.ImplementInterface;

Expand Down Expand Up @@ -43,7 +48,6 @@ public async Task<Document> ImplementInterfaceAsync(
if (state == null)
return document;

// TODO: https://github.com/dotnet/roslyn/issues/60990
// While implementing just one default action, like in the case of pressing enter after interface name in VB,
// choose to implement with the dispose pattern as that's the Dev12 behavior.
var implementDisposePattern = ShouldImplementDisposePattern(model.Compilation, state, explicitly: false);
Expand Down Expand Up @@ -94,4 +98,59 @@ public async Task<Document> ImplementInterfaceAsync(
this, document, info, options, configuration);
return await generator.ImplementInterfaceAsync(cancellationToken).ConfigureAwait(false);
}

public async Task<ISymbol> ExplicitlyImplementSingleInterfaceMemberAsync(
Document document,
IImplementInterfaceInfo info,
ISymbol member,
ImplementTypeOptions options,
CancellationToken cancellationToken)
{
var configuration = new ImplementInterfaceConfiguration()
{
Abstractly = false,
Explicitly = true,
OnlyRemaining = false,
ImplementDisposePattern = false,
ThroughMember = null,
};
var generator = new ImplementInterfaceGenerator(
this, document, info, options, configuration);
var implementedMembers = await generator.GenerateExplicitlyImplementedMembersAsync(member, options.PropertyGenerationBehavior, cancellationToken).ConfigureAwait(false);
cancellationToken.ThrowIfCancellationRequested();

var singleImplemented = implementedMembers[0];
Contract.ThrowIfNull(singleImplemented);

// Since non-indexer properties are the only symbols that get their implementing accessor symbols returned,
// we have to process the created symbols and reduce to the single property wherein the accessors are contained
if (member is IPropertySymbol { IsIndexer: false })
{
IPropertySymbol? commonContainer = null;
foreach (var implementedMember in implementedMembers)
{
if (implementedMember is IPropertySymbol implementedProperty)
{
commonContainer ??= implementedProperty;
Contract.ThrowIfFalse(commonContainer == implementedProperty, "We should have a common property implemented");
}
else
{
Contract.ThrowIfNull(implementedMember);
var containingProperty = implementedMember!.ContainingSymbol as IPropertySymbol;
Copy link
Member

Choose a reason for hiding this comment

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

why the !, you just threw if it was null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the !

Contract.ThrowIfNull(containingProperty);
commonContainer ??= containingProperty;
Contract.ThrowIfFalse(commonContainer == containingProperty, "We should have a common property implemented");
}
}
Contract.ThrowIfNull(commonContainer);
singleImplemented = commonContainer;
}
else
{
Contract.ThrowIfFalse(implementedMembers.Length == 1, "We missed another case that may return multiple symbols");
}

return singleImplemented;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +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.Collections.Immutable;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Host;
Expand Down Expand Up @@ -30,4 +30,11 @@ Task<Document> ImplementInterfaceAsync(
ImplementTypeOptions options,
ImplementInterfaceConfiguration configuration,
CancellationToken cancellationToken);

Task<ISymbol> ExplicitlyImplementSingleInterfaceMemberAsync(
Document document,
IImplementInterfaceInfo info,
ISymbol member,
ImplementTypeOptions options,
CancellationToken cancellationToken);
}
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,20 @@ private static bool IsUnexpressibleTypeParameter(
return condition1 || condition2 || condition3;
}

public async Task<ImmutableArray<ISymbol?>> GenerateExplicitlyImplementedMembersAsync(
ISymbol member,
ImplementTypePropertyGenerationBehavior propertyGenerationBehavior,
CancellationToken cancellationToken)
{
var compilation = await Document.Project.GetRequiredCompilationAsync(cancellationToken).ConfigureAwait(false);
var syntaxFacts = Document.GetRequiredLanguageService<ISyntaxFactsService>();
var addUnsafe = member.RequiresUnsafeModifier() && !syntaxFacts.IsUnsafeContext(State.InterfaceNode);
return GenerateMembers(
compilation, member, memberName: member.Name, generateInvisibly: true,
generateAbstractly: false, addNew: false, addUnsafe, propertyGenerationBehavior)
.ToImmutableArray();
}

private IEnumerable<ISymbol?> GenerateMembers(
Compilation compilation,
ISymbol member,
Expand Down
Loading
Loading