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

Crash when accessing static hidden methods from zero touch nodes #11238

Merged
merged 14 commits into from
Nov 12, 2020

Conversation

pinzart90
Copy link
Contributor

@pinzart90 pinzart90 commented Nov 9, 2020

Please Note:

  1. Before submitting the PR, please review How to Contribute to Dynamo
  2. Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
  3. PRs will be reviewed from oldest to newest
  4. If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
  5. PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
  6. PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
  7. PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

https://jira.autodesk.com/browse/DYN-1013
Handle hidden static methods from zero touch nodes

There are 3 Parts to this PR:

  1. Fix crash (after fix there would still be warning that function call is ambiguous - could not be matched).
  2. Handle overloaded/hidden static function endpoint resolution during graph execution
  3. Autocomplete functionality - for class types show only unique names (no duplicates); for class members do not show hidden members..

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

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

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

@pinzart90 pinzart90 added the WIP label Nov 9, 2020
@aparajit-pratap
Copy link
Contributor

LGTM other than addressing the one test regression.

@mjkkirschner
Copy link
Member

@pinzart90 looks good, is it possible to add test cases for this?

@aparajit-pratap
Copy link
Contributor

@pinzart90 looks good, is it possible to add test cases for this?

Yes, I forgot, we should add test cases if not already there.

members.AddRange(this.GetProperties().Where(m => m.IsStatic).GroupBy(x => x.Name).Select(y => y.First()));
return members;
// Return a list of members with unique names.
return members.GroupBy(x => x.Name).Select(x => x.First()).ToList();
Copy link
Contributor Author

@pinzart90 pinzart90 Nov 11, 2020

Choose a reason for hiding this comment

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

Seems like we were filtering out members with duplicate names within the scope of each class by use of .GroupBy(x => x.Name).Select(y => y.First())
I moved the filtering logic at the end so that we remove duplicates across all class hierarchies .

// 3. Function from the global scope - no this pointer and no class scope.
// In this case the svThisPtr.metaData.type will hold the global
// ex: SomeGlobalFunction();
//
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aparajit-pratap do you agree with these 3 scenarios ? Did I get them right ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The 3 types of methods look right but I'm not sure about this statement for global functions:

In this case the svThisPtr.metaData.type will hold the global 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm...from what I've seen in calls stacks...I would agree with you...I will remove that part

//Walk the class tree structure to find the method

while (runtimeCore.DSExecutable.classTable.ClassNodes[typeID].Base != Constants.kInvalidIndex)
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 am not sure this part of the matching algorithm makes sense for global functions...
TypeId here (svThisPtr.metaData.type) seems to always be initialized to 0 and ClassNodes[0] seems to always holdsthe Null primitive type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. I think typeID in the case of global functions will be 0 and its base id will be -1 (invalid index) in that case.

// Create a set of unique function and constructor descriptions.
// In this case we use ToString() to get the unique description of the members (func signature, constructor names).
var derivedClassMembers = new HashSet<string>();
members.ForEach(x => derivedClassMembers.Add(x.ToString()));
Copy link
Contributor

@aparajit-pratap aparajit-pratap Nov 11, 2020

Choose a reason for hiding this comment

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

Are you going to make the same simplification in this commit over 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.

GetOverloadsOnType must show duplicate names, so I cannot apply the simplification.
The existing code will filter duplicate signatures .

@aparajit-pratap
Copy link
Contributor

Let's wait for Dynamo Selfserve PR check. Other than that LGTM.

@pinzart90
Copy link
Contributor Author

Looks like one of the tests I added is failing https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/1736/testReport/Dynamo.Tests/
THis passes on my machine (with no diffs from this PR)
@aparajit-pratap 😕

@aparajit-pratap
Copy link
Contributor

Looks like one of the tests I added is failing https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/1736/testReport/Dynamo.Tests/
THis passes on my machine (with no diffs from this PR)
@aparajit-pratap 😕

Are you running the test individually on your machine? If so, that may be the difference. Try running the test as part of the whole test fixture CodeBlockNodeTests and see if you can reproduce the failure as on the test machine. I suspect FFITarget dll may already be loaded after running the test Resolve_NamespaceConflict_LoadLibrary and then your test is trying to load it again.

