IOperation API extensions to expose low-level operations #21705

edgardozoppi · 2017-08-23T22:37:28Z

Since there are several requested changes I have decided to create a new PR #21810 with those changes.
Please take a look to the new PR #21810 instead.

These changes add a new overload to the SemanticModel.GetOperation method to generate an IOperation tree from the compiler's internal BoundTree after performing the local rewriting transformation phase. The new low-level IOperation tree reuses most of the original IOperation interfaces and extend them by adding a few newly created ones that can only appear after the local rewriting phase. This has been done to support both C# and VB.

Remarks
These changes are experimental so we still need to keep them behind the IOperation feature flag even after releasing the API in Dev 15.5.

More detailed information of the changes can be found in this issue #21507.

…tions after the local rewritting phase.

…ase.

CyrusNajmabadi · 2017-08-23T22:58:32Z

and extend them by adding a few newly created ones that can only appear after the local rewriting phase.

Can you give a rundown on what these are? Thanks!

CyrusNajmabadi · 2017-08-23T22:59:14Z

after performing the local rewriting transformation phase.

Can you give a brief explanation of what phases of lowering this does? i.e. what happens to lambdas, iterators, async, other-code after the "local rewriting" phase.

Thanks!

edgardozoppi · 2017-08-24T21:48:49Z

Can you give a rundown on what these are? Thanks!

Please take a look to this issue #21507.

Can you give a brief explanation of what phases of lowering this does? i.e. what happens to lambdas, iterators, async, other-code after the "local rewriting" phase.

This new GetOperation method overload only performs the local rewriting lowering phase by using the LocalRewriter classes (C# and VB versions respectively). Not sure exactly what are all the languages constructs that this phase transforms. I guess that is a question that someone from the compiler team can answer more precisely. You can take a look to the LocalRewriter implementation for C# here and for VB here.

There are other lowering phases to transform local functions, lambdas, iterators, async/await, etc. All those phases happen after the local rewriting when the compiler has to emit code. But for generating the low-level IOperation tree we are only using the first phase.

agocke

I would expect some unit test coverage for these changes before merging.

agocke · 2017-08-24T22:54:45Z

src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs

+            var compilationState = new TypeCompilationState(method.ContainingType, _compilation, null);
+
+            var dynamicAnalysisSpans = ImmutableArray<CodeAnalysis.CodeGen.SourceSpan>.Empty;
+            //var synthesizedSubmissionFields = method.ContainingType.IsSubmissionClass ? new SynthesizedSubmissionFields(_compilation, method.ContainingType) : null;


Remove commented-out code.

agocke · 2017-08-24T22:55:15Z

src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs

+            var loweredBody = LocalRewriter.Rewrite(
+                    _compilation,
+                    method,
+                    -1,


Magic number

agocke · 2017-08-24T23:01:02Z

src/Compilers/CSharp/Portable/Compilation/SyntaxTreeSemanticModel.cs

+            node = SyntaxFactory.GetStandaloneNode(node);
+
+            var model = this.GetMemberModel(node);
+            if (model != null)


return model?.GetOperationWorker(method, node, cancellationToken)

agocke · 2017-08-24T23:04:18Z

src/Compilers/Core/Portable/Operations/IConditionalGotoStatement.cs

+        /// </summary>
+        ILabelSymbol Target { get; }
+        /// <summary>
+        /// Jump if the condition is true.


This is unclear. What does it do if this is false and the condition is true?

I guess it won't jump in that case. I just copied that property from BoundConditionalGoto.JumpIfTrue property. I think it means if the conditional goto branch is behaving like JE (jumpe on equals) or JNE (jump on not equals). So, if the property JumpIfTrue is false, it will only jump when the condition is false.

agocke · 2017-08-24T23:06:41Z

src/Compilers/Core/Portable/Generated/Operations.xml.Generated.cs

@@ -858,6 +858,173 @@ public LazyBlockStatement(Lazy<ImmutableArray<IOperation>> statements, Immutable
    }

    /// <summary>
+    /// Represents a sequence of expressions.
+    /// </summary>
+    internal abstract partial class BaseSequenceExpression : Operation, ISequenceExpression


I see generated code here, but I don't see any changes to a generate base.

@agocke even though file is .generated.cs, it is a hand crafted file. #20204 tracks writing an operation generator similar to the syntax and bound node generators.

@mavasani This is very confusing. Can we please rename it to not included generated in the file name?

mavasani

LGTM, modulo addition of unit tests. @edgardozoppi Lets work today on adding unit tests for the new APIs.

mavasani · 2017-08-25T16:27:40Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

@@ -465,11 +465,18 @@ private int CheckAndAdjustPositionForSpeculativeAttribute(int position)

        #endregion Helpers for speculative binding

-        protected override IOperation GetOperationCore(SyntaxNode node, CancellationToken cancellationToken)
+        protected override IOperation GetOperationCore(IMethodSymbol method, SyntaxNode body, CancellationToken cancellationToken)


Can we consider changing the API to take only one of the symbol OR body parameters? Alternatively, please file a separate issue to track removal of one of these parameters.

mavasani · 2017-08-25T16:39:40Z

src/Compilers/Core/Portable/Generated/Operations.xml.Generated.cs

@@ -858,6 +858,173 @@ public LazyBlockStatement(Lazy<ImmutableArray<IOperation>> statements, Immutable
    }

    /// <summary>
+    /// Represents a sequence of expressions.
+    /// </summary>
+    internal abstract partial class BaseSequenceExpression : Operation, ISequenceExpression


@agocke even though file is .generated.cs, it is a hand crafted file. #20204 tracks writing an operation generator similar to the syntax and bound node generators.

…d overload.

AlekseyTs · 2017-08-29T04:15:51Z

src/Compilers/Core/Portable/Compilation/SemanticModel.cs

+        /// <param name="method">The method symbol.</param>
+        /// <param name="cancellationToken">An optional cancellation token.</param>
+        /// <returns></returns>
+        public IOperation GetOperation(IMethodSymbol method, CancellationToken cancellationToken = default)


public IOperation GetOperation(IMethodSymbol method, CancellationToken cancellationToken = default) [](start = 8, length = 99)

It is not clear why this API is on SemanticModel. SemanticModel is created for a particular syntax tree, this API doesn't depend on syntax at all. Consider moving it to Compilation.

AlekseyTs · 2017-08-29T04:19:09Z

src/Compilers/Core/Portable/Operations/ISequenceExpression.cs

+        /// <summary>
+        /// Expressions contained within the sequence.
+        /// </summary>
+        ImmutableArray<IOperation> Expressions { get; }


Expressions [](start = 35, length = 11)

SideEffects?

AlekseyTs · 2017-08-29T04:31:00Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

-            var csnode = (CSharpSyntaxNode)node;
-            CheckSyntaxNode(csnode);
-            return this.GetOperationWorker(csnode, GetOperationOptions.Lowest, cancellationToken);
+            var csmethod = (MethodSymbol)method;


(MethodSymbol)method [](start = 27, length = 20)

Please use EnsureCSharpSymbolOrNull helper.

AlekseyTs · 2017-08-29T04:32:35Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

-            CheckSyntaxNode(csnode);
-            return this.GetOperationWorker(csnode, GetOperationOptions.Lowest, cancellationToken);
+            var csmethod = (MethodSymbol)method;
+            CheckSymbol(csmethod);


CheckSymbol(csmethod); [](start = 12, length = 22)

It looks like this helper is using "symbol" as parameter name, I would expect "method" to be used for this call site instead.

AlekseyTs · 2017-08-29T04:37:16Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

+            var csmethod = (MethodSymbol)method;
+            CheckSymbol(csmethod);
+            var csbody = csmethod.GetBodySyntax();
+            CheckSyntaxNode(csbody);


CheckSyntaxNode(csbody); [](start = 12, length = 24)

I think consumer might be confused by an exception thrown by this helper because syntax is not an input for the GetOperation API.

AlekseyTs · 2017-08-29T04:45:02Z

src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs

+        private BoundStatement LowerMethodBody(MethodSymbol method, BoundStatement body)
+        {
+            const int methodOrdinal = -1;
+            var diagnostics = DiagnosticBag.GetInstance();


DiagnosticBag.GetInstance() [](start = 30, length = 27)

Are we leaking diagnostics bag from the pool?

AlekseyTs · 2017-08-29T04:48:10Z

src/Compilers/CSharp/Portable/Symbols/MethodSymbolExtensions.cs

+
+        internal static CSharpSyntaxNode GetBodySyntax(this MethodSymbol method)
+        {
+            var syntaxReference = method.DeclaringSyntaxReferences.FirstOrDefault();


var [](start = 12, length = 3)

Please spell out the type.

AlekseyTs · 2017-08-29T04:50:22Z

src/Compilers/CSharp/Portable/Symbols/MethodSymbolExtensions.cs

+        internal static CSharpSyntaxNode GetBodySyntax(this MethodSymbol method)
+        {
+            var syntaxReference = method.DeclaringSyntaxReferences.FirstOrDefault();
+            var node = syntaxReference.GetCSharpSyntax();


syntaxReference [](start = 23, length = 15)

Could syntaxReference be null here? Would it be safe to call GetCSharpSyntax in this case?

AlekseyTs · 2017-08-29T17:49:33Z

src/Compilers/CSharp/Portable/Symbols/MethodSymbolExtensions.cs

+
+        internal static CSharpSyntaxNode GetBodySyntax(this MethodSymbol method)
+        {
+            var syntaxReference = method.DeclaringSyntaxReferences.FirstOrDefault();


FirstOrDefault [](start = 67, length = 14)

It is not clear why FirstOrDefault is the right thing to do. It is not clear why are we doing this through DeclaringSyntaxReferences at all. I think that we should only handle top-level methods explicitly declared in source (not lambdas, not local functions, etc.) It'll probably make sense to do what MethodCompiler.BindMethodBody does rather than inventing some new way of getting the bound body, perhaps share some code with that method.

AlekseyTs · 2017-08-29T17:57:52Z

src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs

+                // as the start of a tree to get operations for. This is guaranteed by the builder that populates the node map, as it will call
+                // UnboundLambda.BindForErrorRecovery() when it encounters an UnboundLambda node.
+                Debug.Assert(loweredBody.Kind != BoundKind.UnboundLambda);
+                result = _operationFactory.Create(loweredBody);


result = _operationFactory.Create(loweredBody); [](start = 16, length = 47)

It feels wrong to tie lowered tree to any SemanticModel. SemanticModel deals with un-lowered bound trees, who knows what negative effects will this operation have, how will it affect the caches, etc. I think there should be no interaction between SemanticModel and the tree built of off lowered bound nodes.

AlekseyTs · 2017-08-29T18:06:45Z

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs

+        private IBlockStatement CreateBoundStatementListOperation(BoundStatementList boundStatementList)
+        {
+            Lazy<ImmutableArray<IOperation>> statements =
+                new Lazy<ImmutableArray<IOperation>>(() => boundStatementList.Statements.Select(s => Create(s)).Where(s => s != null && s.Kind != OperationKind.None).ToImmutableArray());


s.Kind != OperationKind.None [](start = 136, length = 28)

Why is this the right condition? Isn't this potentially going to remove significant portion of the tree?

AlekseyTs · 2017-08-29T18:07:02Z

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs

+        private ISequenceExpression CreateBoundSequenceOperation(BoundSequence boundSequence)
+        {
+            Lazy<ImmutableArray<IOperation>> expressions =
+                new Lazy<ImmutableArray<IOperation>>(() => boundSequence.SideEffects.Select(s => Create(s)).Where(s => s != null && s.Kind != OperationKind.None).ToImmutableArray());


s.Kind != OperationKind.None [](start = 132, length = 28)

Same question.

AlekseyTs · 2017-08-29T18:11:02Z

src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IConditionalGotoStatement.cs

+string expectedOperationTree = @"
+IBlockStatement (1 statements) (OperationKind.BlockStatement) (Syntax: '{ ... }')
+  IBlockStatement (3 statements) (OperationKind.BlockStatement) (Syntax: 'if (p < 0) p = 0;')
+    IConditionalGotoStatement (JumpIfTrue: False) (OperationKind.ConditionalGotoStatement) (Syntax: 'p < 0')


IConditionalGotoStatement (JumpIfTrue: False) (OperationKind.ConditionalGotoStatement) (Syntax: 'p < 0') [](start = 4, length = 104)

From this output, how do I make sense where the jump is going to?

The problem is that the labels of IConditionalGotoStatement are automatically generated by the compiler when lowering the method body, so they have different names depending on the order in which the unit tests run.

AlekseyTs · 2017-08-29T18:12:48Z

src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IConditionalGotoStatement.cs

+      Expression: ISimpleAssignmentExpression (OperationKind.SimpleAssignmentExpression, Type: System.Int32) (Syntax: 'p = 0')
+          Left: IParameterReferenceExpression: p (OperationKind.ParameterReferenceExpression, Type: System.Int32) (Syntax: 'p')
+          Right: ILiteralExpression (Text: 0) (OperationKind.LiteralExpression, Type: System.Int32, Constant: 0) (Syntax: '0')
+    IBranchStatement (BranchKind.GoTo) (OperationKind.BranchStatement) (Syntax: 'if (p < 0)  ... else p = 1;')


IBranchStatement (BranchKind.GoTo) (OperationKind.BranchStatement) (Syntax: 'if (p < 0) ... else p = 1;') [](start = 4, length = 106)

How can I determine where this branch jumps to?

Same as before.

AlekseyTs · 2017-08-29T18:17:19Z

It looks like added tests do not cover all added/modified code paths.

AlekseyTs · 2017-08-29T18:18:09Z

Done with review pass for C# side of changes (iteration 12).

edgardozoppi · 2017-08-30T08:23:46Z

Since there are several requested changes I have decided to create a new PR #21810 with those changes. Also, because I did the changes in a new branch in my fork that has all the latest updates of the features/ioperation branch.

Please continue the review in the new PR #21810.

edgardozoppi added 8 commits July 26, 2017 18:14

Adding GetOperation method overload in SemanticModel to get the opera…

78c6e64

…tions after the local rewritting phase.

Starting to add support for getting IOperation tree after lowering ph…

6fe7c1c

…ase.

Adding ISequenceExpression operation.

32b71c6

Fixing bug to return low-level IOperation.

3844589

Adding IOperationKind.SequenceExpression.

db07ba9

Create low-level IOperations for valid bound nodes.

eab1f61

Adding support for VB.

9d1c3f4

Fixing bug by skipping null IOperations.

47a2e94

edgardozoppi added Area-Analyzers Area-Compilers Feature - IOperation IOperation labels Aug 23, 2017

edgardozoppi self-assigned this Aug 23, 2017

edgardozoppi requested review from agocke, sharwell, gafter, ManishJayaswal, mavasani, jinujoseph and a team August 23, 2017 22:37

dnfclas added the cla-already-signed label Aug 23, 2017

AlekseyTs requested a review from a team August 24, 2017 16:53

agocke requested changes Aug 24, 2017

View reviewed changes

mavasani approved these changes Aug 25, 2017

View reviewed changes

mavasani requested review from a team August 25, 2017 16:41

edgardozoppi added 2 commits August 25, 2017 15:18

Some changes requested in the PR.

1d79764

Removing body parameter from the new SemanticModel.GetOperation metho…

5dd510b

…d overload.

edgardozoppi added 2 commits August 28, 2017 16:38

Adding unit test for IConditionalGotoStatement operation.

bffb09e

Adding unit tests and fixing compiler generated labels bug.

ecffe01

AlekseyTs reviewed Aug 29, 2017

View reviewed changes

edgardozoppi mentioned this pull request Aug 30, 2017

IOperation API extensions to expose low-level operations (v2) #21810

Closed

edgardozoppi closed this Aug 30, 2017

IOperation API extensions to expose low-level operations #21705

IOperation API extensions to expose low-level operations #21705

Conversation

edgardozoppi commented Aug 23, 2017 • edited Loading

CyrusNajmabadi commented Aug 23, 2017

CyrusNajmabadi commented Aug 23, 2017

edgardozoppi commented Aug 24, 2017

agocke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Aug 29, 2017

AlekseyTs commented Aug 29, 2017

edgardozoppi commented Aug 30, 2017

edgardozoppi commented Aug 23, 2017 •

edited

Loading

AlekseyTs Aug 29, 2017 •

edited

Loading