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

EnC: Implement support for partial properties #74494

Merged
merged 4 commits into from
Jul 29, 2024
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
11 changes: 10 additions & 1 deletion src/Compilers/CSharp/Portable/Symbols/PropertySymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,15 @@
using System.Collections.Immutable;
using System.Diagnostics;
using System.Linq;
using Microsoft.CodeAnalysis.Symbols;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp.Symbols
{
/// <summary>
/// Represents a property or indexer.
/// </summary>
internal abstract partial class PropertySymbol : Symbol
internal abstract partial class PropertySymbol : Symbol, IPropertySymbolInternal
{
/// <summary>
/// As a performance optimization, cache parameter types and refkinds - overload resolution uses them a lot.
Expand Down Expand Up @@ -321,6 +322,14 @@ internal virtual bool IsExplicitInterfaceImplementation
/// </remarks>
public abstract ImmutableArray<PropertySymbol> ExplicitInterfaceImplementations { get; }

#nullable enable
internal virtual PropertySymbol? PartialImplementationPart => null;
internal virtual PropertySymbol? PartialDefinitionPart => null;
tmat marked this conversation as resolved.
Show resolved Hide resolved

IPropertySymbolInternal? IPropertySymbolInternal.PartialImplementationPart => PartialImplementationPart;
IPropertySymbolInternal? IPropertySymbolInternal.PartialDefinitionPart => PartialDefinitionPart;
#nullable disable

/// <summary>
/// Gets the kind of this symbol.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,9 @@ ImmutableArray<CustomModifier> IPropertySymbol.RefCustomModifiers
RefKind IPropertySymbol.RefKind => _underlying.RefKind;

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

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

bool IPropertySymbol.IsPartialDefinition => (_underlying as SourcePropertySymbol)?.IsPartialDefinition ?? false;
#nullable disable
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ public override OneOrMany<SyntaxList<AttributeListSyntax>> GetAttributeDeclarati
// This is an error scenario (requires using a property initializer and field-targeted attributes on partial property implementation part).
|| this.BackingField is not null);

if (PartialImplementationPart is { } implementationPart)
if (SourcePartialImplementationPart is { } implementationPart)
{
return OneOrMany.Create(
((BasePropertyDeclarationSyntax)CSharpSyntaxNode).AttributeLists,
Expand All @@ -191,7 +191,7 @@ public override OneOrMany<SyntaxList<AttributeListSyntax>> GetAttributeDeclarati
}
}

protected override SourcePropertySymbolBase? BoundAttributesSource => PartialDefinitionPart;
protected override SourcePropertySymbolBase? BoundAttributesSource => SourcePartialDefinitionPart;

public override IAttributeTargetSymbol AttributesOwner => this;

Expand Down Expand Up @@ -727,9 +727,11 @@ private void PartialPropertyChecks(SourcePropertySymbol implementation, BindingD

internal bool IsPartialImplementation => IsPartial && (AccessorsHaveImplementation || HasExternModifier);

internal SourcePropertySymbol? PartialDefinitionPart => IsPartialImplementation ? OtherPartOfPartial : null;
internal SourcePropertySymbol? SourcePartialDefinitionPart => IsPartialImplementation ? OtherPartOfPartial : null;
internal SourcePropertySymbol? SourcePartialImplementationPart => IsPartialDefinition ? OtherPartOfPartial : null;

internal SourcePropertySymbol? PartialImplementationPart => IsPartialDefinition ? OtherPartOfPartial : null;
internal override PropertySymbol? PartialDefinitionPart => SourcePartialDefinitionPart;
internal override PropertySymbol? PartialImplementationPart => SourcePartialImplementationPart;

internal static void InitializePartialPropertyParts(SourcePropertySymbol definition, SourcePropertySymbol implementation)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2567,56 +2567,52 @@ .locals init ([unchanged] V_0,
[Fact]
public void PartialMethod()
{
var source =
@"partial class C
{
static partial void M1();
static partial void M2();
static partial void M3();
static partial void M1() { }
static partial void M2() { }
}";
var compilation0 = CreateCompilation(source, parseOptions: TestOptions.Regular.WithNoRefSafetyRulesAttribute(), options: TestOptions.DebugDll);
var compilation1 = compilation0.WithSource(source);

var bytes0 = compilation0.EmitToArray();
using var md0 = ModuleMetadata.CreateFromImage(bytes0);
var reader0 = md0.MetadataReader;

CheckNames(reader0, reader0.GetMethodDefNames(), "M1", "M2", ".ctor");

var method0 = compilation0.GetMember<MethodSymbol>("C.M2").PartialImplementationPart;
var method1 = compilation1.GetMember<MethodSymbol>("C.M2").PartialImplementationPart;

var generation0 = CreateInitialBaseline(compilation0,
md0,
EmptyLocalsProvider);

var diff1 = compilation1.EmitDifference(
generation0,
ImmutableArray.Create(SemanticEdit.Create(SemanticEditKind.Update, method0, method1)));

var methods = diff1.TestData.GetMethodsByName();
Assert.Equal(1, methods.Count);
Assert.True(methods.ContainsKey("C.M2()"));

using var md1 = diff1.GetMetadata();
var reader1 = md1.Reader;
var readers = new[] { reader0, reader1 };

EncValidation.VerifyModuleMvid(1, reader0, reader1);

CheckNames(readers, reader1.GetMethodDefNames(), "M2");
using var _ = new EditAndContinueTest(options: TestOptions.DebugDll)
.AddBaseline(
source: """
partial class C
{
static partial void M1();
static partial void M2();
static partial void M3();
static partial void M1() { }
static partial void M2() { }
}
""",
validator: v =>
{
v.VerifyMethodDefNames("M1", "M2", ".ctor");
})
.AddGeneration(
source: """
partial class C
{
static partial void M1();
static partial void M2();
static partial void M3();
static partial void M1() { }
static partial void M2() { }
}
""",
edits:
[
Edit(SemanticEditKind.Update, c => c.GetMember<IMethodSymbol>("C.M2").PartialImplementationPart),
],
validator: v =>
{
v.VerifyMethodDefNames("M2");

CheckEncLog(reader1,
Row(2, TableIndex.AssemblyRef, EditAndContinueOperation.Default),
Row(6, TableIndex.TypeRef, EditAndContinueOperation.Default),
Row(2, TableIndex.MethodDef, EditAndContinueOperation.Default));
v.VerifyEncLogDefinitions(
[
Row(2, TableIndex.MethodDef, EditAndContinueOperation.Default)
]);

CheckEncMap(reader1,
Handle(6, TableIndex.TypeRef),
Handle(2, TableIndex.MethodDef),
Handle(2, TableIndex.AssemblyRef));
v.VerifyEncMapDefinitions(
[
Handle(2, TableIndex.MethodDef)
]);
})
.Verify();
}

[WorkItem(60804, "https://github.com/dotnet/roslyn/issues/60804")]
Expand Down Expand Up @@ -2711,6 +2707,100 @@ .maxstack 8
.Verify();
}

[Fact]
public void PartialProperty()
{
using var _ = new EditAndContinueTest(options: TestOptions.DebugDll)
.AddBaseline(
source: """
partial class C
{
partial int P { get; }
partial int P => 1;
}
""",
validator: v =>
{
v.VerifyMethodDefNames("get_P", ".ctor");
})
.AddGeneration(
source: """
partial class C
{
[System.Obsolete]partial int P { get; }
partial int P => 1;
}
""",
edits:
[
Edit(SemanticEditKind.Update, c => c.GetMember<IPropertySymbol>("C.P").PartialImplementationPart),
],
validator: v =>
{
v.VerifyMethodDefNames();

v.VerifyEncLogDefinitions(
[
Row(1, TableIndex.Property, EditAndContinueOperation.Default),
Row(4, TableIndex.CustomAttribute, EditAndContinueOperation.Default),
Row(2, TableIndex.MethodSemantics, EditAndContinueOperation.Default)
]);

v.VerifyEncMapDefinitions(
[
Handle(4, TableIndex.CustomAttribute),
Handle(1, TableIndex.Property),
Handle(2, TableIndex.MethodSemantics)
]);
})
.Verify();
}

[Fact]
public void PartialProperty_Accessor()
{
using var _ = new EditAndContinueTest(options: TestOptions.DebugDll)
.AddBaseline(
source: """
partial class C
{
partial int P { get; }
partial int P => 1;
}
""",
validator: v =>
{
v.VerifyMethodDefNames("get_P", ".ctor");
})
.AddGeneration(
source: """
partial class C
{
partial int P { get; }
partial int P => 2;
}
""",
edits:
[
Edit(SemanticEditKind.Update, c => c.GetMember<IMethodSymbol>("C.get_P").PartialImplementationPart),
],
validator: v =>
{
v.VerifyMethodDefNames("get_P");

v.VerifyEncLogDefinitions(
[
Row(1, TableIndex.MethodDef, EditAndContinueOperation.Default)
]);

v.VerifyEncMapDefinitions(
[
Handle(1, TableIndex.MethodDef)
]);
})
.Verify();
}

[Fact]
public void Method_WithAttributes_Add()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -924,7 +924,7 @@ .maxstack 2
Assert.True(propDefinition.IsPartialDefinition);

var propImplementation = propDefinition.PartialImplementationPart!;
Assert.True(propImplementation.IsPartialImplementation);
Assert.True(propImplementation.IsPartialImplementation());

Assert.Same(propDefinition, propImplementation.PartialDefinitionPart);
Assert.Null(propImplementation.PartialImplementationPart);
Expand Down Expand Up @@ -998,7 +998,7 @@ .maxstack 1
Assert.True(propDefinition.IsPartialDefinition);

var propImplementation = propDefinition.PartialImplementationPart!;
Assert.True(propImplementation.IsPartialImplementation);
Assert.True(propImplementation.IsPartialImplementation());

Assert.Same(propDefinition, propImplementation.PartialDefinitionPart);
Assert.Null(propImplementation.PartialImplementationPart);
Expand Down Expand Up @@ -1084,7 +1084,7 @@ .maxstack 1
Assert.True(propDefinition.IsPartialDefinition);

var propImplementation = propDefinition.PartialImplementationPart!;
Assert.True(propImplementation.IsPartialImplementation);
Assert.True(propImplementation.IsPartialImplementation());

Assert.Same(propDefinition, propImplementation.PartialDefinitionPart);
Assert.Null(propImplementation.PartialImplementationPart);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -412,9 +412,8 @@ private void CalculateChanges(

var newMember = GetRequiredInternalSymbol(edit.NewSymbol);

// Partial methods are supplied as implementations but recorded
// Partial methods/properties/indexers are supplied as implementations but recorded
// internally as definitions since definitions are used in emit.
// https://github.com/dotnet/roslyn/issues/73772: should we also make sure to use the definition for a partial property?
if (newMember.Kind == SymbolKind.Method)
{
var newMethod = (IMethodSymbolInternal)newMember;
Expand All @@ -438,6 +437,16 @@ private void CalculateChanges(
updatedMethodsPerType.Add((oldMethod.PartialDefinitionPart ?? oldMethod, (IMethodSymbolInternal)newMember));
}
}
else if (newMember.Kind == SymbolKind.Property)
{
var newProperty = (IPropertySymbolInternal)newMember;

// Partial properties should be implementations, not definitions.
Debug.Assert(newProperty.PartialImplementationPart == null);
Debug.Assert(edit.OldSymbol == null || ((IPropertySymbol)edit.OldSymbol).PartialImplementationPart == null);

newMember = newProperty.PartialDefinitionPart ?? newMember;
}

AddContainingSymbolChanges(changesBuilder, newMember);

Expand Down
9 changes: 4 additions & 5 deletions src/Compilers/Core/Portable/Emit/SemanticEdit.cs
Original file line number Diff line number Diff line change
Expand Up @@ -142,15 +142,14 @@ public SemanticEdit(SemanticEditKind kind, ISymbol? oldSymbol, ISymbol? newSymbo
}
}

// https://github.com/dotnet/roslyn/issues/73772: should we also do this check for partial properties?
if (oldSymbol is IMethodSymbol { PartialImplementationPart: not null })
if (oldSymbol is IMethodSymbol { PartialImplementationPart: not null } or IPropertySymbol { PartialImplementationPart: not null })
{
throw new ArgumentException("Partial method implementation required", nameof(oldSymbol));
throw new ArgumentException("Partial member implementation required", nameof(oldSymbol));
}

if (newSymbol is IMethodSymbol { PartialImplementationPart: not null })
if (newSymbol is IMethodSymbol { PartialImplementationPart: not null } or IPropertySymbol { PartialImplementationPart: not null })
{
throw new ArgumentException("Partial method implementation required", nameof(newSymbol));
throw new ArgumentException("Partial member implementation required", nameof(newSymbol));
}

if (kind == SemanticEditKind.Delete && oldSymbol is not (IMethodSymbol or IPropertySymbol or IEventSymbol))
Expand Down
11 changes: 11 additions & 0 deletions src/Compilers/Core/Portable/Symbols/IPropertySymbolInternal.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

namespace Microsoft.CodeAnalysis.Symbols;

internal interface IPropertySymbolInternal : ISymbolInternal
{
IPropertySymbolInternal? PartialImplementationPart { get; }
IPropertySymbolInternal? PartialDefinitionPart { get; }
}
Loading