Implement definite assignment analysis for tuples #13107
Conversation
Fixes C# portion of dotnet#13046
|
@dotnet/roslyn-compiler - please review |
gafter
added
Area-Compilers
Feature Request
New Language Feature - Tuples
| // Do not report virtual tuple fields. | ||
| // They are additional aliases to the fields of the underlying struct or nested extensions. | ||
| // and as such are already accounted for via the nonvirtual fields. | ||
| if (field is TupleVirtualElementFieldSymbol) |
There was a problem hiding this comment.
field is TupleVirtualElementFieldSymbol [](start = 28, length = 39)
I think this test would be better placed inside ShouldIgnoreStructField
There was a problem hiding this comment.
Can you please make this the test a new internal virtual boolean property (FieldSymbol.IsVirtualTupleElement) rather than testing for particular implementation types? The latter makes the code more fragile as we refactor inheritance hierarchies, while the virtual property would make the dependency explicit.
In reply to: 74524647 [](ancestors = 74524647)
There was a problem hiding this comment.
ShouldIgnoreStructField is a method emulating old compiler behavior for not other reasosn than compat and only if _dev12CompilerCompatibility is set.
Ignoring virtual tuple fields has a different reason and is not conditional on a flag. I think we should not mix these.
In reply to: 74525116 [](ancestors = 74525116,74524647)
There was a problem hiding this comment.
Added a virtual property for the virtual field check.
In reply to: 74851677 [](ancestors = 74851677,74525116,74524647)
|
Other than the comment about the type test and virtual property, looks good. In reply to: 239323243 [](ancestors = 239323243) |
| containingSlot = GetOrCreateSlot(restField, containingSlot); | ||
| containingType = restField.Type.TupleUnderlyingTypeOrSelf(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Would it be worth extracting a helper method for use here and in VariableSlot above: int GetTupleFieldSlot(TupleFieldSymbol field, int containingSlot, bool createIfMissing)?
There was a problem hiding this comment.
extracted as
int DescendThroughTupleRestFields(ref Symbol symbol, int containingSlot, bool forceContainingSlotsToExist)
In reply to: 74526518 [](ancestors = 74526518)
| { | ||
| TypeSymbol containingType = ((TupleTypeSymbol)symbol.ContainingType).UnderlyingNamedType; | ||
|
|
||
| // for tuple fields the varible indentifier represents the underlying field |
|
LGTM |
| @@ -700,6 +705,52 @@ protected int GetOrCreateSlot(Symbol symbol, int containingSlot = 0) | |||
| return slot; | |||
| } | |||
|
|
|||
| // Descends through Rest fields of a tuple is "symbol" is an extended field | |||
There was a problem hiding this comment.
First "is" on this line should be an "if".
|
LGTM Thanks |
| private int DescendThroughTupleRestFields(ref Symbol symbol, int containingSlot, bool forceContainingSlotsToExist) | ||
| { | ||
| var fieldSymbol = symbol as TupleFieldSymbol; | ||
| if ((object)fieldSymbol != null) | ||
| { | ||
| TypeSymbol containingType = ((TupleTypeSymbol)symbol.ContainingType).UnderlyingNamedType; | ||
|
|
||
| // for tuple fields the varible indentifier represents the underlying field | ||
| // for tuple fields the variable indentifier represents the underlying field |
|
LGTM |
|
Test this please |
1 similar comment
|
Test this please |
|
@dotnet-bot test windows_debug_unit64_prtest please |
|
@dotnet-bot test windows_debug_unit32_prtest please |
…bols Fixed some typos. Made virtual tuple elements to have right element IDs (needed since we no longer want to rely on symbol types) Made virtual tuple field equality be sensitive to names. (needed since we no longer can rely on only default elements having IDs)
When recording assignments of tuple fields, associate tracking slots with underlying field symbols. This way the element assignment to a tuple field and to a corresponding "ItemX" field would be tracked equivalently.
When checking whether a tuple is definitely assigned, only look at nonvirtual fields - fields that map directly to the actual fields in the top level ValueTuple.
Fixes C# portion of #13046