Partial properties: public API and IDE features #73603
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
RikkiGibson
force-pushed
the
partial-properties-8
branch
from
a2f7972
to
b801092
Compare
...Workspaces/Core/Portable/FindSymbols/FindReferences/Finders/PropertySymbolReferenceFinder.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/Test2/FindReferences/FindReferencesTests.PropertySymbols.vb
Outdated
Show resolved
Hide resolved
src/EditorFeatures/Test2/FindReferences/FindReferencesTests.PropertySymbols.vb
Show resolved
Hide resolved
|
@RikkiGibson fixed. Thanks. The issue here was that we were losing which part this was (the def or impl). So when sending from OOP to local host we picked up the wrong symbol. |
|
Thank you Cyrus! |
|
Done with pass. IDE still lgtm. |
…/roslyn into partial-properties-8
|
My plan is to There are many places where the Partial*Part APIs on methods are being used in the features and workspaces layers already. I will identify where it's possible that use of the partial property APIs are desired as well, and add issue links to those locations. |
|
@jcouv for review of the compiler side changes |
| /// If this is a partial property implementation part, returns the corresponding | ||
| /// definition part. Otherwise null. | ||
| /// </summary> | ||
| IPropertySymbol? PartialDefinitionPart { get; } |
There was a problem hiding this comment.
Looking at the usages of the existing public APIs for partial methods, I feel there are more IDE scenario and potentially EnC scenarios that will need to be adjusted for partial properties/indexers. Are those known/understood?
There was a problem hiding this comment.
IDE scenarios outside of hte min-bar are entirely optional to change.
| MethodSymbol method => method.PartialImplementationPart, | ||
| SourcePropertySymbol property => property.PartialImplementationPart, | ||
| _ => null | ||
| }; |
There was a problem hiding this comment.
This PR includes some tests for GetDeclaredSymbol public API. Do we have tests for GetDeclaredSymbol on the parameters of a partial indexer?
It looks like there is additional logic in this file that looks at PartialDefinitionPart for partial methods for that scenario, which might need to be updated to account for partial properties. #Resolved
There was a problem hiding this comment.
I have addressed the remaining usages of PartialDefinition/ImplementationPart in this file.
There is one other usage of PartialDefinitionPart which doesn't need to account for properties, because it is for a method type parameter scenario, and properties don't have type parameters.
| @@ -2033,9 +2037,7 @@ internal override ImmutableArray<ISymbol> GetDeclaredSymbols(BaseFieldDeclaratio | |||
| return null; | |||
| } | |||
|
|
|||
| return | |||
| GetParameterSymbol(method.Parameters, parameter, cancellationToken) ?? | |||
| ((object)method.PartialDefinitionPart == null ? null : GetParameterSymbol(method.PartialDefinitionPart.Parameters, parameter, cancellationToken)); | |||
There was a problem hiding this comment.
I believe this check of 'PartialDefinitionPart' did not make sense. The containing method GetMethodParameterSymbol(ParameterSyntax, CancellationToken) works by grabbing the parent MemberDeclarationSyntax of the given ParameterSyntax. It then gets the symbol for the MemberDeclarationSyntax, and searches the symbol's parameters for one which matches the original input ParameterSyntax.
We should expect that GetDeclaredSymbol returns the correct partial symbol corresponding to the declaration syntax which was passed in. So there is no benefit, in the case where searching the parameters of the symbol we got resulted in no match, to also searching the parameters of the definition part which was related to the symbol we got.
| @@ -2165,10 +2164,7 @@ public override ITypeParameterSymbol GetDeclaredSymbol(TypeParameterSyntax typeP | |||
| return this.GetTypeParameterSymbol(typeSymbol.TypeParameters, typeParameter).GetPublicSymbol(); | |||
|
|
|||
| case MethodSymbol methodSymbol: | |||
| return (this.GetTypeParameterSymbol(methodSymbol.TypeParameters, typeParameter) ?? | |||
| ((object)methodSymbol.PartialDefinitionPart == null | |||
There was a problem hiding this comment.
I think this check didn't make sense for the same reason as #73603 (comment).
|
@dotnet/roslyn-compiler for second review |
|
@dotnet/roslyn-compiler for review |
RikkiGibson
merged commit 6ccd0ec
into
dotnet:features/partial-properties
Test plan: #73090
Public API design: #73411