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

Prepare partial properties feature for merge #73773

Merged
Merged
Show file tree
Hide file tree
Changes from 11 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 @@ -234,6 +234,7 @@ SyntaxKind.IndexerDeclaration or
var symbol = model.GetSymbolInfo(invocation.Expression, cancellationToken).Symbol;
if (symbol is not IMethodSymbol method || method.PartialImplementationPart is not null)
{
// https://github.com/dotnet/roslyn/issues/73772: should we also bail out on a partial property?
// We don't handle partial methods yet
return null;
}
Expand Down
23 changes: 11 additions & 12 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2310,18 +2310,6 @@ internal enum ErrorCode
ERR_InterceptsLocationDataInvalidPosition = 9235,
INF_TooManyBoundLambdas = 9236,

// PROTOTYPE(partial-properties): pack
ERR_PartialPropertyMissingImplementation = 9300,
ERR_PartialPropertyMissingDefinition = 9301,
ERR_PartialPropertyDuplicateDefinition = 9302,
ERR_PartialPropertyDuplicateImplementation = 9303,
ERR_PartialPropertyMissingAccessor = 9304,
ERR_PartialPropertyUnexpectedAccessor = 9305,
ERR_PartialPropertyInitMismatch = 9306,
ERR_PartialPropertyTypeDifference = 9307,
WRN_PartialPropertySignatureDifference = 9308,
ERR_PartialPropertyRequiredDifference = 9309,

#endregion

WRN_BadYieldInLock = 9237,
Expand All @@ -2337,6 +2325,17 @@ internal enum ErrorCode
ERR_BadNonVirtualInterfaceMemberAccessOnAllowsRefLike = 9246,
ERR_BadAllowByRefLikeEnumerator = 9247,

ERR_PartialPropertyMissingImplementation = 9248,
ERR_PartialPropertyMissingDefinition = 9249,
ERR_PartialPropertyDuplicateDefinition = 9250,
ERR_PartialPropertyDuplicateImplementation = 9251,
ERR_PartialPropertyMissingAccessor = 9252,
ERR_PartialPropertyUnexpectedAccessor = 9253,
ERR_PartialPropertyInitMismatch = 9254,
ERR_PartialPropertyTypeDifference = 9255,
WRN_PartialPropertySignatureDifference = 9256,
ERR_PartialPropertyRequiredDifference = 9257,

// Note: you will need to do the following after adding errors:
// 1) Update ErrorFacts.IsBuildOnlyDiagnostic (src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs)

Expand Down
3 changes: 1 addition & 2 deletions src/Compilers/CSharp/Portable/Errors/MessageID.cs
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,7 @@ internal enum MessageID

IDS_FeatureRefStructInterfaces = MessageBase + 12844,

// PROTOTYPE(partial-properties): pack
IDS_FeaturePartialProperties = MessageBase + 13000,
IDS_FeaturePartialProperties = MessageBase + 12845,
}

// Message IDs may refer to strings that need to be localized.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,7 @@ internal Variables GetRootScope()

internal Variables? GetVariablesForMethodScope(MethodSymbol method)
{
// https://github.com/dotnet/roslyn/issues/73772: is this needed if we also delete the weird cascading in EnterParameters?
method = method.PartialImplementationPart ?? method;
var variables = this;
while (true)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2764,6 +2764,7 @@ private void EnterParameters()
}

// The partial definition part may include optional parameters whose default values we want to simulate assigning at the beginning of the method
// https://github.com/dotnet/roslyn/issues/73772: is this actually used/meaningful?
methodSymbol = methodSymbol.PartialDefinitionPart ?? methodSymbol;

var methodParameters = methodSymbol.Parameters;
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -298,8 +298,6 @@ IMethodSymbol IMethodSymbol.PartialDefinitionPart
}
}

// PROTOTYPE(partial-properties): Perhaps this API should be implemented as '_underlying.OriginalDefinition.IsPartialDefinition()' instead.
// However, this would be a behavior change. Callers may have been assuming that if the API returned true, then the receiver is an original definition symbol.
bool IMethodSymbol.IsPartialDefinition => _underlying.IsDefinition && _underlying.IsPartialDefinition();

