[DYN-1682]: Improve node warning, autocomplete and Node2Code behavior…

… with class name conflicts (#10558)

* return single matching class name for exact match

* fix code and update test

* refactor code

* code fixes to account for name shortening in node to code

* code cleanup

* fix legacy bug with getting unique shortest name with hidden classes

* final fixes

* do not add hidden class to element resolver

* more fixes

* add tests

* some final tests

* update failing tests

* add test for multiple def warning

* code cleanup and one more test

* make protocore internals visible to dynamocore

* add docs

* add docs to ShortestQualifiedNameReplacer class

src/DynamoCore/Engine/CodeCompletion/CodeCompletionServices.cs

@@ -191,7 +191,7 @@ private void AddTypesToCompletionData(string stringToComplete, List<CompletionDa
             var partialName = stringToComplete.ToLower();
             // Add matching Classes
             var classMirrorGroups = StaticMirror.GetAllTypes(core).
-                Where(x => !x.IsHiddenInLibrary && x.Alias.ToLower().Contains(partialName)).
+                Where(x => x.Alias.ToLower().Contains(partialName)).
                     GroupBy(x => x.Alias);
 
             foreach (var classMirrorGroup in classMirrorGroups)
@@ -214,7 +214,7 @@ private void AddTypesToCompletionData(string stringToComplete, List<CompletionDa
                 }
                 // Filter out empty types
                 completions.AddRange(classMirrorGroup.
-                    Where(x => !x.IsEmpty).
+                    Where(x => !x.IsEmpty && !x.IsHiddenInLibrary).
                         Select(x =>
                                 CompletionData.ConvertMirrorToCompletionData(x, useShorterName,
                                     resolver: resolver)));

src/DynamoCore/Engine/ShortestQualifiedNameReplacer.cs

@@ -1,4 +1,5 @@
 using System.Linq;
+
 using ProtoCore;
 using ProtoCore.AST.AssociativeAST;
 using ProtoCore.DSASM;
@@ -10,7 +11,11 @@
 namespace Dynamo.Engine
 {
     /// <summary>
-    /// Replace a fully qualified function call with short name. 
+    /// Replaces a fully qualified class name with a short name.
+    /// Ensures that fully qualified namespaces of classes with the same name are
+    /// automatically shortened to partial namespaces so that they can still be resolved uniquely.
+    /// E.g., given {"A.B.C.D.E", "X.Y.A.B.E.C.E", "X.Y.A.C.B.E"}, all with the same class E,
+    /// their shortest unique names would be: {"D.E", "E.E", "C.B.E"}.
     /// </summary>
     internal class ShortestQualifiedNameReplacer : AstReplacer
     {
@@ -90,7 +95,40 @@ public override AssociativeNode VisitIdentifierListNode(IdentifierListNode node)
                 RightNode = rightNode,
                 Optr = Operator.dot
             };
-            return leftNode != newLeftNode ? node : RewriteNodeWithShortName(node);
+            return node;
+        }
+
+        public override AssociativeNode VisitIdentifierNode(IdentifierNode node)
+        {
+            var name = node.ToString();
+            string[] strIdentList = name.Split('.');
+
+            // return IdentifierNode if identifier string is not separated by '.'
+            if (strIdentList.Length == 1)
+            {
+                return new IdentifierNode(strIdentList[0]);
+            }
+
+            // create IdentifierListNode from string such that
+            // RightNode is IdentifierNode representing classname
+            // and LeftNode is IdentifierNode representing one or more namespaces.
+            var rightNode = new IdentifierNode(strIdentList.Last());
+            string ident = "";
+            for (int i = 0; i < strIdentList.Length - 1; i++)
+            {
+                if (!string.IsNullOrEmpty(ident))
+                    ident += ".";
+                ident += strIdentList[i];
+            }
+            var leftNode = new IdentifierNode(ident);
+
+            var identListNode = new IdentifierListNode
+            {
+                LeftNode = leftNode,
+                RightNode = rightNode,
+                Optr = Operator.dot
+            };
+            return identListNode.Accept(this);
         }
 
         private bool TryShortenClassName(IdentifierListNode node, out AssociativeNode shortNameNode)
@@ -108,84 +146,79 @@ private bool TryShortenClassName(IdentifierListNode node, out AssociativeNode sh
             if (matchingClasses.Length == 0)
                 return false;
 
-            string className = qualifiedName.Split('.').Last();
-
-            var symbol = new ProtoCore.Namespace.Symbol(qualifiedName);
-            if (!symbol.Matches(node.ToString()))
+            if(qualifiedName != node.ToString())
                 return false;
 
+            string className = qualifiedName.Split('.').Last();
+
             shortNameNode = CreateNodeFromShortName(className, qualifiedName);
             return shortNameNode != null;
         }
 
-        private IdentifierListNode RewriteNodeWithShortName(IdentifierListNode node)
+        private AssociativeNode CreateNodeFromShortName(string className, string qualifiedName)
         {
-            // Get class name from AST
-            string qualifiedName = CoreUtils.GetIdentifierExceptMethodName(node);
+            // Get the list of conflicting namespaces that contain the same class name
+            var matchingClasses = classTable.ClassNodes.Where(
+                x => x.Name.Split('.').Last().Equals(className)).ToList();
 
-            // if it is a global method
-            if (string.IsNullOrEmpty(qualifiedName))
-                return node;
+            if (matchingClasses.Count == 0)
+                return null;
 
-            // Make sure qualifiedName is not a property
-            var lNode = node.LeftNode;
-            var matchingClasses = classTable.GetAllMatchingClasses(qualifiedName);
-            while (matchingClasses.Length == 0 && lNode is IdentifierListNode)
+            if (matchingClasses.Count == 1)
             {
-                qualifiedName = lNode.ToString();
-                matchingClasses = classTable.GetAllMatchingClasses(qualifiedName);
-                lNode = ((IdentifierListNode)lNode).LeftNode;
+                // If there is no class conflict simply use the class name as the shortest name.
+                return CoreUtils.CreateNodeFromString(className);
             }
-            qualifiedName = lNode.ToString();
-            string className = qualifiedName.Split('.').Last();
-
-            var newIdentList = CreateNodeFromShortName(className, qualifiedName);
-            if (newIdentList == null)
-                return node;
 
-            // Replace class name in input node with short name (newIdentList)
-            node = new IdentifierListNode
+            var shortName = resolver?.LookupShortName(qualifiedName);
+            if (!string.IsNullOrEmpty(shortName))
             {
-                LeftNode = newIdentList,
-                RightNode = node.RightNode,
-                Optr = Operator.dot
-            };
-            return node;
-        }
+                return CoreUtils.CreateNodeFromString(shortName);
+            }
 
-        private AssociativeNode CreateNodeFromShortName(string className, string qualifiedName)
-        {
-            // Get the list of conflicting namespaces that contain the same class name
-            var matchingClasses = CoreUtils.GetResolvedClassName(classTable, AstFactory.BuildIdentifier(className));
-            if (matchingClasses.Length == 0)
-                return null;
+            // Use the namespace list to derive the list of shortest unique names
+            var symbolList =
+                matchingClasses.Select(matchingClass => new Symbol(matchingClass.Name));
+            var shortNames = Symbol.GetShortestUniqueNames(symbolList);
 
-            string shortName;
-            // if there is no class conflict simply use the class name as the shortest name
-            if (matchingClasses.Length == 1)
+            // remove hidden class if any from shortNames before proceeding
+            var hiddenClassNodes = matchingClasses.
+                Where(x => x.ClassAttributes?.HiddenInLibrary ?? false).ToList();
+            if(hiddenClassNodes.Any())
             {
-                shortName = className;
+                foreach (var hiddenClassNode in hiddenClassNodes)
+                {
+                    var keyToRemove = new Symbol(hiddenClassNode.Name);
+                    shortNames.Remove(keyToRemove);
+                }
             }
-            else
+
+            // Get the shortest name corresponding to the fully qualified name
+            if(shortNames.TryGetValue(new Symbol(qualifiedName), out shortName))
             {
-                shortName = resolver != null ? resolver.LookupShortName(qualifiedName) : null;
+                return CoreUtils.CreateNodeFromString(shortName);
+            }
 
-                if (string.IsNullOrEmpty(shortName))
+            // If shortName for fully qualified classname is not found, it could be a base class
+            // present in class hierarchy of any of the other matchingClasses, in which case
+            // set shortName to the one for the derived class.
+            var qualifiedClassNode = matchingClasses.FirstOrDefault(x => x.Name == qualifiedName);
+            var classHierarchies = matchingClasses.Where(x => x != qualifiedClassNode).
+                Select(y => classTable.GetClassHierarchy(y));
+            foreach (var hierarchy in classHierarchies)
+            {
+                // If A derives from B, which derives from C, the hierarchy for A 
+                // is listed in that order: [A, B, C], so we start searching in reverse order.
+                for (int i = hierarchy.Count - 1; i > 0; i--)
                 {
-                    // Use the namespace list as input to derive the list of shortest unique names
-                    var symbolList =
-                        matchingClasses.Select(matchingClass => new ProtoCore.Namespace.Symbol(matchingClass))
-                            .ToList();
-                    var shortNames = ProtoCore.Namespace.Symbol.GetShortestUniqueNames(symbolList);
-
-                    // Get the shortest name corresponding to the fully qualified name
-                    shortName = shortNames[new ProtoCore.Namespace.Symbol(qualifiedName)];
+                    if (hierarchy[i] == qualifiedClassNode)
+                    {
+                        shortName = shortNames[new Symbol(hierarchy[0].Name)];
+                        return CoreUtils.CreateNodeFromString(shortName);
+                    }
                 }
             }
-            // Rewrite the AST using the shortest name
-            var newIdentList = CoreUtils.CreateNodeFromString(shortName);
-            return newIdentList;
-
+            return null;
         }
     }
 }

