-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add IOperation support for tuple expressions #19636
Conversation
Fixes #10856 ``` public interface ITupleExpression : IOperation { /// <summary> /// Elements for tuple expression. /// </summary> ImmutableArray<IOperation> Elements { get; } } ```
Tagging @AlekseyTs @cston @dotnet/analyzer-ioperation @dotnet/roslyn-compiler #Closed |
Ping... #Closed |
👍 #Resolved |
} | ||
|
||
[Fact, WorkItem(10856, "https://github.com/dotnet/roslyn/issues/10856")] | ||
public void TupleExpression_NamedArguments() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NamedElements
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, can rename the test.
@@ -828,6 +832,7 @@ virtual Microsoft.CodeAnalysis.Semantics.OperationVisitor.VisitSwitchStatement(M | |||
virtual Microsoft.CodeAnalysis.Semantics.OperationVisitor.VisitSyntheticLocalReferenceExpression(Microsoft.CodeAnalysis.Semantics.ISyntheticLocalReferenceExpression operation) -> void | |||
virtual Microsoft.CodeAnalysis.Semantics.OperationVisitor.VisitThrowStatement(Microsoft.CodeAnalysis.Semantics.IThrowStatement operation) -> void | |||
virtual Microsoft.CodeAnalysis.Semantics.OperationVisitor.VisitTryStatement(Microsoft.CodeAnalysis.Semantics.ITryStatement operation) -> void | |||
virtual Microsoft.CodeAnalysis.Semantics.OperationVisitor.VisitTupleExpression(Microsoft.CodeAnalysis.Semantics.ITupleExpression operation) -> void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -3837,6 +3837,63 @@ public LazyTryStatement(Lazy<IBlockStatement> body, Lazy<ImmutableArray<ICatchCl | |||
} | |||
|
|||
/// <summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, how is this file generated? What are the inputs used and the command to run? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently a hand crafted file, but @heejaechang is working on a generator (similar to our bound node and syntax node generator based on an xml). @jinujoseph - I think we need to check that in soon to avoid extra manual work in coding and reviews and also prevent unintentional coding errors in the generated file. Do we plan to enable this soon? #Resolved
|
||
private static ITupleExpression CreateBoundTupleExpressionOperation(BoundTupleExpression boundTupleExpression) | ||
{ | ||
Lazy<ImmutableArray<IOperation>> elements = new Lazy<ImmutableArray<IOperation>>(() => GetTupleElements(boundTupleExpression)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: you could omit the type (use var
) since type is spelled out on the right-hand-side. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have followed the existing pattern used in this file @heejaechang probably we can clean this up for the entire file in one of your refactorings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use of var
is not a requirement. It is the other way around, we forbid use of var
in some cases.
|
||
private static ITupleExpression CreateBoundTupleExpressionOperation(BoundTupleExpression boundTupleExpression) | ||
{ | ||
Lazy<ImmutableArray<IOperation>> elements = new Lazy<ImmutableArray<IOperation>>(() => GetTupleElements(boundTupleExpression)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lambda (and all others above) are creating closures. The compiler generally tries to avoid this, so I'm surprised to see this a recurring pattern in this file. Maybe something to consider for a follow-up refactoring as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, certainly. Let me file an issue to track all your review comments here.. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #19720
{ | ||
private readonly Lazy<ImmutableArray<IOperation>> _lazyElements; | ||
|
||
public LazyTupleExpression(Lazy<ImmutableArray<IOperation>> elements, bool isInvalid, SyntaxNode syntax, ITypeSymbol type, Optional<object> constantValue) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the type may be null
. Consider naming this field typeOpt
if that is allowed by the generation tool.
bool isInvalid = boundTupleExpression.HasErrors; | ||
SyntaxNode syntax = boundTupleExpression.Syntax; | ||
ITypeSymbol type = boundTupleExpression.Type; | ||
Optional<object> constantValue = ConvertToOptional(boundTupleExpression.ConstantValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think tuple can ever have a constant value. #WontFix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think tuple can ever have a constant value.
The bound node will reflect that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep this as is. If tuples could ever become constnat, then this code will continue to work.
@@ -340,6 +340,15 @@ private static ImmutableArray<IVariableDeclaration> GetVariableDeclarationStatem | |||
OperationFactory.CreateVariableDeclaration(declaration.LocalSymbol, Create(declaration.InitializerOpt), declaration.Syntax))); | |||
} | |||
|
|||
private static readonly ConditionalWeakTable<BoundTupleExpression, object> s_tupleElementsMappings = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason not to have specific type (as opposed to object
) for the values stored in the table?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@heejaechang? Comment tracked in #19720
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with CWT is that it has a 'class' constraint on the values. But we're storing ImmutableArrays. So we end up needing to box things into object to satisfy the CWT.
IOperation: (OperationKind.None) (Syntax: '(x, x + y)') | ||
Children(2): IParameterReferenceExpression: x (OperationKind.ParameterReferenceExpression, Type: System.Int32) (Syntax: 'x') | ||
ITupleExpression (OperationKind.TupleExpression, Type: (System.Int32 x, System.Int32)) (Syntax: '(x, x + y)') | ||
Elements(2): IParameterReferenceExpression: x (OperationKind.ParameterReferenceExpression, Type: System.Int32) (Syntax: 'x') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of formatting, maybe IParameterReferenceExpression: x ...
should be on the next line, indented with IBinaryOperatorExpression ...
? #Pending
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we hope to make that change in the dump format, but it would require updating almost all the IOperation tests. We have it in our backlog though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// <summary> | ||
/// Elements for tuple expression. | ||
/// </summary> | ||
ImmutableArray<IOperation> Elements { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a general comment beyond this PR, but I'm concerned about materializing more data structures to represent existing information. I would have expected IOperation to be much more "view-like".
} | ||
"; | ||
string expectedOperationTree = @" | ||
IOperation: (OperationKind.None) (Syntax: '(uint x, ui ... Point(0, 1)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also beyond the scope of this PR, but we have some test infrastructure to dump tree information (TreeDumperNode
). Maybe it would be useful for IOperation tests, rather than inventing a new format?
} | ||
"; | ||
string expectedOperationTree = @" | ||
ITupleExpression (OperationKind.TupleExpression, Type: (System.UInt32, System.UInt32)) (Syntax: '(1, 2)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to check what conversion node we have on top of this node, if any.
} | ||
"; | ||
string expectedOperationTree = @" | ||
ITupleExpression (OperationKind.TupleExpression, Type: (System.Int32, System.Int32)) (Syntax: '(1, 2)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to check what conversion node we have on top of this node, if any. #Resolved
} | ||
"; | ||
string expectedOperationTree = @" | ||
ITupleExpression (OperationKind.TupleExpression, Type: (System.UInt32, System.String)) (Syntax: '(1, null)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to check what conversion node we have on top of this node, if any.
} | ||
"; | ||
string expectedOperationTree = @" | ||
ITupleExpression (OperationKind.TupleExpression, Type: (System.Int32, System.Int32)) (Syntax: '(1, 2)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to check what conversion node we have on top of this node, if any.
} | ||
"; | ||
string expectedOperationTree = @" | ||
ITupleExpression (OperationKind.TupleExpression, Type: (System.Int16 A, System.String B)) (Syntax: '(A: 1, B: null)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to check what conversion node we have on top of this node, if any.
IVariableDeclaration (1 variables) (OperationKind.VariableDeclaration) (Syntax: 'C t /*<bind ... *</bind>*/;') | ||
Variables: Local_1: C t | ||
Initializer: IConversionExpression (ConversionKind.OperatorMethod, Implicit) (OperatorMethod: C C.op_Implicit((System.Int32, System.String) x)) (OperationKind.ConversionExpression, Type: C) (Syntax: '(0, null)') | ||
IConversionExpression (ConversionKind.CSharp, Implicit) (OperationKind.ConversionExpression, Type: (System.Int32, System.String)) (Syntax: '(0, null)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IConversionExpression (ConversionKind.CSharp, Implicit) (OperationKind.ConversionExpression, Type: (System.Int32, System.String)) (Syntax: '(0, null)') [](start = 8, length = 151)
This conversion feels unexpected, it looks like there is no type change from the operand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlekseyTs Is this a compiler issue? I will file a bug regardless...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a compiler issue?
We should not expect that the shape of the bound tree is what we want IOperation tree to have. It is possible that this particular problem can be addressed by adjusting the shape of the bound tree, it has to be investigated. I would treet this an IOperation issue as long as compiler behaves as expected (the shape of the bound tree is not part of the behavior) and other public APIs behave as expected too.
"; | ||
string expectedOperationTree = @" | ||
IOperation: (OperationKind.None) (Syntax: 'var (x, y) ... Point(0, 1)') | ||
Children(2): ITupleExpression (OperationKind.TupleExpression, Type: (System.Int32 x, System.Int32 y)) (Syntax: 'var (x, y)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Children(2): ITupleExpression (OperationKind.TupleExpression, Type: (System.Int32 x, System.Int32 y)) (Syntax: 'var (x, y)') [](start = 2, length = 124)
This doesn't look right, var (x, y)
is not a tuple expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not surprising to me. var (x, y)
is the short-hand for (var x, var y)
and so is bound the same way.
What should we get here instead?
In reply to: 118122825 [](ancestors = 118122825)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should we get here instead?
The question is not how we represent things internally, but how IOperation should represent this. Syntactically this is not a tuple expression, perhaps it should be represented by a special IOperation node, a declaration expression, etc. Something for the design team to decide.
"; | ||
string expectedOperationTree = @" | ||
IOperation: (OperationKind.None) (Syntax: '(uint x, ui ... Point(0, 1)') | ||
Children(2): ITupleExpression (OperationKind.TupleExpression, Type: (System.UInt32 x, System.UInt32 y)) (Syntax: '(uint x, uint y)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Children(2): ITupleExpression (OperationKind.TupleExpression, Type: (System.UInt32 x, System.UInt32 y)) (Syntax: '(uint x, uint y)') [](start = 2, length = 132)
This doesn't look right, (uint x, uint y)
is not a tuple expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is a tuple expression, containing two declaration expressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is a tuple expression, containing two declaration expressions.
I see, that makes sense now. The elements of the tuple should probably reflect that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Each element should be a bound local with a flag set marking it as a declaration. new BoundLocal(..., isDeclaration: true, ...)
.
Maybe we should expose that flag, to distinguish plain locals and declaration expressions?
In reply to: 118129477 [](ancestors = 118129477)
@@ -161,6 +161,8 @@ Namespace Microsoft.CodeAnalysis.Semantics | |||
Return CreateBoundAddHandlerStatementOperation(DirectCast(boundNode, BoundAddHandlerStatement)) | |||
Case BoundKind.RemoveHandlerStatement | |||
Return CreateBoundRemoveHandlerStatementOperation(DirectCast(boundNode, BoundRemoveHandlerStatement)) | |||
Case BoundKind.TupleLiteral, BoundKind.ConvertedTupleLiteral |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BoundKind.ConvertedTupleLiteral [](start = 45, length = 31)
Please put this value on the next line for readability.
End Class]]>.Value | ||
|
||
Dim expectedOperationTree = <![CDATA[ | ||
IConversionExpression (ConversionKind.Basic, Implicit) (OperationKind.ConversionExpression, Type: (System.UInt32, System.UInt32)) (Syntax: '(1, 2)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like similar test in C# does not include this conversion. The conversion feels unexpected, there is no type change from the operand.
|
||
Namespace Microsoft.CodeAnalysis.VisualBasic.UnitTests.Semantics | ||
|
||
Partial Public Class IOperationTests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IOperationTests [](start = 25, length = 15)
Many comments for C# tests are applicable for VB tests as well.
End Class]]>.Value | ||
|
||
Dim expectedOperationTree = <![CDATA[ | ||
IConversionExpression (ConversionKind.Basic, Implicit) (OperationKind.ConversionExpression, Type: (System.UInt32, System.String)) (Syntax: '(1, Nothing)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment.
End Class]]>.Value | ||
|
||
Dim expectedOperationTree = <![CDATA[ | ||
IConversionExpression (ConversionKind.Basic, Implicit) (OperationKind.ConversionExpression, Type: (A As System.Int32, B As System.Int32)) (Syntax: '(1, 2)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conversion feels unexpected, given the syntax we examine.
End Class]]>.Value | ||
|
||
Dim expectedOperationTree = <![CDATA[ | ||
IConversionExpression (ConversionKind.Basic, Implicit) (OperationKind.ConversionExpression, Type: (A As System.Int16, B As System.String)) (Syntax: '(A:=1, B:=Nothing)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conversion feels unexpected given the syntax we examine. Also, there is no type change from the operand.
End Class]]>.Value | ||
|
||
Dim expectedOperationTree = <![CDATA[ | ||
IConversionExpression (ConversionKind.Basic, Implicit) (OperationKind.ConversionExpression, Type: (A As System.Int16, B As System.String)) (Syntax: '(New C(0), c1)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conversion feels strange too.
|
||
Dim expectedOperationTree = <![CDATA[ | ||
IConversionExpression (ConversionKind.Basic, Implicit) (OperationKind.ConversionExpression, Type: C) (Syntax: '(0, Nothing)') | ||
IConversionExpression (ConversionKind.OperatorMethod, Implicit) (OperatorMethod: Function C.op_Implicit(x As (System.Int32, System.String)) As C) (OperationKind.ConversionExpression, Type: (System.Int32, System.Object)) (Syntax: '(0, Nothing)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type: (System.Int32, System.Object) [](start = 185, length = 35)
I am confused, why the type is a tuple when the operator returns C?
IConversionExpression (ConversionKind.Basic, Implicit) (OperationKind.ConversionExpression, Type: C) (Syntax: '(0, Nothing)') | ||
IConversionExpression (ConversionKind.OperatorMethod, Implicit) (OperatorMethod: Function C.op_Implicit(x As (System.Int32, System.String)) As C) (OperationKind.ConversionExpression, Type: (System.Int32, System.Object)) (Syntax: '(0, Nothing)') | ||
IConversionExpression (ConversionKind.Basic, Implicit) (OperationKind.ConversionExpression, Type: (System.Int32, System.Object)) (Syntax: '(0, Nothing)') | ||
ITupleExpression (OperationKind.TupleExpression, Type: (System.Int32, System.Object)) (Syntax: '(0, Nothing)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ITupleExpression (OperationKind.TupleExpression, Type: (System.Int32, System.Object)) (Syntax: '(0, Nothing)') [](start = 6, length = 110)
Everything above tuple literal looks strange.
End Operator | ||
|
||
Public Sub M(c1 As C) | ||
Dim t As (Integer, String) = c1'BIND:"Dim t As (Integer, String) = c1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'BIND:"Dim t As (Integer, String) = c1" [](start = 39, length = 39)
It would also be interesting testing what we return just for c1
|
||
public void M(C c1) | ||
{ | ||
(int, string) t /*<bind>*/= c1/*</bind>*/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c1 [](start = 36, length = 2)
What do we get when we bind just c1
?
Variables: Local_1: t As (System.Int32, System.String) | ||
Initializer: IConversionExpression (ConversionKind.Basic, Implicit) (OperationKind.ConversionExpression, Type: (System.Int32, System.String)) (Syntax: 'c1') | ||
IConversionExpression (ConversionKind.OperatorMethod, Implicit) (OperatorMethod: Function C.op_Implicit(c As C) As (System.Int32, System.String)) (OperationKind.ConversionExpression, Type: C) (Syntax: 'c1') | ||
IParameterReferenceExpression: c1 (OperationKind.ParameterReferenceExpression, Type: C) (Syntax: 'c1') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IParameterReferenceExpression: c1 (OperationKind.ParameterReferenceExpression, Type: C) (Syntax: 'c1') [](start = 10, length = 102)
The two conversions above look strange.
|
||
public void M(C c1) | ||
{ | ||
/*<bind>*/(short, string) t = (new C(0), c1);/*</bind>*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(new C(0), c1) [](start = 38, length = 14)
What is the result when we bind just the tuple?
|
||
namespace Microsoft.CodeAnalysis.CSharp.UnitTests | ||
{ | ||
public partial class IOperationTests : SemanticModelTestBase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IOperationTests [](start = 25, length = 15)
Do we have a test that uses user-defined conversion on a tuple literal, which requires conversions for tuple elements, including user-defined ones?
End Operator | ||
|
||
Public Sub M(c1 As C) | ||
Dim t As (Short, String) = (New C(0), c1)'BIND:"Dim t As (Short, String) = (New C(0), c1)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(New C(0), c1) [](start = 83, length = 14)
What happens if we bind just the tuple?
IVariableDeclarationStatement (1 declarations) (OperationKind.VariableDeclarationStatement, IsInvalid) (Syntax: 'Dim t As (S ... w C(0), c1)') | ||
IVariableDeclaration (1 variables) (OperationKind.VariableDeclaration, IsInvalid) (Syntax: 't') | ||
Variables: Local_1: t As (System.Int16, System.String) | ||
Initializer: ITupleExpression (OperationKind.TupleExpression, Type: (System.Int16, c1 As System.String), IsInvalid) (Syntax: '(New C(0), c1)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ITupleExpression (OperationKind.TupleExpression, Type: (System.Int16, c1 As System.String), IsInvalid) (Syntax: '(New C(0), c1)') [](start = 17, length = 129)
Given other tests, it is not clear if we want to have a conversion on top of this node.
Done with review pass. I think we need to have a discussion about how tuple literal conversions should be represented. |
Children(2): ITupleExpression (OperationKind.TupleExpression, Type: (System.UInt32 x, System.UInt32 y)) (Syntax: '(uint x, uint y)') | ||
Elements(2): ILocalReferenceExpression: x (OperationKind.LocalReferenceExpression, Type: System.UInt32) (Syntax: 'uint x') | ||
ILocalReferenceExpression: y (OperationKind.LocalReferenceExpression, Type: System.UInt32) (Syntax: 'uint y') | ||
IConversionExpression (ConversionKind.Invalid, Implicit) (OperationKind.ConversionExpression, Type: (System.UInt32 x, System.UInt32 y)) (Syntax: 'new Point(0, 1)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a surprised by ConversionKind.Invalid
here.
In the bound tree, I expect a deconstruction conversion. That conversion is "secret" (implementation-only, will likely not be in the spec). I'm not sure how it should be surfaced, but "invalid" seems wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a surprised by ConversionKind.Invalid here.
We are still figuring out how deconstruction should be represented. At the moment, we just show what the bound node has. Since conversion kind is not one of the conversions supported by the language, it is surfaced as invalid, I guess.
Thanks for the reviews! I have added some additional comments to #10856 (comment) so we can discuss this at the next IOperation design meeting. |
Closing in favor of #20749 |
Fixes #10856