Skip to content

Commit ffca21e

Browse files
KathleenDollardKathleenDollard
committed
Replacing SymbolByName and changes to GetValue
Design previously had SymbolByName and GetValue on SymbolResultTree which is a parse time only concern. These were moved to ParseResult for accessibility outside parsing.
1 parent 5398b09 commit ffca21e

File tree

9 files changed

+410
-244
lines changed

9 files changed

+410
-244
lines changed

‎src/System.CommandLine.Subsystems.Tests/ValueSubsystemTests.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ public void Value_is_always_activated()
3030
isActive.Should().BeTrue();
3131
}
3232

33-
[Fact(Skip ="WIP")]
33+
[Fact(Skip = "WIP")]
3434
public void ValueSubsystem_returns_values_that_are_entered()
3535
{
3636
var consoleHack = new ConsoleHack().RedirectToBuffer(true);

‎src/System.CommandLine.Tests/ParserTests.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1501,7 +1501,7 @@ public void Command_argument_arity_can_be_a_range_with_a_lower_bound_greater_tha
15011501
new CliToken("5", CliTokenType.Argument, argument, dummyLocation));
15021502
}
15031503

1504-
[Fact]
1504+
[Fact(Skip ="Waiting for CliError work")]
15051505
public void When_command_arguments_are_fewer_than_minimum_arity_then_an_error_is_returned()
15061506
{
15071507
var command = new CliCommand("the-command")
@@ -1590,7 +1590,7 @@ public void Option_argument_arity_can_be_a_range_with_a_lower_bound_greater_than
15901590
new CliToken("5", CliTokenType.Argument, default, dummyLocation));
15911591
}
15921592

