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

Partial properties: public API and IDE features #73603

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 @@ -1799,10 +1799,14 @@ private Symbol GetDeclaredMember(NamespaceOrTypeSymbol container, TextSpan decla
zeroWidthMatch = symbol;
}

// Handle the case of the implementation of a partial method.
var partial = symbol.Kind == SymbolKind.Method
? ((MethodSymbol)symbol).PartialImplementationPart
: null;
// Handle the case of the implementation of a partial member.
Symbol partial = symbol switch
{
MethodSymbol method => method.PartialImplementationPart,
SourcePropertySymbol property => property.PartialImplementationPart,
_ => null
};
Copy link
Member

@jcouv jcouv May 24, 2024

Choose a reason for hiding this comment

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

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

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 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.


if ((object)partial != null)
{
var loc = partial.GetFirstLocation();
Expand Down Expand Up @@ -2033,9 +2037,7 @@ private ParameterSymbol GetMethodParameterSymbol(
return null;
}

return
GetParameterSymbol(method.Parameters, parameter, cancellationToken) ??
((object)method.PartialDefinitionPart == null ? null : GetParameterSymbol(method.PartialDefinitionPart.Parameters, parameter, cancellationToken));
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 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.

return GetParameterSymbol(method.Parameters, parameter, cancellationToken);
}

