Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DYN-982 - Fix issues with nested language blocks #12658

Merged
merged 8 commits into from
Mar 2, 2022

Conversation

aparajit-pratap
Copy link
Contributor

@aparajit-pratap aparajit-pratap commented Feb 25, 2022

Purpose

DYN-982 - Fix issues with nested language blocks
There were 2 issues with nested language blocks. The following script typed in a code block node is an example of this.

  1. a and x are global variables as they are in the outermost scope and are renamed to have globally unique names like a_<guide>, and x_guid, where guid is the same as that assigned to the node model. This is so that the user can use the same names in another code block node without running into name conflicts. The issue was that although the variables were renamed inside the Imperative block as well, the names were not propagating to the nested Associative block, due to which these symbols were undefined in that scope. The variable renaming is done using the IdentifierInPlaceMapper classes, which have been refactored in this PR to take care of the renaming issue.
  2. The second issue was the crash due to associative-update. Each graph node in the dependency graph is connected to other graph nodes that have a dependency on it. When the graph node is marked dirty, all of its connected graph nodes should also be marked dirty for associative-update to work properly and all dependent code to execute. However, only those graph nodes that belonged to the same language block were being marked dirty earlier due to which, if code inside a dependent node was in another language block, it would not re-execute. This is fixed by marking all connected graph nodes dirty even if they belonged to other language blocks.
a = 1;
x = 3;
i = [Imperative]
{
    c = 0;
    b = 0..4;
    while(a < 2)
    {
        [Associative]
        {
            c = x*b;
        }
        a = a+0.2;
    }
    return [c,a];
};
  • Moved ImperativeIdentifierInPlaceMapper from CodeBlockNodeModel to ProtoCore project
  • Added a similar AssociativeIdentifierInPlaceMapper to ProtoCore
  • Mark all dependent graph nodes dirty

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.
  • This PR modifies some build requirements and the readme is updated

Release Notes

Fix issues with nested language blocks

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

@aparajit-pratap aparajit-pratap changed the title [WIP] DYN-982 - Fix issues with nested language blocks DYN-982 - Fix issues with nested language blocks Mar 1, 2022
@aparajit-pratap aparajit-pratap added PTAL Please Take A Look 👀 and removed WIP labels Mar 1, 2022
var cbn = node.CodeBlockNode as CodeBlockNode;
if (cbn == null)
{
var assoc_cbn = node.CodeBlockNode as AST.AssociativeAST.CodeBlockNode;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this handles associative blocks inside of imperative blocks, what about the other way around?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good catch! Will make a similar change for the associative class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had made the change but had commented it out for some reason and then cleaned up the commented code 🤷‍♂️

@mjkkirschner
Copy link
Member

mjkkirschner commented Mar 1, 2022

@aparajit-pratap just had two questions and thanks for the tests!
hopefully we can get them running again soon on the ci machines.

@aparajit-pratap aparajit-pratap merged commit edbe280 into DynamoDS:master Mar 2, 2022
@aparajit-pratap aparajit-pratap deleted the dyn-982 branch March 2, 2022 03:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PTAL Please Take A Look 👀
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants