continue custom node element binding issues pt1 (#9609)

* minor code cleanup

* fix to compare trace callsite identifiers for custom nodes

* add some comments
add test which fails before change

* add test with multiple custom nodes

* remove todo - confirmed this is node guid in cases I tested

src/Engine/ProtoCore/CodeGen.cs

@@ -163,10 +163,10 @@ protected void GenerateCallsiteIdentifierForGraphNode(AssociativeGraph.GraphNode
             // Build the unique ID for a callsite 
             string callsiteIdentifier =
                 procName + 
-                "_InClassDecl" + globalClassIndex + 
-                "_InFunctionScope" + globalProcIndex + 
-                "_Instance" + functionCallInstance.ToString() + 
-                "_" + graphNode.guid.ToString();
+                Constants.kSingleUnderscore + Constants.kInClassDecl + globalClassIndex + 
+                Constants.kSingleUnderscore + Constants.kInFunctionScope + globalProcIndex +
+                Constants.kSingleUnderscore + Constants.kInstance + functionCallInstance +
+                Constants.kSingleUnderscore + graphNode.guid;
 
             // TODO Jun: Address this in MAGN-3774
             // The current limitation is retrieving the cached trace data for multiple callsites in a single CBN

src/Engine/ProtoCore/DSASM/Defs.cs

@@ -395,6 +395,10 @@ public struct Constants
         public const string kDoubleUnderscores = "__";
         public const string kSingleUnderscore = "_";
         public const string kTempVarForTypedIdentifier = "%tTypedIdent";
+
+        internal const string kInClassDecl = "InClassDecl";
+        internal const string kInFunctionScope = "InFunctionScope";
+        internal const string kInstance = "Instance";
     }
 
     public enum MemoryRegion

src/Engine/ProtoCore/Lang/CallSite.cs

@@ -75,14 +75,12 @@ public bool HasAnyNestedData
 
                     if (HasData)
                         return true;
-                    else
-                    {
-                        //Not empty, and doesn't have data so test recursive
-                        Validity.Assert(NestedData != null,
-                            "Invalid recursion logic, this is a VM bug, please report to the Dynamo Team");
+
+                    //Not empty, and doesn't have data so test recursive
+                    Validity.Assert(NestedData != null,
+                        "Invalid recursion logic, this is a VM bug, please report to the Dynamo Team");
 
-                        return NestedData.Any(srtd => srtd.HasAnyNestedData);
-                    }
+                    return NestedData.Any(srtd => srtd.HasAnyNestedData);
                 }
             }
 
@@ -172,26 +170,21 @@ public ISerializable GetLeftMostData()
             {
                 if (HasData)
                     return Data;
-                else
-                {
-                    if (!HasNestedData)
-                        return null;
-                    else
-                    {
+
+                if (!HasNestedData)
+                    return null;
 #if DEBUG
 
-                        Validity.Assert(NestedData != null, "Nested data has changed null status since last check, suspected race");
-                        Validity.Assert(NestedData.Count > 0, "Empty subnested array, please file repo data with @lukechurch, relates to MAGN-4059");
+                Validity.Assert(NestedData != null, "Nested data has changed null status since last check, suspected race");
+                Validity.Assert(NestedData.Count > 0, "Empty subnested array, please file repo data with @lukechurch, relates to MAGN-4059");
 #endif
 
-                        //Safety trap to protect against an empty array, need repro test to figure out why this is getting set with empty arrays
-                        if (NestedData.Count == 0)
-                            return null;
+                //Safety trap to protect against an empty array, need repro test to figure out why this is getting set with empty arrays
+                if (NestedData.Count == 0)
+                    return null;
 
-                        SingleRunTraceData nestedTraceData = NestedData[0];
-                        return nestedTraceData.GetLeftMostData();
-                    }
-                }
+                SingleRunTraceData nestedTraceData = NestedData[0];
+                return nestedTraceData.GetLeftMostData();
             }
 
             public List<SingleRunTraceData> NestedData;
@@ -238,7 +231,7 @@ public List<ISerializable> RecursiveGetNestedData()
 
         /// <summary>
         /// TraceBinder is used to find assemblies to be used for
-        /// deserialization in cases where the exact assemlby that was
+        /// deserialization in cases where the exact assembly that was
         /// used in the serialization is not available. 
         /// </summary>
         internal class TraceBinder : SerializationBinder