private ParameterSymbol GetIndexerParameterSymbol(
Expand Down Expand Up @@ -2127,11 +2129,8 @@ private ParameterSymbol GetDelegateParameterSymbol(
}

/// <summary>
/// Given a type parameter declaration (field or method), get the corresponding symbol
/// Given a type parameter declaration (on a type or method), get the corresponding symbol
/// </summary>
/// <param name="typeParameter"></param>
/// <param name="cancellationToken">The cancellation token.</param>
/// <returns></returns>
public override ITypeParameterSymbol GetDeclaredSymbol(TypeParameterSyntax typeParameter, CancellationToken cancellationToken = default(CancellationToken))
{
if (typeParameter == null)
Expand Down Expand Up @@ -2165,10 +2164,7 @@ private ParameterSymbol GetDelegateParameterSymbol(
return this.GetTypeParameterSymbol(typeSymbol.TypeParameters, typeParameter).GetPublicSymbol();

case MethodSymbol methodSymbol:
return (this.GetTypeParameterSymbol(methodSymbol.TypeParameters, typeParameter) ??
((object)methodSymbol.PartialDefinitionPart == null
Copy link
Contributor Author

@RikkiGibson RikkiGibson May 28, 2024

Choose a reason for hiding this comment

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

I think this check didn't make sense for the same reason as #73603 (comment).

? null
: this.GetTypeParameterSymbol(methodSymbol.PartialDefinitionPart.TypeParameters, typeParameter))).GetPublicSymbol();
return this.GetTypeParameterSymbol(methodSymbol.TypeParameters, typeParameter).GetPublicSymbol();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,14 @@ ImmutableArray<CustomModifier> IPropertySymbol.RefCustomModifiers

RefKind IPropertySymbol.RefKind => _underlying.RefKind;

#nullable enable
IPropertySymbol? IPropertySymbol.PartialDefinitionPart => (_underlying as SourcePropertySymbol)?.PartialDefinitionPart.GetPublicSymbol();

IPropertySymbol? IPropertySymbol.PartialImplementationPart => (_underlying as SourcePropertySymbol)?.PartialImplementationPart.GetPublicSymbol();

bool IPropertySymbol.IsPartialDefinition => (_underlying as SourcePropertySymbol)?.IsPartialDefinition ?? false;
#nullable disable

#region ISymbol Members

protected override void Accept(SymbolVisitor visitor)
Expand Down
153 changes: 153 additions & 0 deletions src/Compilers/CSharp/Test/Symbol/Symbols/PartialPropertiesTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4450,6 +4450,159 @@ partial class C
Diagnostic(ErrorCode.ERR_InvalidModifierForLanguageVersion, "this").WithArguments("partial", "12.0", "preview").WithLocation(7, 24));
}

[Fact]
public void GetDeclaredSymbol_01()
{
var source = ("""
partial class C
{
public partial int Prop { get; }
public partial int Prop { get => 1; }
}
""".NormalizeLineEndings(), "Program.cs");
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved

var comp = CreateCompilation(source);
var tree = comp.SyntaxTrees[0];

var model = comp.GetSemanticModel(tree);
var properties = tree.GetRoot().DescendantNodes().OfType<PropertyDeclarationSyntax>().ToArray();
Assert.Equal(2, properties.Length);

var defSymbol = model.GetDeclaredSymbol(properties[0])!;
Assert.Equal("System.Int32 C.Prop { get; }", defSymbol.ToTestDisplayString());

var implSymbol = model.GetDeclaredSymbol(properties[1])!;
Assert.Equal("System.Int32 C.Prop { get; }", implSymbol.ToTestDisplayString());

Assert.NotEqual(defSymbol, implSymbol);
Assert.Same(implSymbol, defSymbol.PartialImplementationPart);
Assert.Same(defSymbol, implSymbol.PartialDefinitionPart);
Assert.True(defSymbol.IsPartialDefinition);
Assert.False(implSymbol.IsPartialDefinition);

// This is consistent with partial methods.
Assert.Equal("SourceFile(Program.cs[43..47))", defSymbol.Locations.Single().ToString());
Assert.Equal("SourceFile(Program.cs[81..85))", implSymbol.Locations.Single().ToString());
}

[Fact]
public void GetDeclaredSymbol_02()
{
// Property contained in generic type. Check original definition and constructed symbols.
var source = ("""
partial class C<T>
{
public partial int Prop { get; }
public partial int Prop { get => 1; }
}
""".NormalizeLineEndings(), "Program.cs");

var comp = CreateCompilation(source);
var tree = comp.SyntaxTrees[0];

var model = comp.GetSemanticModel(tree);
var properties = tree.GetRoot().DescendantNodes().OfType<PropertyDeclarationSyntax>().ToArray();
Assert.Equal(2, properties.Length);

var defSymbol = model.GetDeclaredSymbol(properties[0])!;
Assert.Equal("System.Int32 C<T>.Prop { get; }", defSymbol.ToTestDisplayString());

var implSymbol = model.GetDeclaredSymbol(properties[1])!;
Assert.Equal("System.Int32 C<T>.Prop { get; }", implSymbol.ToTestDisplayString());

Assert.NotEqual(defSymbol, implSymbol);
Assert.Same(implSymbol, defSymbol.PartialImplementationPart);
Assert.Same(defSymbol, implSymbol.PartialDefinitionPart);

Assert.True(defSymbol.IsPartialDefinition);
Assert.False(implSymbol.IsPartialDefinition);

// This is consistent with partial methods.
Assert.Equal("SourceFile(Program.cs[46..50))", defSymbol.Locations.Single().ToString());
Assert.Equal("SourceFile(Program.cs[84..88))", implSymbol.Locations.Single().ToString());

var intSymbol = comp.GetSpecialType(SpecialType.System_Int32);
var cOfTSymbol = defSymbol.ContainingType!;
var cOfIntSymbol = cOfTSymbol.Construct([intSymbol]);

// Constructed symbols always return null/false from the partial-related public APIs
var defOfIntSymbol = (IPropertySymbol)cOfIntSymbol.GetMember("Prop");
Assert.Equal("System.Int32 C<System.Int32>.Prop { get; }", defOfIntSymbol.ToTestDisplayString());
Assert.Null(defOfIntSymbol.PartialImplementationPart);
Assert.False(defOfIntSymbol.IsPartialDefinition);
}

[Fact]
public void GetDeclaredSymbol_03()
{
// Indexer
var source = ("""
partial class C
{
public partial int this[int i] { get; }
public partial int this[int i] { get => 1; }
}
""".NormalizeLineEndings(), "Program.cs");

var comp = CreateCompilation(source);
var tree = comp.SyntaxTrees[0];

var model = comp.GetSemanticModel(tree);
var indexers = tree.GetRoot().DescendantNodes().OfType<IndexerDeclarationSyntax>().ToArray();
Assert.Equal(2, indexers.Length);

var defSymbol = model.GetDeclaredSymbol(indexers[0])!;
Assert.Equal("System.Int32 C.this[System.Int32 i] { get; }", defSymbol.ToTestDisplayString());

var implSymbol = model.GetDeclaredSymbol(indexers[1])!;
Assert.Equal("System.Int32 C.this[System.Int32 i] { get; }", implSymbol.ToTestDisplayString());

Assert.NotEqual(defSymbol, implSymbol);
Assert.Same(implSymbol, defSymbol.PartialImplementationPart);
Assert.Same(defSymbol, implSymbol.PartialDefinitionPart);

Assert.True(defSymbol.IsPartialDefinition);
Assert.False(implSymbol.IsPartialDefinition);

// This is consistent with partial methods.
Assert.Equal("SourceFile(Program.cs[43..47))", defSymbol.Locations.Single().ToString());
Assert.Equal("SourceFile(Program.cs[88..92))", implSymbol.Locations.Single().ToString());
}

[Fact]
public void GetDeclaredSymbol_04()
{
// Indexer parameter
var source = ("""
partial class C
{
public partial int this[int i] { get; }
public partial int this[int i] { get => 1; }
}
""".NormalizeLineEndings(), "Program.cs");

var comp = CreateCompilation(source);
var tree = comp.SyntaxTrees[0];

var model = comp.GetSemanticModel(tree);
var parameters = tree.GetRoot().DescendantNodes().OfType<ParameterSyntax>().ToArray();
Assert.Equal(2, parameters.Length);

var defSymbol = model.GetDeclaredSymbol(parameters[0])!;
Assert.Equal("System.Int32 i", defSymbol.ToTestDisplayString());

var implSymbol = model.GetDeclaredSymbol(parameters[1])!;
Assert.Equal("System.Int32 i", implSymbol.ToTestDisplayString());

Assert.NotEqual(defSymbol, implSymbol);
Assert.Same(implSymbol, ((IPropertySymbol)defSymbol.ContainingSymbol).PartialImplementationPart!.Parameters[0]);
Assert.Same(defSymbol, ((IPropertySymbol)implSymbol.ContainingSymbol).PartialDefinitionPart!.Parameters[0]);

// This is consistent with partial methods.
Assert.Equal("SourceFile(Program.cs[52..53))", defSymbol.Locations.Single().ToString());
Assert.Equal("SourceFile(Program.cs[97..98))", implSymbol.Locations.Single().ToString());
}

// PROTOTYPE(partial-properties): override partial property where base has modopt
}
}
3 changes: 3 additions & 0 deletions src/Compilers/Core/Portable/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
Microsoft.CodeAnalysis.IParameterSymbol.IsParamsCollection.get -> bool
Microsoft.CodeAnalysis.IParameterSymbol.IsParamsArray.get -> bool
Microsoft.CodeAnalysis.IPropertySymbol.PartialDefinitionPart.get -> Microsoft.CodeAnalysis.IPropertySymbol?
Microsoft.CodeAnalysis.IPropertySymbol.PartialImplementationPart.get -> Microsoft.CodeAnalysis.IPropertySymbol?
Microsoft.CodeAnalysis.IPropertySymbol.IsPartialDefinition.get -> bool
Microsoft.CodeAnalysis.Operations.ArgumentKind.ParamCollection = 4 -> Microsoft.CodeAnalysis.Operations.ArgumentKind
Microsoft.CodeAnalysis.Diagnostics.SuppressionInfo.ProgrammaticSuppressions.get -> System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.Diagnostics.Suppression>
Microsoft.CodeAnalysis.Emit.InstrumentationKind.ModuleCancellation = 3 -> Microsoft.CodeAnalysis.Emit.InstrumentationKind
Expand Down
17 changes: 17 additions & 0 deletions src/Compilers/Core/Portable/Symbols/IPropertySymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -110,5 +110,22 @@ public interface IPropertySymbol : ISymbol
/// The list of custom modifiers, if any, associated with the type of the property.
/// </summary>
ImmutableArray<CustomModifier> TypeCustomModifiers { get; }

/// <summary>
/// If this is a partial property implementation part, returns the corresponding
/// definition part. Otherwise null.
/// </summary>
IPropertySymbol? PartialDefinitionPart { get; }
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

IDE scenarios outside of hte min-bar are entirely optional to change.


/// <summary>
/// If this is a partial property definition part, returns the corresponding
/// implementation part. Otherwise null.
/// </summary>
IPropertySymbol? PartialImplementationPart { get; }

/// <summary>
/// Returns true if this is a partial definition part. Otherwise false.
/// </summary>
bool IsPartialDefinition { get; }
}
}
21 changes: 21 additions & 0 deletions src/Compilers/VisualBasic/Portable/Symbols/PropertySymbol.vb
Original file line number Diff line number Diff line change
Expand Up @@ -621,6 +621,27 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols
End Get
End Property

Private ReadOnly Property IPropertySymbol_PartialDefinitionPart As IPropertySymbol Implements IPropertySymbol.PartialDefinitionPart
Get
' Feature not supported in VB
Return Nothing
End Get
End Property

Private ReadOnly Property IPropertySymbol_PartialImplementationPart As IPropertySymbol Implements IPropertySymbol.PartialImplementationPart
Get
' Feature not supported in VB
Return Nothing
End Get
End Property

Private ReadOnly Property IPropertySymbol_IsPartialDefinition As Boolean Implements IPropertySymbol.IsPartialDefinition
Get
' Feature not supported in VB
Return False
End Get
End Property

