Skip to content

Partial properties: duplicate membersByName before merging accessors #74969

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 3, 2024
Merged

Partial properties: duplicate membersByName before merging accessors #74969

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
49 changes: 18 additions & 31 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3668,12 +3668,7 @@ void mergePartialMethods(ref Dictionary<ReadOnlyMemory<char>, ImmutableArray<Sym
}
else
{
if ((object)membersByName == _lazyEarlyAttributeDecodingMembersDictionary)
{
// Avoid mutating the cached dictionary and especially avoid doing this possibly on multiple threads in parallel.
membersByName = new Dictionary<ReadOnlyMemory<char>, ImmutableArray<Symbol>>(membersByName, ReadOnlyMemoryOfCharComparer.Instance);
}

DuplicateMembersByNameIfCached(ref membersByName);
membersByName[name] = FixPartialMember(membersByName[name], prevMethod, currentMethod);
}
}
Expand All @@ -3692,40 +3687,23 @@ void mergePartialProperties(ref Dictionary<ReadOnlyMemory<char>, ImmutableArray<
}
else
{
var (currentGet, prevGet) = ((SourcePropertyAccessorSymbol?)currentProperty.GetMethod, (SourcePropertyAccessorSymbol?)prevProperty.GetMethod);
if (currentGet != null || prevGet != null)
{
var accessorName = (currentGet ?? prevGet)!.Name.AsMemory();
mergeAccessors(ref membersByName, accessorName, currentGet, prevGet);
}

var (currentSet, prevSet) = ((SourcePropertyAccessorSymbol?)currentProperty.SetMethod, (SourcePropertyAccessorSymbol?)prevProperty.SetMethod);
if (currentSet != null || prevSet != null)
{
var accessorName = (currentSet ?? prevSet)!.Name.AsMemory();
mergeAccessors(ref membersByName, accessorName, currentSet, prevSet);
}

if ((object)membersByName == _lazyEarlyAttributeDecodingMembersDictionary)
{
// Avoid mutating the cached dictionary and especially avoid doing this possibly on multiple threads in parallel.
membersByName = new Dictionary<ReadOnlyMemory<char>, ImmutableArray<Symbol>>(membersByName, ReadOnlyMemoryOfCharComparer.Instance);
}

DuplicateMembersByNameIfCached(ref membersByName);
Copy link
Contributor Author

@cston cston Aug 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Previously, membersByName was modified in mergeAccessors before duplicating.

mergeAccessors(ref membersByName, (SourcePropertyAccessorSymbol?)currentProperty.GetMethod, (SourcePropertyAccessorSymbol?)prevProperty.GetMethod);
mergeAccessors(ref membersByName, (SourcePropertyAccessorSymbol?)currentProperty.SetMethod, (SourcePropertyAccessorSymbol?)prevProperty.SetMethod);
membersByName[name] = FixPartialMember(membersByName[name], prevProperty, currentProperty);
}

void mergeAccessors(ref Dictionary<ReadOnlyMemory<char>, ImmutableArray<Symbol>> membersByName, ReadOnlyMemory<char> name, SourcePropertyAccessorSymbol? currentAccessor, SourcePropertyAccessorSymbol? prevAccessor)
void mergeAccessors(ref Dictionary<ReadOnlyMemory<char>, ImmutableArray<Symbol>> membersByName, SourcePropertyAccessorSymbol? currentAccessor, SourcePropertyAccessorSymbol? prevAccessor)
Copy link
Member

Choose a reason for hiding this comment

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

It also seems like this function could take membersByName by value instead.

{
Debug.Assert(currentAccessor != null || prevAccessor != null);
if (currentAccessor != null && prevAccessor != null)
if (currentAccessor is { } && prevAccessor is { })
{
var name = currentAccessor.Name.AsMemory();
var implementationAccessor = currentProperty.IsPartialDefinition ? prevAccessor : currentAccessor;
membersByName[name] = Remove(membersByName[name], implementationAccessor);
}
else
else if (currentAccessor is { } || prevAccessor is { })
{
var (foundAccessor, containingProperty, otherProperty) = prevAccessor != null ? (prevAccessor, prevProperty, currentProperty) : (currentAccessor!, currentProperty, prevProperty);
var (foundAccessor, containingProperty, otherProperty) = prevAccessor is { } ? (prevAccessor, prevProperty, currentProperty) : (currentAccessor!, currentProperty, prevProperty);
// When an accessor is present on definition but not on implementation, the accessor is said to be missing on the implementation.
// When an accessor is present on implementation but not on definition, the accessor is said to be unexpected on the implementation.
var (errorCode, propertyToBlame) = foundAccessor.IsPartialDefinition
Expand All @@ -3737,6 +3715,15 @@ void mergeAccessors(ref Dictionary<ReadOnlyMemory<char>, ImmutableArray<Symbol>>
}
}

private void DuplicateMembersByNameIfCached(ref Dictionary<ReadOnlyMemory<char>, ImmutableArray<Symbol>> membersByName)
{
if ((object)membersByName == _lazyEarlyAttributeDecodingMembersDictionary)
{
// Avoid mutating the cached dictionary and especially avoid doing this possibly on multiple threads in parallel.
membersByName = new Dictionary<ReadOnlyMemory<char>, ImmutableArray<Symbol>>(membersByName, ReadOnlyMemoryOfCharComparer.Instance);
}
}

/// <summary>Links together the definition and implementation parts of a partial method. Returns a member list which has the implementation part removed.</summary>
private static ImmutableArray<Symbol> FixPartialMember(ImmutableArray<Symbol> symbols, SourceOrdinaryMethodSymbol part1, SourceOrdinaryMethodSymbol part2)
{
Expand Down