@@ -286,7 +279,7 @@ public override System.Type BindToType(string assemblyName, string typeName)
         /// Normal usage patten is:
         /// 1. Instantiate
         /// 2. Push Trace data from callsite
-        /// 3. Call GetObjectData to serialise it onto a stream
+        /// 3. Call GetObjectData to serialize it onto a stream
         /// 4. Recreate using the special constructor
         /// </summary>
         [Serializable]
@@ -325,14 +318,13 @@ public TraceSerialiserHelper(SerializationInfo info, StreamingContext context)
 #if DEBUG
                         Debug.WriteLine("Deserialization of trace data failed.");
 #endif
-                        continue;
                     }
                 }
 
             }
 
             /// <summary>
-            /// Save the data into the standard serialisation pattern
+            /// Save the data into the standard serialization pattern
             /// </summary>
             public void GetObjectData(SerializationInfo info, StreamingContext context)
             {
@@ -615,7 +607,7 @@ public bool WillCallReplicate(Context context, List<StackValue> arguments,
 
         /// <summary>
         /// Call this method to obtain the Base64 encoded string that 
-        /// represent this instance of CallSite;s trace data
+        /// represents this callsite instance's trace data
         /// </summary>
         /// <returns>Returns the Base64 encoded string that represents the
         /// trace data of this callsite

src/Engine/ProtoCore/RuntimeData.cs

@@ -1,6 +1,7 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using System.Text.RegularExpressions;
 using ProtoCore.AssociativeGraph;
 using ProtoCore.DSASM;
 using ProtoCore.Utils;
@@ -25,11 +26,26 @@ public class RuntimeData
 
 
         private Dictionary<Guid, List<CallSite.RawTraceData>> uiNodeToSerializedDataMap = null;
+
+        private static readonly string identifierPattern = @"([a-zA-Z-_@0-9]+)";
+        private static readonly string indexPattern = @"([-+]?\d+)";
+        private static readonly string callsiteIDPattern =
+            identifierPattern +
+            Constants.kInClassDecl + indexPattern +
+            Constants.kSingleUnderscore + Constants.kInFunctionScope + indexPattern +
+            Constants.kSingleUnderscore + Constants.kInstance + indexPattern +
+            identifierPattern;
+        private static readonly string joinPattern = ';' + callsiteIDPattern;
+        private static readonly string fullCallsiteID = callsiteIDPattern + string.Format("({0})*", joinPattern);
+
+        /// <summary>
+        /// Map from callsite id to callsite.
+        /// </summary>
         public IDictionary<string, CallSite> CallsiteCache { get; set; }
         /// <summary>		
         /// Map from a callsite's guid to a graph UI node. 		
         /// </summary>
-        public Dictionary<Guid, Guid> CallSiteToNodeMap { get; private set; }
+        public Dictionary<Guid, Guid> CallSiteToNodeMap { get; }
 
  #endregion
 
@@ -289,7 +305,7 @@ public void SetTraceDataForNodes(
 
         /// <summary>
         /// Call this method to pop the top-most serialized callsite trace data.
-        /// Note that this call only pops off a signle callsite trace data 
+        /// Note that this call only pops off a single callsite trace data 
         /// belonging to a given UI node denoted by the given node guid.
         /// </summary>
         /// <param name="nodeGuid">The Guid of a given UI node whose top-most 
@@ -320,7 +336,7 @@ private string GetAndRemoveTraceDataForNode(Guid nodeGuid, string callsiteID)
             {
                 for (int i = 0; i < callsiteDataList.Count; i++)
                 {
-                    if (callsiteDataList[i].ID == callsiteID)
+                    if (DoCallSiteIDsMatch(callsiteID, callsiteDataList[i].ID))
                     {
                         callsiteTraceData = callsiteDataList[i].Data;
                         callsiteDataList.RemoveAt(i);
@@ -344,10 +360,45 @@ private string GetAndRemoveTraceDataForNode(Guid nodeGuid, string callsiteID)
             {
                 return GetAndRemoveTraceDataForNode(nodeGuid, string.Empty);
             }
-            else
+
+            return callsiteTraceData;
+        }
+
+        private static bool DoCallSiteIDsMatch(string compilerGenerated, string deserialized)
+        {
+            if (compilerGenerated == deserialized) return true;
+
+            var matches1 = Regex.Match(compilerGenerated, fullCallsiteID);
+            var matches2 = Regex.Match(deserialized, fullCallsiteID);
+
+            if (matches1.Groups.Count != matches2.Groups.Count) return false;
+
+            // If both group counts match, they should number 12 in all.
+            // We should ignore checking for the 1st, 7th, and 10th group specifically
+            // as per the Regex pattern (for fullCallsiteID) since that group includes the function scope
+            // that can vary for custom nodes or DS functions that make nested calls to
+            // host element creation methods.
+            //Groups
+            //0: full string
+            //1: function id
+            //2: global class index
+            //3: global function
+            //4: function call id
+            //5: outer node instance guid
+            //6: name,global class index, funcscope,instance,guid,
+            //7: name,
+            //8: global class index,
+            //9: function scope,
+            //10: instance,
+            //11: node instance guid
+            for (int i = 0; i < matches1.Groups.Count; i++)
             {
-                return callsiteTraceData;
+                if (i == 0 || i == 6 || i == 9) continue;
+
+                if (matches1.Groups[i].Value != matches2.Groups[i].Value) return false;
             }
+
+            return true;
         }
 
         #endregion // Trace Data Serialization Methods/Members

test/DynamoCoreTests/CallsiteTests.cs

@@ -12,6 +12,7 @@
 using Dynamo.Graph.Nodes;
 using static ProtoCore.CallSite;
 using System.Diagnostics;
+using Dynamo.Graph.Nodes.CustomNodes;
 
 namespace Dynamo.Tests
 {
@@ -165,6 +166,46 @@ public void Callsite_ElementBinding()
             AssertPreviewValue("5f277520-13aa-4833-aa82-b17a822e6d8c", 3);
         }
 
+        // This test dyn contains a manually edited callsite data with modified function scopes that do not exist
+        // this ensures function scope is not used during trace reconciliation for custom nodes.
+
+        [Test]
+        public void Callsite_ElementBinding_CustomNodes()
+        {
+            var ws = Open<HomeWorkspaceModel>(TestDirectory, callsiteDir, "element_binding_customNodes_modified.dyn");
+            CurrentDynamoModel.TraceReconciliationProcessor = new TestTraceReconciliationProcessor(0);
+            Assert.AreEqual(6, ws.Nodes.Count());
+            var dummyNodes = ws.Nodes.OfType<DummyNode>();
+            Assert.AreEqual(0, dummyNodes.Count());
+            Assert.IsFalse(ws.Nodes.OfType<Function>().First().IsInErrorState);
+
+            BeginRun();
+            AssertPreviewValue("da39dbe5f59649b18c2fb6ca54acba7b", 1234567899);
+            AssertPreviewValue("2366239164a9441a8c4dcd981d9cf542", 2);
+            AssertPreviewValue("342f96575f8942c890867d88495fb0db", 3);
+
+        }
+
+        // This test dyn contains a manually edited callsite data with modified function scopes that do not exist
+        // this ensures function scope is not used during trace reconciliation for custom nodes.
+
+        [Test]
+        public void Callsite_ElementBinding_CustomNodes_multiple()
+        {
+            var ws = Open<HomeWorkspaceModel>(TestDirectory, callsiteDir, "element_binding_customNodes_modified_multiple.dyn");
+            CurrentDynamoModel.TraceReconciliationProcessor = new TestTraceReconciliationProcessor(0);
+            Assert.AreEqual(9, ws.Nodes.Count());
+            var dummyNodes = ws.Nodes.OfType<DummyNode>();
+            Assert.AreEqual(0, dummyNodes.Count());
+            Assert.IsFalse(ws.Nodes.OfType<Function>().First().IsInErrorState);
+
+            BeginRun();
+            AssertPreviewValue("da39dbe5f59649b18c2fb6ca54acba7b", 111111);
+            AssertPreviewValue("2366239164a9441a8c4dcd981d9cf542", 222222);
+            AssertPreviewValue("8cfce012280342f3bd688520d68a7f66", 333333);
+            AssertPreviewValue("08448232ee094aad8280e9a99ed44f46", 444444);
+        }
+
         [Test]
         public void Callsite_ElementBinding_Timing()
         {