Public Overrides Sub Accept(visitor As SymbolVisitor)
visitor.VisitProperty(Me)
End Sub
Expand Down
14 changes: 14 additions & 0 deletions src/EditorFeatures/CSharpTest/NavigateTo/NavigateToTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,20 @@ public async Task FindPartialMethods(TestHost testHost, Composition composition)
});
}

[Theory, CombinatorialData]
public async Task FindPartialProperties(TestHost testHost, Composition composition)
{
await TestAsync(testHost, composition, "partial class Goo { partial int Prop { get; set; } } partial class Goo { partial int Prop { get => 1; set { } } }", async w =>
{
var expecteditem1 = new NavigateToItem("Prop", NavigateToItemKind.Property, "csharp", null, null, s_emptyExactPatternMatch, null);
var expecteditems = new List<NavigateToItem> { expecteditem1, expecteditem1 };

var items = await _aggregator.GetItemsAsync("Prop");

VerifyNavigateToResultItems(expecteditems, items);
});
}

[Theory, CombinatorialData]
public async Task FindPartialMethodDefinitionOnly(TestHost testHost, Composition composition)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1558,5 +1558,32 @@ class P
</Workspace>
Await TestAPIAndFeature(input, kind, host)
End Function

<WorkItem("http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/538886")>
<WpfTheory, CombinatorialData>
Public Async Function TestCSharp_PartialProperty1(kind As TestKind, host As TestHost) As Task
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
Dim input =
<Workspace>
<Project Language="C#" CommonReferences="true">
<Document>
using System;
namespace ConsoleApplication22
{
class Program
{
public static partial int {|Definition:Prop|} { get; }
public static partial int {|Definition:P$$rop|} => 1;

static void Main(string[] args)
{
int temp = Program.[|Prop|];
}
}
}
</Document>
</Project>
</Workspace>
Await TestAPIAndFeature(input, kind, host)
End Function
End Class
End Namespace
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,39 @@ class Program
Await TestAsync(workspace)
End Function

<WpfFact>
Public Async Function TestCSharpGotoDefinitionPartialProperty() As Task
Dim workspace =
<Workspace>
<Project Language="C#" CommonReferences="true">
<Document>
partial class Test
{
public partial int Prop { get; set; }
}
</Document>
<Document>
partial class Test
{
void Goo()
{
var t = new Test();
int i = t.Prop$$;
}

public partial void [|Prop|]
{
get => throw new NotImplementedException();
set => throw new NotImplementedException();
}
}
</Document>
</Project>
</Workspace>

Await TestAsync(workspace)
End Function

<WpfFact>
Public Async Function TestCSharpGotoDefinitionOnMethodCall1() As Task
Dim workspace =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,9 @@ internal static class GoToDefinitionFeatureHelpers

symbol = definition ?? symbol;

// If it is a partial method declaration with no body, choose to go to the implementation
// that has a method body.
if (symbol is IMethodSymbol method)
symbol = method.PartialImplementationPart ?? symbol;
// If symbol has a partial implementation part, prefer to go to it, since that is where the body is.
symbol = (symbol as IMethodSymbol)?.PartialImplementationPart ?? symbol;
symbol = (symbol as IPropertySymbol)?.PartialImplementationPart ?? symbol;

return symbol;
}
Expand Down
Loading