Handle IForToLoopOperation over numeric types in control flow graph #26734

AlekseyTs · 2018-05-09T05:42:08Z

No description provided.

AlekseyTs · 2018-05-09T16:44:19Z

@333fred, @mavasani, @cston, @dotnet/roslyn-compiler, @dotnet/analyzer-ioperation Please review #Closed

333fred · 2018-05-09T17:28:59Z

src/Compilers/Core/Portable/Operations/Loops/ForToLoopOperationInfo.cs

+
+namespace Microsoft.CodeAnalysis.Operations
+{
+    internal class ForToLoopOperationUserDefinedInfo


Any reason this can't just be a struct? #Resolved

Any reason this can't just be a struct?

It is not common to need this information. The intent is to save some memory for the common case.

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

333fred · 2018-05-09T17:30:11Z

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

-        public ForToLoopStatement(ImmutableArray<ILocalSymbol> locals, ILabelSymbol continueLabel, ILabelSymbol exitLabel, IOperation loopControlVariable, IOperation initialValue, IOperation limitValue, IOperation stepValue, IOperation body, ImmutableArray<IOperation> nextVariables, SemanticModel semanticModel, SyntaxNode syntax, ITypeSymbol type, Optional<object> constantValue, bool isImplicit) :
-            base(locals, continueLabel, exitLabel, semanticModel, syntax, type, constantValue, isImplicit)
+        public ForToLoopStatement(ImmutableArray<ILocalSymbol> locals, bool isChecked,
+                                  (ILocalSymbol LoopObject, ForToLoopOperationUserDefinedInfo UserDefinedInfo) info, 


[](start = 116, length = 1)

Lots of trailing whitespace. #WontFix

jaredpar · 2018-05-09T17:37:31Z

src/Compilers/Core/Portable/SpecialTypeExtensions.cs

+            }
+        }
+
+        public static int VBForToShiftBits(this SpecialType specialType)


Can we add a comment on the intent of this method or update the name. Unsure exactly what this is for at a glance. #Resolved

Can we add a comment on the intent of this method or update the name. Unsure exactly what this is for at a glance.

Will do

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

jaredpar · 2018-05-09T17:39:54Z

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

@@ -2364,11 +2364,21 @@ internal sealed partial class LazyForLoopStatement : BaseForLoopStatement, IForL
    /// </summary>
    internal abstract partial class BaseForToLoopStatement : LoopStatement, IForToLoopOperation


BaseForToLoopStatement [](start = 36, length = 22)

Which change is causing this generated file to get updated? Was expecting to see a change to on of our XML files or generators but not seeing that. #Resolved

@jaredpar We have still not hooked up the generator to this file: #20204. We should probably rename this file to remove the .generated.cs suffix until that is implemented. #Resolved

Ah okay. Thanks for the explanation.

As for renaming. I'm ambivalent. I don't want to create unnecessary work here if there is an issue tracking it.

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

I'm planning on addressing the issue during the hackathon.

In reply to: 187121166 [](ancestors = 187121166,187120137)

jaredpar · 2018-05-09T17:43:28Z

src/Test/Utilities/Portable/Compilation/OperationTreeVerifier.cs

+                _ = userDefinedInfo.Subtraction.Value;
+                _ = userDefinedInfo.LessThanOrEqual.Value;
+                _ = userDefinedInfo.GreaterThanOrEqual.Value;
+            }


Consider adding a method to ForToLoopOperationUserDefinedInfo to force the evaluation of the lazies instances rather than enumerating the fields here. This would be more robust in the face of adding additional fields in the future. #Resolved

Consider adding a method to ForToLoopOperationUserDefinedInfo to force the evaluation of the lazies instances rather than enumerating the fields here. This would be more robust in the face of adding additional fields in the future.

I do not expect us to add anything else to that class in the future. I also think this method would look strange in the class because it is only useful as a test.

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

jaredpar · 2018-05-09T17:44:06Z

src/Test/Utilities/Portable/Compilation/OperationTreeVerifier.cs


            Visit(operation.LoopControlVariable, "LoopControlVariable");
            Visit(operation.InitialValue, "InitialValue");
            Visit(operation.LimitValue, "LimitValue");
            Visit(operation.StepValue, "StepValue");
            Visit(operation.Body, "Body");
            VisitArray(operation.NextVariables, "NextVariables", logElementCount: true);
+
+            (ILocalSymbol loopObject, ForToLoopOperationUserDefinedInfo userDefinedInfo) = ((BaseForToLoopStatement)operation).Info;


