[DYN-1682]: Improve node warning, autocomplete and Node2Code behavior with class name conflicts

[aparajit-pratap]

Purpose

This attempts to fix an inconsistent behavior with multiple definition warnings being shown when making certain function calls in CBN's vs. executing the nodes without errors.

In the example below, there are 2 List classes, one, the built-in type and the other from a package (Orchid). Autocomplete displays both the List classes, the second one prefixed with a namespace (Orchid) to distinguish it uniquely from the first. Autocomplete indicates that the first List type can be used as-is. Even then the node would show multiple definition warnings indicating it cannot resolve which List class to use. However, at runtime, this does not pose an issue and the node executes without any errors. This clearly inconsistent behavior is fixed here.

The new behavior shows no multiple definition warnings when the CBN is initially created and is consistent both with the autocomplete prompts and the successful execution.

---

In addition to the above, few other existing bugs have been fixed in CBN autocomplete and in Node to Code. These are better explained with an example of List, DSCore.List, and Orchid.Core.List, where DSCore.List is hidden from the library and List is a class in the global namespace that derives from DSCore.List. Orchid.Core.List is an external class from a package.

Some notes to keep in mind:

1. These include fixes to a ShortestQualifiedNameReplacer class that basically rewrites a collection of classes, all having the same name, but with different namespaces. This component ensures that the full names of these classes are automatically shortened (to make CBN code less verbose) so that they can still be resolved uniquely. For example, in this case, Orchid.Core.List, can be renamed to Orchid.List, by dropping the Core, from its full namespace as DesignScript (in its implementation of Dynamo only) can still recognize it uniquely, separate from the other two.
2. CBN autocomplete does not work on namespaces, only on classes as the concept of namespaces is not expected to be known by end-users. However, we cannot completely do away with showing namespaces in CBNs in cases where we need to differentiate between classes with the same classname coming from different libraries.
3. Hidden classes (with IsVisibleInLibrary(false) attribute) do not show in the library and for consistency, we should not make them appear in code in CBNs either. However, a hidden class can have explicitly visible methods and properties (if overridden with IsVisibleInLibrary(true) attribute), which will show up as nodes in the library. Only if such a class is inherited by a subclass, will its methods be displayed in autocomplete in CBN under the derived class. An example of such a class is DSCore.List, where its methods are visible as nodes in the library and they appear under List in CBN autocomplete. Hidden classes if used in CBN (even if not discovered by autocomplete) will still autocomplete to show its members.
4. A component called ElementResolver maintains a map of short names vs. fully qualified names, based on their use in CBNs. The purpose of this map is such that if a new library with a conflicting classname is imported into an existing graph, CBN code continues to work and does not break even in the presence of a new conflict that is introduced at a later time. These conflicts if left to be resolved by the compiler could give new results causing existing graphs to break. In such cases, we lean on the ER to give us the exact classname that was resolved earlier.

1. Autocomplete behavior and fixes

• When beginning to type L, i, s..., autocomplete begins to filter all the classes with the name List and only shows List and Orchid.List as it hides the hidden DSCore.List class
• When autocompleting on List, all methods and properties on List and its base class (DSCore.List) are displayed
• When typing DSCore.List - (1) no autocomplete on DSCore namespace is expected and (2) code completion on DSCore.List, will display method list only on that base class (not on the derived List class)
• Code completion of hidden class (if typed without autocomplete assist), still displays members of hidden class

2. Node warning fixes

• This section essentially refers to the original bug (DYN-1682), that we planned to fix (before this task exploded to include all these other related improvements). This bug refers to the fact that a node from either List, DSCore.List or Orchid.Core.List must not throw a multiple definition warning at compile time as all of them are expected to resolve uniquely (both as nodes as well as in CBN code).

Before:

After:

3. Node To Code behavior and fixes

• N2C on List nodes should generate List._MethodName_ and List._PropertyName(obj)_ in CBN
• N2C on DSCore.List nodes should also generate the List._MethodName_ as above (since DSCore.List is hidden and is a base class of List)
• N2C on Orchid.Core.List node should generate CBN with the shortest unique name when compared with all other classes with the name List, which in this case is Orchid.List._MethodName_.
• What if we perform N2C on a class that is hidden, and its methods (aka nodes) are visible but it is not inherited by any other visible class? In such a rare case, N2C generates the fully qualified namespace (as the user might expect to see something in code for a node that exists, even though its class is hidden)

Before:

After:

4. Prefixing classes with full namespaces without assistance from autocomplete

• In cases where advanced users may be aware of full namespaces of classes they wish to use in CBN, there will be no autocomplete assistance on those namespaces, only on the final classname that they type. The full namespaces will, however, be added to the ElementResolver. For e.g., if a user types the full name: Orchid.Core.List, even though Orchid.List would suffice, Orchid.Core.List would start appearing in autocomplete and in N2C code.
• If a hidden class is typed there will be no indication of its existence in autocomplete (as it's meant to be hidden), the CBN will still execute (as it's a valid class imported into the VM), however, it will not be added to the ElementResolver as if it is, the hidden class will start to unexpectedly appear in CBN autocomplete and N2C behavior, which will be confusing to the end-user. For example, if DSCore.List is typed into a CBN, it will not be added to the ElementResolver so that it does not appear in any CBN autocompletion lists or N2C code generated subsequently.

Note that there is a proposal to display the contents of the ER map in the UI, so that it is not a Blackbox and so that users can understand why in their graph certain namespaces in CBNs are working and why others are not.

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.

Reviewers

@DynamoDS/dynamo

FYIs

@Amoursol

[mjkkirschner]

@aparajit-pratap it looks good except for one question regarding tests.

[QilongTang]

LGTM

[aparajit-pratap]

There is a regression I found in node to code (for which there was no test) so I'm continuing on this to fix that.

[QilongTang]

👍

[QilongTang]

Does internal work here?

[aparajit-pratap]

No, since this method is defined in ProtoCore but now being used in DynamoCore.

[mjkkirschner]

I understand this is being used by dynamo core - we also can use the internalsVisibleTo attribute to expose internal members to other assemblies which we own.

We should develop this principal further, but a few points.

• ProtoCore.dll is currently shipped as a part of the DynamoCore nuget package. To me looking at this package, as an external developer I see this as part of the API.
• A public method in this assembly which is shipped as part of a nuget package, will end up needing to be maintained.

Is this method something that should be used externally by integrators? Is there a compelling reason to make it part of the API? An interesting use case? A workflow that cannot be implemented without it?

If not, then what is the benefit?

If yes, then great, let's add documentation to it.

[aparajit-pratap]

Have we made a decision on going forward with reducing the API surface of DynamoCore, DynamoCoreWpf etc. by continuing to expose them as our public API or are we deciding on keeping them as-is and making them internal and having a separate API layer for public consumption (based on your RFC)?

Whatever is the strategy, I think then we should come up with certain coding guidelines before we continue to make PR's. Going by the RFC again, it sounds like none of ProtoCore should be part of the API and we should surface just the AST's publicly. I'll make this method internal as well for now.

[mjkkirschner]

No, we have made no decision -very much welcome those discussions - there are many options.
Thanks for addressing this, I don't think we can stop all work before we entirely decide this - but what I am suggesting is that we proceed conservatively until we do so.

I think it's likely we will find that some users are already using ProtoCore, and other binaries we didn't consider fully as APIs.

For example, the Civil3d integration has very specific interactions with callsites and trace, I am not 100% what APIs that team uses to control and redirect trace but I guess it is in protocore.

[mjkkirschner]

just double checking - this should always be 0?

[aparajit-pratap]

Yes, we want the most derived class in that list.