INamedTypeSymbol IMethodSymbol.AssociatedAnonymousDelegate
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ namespace Microsoft.CodeAnalysis.CSharp.Symbols
{
internal sealed class SourcePropertySymbol : SourcePropertySymbolBase
{
// PROTOTYPE(partial-properties): Verify that the increase in memory consumption from this is acceptable
private SourcePropertySymbol? _otherPartOfPartial;

internal static SourcePropertySymbol Create(SourceMemberContainerTypeSymbol containingType, Binder bodyBinder, PropertyDeclarationSyntax syntax, BindingDiagnosticBag diagnostics)
Expand Down Expand Up @@ -175,7 +174,10 @@ public override OneOrMany<SyntaxList<AttributeListSyntax>> GetAttributeDeclarati
{
// Attributes on partial properties are owned by the definition part.
// If this symbol has a non-null PartialDefinitionPart, we should have accessed this method through that definition symbol instead
Debug.Assert(PartialDefinitionPart is null);
Debug.Assert(PartialDefinitionPart is null
// We might still get here when asking for the attributes on a backing field.
// This is an error scenario (requires using a property initializer and field-targeted attributes on partial property implementation part).
|| this.BackingField is not null);
Comment on lines +177 to +180
Copy link
Contributor Author

@RikkiGibson RikkiGibson May 30, 2024

Choose a reason for hiding this comment

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

@cston I was anticipating that perhaps the synthesized BackingField symbol will be shared between partial property parts, or otherwise the two field symbols will reference each other similarly as the associated partial property symbols. But, this work won't happen until field keyword feature is implemented, and how specifically it will be done is TBD.

Currently, a different backing field symbol can be created for each partial property part, which is used only for error recovery (e.g. when an initializer is used on the property.)


if (PartialImplementationPart is { } implementationPart)
{
Expand Down Expand Up @@ -610,6 +612,7 @@ internal override void AfterAddingTypeMembersChecks(ConversionsBase conversions,
if (IsPartialDefinition && OtherPartOfPartial is { } implementation)
{
PartialPropertyChecks(implementation, diagnostics);
implementation.CheckInitializerIfNeeded(diagnostics);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ protected SourcePropertySymbolBase(
{
Debug.Assert(!isExpressionBodied || !isAutoProperty);
Debug.Assert(!isExpressionBodied || !hasInitializer);
Debug.Assert(!isExpressionBodied || accessorsHaveImplementation); // PROTOTYPE(partial-properties): further adjust asserts?
Debug.Assert(!isExpressionBodied || accessorsHaveImplementation);
Debug.Assert((modifiers & DeclarationModifiers.Required) == 0 || this is SourcePropertySymbol);

_syntaxRef = syntax.GetReference();
Expand Down Expand Up @@ -279,20 +279,20 @@ explicitlyImplementedProperty is null ?
internal bool IsExpressionBodied
=> (_propertyFlags & Flags.IsExpressionBodied) != 0;

private void CheckInitializer(
bool isAutoProperty,
bool isInterface,
bool isStatic,
Location location,
BindingDiagnosticBag diagnostics)
protected void CheckInitializerIfNeeded(BindingDiagnosticBag diagnostics)
{
if (isInterface && !isStatic)
if ((_propertyFlags & Flags.HasInitializer) == 0)
{
diagnostics.Add(ErrorCode.ERR_InstancePropertyInitializerInInterface, location);
return;
}
else if (!isAutoProperty)

if (ContainingType.IsInterface && !IsStatic)
{
diagnostics.Add(ErrorCode.ERR_InitializerOnNonAutoProperty, location);
diagnostics.Add(ErrorCode.ERR_InstancePropertyInitializerInInterface, Location);
}
else if (!IsAutoProperty)
{
diagnostics.Add(ErrorCode.ERR_InitializerOnNonAutoProperty, Location);
}
}

Expand Down Expand Up @@ -692,11 +692,7 @@ internal override void AfterAddingTypeMembersChecks(ConversionsBase conversions,
this.CheckAccessibility(Location, diagnostics, isExplicitInterfaceImplementation);
this.CheckModifiers(isExplicitInterfaceImplementation, Location, IsIndexer, diagnostics);

bool hasInitializer = (_propertyFlags & Flags.HasInitializer) != 0;
if (hasInitializer)
{
CheckInitializer(IsAutoProperty, ContainingType.IsInterface, IsStatic, Location, diagnostics);
}
CheckInitializerIfNeeded(diagnostics);

if (RefKind != RefKind.None && IsRequired)
{
Expand Down Expand Up @@ -1109,20 +1105,23 @@ private CustomAttributesBag<CSharpAttributeData> GetAttributesBag()
// prevent infinite recursion:
Debug.Assert(!ReferenceEquals(copyFrom, this));

// The property is responsible for completion of the backing field
// NB: when the **field keyword feature** is implemented, it's possible that synthesized field symbols will also be merged or shared between partial property parts.
// If we do that then this check should possibly be moved, and asserts adjusted accordingly.
_ = BackingField?.GetAttributes();
Comment on lines +1108 to +1111
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cston This comment and the below assert are likely relevant for the field keyword feature.


bool bagCreatedOnThisThread;
if (copyFrom is not null)
{
// When partial properties get the ability to have a backing field,
// the implementer will have to decide how the BackingField symbol works in 'copyFrom' scenarios.
Debug.Assert(BackingField is null);
Debug.Assert(!IsAutoProperty);

var attributesBag = copyFrom.GetAttributesBag();
bagCreatedOnThisThread = Interlocked.CompareExchange(ref _lazyCustomAttributesBag, attributesBag, null) == null;
}
else
{
// The property is responsible for completion of the backing field
_ = BackingField?.GetAttributes();
bagCreatedOnThisThread = LoadAndValidateAttributes(GetAttributeDeclarations(), ref _lazyCustomAttributesBag);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11220,6 +11220,45 @@ void local1()
Assert.True(verifier.HasLocalsInit("C.<get_PropNoAttribute>g__local1|3_0"));
}

[Theory]
[InlineData("[SkipLocalsInit]", "")]
[InlineData("", "[SkipLocalsInit]")]
public void SkipLocalsInit_PartialPropertyAccessor_ContainsLocalFunction(string defAttrs, string implAttrs)
{
// SkipLocalsInit applied to either part affects the property and nested functions
var source = $$"""
using System.Runtime.CompilerServices;

public partial class C
{
{{defAttrs}}
partial int PropWithAttribute { get; }

{{implAttrs}}
partial int PropWithAttribute
{
get
{
int w = 1;
w = w + w + w + w;

void local1()
{
int x = 1;
x = x + x + x + x;
}

return 0;
}
}
}
""";

var verifier = CompileAndVerifyWithSkipLocalsInit(source);
Assert.False(verifier.HasLocalsInit("C.PropWithAttribute.get"));
Assert.False(verifier.HasLocalsInit("C.<get_PropWithAttribute>g__local1|1_0"));
}

[Fact]
public void SkipLocalsInit_EventAccessor_ContainsLocalFunction()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1972,7 +1972,7 @@ void verify(CSharpTestSource source)
Assert.Equal("p2", indexer.Parameters.Single().Name);
});
verifier.VerifyDiagnostics(
// (5,24): warning CS9308: Partial property declarations 'int C.this[int p2]' and 'int C.this[int p1]' have signature differences.
// (5,24): warning CS9256: Partial property declarations 'int C.this[int p2]' and 'int C.this[int p1]' have signature differences.
// public partial int this[int p1] => 42;
Diagnostic(ErrorCode.WRN_PartialPropertySignatureDifference, "this").WithArguments("int C.this[int p2]", "int C.this[int p1]").WithLocation(5, 24));

Expand Down Expand Up @@ -2033,7 +2033,7 @@ void verify(CSharpTestSource source)
// (4,42): warning CS1734: XML comment on 'C.this[int]' has a paramref tag for 'p2', but there is no parameter by that name
// /** <summary>Accepts <paramref name="p2"/>.</summary> */
Diagnostic(ErrorCode.WRN_UnmatchedParamRefTag, "p2").WithArguments("p2", "C.this[int]").WithLocation(4, 42),
// (5,24): warning CS9308: Partial property declarations 'int C.this[int p2]' and 'int C.this[int p1]' have signature differences.
// (5,24): warning CS9256: Partial property declarations 'int C.this[int p2]' and 'int C.this[int p1]' have signature differences.
// public partial int this[int p1] => 42;
Diagnostic(ErrorCode.WRN_PartialPropertySignatureDifference, "this").WithArguments("int C.this[int p2]", "int C.this[int p1]").WithLocation(5, 24));

Expand Down Expand Up @@ -2094,7 +2094,7 @@ void verify(CSharpTestSource source)
Assert.Equal("p2", indexer.Parameters.Single().Name);
});
verifier.VerifyDiagnostics(
// (4,24): warning CS9308: Partial property declarations 'int C.this[int p2]' and 'int C.this[int p1]' have signature differences.
// (4,24): warning CS9256: Partial property declarations 'int C.this[int p2]' and 'int C.this[int p1]' have signature differences.
// public partial int this[int p1] => 42;
Diagnostic(ErrorCode.WRN_PartialPropertySignatureDifference, "this").WithArguments("int C.this[int p2]", "int C.this[int p1]").WithLocation(4, 24),
// (4,42): warning CS1734: XML comment on 'C.this[int]' has a paramref tag for 'p1', but there is no parameter by that name
Expand Down Expand Up @@ -2158,7 +2158,7 @@ void verify(CSharpTestSource source)
Assert.Equal("p2", indexer.Parameters.Single().Name);
});
verifier.VerifyDiagnostics(
// (4,24): warning CS9308: Partial property declarations 'int C.this[int p2]' and 'int C.this[int p1]' have signature differences.
// (4,24): warning CS9256: Partial property declarations 'int C.this[int p2]' and 'int C.this[int p1]' have signature differences.
// public partial int this[int p1] => 42;
Diagnostic(ErrorCode.WRN_PartialPropertySignatureDifference, "this").WithArguments("int C.this[int p2]", "int C.this[int p1]").WithLocation(4, 24));

Expand Down
Loading
Loading