Name based lookup fixes #2255

jonsequitur · 2023-08-01T00:12:07Z

These changes add support for SymbolResult.GetResult(string name) to bring it into parity with GetValue, which already had both symbol- and name-based lookups. This will allow custom parse delegates to look up other symbols without having to have a reference to the symbol object.

This also fixes a bug where name-based lookups could not look up values for options or arguments on a parent command of the one invoked.

There are also some cleanups of dead code here, and CliCommand now implements IEnumerable rather than IEnumerable<T>. This was only ever done to support C# collection initializer syntax and this change will keep LINQ extension methods out of the IntelliSense list.

adamsitnik

I wrote that in the comments, but since it's important I am going to repeat it here: adding any method to SymbolResult is tricky, as the type can be used before the parsing is finished: inside a validator and/or a custom parser. And the result is dependent on the order of provided args. We can also fall into a trap of never-ending recursion (two custom parsers, each trying to get the other's value during parsing).

That is why I've added it only to ParseResult, which makes it very predictable and performant. And I did that based on clear user requirements: "I want to get a value once parsing is finished".
I understand that the current implementation makes it possible to provide the values by name only for the parsed command, but I also did that on purpose as I wanted to avoid adding a complexity before anybody asks for it. If we have such a need and it makes sense, we can change the behavior to include parents in the search, but in my opinion we should avoid doing that proactively (I don't see any issues being referenced here), as again we might add complexity that will be expensive to maintain.

adamsitnik · 2023-08-01T13:22:12Z

...ility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt

@@ -35,7 +35,7 @@ System.CommandLine
    public System.Void AcceptLegalFileNamesOnly()
    public System.Void AcceptLegalFilePathsOnly()
    public System.Void AcceptOnlyFromAmong(System.String[] values)
-  public class CliCommand : CliSymbol, System.Collections.Generic.IEnumerable<CliSymbol>, System.Collections.IEnumerable
+  public class CliCommand : CliSymbol, System.Collections.IEnumerable


nice, the fewer interfaces we implement, the better

BTW in the future we might want to use https://github.com/dotnet/csharplang/blob/main/proposals/collection-expressions.md#create-methods (sample usage: dotnet/runtime#88470)

adamsitnik · 2023-08-01T13:24:58Z

...ility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt

    public T GetValue<T>(CliArgument<T> argument)
    public T GetValue<T>(CliOption<T> option)
+    public T GetValue<T>(System.String name)


I've mixed feelings about adding this method to SymbolResult, as the type can be used before the parsing is finished. Example: inside a validator or a custom parser. Moreover, depending on the order of provided args, the behavior can be different.

Example:

--option1 foo --option2 bar --option2 bar --option1 foo

If a custom parser provided for --option1 tries to get value of --option2 then in the first scenario it will get null/exception and value in the second.

It's not clear to me what we gain from exposing these methods, while it's clear that they are going to increase the complexity and cause trouble in the future. The PR is not linked to any issue, so I've no idea whether any customers asked for it or whether we believe that it might be useful for some scenarios in the future.

That scenario is the reason this is a draft PR. I agree about the bug and I plan to address it before considering this PR ready.

SymbolResult.GetResult(string name) is something that we've discussed adding a number of times in our syncs and for the sake of code simplicity and consistency, I believe this should be the core of the GetValue(string name) implementation. The motivation is the same as for GetValue(string name). Without this API, the only way to call GetResult in a custom parser is by having a reference to the symbol, which is awkward for the same reasons as it was for GetValue(CliSymbol symbol). The customer complaints about that awkwardness apply equally to GetResult, though GetResult is a less-used API. But in a custom parser, GetResult is more important than GetValue. (And note that I haven't added an equivalent method to ParseResult for this reason.)

it's clear that they are going to increase the complexity and cause trouble in the future

I'm not sure this is the case. Aside from the name-based lookup itself, the binding code here all uses the pre-existing code path, which has been stable for some time.

adamsitnik · 2023-08-01T13:35:37Z

src/System.CommandLine.Tests/GetValueByNameParserTests.cs

@@ -275,4 +308,22 @@ public void Parse_errors_have_precedence_over_type_mismatch()
            .Throw<InvalidOperationException>()
            .Where(ex => ex.Message == LocalizationResources.RequiredOptionWasNotProvided("--required"));
    }
+
+    [Fact]
+    public void Recursive_option_on_parent_command_can_be_looked_up_when_subcommand_is_specified()


What is the result of getting a value by name, if there are two commands in the same parse tree that define a symbol with the same name, but it's mandatory for one and optional for another?

CliRootCommand cmd = new() { new CliOption<string>("--opt") { Required = false }, new CliCommand("subcommand") { new CliOption<string>("--opt") { Required = true } } }; ParseResult result = cmd.Parse("--opt some subcommand"); result.GetValue<string>("--opt").Should.What(??)