@pinzart90
Copy link
Contributor Author

Looks like one of the tests I added is failing https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/1736/testReport/Dynamo.Tests/
THis passes on my machine (with no diffs from this PR)
@aparajit-pratap 😕

Are you running the test individually on your machine? If so, that may be the difference. Try running the test as part of the whole test fixture CodeBlockNodeTests and see if you can reproduce the failure as on the test machine. I suspect FFITarget dll may already be loaded after running the test Resolve_NamespaceConflict_LoadLibrary and then your test is trying to load it again.

I think you are correct. How would you suggest I fix this? Load FFITarget.dll once before all tests start ? or check if it is loaded ..and skip ?

@aparajit-pratap
Copy link
Contributor

aparajit-pratap commented Nov 11, 2020

Yeah, I would suggest you load ffitarget for all tests in the beginning here:

libraries.Add("DSCoreNodes.dll");

but that would mean you'd need to edit this test Resolve_NamespaceConflict_LoadLibrary

@pinzart90 pinzart90 merged commit 86485ee into master Nov 12, 2020
@pinzart90 pinzart90 deleted the crash_hidden_methods branch November 12, 2020 18:17
mjkkirschner added a commit to SHKnudsen/Dynamo that referenced this pull request Nov 23, 2020
* Fix sorting for number keys in List.SortByKey (DynamoDS#11241)

* fix sorting for number keys in List.SortByKey

* add unit test

* Crash when accessing static hidden methods from zero touch nodes (DynamoDS#11238)

* handle hidden static methods from zero touch nodes

* add images (DynamoDS#11243)

* Revert "Adding copy of missing file from LibG nuget package (DynamoDS#11218)" (DynamoDS#11254)

This reverts commit a71b5c8.

* Revert "Revert "Adding copy of missing file from LibG nuget package (DynamoDS#11218)" (DynamoDS#11254)" (DynamoDS#11255)

This reverts commit 257ac14.

* Undock/redock extension tabs to windows (DynamoDS#11246)

* Somewhat working version

* Pretty much working version

* Finishing touches and test

* Cleanup

* Prevent both window and tab

* Improve method name and comment

* Forgot to include this in last commit

* add analytics to extensions load event (DynamoDS#11258)

* Revert Project Reference Changes (DynamoDS#11256)

* Initial Commit

* fix mono build

* Update to latest libG nuget version

* Cleanup entries VS Studio failed to update

* Fix Mono build

* Add the private flag back (modified by VS Studio)

* Regressions

* Add back missing resource

* Change DefaultValue setter to public (DynamoDS#11251)

* update load asm binaries from registry logic (DynamoDS#11261)

* update load asm binaries from registry

* Add coverage for docking (DynamoDS#11262)

* Adding a public API on the View extension that gets triggered when the view extension is closed. (DynamoDS#11260)

* Adding a public API on the View extension that gets triggered when the view extension is closed.

* Update variable name

* changing the name to ViewExtensionBaseClass

* Addressing some comments.

* Adding unit test.

* Changing the name to Closed()

* Some more comments.

* Adding an additional test

* Adding checks for the Menu items before setting its value.

* Update Style (DynamoDS#11263)

* CustomNodes that replicate do not access trace correctly. (DynamoDS#11244)

* add failing test

* this works, but seems like cheating.
more tests

* new tests, one failing, needs discussion

* fix test - still fails but as expected

* tests all pass with this change - but looking for better alternatives

* add test
use new field - cant find a better way currently
add comments

* revert internal
update test

* reset guid map before each test

* failing test

* reset

* whitespace

* Localize buttons (DynamoDS#11265)

* update version num and tt file (DynamoDS#11269)

* Fix merge error

* Fix merge error

* Revert "Fix merge error"

This reverts commit 0a21ca4.

Co-authored-by: aparajit-pratap <[email protected]>
Co-authored-by: pinzart90 <[email protected]>
Co-authored-by: Sylvester Knudsen <[email protected]>
Co-authored-by: Aaron (Qilong) <[email protected]>
Co-authored-by: Michael Kirschner <[email protected]>
Co-authored-by: Martin Misol Monzo <[email protected]>
Co-authored-by: Ashish Aggarwal <[email protected]>
Co-authored-by: reddyashish <[email protected]>
sm6srw added a commit that referenced this pull request Dec 2, 2020
* add packageDocManager to handle package documentation links

* new viewModelEventArgs to send node annotation data to doc browser

* Handle node annotation in doc browser

* support custom NodeModels + fix watcher event bug

* Update NodeAnnotationNotFound.md

* Update NodeAnnotationNotFound.md

* Comment updates

* Update AssemblySharedInfo.cs

* Update src/DynamoCoreWpf/ViewModels/Core/DynamoViewModelEventArgs.cs

* addtional comment updates + clean up

* clean up packageDocManager

* Update DocumentationBrowserViewModel.cs

* handle overloads + fix hot reload bug

* use HtmlSanitizer to clean potential dangerous content from markdown files

* move PackageDocumentationManager to DocumentationBrowser

* update syntax highlighting + markdownStyling updates

* comment updates

* add Markdig to License files

* Change GetPackageName to GetMainCategory

* update markdig + htmlsanitizer version

* fix failing tests + add new node documentaion tests

* fix broken test

* update link to wiki

* Update DocumentationBrowserViewExtension.cs

* Add MD2HTML Tool

* First pass - singleton version

* Revert "Add MD2HTML Tool"

This reverts commit 71509da.

* Revert "First pass - singleton version"

This reverts commit bbefc36.

* fix test

* add doc folder to PackageDirectoryBuilder (#10)

* add doc folder to PackageDirectoryBuilder

* update tests

* comment update

* Use a CLI tool for converting MD to HTML and for Sanitizing HTML as a way to not add any new 3rd party dependencies to Dynamo (#11)

* Revert "Revert "Add MD2HTML Tool""

This reverts commit 99b42f2.

* Revert "Revert "First pass - singleton version""

This reverts commit f655c39.

* Implement HTML Sanitizer

* Remove unneeded dependecies from the doc extension

* Cleanup naming

* More naming cleanups

* Rename files

* Rename folder

* Add comments

* Use literal strings

* Hardening wrapper class

* Simplify API

* Make wrapper API internal

* Add Tools project to Mono solution

* Fix directory name (case)

* Add missing internal

* Use IDispose pattern

* Fix literal string

* Fix cross platform issues

* Use CS.props file

* Undo change to AssemblySharedInfo.cs

* Use shared assembly info

* Add more docs

* Add ndesk support and implement help option

* Rename class

* Add missing change

* Remove the use of ref

* Fix typo

* Fix toolpath and use nameof()

* Improve error handling

* Move strings to resources

* Enhance error messages

* Merge master (#12)

* Fix sorting for number keys in List.SortByKey (#11241)

* fix sorting for number keys in List.SortByKey

* add unit test

* Crash when accessing static hidden methods from zero touch nodes (#11238)

* handle hidden static methods from zero touch nodes

* add images (#11243)

* Revert "Adding copy of missing file from LibG nuget package (#11218)" (#11254)

This reverts commit a71b5c8.

* Revert "Revert "Adding copy of missing file from LibG nuget package (#11218)" (#11254)" (#11255)

This reverts commit 257ac14.

* Undock/redock extension tabs to windows (#11246)

* Somewhat working version

* Pretty much working version

* Finishing touches and test

* Cleanup

* Prevent both window and tab

* Improve method name and comment

* Forgot to include this in last commit

* add analytics to extensions load event (#11258)

* Revert Project Reference Changes (#11256)

* Initial Commit

* fix mono build

* Update to latest libG nuget version

* Cleanup entries VS Studio failed to update

* Fix Mono build

* Add the private flag back (modified by VS Studio)

* Regressions

* Add back missing resource

* Change DefaultValue setter to public (#11251)

* update load asm binaries from registry logic (#11261)

* update load asm binaries from registry

* Add coverage for docking (#11262)

* Adding a public API on the View extension that gets triggered when the view extension is closed. (#11260)

* Adding a public API on the View extension that gets triggered when the view extension is closed.

* Update variable name

* changing the name to ViewExtensionBaseClass

* Addressing some comments.

* Adding unit test.

* Changing the name to Closed()

* Some more comments.

* Adding an additional test

* Adding checks for the Menu items before setting its value.

* Update Style (#11263)

* CustomNodes that replicate do not access trace correctly. (#11244)

* add failing test

* this works, but seems like cheating.
more tests

* new tests, one failing, needs discussion

* fix test - still fails but as expected

* tests all pass with this change - but looking for better alternatives

* add test
use new field - cant find a better way currently
add comments

* revert internal
update test

* reset guid map before each test

* failing test

* reset

* whitespace

* Localize buttons (#11265)

* update version num and tt file (#11269)

* Fix merge error

* Fix merge error

* Revert "Fix merge error"

This reverts commit 0a21ca4.

Co-authored-by: aparajit-pratap <[email protected]>
Co-authored-by: pinzart90 <[email protected]>
Co-authored-by: Sylvester Knudsen <[email protected]>
Co-authored-by: Aaron (Qilong) <[email protected]>
Co-authored-by: Michael Kirschner <[email protected]>
Co-authored-by: Martin Misol Monzo <[email protected]>
Co-authored-by: Ashish Aggarwal <[email protected]>
Co-authored-by: reddyashish <[email protected]>

* Extract more strings (#13)

* Fix failing tests (#14)

* Fix failing tests

* Revert "Fix failing tests"

This reverts commit 824f785.

* Fix failing tests

* Properly dispose all string writers

* More failing test fixes (#15)

* Fix for regression in multi-output node preview  (#11266)

* fix for multi-output node preview regression

* update tests

* add test

* revert unwanted changes

* update test

* Life cycle refactoring: Get rid of singleton. Let the model create and own the MarkdownHandler on first use.

* Fix crash in some document browser tests

* Properly dispose all DocumentBrowserViewExtension objects

* Revert "Fix crash in some document browser tests"

This reverts commit ab146f9.

* Display input port type specific default suggestions only (#11268)

* input port type specific default suggestions

* test fix

* Test fixes

* Use InTestMode flag

* Delete empty lines

* turn off hit testing visibility for DM related render packages (#11282)

* Apply syntax highlighting to multi-line strings (#11289)

* Also sanitize node documentation html

* Save/Load view extension size and location (#11278)

Information of where an extension control was last shown is saved and
retrieved as preference settings. These are actually not directly
visible for the user and are updating automatically, just like the main
Dynamo window size. These settings are used when showing the extension
control, to restore it to how it used to be before closing it.

The available information for each view extension is:

1. How it is shown. For now this can be either as a floating window or
docked to the right-side panel.

2. In case it is shown as a floating window, the following information
about the window is saved:
- Location by Top/Left coordinates
- Size by Width/Height
- Whether the window is maximized

* add revit dlls to assembly conflict checker ignore list (#11285)

* Update StartupUtils.cs

Co-authored-by: pinzart <[email protected]>

* Fix sanitizing log warnings

* Node AutoComplete suggestions sorted alphabetically within the same group (#11280)


* add/update tests

* Address review comments

* Add ILMerge

* Fix typo

* Fix mono build

* Revert "Fix mono build"

This reverts commit 9904525.

* Refine mono solution

* Remove task condition

* Mono build fix

* Add Readme

Co-authored-by: aparajit-pratap <[email protected]>
Co-authored-by: Ashish Aggarwal <[email protected]>
Co-authored-by: Laurence Elsdon <[email protected]>
Co-authored-by: Martin Misol Monzo <[email protected]>
Co-authored-by: pinzart90 <[email protected]>
Co-authored-by: pinzart <[email protected]>

Co-authored-by: Radu Gidei <[email protected]>
Co-authored-by: Jorgen Dahl <[email protected]>
Co-authored-by: Jorgen Dahl <[email protected]>
Co-authored-by: aparajit-pratap <[email protected]>
Co-authored-by: pinzart90 <[email protected]>
Co-authored-by: Aaron (Qilong) <[email protected]>
Co-authored-by: Michael Kirschner <[email protected]>
Co-authored-by: Martin Misol Monzo <[email protected]>
Co-authored-by: Ashish Aggarwal <[email protected]>
Co-authored-by: reddyashish <[email protected]>
Co-authored-by: Laurence Elsdon <[email protected]>
Co-authored-by: pinzart <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants