DYN-1541: Issues with code block function default arguments (#9528)

ColinDayOrg · web-flow · commit 30e14a13b7ef · 2019-03-26T07:15:42.000-07:00
* Update changes from librarie.js to fix QNTM-3710

* Add some initial default argument tests

* Add one argument one default test

* Comment out a purposely failing test

* Test multiple code blocks with default arguments

* Add a (commented out) test that fails due to indeterminate functions

* Add tests for deleting code block nodes with functions with default parameters

* Remove deafult arguments to avoid a potential crash

* Comment out code block deletion for testing

* Uncomment test code

* remove emitting if statement node for associative inline conditionals

* obsolete property

* Add recordable test for node deletion

* Restore public signature

* Remove obsolete test and update comment

* Use correct issue name in comment

* Fix typo
diff --git a/src/Engine/ProtoAssociative/CodeGen.cs b/src/Engine/ProtoAssociative/CodeGen.cs
@@ -3484,7 +3484,6 @@ private void EmitConstructorDefinitionNode(AssociativeNode node, ref ProtoCore.T
                         EmitBinaryExpressionNode(bNodeTemp, ref inferedType);
                         //duild an inline conditional node for statement: defaultarg = (temp == DefaultArgNode) ? defaultValue : temp;
                         InlineConditionalNode icNode = new InlineConditionalNode();
-                        icNode.IsAutoGenerated = true;
                         BinaryExpressionNode cExprNode = new BinaryExpressionNode();
                         cExprNode.Optr = ProtoCore.DSASM.Operator.eq;
                         cExprNode.LeftNode = iNodeTemp;
@@ -3817,7 +3816,6 @@ private void EmitFunctionDefinitionNode(AssociativeNode node, ref ProtoCore.Type
                         EmitBinaryExpressionNode(bNodeTemp, ref inferedType, false, null, ProtoCore.CompilerDefinitions.SubCompilePass.UnboundIdentifier);
                         //duild an inline conditional node for statement: defaultarg = (temp == DefaultArgNode) ? defaultValue : temp;
                         InlineConditionalNode icNode = new InlineConditionalNode();
-                        icNode.IsAutoGenerated = true;
                         BinaryExpressionNode cExprNode = new BinaryExpressionNode();
                         cExprNode.Optr = ProtoCore.DSASM.Operator.eq;
                         cExprNode.LeftNode = iNodeTemp;
@@ -4308,132 +4306,118 @@ private void EmitInlineConditionalNode(AssociativeNode node, ref ProtoCore.Type
             }
 
             InlineConditionalNode inlineConditionalNode = node as InlineConditionalNode;
-            // TODO: Jun, this 'if' condition needs to be removed as it was the old implementation - pratapa
-            if (inlineConditionalNode.IsAutoGenerated)
-            {
-                // Normal inline conditional
-
-                IfStatementNode ifNode = new IfStatementNode();
-                ifNode.ifExprNode = inlineConditionalNode.ConditionExpression;
-                List<AssociativeNode> ifBody = new List<AssociativeNode>();
-                List<AssociativeNode> elseBody = new List<AssociativeNode>();
-                ifBody.Add(inlineConditionalNode.TrueExpression);
-                elseBody.Add(inlineConditionalNode.FalseExpression);
-                ifNode.IfBody = ifBody;
-                ifNode.ElseBody = elseBody;
+            // CPS inline conditional
 
-                EmitIfStatementNode(ifNode, ref inferedType, graphNode);
-            }
-            else
-            {
-                // CPS inline conditional
-                
-                FunctionCallNode inlineCall = new FunctionCallNode();
-                IdentifierNode identNode = new IdentifierNode();
-                identNode.Name = ProtoCore.DSASM.Constants.kInlineConditionalMethodName;
-                inlineCall.Function = identNode;
+            FunctionCallNode inlineCall = new FunctionCallNode();
+            IdentifierNode identNode = new IdentifierNode();
+            identNode.Name = ProtoCore.DSASM.Constants.kInlineConditionalMethodName;
+            inlineCall.Function = identNode;
 
-                DebugProperties.BreakpointOptions oldOptions = core.DebuggerProperties.breakOptions;
-                DebugProperties.BreakpointOptions newOptions = oldOptions;
-                newOptions |= DebugProperties.BreakpointOptions.EmitInlineConditionalBreakpoint;
-                core.DebuggerProperties.breakOptions = newOptions;
+            DebugProperties.BreakpointOptions oldOptions = core.DebuggerProperties.breakOptions;
+            DebugProperties.BreakpointOptions newOptions = oldOptions;
+            newOptions |= DebugProperties.BreakpointOptions.EmitInlineConditionalBreakpoint;
+            core.DebuggerProperties.breakOptions = newOptions;
 
-                core.DebuggerProperties.highlightRange = new ProtoCore.CodeModel.CodeRange
+            core.DebuggerProperties.highlightRange = new ProtoCore.CodeModel.CodeRange
+            {
+                StartInclusive = new ProtoCore.CodeModel.CodePoint
                 {
-                    StartInclusive = new ProtoCore.CodeModel.CodePoint
-                    {
-                        LineNo = parentNode.line,
-                        CharNo = parentNode.col
-                    },
+                    LineNo = parentNode.line,
+                    CharNo = parentNode.col
+                },
 
-                    EndExclusive = new ProtoCore.CodeModel.CodePoint
-                    {
-                        LineNo = parentNode.endLine,
-                        CharNo = parentNode.endCol
-                    }
-                };
-
-                // As SSA conversion is enabled, we have got the values of
-                // true and false branch, so it isn't necessary to create 
-                // language blocks.
-                if (core.Options.GenerateSSA)
+                EndExclusive = new ProtoCore.CodeModel.CodePoint
                 {
-                    inlineCall.FormalArguments.Add(inlineConditionalNode.ConditionExpression);
-                    inlineCall.FormalArguments.Add(inlineConditionalNode.TrueExpression);
-                    inlineCall.FormalArguments.Add(inlineConditionalNode.FalseExpression);
+                    LineNo = parentNode.endLine,
+                    CharNo = parentNode.endCol
                 }
-                else
-                {
-                    // True condition language block
-                    BinaryExpressionNode bExprTrue = AstFactory.BuildReturnStatement(inlineConditionalNode.TrueExpression);
-
-                    LanguageBlockNode langblockT = new LanguageBlockNode();
-                    int trueBlockId = Constants.kInvalidIndex;
-                    langblockT.codeblock.Language = ProtoCore.Language.Associative;
-                    core.AssocNode = bExprTrue;
-                    core.InlineConditionalBodyGraphNodes.Push(new List<GraphNode>());
-                    EmitDynamicLanguageBlockNode(langblockT, bExprTrue, ref inferedType, ref trueBlockId, graphNode, ProtoCore.CompilerDefinitions.SubCompilePass.None);
-                    List<GraphNode> trueBodyNodes = core.InlineConditionalBodyGraphNodes.Pop();
-
-                    // Append dependent nodes of the inline conditional 
-                    foreach (GraphNode gnode in trueBodyNodes)
-                        foreach (GraphNode dNode in gnode.dependentList)
-                            graphNode.PushDependent(dNode);
-
-                    core.AssocNode = null;
-                    DynamicBlockNode dynBlockT = new DynamicBlockNode(trueBlockId);
-
-                    // False condition language block
-                    BinaryExpressionNode bExprFalse = AstFactory.BuildReturnStatement(inlineConditionalNode.FalseExpression);
+            };
 
-                    LanguageBlockNode langblockF = new LanguageBlockNode();
-                    int falseBlockId = Constants.kInvalidIndex;
-                    langblockF.codeblock.Language = ProtoCore.Language.Associative;
-                    core.AssocNode = bExprFalse;
-                    core.InlineConditionalBodyGraphNodes.Push(new List<GraphNode>());
-                    EmitDynamicLanguageBlockNode(langblockF, bExprFalse, ref inferedType, ref falseBlockId, graphNode, ProtoCore.CompilerDefinitions.SubCompilePass.None);
-                    List<GraphNode> falseBodyNodes = core.InlineConditionalBodyGraphNodes.Pop();
+            // As SSA conversion is enabled, we have got the values of
+            // true and false branch, so it isn't necessary to create 
+            // language blocks.
+            if (core.Options.GenerateSSA)
+            {
+                inlineCall.FormalArguments.Add(inlineConditionalNode.ConditionExpression);
+                inlineCall.FormalArguments.Add(inlineConditionalNode.TrueExpression);
+                inlineCall.FormalArguments.Add(inlineConditionalNode.FalseExpression);
+            }
+            else
+            {
+                // True condition language block
+                BinaryExpressionNode bExprTrue = AstFactory.BuildReturnStatement(inlineConditionalNode.TrueExpression);
+
+                LanguageBlockNode langblockT = new LanguageBlockNode();
+                int trueBlockId = Constants.kInvalidIndex;
+                langblockT.codeblock.Language = ProtoCore.Language.Associative;
+                core.AssocNode = bExprTrue;
+                core.InlineConditionalBodyGraphNodes.Push(new List<GraphNode>());
+                EmitDynamicLanguageBlockNode(langblockT, bExprTrue, ref inferedType, ref trueBlockId, graphNode,
+                    ProtoCore.CompilerDefinitions.SubCompilePass.None);
+                List<GraphNode> trueBodyNodes = core.InlineConditionalBodyGraphNodes.Pop();
+
+                // Append dependent nodes of the inline conditional 
+                foreach (GraphNode gnode in trueBodyNodes)
+                foreach (GraphNode dNode in gnode.dependentList)
+                    graphNode.PushDependent(dNode);
 
-                    // Append dependent nodes of the inline conditional 
-                    foreach (GraphNode gnode in falseBodyNodes)
-                        foreach (GraphNode dNode in gnode.dependentList)
-                            graphNode.PushDependent(dNode);
+                core.AssocNode = null;
+                DynamicBlockNode dynBlockT = new DynamicBlockNode(trueBlockId);
+
+                // False condition language block
+                BinaryExpressionNode bExprFalse =
+                    AstFactory.BuildReturnStatement(inlineConditionalNode.FalseExpression);
+
+                LanguageBlockNode langblockF = new LanguageBlockNode();
+                int falseBlockId = Constants.kInvalidIndex;
+                langblockF.codeblock.Language = ProtoCore.Language.Associative;
+                core.AssocNode = bExprFalse;
+                core.InlineConditionalBodyGraphNodes.Push(new List<GraphNode>());
+                EmitDynamicLanguageBlockNode(langblockF, bExprFalse, ref inferedType, ref falseBlockId, graphNode,
+                    ProtoCore.CompilerDefinitions.SubCompilePass.None);
+                List<GraphNode> falseBodyNodes = core.InlineConditionalBodyGraphNodes.Pop();
+
+                // Append dependent nodes of the inline conditional 
+                foreach (GraphNode gnode in falseBodyNodes)
+                foreach (GraphNode dNode in gnode.dependentList)
+                    graphNode.PushDependent(dNode);
 
-                    core.AssocNode = null;
-                    DynamicBlockNode dynBlockF = new DynamicBlockNode(falseBlockId);
+                core.AssocNode = null;
+                DynamicBlockNode dynBlockF = new DynamicBlockNode(falseBlockId);
 
-                    inlineCall.FormalArguments.Add(inlineConditionalNode.ConditionExpression);
-                    inlineCall.FormalArguments.Add(dynBlockT);
-                    inlineCall.FormalArguments.Add(dynBlockF);
-                }
+                inlineCall.FormalArguments.Add(inlineConditionalNode.ConditionExpression);
+                inlineCall.FormalArguments.Add(dynBlockT);
+                inlineCall.FormalArguments.Add(dynBlockF);
+            }
 
-                core.DebuggerProperties.breakOptions = oldOptions;
-                core.DebuggerProperties.highlightRange = new ProtoCore.CodeModel.CodeRange
+            core.DebuggerProperties.breakOptions = oldOptions;
+            core.DebuggerProperties.highlightRange = new ProtoCore.CodeModel.CodeRange
+            {
+                StartInclusive = new ProtoCore.CodeModel.CodePoint
                 {
-                    StartInclusive = new ProtoCore.CodeModel.CodePoint
-                    {
-                        LineNo = Constants.kInvalidIndex,
-                        CharNo = Constants.kInvalidIndex
-                    },
-
-                    EndExclusive = new ProtoCore.CodeModel.CodePoint
-                    {
-                        LineNo = Constants.kInvalidIndex,
-                        CharNo = Constants.kInvalidIndex
-                    }
-                };
-
-                // Save the pc and store it after the call
-                EmitFunctionCallNode(inlineCall, ref inferedType, false, graphNode, ProtoCore.CompilerDefinitions.SubCompilePass.UnboundIdentifier);
-                EmitFunctionCallNode(inlineCall, ref inferedType, false, graphNode, ProtoCore.CompilerDefinitions.SubCompilePass.None, parentNode);
+                    LineNo = Constants.kInvalidIndex,
+                    CharNo = Constants.kInvalidIndex
+                },
 
-                // Need to restore those settings.
-                if (graphNode != null)
+                EndExclusive = new ProtoCore.CodeModel.CodePoint
                 {
-                    graphNode.isInlineConditional = isInlineConditionalFlag;
-                    graphNode.updateBlock.startpc = startPC;
-                    graphNode.isReturn = isReturn;
+                    LineNo = Constants.kInvalidIndex,
+                    CharNo = Constants.kInvalidIndex
                 }
+            };
+
+            // Save the pc and store it after the call
+            EmitFunctionCallNode(inlineCall, ref inferedType, false, graphNode,
+                ProtoCore.CompilerDefinitions.SubCompilePass.UnboundIdentifier);
+            EmitFunctionCallNode(inlineCall, ref inferedType, false, graphNode,
+                ProtoCore.CompilerDefinitions.SubCompilePass.None, parentNode);
+
+            // Need to restore those settings.
+            if (graphNode != null)
+            {
+                graphNode.isInlineConditional = isInlineConditionalFlag;
+                graphNode.updateBlock.startpc = startPC;
+                graphNode.isReturn = isReturn;
             }
         }
 
diff --git a/src/Engine/ProtoCore/Parser/AssociativeAST.cs b/src/Engine/ProtoCore/Parser/AssociativeAST.cs
@@ -1992,16 +1992,16 @@ public class InlineConditionalNode : AssociativeNode
         public AssociativeNode ConditionExpression { get; set; }
         public AssociativeNode TrueExpression { get; set; }
         public AssociativeNode FalseExpression { get; set; }
+
+        [Obsolete("IsAutogenerated property is deprecated and is not used. Please remove in 3.0")]
         public bool IsAutoGenerated { get; set; }
 
         public InlineConditionalNode()
         {
-            IsAutoGenerated = false;
         }
 
         public InlineConditionalNode(InlineConditionalNode rhs) : base(rhs)
         {
-            IsAutoGenerated = false;
             ConditionExpression = NodeUtils.Clone(rhs.ConditionExpression);
             TrueExpression = NodeUtils.Clone(rhs.TrueExpression);
             FalseExpression = NodeUtils.Clone(rhs.FalseExpression);
diff --git a/test/DynamoCoreTests/CodeBlockNodeTests.cs b/test/DynamoCoreTests/CodeBlockNodeTests.cs
@@ -212,6 +212,87 @@ public void TestStatement_ArrayReference()
 #endif
         protected double tolerance = 1e-4;
 
+        // Note: This test fails, even though the individual items pass as separate tests
+        //       This is preventing putting all of the tests in a single method
+        //[Test]
+        //[Category("UnitTests")]
+        //public void TestFunctionDefaultParameters()
+        //{
+        //    var codeBlockNode = CreateCodeBlockNode();
+        //    var guid = codeBlockNode.GUID.ToString();
+
+        //    UpdateCodeBlockNodeContent(codeBlockNode, "def test(x:int = 1){return = x;}test();");
+        //    AssertPreviewValue(guid, 1);
+
+        //    UpdateCodeBlockNodeContent(codeBlockNode, "def test(x:int = 1, y:int= 2){return = x + y;}test();");
+        //    AssertPreviewValue(guid, 3);
+        //}
+
+        [Test]
+        [Category("UnitTests")]
+        public void TestFunctionSingleDefaultParameter()
+        {
+            var codeBlockNode = CreateCodeBlockNode();
+            UpdateCodeBlockNodeContent(codeBlockNode, "def test(x:int = 1){return = x;}test();");
+            AssertPreviewValue(codeBlockNode.GUID.ToString(), 1);
+        }
+
+        [Test]
+        [Category("UnitTests")]
+        public void TestFunctionTwoDefaultParameters()
+        {
+            var codeBlockNode = CreateCodeBlockNode();
+            UpdateCodeBlockNodeContent(codeBlockNode, "def test(x:int = 1, y:int= 2){return = x + y;}test();");
+            AssertPreviewValue(codeBlockNode.GUID.ToString(), 3);
+        }
+
+        [Test]
+        [Category("UnitTests")]
+        public void TestFunctionThreeDefaultParameters()
+        {
+            var codeBlockNode = CreateCodeBlockNode();
+            UpdateCodeBlockNodeContent(codeBlockNode, "def test(x:int = 1, y:int= 2, z:int= 3){return = x + y + z;}test();");
+            AssertPreviewValue(codeBlockNode.GUID.ToString(), 6);
+        }
+
+        [Test]
+        [Category("UnitTests")]
+        public void TestFunctionOneArgumentOneDefaultParameter()
+        {
+            var codeBlockNode = CreateCodeBlockNode();
+            UpdateCodeBlockNodeContent(codeBlockNode, "def test(x:int, y:int= 2){return = x + y;}test(1);");
+            AssertPreviewValue(codeBlockNode.GUID.ToString(), 3);
+        }
+
+        [Test]
+        [Category("UnitTests")]
+        public void TestFunctionMultipleBlocksDefaultParameters()
+        {
+            var codeBlockNode1 = CreateCodeBlockNode();
+            UpdateCodeBlockNodeContent(codeBlockNode1, "def test1(x:int = 1, y:int= 2){return = x + y;}test1();");
+            var codeBlockNode2 = CreateCodeBlockNode();
+            UpdateCodeBlockNodeContent(codeBlockNode2, "def test2(x, y = 2, z = 3){return = x + y + z;}test2(1);");
+
+            AssertPreviewValue(codeBlockNode1.GUID.ToString(), 3);
+            AssertPreviewValue(codeBlockNode2.GUID.ToString(), 6);
+        }
+
+        // Note: DYN-1684 - This test is expected to fail due to the functions being indeterminate
+        //       We will need to figure out a way to test this once error handling is implemented
+        //[Test]
+        //[Category("UnitTests")]
+        //public void TestIndeterminateFunctionDefaultParameters()
+        //{
+        //    // Note that both code blocks contain functions called "test" that are indeterminate
+        //    var codeBlockNode1 = CreateCodeBlockNode();
+        //    UpdateCodeBlockNodeContent(codeBlockNode1, "def test(x:int = 1, y:int= 2){return = x + y;}test();");
+        //    var codeBlockNode2 = CreateCodeBlockNode();
+        //    UpdateCodeBlockNodeContent(codeBlockNode2, "def test(x, y = 2, z = 3){return = x + y + z;}test(1);");
+
+        //    AssertPreviewValue(codeBlockNode1.GUID.ToString(), 3);
+        //    AssertPreviewValue(codeBlockNode2.GUID.ToString(), 6);
+        //}
+
         [Test]
         [Category("UnitTests")]
         public void TestVarRedefinitionInFunctionDef()
@@ -224,7 +305,7 @@ public void TestVarRedefinitionInFunctionDef()
             var guid = "bbf7919d-d578-4b54-90b1-7df8f01483c6";
             var cbn = CurrentDynamoModel.CurrentWorkspace.NodeFromWorkspace<CodeBlockNodeModel>(
                 Guid.Parse(guid));
-            
+
 
             Assert.IsNotNull(cbn);
             Assert.AreEqual(ElementState.PersistentWarning, cbn.State);
diff --git a/test/DynamoCoreWpfTests/CodeBlockNodeTests.cs b/test/DynamoCoreWpfTests/CodeBlockNodeTests.cs
@@ -182,5 +182,15 @@ public void TestSyntaxHighlightRuleForDigits()
             string[] expected = new string[] { "-2468.2342E+04", "34534.345345", "23423", "-98.7", "0", "10", "2", "-555" };
             Assert.IsTrue(expected.SequenceEqual(actual));
         }
+
+        [Test, RequiresSTA]
+        public void TestFunctionMultipleBlocksDefaultParametersDeleteFirst()
+        {
+            RunCommandsFromFile("DeleteCrashCommands.xml", (commandTag) =>
+            {
+                // NOTE: Nothing needs to be tested here other than that this test does not crash
+            });
+        }
+
     }
 }
diff --git a/test/core/recorded/DeleteCrashCommands.xml b/test/core/recorded/DeleteCrashCommands.xml

Original file line number	Diff line number	Diff line change
`@@ -1992,16 +1992,16 @@ public class InlineConditionalNode : AssociativeNode`
`1992`	`1992`	`public AssociativeNode ConditionExpression { get; set; }`
`1993`	`1993`	`public AssociativeNode TrueExpression { get; set; }`
`1994`	`1994`	`public AssociativeNode FalseExpression { get; set; }`
	`1995`	`+`
	`1996`	`+ [Obsolete("IsAutogenerated property is deprecated and is not used. Please remove in 3.0")]`
`1995`	`1997`	`public bool IsAutoGenerated { get; set; }`
`1996`	`1998`
`1997`	`1999`	`public InlineConditionalNode()`
`1998`	`2000`	`{`
`1999`		`- IsAutoGenerated = false;`
`2000`	`2001`	`}`
`2001`	`2002`
`2002`	`2003`	`public InlineConditionalNode(InlineConditionalNode rhs) : base(rhs)`
`2003`	`2004`	`{`
`2004`		`- IsAutoGenerated = false;`
`2005`	`2005`	`ConditionExpression = NodeUtils.Clone(rhs.ConditionExpression);`
`2006`	`2006`	`TrueExpression = NodeUtils.Clone(rhs.TrueExpression);`
`2007`	`2007`	`FalseExpression = NodeUtils.Clone(rhs.FalseExpression);`
Original file line number	Diff line number	Diff line change
`@@ -182,5 +182,15 @@ public void TestSyntaxHighlightRuleForDigits()`
`182`	`182`	`string[] expected = new string[] { "-2468.2342E+04", "34534.345345", "23423", "-98.7", "0", "10", "2", "-555" };`
`183`	`183`	`Assert.IsTrue(expected.SequenceEqual(actual));`
`184`	`184`	`}`
	`185`	`+`
	`186`	`+ [Test, RequiresSTA]`
	`187`	`+ public void TestFunctionMultipleBlocksDefaultParametersDeleteFirst()`
	`188`	`+ {`
	`189`	`+ RunCommandsFromFile("DeleteCrashCommands.xml", (commandTag) =>`
	`190`	`+ {`
	`191`	`+ // NOTE: Nothing needs to be tested here other than that this test does not crash`
	`192`	`+ });`
	`193`	`+ }`
	`194`	`+`
`185`	`195`	`}`
`186`	`196`	`}`