Browse Source

Fix #3568: read a record's member order off its generated members

The order of a record's fields and properties has to be known, because
Equals, GetHashCode, PrintMembers and the copy constructor are recognised
by walking their bodies in lockstep with it. It was assumed to be every
property followed by every field, so a record that declares a field
before a property desynchronised all four at once: none was recognised as
generated, all of them were emitted, and the auto-properties lost their
backing fields to raw <Property>k__BackingField accesses - output that
does not compile.

The order is in the generated members themselves, but no single one has
all of it: Equals compares everything that carries state and never a
computed property, PrintMembers prints everything public and never a
private field. Both follow declaration order, so the two sequences are
merged along the members they share, which puts a private field and a
computed property back in the right places relative to each other.
Members neither of them mentions - EqualityContract, static members -
keep the position they had.

Where the two orders conflict, which can only happen for a member that
one of them never sees, the equality order wins; nothing in the metadata
says more, and the choice cannot change more than the order the members
are printed in.

Assisted-by: Claude:claude-opus-5:Claude Code
pull/4111/head
Siegfried Pammer 2 weeks ago
parent
commit
0e6b9a7e26
  1. 34
      ICSharpCode.Decompiler.Tests/TestCases/Pretty/Records.cs
  2. 201
      ICSharpCode.Decompiler/CSharp/RecordDecompiler.cs

34
ICSharpCode.Decompiler.Tests/TestCases/Pretty/Records.cs

@ -243,6 +243,40 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty @@ -243,6 +243,40 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
private string? WebValue2;
}
public record FieldBeforeProperty(int ID, string Text)
{
public int Field;
public int Property { get; set; }
}
public record FieldsAndPropertiesInterleaved(int ID)
{
public int First;
public int Middle { get; set; }
public int Last;
}
public record PrivateFieldAndComputedProperty(int ID)
{
public int PublicField;
private int privateField;
public int Computed => privateField + PublicField;
public int Auto { get; set; }
}
public record DerivedWithInterleavedMembers(int B) : Base(B.ToString())
{
public int Field;
public int Property { get; set; }
}
}
#if CS100

201
ICSharpCode.Decompiler/CSharp/RecordDecompiler.cs

