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

Fix crash with incorrect syntax in CBN in custom node #9258

Merged
merged 3 commits into from
Nov 26, 2018

Conversation

aparajit-pratap
Copy link
Contributor

@aparajit-pratap aparajit-pratap commented Nov 21, 2018

Purpose

JIRA: https://jira.autodesk.com/browse/QNTM-4425
GH Issue: #8911

The crash occurs at the time of compilation of the custom node definition. It tries to build a return AST statement with the output from the code block node but since the code block output is null owing to the syntax error, an ArgumentNullException is thrown from the AstFactory.BuildReturnStatement, which apparently isn't caught anywhere and results in a crash. The fix is to build a NullNode if the rhs returned from the code block AST is null and then pass this NullNode as input to the BuildReturnStatement method to generate AST for a return = null; statement.

Declarations

Check these if you believe they are true

  • The code base 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.

Reviewers

@mjkkirschner @QilongTang @ColinDayOrg

Adding a test case would be tricky here - it would be best to add a recordable test but I'm not sure if we can add these for a custom node workspace. Will check.

@mjkkirschner
Copy link
Member

@aparajit-pratap thanks for the explanation LGTM -
custom node recordable tests should be possible, there are commands for creating custom node workspaces and switching to them.

@QilongTang
Copy link
Contributor

LGTM

@aparajit-pratap aparajit-pratap merged commit 570486d into DynamoDS:master Nov 26, 2018
@aparajit-pratap aparajit-pratap deleted the customCrash branch November 26, 2018 17:28
@horatiubota horatiubota added the error/warning/crash Issues mentioning a Dynamo error, warning or crash message label Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error/warning/crash Issues mentioning a Dynamo error, warning or crash message
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants