From 31dfad1b20ee289c9fd517b2672389d6c4a79f2d Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 8 Nov 2022 20:11:13 +0100 Subject: [PATCH 1/4] introduce static CompletionContext.Empty: * cache it so it does not get created more than once * make it public so it can be reused by our users who write tests * use it places where it can improve performance (including benchmarks, which should not include this allocation) --- ...sts.System_CommandLine_api_is_not_changed.approved.txt | 1 + .../CommandLine/Perf_Suggestions.cs | 3 ++- src/System.CommandLine/Completions/CompletionContext.cs | 8 +++++++- src/System.CommandLine/Help/HelpBuilder.Default.cs | 2 +- src/System.CommandLine/Parsing/ParseResultVisitor.cs | 3 ++- 5 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt index 73a3e868cc..0eadcc6642 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt @@ -275,6 +275,7 @@ System.CommandLine.Binding public System.Boolean TryGetValue(IValueDescriptor valueDescriptor, BindingContext bindingContext, ref System.Object& boundValue) System.CommandLine.Completions public abstract class CompletionContext + public static CompletionContext Empty { get; } public System.CommandLine.ParseResult ParseResult { get; } public System.String WordToComplete { get; } public class CompletionItem diff --git a/src/System.CommandLine.Benchmarks/CommandLine/Perf_Suggestions.cs b/src/System.CommandLine.Benchmarks/CommandLine/Perf_Suggestions.cs index 2ea50046c7..ab680141a2 100644 --- a/src/System.CommandLine.Benchmarks/CommandLine/Perf_Suggestions.cs +++ b/src/System.CommandLine.Benchmarks/CommandLine/Perf_Suggestions.cs @@ -2,6 +2,7 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System.Collections.Generic; +using System.CommandLine.Completions; using System.Linq; using BenchmarkDotNet.Attributes; using BenchmarkDotNet.Engines; @@ -43,7 +44,7 @@ public void Setup_FromSymbol() [Benchmark] public void SuggestionsFromSymbol() { - _testSymbol.GetCompletions().Consume(new Consumer()); + _testSymbol.GetCompletions(CompletionContext.Empty).Consume(new Consumer()); } [GlobalSetup(Target = nameof(SuggestionsFromParseResult))] diff --git a/src/System.CommandLine/Completions/CompletionContext.cs b/src/System.CommandLine/Completions/CompletionContext.cs index 720dc33984..bbbca5713d 100644 --- a/src/System.CommandLine/Completions/CompletionContext.cs +++ b/src/System.CommandLine/Completions/CompletionContext.cs @@ -11,6 +11,8 @@ namespace System.CommandLine.Completions /// public abstract class CompletionContext { + private static CompletionContext _empty; + internal CompletionContext(ParseResult parseResult, string wordToComplete) { ParseResult = parseResult; @@ -23,7 +25,11 @@ internal CompletionContext(ParseResult parseResult, string wordToComplete) /// The parse result for which completions are being requested. public ParseResult ParseResult { get; } - internal static CompletionContext Empty() => new TokenCompletionContext(ParseResult.Empty()); + /// + /// Gets an empty CompletionContext. + /// + /// Can be used for testing purposes. + public static CompletionContext Empty => _empty ??= new TokenCompletionContext(ParseResult.Empty()); /// /// Gets the text to be matched for completion, which can be used to filter a list of completions. diff --git a/src/System.CommandLine/Help/HelpBuilder.Default.cs b/src/System.CommandLine/Help/HelpBuilder.Default.cs index 83f5ba4564..0ab8aebe9e 100644 --- a/src/System.CommandLine/Help/HelpBuilder.Default.cs +++ b/src/System.CommandLine/Help/HelpBuilder.Default.cs @@ -65,7 +65,7 @@ public static string GetArgumentUsageLabel(Argument argument) string firstColumn; var completions = (argument is { } a - ? a.GetCompletions() + ? a.GetCompletions(CompletionContext.Empty) : Array.Empty()) .Select(item => item.Label) .ToArray(); diff --git a/src/System.CommandLine/Parsing/ParseResultVisitor.cs b/src/System.CommandLine/Parsing/ParseResultVisitor.cs index 97834fe642..31edfb3db5 100644 --- a/src/System.CommandLine/Parsing/ParseResultVisitor.cs +++ b/src/System.CommandLine/Parsing/ParseResultVisitor.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.CommandLine.Binding; +using System.CommandLine.Completions; using System.CommandLine.Help; using System.Linq; @@ -522,7 +523,7 @@ private void ValidateAndConvertArgumentResult(ArgumentResult argumentResult) { if (argument.FirstParent?.Symbol is Option option) { - var completions = option.GetCompletions().ToArray(); + var completions = option.GetCompletions(CompletionContext.Empty).ToArray(); if (completions.Length > 0) { From 3b25f1a24d1999bb6bf16ec55a0833e7c16c9c1d Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 8 Nov 2022 20:16:53 +0100 Subject: [PATCH 2/4] remove Symbol.GetCompletions(void) --- ...ommandLine_api_is_not_changed.approved.txt | 1 - .../CompletionTests.cs | 21 ++++++++++--------- src/System.CommandLine/Symbol.cs | 4 ---- 3 files changed, 11 insertions(+), 15 deletions(-) diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt index 0eadcc6642..d25d2c3551 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt @@ -248,7 +248,6 @@ System.CommandLine public System.Boolean IsHidden { get; set; } public System.String Name { get; set; } public System.Collections.Generic.IEnumerable Parents { get; } - public System.Collections.Generic.IEnumerable GetCompletions() public System.Collections.Generic.IEnumerable GetCompletions(System.CommandLine.Completions.CompletionContext context) public System.String ToString() System.CommandLine.Binding diff --git a/src/System.CommandLine.Tests/CompletionTests.cs b/src/System.CommandLine.Tests/CompletionTests.cs index 67e59ea177..c2d1f5abd5 100644 --- a/src/System.CommandLine.Tests/CompletionTests.cs +++ b/src/System.CommandLine.Tests/CompletionTests.cs @@ -1,6 +1,7 @@ // Copyright (c) .NET Foundation and contributors. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +using System.CommandLine.Completions; using System.CommandLine.Parsing; using System.CommandLine.Tests.Utility; using System.IO; @@ -27,7 +28,7 @@ public void Option_GetCompletions_returns_argument_completions_if_configured() var option = new Option("--hello") .AddCompletions("one", "two", "three"); - var completions = option.GetCompletions(); + var completions = option.GetCompletions(CompletionContext.Empty); completions .Select(item => item.Label) @@ -45,7 +46,7 @@ public void Command_GetCompletions_returns_available_option_aliases() new Option("--three", "option three") }; - var completions = command.GetCompletions(); + var completions = command.GetCompletions(CompletionContext.Empty); completions .Select(item => item.Label) @@ -69,7 +70,7 @@ public void Command_GetCompletions_returns_available_option_aliases_for_global_o rootCommand.AddGlobalOption(new Option("--three", "option three")); - var completions = subcommand.GetCompletions(); + var completions = subcommand.GetCompletions(CompletionContext.Empty); completions .Select(item => item.Label) @@ -87,7 +88,7 @@ public void Command_GetCompletions_returns_available_subcommands() new Command("three") }; - var completions = command.GetCompletions(); + var completions = command.GetCompletions(CompletionContext.Empty); completions .Select(item => item.Label) @@ -104,7 +105,7 @@ public void Command_GetCompletions_returns_available_subcommands_and_option_alia new Option("--option") }; - var completions = command.GetCompletions(); + var completions = command.GetCompletions(CompletionContext.Empty); completions.Select(item => item.Label) .Should() @@ -125,7 +126,7 @@ public void Command_GetCompletions_returns_available_subcommands_and_option_alia } }; - var completions = command.GetCompletions(); + var completions = command.GetCompletions(CompletionContext.Empty); completions.Select(item => item.Label) .Should() @@ -142,7 +143,7 @@ public void Command_GetCompletions_without_text_to_match_orders_alphabetically() new Command("andmyothersubcommand"), }; - var completions = command.GetCompletions(); + var completions = command.GetCompletions(CompletionContext.Empty); completions .Select(item => item.Label) @@ -158,7 +159,7 @@ public void Command_GetCompletions_does_not_return_argument_names() new Argument("the-argument") }; - var completions = command.GetCompletions(); + var completions = command.GetCompletions(CompletionContext.Empty); completions .Select(item => item.Label) @@ -921,7 +922,7 @@ public void Completions_for_options_provide_a_description() var description = "The option before -y."; var option = new Option("-x", description); - var completions = new RootCommand { option }.GetCompletions(); + var completions = new RootCommand { option }.GetCompletions(CompletionContext.Empty); completions.Should().ContainSingle() .Which @@ -936,7 +937,7 @@ public void Completions_for_subcommands_provide_a_description() var description = "The description for the subcommand"; var subcommand = new Command("-x", description); - var completions = new RootCommand { subcommand }.GetCompletions(); + var completions = new RootCommand { subcommand }.GetCompletions(CompletionContext.Empty); completions.Should().ContainSingle() .Which diff --git a/src/System.CommandLine/Symbol.cs b/src/System.CommandLine/Symbol.cs index 5e4ccd085e..4ee535b15f 100644 --- a/src/System.CommandLine/Symbol.cs +++ b/src/System.CommandLine/Symbol.cs @@ -80,10 +80,6 @@ public IEnumerable Parents /// /// Gets completions for the symbol. /// - public IEnumerable GetCompletions() => - GetCompletions(CompletionContext.Empty()); - - /// public abstract IEnumerable GetCompletions(CompletionContext context); /// From 5f12a6f5d44ad7f20f14586583a78ef42fbc9903 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 8 Nov 2022 20:27:15 +0100 Subject: [PATCH 3/4] fix the warning --- src/System.CommandLine/Completions/CompletionContext.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.CommandLine/Completions/CompletionContext.cs b/src/System.CommandLine/Completions/CompletionContext.cs index bbbca5713d..0f8f928e9b 100644 --- a/src/System.CommandLine/Completions/CompletionContext.cs +++ b/src/System.CommandLine/Completions/CompletionContext.cs @@ -11,7 +11,7 @@ namespace System.CommandLine.Completions /// public abstract class CompletionContext { - private static CompletionContext _empty; + private static CompletionContext? _empty; internal CompletionContext(ParseResult parseResult, string wordToComplete) { From 2eeb0d18e31e5cf21d452502d4c4ae463a8c11e9 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 8 Nov 2022 20:27:33 +0100 Subject: [PATCH 4/4] argument is never null here? --- src/System.CommandLine/Help/HelpBuilder.Default.cs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/System.CommandLine/Help/HelpBuilder.Default.cs b/src/System.CommandLine/Help/HelpBuilder.Default.cs index 0ab8aebe9e..caf184506b 100644 --- a/src/System.CommandLine/Help/HelpBuilder.Default.cs +++ b/src/System.CommandLine/Help/HelpBuilder.Default.cs @@ -64,11 +64,10 @@ public static string GetArgumentUsageLabel(Argument argument) } string firstColumn; - var completions = (argument is { } a - ? a.GetCompletions(CompletionContext.Empty) - : Array.Empty()) - .Select(item => item.Label) - .ToArray(); + var completions = argument + .GetCompletions(CompletionContext.Empty) + .Select(item => item.Label) + .ToArray(); var arg = argument; var helpName = arg?.HelpName ?? string.Empty;