From 36f234c47397157a31f1b6b7b826a8f7d19ca871 Mon Sep 17 00:00:00 2001 From: Martin Misol Monzo Date: Tue, 5 May 2020 08:53:45 -0400 Subject: [PATCH 1/2] Handle missing instance calling method statically A code block node calling an instance method in its static form would make Dynamo crash if the instance was not provided. An example of this would be calling 'Curve.Patch();'. The cause of the issue was that the default argument was ultimately tried to be interpreted as a pointer. By avoiding that wrong conversion the engine is now able to surface the real problem as a human-readable warning. --- .../ProtoCore/Lang/FFIFunctionEndPoint.cs | 10 ++----- test/DynamoCoreTests/CodeBlockNodeTests.cs | 30 +++++++++++++++++-- 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/src/Engine/ProtoCore/Lang/FFIFunctionEndPoint.cs b/src/Engine/ProtoCore/Lang/FFIFunctionEndPoint.cs index 8a415345788..5ec7ece44de 100644 --- a/src/Engine/ProtoCore/Lang/FFIFunctionEndPoint.cs +++ b/src/Engine/ProtoCore/Lang/FFIFunctionEndPoint.cs @@ -97,10 +97,10 @@ public override StackValue Execute(Runtime.Context c, List formalPar return StackValue.Null; } - // Check if this is a 'this' pointer function overload that was generated by the compiler - if (null != mFNode && mFNode.IsAutoGeneratedThisProc) + // Check if this is a 'this' pointer function overload that was generated by the compiler + const int thisPtrIndex = 0; + if (null != mFNode && mFNode.IsAutoGeneratedThisProc && formalParameters[thisPtrIndex].IsPointer) { - int thisPtrIndex = 0; bool isStaticCall = svThisPtr.IsPointer && Constants.kInvalidPointer == svThisPtr.Pointer; if (isStaticCall) { @@ -116,10 +116,6 @@ public override StackValue Execute(Runtime.Context c, List formalPar return StackValue.Null; } - // These are the op types allowed. - Validity.Assert(formalParameters[thisPtrIndex].IsPointer || - formalParameters[thisPtrIndex].IsDefaultArgument); - svThisPtr = formalParameters[thisPtrIndex]; formalParameters.RemoveAt(thisPtrIndex); diff --git a/test/DynamoCoreTests/CodeBlockNodeTests.cs b/test/DynamoCoreTests/CodeBlockNodeTests.cs index 905b08ad00f..013e2e218bb 100644 --- a/test/DynamoCoreTests/CodeBlockNodeTests.cs +++ b/test/DynamoCoreTests/CodeBlockNodeTests.cs @@ -11,6 +11,7 @@ using Dynamo.Graph.Workspaces; using Dynamo.Models; using Dynamo.Properties; +using Dynamo.Scheduler; using NUnit.Framework; using ProtoCore.AST.AssociativeAST; using ProtoCore.DSASM; @@ -274,6 +275,29 @@ public void TestFunctionMultipleBlocksDefaultParameters() AssertPreviewValue(codeBlockNode2.GUID.ToString(), 6); } + [Test] + [Category("UnitTests")] + public void CallingInstanceMethodStaticallyWithoutInstanceDoesNotCrash() + { + TaskStateChangedEventHandler evaluationDidNotFailHandler = + (DynamoScheduler sender, TaskStateChangedEventArgs args) => + { + Assert.AreNotEqual(TaskStateChangedEventArgs.State.ExecutionFailed, args.CurrentState); + }; + try + { + CurrentDynamoModel.Scheduler.TaskStateChanged += evaluationDidNotFailHandler; + var codeBlockNode = CreateCodeBlockNode(); + UpdateCodeBlockNodeContent(codeBlockNode, "Curve.Patch();"); + Assert.AreEqual(ElementState.Warning, codeBlockNode.State); + Assert.AreEqual("Failed to obtain this object for 'Curve.Patch'", codeBlockNode.ToolTipText); + } + finally + { + CurrentDynamoModel.Scheduler.TaskStateChanged -= evaluationDidNotFailHandler; + } + } + // 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] @@ -286,9 +310,9 @@ public void TestFunctionMultipleBlocksDefaultParameters() // 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); - //} + // AssertPreviewValue(codeBlockNode1.GUID.ToString(), 3); + // AssertPreviewValue(codeBlockNode2.GUID.ToString(), 6); + //} [Test] [Category("UnitTests")] From e0449ebbe82c7b013cdb0e12a78ed1ddaa16b37c Mon Sep 17 00:00:00 2001 From: Martin Misol Monzo Date: Tue, 5 May 2020 15:47:50 -0400 Subject: [PATCH 2/2] Limit change to only svThisPtr assignment --- .../ProtoCore/Lang/FFIFunctionEndPoint.cs | 18 +++++++++++++----- test/DynamoCoreTests/CodeBlockNodeTests.cs | 6 +++--- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/src/Engine/ProtoCore/Lang/FFIFunctionEndPoint.cs b/src/Engine/ProtoCore/Lang/FFIFunctionEndPoint.cs index 5ec7ece44de..e4b1c1286b2 100644 --- a/src/Engine/ProtoCore/Lang/FFIFunctionEndPoint.cs +++ b/src/Engine/ProtoCore/Lang/FFIFunctionEndPoint.cs @@ -97,10 +97,10 @@ public override StackValue Execute(Runtime.Context c, List formalPar return StackValue.Null; } - // Check if this is a 'this' pointer function overload that was generated by the compiler - const int thisPtrIndex = 0; - if (null != mFNode && mFNode.IsAutoGeneratedThisProc && formalParameters[thisPtrIndex].IsPointer) + // Check if this is a 'this' pointer function overload that was generated by the compiler + if (null != mFNode && mFNode.IsAutoGeneratedThisProc) { + int thisPtrIndex = 0; bool isStaticCall = svThisPtr.IsPointer && Constants.kInvalidPointer == svThisPtr.Pointer; if (isStaticCall) { @@ -116,11 +116,19 @@ public override StackValue Execute(Runtime.Context c, List formalPar return StackValue.Null; } - svThisPtr = formalParameters[thisPtrIndex]; + // These are the op types allowed. + Validity.Assert(formalParameters[thisPtrIndex].IsPointer || + formalParameters[thisPtrIndex].IsDefaultArgument); + + // Make sure we to pass a pointer to unmarshal. + if (formalParameters[thisPtrIndex].IsPointer) + { + svThisPtr = formalParameters[thisPtrIndex]; + } formalParameters.RemoveAt(thisPtrIndex); } - + formalParameters.Add(svThisPtr); Object ret = mFunctionPointer.Execute(c, mInterpreter, formalParameters); diff --git a/test/DynamoCoreTests/CodeBlockNodeTests.cs b/test/DynamoCoreTests/CodeBlockNodeTests.cs index 013e2e218bb..316affe73e5 100644 --- a/test/DynamoCoreTests/CodeBlockNodeTests.cs +++ b/test/DynamoCoreTests/CodeBlockNodeTests.cs @@ -310,9 +310,9 @@ public void CallingInstanceMethodStaticallyWithoutInstanceDoesNotCrash() // 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); - //} + // AssertPreviewValue(codeBlockNode1.GUID.ToString(), 3); + // AssertPreviewValue(codeBlockNode2.GUID.ToString(), 6); + //} [Test] [Category("UnitTests")]