@ -375,18 +375,207 @@ namespace ICSharpCode.Decompiler.CSharp @@ -375,18 +375,207 @@ namespace ICSharpCode.Decompiler.CSharp
}
}
static List<IMember> DetectMemberOrder(ITypeDefinition recordTypeDef, Dictionary<IField, IProperty> backingFieldToAutoProperty)
List<IMember> DetectMemberOrder(ITypeDefinition recordTypeDef, Dictionary<IField, IProperty> backingFieldToAutoProperty)
{
// For records, the order of members is important:
// Equals/GetHashCode/PrintMembers must agree on an order of fields+properties.
// The IL metadata has the order of fields and the order of properties, but we
// need to detect the correct interleaving.
// We could try to detect this from the PrintMembers body, but let's initially
// restrict ourselves to the common case where the record only uses properties.
// The IL metadata has the order of fields and the order of properties, but not the
// interleaving of the two, so it is read back out of the members the compiler generated
// from the declaration order (issue #3568).
var subst = recordTypeDef.AsParameterizedType().GetSubstitution();
return recordTypeDef.Properties.Select(p => p.Specialize(subst)).Concat(
var allMembers = recordTypeDef.Properties.Select(p => p.Specialize(subst)).Concat(
recordTypeDef.Fields.Select(f => (IField)f.Specialize(subst)).Where(f => !backingFieldToAutoProperty.ContainsKey(f))
).ToList();
// Equals compares every member that carries state, in declaration order, and skips a
// property without a backing field; PrintMembers prints every public member, in the same
// order, and skips a private one. Neither alone sees all of them, so the two sequences
// are merged along the members they share.
var equalityOrder = DetectOrderFromEquals(allMembers);
var printOrder = DetectOrderFromPrintMembers(allMembers);
var declarationOrder = equalityOrder != null && printOrder != null
? MergeAlongCommonMembers(equalityOrder, printOrder)
: equalityOrder ?? printOrder;
if (declarationOrder == null)
{
// Nothing to read the order from: the properties-first order is right for a record
// that uses only properties, which is the common case.
return allMembers;
}
// A generated member mentions neither EqualityContract, nor a static member, nor a
// private one that carries no state. Those keep the position they had, and the members
// whose order was read off fill the positions that are left, in that order.
var ordered = new List<IMember>(allMembers.Count);
int next = 0;
foreach (var member in allMembers)
{
if (declarationOrder.Contains(member))
ordered.Add(declarationOrder[next++]);
else
ordered.Add(member);
}
return ordered;
}
/// <summary>
/// Merges two orderings of overlapping subsets into one. Members the two disagree about -
/// which can only be members that appear in one of them - are ordered by
/// <paramref name="first"/>, whose sequence is the one the equality members follow.
/// </summary>
static List<IMember> MergeAlongCommonMembers(List<IMember> first, List<IMember> second)
{
var result = new List<IMember>();
int i = 0, j = 0;
while (i < first.Count && j < second.Count)
{
if (first[i].Equals(second[j]))
{
result.Add(first[i]);
i++;
j++;
}
else if (!second.Skip(j).Contains(first[i]))
{
// Only the first sequence has this one, so it goes here.
result.Add(first[i]);
i++;
}
else if (!first.Skip(i).Contains(second[j]))
{
result.Add(second[j]);
j++;
}
else
{
// Both sequences still hold both members, in opposite orders. Only one of the
// two can be right and nothing here says which, so the equality order wins.
result.Add(first[i]);
i++;
}
}
foreach (var member in first.Skip(i).Concat(second.Skip(j)))
{
if (!result.Contains(member))
result.Add(member);
}
return result;
}
/// <summary>
/// The members compared by the generated Equals, in the order it compares them.
/// </summary>
List<IMember>? DetectOrderFromEquals(List<IMember> allMembers)
{
var equalsMethod = recordTypeDef.GetMethods(
m => m.Name == "Equals" && m.Parameters.Count == 1 && !m.IsStatic
&& IsRecordType(m.Parameters[0].Type),
GetMemberOptions.IgnoreInheritedMembers).FirstOrDefault();
if (equalsMethod == null)
return null;
var body = DecompileBody(equalsMethod);
if (body == null || body.Instructions.Count == 0)
return null;
if (!body.Instructions[0].MatchReturn(out var returnValue))
return null;
if (returnValue.MatchLogicOr(out _, out var rhs))
{
// this == other || ...
returnValue = rhs;
}
var order = new List<IMember>();
foreach (var condition in UnpackLogicAndChain(returnValue))
{
// callvirt Equals(call get_Default(), ldfld <A>k__BackingField(ldloc this), ...)
if (condition is not CallVirt { Method: { Name: "Equals" } } equalsCall)
continue;
if (equalsCall.Arguments.Count != 3)
continue;
if (!MatchMemberAccessOnThis(equalsCall.Arguments[1], allMembers, out var member))
continue;
if (!order.Contains(member))
order.Add(member);
}
return order.Count > 0 ? order : null;
}
/// <summary>
/// The members printed by the generated PrintMembers, in the order it prints them. The names
/// come from the string constants it appends, which say "Name = " and ", Name = ".
/// </summary>
List<IMember>? DetectOrderFromPrintMembers(List<IMember> allMembers)
{
var printMembers = recordTypeDef.GetMethods(
m => m.Name == "PrintMembers" && m.Parameters.Count == 1 && !m.IsStatic,
GetMemberOptions.IgnoreInheritedMembers).FirstOrDefault();
if (printMembers == null)
return null;
var body = DecompileBody(printMembers);
if (body == null)
return null;
var function = body.Ancestors.OfType<ILFunction>().SingleOrDefault();
var builder = function?.Variables.SingleOrDefault(
v => v.Kind == VariableKind.Parameter && v.Index == 0);
if (builder == null)
return null;
var order = new List<IMember>();
// The name and the " = " after it are one constant for a current compiler and separate
// appends for an older one, so consecutive constants are joined before being read.
string? pending = null;
foreach (var instruction in body.Instructions)
{
if (MatchStringBuilderAppend(instruction, builder, out var value)
&& value.MatchLdStr(out string? text))
{
pending += text;
continue;
}
if (pending != null && !AddMemberNamedBy(pending))
return null;
pending = null;
}
if (pending != null && !AddMemberNamedBy(pending))
return null;
return order.Count > 0 ? order : null;
bool AddMemberNamedBy(string text)
{
if (!text.EndsWith(" = ", StringComparison.Ordinal))
return true; // the separator alone, or a constant that names nothing
string name = text.Substring(0, text.Length - " = ".Length);
if (name.StartsWith(", ", StringComparison.Ordinal))
name = name.Substring(2);
var member = allMembers.FirstOrDefault(m => m.Name == name);
if (member == null)
return false;
if (!order.Contains(member))
order.Add(member);
return true;
}
}
/// <summary>
/// The member a generated body reads off "this": a field directly, or the property a
/// backing field belongs to.
/// </summary>
bool MatchMemberAccessOnThis(ILInstruction inst, List<IMember> allMembers,
[NotNullWhen(true)] out IMember? member)
{
member = null;
if (inst.MatchLdFld(out var target, out var field) || inst.MatchLdFlda(out target, out field))
{
if (!target.MatchLdThis())
return false;
if (backingFieldToAutoProperty.TryGetValue(field, out var property))
member = property;
else
member = allMembers.FirstOrDefault(m => m.Equals(field));
}
else if (inst is CallInstruction { Arguments: { Count: 1 } } getterCall
&& getterCall.Arguments[0].MatchLdThis())
{
member = allMembers.OfType<IProperty>().FirstOrDefault(
p => getterCall.Method.Equals(p.Getter));
}
return member != null;
}
/// <summary>

Loading…
Cancel
Save