Binary Expressions deleted by live runner make their first input symbols null. #10512

mjkkirschner · 2020-03-27T21:48:52Z

Purpose

When a binary expression is deleted by the live runner - the binary AST is walked and all binary expressions within that subtree are assigned to null...

the change was from:
#7282

The behavior before this PR was that top level binary expressions were assigned to null and no AST walking was done.

I'm guessing @ke-yu was trying to support multiple assignments like:
a = 3;
b = a;
all being set to null.

but the consequence was other binary expressions like
a && b end up resulting in this a = null

I'm opening this PR for feedback because I'm not clear on what binary expressions should actually be set to null, only assignments? Right now I am only stopping OR and AND - since that effects this particular bug, but I don't think thats the whole story.
all good.

still good!

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.

aparajit-pratap

@mjkkirschner I didn't realize this was submitted only in 2.0 but you're right, this is the breaking change. Good find!

The reason why only the UI nodes (for logic operators) cause the issue and not the && or || node (neither the operators used in a CBN), is because in the latter cases, the expression is actually a function call (in order to support replication) and in the case of the UI nodes, we are specifically building a binary expression AST:

Dynamo/src/Libraries/CoreNodeModels/Logic/AndOr.cs

Line 51 in a6bbbc0

AstFactory.BuildBinaryExpression(node, current, _op)))

For the same reason, replication doesn't work for And and Or UI nodes as we are not building function call ASTs. I can therefore think of a couple of ways to solve this:

Change the BuildOutputAST method to generate function calls for the logic UI nodes instead of binary exp. ASTs. Doing this will fix this issue and also enable replication to work for these nodes thereby making them consistent with the other ways of using these operators.
Revert @ke-yu's change to walk the AST in BuildNullAssignments - in other words, just assign the top-level expression to null OR
Leave BuildNullAssignments as it is, just add a check here to specifically check if the binary exp. AST is an assignment - meaning assign it to null only if it is an assignment expression, not otherwise.

ignore all expressions which are not assigments in the nullifyBinaryExpression code

mjkkirschner · 2020-04-01T22:35:31Z

@aparajit-pratap please take another look, went with your third suggestion and added 2 tests. I still need to run the self serve to see if it breaks anything.

mjkkirschner · 2020-04-02T15:16:33Z

@aparajit-pratap ~~unfortunately, a bunch of failures in self serve, looking into it.~~ - Think I got it now, was missing latest fixes on master.

aparajit-pratap · 2020-04-02T16:41:48Z

Curious, what did you think about making the UI logic nodes support replication?

aparajit-pratap · 2020-04-02T16:45:42Z

src/Engine/ProtoScript/Runners/LiveRunner.cs

@@ -982,7 +983,7 @@ private List<BinaryExpressionNode> BuildNullAssignments(List<AssociativeNode> as
                }

                IdentifierNode leftNode = bNode.LeftNode as IdentifierNode;
-                if (leftNode == null || leftNode.ArrayDimensions != null)
+                if (leftNode == null || leftNode.ArrayDimensions != null  || bNode.Optr != Operator.assign)


Minor, but why not have this check on line 980?

you're right, that seems like a more appropriate place, where the binary node is validated, I can move it.

mjkkirschner · 2020-04-02T16:54:12Z

Curious, what did you think about making the UI logic nodes support replication?

I thought that could get very difficult because the ZT versions only support two inputs and the UI ones support more than 2 - so I believe we'd end up doing binary expressions anyway - we could do both though? This fix and also the change to UI nodes?

aparajit-pratap · 2020-04-02T17:00:59Z

test/Engine/ProtoTest/LiveRunnerTests/MicroFeatureTests.cs

@@ -852,6 +852,93 @@ public void GraphILTest_DeletedNode01()

        }

+        [Test]
+        public void GraphILTest_DeletedBinaryExpresionDoesNotEffectReferences()


Effectively this test is simulating the same thing that would happen in the Dynamo UI when running the graph, deleting the node, and rerunning it or is it testing something additionally?