That would be a parse error if there's no default value for the inner --opt. But if the inner --opt has a default value, then the name-based lookup would return its value, while the outer --opt would be shadowed when using the name-based API.

This is one of the edge cases around name-based lookups that I don't think can ever be resolved and was why in its previous incarnation, the name-based lookups returned e.g. IEnumerable<SymbolResult>, which was technically correct but hard to understand. The current compromise is that for the sake of the name-based lookup, the inner shadows the outer. And if you need an unambiguous way to check the values for both of them, you have to use the GetValue(CliSymbol symbol)

adamsitnik · 2023-08-01T13:37:19Z

src/System.CommandLine.Tests/CustomParsingTests.cs

+
+namespace System.CommandLine.Tests;
+
+public class CustomParsingTests


it would be easier to review this PR if you have separated the test refactor from the actual change. In case of CustomParsingTests I simply trust that you just moved the logic to new type without any modifications.

That's correct.

The cleanups and this are in separate commits to make reviewing simpler. I moved this out to a separate class because not all of the custom parsing tests are argument tests, and because I'll be adding more tests here to cover using name-based lookups within custom parsers.

adamsitnik · 2023-08-01T13:41:45Z

src/System.CommandLine/CliOption.cs

-        /// <summary>
-        /// Gets or sets the <see cref="Type" /> that the option's parsed tokens will be converted to.
-        /// </summary>
-        public abstract Type ValueType { get; }


Are there no valid scenarios for this API? Are you sure it should be removed?

If so, they're not represented by any tests. I think it was vestigial and only needed by reflection-based implementations that have been removed.

@KathleenDollard is this property something you've needed for source generator work?

adamsitnik · 2023-08-01T13:43:44Z

src/System.CommandLine/Parsing/OptionResult.cs

        public T GetValueOrDefault<T>() =>
-            ArgumentConversionResult.ConvertIfNeeded(typeof(T))
+            ArgumentConversionResult


should the method return T??

jonsequitur · 2023-08-01T20:18:19Z

I understand that the current implementation makes it possible to provide the values by name only for the parsed command, but I also did that on purpose as I wanted to avoid adding a complexity before anybody asks for it.

I've already had a couple of people ask why the name-based lookup wasn't working when the actual issue was that it didn't do parent lookups. I think that behavior was unintuitive, especially as it's probably the 80% case for recursive options.

jonsequitur · 2023-08-01T20:30:11Z

And the result is dependent on the order of provided args. We can also fall into a trap of never-ending recursion (two custom parsers, each trying to get the other's value during parsing).

This was already supported and doesn't result in endless recursion.

Does this passing test give you any reassurance? Can you think of additional tests?

(I'm also adding option tests, not all of which pass yet.)

Edit: This also works:

fix parent lookups for GetValue(string), add GetResult(string)

ef70c29

jonsequitur requested review from Keboo and adamsitnik August 1, 2023 00:12

remove dead code and other cleanups

0f0311c

jonsequitur force-pushed the name-based-lookup-fixes branch from 7826a04 to 0f0311c Compare August 1, 2023 00:17

jonsequitur added 2 commits July 31, 2023 17:18

split out CustomParsingTests

6632464

make CliCommand IEnumerable, not IEnumerable<T>

fdfce0b

adamsitnik requested changes Aug 1, 2023

View reviewed changes

remove EditorBrowsable(Never) from CliCommand.Add overloads

73f144f

add custom parsing tests to cover SymbolResult.GetResult by name

50a23f6

jonsequitur marked this pull request as ready for review August 7, 2023 15:44

jonsequitur merged commit a045dd5 into dotnet:main Aug 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Name based lookup fixes #2255

Name based lookup fixes #2255

jonsequitur commented Aug 1, 2023 •

edited

Loading

adamsitnik left a comment

adamsitnik Aug 1, 2023

adamsitnik Aug 1, 2023

jonsequitur Aug 1, 2023

adamsitnik Aug 1, 2023

jonsequitur Aug 1, 2023

adamsitnik Aug 1, 2023

jonsequitur Aug 1, 2023

adamsitnik Aug 1, 2023

jonsequitur Aug 1, 2023

adamsitnik Aug 1, 2023

jonsequitur commented Aug 1, 2023

jonsequitur commented Aug 1, 2023 •

edited

Loading


		namespace System.CommandLine.Tests;

		public class CustomParsingTests

Name based lookup fixes #2255

Name based lookup fixes #2255

Conversation

jonsequitur commented Aug 1, 2023 • edited Loading

adamsitnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonsequitur commented Aug 1, 2023

jonsequitur commented Aug 1, 2023 • edited Loading

jonsequitur commented Aug 1, 2023 •

edited

Loading

jonsequitur commented Aug 1, 2023 •

edited

Loading