diff --git a/src/DynamoCore/Engine/CodeGeneration/AstBuilder.cs b/src/DynamoCore/Engine/CodeGeneration/AstBuilder.cs index b0cd75103e5..6c12a3d7f67 100644 --- a/src/DynamoCore/Engine/CodeGeneration/AstBuilder.cs +++ b/src/DynamoCore/Engine/CodeGeneration/AstBuilder.cs @@ -238,7 +238,7 @@ private void CompileToAstNodes( #endif var scopedNode = node as ScopedNodeModel; - IEnumerable astNodes = + IEnumerable astNodes = scopedNode != null ? scopedNode.BuildAstInScope(inputAstNodes, verboseLogging, this) : node.BuildAst(inputAstNodes, context); @@ -335,7 +335,24 @@ public IEnumerable>> CompileToAstN if (context == CompilationContext.DeltaExecution) { - sortedNodes = sortedNodes.Where(n => n.IsModified); + // When any non-input node in sortedNodes is marked IsModified, the DesignScript + // compiler will assign it a new, higher PC. Other non-input nodes in sortedNodes + // (which are already limited to downstream nodes of the modified set by + // ComputeModifiedNodes) keep their old, lower PCs. Because ApplyUpdate iterates + // graph nodes in ascending PC order, those nodes execute before the modified + // upstream node, consuming stale values. The upstream then runs, changes its + // output, and marks them dirty again — causing every affected node to execute + // twice. To prevent this, when any non-input node is IsModified, include all + // non-input nodes from sortedNodes in the compilation pass. The LiveRunner + // force-recompile path injects their ASTs into the delta with new, higher PCs, + // restoring topological execution order. + // Note: IsModified covers any pending re-evaluation, not only structural changes + // (e.g. reconnects). Non-structural triggers cause harmless extra compilation; + // the LiveRunner skips the force-recompile for subtrees whose AST is unchanged. + bool hasModifiedNonInputNode = sortedNodes.Any(n => n.IsModified && !n.IsInputNode); + sortedNodes = hasModifiedNonInputNode + ? sortedNodes.Where(n => n.IsModified || !n.IsInputNode) + : sortedNodes.Where(n => n.IsModified); } var result = new List>(); diff --git a/src/Engine/ProtoCore/RuntimeData.cs b/src/Engine/ProtoCore/RuntimeData.cs index df67c8a0555..52b25665024 100644 --- a/src/Engine/ProtoCore/RuntimeData.cs +++ b/src/Engine/ProtoCore/RuntimeData.cs @@ -138,7 +138,18 @@ public CallSite GetCallSite(int classScope, string methodName, Executable execut csInstance.UpdateCallSite(classScope, methodName); if (runtimeCore.Options.IsDeltaExecution) { - runtimeCore.RuntimeStatus.ClearWarningForExpression(graphNode.exprUID); + // Clear warnings for the specific Dynamo node being executed (by GUID + exprUID). + // Using exprUID alone is unsafe because unrelated nodes can share the same exprUID + // in delta compilation (e.g. force-recompiled downstream nodes), causing one + // node's ClearWarningForExpression call to accidentally wipe another node's + // runtime warnings. Using GUID alone is unsafe when a single Dynamo node compiles + // to multiple AST expressions with distinct exprUIDs — clearing all warnings for + // the GUID would remove a sibling expression's warning before it re-executes. + // Fall back to exprUID for internal graph nodes (guid=Empty). + if (!graphNode.guid.Equals(System.Guid.Empty)) + runtimeCore.RuntimeStatus.ClearWarningsForGraphNode(graphNode.guid, graphNode.exprUID); + else + runtimeCore.RuntimeStatus.ClearWarningForExpression(graphNode.exprUID); } } diff --git a/src/Engine/ProtoCore/RuntimeStatus.cs b/src/Engine/ProtoCore/RuntimeStatus.cs index 5cbc5792d75..f36fc179f5b 100644 --- a/src/Engine/ProtoCore/RuntimeStatus.cs +++ b/src/Engine/ProtoCore/RuntimeStatus.cs @@ -150,6 +150,24 @@ public void ClearWarningsForGraph(Guid guid) } } + /// + /// Clears runtime warnings and infos that belong to a specific graph node GUID + /// AND a specific expression ID. This is more precise than + /// and is needed when a single Dynamo node compiles to multiple AST expressions that each + /// get a distinct exprUID — clearing by GUID alone would remove warnings from sibling + /// expressions that have not yet been re-executed. + /// + /// The graph-node GUID of the Dynamo node being re-executed. + /// The expression UID of the specific AST expression being re-executed. + public void ClearWarningsForGraphNode(Guid guid, int exprUID) + { + using (rwl.CreateWriteLock()) + { + warnings.RemoveAll(w => w.GraphNodeGuid.Equals(guid) && w.ExpressionID == exprUID); + infos.RemoveAll(w => w.GraphNodeGuid.Equals(guid) && w.ExpressionID == exprUID); + } + } + public void ClearWarningsForAst(int astID) { using (rwl.CreateWriteLock()) diff --git a/src/Engine/ProtoScript/Runners/LiveRunner.cs b/src/Engine/ProtoScript/Runners/LiveRunner.cs index 41b391401de..8bb9217a2e3 100644 --- a/src/Engine/ProtoScript/Runners/LiveRunner.cs +++ b/src/Engine/ProtoScript/Runners/LiveRunner.cs @@ -690,6 +690,63 @@ private void SetNestedLanguageBlockASTGuids(Guid guid, List } } + /// + /// Compares two AST nodes by textual content rather than object identity. + /// BuildAst always creates fresh objects, so reference equality fails even for + /// semantically unchanged nodes. ToString() produces a stable, identity-independent + /// representation that reflects the actual code the node will generate. + /// + /// ToString() alone is not always sufficient: FunctionCallNode.ToString() returns the + /// literal "null" for internal methods (those starting with '%') whose unprefixed name + /// doesn't parse as a binary or unary operator (e.g. "%conditionalIf" used by the If + /// node), hiding the actual arguments and their replication guides. When the stringified + /// form contains "null" — the narrow case where the textual form may be lossy — fall + /// back to the structural Equals operator (overridden on each AssociativeNode subclass + /// to do recursive content comparison) to confirm the match. ASTs that don't stringify + /// to a form containing "null" use only the fast textual comparison. + /// + private static bool AreAstNodesContentEqual(AssociativeNode a, AssociativeNode b) + { + if (ReferenceEquals(a, b)) return true; + if (a == null || b == null) return false; + var sa = a.ToString(); + var sb = b.ToString(); + if (sa != sb) return false; + if (sa.Contains(ProtoCore.DSDefinitions.Keyword.Null)) + { + return a.Equals(b); + } + return true; + } + + /// + /// Returns true if the subtree's AST nodes differ from the cached version. + /// Used to distinguish structural changes (reconnect) from identity changes (slider value). + /// Detects both additions (new nodes not in cache) and removals (cached nodes no longer present). + /// Uses content-based comparison so that freshly compiled but semantically unchanged nodes + /// are correctly detected as unmodified. + /// + private bool SubtreeHasActualChanges(Subtree subtree) + { + Subtree cached; + if (!currentSubTreeList.TryGetValue(subtree.GUID, out cached) || cached.AstNodes == null) + return subtree.AstNodes != null && subtree.AstNodes.Any(); + + var newNodes = subtree.AstNodes; + var cachedNodes = cached.AstNodes; + + // Null or empty new AST when cache had content means all nodes were removed. + if (newNodes == null || !newNodes.Any()) + return cachedNodes.Any(); + + // Different count means nodes were added or removed. + if (newNodes.Count() != cachedNodes.Count()) + return true; + + // Same count: any new node absent from the cache indicates a structural change. + return newNodes.Any(node => !cachedNodes.Any(prev => AreAstNodesContentEqual(prev, node))); + } + /// /// Update the cached ASTs in the subtree given the modified ASTs /// @@ -816,6 +873,16 @@ private IEnumerable GetDeltaAstListModified(List modif } } + // When any non-input subtree has structural changes (e.g. a reconnect), delta + // compilation recompiles that node at a new, higher PC. Unchanged downstream nodes + // keep their old, lower PCs. ApplyUpdate then finds them first (lower PC = earlier + // in the graph list) and runs them with stale upstream values, then the upstream + // node executes and marks them dirty again — causing each downstream node to execute + // twice. To prevent this, force-recompile all unchanged non-input subtrees so they + // receive new, higher PCs that respect the topological execution order. + bool anyNonInputActuallyModified = modifiedSubTrees.Any( + st => !st.IsInput && SubtreeHasActualChanges(st)); + for (int n = 0; n < modifiedSubTrees.Count(); ++n) { var modifiedSubTree = modifiedSubTrees[n]; @@ -832,6 +899,35 @@ private IEnumerable GetDeltaAstListModified(List modif var modifiedASTList = GetModifiedNodes(modifiedSubTree, redefinitionAllowed, out modifiedInputAST); csData.ModifiedNodesForRuntimeSetValue.AddRange(modifiedInputAST); + // If another non-input subtree was structurally modified and this non-input + // subtree appears unchanged, force-recompile it so its new graph node lands + // at a PC higher than the recompiled upstream node. Also deactivate the old + // graph nodes so they do not co-execute with the new ones in ApplyUpdate. + if (!modifiedSubTree.IsInput && anyNonInputActuallyModified + && !modifiedASTList.Any() && !modifiedInputAST.Any() + && !SubtreeHasActualChanges(modifiedSubTree)) + { + Subtree cachedSubTree; + if (currentSubTreeList.TryGetValue(modifiedSubTree.GUID, out cachedSubTree) + && cachedSubTree.AstNodes != null) + { + var cachedBinaryNodes = cachedSubTree.AstNodes.OfType().ToList(); + // Deactivate the old graph nodes so they are replaced by freshly compiled ones. + csData.RemovedBinaryNodesFromModification.AddRange(cachedBinaryNodes); + // Stamp the GUID and inject directly into the delta list so the compiler + // assigns new, higher PCs — keeping modifiedASTList empty so + // UpdateCachedASTList does not see these as modifications and double + // the cached AST list. + foreach (var bnode in cachedBinaryNodes) + { + bnode.guid = modifiedSubTree.GUID; + bnode.IsInputExpression = false; + SetNestedLanguageBlockASTGuids(modifiedSubTree.GUID, new List() { bnode }); + } + deltaAstList.AddRange(cachedBinaryNodes); + } + } + modifiedSubTree.ModifiedAstNodes.Clear(); modifiedSubTree.ModifiedAstNodes.AddRange(modifiedASTList); modifiedSubTree.ModifiedAstNodes.AddRange(modifiedInputAST); @@ -860,7 +956,7 @@ private IEnumerable GetDeltaAstListModified(List modif } else { - // No delta computation. + // No delta computation. // Right now it is only disabled for code block node, but we may // completely disable it. Details about why doing so: // https://github.com/DynamoDS/Dynamo/pull/7282 @@ -997,7 +1093,7 @@ private List GetModifiedNodes(Subtree subtree, bool redefinitio bool nodeFound = false; foreach (AssociativeNode prevNode in st.AstNodes) { - if (prevNode.Equals(node)) + if (AreAstNodesContentEqual(prevNode, node)) { nodeFound = true; break; @@ -1098,7 +1194,7 @@ private List GetInactiveASTList(List prevASTLi bool prevNodeFoundInNewList = false; foreach (AssociativeNode newNode in newASTList) { - if (prevNode.Equals(newNode)) + if (AreAstNodesContentEqual(prevNode, newNode)) { // prev node still exists in the new list prevNodeFoundInNewList = true; @@ -1127,7 +1223,7 @@ private List GetUnmodifiedASTList(List prevAST { foreach (AssociativeNode newNode in newASTList) { - if (prevNode.Equals(newNode)) + if (AreAstNodesContentEqual(prevNode, newNode)) { existingList.Add(prevNode); break; diff --git a/test/Engine/ProtoTest/LiveRunnerTests/MicroFeatureTests.cs b/test/Engine/ProtoTest/LiveRunnerTests/MicroFeatureTests.cs index 1f859d07107..e71b58722c6 100644 --- a/test/Engine/ProtoTest/LiveRunnerTests/MicroFeatureTests.cs +++ b/test/Engine/ProtoTest/LiveRunnerTests/MicroFeatureTests.cs @@ -6095,7 +6095,75 @@ public void RegressMAGN7759() // There should be no warnings Assert.AreEqual(0, liveRunner.RuntimeCore.RuntimeStatus.WarningCount); } - + + /// + /// Regression test for DYN-5128. + /// When an upstream node is structurally modified (e.g. a wire reconnect), downstream + /// nodes that are included in the update with unchanged ASTs must receive new, higher PCs + /// so that the runtime executes them after the upstream node. Without the fix, unchanged + /// downstream nodes kept their original lower PCs and ran before the upstream, consuming + /// stale output and executing a second time after the upstream completed. + /// + [Test] + [Category("UnitTests")] + public void WhenUpstreamNodeReconnectedThenDownstreamNodesReceiveUpdatedValues() + { + // Arrange: three nodes in a chain A -> B -> C + var guidA = Guid.NewGuid(); + var guidB = Guid.NewGuid(); + var guidC = Guid.NewGuid(); + + var addedA = TestFrameWork.CreateSubTreeFromCode(guidA, "a = 1;"); + addedA.DeltaComputation = true; + var addedB = TestFrameWork.CreateSubTreeFromCode(guidB, "b = a + 10;"); + addedB.DeltaComputation = true; + var addedC = TestFrameWork.CreateSubTreeFromCode(guidC, "c = b + 100;"); + addedC.DeltaComputation = true; + + liveRunner.UpdateGraph(new GraphSyncData(null, new List { addedA, addedB, addedC }, null)); + + AssertValue("a", 1); + AssertValue("b", 11); + AssertValue("c", 111); + + // Act: simulate reconnect — A changes structurally; B and C are included with their + // unchanged ASTs in topological order, as AstBuilder now does after the DYN-5128 fix. + // Reuse the same AstNodes object references from the initial add so that + // BinaryExpressionNode.Equals returns true in GetModifiedNodes, leaving modifiedASTList + // empty for B and C. The LiveRunner force-recompile path then gives B and C new PCs + // higher than A's new PC so the runtime executes them in the correct topological order. + var modA = TestFrameWork.CreateSubTreeFromCode(guidA, "a = 2;"); + modA.DeltaComputation = true; + var modB = new Subtree(addedB.AstNodes, guidB) { DeltaComputation = true }; + var modC = new Subtree(addedC.AstNodes, guidC) { DeltaComputation = true }; + + liveRunner.UpdateGraph(new GraphSyncData(null, null, + new List { modA, modB, modC })); + + // Assert values: B and C must reflect A's new value, not the stale one. + AssertValue("a", 2); + AssertValue("b", 12); + AssertValue("c", 112); + + // Assert PC ordering: after the reconnect the active graph nodes for A, B, C must + // be ordered A.startpc < B.startpc < C.startpc. If the fix regresses, B and C keep + // their original low PCs (below A's new PC) and the runtime executes them before A, + // causing double execution. + var graphNodes = runtimeCore.DSExecutable.instrStreamList[0].dependencyGraph.GraphList; + + // OriginalAstID == 0 identifies auto-generated infrastructure nodes (null-assignments, + // dependency tracking stubs). All user-expression graph nodes have OriginalAstID > 0 + // because BinaryExpressionNode constructors always set OriginalAstID = ID and + // Node.ID = ++sID (starts at 1). Exclude them so PC ordering reflects only the + // actual expression evaluation nodes for A, B, and C. + int pcA = graphNodes.Where(n => n.guid == guidA && n.isActive && n.OriginalAstID != 0).Min(n => n.updateBlock.startpc); + int pcB = graphNodes.Where(n => n.guid == guidB && n.isActive && n.OriginalAstID != 0).Min(n => n.updateBlock.startpc); + int pcC = graphNodes.Where(n => n.guid == guidC && n.isActive && n.OriginalAstID != 0).Min(n => n.updateBlock.startpc); + + Assert.Less(pcA, pcB, "Node A must have a lower PC than node B"); + Assert.Less(pcB, pcC, "Node B must have a lower PC than node C"); + } + } }