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
Using declaration ioperation #39125
Using declaration ioperation #39125
Conversation
899598d
to
41d62fd
Compare
@dotnet/roslyn-compiler for review please |
@@ -2899,4 +2899,21 @@ | |||
</Comments> | |||
</Property> | |||
</Node> | |||
<Node Name="IUsingVariableDeclarationOperation" Base="IOperation"> |
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.
IUsingVariableDeclarationOperation [](start = 14, length = 34)
It looks like syntactically a Using Declaration is just a Local Declaration with additional keywords. Should we simply reuse the existing node, IVariableDeclarationOperation, which already says that it is used to represent "(3) C# using declarations"?
#Closed
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.
IVariableDeclarationGroupOperation
is used as a child node of the regular using statement. So you have a parent IUsingOperation
with a VariableDeclarationGroup and a body. That's the same for the other types mentioned in the comments.
This way we keep the same shape for using declarations: you have a parent using declaration, which contains a child declaration group of the variables declared.
I originally went the route of updating DeclarationGroup with an enum on it to specify if it was using/async using/none, but this proved simpler as it was never used in any of the other cases, and this way you can keep the tree shape the same and visit the using declaration essentially the same as the statement. #Closed
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 am not following why would we need to make any changes to DeclarationGroup. Or why similarities with IUsingOperation are important. I think we should simply add IsUsing and IsAsynchronous properties to IVariableDeclarationOperation instead of adding new node.
In reply to: 332695447 [](ancestors = 332695447)
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 agree with @AlekseyTs here. That's how i would have expected thsi would be represented. #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.
Oh I see what you're referring to, that was from the initial 'dummy' implementation we did. Yep, we could definitely do that if you think it's better than introducing a new node? #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 agree with reusing the IVariableDeclarationOperation
for this, though I'm not sure I agree with the two bool flags. These aren't independent variables: if IsAsync
were true, IsUsing
must, by definition, be true. I would likely prefer an enum over two bools. This will also give us flexibility if we add new types of variable declarations in the future, as we can add new kinds rather than throwing a whole bunch of flag booleans on the type itself. #Closed
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.
Yeah, enum is how I originally designed it interestingly enough, then split it out into its own operation, because it felt weird having most of the declarations have UsingType=None
#Closed
/// This interface is reserved for implementation by its associated APIs. We reserve the right to | ||
/// change it in the future. | ||
/// </remarks> | ||
public interface IUsingVariableDeclarationOperation : IOperation |
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.
Dumb question. Why is this a new iop, as opposed to being a bit on IVariableDeclarationGroupOperation ? #Closed
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.
Resolved #Resolved
@@ -1364,28 +1364,28 @@ private IOperation FinishVisitingStatement(IOperation originalOperation, IOperat | |||
return result ?? MakeInvalidOperation(originalOperation.Syntax, originalOperation.Type, ImmutableArray<IOperation>.Empty); | |||
} | |||
|
|||
private void VisitStatements(ArrayBuilder<IOperation> statements) | |||
private void VisitStatements(IList<IOperation> statements) |
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.
why this change. wont' this cause interface dispatches now? #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.
Derp. Fixed. #Closed
As noted offline, we need tests for async using declarations. #Closed |
@@ -2036,6 +2035,13 @@ | |||
</summary> | |||
</Comments> | |||
</Property> | |||
<Property Name="UsingKind" Type="UsingKind"> |
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 this is what I'd do either. UsingKind
limits future extensibility in the same was that the two flag bools do. Rather, I'd suggest something like DeclarationKind
, with options of Local
, Using
, and AsyncUsing
. #Closed
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.
For example, there's an open request for a fixed
declaration statement, that would fit in nicely with this API. #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.
Any reason these aren't just bools? IsUsing
IsAsync
? (and, in the future IsFixed
)? #Closed
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.
DeclarationKind
is problematic because it already exists as public in Microsoft.CodeAnalysis.Editing
LocalDeclarationKind
already exists in Microsoft.CodeAnalysis.VisualBasic.Symbols although thats Friend
so we could conceivable promote it and re-use it for this purpose?
We could do the two bools solution, but is a bit odd as you can't have IsAsync be true without IsUsing.
#Closed
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.
@333fred i only know DeclarationKind already exists because that's how I first defined it. Non clashing name suggestions welcome, and i'll happily update #Closed
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.
@CyrusNajmabadi #39125 (comment).
I'll mull on the DeclarationKind naming. #Closed
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.
Actually, I thought it collided a lot more than it did. I've changed it to DeclarationKind
#Closed
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 feel hugely strongly. I'm personally ok with booleans where one implies the other. But i'm not going to die on this hill :) #Closed
fc35010
to
e89686a
Compare
e89686a
to
07733a2
Compare
/// <summary> | ||
/// A local declaration. | ||
/// </summary> | ||
Local = 0, |
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.
Local [](start = 8, length = 5)
Are other kinds not local? Perhaps "Default" or "None"? #Closed
@@ -475,6 +475,10 @@ public override void VisitVariableDeclaration(IVariableDeclarationOperation oper | |||
{ | |||
var variableCount = operation.Declarators.Length; | |||
LogString($"{nameof(IVariableDeclarationOperation)} ({variableCount} declarators)"); | |||
if (operation.DeclarationKind != DeclarationKind.Local) |
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.
if (operation.DeclarationKind != DeclarationKind.Local) [](start = 12, length = 55)
Please make necessary adjustments to TestOperationVisitor #Closed
@@ -1,17 +1,19 @@ | |||
| |||
Microsoft Visual Studio Solution File, Format Version 12.00 | |||
# Visual Studio 15 |
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 am assuming changes to this file were not intentional. Please revert/exclude from the PR. #Closed
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.
There were intentional. The CompilerTools.sln didn't have the new CompilersIOperationGenerator added to it. #Closed
|
||
namespace Microsoft.CodeAnalysis.Operations | ||
{ | ||
public enum DeclarationKind |
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.
DeclarationKind [](start = 16, length = 15)
VariableDeclarationKind? #Closed
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 also like this suggestion, it follows the rest of our Kind enums.
In reply to: 333643366 [](ancestors = 333643366)
@@ -224,6 +224,7 @@ internal IOperation CreateInternal(BoundNode boundNode) | |||
case BoundKind.LocalDeclaration: | |||
return CreateBoundLocalDeclarationOperation((BoundLocalDeclaration)boundNode); | |||
case BoundKind.MultipleLocalDeclarations: | |||
case BoundKind.UsingLocalDeclarations: |
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.
case BoundKind.UsingLocalDeclarations: [](start = 16, length = 38)
It looks like changes made to bound trees to support Using Declarations violate the invariant that only leaf nodes can have kinds. I.e. base nodes shouldn't have kinds. This way we can be sure that type checks for nodes that have kinds are equivalent to kind checks. #Closed
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.
So do you want me to change this as part of this PR, or should we just log a bug and fix it in the future? #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.
So do you want me to change this as part of this PR, or should we just log a bug and fix it in the future?
I think it is a simple fix - introduction of a common base node. Should fit into this PR.
In reply to: 333678166 [](ancestors = 333678166)
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.
Done. #Resolved
@@ -1755,16 +1754,19 @@ private IVariableDeclarationGroupOperation CreateBoundMultipleLocalDeclarationsO | |||
SyntaxNode declarationSyntax = declarationGroupSyntax.IsKind(SyntaxKind.LocalDeclarationStatement) ? | |||
((LocalDeclarationStatementSyntax)declarationGroupSyntax).Declaration : | |||
declarationGroupSyntax; | |||
|
|||
bool isUsing = declarationGroupSyntax.IsKind(SyntaxKind.LocalDeclarationStatement) && ((LocalDeclarationStatementSyntax)declarationGroupSyntax).UsingKeyword != default; |
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.
declarationGroupSyntax.IsKind(SyntaxKind.LocalDeclarationStatement) && ((LocalDeclarationStatementSyntax)declarationGroupSyntax).UsingKeyword != default; [](start = 27, length = 153)
We shouldn't be relying on syntax in IOperation factory. All required information should be captured in bound nodes during binding and factory should rely only on information in bound nodes. #Closed
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.
Agreed, but we were already relying on syntax here to distinguish between using/fixed and localdecl. Should we just create an issue to fix the underlying bound nodes (as well as splitting them out above) and link it here? #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.
Should we just create an issue to fix the underlying bound nodes (as well as splitting them out above) and link it here?
I don't think so. Picking best syntax to associate node with is one thing, reinterring semantic meaning (which should be clear from the bound node itself) from syntax is another.
In reply to: 333679279 [](ancestors = 333679279)
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.
Fixed. #Resolved
@@ -1755,16 +1754,19 @@ private IVariableDeclarationGroupOperation CreateBoundMultipleLocalDeclarationsO | |||
SyntaxNode declarationSyntax = declarationGroupSyntax.IsKind(SyntaxKind.LocalDeclarationStatement) ? | |||
((LocalDeclarationStatementSyntax)declarationGroupSyntax).Declaration : | |||
declarationGroupSyntax; | |||
|
|||
bool isUsing = declarationGroupSyntax.IsKind(SyntaxKind.LocalDeclarationStatement) && ((LocalDeclarationStatementSyntax)declarationGroupSyntax).UsingKeyword != default; | |||
bool isAsync = isUsing && ((LocalDeclarationStatementSyntax)declarationGroupSyntax).AwaitKeyword != default; |
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.
bool isAsync = isUsing && ((LocalDeclarationStatementSyntax)declarationGroupSyntax).AwaitKeyword != default; [](start = 12, length = 108)
Same comment. #Closed
// We do the same if the declarationSyntax was a using declaration, as it's bound as if it were a using statement | ||
bool isUsing = declarationGroupSyntax.IsKind(SyntaxKind.LocalDeclarationStatement) && ((LocalDeclarationStatementSyntax)declarationGroupSyntax).UsingKeyword != default; | ||
bool isImplicit = declarationGroupSyntax == declarationSyntax || boundMultipleLocalDeclarations.WasCompilerGenerated || isUsing; | ||
bool isImplicit = declarationGroupSyntax == declarationSyntax || boundMultipleLocalDeclarations.WasCompilerGenerated; | ||
return new VariableDeclarationGroupOperation(ImmutableArray.Create(multiVariableDeclaration), _semanticModel, declarationGroupSyntax, type, constantValue, isImplicit); |
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.
return new VariableDeclarationGroupOperation(ImmutableArray.Create(multiVariableDeclaration), _semanticModel, declarationGroupSyntax, type, constantValue, isImplicit); [](start = 12, length = 167)
I guess I was confused before about what node maps to the variable declaration statement. Since it looks like IVariableDeclarationGroupOperation
maps to a statement, I think the new property should be added to that node instead. #Closed
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.
Extracted to group instead #Resolved
StartVisitingStatement(operation); | ||
|
||
var declarationKind = operation.DeclarationKind; | ||
Debug.Assert(declarationKind != DeclarationKind.Default); |
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.
Consider instead asserting that the kind is either Using
or AsynchronousUsing
#Resolved
operation.Syntax, | ||
operation.Type, | ||
operation.ConstantValue, | ||
isImplicit: true); |
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: extra newline below. #Resolved
private IVariableDeclarationGroupOperation CreateBoundUsingLocalDeclarationOperation(BoundUsingLocalDeclarations boundUsingLocalDeclarations) | ||
{ | ||
bool isAsync = boundUsingLocalDeclarations.AwaitOpt is object; | ||
DeclarationKind declarationKid = (isAsync) ? DeclarationKind.AsynchronousUsing : DeclarationKind.Using; |
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.
(isAsync) [](start = 45, length = 9)
Nit: extraneous parens #Resolved
@@ -2427,25 +2427,25 @@ void M1() | |||
IVariableDeclarationOperation (1 declarators) (OperationKind.VariableDeclaration, Type: null, IsInvalid) (Syntax: 'int[y switc ... new int[0]') | |||
Ignored Dimensions(1): | |||
ISwitchExpressionOperation (1 arms) (OperationKind.SwitchExpression, Type: System.Int32, IsInvalid) (Syntax: 'y switch { int z => 42 }') | |||
Value: | |||
Value: |
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: are the only changes in this file whitespace? Can they be reverted? #Resolved
Done review pass (commit 14). Overall looks good, just a couple of small things to address. #Resolved |
@@ -2002,7 +2009,6 @@ | |||
Current Usage: | |||
(1) C# VariableDeclaration | |||
(2) C# fixed declarations | |||
(3) C# using declarations |
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.
(3) C# using declarations [](start = 10, length = 25)
Consider renumbering remaining items below #Closed
// a using statement introduces a 'logical' block after declaration, we synthesize one here in order to analyze it like a regular using | ||
BlockOperation logicalBlock = new BlockOperation( | ||
operations: statements, | ||
locals: default, |
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.
Do we ever create blocks with default locals? Perhaps using an empty array would be more robust. #Closed
HandleUsingOperationParts( | ||
resources: operation, | ||
body: logicalBlock, | ||
locals: default, |
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.
default [](start = 24, length = 7)
Same comment about using an empty array instead. #Closed
{ | ||
bool isAsync = boundUsingLocalDeclarations.AwaitOpt is object; | ||
VariableDeclarationKind declarationKid = isAsync ? VariableDeclarationKind.AsynchronousUsing : VariableDeclarationKind.Using; | ||
SyntaxNode declarationSyntax = ((LocalDeclarationStatementSyntax)boundUsingLocalDeclarations.Syntax).Declaration; |
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.
(LocalDeclarationStatementSyntax) [](start = 44, length = 33)
It would be more robust to not assume that the cast is going to succeed. For example, a compiler generated node doesn't have to be associated with a local declaration statement syntax. #Closed
bool declarationIsImplicit = boundUsingLocalDeclarations.WasCompilerGenerated; | ||
|
||
IVariableDeclarationOperation multiVariableDeclaration = new CSharpLazyVariableDeclarationOperation(this, boundUsingLocalDeclarations, _semanticModel, declarationSyntax, type: null, constantValue: default, declarationIsImplicit); | ||
return new VariableDeclarationGroupOperation(ImmutableArray.Create(multiVariableDeclaration), declarationKid, _semanticModel, boundUsingLocalDeclarations.Syntax, type: null, constantValue: default, isImplicit: false); |
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.
false [](start = 222, length = 5)
It is not clear why we simply assume that the node is not implicit.
In general, it looks like we can share code with CreateBoundMultipleLocalDeclarationsOperation
. That method can be changed to take BoundMultipleLocalDeclarationsBase and figure out declaration kind based on the actual kind of the node, plus accessing BoundUsingLocalDeclarations.AwaitOpt if necessary. #Closed
Done with review pass (iteration 16) #Closed |
- Combine visits in operation factory - Fix numbering - Adjust free ordering in CFGBuilder
@@ -2105,5 +2086,16 @@ private IInstanceReferenceOperation CreateCollectionValuePlaceholderOperation(Bo | |||
bool isImplicit = placeholder.WasCompilerGenerated; | |||
return new InstanceReferenceOperation(referenceKind, _semanticModel, syntax, type, constantValue, isImplicit); | |||
} | |||
|
|||
//private IVariableDeclarationGroupOperation CreateBoundUsingLocalDeclarationOperation(BoundUsingLocalDeclarations boundUsingLocalDeclarations) |
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.
//private IVariableDeclarationGroupOperation CreateBoundUsingLocalDeclarationOperation(BoundUsingLocalDeclarations boundUsingLocalDeclarations) [](start = 8, length = 143)
Please remove commented out code. #Closed
Done with review pass (iteration 17) #Closed |
- Asset operation type - Cleanup free logic
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.
LGTM (iteration 19), assuming CI will pass.
{ | ||
VisitStatement(statement); | ||
// visit the using decl with the following statements | ||
VisitUsingVariableDeclarationOperation(declarationGroup, followingStatements.ToImmutableAndFree()); |
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.
followingStatements.ToImmutableAndFree() [](start = 77, length = 40)
Consider using ImmutableArray.Create(statements, i + 1, statements.Length - i)
. #Closed
{ | ||
// The syntax for the boundMultipleLocalDeclarations can either be a LocalDeclarationStatement or a VariableDeclaration, depending on the context | ||
// The syntax for the boundMultipleLocalDeclarationsBase can either be a LocalDeclarationStatement or a VariableDeclaration, depending on the context |
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.
Base [](start = 64, length = 4)
The parameter name does not include "Base".
bool declarationIsImplicit = boundMultipleLocalDeclarations.WasCompilerGenerated; | ||
IVariableDeclarationOperation multiVariableDeclaration = new CSharpLazyVariableDeclarationOperation(this, boundMultipleLocalDeclarations, _semanticModel, declarationSyntax, null, default, declarationIsImplicit); | ||
|
||
// If this is a using declaration, work out it's declaration kind |
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's [](start = 56, length = 4)
Should this be "its", or perhaps "the"?
&& declarationGroup.DeclarationKind != VariableDeclarationKind.Default) | ||
{ | ||
// visit the using decl with the following statements | ||
var followingStatements = ImmutableArray.Create(statements, Math.Min(i + 1, statements.Length - 1), statements.Length - i - 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.
Math.Min(i + 1, statements.Length - 1) [](start = 80, length = 38)
Won't this copy the last statement in the block, even if that is the using
statement? #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.
ImmutableArray.Create(statements, i + 1, statements.Length - i)
should work, even if i + 1 == statements.Length
.
In reply to: 335196847 [](ancestors = 335196847)
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.
But what about when there is only one statement in the list, which is a using decl? i = 0 => i + 1 = 1, statements.Length - i == 1
which is incorrect.
In reply to: 335199463 [](ancestors = 335199463,335196847)
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.
No, because the length becomes zero in that case and you end up with an empty array. I've added UsingDeclaration_Flow_11
to test this scenario.
In reply to: 335196847 [](ancestors = 335196847)
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.
IOperation and CFG support for using declarations
Fixes #32100