Enhance explicit interface implementation completion provider #75568
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
dotnet-policy-service
bot
added
Community
| { | ||
| throw new System.NotImplementedException(); | ||
| } | ||
| } | ||
| """; | ||
| // TODO: Consider adding the default value too. |
There was a problem hiding this comment.
Since default values are meaningless in explicit interface implementations, showing them in the completion item is also unimportant. Removed this comment to reflect the sanity of the current case.
| var implementedMembers = await generator.GenerateExplicitlyImplementedMembersAsync(member, options.PropertyGenerationBehavior, cancellationToken).ConfigureAwait(false); | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
|
|
||
| var singleImplemented = implementedMembers[0]!; |
There was a problem hiding this comment.
Removing the ! and asserting not null as a contract
| if (implementedMember is IPropertySymbol implementedProperty) | ||
| { | ||
| commonContainer ??= implementedProperty; | ||
| Debug.Assert(commonContainer == implementedProperty, "We should have a common property implemented"); |
There was a problem hiding this comment.
make these intro contract calls.
| """ | ||
| await new VerifyCS.Test | ||
| { | ||
| TestCode = """ | ||
| using System; |
There was a problem hiding this comment.
i woudl indent these strings.
| Options = { AllOptionsOff }, | ||
|
|
||
| CodeActionEquivalenceKey = "True;False;False:global::IGoo;Microsoft.CodeAnalysis.ImplementInterface.AbstractImplementInterfaceService+ImplementInterfaceCodeAction;", | ||
| // 🐛 one value is generated with 0L instead of 0 |
There was a problem hiding this comment.
not sure what this means.
There was a problem hiding this comment.
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.
| @@ -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" | |||
There was a problem hiding this comment.
this seems... undesirable. why have AllowNull on something known to be a value type?
There was a problem hiding this comment.
I'll crosscheck with the explicit interface implementation refactoring to see whether this is the existing behavior
There was a problem hiding this comment.
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 | ||
| { | ||
| Contract.ThrowIfNull(implementedMember); | ||
| var containingProperty = implementedMember!.ContainingSymbol as IPropertySymbol; |
There was a problem hiding this comment.
why the !, you just threw if it was null.
| switch (caretTarget) | ||
| { | ||
| case EventFieldDeclarationSyntax: | ||
| return caretTarget.GetLocation().SourceSpan.End; |
There was a problem hiding this comment.
to avoid all this repetition, just have tehse helpers return the node you want to move to the end to. The caller can do the .GetLocation().SourceSpan.End bit.
| case BasePropertyDeclarationSyntax propertyDeclaration: | ||
| { | ||
| // property: no accessors; move to the end of the declaration | ||
| if (propertyDeclaration.AccessorList != null && propertyDeclaration.AccessorList.Accessors.Any()) |
There was a problem hiding this comment.
| if (propertyDeclaration.AccessorList != null && propertyDeclaration.AccessorList.Accessors.Any()) | |
| if (propertyDeclaration.AccessorList is { Accessors.Count: > 0 }) |
There was a problem hiding this comment.
| if (propertyDeclaration.AccessorList != null && propertyDeclaration.AccessorList.Accessors.Any()) | |
| if (propertyDeclaration.AccessorList is { Accessors: [var firstAccessor, ..] }) |
| using Microsoft.CodeAnalysis.ErrorReporting; | ||
| using System.Linq; | ||
| using Roslyn.Utilities; | ||
| using Microsoft.CodeAnalysis.Editing; |
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
all off these need docs. and docs on 'why' not 'what' :)
| catch (Exception e) | ||
| when (FatalError.ReportAndCatchUnlessCanceled(e, ErrorSeverity.General)) |
There was a problem hiding this comment.
| catch (Exception e) | |
| when (FatalError.ReportAndCatchUnlessCanceled(e, ErrorSeverity.General)) | |
| catch (Exception e) when (FatalError.ReportAndCatchUnlessCanceled(e, ErrorSeverity.General)) |
| IEventSymbol eventSymbol => ToDisplayString(eventSymbol), | ||
| IPropertySymbol propertySymbol => ToDisplayString(propertySymbol, semanticModel), | ||
| IMethodSymbol methodSymbol => ToDisplayString(methodSymbol, semanticModel), | ||
| _ => "" // This shouldn't happen. |
| { | ||
| RefKind.Out => "out ", | ||
| RefKind.Ref => "ref ", | ||
| RefKind.In => "in ", |
There was a problem hiding this comment.
what about things like scoped?
| var document = context.Document; | ||
| var position = context.Position; | ||
| var cancellationToken = context.CancellationToken; | ||
| var implementInterfaceService = (AbstractImplementInterfaceService)newDocument.GetRequiredLanguageService<IImplementInterfaceService>(); |
There was a problem hiding this comment.
why are you casting to the derived type?
There was a problem hiding this comment.
AbstractImplementInterfaceService.State.Generate requires an AbstractImplementInterfaceService, hence the cast
There was a problem hiding this comment.
why are you accessing that here. it's an internal implementation detail. If you need information from it, it should be through the interface.
| var semanticModel = await newDocument.GetSemanticModelAsync(cancellationToken) | ||
| .ConfigureAwait(false); | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| Debug.Assert(semanticModel != null); |
There was a problem hiding this comment.
use GetRequiredSmenaitcModol, remov ethis assert.
| var syntaxTree = await document.GetRequiredSyntaxTreeAsync(cancellationToken).ConfigureAwait(false); | ||
| var interfaceNode = await GetInterfaceNodeInCompletionAsync(newDocument, completionItem, baseMemberInterfaceType, cancellationToken).ConfigureAwait(false); | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| Debug.Assert(interfaceNode != null); |
There was a problem hiding this comment.
remove these asserts. either make contracts, or ensure that these paths always succeed, or bail out if you can't do that.
| private static async Task<SyntaxNode?> GetInterfaceNodeInCompletionAsync(Document document, CompletionItem item, INamedTypeSymbol baseMemberInterfaceType, CancellationToken cancellationToken) | ||
| { | ||
| var syntaxTree = await document.GetRequiredSyntaxTreeAsync(cancellationToken).ConfigureAwait(false); | ||
| cancellationToken.ThrowIfCancellationRequested(); |
There was a problem hiding this comment.
don't do this. here are hte rules on cacncellation tokens:
- check them on the entrypoints of methods that take them
- check them in the start of loops, if hte loop isn't calling anything that takes teh cancelation token.
| cancellationToken.ThrowIfCancellationRequested(); | ||
| var documentSemanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false); | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| Debug.Assert(documentSemanticModel != null, "Expected a semantic model out of the document"); |
There was a problem hiding this comment.
GetRequiredCancellationToken
There was a problem hiding this comment.
GetRequiredSemanticModel?
| Debug.Assert(documentSemanticModel != null, "Expected a semantic model out of the document"); | ||
|
|
||
| var compilation = documentSemanticModel.Compilation; | ||
| var node = syntaxTree.FindNode(item.Span, false, true, cancellationToken); |
There was a problem hiding this comment.
named parameters for false/true.
| if (interfaceNode != null) | ||
| { | ||
| return interfaceNode; | ||
| } |
There was a problem hiding this comment.
prior to this it would be good to doc the general idea of what's going on. what you're looking for and what you expect the shape of the tree to be.
|
Done with pass. Also, you have a file with a conflict in it. please bring this PR up to date with main. Thanks :) |
|
@CyrusNajmabadi ready for review again |
Closes #75435
Closes #72224 -- this was working but the test had a different expected outcome, this PR unskips the test
Closes #70458
Implementation details
The
ExplicitInterfaceMemberCompletionProvideris now up to par with theOverrideCompletionProvider, with some abstractions to share between the two like theItemGetter. The characters[and<no longer commit the selected completion item since the completion provider now fills the implementation body and the signature completely.The
AbstractMemberInsertingCompletionProvideritself also became faster by doing less work when replacing the completed text in the original document via annotations. This behavioral adjustment was also necessary because of problems that arose while changing theExplicitInterfaceMemberCompletionProviderto be anAbstractMemberInsertingCompletionProvider.Another fixed bug is the explicit interface implementation generator for abstract conversion operators, which were not handled in the "Implement interface explicitly" code fixes. This was resolved for both the fixes and the completion provider.
When the user is typing out an explicit interface declaration member, the provider ignores the potential difference in the return type. This is justified as the user could easily mistake the return type, and so we do not want to filter out the actual candidates that can be offered. Besides, in most cases the recommended members are too few to need this filtering. If the user types out a return type and then we complete a member that has a different return type, the result includes the actual signature of the implemented member, discarding the old return type.
The spell fix suggestions for explicit interface implementation members were removed because of the complexity of handling the generated code. The justification is that it's easier for the user to manually type the member out because of the improvements to the completion provider.
The symbol generators no longer ignore the attributes on the parameters of explicit interface implementations, since they are legal to use in that context.
Tests