Add ability to parse command line switches without a value

[jdmallen]

This is in response to issue #36024 where I was asked by @maryamariyan to submit a pull request based on my demonstration.

Applying this PR will:

• Modify the CommandLineConfigurationProvider to allow for individual switches to be evaluated as having the value "true", rather than be ignored altogether.
• Modify existing tests (one being commandeered entirely) to account for this change.

As an example, one can currently provide key-value pairs currently in the following formats:

.\myProgram.exe -help=true
.\myProgram.exe --help=true
.\myProgram.exe /help=true
.\myProgram.exe --help true
.\myProgram.exe help=true

Once this PR is merged, the following will also set the configuration key "help" to the value "true", rather than be ignored completely:

.\myProgram.exe -help
.\myProgram.exe --help
.\myProgram.exe /help

A potential breaking change is the necessity to bring in System.Linq in CommandLineConfigurationProvider to easily transform an IEnumerable<string> into a string[]. However, this will likely never be needed, as the only type that Args will ever be is string[], and ToArray will only be called if the safe cast to string[] fails. This has been addressed; see below.

I have tried my best to adhere to the original coding style of the files modified. Please let me know if anything needs adjusting and I will happily accommodate.

Please see the original issue comments (#36024) for more details.

[dnfadmin]

All CLA requirements met.

[ghost]

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

[maryamariyan]

Thanks @am11 and @campersau for the review and @jdmallen for your PR.

@HaoK would you consider this enhancement worth the behavior change it introduces?

[HaoK]

The change in behavior doesn't seem that unreasonable, but maybe its worth doing with a opt-in flag on the commandline source

[safern]

@HaoK do you think this is really breaking? I guess this would only apply to boolean flags, so this is just extending functionality other than breaking, right?

[HaoK]

I didn't look at the implementation, I guess if its narrow in scope only to bool switches, its not really breaking

[safern]

Yeah, that would be my expectation... to me this feature should only apply to boolean switches.

[jdmallen]

If there's anything else you'd like me to change, please let me know and I'd be happy to do so. :) I'll also understand if you pass on it. This should only change it so that switch keys lacking values are no longer ignored.

[maryamariyan]

/azp run runtime

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[maryamariyan]

@jdmallen test logs show the failure below:

/root/helix/work/workitem /root/helix/work/workitem
  Discovering: Microsoft.Extensions.Hosting.Unit.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  Microsoft.Extensions.Hosting.Unit.Tests (found 86 test cases)
  Starting:    Microsoft.Extensions.Hosting.Unit.Tests (parallel test collections = on, max threads = 2)
�[40m�[32minfo�[39m�[22m�[49m: Microsoft.Extensions.Hosting.Tests.HostTests[0]
      Request starting
    Microsoft.Extensions.Hosting.Tests.HostTests.CreateDefaultBuilder_IncludesCommandLineArguments [FAIL]
      System.IO.DirectoryNotFoundException : /root/helix/work/workitem/true/
      Stack Trace:
        /_/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFileProvider.cs(70,0): at Microsoft.Extensions.FileProviders.PhysicalFileProvider..ctor(String root, ExclusionFilters filters)
        /_/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFileProvider.cs(44,0): at Microsoft.Extensions.FileProviders.PhysicalFileProvider..ctor(String root)
        /_/src/libraries/Microsoft.Extensions.Hosting/src/HostBuilder.cs(163,0): at Microsoft.Extensions.Hosting.HostBuilder.CreateHostingEnvironment()
        /_/src/libraries/Microsoft.Extensions.Hosting/src/HostBuilder.cs(128,0): at Microsoft.Extensions.Hosting.HostBuilder.Build()
        /_/src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/HostTests.cs(42,0): at Microsoft.Extensions.Hosting.Tests.HostTests.CreateDefaultBuilder_IncludesCommandLineArguments()
        /_/src/mono/netcore/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs(378,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
  Finished:    Microsoft.Extensions.Hosting.Unit.Tests
=== TEST EXECUTION SUMMARY ===
   Microsoft.Extensions.Hosting.Unit.Tests  Total: 86, Errors: 0, Failed: 1, Skipped: 0, Time: 3.356s
/root/helix/work/workitem

[jdmallen]

@maryamariyan If I'm reading the test results correctly, when compared to the test itself, it appears the string "true" is being appended to the second argument expected, which is decidedly unexpected and resulting in the exception. I'll try to figure out how to get these tests in Hosting to work against my changes and push again. I'm not sure why it's doing that yet.

[jdmallen]

Also, I rebased off master and pushed.

[am11]

One possible solution is to selectively apply this line only in Windows environments

IMO, confining /option style to Windows is a reasonable solution to that problem, and making hyphened options work cross platform is the right goal. In Unix-y worlds, the argument parsing conventions are standardized for shell and utilities, (IEEE 1003.1-2017 (POSIX)), and it comes natural to muscle memory. e.g. devs pay attention to these details: --option represents a full name, -o represents alias, and -option represents multiple aliases -o -p -t -i -o -n batched together, e.g. git clean -xdf or rm -rf and so on. (the fix to violation of POSIX shell often goes against PowerShell's convention of options, where single hyphen followed by multi characters is a standard :)

[jdmallen]

@am11 I agree with what you said, and generally prefer how POSIX handles options. However, in order to support both styles, I think by adding my value-less switch feature, this library can no longer be OS-agnostic. I'm playing around with this as a static field in CommandLineConfigurationProvider:

        private static bool s_isWindows =
#if NET461
            true;
#else
            RuntimeInformation.IsOSPlatform(OSPlatform.Windows);
#end

I'm using it to only check for the /-style flags if that field is true, otherwise treat them as any other invalid key or as a Linux-root path value. However, it feels messy; especially with the tests, since I have to conditionally test all the /key-style flags in Windows only.

Are there any other libraries you can think of that vary in function depending on the OS so I can see how to properly do this? I feel like I'm close, but I want to make sure I'm adhering to how it's supposed to be done.

I'm also considering going buck-wild with this and supporting the full POSIX standard for options when the environment is not Windows. Adding the feature to have valueless flags as command line switches is only a piece of the pie, as you noted (--option != (-option == -o -p -t -i -o -n)).

[am11]

The automatic binding of uneven number of user-specified arguments that works interchangeably with Windows-styles (cmd /option and PowerShell -option) and Unix (--option) on all platforms seamlessly is hard to achieve - if not impossible - without strongly-typed binding as https://github.com/dotnet/command-line-api/ provides. CLI API does not enforce any argument style per se; as it works with strong registration-based mechanism.

This library (Microsoft.Extensions.Configuration) aim to be dynamic (without regsitration), style-agnostic and cross-platform, and has undefined behaviors as is; as you have pointed out above in "IgnoreThisKey":"--contentroot" example. With the current state of this PR, the bug is shifted to one position down and I personally don't see it as a bad impact given the inherent platforms/styles disparities. However, if we can support the old behavior for even number of arguments to avoid this breakage, it would make the PR easier to review/ship. We can then separately propose introducing strict POSIX conformance mode, as an addition to M.E.C.

Side note on command-line-api: it currently does not support stdin (which, in case of POSIX shell style, is represented by single hyphen -, alias to /dev/stdin). We have recently added stdin support in dotnet/format, which consumes command-line-api, but required special casing - and /dev/stdin.

[jdmallen]

Just checking in to say this PR isn't dead. Been working OT on lots of 1/1 deliverables at work-- should have more time for this either around Christmastime or in January. I appreciate all the feedback I've received so far!

[safern]

Wouldn't this be calling TryProcessArgs with the last arg twice? previousArg will be pointing to enumerator.Current which is the same as currentArg if it entered the while loop.

[jdmallen]

The variable previousArg gets assigned the value of currentArg at the end of the last loop in the while block. When it reaches the point where it processes the last argument (outside the while loop), previousArg and currentArg are indeed the same value, but that value has not yet been passed as the previousArg to the TryProcessArgs method. It only goes through it once.

[safern]

what would happen if the value expected for the key is not a boolean? Do we have validation for that?

[jdmallen]

I'm not sure I understand the question. The value here gets assigned "true" if the next command line argument found is another key. For example:

./program.exe -Key1 --Key2 someValue /Key3 Key4=42

would generate the dictionary { {"Key1", "true"}, {"Key2", "someValue"}, {"Key3", "true"}, {"Key1", "42"} }.

The absence of a switch without a value can be interpreted as a falsy value for that switch. For example, if my program is looking for a value for Key1 in IConfiguration to determine whether or not to enable a feature, and I removed "Key1" from the execution arguments, I would simply not enable that feature if it were missing, much like a switch on a typical *nix program.

Without a given value, true is the best guess. However, I could assign it pretty much any string value I want. I just picked "true" since it seemed the most intuitive. Consumers of this library can still bind the configuration to a POCO, much like appsettings.json configuration, and {"Key1", "true"} in the IConfiguration would be successfully parsed and assigned to something like public bool Key1 { get; set; }.

[safern]

While I'm also inclined to doing the StartsWith('/') evaluation on Linux only, what would happen if I want to pass down a configuration value that starts with / on Windows it would be broken by this change as well, right?

So imagine I want to say, --api-endpoint /clients/7, then my app would be broken on Windows as --api-endpoint will now return api-endpoint: true, right?

It concerns me that we could potentially break people that might pass down a value that starts with / where the value is not a path and that value could appear on Windows as well.

[safern]

Friendly ping @jdmallen, have you had a chance to look at the comments @maryamariyan and I left?

[jdmallen]

Friendly ping @jdmallen, have you had a chance to look at the comments @maryamariyan and I left?

Hi there! Yes, I've had a chance to look over the comments, and thanks for reviewing. I finally cut out time this weekend to dig in and apply the requested changes and ask/answer questions as they arise. I know you folks are probably eager to wrap up active development on 6.0 and I don't want to get in the way of that!

[safern]

Thank you @jdmallen. Take your time, we just want to make sure PRs don't go dead and try to keep a sane number of open PRs, only PRs which are active. I'll be waiting for your changes to review them 😄

[jdmallen]

While I'm also inclined to doing the StartsWith('/') evaluation on Linux only, what would happen if I want to pass down a configuration value that starts with / on Windows it would be broken by this change as well, right?

So imagine I want to say, --api-endpoint /clients/7, then my app would be broken on Windows as --api-endpoint will now return api-endpoint: true, right?

It concerns me that we could potentially break people that might pass down a value that starts with / where the value is not a path and that value could appear on Windows as well.

That is a bit of a conundrum, and relative URIs were not something I thought of as a possible situation. I did already add a check for multiple /s in the key name (and to treat it as a value if found), but if something like --api-endpoint /clients came through... I'm not sure how to reconcile it with this PR. There's no way to determine if they're two separate valueless switches or a key value pair, short of forcing users to stick an = between the two (which will always work). I'm open to suggestions, but I'm pretty sure that will kill this PR if that's a no-go.

[jdmallen]

@safern & @maryamariyan: Pushed a bunch of changes. Hopefully they resolve the errors in the HostTests and on the Linux environment tests. Please lemme know what you think about my responses above.

[jdmallen]

I think I need to fix the tests, since they are still OS-agnostic and the code no longer is.

[jdmallen]

Oh, also, I tried to make this field readonly per the style guide, but whenever I do, the compiler fails with CA1802 and tells me to make it a const, which I cannot do with the call to IsOSPlatform. I think there might be a bug with the code inspection with regard to the pre-compiler directive. I think it only sees the assignment to true.

[ericstj]

@jdmallen were you still planning to fix tests before having reviewers take another look?

[safern]

I think there might be a bug with the code inspection with regard to the pre-compiler directive. I think it only sees the assignment to true.

Could it be that you are getting the diagnostic error for the net461 build? So probably you need to make it a const for net461 and a readonly field for non net461?

[safern]

Closing the PR since there has not been any activity nor author response. Feel free to re-open if you want to keep working on this and have any updates.