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-1541: Issues with code block function default arguments #9528

Merged
merged 38 commits into from Mar 26, 2019

Conversation

ColinDayOrg
Copy link
Contributor

@ColinDayOrg ColinDayOrg commented Feb 28, 2019

Purpose

This PR addresses issues that were found when deleting code blocks that contain function definitions that have default arguments.

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
@aparajit-pratap

FYIs

@mjkkirschner
Copy link
Member

@ColinDayOrg should this still be marked WIP / DNM? Is it ready for review?

@@ -141,7 +141,7 @@ public class CodeBlock
/// <param name="procTable"></param>
/// <param name="isBreakableBlock"></param>
/// <param name="core"></param>
public CodeBlock(Guid guid, CodeBlockType type, ProtoCore.Language langId, int cbID, SymbolTable symbols, ProcedureTable procTable, bool isBreakableBlock = false, ProtoCore.Core core = null)
public CodeBlock(Guid guid, CodeBlockType type, ProtoCore.Language langId, int cbID, SymbolTable symbols, ProcedureTable procTable, bool isBreakableBlock, ProtoCore.Core core)
Copy link
Member

Choose a reason for hiding this comment

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

this is a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, you are correct, I will revert it. However, these default arguments are used in the code without being checked for being null. It looks like the code has been this way for a while, not sure what you would like to happen here (other than no changes).

@ColinDayOrg ColinDayOrg changed the title [WIP/DNM] DYN-1541: Issues with code block function default arguments DYN-1541: Issues with code block function default arguments Mar 21, 2019
@@ -212,6 +212,127 @@ public void TestStatement_ArrayReference()
#endif
protected double tolerance = 1e-4;

// Note: This test fails, even though the individual items pass as separate tests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code is here to show an issue with the testing system, it can be removed if desired, not sure what the policy is for this.

Copy link
Member

Choose a reason for hiding this comment

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

what is the failure mode here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The failure mode is that the second test fails even though if the two tests are run in separate test runs they both pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you look at TestFunctionDefaultParameters versus the following TestFunctionSingleDefaultParameter and TestFunctionTwoDefaultParameters you can see that the tests are the same, yet if they are concatenated into one test the test will fail. I did not really look into why, I was just notating that there is potentially an issue with the testing system there that may trip up other people in the future.

Copy link
Member

Choose a reason for hiding this comment

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

hmm - the difference between running these in a single test and and in multiple tests is that Dynamo is being shut down and new workspaces are being opened in the new tests... that could be a problem with the testing system or it could be a real bug...

Copy link
Member

Choose a reason for hiding this comment

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

when you interact manually with Dynamo you cannot reproduce this failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To memorialize our recent Zoom conversation, each test passes separately, but putting them both in the same test case fails. Put another way, I can reproduce both issues manually, but in the testing framework having both tests run together fails. I would guess at this point that it is an issue with the testing framework and not Dynamo, but this is just a guess at this point. The main thought was just to get it notated so that other people may avoid some future pain, I'm not sure how much of importance this particular issue has for the everyday user.

AssertPreviewValue(codeBlockNode2.GUID.ToString(), 6);
}

// Note: DYN-1684 - This test is expected to fail due to the functions being indeterminate
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is here for notating the issue of code blocks with duplicate function names. It is not really part of this issue and can be removed if desired, but I left it in to memorialize the issue for the future. Note that once this duplicate name issue is addressed this test will not be an accurate description of the functionality and will need to be changed or removed.

@mjkkirschner
Copy link
Member

@ColinDayOrg I don't think @aparajit-pratap will be able to review further, would you mind pinging another reviewer to check this out - I will also review.

List<GraphNode> trueBodyNodes = core.InlineConditionalBodyGraphNodes.Pop();

// Append dependent nodes of the inline conditional
foreach (GraphNode gnode in trueBodyNodes)
Copy link
Member

Choose a reason for hiding this comment

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

find this style with no braces kind of difficult to read / prone to future errors - what do you think?

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 actually find the requirement to have the extra braces unnecessary and creates code that is more unreadable (mostly due to adding unnecessary code and code lines), but the bracing wars is a long running issue across multiple companies, I really have no dog in this race. That being said, these changes were made by Aparajit, I can change them however you would like.

{
RunCommandsFromFile("DeleteCrashCommands.xml", (commandTag) =>
{
// NOTE: Nothing needs to be tested here other than that this test does not crash
Copy link
Member

Choose a reason for hiding this comment

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

does this need to be wrapped in an assert does not throw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is probably true, I will add this

@mjkkirschner
Copy link
Member

@ColinDayOrg there are a lot of formatting changes here - can you sum up what the spirit of this code change is?

@ColinDayOrg ColinDayOrg requested review from alfarok and removed request for aparajit-pratap March 21, 2019 17:40
@QilongTang
Copy link
Contributor

Same for me, pretty hard to understand this and I am thinking about debugging myself to understand the intention better

@ColinDayOrg
Copy link
Contributor Author

After debugging and determining what the cause of the issue was; basically if there is a default argument in a function statement in a code block, when the code block is deleted there is a child code block (code block on the VM level, not a code block node) that gets deleted, which causes the indexing to get out of sync leading to an assertion failing which causes the VM crash. My original solution was to not delete code blocks that were created due to a default argument, but after suggesting this Aparajit said that it was probably better to not add the child code blocks in the first place. This is where most of these code changes came from, I would be happy to elaborate or have a further conversation if it would be helpful.

@alfarok
Copy link
Contributor

alfarok commented Mar 22, 2019

Hey @ColinDayOrg can you provide a sample of how to reproduce the crash that is now addressed? I am looking at the associated Jira task DYN-1541 and am still a bit confused. In the description the following sample is provided:

def foo(x, y = 1, z = 2)
{
    return = x + y + z;
};
foo(2); // returns null instead of 5

Mocking this locally (without these changes) I get a result of 4 not null or 5. This may not be the issue that is being addressed but I am trying to better understand what is addressed in this PR vs what the task outlines.

@alfarok
Copy link
Contributor

alfarok commented Mar 25, 2019

@ColinDayOrg Thanks for the detailed explanation of the issue on Friday. I was able to reproduce the VM crash locally and can verify it is no longer occurring with these changes. I beat around on it a bit with operations such as undo/redo with no further issues. As we discussed last week it would be good to get this merged into 2.3 if the team is okay, keeping an eye out for any strange behavior as it gets heavier usage since these changes run pretty deep.

@mjkkirschner
Copy link
Member

@ColinDayOrg after you file the followup for the test LGTM.

@mjkkirschner mjkkirschner added the LGTM Looks good to me label Mar 25, 2019
@ColinDayOrg
Copy link
Contributor Author

The test issue has been filed as DYN-1763

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM Looks good to me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants