Add IOperation support for ILocalFunctionStatement by mavasani · Pull Request #20177 · dotnet/roslyn

mavasani · 2017-06-12T21:15:58Z

Fixes #19902

Fixes dotnet#19902

mavasani · 2017-06-12T21:16:22Z

Tagging @dotnet/analyzer-ioperation @dotnet/roslyn-compiler for review

333fred · 2017-06-12T22:56:02Z

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

+        private static ILocalFunctionStatement CreateBoundLocalFunctionStatementOperation(BoundLocalFunctionStatement boundLocalFunctionStatement)
+        {
+            IMethodSymbol localFunctionSymbol = boundLocalFunctionStatement.Symbol;
+            Lazy<IBlockStatement> body = new Lazy<IBlockStatement>(() => (IBlockStatement)Create(boundLocalFunctionStatement.Body));


Can we assert that the body is in fact a block statement before the lazy, that way if it ever fails it'll fail in a helpful location with a better stack? #ByDesign

Can we assert that the body is in fact a block statement before the lazy, that way if it ever fails it'll fail in a helpful location with a better stack?

It looks like Body is statically typed as BoundBlock.

In reply to: 121546721 [](ancestors = 121546721)

333fred · 2017-06-12T23:00:27Z

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

@@ -0,0 +1,309 @@
+// Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.


Can you add some tests for invalid local function syntax? Such as int Local() => ; #Resolved

Done

In reply to: 121547310 [](ancestors = 121547310)

AlekseyTs · 2017-06-13T06:03:40Z

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

+    /// </summary>
+    internal abstract partial class BaseLocalFunctionStatement : Operation, ILocalFunctionStatement
+    {
+        protected BaseLocalFunctionStatement(IMethodSymbol localFunctionSymbol, bool isInvalid, SyntaxNode syntax, ITypeSymbol type, Optional<object> constantValue) :


ITypeSymbol type, Optional constantValue [](start = 115, length = 48)

Are these ever not null? #Closed

I have used the common pattern already used in this generated file.

In reply to: 121588312 [](ancestors = 121588312)

Added a note to #19720 (comment) to address this review comment.

AlekseyTs · 2017-06-13T06:05:27Z

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

+        public LazyLocalFunctionStatement(IMethodSymbol localFunctionSymbol, Lazy<IBlockStatement> body, bool isInvalid, SyntaxNode syntax, ITypeSymbol type, Optional<object> constantValue)
+            : base(localFunctionSymbol, isInvalid, syntax, type, constantValue)
+        {
+            _lazyBody = body ?? throw new System.ArgumentNullException("body");


"body" [](start = 71, length = 6)

Use nameof? #Closed

Use nameof?

It looks like there was no any response to this comment (#20177 (review)). #Closed

I have used the existing pattern in this file. @heejaechang may want to address this in the generator. #Closed

I am confused, are you actually running a generator, or adding this code manually. If the latter, please consider addressing provided feedback. For feedback you didn't addressed because of using existing pattern, pleas open issues to follow up (applies to other similar comments). #Closed

I didn't address feedback as this will eventually become a generated file. I can file issues so we can address them in the generator when it is eventually checked in. Does that seem fine?

I can file issues so we can address them in the generator when it is eventually checked in. Does that seem fine?

Yes, as long as we have issues tracking the debt. #Closed

@AlekseyTs Added a note to #19720 (comment) to address this review comment.

AlekseyTs · 2017-06-13T06:10:24Z

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

+            bool isInvalid = boundLocalFunctionStatement.HasErrors;
+            SyntaxNode syntax = boundLocalFunctionStatement.Syntax;
+            ITypeSymbol type = null;
+            Optional<object> constantValue = default(Optional<object>);


Optional constantValue = default(Optional); [](start = 12, length = 59)

This and the previous statement should be pushed into the constructor, I think. #Closed

I have followed the general pattern used in this file. Heejae, you might want to take care of this when we checkin the generator.

In reply to: 121589002 [](ancestors = 121589002)

@AlekseyTs Added a note to #19720 (comment) to address this review comment.

AlekseyTs · 2017-06-13T06:15:51Z

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

+    IReturnStatement (OperationKind.ReturnStatement) (Syntax: 'x + Local2(p1)')
+      IBinaryOperatorExpression (BinaryOperationKind.IntegerAdd) (OperationKind.BinaryOperatorExpression, Type: System.Int32) (Syntax: 'x + Local2(p1)')
+        Left: ILocalReferenceExpression: x (OperationKind.LocalReferenceExpression, Type: System.Int32) (Syntax: 'x')
+        Right: IInvocationExpression (static System.Int32 Local2(System.Int32 p1)) (OperationKind.InvocationExpression, Type: System.Int32) (Syntax: 'Local2(p1)')


static [](start = 38, length = 6)

interesting, display says the local function is static. @agocke is this intentional? #Closed

Aleksey, this is due to the dumper printing out "static" when operation.Instance is null. See here: http://source.roslyn.io/#Roslyn.Test.Utilities/Compilation/OperationTreeVerifier.cs,539

In reply to: 121589587 [](ancestors = 121589587)

this is due to the dumper printing out "static" when operation.Instance is null.

We should fix that. The dumper should show what is there, not invent things to confuse people. #Closed

Filed #20194

AlekseyTs · 2017-06-13T06:19:41Z

Done with review pass. #Closed

AlekseyTs · 2017-06-14T04:40:39Z

Done with review pass. #Closed

mavasani · 2017-06-14T16:21:26Z

retest windows_debug_vs-integration_prtest please

mavasani · 2017-06-14T16:21:38Z

retest windows_release_vs-integration_prtest please

AlekseyTs

LGTM

…alFunctionStatement

mavasani · 2017-06-14T18:43:45Z

Ignoring the vs integration test failures, these are still not up yet.

mavasani · 2017-06-14T18:44:28Z

@jcouv @cston Can I get another review?

Add support for ILocalFunctionStatement

f94aaf2

Fixes dotnet#19902

dnfclas added the cla-already-signed label Jun 12, 2017

mavasani changed the title ~~Add support for ILocalFunctionStatement~~ Add IOperation support for ILocalFunctionStatement Jun 12, 2017

333fred reviewed Jun 12, 2017

View reviewed changes

AlekseyTs reviewed Jun 13, 2017

View reviewed changes

mavasani mentioned this pull request Jun 13, 2017

Fix operation tree dumper to not print "static" for local function invocations #20194

Closed

Address PR feedback

46a8ad1

mavasani mentioned this pull request Jun 14, 2017

OperationFactory/Operations.Generated.cs refactoring #19720

Closed

heejaechang mentioned this pull request Jun 14, 2017

Create Generator similar to bound node generator that creates basic IOperation types #20204

Closed

AlekseyTs approved these changes Jun 14, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/features/ioperation' into ILoc…

6d5eee8

…alFunctionStatement

mavasani mentioned this pull request Jun 14, 2017

Remove the AnalyzerManager singleton to prevent storing analyzer exec… #20109

Merged

mavasani added Area-Analyzers Area-Compilers labels Jun 14, 2017

jaredpar approved these changes Jun 14, 2017

View reviewed changes

mavasani merged commit 5492bf0 into dotnet:features/ioperation Jun 14, 2017

mavasani deleted the ILocalFunctionStatement branch June 14, 2017 22:05

mavasani mentioned this pull request Jun 14, 2017

Operation tree contains no nodes for local functions #19902

Closed

		@@ -0,0 +1,309 @@
		// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

Conversation

mavasani commented Jun 12, 2017

Uh oh!

mavasani commented Jun 12, 2017

Uh oh!

333fred Jun 12, 2017 • edited by mavasani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jun 12, 2017 • edited by mavasani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mavasani Jun 14, 2017 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mavasani commented Jun 14, 2017

Uh oh!

mavasani commented Jun 14, 2017

Uh oh!

AlekseyTs left a comment

333fred Jun 12, 2017 •

edited by mavasani

Loading

333fred Jun 12, 2017 •

edited by mavasani

Loading

AlekseyTs Jun 13, 2017 •

edited

Loading

AlekseyTs Jun 13, 2017 •

edited

Loading

AlekseyTs Jun 14, 2017 •

edited

Loading

mavasani Jun 14, 2017 •

edited by AlekseyTs

Loading

AlekseyTs Jun 14, 2017 •

edited

Loading

AlekseyTs Jun 14, 2017 •

edited

Loading

AlekseyTs Jun 13, 2017 •

edited

Loading

AlekseyTs Jun 13, 2017 •

edited

Loading

AlekseyTs Jun 13, 2017 •

edited

Loading

AlekseyTs commented Jun 13, 2017 •

edited

Loading

AlekseyTs commented Jun 14, 2017 •

edited

Loading