second step on separating IOperation from BoundTree. #18318
Conversation
this doesn't replace or change anything yet, just bunch of Operation types for next step. next step is creating CSharp/VBFactory which return these Operation type and replace callsite that return bound node with this. after that, I will create xml/generator from Operation.xml.Generated.cs which can produce Operation.xml.Generated.cs
|
@dotnet/analyzer-ioperation can you take a look? |
| /// Root type for representing the abstract semantics of C# and VB statements and expressions. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// This interface is reserved for implementation by its associated APIs. We reserve the right to |
There was a problem hiding this comment.
💡 This remark probably doesn't apply to the Operation class.
There was a problem hiding this comment.
thank you, will change.
|
@sharwell I will change path to new location. |
|
@dotnet/roslyn-infrastructure vsi test failed - |
|
retest windows_release_vs-integration_prtest please |
|
ping? can you guys take a look? I would like this to be in to make working on my next step easier |
|
Will look shortly. |
|
|
||
| using System.Collections.Immutable; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.Semantics |
There was a problem hiding this comment.
are we checking in generated .cs files? i thought the pattern was that an initial compilation step just generated them for you.
There was a problem hiding this comment.
that is last step on my work.
"after that, I will create xml/generator from Operation.xml.Generated.cs which can produce Operation.xml.Generated.cs"
I dont have generator yet. I wrote custom tool to create this file from IOperation interfaces. I still used the file name I will use at the end.
| /// This interface is reserved for implementation by its associated APIs. We reserve the right to | ||
| /// change it in the future. | ||
| /// </remarks> | ||
| internal sealed partial class AddressOfExpression : Operation, IAddressOfExpression |
There was a problem hiding this comment.
Intresting design question: as these are now a full new tree, is there value in having the interfaces at all? Let's discuss this at our meeting. The interface approach was necessayr so that we could implement them on the bound-nodes across VB/C#. But if bound-nodes are not the objects anymore, we don't really need interfaces.
There was a problem hiding this comment.
it is in the design meeting agenda today.
There was a problem hiding this comment.
First thing in our list to discuss #18246
| /// Addressed reference. | ||
| /// </summary> | ||
| public IOperation Reference { get; } | ||
| public override void Accept(OperationVisitor visitor) |
There was a problem hiding this comment.
this is generated one. style is not part of thing I will do this time.
| public AddressOfExpression(IOperation reference, bool isInvalid, SyntaxNode syntax, ITypeSymbol type, Optional<object> constantValue) : | ||
| base(OperationKind.AddressOfExpression, isInvalid, syntax, type, constantValue) | ||
| { | ||
| Reference = reference; |
There was a problem hiding this comment.
Consider null checks (or create bug to track that).
There was a problem hiding this comment.
sure we can do that.
There was a problem hiding this comment.
@CyrusNajmabadi @AlekseyTs I just made all parameter that has ref type to null check, but now I am kind of not sure whether that is okay since I dont know which parameter is allowed to be null or not.
| { | ||
| visitor.VisitAddressOfExpression(this); | ||
| } | ||
| public override TResult Accept<TArgument, TResult>(OperationVisitor<TArgument, TResult> visitor, TArgument argument) |
There was a problem hiding this comment.
same, styling will come later when we create Generator.
There was a problem hiding this comment.
yup. agreed. If it's generated, i don't really even care :)
| private readonly AssignmentExpression _assignment; | ||
|
|
||
| public Assignment(IOperation target, IOperation value, SyntaxNode syntax) | ||
| private static bool IsInvalidOperation(IOperation condition, IOperation ifTrue, IOperation ifFalse, ITypeSymbol resultType) |
There was a problem hiding this comment.
this seems like a weird method to have here.
There was a problem hiding this comment.
hmmm suggestion?
There was a problem hiding this comment.
No. I see how it's being used now. It's just unfortunate. I think we'd like to get rid of the whole 'IsInvalid' portion of the API (eventually).
|
👍 |
|
needs @dotnet/roslyn-compiler Signoff as well. @dotnet/roslyn-compiler can we get some eyes. This is a largely mechanical change that Heejae is making and we'd like to get all the mechanical stuff in so we can move onto more interesting and complex work. Thanks! |
|
LGTM |
|
Is there a PR for the first step? Could you share a link? |
|
@AlekseyTs #18224 here are first step. |
|
are we okay for me to check this in? since It won't replace existing bound node yet, it will not affect anything yet except extra code checkin to the repo. |
|
yes, @AlekseyTs feel free to leave the feedback, which should be addressed in subsequent PR |
| /// <returns>Argument corresponding to the parameter.</returns> | ||
| public IArgument GetArgumentMatchingParameter(IParameterSymbol parameter) | ||
| { | ||
| throw new System.NotImplementedException(); |
There was a problem hiding this comment.
throw new System.NotImplementedException(); [](start = 12, length = 43)
It is not clear why throwing is the right thing to do.
There was a problem hiding this comment.
these files are just place holder file now. next PR will make these to do right thing.
| /// <returns>Argument corresponding to the parameter.</returns> | ||
| public IArgument GetArgumentMatchingParameter(IParameterSymbol parameter) | ||
| { | ||
| throw new System.NotImplementedException(); |
There was a problem hiding this comment.
throw new System.NotImplementedException(); [](start = 12, length = 43)
It is not clear why throwing is the right thing to do.
| /// <returns>Argument corresponding to the parameter.</returns> | ||
| public IArgument GetArgumentMatchingParameter(IParameterSymbol parameter) | ||
| { | ||
| throw new System.NotImplementedException(); |
There was a problem hiding this comment.
throw new System.NotImplementedException(); [](start = 12, length = 43)
It is not clear why throwing is the right thing to do.
| @@ -569,7 +569,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic | |||
| End If | |||
|
|
|||
| Dim stepOperand As IOperation = If(stepValue.IsConstant, DirectCast(stepValue, IOperation), New Temporary(SyntheticLocalKind.ForLoopStepValue, BoundFor, stepValue)) | |||
| statements.Add(New CompoundAssignment(controlReference, stepOperand, Semantics.Expression.DeriveAdditionKind(controlType), Nothing, stepValue.Syntax)) | |||
| statements.Add(New ExpressionStatement(controlReference, stepOperand, Semantics.Expression.DeriveAdditionKind(controlType), Nothing, stepValue.Syntax)) | |||
There was a problem hiding this comment.
ExpressionStatement [](start = 51, length = 19)
It is not clear why are we creating a different node here.
There was a problem hiding this comment.
I dont know either. that was existing behavior. this I just renamed type name
There was a problem hiding this comment.
I dont know either. that was existing behavior. this I just renamed type name
Why are we merging PR if the answer to the question is: "I dont know either."
Could you please elaborate what did you renamed? And why is it the right thing to do to create an ExpressionStatement instead of an CoumpoundAssignment node in this place?
There was a problem hiding this comment.
CompoundAssignment and Assignment was implementing IExpressionStatement. basically it was duplicated code. so I made ExpressionStatement with 2 different constructor. and used it in both places. except the contructor, both were same code. basically exact same code with different field names which implements same IExpressionStatement interface. basically duplicated code.
There was a problem hiding this comment.
@AlekseyTs @genlu once we get pass the part that CompoundAssigment and Assignment was just duplicated code which is actually ExpressionStatement (expression in the ExpressionStatement is either CompoundAssignement or Assignment, so it looks like duplicated code are created just for the expression? I dont know, but see this - https://github.com/dotnet/roslyn/pull/18318/files#diff-5e5ff960b3a6f83d52b5b20c6e12fcf2L47)
why the code do "statements.Add(/New/ CompoundAssignment" and create new ExpressionStatement rather than getting it from somewhere (I dont know where I would get one), I dont know. I wasn't trying to change that logic.
There was a problem hiding this comment.
I think biggest confusion comes from existence of this file (Expression/Statement - #18318 (review))
those shouldn't be implementation of IOperation. I understand they are needed when Bound node directly implemented IOperation. now two are separated, code in expression.cs/statement.cs should be just factory rather than another IOperation implementation.
| @@ -592,17 +592,17 @@ Namespace Microsoft.CodeAnalysis.VisualBasic | |||
| ' ControlVariable = InitialValue | |||
| Dim controlReference As IOperation = BoundFor.ControlVariable | |||
| If controlReference IsNot Nothing Then | |||
| statements.Add(New Assignment(controlReference, BoundFor.InitialValue, BoundFor.InitialValue.Syntax)) | |||
| statements.Add(New ExpressionStatement(controlReference, BoundFor.InitialValue, BoundFor.InitialValue.Syntax)) | |||
There was a problem hiding this comment.
ExpressionStatement [](start = 47, length = 19)
It is not clear why are we creating a different node here and below.
|
Done with review pass. |
|
@genlu I will do that in next PR. |
| public VariableDeclaration(ILocalSymbol variable, IOperation initialValue, SyntaxNode syntax) : | ||
| this(variable, | ||
| initialValue, | ||
| variable == null || (initialValue != null && initialValue.IsInvalid), |
There was a problem hiding this comment.
if you are talking about variable == null .. and stuff, in last design meeting, people were talking about removing IsInvalid parameter all together, so I will leave it for now.
There was a problem hiding this comment.
| { | ||
| public ConditionalChoice(IOperation condition, IOperation ifTrue, IOperation ifFalse, ITypeSymbol resultType, SyntaxNode syntax) | ||
| public ConditionalChoiceExpression(IOperation condition, IOperation ifTrue, IOperation ifFalse, ITypeSymbol resultType, SyntaxNode syntax) : | ||
| this(condition, |
There was a problem hiding this comment.
nit: use named arguments when calling base constructors.
| using System.Linq; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.Semantics | ||
| { | ||
| internal sealed class ConditionalChoice : IConditionalChoiceExpression | ||
| internal sealed partial class ConditionalChoiceExpression : IConditionalChoiceExpression |
There was a problem hiding this comment.
I believe eventually, all code here should be removed and moved to Factory since logic here doesn't actually belong to Operation type itself but the factory that determine what to give in as each property values.
| } | ||
| } | ||
|
|
||
| internal sealed class CompoundAssignment : IExpressionStatement | ||
| internal sealed partial class ExpressionStatement : IExpressionStatement |
There was a problem hiding this comment.
ExpressionStatement [](start = 34, length = 19)
It feels like statements do not belong in this file.
There was a problem hiding this comment.
Statements.cs/vb Expressions.cs/vb in Operations directory no longer needed. I just did this to make things build and make this PR to only contains the new Operation Types. those 4 file will be removed in my next PR.
| public Optional<object> ConstantValue => default(Optional<object>); | ||
|
|
||
| public void Accept(OperationVisitor visitor) | ||
| public ExpressionStatement(IOperation target, IOperation value, SyntaxNode syntax) : |
There was a problem hiding this comment.
ExpressionStatement [](start = 15, length = 19)
I think we shouldn't have constructors like this one and the one below. They make it very hard to understand what is going on when one is reading code that uses them.
this doesn't replace or change anything yet, just bunch of Operation types for next step.
next step is creating CSharp/VBFactory which return these Operation type and replace callsite that return bound node with this.
after that, I will create xml/generator from Operation.xml.Generated.cs which can produce Operation.xml.Generated.cs