Skip to content

Commit

Permalink
Don't wrap fields in ValueTuple definitions (#44231)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jcouv
jcouv authored Mar 26, 2021
1 parent 8c53fb7 commit f23a180
Show file tree
Hide file tree
Showing 25 changed files with 1,544 additions and 320 deletions.
33 changes: 17 additions & 16 deletions src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -554,24 +554,25 @@ private void CompileNamedType(NamedTypeSymbol containingType)

case SymbolKind.Field:
{
var field = (FieldSymbol)member;
var fieldSymbol = (field.TupleUnderlyingField ?? field) as SourceMemberFieldSymbol;
if ((object)fieldSymbol != null)
var fieldSymbol = (FieldSymbol)member;
if (member is TupleErrorFieldSymbol)
{
if (fieldSymbol.IsConst)
{
// We check specifically for constant fields with bad values because they never result
// in bound nodes being inserted into method bodies (in which case, they would be covered
// by the method-level check).
ConstantValue constantValue = fieldSymbol.GetConstantValue(ConstantFieldsInProgress.Empty, earlyDecodingWellKnownAttributes: false);
SetGlobalErrorIfTrue(constantValue == null || constantValue.IsBad);
}
break;
}

if (fieldSymbol.IsFixedSizeBuffer && compilationState.Emitting)
{
// force the generation of implementation types for fixed-size buffers
TypeSymbol discarded = fieldSymbol.FixedImplementationType(compilationState.ModuleBuilderOpt);
}
if (fieldSymbol.IsConst)
{
// We check specifically for constant fields with bad values because they never result
// in bound nodes being inserted into method bodies (in which case, they would be covered
// by the method-level check).
ConstantValue constantValue = fieldSymbol.GetConstantValue(ConstantFieldsInProgress.Empty, earlyDecodingWellKnownAttributes: false);
SetGlobalErrorIfTrue(constantValue == null || constantValue.IsBad);
}

if (fieldSymbol.IsFixedSizeBuffer && compilationState.Emitting)
{
// force the generation of implementation types for fixed-size buffers
TypeSymbol discarded = fieldSymbol.FixedImplementationType(compilationState.ModuleBuilderOpt);
}
break;
}
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Emitter/Model/NamedTypeSymbolAdapter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,7 @@ Cci.ITypeReference Cci.ITypeDefinition.GetBaseClass(EmitContext context)
foreach (var f in AdaptedNamedTypeSymbol.GetFieldsToEmit())
{
Debug.Assert((object)(f.TupleUnderlyingField ?? f) == f);
Debug.Assert(!(f is TupleErrorFieldSymbol));
if (isStruct || f.GetCciAdapter().ShouldInclude(context))
{
yield return f.GetCciAdapter();
Expand Down
16 changes: 10 additions & 6 deletions src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -276,12 +276,14 @@ private void ValidateReferencedAssembly(AssemblySymbol assembly, AssemblyReferen
AddSymbolLocation(result, member);
break;
case SymbolKind.Field:
// NOTE: Dev11 does not add synthesized backing fields for properties,
// but adds backing fields for events, Roslyn adds both
if (member is TupleErrorFieldSymbol)
{
var field = (FieldSymbol)member;
AddSymbolLocation(result, field.TupleUnderlyingField ?? field);
break;
}

// NOTE: Dev11 does not add synthesized backing fields for properties,
// but adds backing fields for events, Roslyn adds both
AddSymbolLocation(result, member);
break;

case SymbolKind.Event:
Expand All @@ -291,7 +293,7 @@ private void ValidateReferencedAssembly(AssemblySymbol assembly, AssemblyReferen
FieldSymbol field = ((EventSymbol)member).AssociatedField;
if ((object)field != null)
{
AddSymbolLocation(result, field.TupleUnderlyingField ?? field);
AddSymbolLocation(result, field);
}
}
break;
Expand Down Expand Up @@ -1028,7 +1030,9 @@ internal Cci.IFieldReference Translate(
bool needDeclaration = false)
{
Debug.Assert(fieldSymbol.IsDefinitionOrDistinct());
Debug.Assert(!fieldSymbol.IsVirtualTupleField && (object)(fieldSymbol.TupleUnderlyingField ?? fieldSymbol) == fieldSymbol, "tuple fields should be rewritten to underlying by now");
Debug.Assert(!fieldSymbol.IsVirtualTupleField &&
(object)(fieldSymbol.TupleUnderlyingField ?? fieldSymbol) == fieldSymbol &&
fieldSymbol is not TupleErrorFieldSymbol, "tuple fields should be rewritten to underlying by now");

if (!fieldSymbol.IsDefinition)
{
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/FlowAnalysis/LocalDataFlowPass.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ protected virtual int GetOrCreateSlot(Symbol symbol, int containingSlot = 0, boo
/// </summary>
private int DescendThroughTupleRestFields(ref Symbol symbol, int containingSlot, bool forceContainingSlotsToExist)
{
if (symbol is TupleFieldSymbol fieldSymbol)
if (symbol is TupleElementFieldSymbol fieldSymbol)
{
TypeSymbol containingType = symbol.ContainingType;

Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ public PossiblyConditionalState Clone()
var output = new BoundDagTemp(e.Syntax, type.Type, e);
int outputSlot = -1;
var originalTupleElement = e.Input.IsOriginalInput && !originalInputElementSlots.IsDefault
? field as TupleFieldSymbol
? field
: null;
if (originalTupleElement is not null)
{
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -527,7 +527,7 @@ private void AddTupleTypeName(INamedTypeSymbol symbol)
}

VisitFieldType(element);
if (!element.IsImplicitlyDeclared)
if (element.IsExplicitlyNamedTupleElement)
{
AddSpace();
builder.Add(CreatePart(SymbolDisplayPartKind.FieldName, symbol, element.Name));
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/Compilation_WellKnownMembers.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ internal override ITypeSymbolInternal CommonGetWellKnownType(WellKnownType wellk
return GetWellKnownType(wellknownType);
}

internal static Symbol? GetRuntimeMember(NamedTypeSymbol declaringType, in MemberDescriptor descriptor, SignatureComparer<MethodSymbol, FieldSymbol, PropertySymbol, TypeSymbol, ParameterSymbol> comparer, AssemblySymbol accessWithinOpt)
internal static Symbol? GetRuntimeMember(NamedTypeSymbol declaringType, in MemberDescriptor descriptor, SignatureComparer<MethodSymbol, FieldSymbol, PropertySymbol, TypeSymbol, ParameterSymbol> comparer, AssemblySymbol? accessWithinOpt)
{
var members = declaringType.GetMembers(descriptor.Name);
return GetRuntimeMember(members, descriptor, comparer, accessWithinOpt);
Expand Down
23 changes: 22 additions & 1 deletion src/Compilers/CSharp/Portable/Symbols/FieldSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,16 @@ public virtual bool IsDefaultTupleElement
{
get
{
Debug.Assert(!(this is TupleElementFieldSymbol or TupleErrorFieldSymbol));
return TupleElementIndex >= 0;
}
}

public virtual bool IsExplicitlyNamedTupleElement
{
get
{
Debug.Assert(!(this is TupleElementFieldSymbol or TupleErrorFieldSymbol));
return false;
}
}
Expand All @@ -418,6 +428,7 @@ public virtual FieldSymbol TupleUnderlyingField
{
get
{
Debug.Assert(!(this is TupleElementFieldSymbol));
return ContainingType.IsTupleType ? this : null;
}
}
Expand All @@ -430,7 +441,8 @@ public virtual FieldSymbol CorrespondingTupleField
{
get
{
return null;
Debug.Assert(!(this is TupleElementFieldSymbol));
return TupleElementIndex >= 0 ? this : null;
}
}

Expand All @@ -451,6 +463,15 @@ public virtual int TupleElementIndex
{
get
{
// wrapped tuple fields already have this information and override this property
Debug.Assert(!(this is TupleElementFieldSymbol or TupleErrorFieldSymbol));

var map = ContainingType.TupleFieldDefinitionsToIndexMap;
if (map is object && map.TryGetValue(this.OriginalDefinition, out int index))
{
return index;
}

return -1;
}
}
Expand Down
7 changes: 3 additions & 4 deletions src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -889,7 +889,7 @@ internal override IEnumerable<FieldSymbol> GetFieldsToEmit()
else
{
// If there are any non-event fields, they are at the very beginning.
IEnumerable<FieldSymbol> nonEventFields = GetMembers<FieldSymbol>(this.GetMembers(), SymbolKind.Field, offset: 0).Select(f => f.TupleUnderlyingField ?? f);
IEnumerable<FieldSymbol> nonEventFields = GetMembers<FieldSymbol>(this.GetMembers().WhereAsArray(m => !(m is TupleErrorFieldSymbol)), SymbolKind.Field, offset: 0);

// Event backing fields are not part of the set returned by GetMembers. Let's add them manually.
ArrayBuilder<FieldSymbol> eventFields = null;
Expand All @@ -900,9 +900,6 @@ internal override IEnumerable<FieldSymbol> GetFieldsToEmit()
if ((object)associatedField != null)
{
Debug.Assert((object)associatedField.AssociatedSymbol != null);

associatedField = associatedField.TupleUnderlyingField ?? associatedField;

Debug.Assert(!nonEventFields.Contains(associatedField));

if (eventFields == null)
Expand Down Expand Up @@ -1320,7 +1317,9 @@ private void LoadMembers()

if (IsTupleType)
{
int originalCount = members.Count;
members = AddOrWrapTupleMembers(members.ToImmutableAndFree());
membersCount += (members.Count - originalCount); // account for added tuple error fields
Debug.Assert(members is object);
}

Expand Down
8 changes: 8 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/PublicModel/FieldSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,14 @@ IFieldSymbol IFieldSymbol.CorrespondingTupleField
}
}

bool IFieldSymbol.IsExplicitlyNamedTupleElement
{
get
{
return _underlying.IsExplicitlyNamedTupleElement;
}
}

bool IFieldSymbol.IsConst => _underlying.IsConst;

bool IFieldSymbol.IsReadOnly => _underlying.IsReadOnly;
Expand Down
14 changes: 14 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Retargeting/RetargetingNamedTypeSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,20 @@ public override ImmutableArray<Symbol> GetMembers(string name)
return this.RetargetingTranslator.Retarget(_underlyingType.GetMembers(name));
}

public override void InitializeTupleFieldDefinitionsToIndexMap()
{
Debug.Assert(this.IsTupleType);
Debug.Assert(this.IsDefinition); // we only store a map for definitions

var retargetedMap = new SmallDictionary<FieldSymbol, int>(ReferenceEqualityComparer.Instance);
foreach ((FieldSymbol field, int index) in _underlyingType.TupleFieldDefinitionsToIndexMap)
{
retargetedMap.Add(this.RetargetingTranslator.Retarget(field), index);
}

this.TupleData!.SetFieldDefinitionsToIndexMap(retargetedMap);
}

internal override IEnumerable<FieldSymbol> GetFieldsToEmit()
{
foreach (FieldSymbol f in _underlyingType.GetFieldsToEmit())
Expand Down
24 changes: 22 additions & 2 deletions src/Compilers/CSharp/Portable/Symbols/Retargeting/RetargetingSymbolTranslator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ internal partial class RetargetingModuleSymbol
private readonly Func<Symbol, RetargetingNamespaceSymbol> _createRetargetingNamespace;
private readonly Func<Symbol, RetargetingTypeParameterSymbol> _createRetargetingTypeParameter;
private readonly Func<Symbol, RetargetingNamedTypeSymbol> _createRetargetingNamedType;
private readonly Func<Symbol, RetargetingFieldSymbol> _createRetargetingField;
private readonly Func<Symbol, FieldSymbol> _createRetargetingField;
private readonly Func<Symbol, RetargetingPropertySymbol> _createRetargetingProperty;
private readonly Func<Symbol, RetargetingEventSymbol> _createRetargetingEvent;

Expand All @@ -57,9 +57,29 @@ private RetargetingNamedTypeSymbol CreateRetargetingNamedType(Symbol symbol)
return new RetargetingNamedTypeSymbol(this, (NamedTypeSymbol)symbol);
}

private RetargetingFieldSymbol CreateRetargetingField(Symbol symbol)
private FieldSymbol CreateRetargetingField(Symbol symbol)
{
Debug.Assert(ReferenceEquals(symbol.ContainingModule, _underlyingModule));
if (symbol is TupleErrorFieldSymbol tupleErrorField)
{
var correspondingTupleField = tupleErrorField.CorrespondingTupleField;
Debug.Assert(correspondingTupleField is TupleErrorFieldSymbol);

var retargetedCorrespondingDefaultFieldOpt = (correspondingTupleField == (object)tupleErrorField)
? null
: (TupleErrorFieldSymbol)RetargetingTranslator.Retarget(correspondingTupleField);

return new TupleErrorFieldSymbol(
RetargetingTranslator.Retarget(tupleErrorField.ContainingType, RetargetOptions.RetargetPrimitiveTypesByName),
tupleErrorField.Name,
tupleErrorField.TupleElementIndex,
tupleErrorField.Locations.IsEmpty ? null : tupleErrorField.Locations[0],
this.RetargetingTranslator.Retarget(tupleErrorField.TypeWithAnnotations, RetargetOptions.RetargetPrimitiveTypesByTypeCode),
tupleErrorField.GetUseSiteInfo().DiagnosticInfo,
tupleErrorField.IsImplicitlyDeclared,
retargetedCorrespondingDefaultFieldOpt);
}

return new RetargetingFieldSymbol(this, (FieldSymbol)symbol);
}

Expand Down
17 changes: 7 additions & 10 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1350,14 +1350,18 @@ internal override IEnumerable<FieldSymbol> GetFieldsToEmit()
switch (m.Kind)
{
case SymbolKind.Field:
var field = (FieldSymbol)m;
yield return field.TupleUnderlyingField ?? field;
if (m is TupleErrorFieldSymbol)
{
break;
}

yield return (FieldSymbol)m;
break;
case SymbolKind.Event:
FieldSymbol? associatedField = ((EventSymbol)m).AssociatedField;
if ((object?)associatedField != null)
{
yield return associatedField.TupleUnderlyingField ?? associatedField;
yield return associatedField;
}
break;
}
Expand Down Expand Up @@ -1499,13 +1503,6 @@ internal void AssertMemberExposure(Symbol member, bool forDiagnostics = false)
return;
}

if (member is FieldSymbol && this.IsTupleType)
{
Debug.Assert(forDiagnostics);
return; // There are dangling tuple elements, probably related to https://github.com/dotnet/roslyn/issues/43597
// and will be addressed by a pending PR https://github.com/dotnet/roslyn/pull/44231.
}

Debug.Assert(false, "Premature symbol exposure.");
}

Expand Down
20 changes: 4 additions & 16 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1290,24 +1290,12 @@ public sealed override bool AreLocalsZeroed

private bool HasInstanceFields()
{
var members = this.GetMembersUnordered();
for (var i = 0; i < members.Length; i++)
var fields = this.GetFieldsToEmit();
foreach (var field in fields)
{
var m = members[i];
if (!m.IsStatic)
if (!field.IsStatic)
{
switch (m.Kind)
{
case SymbolKind.Field:
return true;

case SymbolKind.Event:
if (((EventSymbol)m).AssociatedField != null)
{
return true;
}
break;
}
return true;
}
}

Expand Down
8 changes: 8 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Tuples/TupleErrorFieldSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,14 @@ public override bool IsDefaultTupleElement
}
}

public override bool IsExplicitlyNamedTupleElement
{
get
{
return _tupleElementIndex >= 0 && !_isImplicitlyDeclared;
}
}

public override FieldSymbol TupleUnderlyingField
{
get
Expand Down
Loading

0 comments on commit f23a180

Please sign in to comment.