improve lookups using HashSets and Dictionaries

jonsequitur · jonsequitur · commit f919a1157d9d · 2020-07-13T11:35:14.000-07:00
diff --git a/src/System.CommandLine.Tests/ArgumentTests.cs b/src/System.CommandLine.Tests/ArgumentTests.cs
@@ -355,16 +355,13 @@ public void Default_value_and_custom_argument_parser_can_be_used_together()
                 var argument = new Argument<int>(_ => 789, true);
                 argument.SetDefaultValue(123);
 
-                var result =  argument.Parse("");
+                var result = argument.Parse("");
 
-                var argumentResult = result.FindResultFor(argument);
-
-                argumentResult
-                    .GetValueOrDefault<int>()
-                    .Should()
-                    .Be(123);
+                result.ValueForArgument(argument)
+                      .Should()
+                      .Be(123);
             }
-            
+
             [Fact]
             public void Multiple_command_arguments_can_have_custom_parse_delegates()
             {
diff --git a/src/System.CommandLine/ArgumentArity.cs b/src/System.CommandLine/ArgumentArity.cs
@@ -30,16 +30,15 @@ public ArgumentArity(int minimumNumberOfValues, int maximumNumberOfValues)
         public int MaximumNumberOfValues { get; set; }
 
         internal static FailedArgumentConversionArityResult? Validate(
-            SymbolResult? symbolResult,
+            SymbolResult symbolResult,
             IArgument argument,
             int minimumNumberOfValues,
             int maximumNumberOfValues)
         {
             var argumentResult = symbolResult switch
             {
-                CommandResult commandResult => commandResult.Root?.FindResultFor(argument),
-                OptionResult optionResult => optionResult.Children.ResultFor(argument),
-                _ => symbolResult
+                ArgumentResult a => a,
+                _ => symbolResult.Root!.FindResultFor(argument)
             };
 
             var tokenCount = argumentResult?.Tokens.Count ?? 0;
diff --git a/src/System.CommandLine/Binding/ArgumentConversionResultSet.cs b/src/System.CommandLine/Binding/ArgumentConversionResultSet.cs
@@ -8,12 +8,12 @@ namespace System.CommandLine.Binding
 {
     internal class ArgumentConversionResultSet : AliasedSet<ArgumentConversionResult>
     {
-        protected override IReadOnlyList<string> GetAliases(ArgumentConversionResult item)
+        protected override IReadOnlyCollection<string> GetAliases(ArgumentConversionResult item)
         {
             return new[] { item.Argument.Name };
         }
 
-        protected override IReadOnlyList<string> GetRawAliases(ArgumentConversionResult item)
+        protected override IReadOnlyCollection<string> GetRawAliases(ArgumentConversionResult item)
         {
             return new[] { item.Argument.Name };
         }
diff --git a/src/System.CommandLine/Binding/FailedArgumentTypeConversionResult.cs b/src/System.CommandLine/Binding/FailedArgumentTypeConversionResult.cs
@@ -25,7 +25,7 @@ private static string FormatErrorMessage(
             {
                 // TODO: (FailedArgumentTypeConversionResult) localize
 
-                var firstParent = a.Parents.First();
+                var firstParent = a.Parents[0];
 
                 var symbolType =
                     firstParent switch {
@@ -34,7 +34,7 @@ private static string FormatErrorMessage(
                         _ => null
                         };
 
-                var alias = firstParent.RawAliases[0];
+                var alias = firstParent.RawAliases.First();
 
                 return $"Cannot parse argument '{value}' for {symbolType} '{alias}' as expected type {type}.";
             }
diff --git a/src/System.CommandLine/Collections/AliasedSet.cs b/src/System.CommandLine/Collections/AliasedSet.cs
@@ -9,41 +9,23 @@ namespace System.CommandLine.Collections
     public abstract class AliasedSet<T> : IReadOnlyList<T>
         where T : class
     {
+        private protected Dictionary<string, T> _itemsByAlias = new Dictionary<string, T>();
+
         protected IList<T> Items { get; } = new List<T>();
 
         public T? this[string alias] => GetByAlias(alias);
 
         public T? GetByAlias(string alias)
         {
-            for (var i = 0; i < Items.Count; i++)
+            if (_itemsByAlias.TryGetValue(alias, out var value) && 
+                value is { })
             {
-                var item = Items[i];
-
-                if (Contains(GetAliases(item), alias) || 
-                    Contains(GetRawAliases(item), alias))
-                {
-                    return item;
-                }
+                return value;
             }
 
             return null;
         }
 
-        private protected bool Contains(
-            IReadOnlyList<string> aliases,
-            string alias)
-        {
-            for (var i = 0; i < aliases.Count; i++)
-            {
-                if (string.Equals(aliases[i], alias))
-                {
-                    return true;
-                }
-            }
-
-            return false;
-        }
-
         public int Count => Items.Count;
 
         IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
@@ -53,18 +35,50 @@ private protected bool Contains(
         internal virtual void Add(T item)
         {
             Items.Add(item);
+
+            foreach (var alias in GetRawAliases(item))
+            {
+                if (!_itemsByAlias.ContainsKey(alias))
+                {
+                    _itemsByAlias.Add(alias, item);
+                }
+            }
+
+            foreach (var alias in GetAliases(item))
+            {
+                if (!_itemsByAlias.ContainsKey(alias))
+                {
+                    _itemsByAlias.Add(alias, item);
+                }
+            }
         }
 
         internal void Remove(T item)
         {
             Items.Remove(item);
+
+            foreach (var alias in GetRawAliases(item))
+            {
+                if (_itemsByAlias.ContainsKey(alias))
+                {
+                    _itemsByAlias.Remove(alias);
+                }
+            }
+
+            foreach (var alias in GetAliases(item))
+            {
+                if (_itemsByAlias.ContainsKey(alias))
+                {
+                    _itemsByAlias.Remove(alias);
+                }
+            }
         }
 
-        protected abstract IReadOnlyList<string> GetAliases(T item);
+        protected abstract IReadOnlyCollection<string> GetAliases(T item);
 
-        protected abstract IReadOnlyList<string> GetRawAliases(T item);
+        protected abstract IReadOnlyCollection<string> GetRawAliases(T item);
 
-        public bool Contains(string alias) => GetByAlias(alias) != null;
+        public bool Contains(string alias) => _itemsByAlias.ContainsKey(alias);
 
         public T this[int index] => Items[index];
     }
diff --git a/src/System.CommandLine/Collections/SymbolSet.cs b/src/System.CommandLine/Collections/SymbolSet.cs
@@ -3,6 +3,7 @@
 
 using System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
+using System.Linq;
 
 namespace System.CommandLine.Collections
 {
@@ -21,65 +22,51 @@ internal bool IsAnyAliasInUse(
             ISymbol item,
             [MaybeNullWhen(false)] out string aliasAlreadyInUse)
         {
-            var itemRawAliases = GetRawAliases(item);
-
-            for (var i = 0; i < Items.Count; i++)
+            if (item is IArgument)
             {
-                var existingItem = Items[i];
+                // arguments don't have aliases so match based on Name
+                for (var i = 0; i < Items.Count; i++)
+                {
+                    var existing = Items[i];
+                    if (string.Equals(item.Name, existing.Name, StringComparison.Ordinal))
+                    {
+                        aliasAlreadyInUse = existing.Name;
+                        return true;
+                    }
+                }
+            }
+            else
+            {
+                var itemRawAliases = item.RawAliases.ToArray();
 
-                for (var j = 0; j < itemRawAliases.Count; j++)
+                for (var i = 0; i < itemRawAliases.Length; i++)
                 {
-                    var rawAliasToCheckFor = itemRawAliases[j];
+                    var alias = itemRawAliases[i];
 
-                    if (Contains(GetRawAliases(existingItem), rawAliasToCheckFor))
+                    if (_itemsByAlias.ContainsKey(alias))
                     {
-                        aliasAlreadyInUse = rawAliasToCheckFor;
+                        aliasAlreadyInUse = alias;
                         return true;
                     }
                 }
             }
 
             aliasAlreadyInUse = null!;
-            return false;
 
-            static IReadOnlyList<string> GetRawAliases(ISymbol symbol)
-            {
-                return symbol switch
-                {
-                    IArgument arg => new[] { arg.Name },
-                    _ => symbol.RawAliases
-                };
-            }
+            return false;
         }
 
         internal void ThrowIfAnyAliasIsInUse(ISymbol item)
         {
-            string? rawAliasAlreadyInUse;
-
-            switch (item)
+            if (IsAnyAliasInUse(item, out var rawAliasAlreadyInUse))
             {
-                case IOption _:
-                case ICommand _:
-                    if (IsAnyAliasInUse(item, out rawAliasAlreadyInUse))
-                    {
-                        throw new ArgumentException($"Alias '{rawAliasAlreadyInUse}' is already in use.");
-                    }
-
-                    break;
-
-                case IArgument argument:
-                    if (IsAnyAliasInUse(argument, out rawAliasAlreadyInUse))
-                    {
-                        throw new ArgumentException($"Alias '{rawAliasAlreadyInUse}' is already in use.");
-                    }
-
-                    break;
+                throw new ArgumentException($"Alias '{rawAliasAlreadyInUse}' is already in use.");
             }
         }
 
-        protected override IReadOnlyList<string> GetAliases(ISymbol item) =>
+        protected override IReadOnlyCollection<string> GetAliases(ISymbol item) =>
             item.Aliases;
 
-        protected override IReadOnlyList<string> GetRawAliases(ISymbol item) => item.RawAliases;
+        protected override IReadOnlyCollection<string> GetRawAliases(ISymbol item) => item.RawAliases;
     }
 }
diff --git a/src/System.CommandLine/ISymbol.cs b/src/System.CommandLine/ISymbol.cs
@@ -13,9 +13,9 @@ public interface ISymbol : ISuggestionSource
 
         string? Description { get; }
 
-        IReadOnlyList<string> Aliases { get; }
+        IReadOnlyCollection<string> Aliases { get; }
 
-        IReadOnlyList<string> RawAliases { get; }
+        IReadOnlyCollection<string> RawAliases { get; }
 
         bool HasAlias(string alias);
 
diff --git a/src/System.CommandLine/Option.cs b/src/System.CommandLine/Option.cs
@@ -56,7 +56,7 @@ public virtual Argument Argument
 
         private protected override void ChooseNameForUnnamedArgument(Argument argument)
         {
-            argument.Name = Aliases[0].ToLower();
+            argument.Name = Name.ToLower();
         }
     }
 }
diff --git a/src/System.CommandLine/Parsing/ArgumentResult.cs b/src/System.CommandLine/Parsing/ArgumentResult.cs
@@ -51,6 +51,7 @@ internal virtual ArgumentConversionResult Convert(
             var parentResult = Parent;
 
             if (ShouldCheckArity() &&
+                parentResult is {} &&
                 ArgumentArity.Validate(parentResult,
                                        argument,
                                        argument.Arity.MinimumNumberOfValues,
diff --git a/src/System.CommandLine/Parsing/CommandResult.cs b/src/System.CommandLine/Parsing/CommandResult.cs
@@ -35,8 +35,6 @@ internal CommandResult(
 
         public Token Token { get; }
 
-        internal virtual RootCommandResult? Root => (Parent as CommandResult)?.Root;
-
         internal bool TryGetValueForArgument(
             IValueDescriptor valueDescriptor,
             out object? value)
diff --git a/src/System.CommandLine/Parsing/StringExtensions.cs b/src/System.CommandLine/Parsing/StringExtensions.cs
@@ -566,7 +566,8 @@ private static Dictionary<string, Token> ValidTokens(this ICommand command)
 
             for (var commandAliasIndex = 0; commandAliasIndex < command.RawAliases.Count; commandAliasIndex++)
             {
-                var commandAlias = command.RawAliases[commandAliasIndex];
+                var commandAlias = command.RawAliases.ToArray()[commandAliasIndex];
+                
                 tokens.Add(
                     commandAlias,
                     new Token(
@@ -579,7 +580,7 @@ private static Dictionary<string, Token> ValidTokens(this ICommand command)
 
                     for (var childAliasIndex = 0; childAliasIndex < child.RawAliases.Count; childAliasIndex++)
                     {
-                        var childAlias = child.RawAliases[childAliasIndex];
+                        var childAlias = child.RawAliases.ToArray()[childAliasIndex];
 
                         tokens[childAlias] =
                             new Token(
diff --git a/src/System.CommandLine/Parsing/SymbolResult.cs b/src/System.CommandLine/Parsing/SymbolResult.cs
@@ -27,6 +27,8 @@ private protected SymbolResult(
 
         public SymbolResult? Parent { get; }
 
+        internal virtual RootCommandResult? Root => (Parent as CommandResult)?.Root;
+
         public ISymbol Symbol { get; }
 
         public IReadOnlyList<Token> Tokens => _tokens;
diff --git a/src/System.CommandLine/Parsing/SymbolResultSet.cs b/src/System.CommandLine/Parsing/SymbolResultSet.cs
@@ -12,10 +12,10 @@ public class SymbolResultSet : AliasedSet<SymbolResult>
         internal SymbolResult? ResultFor(ISymbol symbol) =>
             Items.SingleOrDefault(i => i.Symbol == symbol);
 
-        protected override IReadOnlyList<string> GetAliases(SymbolResult item) =>
+        protected override IReadOnlyCollection<string> GetAliases(SymbolResult item) =>
             item.Symbol.Aliases;
 
-        protected override IReadOnlyList<string> GetRawAliases(SymbolResult item) =>
+        protected override IReadOnlyCollection<string> GetRawAliases(SymbolResult item) =>
             item.Symbol.RawAliases;
     }
-}
+}
diff --git a/src/System.CommandLine/Symbol.cs b/src/System.CommandLine/Symbol.cs
@@ -11,8 +11,8 @@ namespace System.CommandLine
 {
     public abstract class Symbol : ISymbol
     {
-        private readonly List<string> _aliases = new List<string>();
-        private readonly List<string> _rawAliases = new List<string>();
+        private readonly HashSet<string> _aliases = new HashSet<string>();
+        private readonly HashSet<string> _rawAliases = new HashSet<string>();
         private string _longestAlias = "";
         private string? _specifiedName;
 
@@ -44,9 +44,9 @@ protected Symbol(
             Description = description;
         }
 
-        public IReadOnlyList<string> Aliases => _aliases;
+        public IReadOnlyCollection<string> Aliases => _aliases;
 
-        public IReadOnlyList<string> RawAliases => _rawAliases;
+        public IReadOnlyCollection<string> RawAliases => _rawAliases;
 
         public string? Description { get; set; }
 
@@ -132,7 +132,7 @@ public bool HasAlias(string alias)
 
             return _aliases.Contains(alias.RemovePrefix());
         }
-
+  
         public bool HasRawAlias(string alias) => _rawAliases.Contains(alias);
 
         public bool IsHidden { get; set; }

Original file line number	Diff line number	Diff line change
`@@ -8,12 +8,12 @@ namespace System.CommandLine.Binding`
`8`	`8`	`{`
`9`	`9`	`internal class ArgumentConversionResultSet : AliasedSet<ArgumentConversionResult>`
`10`	`10`	`{`
`11`		`- protected override IReadOnlyList<string> GetAliases(ArgumentConversionResult item)`
	`11`	`+ protected override IReadOnlyCollection<string> GetAliases(ArgumentConversionResult item)`
`12`	`12`	`{`
`13`	`13`	`return new[] { item.Argument.Name };`
`14`	`14`	`}`
`15`	`15`
`16`		`- protected override IReadOnlyList<string> GetRawAliases(ArgumentConversionResult item)`
	`16`	`+ protected override IReadOnlyCollection<string> GetRawAliases(ArgumentConversionResult item)`
`17`	`17`	`{`
`18`	`18`	`return new[] { item.Argument.Name };`
`19`	`19`	`}`
Original file line number	Diff line number	Diff line change
`@@ -25,7 +25,7 @@ private static string FormatErrorMessage(`
`25`	`25`	`{`
`26`	`26`	`// TODO: (FailedArgumentTypeConversionResult) localize`
`27`	`27`
`28`		`- var firstParent = a.Parents.First();`
	`28`	`+ var firstParent = a.Parents[0];`
`29`	`29`
`30`	`30`	`var symbolType =`
`31`	`31`	`firstParent switch {`
`@@ -34,7 +34,7 @@ private static string FormatErrorMessage(`
`34`	`34`	`_ => null`
`35`	`35`	`};`
`36`	`36`
`37`		`- var alias = firstParent.RawAliases[0];`
	`37`	`+ var alias = firstParent.RawAliases.First();`
`38`	`38`
`39`	`39`	`return $"Cannot parse argument '{value}' for {symbolType} '{alias}' as expected type {type}.";`
`40`	`40`	`}`
Original file line number	Diff line number	Diff line change
`@@ -56,7 +56,7 @@ public virtual Argument Argument`
`56`	`56`
`57`	`57`	`private protected override void ChooseNameForUnnamedArgument(Argument argument)`
`58`	`58`	`{`
`59`		`- argument.Name = Aliases[0].ToLower();`
	`59`	`+ argument.Name = Name.ToLower();`
`60`	`60`	`}`
`61`	`61`	`}`
`62`	`62`	`}`