simulating the same thing as in the UI - I verified it failed before this change.

aparajit-pratap · 2020-04-02T17:04:06Z

I thought that could get very difficult because the ZT versions only support two inputs and the UI ones support more than 2 - so I believe we'd end up doing binary expressions anyway - we could do both though? This fix and also the change to UI nodes?

Probably that's something to think about for later, add to the backlog maybe? For now, I think this fix looks good.

mjkkirschner · 2020-04-02T17:44:43Z

@aparajit-pratap the self serve and parallel tests all pass, gonna merge this in.

check if this works - it does

db36f0e

mjkkirschner requested a review from aparajit-pratap March 27, 2020 21:49

mjkkirschner added the WIP label Mar 27, 2020

mjkkirschner changed the title ~~Binary Expressions deleted by live runner make their first input symbols null.~~ FEEDBACK Binary Expressions deleted by live runner make their first input symbols null. Mar 27, 2020

aparajit-pratap reviewed Mar 29, 2020

View reviewed changes

mjkkirschner added 2 commits April 1, 2020 17:57

add micro test

c7d133f

ignore all expressions which are not assigments in the nullifyBinaryExpression code

add new dynamo model test

3609a61

mjkkirschner changed the title ~~FEEDBACK Binary Expressions deleted by live runner make their first input symbols null.~~ Binary Expressions deleted by live runner make their first input symbols null. Apr 1, 2020

mjkkirschner added PTAL Please Take A Look 👀 and removed WIP labels Apr 1, 2020

Merge remote-tracking branch 'upstream/master' into UILogicNodes

6608a8a

aparajit-pratap reviewed Apr 2, 2020

View reviewed changes

aparajit-pratap added the LGTM Looks good to me label Apr 2, 2020

review comment

57884d1

mjkkirschner removed the PTAL Please Take A Look 👀 label Apr 2, 2020

mjkkirschner merged commit 6ce8ce7 into DynamoDS:master Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Binary Expressions deleted by live runner make their first input symbols null. #10512

Binary Expressions deleted by live runner make their first input symbols null. #10512

mjkkirschner commented Mar 27, 2020 •

edited

Loading

aparajit-pratap left a comment •

edited

Loading

mjkkirschner commented Apr 1, 2020

mjkkirschner commented Apr 2, 2020 •

edited

Loading

aparajit-pratap commented Apr 2, 2020

aparajit-pratap Apr 2, 2020 •

edited

Loading

mjkkirschner Apr 2, 2020

mjkkirschner commented Apr 2, 2020 •

edited

Loading

aparajit-pratap Apr 2, 2020

mjkkirschner Apr 2, 2020

aparajit-pratap commented Apr 2, 2020

mjkkirschner commented Apr 2, 2020

Binary Expressions deleted by live runner make their first input symbols null. #10512

Binary Expressions deleted by live runner make their first input symbols null. #10512

Conversation

mjkkirschner commented Mar 27, 2020 • edited Loading

Purpose

Declarations

aparajit-pratap left a comment • edited Loading

Choose a reason for hiding this comment

mjkkirschner commented Apr 1, 2020

mjkkirschner commented Apr 2, 2020 • edited Loading

aparajit-pratap commented Apr 2, 2020

aparajit-pratap Apr 2, 2020 • edited Loading

Choose a reason for hiding this comment

mjkkirschner Apr 2, 2020

Choose a reason for hiding this comment

mjkkirschner commented Apr 2, 2020 • edited Loading

aparajit-pratap Apr 2, 2020

Choose a reason for hiding this comment

mjkkirschner Apr 2, 2020

Choose a reason for hiding this comment

aparajit-pratap commented Apr 2, 2020

mjkkirschner commented Apr 2, 2020

mjkkirschner commented Mar 27, 2020 •

edited

Loading

aparajit-pratap left a comment •

edited

Loading

mjkkirschner commented Apr 2, 2020 •

edited

Loading

aparajit-pratap Apr 2, 2020 •

edited

Loading

mjkkirschner commented Apr 2, 2020 •

edited

Loading