1593-
[Fact]
1593+
[Fact(Skip = "Waiting for CliError work")]
15941594
public void When_option_arguments_are_fewer_than_minimum_arity_then_an_error_is_returned()
15951595
{
15961596
var option = new CliOption<int[]>("-x")

‎src/System.CommandLine/CliCommand.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ public IEnumerable<CliSymbol> Children
7575
/// <summary>
7676
/// Represents all of the options for the command, inherited options that have been applied to any of the command's ancestors.
7777
/// </summary>
78+
// TODO: Consider value of lazy here. It sets up a desire to use awkward approach (HasOptions) for a perf win. Applies to Options and Subcommands also.
7879
public IList<CliOption> Options => _options ??= new (this);
7980

8081
internal bool HasOptions => _options?.Count > 0;

‎src/System.CommandLine/ParseResult.cs

Lines changed: 221 additions & 208 deletions
Large diffs are not rendered by default.

‎src/System.CommandLine/Parsing/ParseOperation.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ internal ParseResult Parse()
8585
_configuration,
8686
_rootCommandResult,
8787
_innermostCommandResult,
88-
_rootCommandResult.SymbolResultTree.GetValueResultDictionary(),
88+
_rootCommandResult.SymbolResultTree,
8989
/*
9090
_tokens,
9191
*/

‎src/System.CommandLine/Parsing/SymbolLookupByName.cs

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
// Copyright (c) .NET Foundation and contributors. All rights reserved.
2+
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
3+
4+
using System.Collections.Generic;
5+
using System.ComponentModel;
6+
using System.Diagnostics.CodeAnalysis;
7+
using System.Linq;
8+
9+
namespace System.CommandLine.Parsing;
10+
11+
// Performance note: Special cases might result in the previous single dictionary being faster, but it did not
12+
// give correct results for many CLIs, and also, it built a dictionary for the full CLI tree, rather than just the
13+
// current commands and its ancestors, so in many cases, this will be faster.
14+
//
15+
// Most importantly, that approach fails for options, like the previous global options, that appear on multiple
16+
// commands, since we are now explicitly putting them on all commands.
17+
18+
public class SymbolLookupByName
19+
{
20+
private class CommandCache(CliCommand command)
21+
{
22+
public CliCommand Command { get; } = command;
23+
public Dictionary<string, CliSymbol> SymbolsByName { get; } = new();
24+
}
25+
26+
private List<CommandCache> cache;
27+
28+
public SymbolLookupByName(ParseResult parseResult)
29+
=> cache = BuildCache(parseResult);
30+
31+
private List<CommandCache> BuildCache(ParseResult parseResult)
32+
{
33+
if (cache is not null)
34+
{
35+
return cache;
36+
}
37+
cache = [];
38+
var commandResult = parseResult.CommandResult;
39+
while (commandResult is not null)
40+
{
41+
var command = commandResult.Command;
42+
if (TryGetCommandCache(command, out var _))
43+
{
44+
throw new InvalidOperationException("Command hierarchy appears to be recursive.");
45+
}
46+
var commandCache = new CommandCache(command);
47+
cache.Add(commandCache);
48+
49+
AddSymbolsToCache(commandCache, command.Options, command);
50+
AddSymbolsToCache(commandCache, command.Arguments, command);
51+
AddSymbolsToCache(commandCache, command.Subcommands, command);
52+
commandResult = (CommandResult?)commandResult.Parent;
53+
}
54+
55+
return cache;
56+
57+
static void AddSymbolsToCache(CommandCache CommandCache, IEnumerable<CliSymbol> symbols, CliCommand command)
58+
{
59+
foreach (var symbol in symbols)
60+
{
61+
if (CommandCache.SymbolsByName.ContainsKey(symbol.Name))
62+
{
63+
throw new InvalidOperationException($"Command {command.Name} has more than one child named \"{symbol.Name}\".");
64+
}
65+
CommandCache.SymbolsByName.Add(symbol.Name, symbol);
66+
}
67+
}
68+
}
69+
70+
private bool TryGetCommandCache(CliCommand command, [NotNullWhen(true)] out CommandCache? commandCache)
71+
{
72+
var candidates = cache.Where(x => x.Command == command);
73+
if (candidates.Any())
74+
{
75+
commandCache = candidates.Single(); // multiples are a failure in construction
76+
return true;
77+
}
78+
commandCache = null;
79+
return false;
80+
}
81+
82+
private bool TryGetSymbolAndParentInternal(string name,
83+
[NotNullWhen(true)] out CliSymbol? symbol,
84+
[NotNullWhen(true)] out CliCommand? parent,
85+
[NotNullWhen(false)] out string? errorMessage,
86+
CliCommand? startCommand,
87+
bool skipAncestors,
88+
bool valuesOnly)
89+
{
90+
startCommand ??= cache.First().Command; // The construction of the dictionary makes this the parseResult.CommandResult - current command
91+
var commandCaches = GetCommandCachesToUse(startCommand);
92+
if (commandCaches is null || !commandCaches.Any())
93+
{
94+
errorMessage = $"Requested command {startCommand.Name} is not in the results.";
95+
symbol = null;
96+
parent = null;
97+
return false;
98+
}
99+
100+
foreach (var commandCache in commandCaches)
101+
{
102+
if (commandCache.SymbolsByName.TryGetValue(name, out symbol))
103+
{
104+
if (symbol is not null && (!valuesOnly || (symbol is CliArgument or CliOption)))
105+
{
106+
parent = commandCache.Command;
107+
errorMessage = null;
108+
return true;
109+
}
110+
}
111+
112+
if (skipAncestors)
113+
{
114+
break;
115+
}
116+
}
117+
118+
errorMessage = $"Requested symbol {name} was not found.";
119+
symbol = null;
120+
parent = null;
121+
return false;
122+
}
123+
124+
public (CliSymbol symbol, CliCommand parent) GetSymbolAndParent(string name, CliCommand? startCommand = null, bool skipAncestors = false, bool valuesOnly = false)
125+
=> TryGetSymbolAndParentInternal(name, out var symbol, out var parent, out var errorMessage, startCommand, skipAncestors, valuesOnly)
126+
? (symbol, parent)
127+
: throw new InvalidOperationException(errorMessage);
128+
129+
public bool TryGetSymbol(string name, out CliSymbol symbol, CliCommand? startCommand = null, bool skipAncestors = false, bool valuesOnly = false)
130+
=> TryGetSymbolAndParentInternal(name, out symbol, out var _, out var _, startCommand, skipAncestors, valuesOnly);
131+
132+
133+
private IEnumerable<CommandCache>? GetCommandCachesToUse(CliCommand currentCommand)
134+
{
135+
if (cache[0].Command == currentCommand)
136+
{
137+
return cache;
138+
}
139+
for (int i = 1; i < cache.Count; i++) // we tested for 0 earlier
140+
{
141+
if (cache[i].Command == currentCommand)
142+
{
143+
return cache.Skip(i);
144+
}
145+
}
146+
return null;
147+
}
148+
}

‎src/System.CommandLine/Parsing/SymbolResult.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ public IEnumerable<ParseError> Errors
9797
/// <returns>A directive result if the directive was matched by the parser, <c>null</c> otherwise.</returns>
9898
public DirectiveResult? GetResult(CliDirective directive) => SymbolResultTree.GetResult(directive);
9999
*/
100+
/* No longer used
100101
/// <summary>
101102
/// Finds a result for a symbol having the specified name anywhere in the parse tree.
102103
/// </summary>
@@ -105,7 +106,6 @@ public IEnumerable<ParseError> Errors
105106
public SymbolResult? GetResult(string name) =>
106107
SymbolResultTree.GetResult(name);
107108
108-
/* Not used
109109
/// <inheritdoc cref="ParseResult.GetValue{T}(CliArgument{T})"/>
110110
public T? GetValue<T>(CliArgument<T> argument)
111111
{
@@ -129,7 +129,6 @@ public IEnumerable<ParseError> Errors
129129
130130
return CliArgument<T>.CreateDefaultValue();
131131
}
132-
*/
133132
134133
/// <summary>
135134
/// Gets the value for a symbol having the specified name anywhere in the parse tree.
@@ -155,6 +154,7 @@ public IEnumerable<ParseError> Errors
155154
156155
return CliArgument<T>.CreateDefaultValue();
157156
}
157+
*/
158158

159159
internal virtual bool UseDefaultValueFor(ArgumentResult argumentResult) => false;
160160
}

‎src/System.CommandLine/Parsing/SymbolResultTree.cs

Lines changed: 33 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,15 @@ internal sealed class SymbolResultTree : Dictionary<CliSymbol, SymbolResult>
1010
{
1111
private readonly CliCommand _rootCommand;
1212
internal List<ParseError>? Errors;
13-
// TODO: unmatched tokens
14-
/*
15-
internal List<CliToken>? UnmatchedTokens;
16-
*/
13+
// TODO: unmatched tokens
14+
/*
15+
internal List<CliToken>? UnmatchedTokens;
16+
*/
1717

1818
// TODO: Looks like this is a SymboNode/linked list because a symbol may appear multiple
1919
// places in the tree and multiple symbols will have the same short name. The question is
2020
// whether creating the multiple node instances is faster than just using lists. Could well be.
21-
private Dictionary<string, SymbolNode>? _symbolsByName;
21+
//private Dictionary<string, SymbolNode>? _symbolsByName;
2222
internal SymbolResultTree(
2323
CliCommand rootCommand,
2424
List<string>? tokenizeErrors)
@@ -35,6 +35,7 @@ internal SymbolResultTree(
3535
}
3636
}
3737
}
38+
3839
internal int ErrorCount => Errors?.Count ?? 0;
3940

4041
internal ArgumentResult? GetResult(CliArgument argument)
@@ -46,11 +47,11 @@ internal SymbolResultTree(
4647
internal OptionResult? GetResult(CliOption option)
4748
=> TryGetValue(option, out SymbolResult? result) ? (OptionResult)result : default;
4849

49-
//TODO: directives
50-
/*
51-
internal DirectiveResult? GetResult(CliDirective directive)
52-
=> TryGetValue(directive, out SymbolResult? result) ? (DirectiveResult)result : default;
53-
*/
50+
//TODO: directives
51+
/*
52+
internal DirectiveResult? GetResult(CliDirective directive)
53+
=> TryGetValue(directive, out SymbolResult? result) ? (DirectiveResult)result : default;
54+
*/
5455
// TODO: Determine how this is used. It appears to be O^n in the size of the tree and so if it is called multiple times, we should reconsider to avoid O^(N*M)
5556
internal IEnumerable<SymbolResult> GetChildren(SymbolResult parent)
5657
{
@@ -67,11 +68,11 @@ internal IEnumerable<SymbolResult> GetChildren(SymbolResult parent)
6768
}
6869
}
6970