loopObject [](start = 26, length = 10)

The loopObject element could be a discard. #WontFix

jaredpar · 2018-05-09T17:47:25Z

src/Test/Utilities/Portable/Compilation/TestOperationVisitor.cs

@@ -255,6 +260,16 @@ public override void VisitForToLoop(IForToLoopOperation operation)
        {
            VisitLoop(operation);
            Assert.Equal(LoopKind.ForTo, operation.LoopKind);
+            _ = operation.IsChecked;
+            (ILocalSymbol loopObject, ForToLoopOperationUserDefinedInfo userDefinedInfo) = ((BaseForToLoopStatement)operation).Info;


The loopObject element can be a discard. #WontFix

jaredpar · 2018-05-09T17:48:17Z

src/Test/Utilities/Portable/Compilation/TestOperationVisitor.cs

@@ -255,6 +260,16 @@ public override void VisitForToLoop(IForToLoopOperation operation)
        {
            VisitLoop(operation);
            Assert.Equal(LoopKind.ForTo, operation.LoopKind);
+            _ = operation.IsChecked;


This line looks suspicious. It implies there is a property here which is side effecting that needs to be executed. #Resolved

This line looks suspicious. It implies there is a property here which is side effecting that needs to be executed.

The purpose of this line is to evaluate the property to make sure it doesn't crash. This is a test code.

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

jaredpar · 2018-05-09T17:52:19Z

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

+        {
+            return new InvalidOperation(ImmutableArray.Create<IOperation>(child1, child2),
+                                        semanticModel: null, syntax, type,
+                                        constantValue: default, isImplicit: true);


Why are invalid operations implicit? #Resolved

Why are invalid operations implicit?

Usually, implicit operations are those that are synthesized. Like this one, for example. It is not a clone of a node present in the original tree.

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

333fred · 2018-05-09T18:02:05Z

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

+                }
+                else
+                {
+                    IOperation controlVariableReferenceforCondition = getLoopControlVariableRefernce(forceImplicit: true); // Yes we are going to reevaluate it again


getLoopControlVariableRefernce [](start = 70, length = 30)

typo: getLoopControlVariableRefernce -> getLoopControlVariableReference #Resolved

mavasani · 2018-05-09T18:16:25Z

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

+                        //RemovePlaceholderReplacement(forStatement.OperatorsOpt.LeftOperandPlaceholder)
+                        //RemovePlaceholderReplacement(forStatement.OperatorsOpt.RightOperandPlaceholder)
+                    }
+                    else if (!operation.StepValue.ConstantValue.HasValue &&


Can StepValue be null? If not, we probably want to update the doc comment. #Resolved

Can StepValue be null? If not, we probably want to update the doc comment.

I don't think it can ever be null. I'll update the doc comment. I will also adjust VisitForToLoop in TestOperationVisitor accordingly.

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

Probably also want to replace the null check with an assert in TestOperationVisitor #Resolved

Probably also want to replace the null check with an assert in TestOperationVisitor

Yes, the null check should be removed there

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

333fred · 2018-05-09T21:33:18Z

Tests look good.

333fred

Comments are OK to be addressed in a follow up PR.

AlekseyTs · 2018-05-09T23:45:31Z

@333fred, @mavasani, @jaredpar In my responses I identified the issues I am going to address in the next PR. These are:

a comment on VBForToShiftBits Handle IForToLoopOperation over numeric types in control flow graph #26734 (review)
a typo in getLoopControlVariableRefernce
Handle IForToLoopOperation over numeric types in control flow graph #26734 (review)
Reflect non-optional nature of StepValue value in doc comment and adjust TestOperationVisitor accordingly Handle IForToLoopOperation over numeric types in control flow graph #26734 (review)

Please let me know if you consider this issues, or any other issues, blocking for this PR. Otherwise, could you please sign-off so that I can resolve merge conflicts and merge the PR, it is going to conflict with any other builder change merged into the branch.

AlekseyTs · 2018-05-10T16:46:44Z

@cston, @dotnet/roslyn-compiler Please review, need a second review

jaredpar

…h_30

Handle IForToLoopOperation over numeric types in control flow graph

3560bde

AlekseyTs added Language-VB Area-Compilers Feature - IOperation IOperation Feature - Flow Analysis Flow Analysis labels May 9, 2018

AlekseyTs requested review from 333fred, mavasani, cston and a team May 9, 2018 05:42

333fred reviewed May 9, 2018

View reviewed changes

jaredpar reviewed May 9, 2018

View reviewed changes

333fred reviewed May 9, 2018

View reviewed changes

mavasani reviewed May 9, 2018

View reviewed changes

333fred approved these changes May 9, 2018

View reviewed changes

mavasani approved these changes May 9, 2018

View reviewed changes

jaredpar approved these changes May 10, 2018

View reviewed changes

AlekseyTs added 2 commits May 10, 2018 11:48

Merge remote-tracking branch 'dotnet/features/dataflow' into FlowGrap…

310f482

…h_30

Follow-up on merge

7dc65af

AlekseyTs merged commit fe66c82 into dotnet:features/dataflow May 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle IForToLoopOperation over numeric types in control flow graph #26734

Handle IForToLoopOperation over numeric types in control flow graph #26734

AlekseyTs commented May 9, 2018

AlekseyTs commented May 9, 2018 •

edited

Loading

333fred May 9, 2018 •

edited by AlekseyTs

Loading

AlekseyTs May 9, 2018

333fred May 9, 2018 •

edited by AlekseyTs

Loading

jaredpar May 9, 2018 •

edited by AlekseyTs

Loading

AlekseyTs May 9, 2018

jaredpar May 9, 2018 •

edited by jcouv

Loading

mavasani May 9, 2018 •

edited by jcouv

Loading

jaredpar May 9, 2018

333fred May 9, 2018

jaredpar May 9, 2018 •

edited by AlekseyTs

Loading

AlekseyTs May 9, 2018

jaredpar May 9, 2018 •

edited by AlekseyTs

Loading

jaredpar May 9, 2018 •

edited by AlekseyTs

Loading

jaredpar May 9, 2018 •

edited by AlekseyTs

Loading

AlekseyTs May 9, 2018

jaredpar May 9, 2018 •

edited by AlekseyTs

Loading

AlekseyTs May 9, 2018

333fred May 9, 2018 •

edited by AlekseyTs

Loading

mavasani May 9, 2018 •

edited by AlekseyTs

Loading

AlekseyTs May 9, 2018 •

edited

Loading

mavasani May 9, 2018 •

edited by AlekseyTs

Loading

AlekseyTs May 9, 2018

333fred commented May 9, 2018

333fred left a comment

AlekseyTs commented May 9, 2018

AlekseyTs commented May 10, 2018

jaredpar left a comment

		@@ -2364,11 +2364,21 @@ internal sealed partial class LazyForLoopStatement : BaseForLoopStatement, IForL
		/// </summary>
		internal abstract partial class BaseForToLoopStatement : LoopStatement, IForToLoopOperation

Handle IForToLoopOperation over numeric types in control flow graph #26734

Handle IForToLoopOperation over numeric types in control flow graph #26734

Conversation

AlekseyTs commented May 9, 2018

AlekseyTs commented May 9, 2018 • edited Loading

333fred May 9, 2018 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred May 9, 2018 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

jaredpar May 9, 2018 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar May 9, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

mavasani May 9, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar May 9, 2018 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar May 9, 2018 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

jaredpar May 9, 2018 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

jaredpar May 9, 2018 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar May 9, 2018 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred May 9, 2018 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

mavasani May 9, 2018 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs May 9, 2018 • edited Loading

Choose a reason for hiding this comment

mavasani May 9, 2018 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred commented May 9, 2018

333fred left a comment

Choose a reason for hiding this comment

AlekseyTs commented May 9, 2018

AlekseyTs commented May 10, 2018

jaredpar left a comment

Choose a reason for hiding this comment

AlekseyTs commented May 9, 2018 •

edited

Loading

333fred May 9, 2018 •

edited by AlekseyTs

Loading

333fred May 9, 2018 •

edited by AlekseyTs

Loading

jaredpar May 9, 2018 •

edited by AlekseyTs

Loading

jaredpar May 9, 2018 •

edited by jcouv

Loading

mavasani May 9, 2018 •

edited by jcouv

Loading

jaredpar May 9, 2018 •

edited by AlekseyTs

Loading

jaredpar May 9, 2018 •

edited by AlekseyTs

Loading

jaredpar May 9, 2018 •

edited by AlekseyTs

Loading

jaredpar May 9, 2018 •

edited by AlekseyTs

Loading

jaredpar May 9, 2018 •

edited by AlekseyTs

Loading

333fred May 9, 2018 •

edited by AlekseyTs

Loading

mavasani May 9, 2018 •

edited by AlekseyTs

Loading

AlekseyTs May 9, 2018 •

edited

Loading

mavasani May 9, 2018 •

edited by AlekseyTs

Loading