src/Engine/ProtoCore/ClassTable.cs

@@ -392,8 +392,15 @@ public class ClassTable
 
         //Symbol table to manage symbols with namespace
         private Namespace.SymbolTable symbolTable = new Namespace.SymbolTable();
-
-        private List<ClassNode> GetClassHierarchy(ClassNode node)
+
+        /// <summary>
+        /// Returns the class hierarchy for a given class node.
+        /// If A derives from B, which in turn derives from C,
+        /// the hierarchy for A returned is in the order: [A, B, C].
+        /// </summary>
+        /// <param name="node"></param>
+        /// <returns>List of classes in hierarchy.</returns>
+        internal List<ClassNode> GetClassHierarchy(ClassNode node)
         {
             var cNodes = new List<ClassNode> {node};
 
@@ -538,10 +545,17 @@ public bool TryGetFullyQualifiedName(string name, out string fullName)
         /// <returns>Array of fully qualified name of all matching symbols</returns>
         public string[] GetAllMatchingClasses(string name)
         {
+            // First check if there is an exact name match with fully qualified classname
+            Symbol symbol;
+            if (symbolTable.TryGetExactSymbol(name, out symbol))
+            {
+                if (Constants.kInvalidIndex != symbol.Id)
+                    return new[] {name};
+            }
+
             var symbols = symbolTable.TryGetSymbols(name, s => s.Matches(name));
 
             var classes = new List<string>();
-
             if (symbols.Length > 1)
             {
                 var baseClass = GetCommonBaseClass(symbols);

src/Engine/ProtoCore/Namespace/ElementRewriter.cs

@@ -169,9 +169,15 @@ private string ResolveClassName(AssociativeNode identifierList)
             if (matchingClasses.Length == 1)
             {
                 resolvedName = matchingClasses[0];
-                var assemblyName = CoreUtils.GetAssemblyFromClassName(classTable, resolvedName);
 
-                elementResolver.AddToResolutionMap(partialName, resolvedName, assemblyName);
+                var classNode = classTable.ClassNodes.FirstOrDefault(x => x.Name == resolvedName);
+                var isHiddenInLibrary = classNode.ClassAttributes?.HiddenInLibrary ?? false;
+                // Do not add a hidden class to element resolver.
+                if (!isHiddenInLibrary)
+                {
+                    var assemblyName = CoreUtils.GetAssemblyFromClassName(classTable, resolvedName);
+                    elementResolver.AddToResolutionMap(partialName, resolvedName, assemblyName);
+                }
             }
             else if (matchingClasses.Length > 1)
             {

src/Engine/ProtoCore/Properties/AssemblyInfo.cs

@@ -11,6 +11,7 @@
 // [assembly: AssemblyKeyFile("ProtoCore.snk")]
 [assembly: InternalsVisibleTo("ProtoTest")]
 [assembly: InternalsVisibleTo("ProtoTestFx")]
+[assembly:InternalsVisibleTo("DynamoCore")]
 [assembly: InternalsVisibleTo("DynamoCoreTests")]
 [assembly: InternalsVisibleTo("DynamoCoreWpfTests")]
 

test/DynamoCoreTests/CodeBlockNodeTests.cs

@@ -2075,6 +2075,24 @@ public void TestCompletionWhenTyping()
             Assert.AreEqual(expected, actual);
         }
 
+        [Test]
+        [Category("UnitTests")]
+        public void TestCompletionWithGlobalClassWhenTyping()
+        {
+            var codeCompletionServices = new CodeCompletionServices(libraryServicesCore);
+            string code = "Dupt";
+            var completions = codeCompletionServices.SearchCompletions(code, Guid.Empty);
+
+            // Expected 3 completion items
+            Assert.AreEqual(3, completions.Count());
+
+            string[] expectedValues = {"DupTargetTest", "A.DupTargetTest", "FFITarget.B.DupTargetTest"};
+            var expected = expectedValues.OrderBy(x => x);
+            var actual = completions.Select(x => x.Text).OrderBy(x => x);
+
+            Assert.AreEqual(expected, actual);
+        }
+
         [Test]
         [Category("UnitTests")]
         public void TestMethodKeywordCompletionWhenTyping()
@@ -2110,6 +2128,29 @@ public void TestCompletionOnType2()
             Assert.AreEqual(5, completions.Count());
         }
 
+        [Test]
+        [Category("UnitTests")]
+        public void TestCompletionOnDerivedTypeReturnsBaseType()
+        {
+            var codeCompletionServices = new CodeCompletionServices(libraryServicesCore);
+            var completions = codeCompletionServices.GetCompletionsOnType("", "DupTargetTest").ToList();
+            Assert.AreEqual(3, completions.Count);
+            Assert.AreEqual("Foo", completions[0].Text);
+            Assert.AreEqual("DupTargetTest", completions[1].Text);
+            Assert.AreEqual("Bar", completions[2].Text);
+        }
+
+        [Test]
+        [Category("UnitTests")]
+        public void TestCompletionOnBaseTypeReturnsOnlyBaseType()
+        {
+            var codeCompletionServices = new CodeCompletionServices(libraryServicesCore);
+            var completions = codeCompletionServices.GetCompletionsOnType("", "FFITarget.C.B.DupTargetTest").ToList();
+            Assert.AreEqual(2, completions.Count);
+            Assert.AreEqual("DupTargetTest", completions[0].Text);
+            Assert.AreEqual("Foo", completions[1].Text);
+        }
+
         [Test]
         [Category("UnitTests")]
         public void TestHiddenClassCompletionWhenTyping()
@@ -2146,7 +2187,7 @@ public void TestHiddenConflictingClassCompletionWhenTyping()
             // Expected 1 completion items
             Assert.AreEqual(1, completions.Count());
 
-            string[] expected = { "AnotherClassWithNameConflict" };
+            string[] expected = { "FirstNamespace.AnotherClassWithNameConflict" };
             var actual = completions.Select(x => x.Text).OrderBy(x => x);
 
             Assert.AreEqual(expected, actual);