70-
internal Dictionary<CliSymbol, ValueResult> GetValueResultDictionary()
71+
internal IReadOnlyDictionary<CliSymbol, ValueResult> BuildValueResultDictionary()
7172
{
7273
var dict = new Dictionary<CliSymbol, ValueResult>();
7374
foreach (KeyValuePair<CliSymbol, SymbolResult> pair in this)
74-
{
75+
{
7576
var result = pair.Value;
7677
if (result is OptionResult optionResult)
7778
{
@@ -92,22 +93,23 @@ internal Dictionary<CliSymbol, ValueResult> GetValueResultDictionary()
9293

9394
internal void AddUnmatchedToken(CliToken token, CommandResult commandResult, CommandResult rootCommandResult)
9495
{
95-
/*
96-
// TODO: unmatched tokens
97-
(UnmatchedTokens ??= new()).Add(token);
98-
99-
if (commandResult.Command.TreatUnmatchedTokensAsErrors)
100-
{
101-
if (commandResult != rootCommandResult && !rootCommandResult.Command.TreatUnmatchedTokensAsErrors)
102-
{
103-
return;
104-
}
105-
106-
*/
96+
/*
97+
// TODO: unmatched tokens
98+
(UnmatchedTokens ??= new()).Add(token);
99+
100+
if (commandResult.Command.TreatUnmatchedTokensAsErrors)
101+
{
102+
if (commandResult != rootCommandResult && !rootCommandResult.Command.TreatUnmatchedTokensAsErrors)
103+
{
104+
return;
105+
}
106+
107+
*/
107108
AddError(new ParseError(LocalizationResources.UnrecognizedCommandOrArgument(token.Value), commandResult));
108109
// }
109110
}
110111

112+
/* No longer used
111113
public SymbolResult? GetResult(string name)
112114
{
113115
if (_symbolsByName is null)
@@ -135,12 +137,12 @@ internal void AddUnmatchedToken(CliToken token, CommandResult commandResult, Com
135137
return null;
136138
}
137139
138-
// TODO: symbolsbyname - this is inefficient
139-
// results for some values may not be queried at all, dependent on other options
140-
// so we could avoid using their value factories and adding them to the dictionary
141-
// could we sort by name allowing us to do a binary search instead of allocating a dictionary?
142-
// could we add codepaths that query for specific kinds of symbols so they don't have to search all symbols?
143-
// Additional Note: Couldn't commands know their children, and thus this involves querying the active command, and possibly the parents
140+
// TODO: symbolsbyname - this is inefficient
141+
// results for some values may not be queried at all, dependent on other options
142+
// so we could avoid using their value factories and adding them to the dictionary
143+
// could we sort by name allowing us to do a binary search instead of allocating a dictionary?
144+
// could we add codepaths that query for specific kinds of symbols so they don't have to search all symbols?
145+
// Additional Note: Couldn't commands know their children, and thus this involves querying the active command, and possibly the parents
144146
private void PopulateSymbolsByName(CliCommand command)
145147
{
146148
if (command.HasArguments)
@@ -192,5 +194,6 @@ void AddToSymbolsByName(CliSymbol symbol)
192194
}
193195
}
194196
}
197+
*/
195198
}
196199
}

‎src/System.CommandLine/System.CommandLine.csproj

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
<Compile Include="ArgumentArity.cs" />
2929
<Compile Include="Binding\ArgumentConversionResult.cs" />
3030
<Compile Include="Parsing\CommandValueResult.cs" />
31+
<Compile Include="Parsing\SymbolLookupByName.cs" />
3132
<Compile Include="Parsing\ValueResultOutcome.cs" />
3233
<Compile Include="Binding\ArgumentConversionResultType.cs" />
3334
<Compile Include="Binding\ArgumentConverter.cs" />

0 commit comments

Comments
 (0)