Skip to content
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

[IOperation] Differing variable declaration in Blocks loops between C# and VB #17998

Closed
333fred opened this issue Mar 20, 2017 · 6 comments
Closed
Labels
Area-Analyzers Bug Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature - IOperation IOperation
Milestone

Comments

@333fred
Copy link
Member

333fred commented Mar 20, 2017

C# Code:

for (int i = 0; i < 5; i++)
{
}

Resulting IOp Tree:

IForLoopStatement (LoopKind.For) (OperationKind.LoopStatement)
  Condition: IBinaryOperatorExpression (BinaryOperationKind.IntegerLessThan) (OperationKind.BinaryOperatorExpression, Type: System.Boolean)
      Left: ILocalReferenceExpression: i (OperationKind.LocalReferenceExpression, Type: System.Int32)
      Right: ILiteralExpression (Text: 5) (OperationKind.LiteralExpression, Type: System.Int32, Constant: 5)
  Local_1: System.Int32 i
  Before: IVariableDeclarationStatement (1 variables) (OperationKind.VariableDeclarationStatement)
      IVariableDeclaration: System.Int32 i (OperationKind.VariableDeclaration)
        Initializer: ILiteralExpression (Text: 0) (OperationKind.LiteralExpression, Type: System.Int32, Constant: 0)
  AtLoopBottom: IExpressionStatement (OperationKind.ExpressionStatement)
      IIncrementExpression (UnaryOperandKind.IntegerPostfixIncrement) (BinaryOperationKind.IntegerAdd) (OperationKind.IncrementExpression, Type: System.Int32)
        Left: ILocalReferenceExpression: i (OperationKind.LocalReferenceExpression, Type: System.Int32)
        Right: ILiteralExpression (OperationKind.LiteralExpression, Type: System.Int32, Constant: 1)
  IBlockStatement (0 statements) (OperationKind.BlockStatement)

Visual Basic code:

For i As Integer = 1 To 5
Next

Resulting IOp Tree:

IForLoopStatement (LoopKind.For) (OperationKind.LoopStatement)
  Condition: IBinaryOperatorExpression (BinaryOperationKind.IntegerLessThanOrEqual) (OperationKind.BinaryOperatorExpression, Type: System.Boolean)
      Left: ILocalReferenceExpression: i (OperationKind.LocalReferenceExpression, Type: System.Int32)
      Right: ILiteralExpression (Text: 5) (OperationKind.LiteralExpression, Type: System.Int32, Constant: 5)
  Before: IExpressionStatement (OperationKind.ExpressionStatement)
      IAssignmentExpression (OperationKind.AssignmentExpression, Type: System.Int32)
        Left: ILocalReferenceExpression: i (OperationKind.LocalReferenceExpression, Type: System.Int32)
        Right: ILiteralExpression (Text: 1) (OperationKind.LiteralExpression, Type: System.Int32, Constant: 1)
  AtLoopBottom: IExpressionStatement (OperationKind.ExpressionStatement)
      ICompoundAssignmentExpression (BinaryOperationKind.IntegerAdd) (OperationKind.CompoundAssignmentExpression, Type: System.Int32)
        Left: ILocalReferenceExpression: i (OperationKind.LocalReferenceExpression, Type: System.Int32)
        Right: IConversionExpression (ConversionKind.Basic, Explicit) (OperationKind.ConversionExpression, Type: System.Int32, Constant: 1)
            ILiteralExpression (OperationKind.LiteralExpression, Type: System.Int32, Constant: 1)
  IBlockStatement (0 statements) (OperationKind.BlockStatement)

There are a few differences here.

  1. There's no IVariableDeclaration(Statement) in VB. In fact, there doesn't appear to be any variable declarations in the accessible IOperation statements for VB.
  2. There's some inconsistency with getting access to the IForLoopStatement. Both languages have, syntax-wise, a ForStatement construct. In C#, this refers to the entire block. In VB, this refers just to the For i As Integer = 1 To 5 section, and is enclosed by a ForBlock construct. You can't get access to the IForLoopStatement from the ForStatement, only the ForBlock. While I can understand the difference between the languages here, perhaps we should allow getting the IForLoopStatement from the ForStatement in VB.
@333fred
Copy link
Member Author

333fred commented Mar 20, 2017

@dotnet/analyzer-ioperation

@333fred 333fred changed the title [IOperation] Differing variable declaration in For loops between C# and VB [IOperation] Differing variable declaration in Blocks loops between C# and VB Mar 21, 2017
@333fred
Copy link
Member Author

333fred commented Mar 21, 2017

This issue also affects If Then Else loops as well. FYI @jinujoseph

@CyrusNajmabadi
Copy link
Member

While I can understand the difference between the languages here, perhaps we should allow getting the IForLoopStatement from the ForStatement in VB.

VB statement/block handling has always been a bit baroque. We'll likely want to make an decision here across all VB statements. I view that as orthogonal to any other issues we find with individual elements.

@jinujoseph jinujoseph added the Concept-API This issue involves adding, removing, clarification, or modification of an API. label Apr 26, 2017
@jinujoseph jinujoseph self-assigned this May 3, 2017
@jinujoseph
Copy link
Contributor

While I can understand the difference between the languages here, perhaps we should allow getting the IForLoopStatement from the ForStatement in VB.

Design Decision:
We will not allow for this. It means you have a situation where two syntax nodes in a language will point to the same IOPeration. Then the IOperation's '.Syntax' will not poitn back at one of these. That violates a previous design decision.

For now, let's keep things similar between the languages. We'll only support getting the Operation on the VB block, and not on hte header statement.

@jinujoseph
Copy link
Contributor

There's no IVariableDeclaration(Statement) in VB. In fact, there doesn't appear to be any variable declarations in the accessible IOperation statements for VB.

This should be fixed by whoever works on these constructs. Any declared variables should show up in the operation tree somehow.

@jinujoseph jinujoseph added this to the 15.5 milestone Jul 13, 2017
@jinujoseph jinujoseph added the Bug label Jul 13, 2017
@tmat tmat modified the milestones: 15.5, 15.later Sep 6, 2017
@jinujoseph jinujoseph removed their assignment Sep 27, 2017
@jinujoseph
Copy link
Contributor

this should be fixed as per our new design forloops

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Analyzers Bug Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature - IOperation IOperation
Projects
None yet
Development

No branches or pull requests

4 participants