Added ElementResolver to resolve namespace conflicts in CBN

[aparajit-pratap]

Overview

The objective here is to implement a strategy to handle namespace conflicts when there are multiple libraries loaded in Dynamo and to prevent breaking users' files as far as possible when they are opened in different Dynamo library environments. The approach here is to:

1. Embed (class => namespace) mapping information into the DYN file when saved
2. An ElementResolver object created when reading the file loads the information into its class resolution map (ResolutionMap)
3. At the pre-compilation stage of the CBN (post-parsing) the ElementResolver reads its ResolutionMap and transforms the AST (obtained from parsing) by swapping its partial class names with the fully resolved names in the ResolutionMap. This happens in the ResolveClassNamespace method.
4. If no mapping is found in the ResolutionMap, the ElementResolver updates it by relying on the compiler and classTable to resolve the namespace for a given partial class name. (If no unique class can be resolved, only then is a conflict warning generated for the user.)
5. The transformed AST containing the fully resolved class identifiers are later sent to the VM for execution. The idea here is to transform the ASTs internally and not have to drop long namespaces in user facing code
6. When the DYN file is saved, the updated ResolutionMap in the ElementResolver is persisted to the file.
   
   

@lukechurch please review this first cut implementation. I would need @junmendoza to help me by providing the implementations of the core functions stubbed out below. These are mainly required by the ElementResolver to initially retrieve the fully resolved names from the compiler when there is no embedded information in the DYN file (i.e. when new code is typed in the CBN as well as migration of existing files). They are also needed to update the ASTs internally with the fully resolved class names which would be eventually sent to the VM.

I need to implement embedding and reading of namespace information from DYN files. Currently I have added TODO placeholders for these calls. Also short name heuristics will come in subsequent submissions.

/// <summary>
        /// Get fully resolved name as identifier list from class table
        /// </summary>
        /// <param name="classTable"> class table in Core </param>
        /// <param name="partialName"> partial class name </param>
        /// <returns> fully resolved name as Identifier list </returns>
        private static string GetResolvedClassName(ClassTable classTable, string partialName);

        /// <summary>
        /// Replace partial identifier with fully resolved identifier list in original AST
        /// This is the same AST that is also passed to the VM for execution
        /// </summary>
        /// <param name="astNode"></param>
        /// <param name="partialName"> partial class name/identifier </param>
        /// <param name="resolvedName"> fully qualified classname </param>
        private static void ReplacePartialWithFullNode(ref AssociativeNode astNode, string partialName, string resolvedName);

        /// <summary>
        /// Find all partial class (Identifier/IdentifierLists) by performing a DFS traversal on input AST node
        /// </summary>
        /// <param name="astNode"> input AST node </param>
        /// <returns> list of IdentifierNode/IdentifierList of Class identifiers </returns>
        private static IEnumerable<string> GetClassIdentifiers(AST.Node astNode);


        // Given an input fully resolved class name, convert it into an Identifier/IdentifierListNode
        private void InitializeNamespaceCache(string[] namespaceList);

[aparajit-pratap]

Adding @ikeough as reviewer

[lukechurch]

@aparajit-pratap It's not entirely clear what you're asking for here. Are you esssentially looking for a design review on the API?

[lukechurch]

Please can you populate these /// comments whilst you're at it?

[lukechurch]

Thanks for this @aparajit-pratap it's good progress. I think the direction could use tweaking a little. I'd like to explore using an API that was less state based.

So there would be a method which resolves a partial name to a full name without using a cache, and we make this sufficiently fast that we can do it directly in the replacement methods without adding more state.

I guess the main challenge here is around the ClassTable structure that you're basing the translation off?

[junmendoza]

Regarding these APIs you propose:

    private static string GetResolvedClassName(ClassTable classTable, string partialName);
    private static void ReplacePartialWithFullNode(ref AssociativeNode astNode, string partialName, string resolvedName);
    private static IEnumerable<string> GetClassIdentifiers(AST.Node astNode);

I think they are generally fine. Ideally we'd be able to yank them out of codegen and call them separately.
The signatures are ok, but ill bring it up in case they need to be modified for any reason.

[aparajit-pratap]

@lukechurch

[lukechurch]

As discussed resolutionMap

[lukechurch]

Hey Aparajit,

Thanks for the discussion, to summarise:

• refactor the Compiler utils -> static to make this easier to review
• It's not really a cache that is being built, it's a lookup map, lets rename and update the design and the code as appropriate
• Lets try and tighten the API up a little. The Element Resolver is doing two things, one is handling lookup s, the other is rewriting the ASTs. Lets separate them into two separate classes (Element Resolver) and (Element Rewriter), but that it's very clear if a method is one of the lookup methods, or one of the rewrite methods.

I suspect we're going to end up wanting the UI to be able to query for lookups in other places in the code.

thanks

[lukechurch]

Dict: String -> String

[junmendoza]

Just for clarity, would it still mean the same if I said ASTResolver and ASTRewiter?
Im not saying the name should be changed btw.

[aparajit-pratap]

Element over here I guess mainly refers to namespace/class resolution. It was a term given by Luke. I'll just leave it at that.

[junmendoza]

Overall, the changes needed for the AST resolving look good. It conforms to what was discussed

[aparajit-pratap]

@ke-yu could you also take a look at this PR. I think @Benglin is tied up.

[Benglin]

This causes multiple enumeration through the astNodes, please move this out as var count = astNodes.Count();, or better still, use foreach (I think that's possible now that it is not an argument by ref).

[Benglin]

Hi @aparajit-pratap, first off my apology for not having time for this (it is large, and with each file added to review it feels heavier and heavier to review), but I manage to park some time for that today. I have made some comments for the things I would like changed (few optional). I still believe this PR can be broken down into small bits with corresponding tests, let's try to do that moving forward :)

[ke-yu]

positive condition goes firstly :-)

[aparajit-pratap]

@Benglin I'm done addressing your comments and adding test cases. Please review one final time. Thanks.

[Benglin]

Thanks for addressing those points, @aparajit-pratap. I have a look through, generally it LGTM.