CLI: Add support for unsupported and deprecated arguments - #41462
CLI: Add support for unsupported and deprecated arguments#41462David Bennett (dkbennett) wants to merge 19 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends WSLC’s CLI parsing model to explicitly represent Supported, Unsupported, and Deprecated arguments/commands. It hides unsupported/deprecated items from help output, emits consistent warnings for deprecated-but-still-functional arguments, and reports intentionally unsupported features (notably --platform) with a dedicated error code (WSLC_E_NOT_SUPPORTED) plus telemetry.
Changes:
- Add
ArgumentState(Supported/Unsupported/Deprecated) with parser enforcement for unsupported arguments and warning emission for deprecated ones. - Add unsupported-command plumbing (recognized by the parser, rejected with a specific exception, and omitted from help).
- Introduce
WSLC_E_NOT_SUPPORTEDacross SDK/IDL surfaces, symbolic error mapping, localization, and unit tests; apply--platformas unsupported across targeted Docker-compatible commands.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/windows/wslc/WSLCCLIParserUnitTests.cpp | Adds unit coverage verifying unsupported options/aliases/positionals throw the correct typed exception and don’t populate ArgMap. |
| test/windows/wslc/WSLCCLICommandUnitTests.cpp | Adds unit coverage for unsupported commands being hidden/recognized and for --platform being declared unsupported across 10 commands; validates symbolic error name mapping. |
| test/windows/wslc/WSLCCLIArgumentUnitTests.cpp | Adds unit coverage for argument state behavior, deprecated warning formatting, and help output hiding for non-visible args. |
| src/windows/WslcSDK/wslcsdk.h | Adds the WSLC_E_NOT_SUPPORTED HRESULT definition to the native SDK header. |
| src/windows/WslcSDK/winrt/wslcsdk.idl | Adds NotSupported to the WinRT error enum surface. |
| src/windows/wslc/core/Main.cpp | Emits deprecated-argument warnings during parsing, adds UnsupportedFeatureException handling with telemetry and standardized help/error output. |
| src/windows/wslc/core/Exceptions.h | Introduces UnsupportedFeatureType and UnsupportedFeatureException for structured unsupported-feature reporting. |
| src/windows/wslc/core/Command.h | Adds UnsupportedCommand and a virtual GetUnsupportedCommands() surface; adds OutputDeprecatedArgumentWarnings() API. |
| src/windows/wslc/core/Command.cpp | Filters non-visible args from help, recognizes unsupported subcommands, skips unsupported args during validation, and emits deprecated-argument warnings. |
| src/windows/wslc/commands/ImageSaveCommand.cpp | Marks ArgType::Platform as unsupported for image save. |
| src/windows/wslc/commands/ImageRemoveCommand.cpp | Marks ArgType::Platform as unsupported for image remove. |
| src/windows/wslc/commands/ImagePushCommand.cpp | Marks ArgType::Platform as unsupported for image push. |
| src/windows/wslc/commands/ImagePullCommand.cpp | Marks ArgType::Platform as unsupported for image pull. |
| src/windows/wslc/commands/ImageLoadCommand.cpp | Marks ArgType::Platform as unsupported for image load. |
| src/windows/wslc/commands/ImageInspectCommand.cpp | Marks ArgType::Platform as unsupported for image inspect. |
| src/windows/wslc/commands/ImageImportCommand.cpp | Marks ArgType::Platform as unsupported for image import. |
| src/windows/wslc/commands/ImageBuildCommand.cpp | Marks ArgType::Platform as unsupported for image build. |
| src/windows/wslc/commands/ContainerRunCommand.cpp | Marks ArgType::Platform as unsupported for container run. |
| src/windows/wslc/commands/ContainerCreateCommand.cpp | Marks ArgType::Platform as unsupported for container create. |
| src/windows/wslc/commands/ContainerCommand.h | Trivial whitespace cleanup in the container command declaration. |
| src/windows/wslc/arguments/ArgumentParser.cpp | Adds centralized unsupported-argument throwing and enforces it across named, alias, positional, and forwarded argument parsing paths. |
| src/windows/wslc/arguments/ArgumentDefinitions.h | Adds the Platform argument definition (name/kind/description) so it can be recognized where declared. |
| src/windows/wslc/arguments/Argument.h | Introduces ArgumentState and helper predicates (IsSupported, IsVisible, IsDeprecated). |
| src/windows/wslc/arguments/Argument.cpp | Implements CreateUnsupported / CreateDeprecated and updates argument-creation commentary accordingly. |
| src/windows/service/inc/wslc.idl | Adds WSLC_E_NOT_SUPPORTED to the internal IDL error defines. |
| src/windows/common/wslutil.cpp | Adds WSLC_E_NOT_SUPPORTED to common HRESULT→symbolic-name mapping. |
| localization/strings/en-US/Resources.resw | Adds localized strings for unsupported command/option/argument errors and deprecated warnings; adds description for --platform. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
localization/strings/en-US/Resources.resw:3279
- The Platform argument description is user-facing and reads ungrammatically (missing articles). Consider rephrasing so it’s a complete sentence (e.g., “Set the platform if the server is multi-platform capable”).
<data name="WSLCCLI_PlatformArgDescription" xml:space="preserve">
<value>Set platform if server is multi-platform capable</value>
</data>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/windows/wslc/arguments/Argument.cpp:74
- CreateUnsupported/CreateDeprecated always call Create(type, false, ...) which forces deprecated/unsupported args to be optional. If a deprecated argument needs to remain required for compatibility, this will silently disable required-argument validation.
Argument Argument::CreateUnsupported(ArgType type, std::optional<argument::Limit> limit)
{
auto argument = Create(type, false, limit);
argument.m_state = ArgumentState::Unsupported;
return argument;
}
Argument Argument::CreateDeprecated(ArgType type, std::optional<argument::Limit> limit, std::optional<std::wstring> desc)
{
auto argument = Create(type, false, limit, std::move(desc));
argument.m_state = ArgumentState::Deprecated;
return argument;
}
src/windows/wslc/arguments/Argument.h:74
- CreateUnsupported/CreateDeprecated hardcode required=false via their implementations, and their signatures don’t allow callers to mark a deprecated argument as required. That makes it impossible to deprecate a required positional/option while preserving existing validation behavior.
// Creates an argument that is recognized by the parser but rejected and omitted from help.
static Argument CreateUnsupported(ArgType type, std::optional<argument::Limit> limit = std::nullopt);
// Creates an argument that is accepted and omitted from help. Its description provides guidance in the warning.
static Argument CreateDeprecated(ArgType type, std::optional<argument::Limit> limit = std::nullopt, std::optional<std::wstring> desc = std::nullopt);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/windows/wslc/arguments/Argument.cpp:74
- Argument::CreateDeprecated currently forwards
desc = std::nulloptinto Argument::Create(), which means deprecated arguments default to the normal help description from ArgumentDefinitions.h. Since Command::OutputDeprecatedArgumentWarnings treatsDescription()as optional deprecation guidance, this will accidentally emit warnings like "'--force' is deprecated. " unless every caller remembers to pass an empty string.
Argument Argument::CreateDeprecated(ArgType type, std::optional<argument::Limit> limit, std::optional<std::wstring> desc)
{
auto argument = Create(type, false, limit, std::move(desc));
argument.m_state = ArgumentState::Deprecated;
return argument;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (1)
localization/strings/en-US/Resources.resw:2244
- The localized usage template now contains two spaces after
Usage:("Usage: {} {}"). This changes help/usage output formatting and can break downstream string-matching (including existing tests/scripts) without a clear functional reason. Prefer keeping a single space ("Usage: {} {}") and, if extra alignment is needed, handle it in the help-rendering code instead of the localized string.
<data name="WSLCCLI_Usage" xml:space="preserve">
<value>Usage: {} {}</value>
<comment>{FixedPlaceholder="{}"}{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
There was a problem hiding this comment.
🔵 Needs a closer look
The new WSLCCLI_DeprecatedArgumentWarning RESW <comment> format likely fails the repo’s localization comment validator (duplicate {FixedPlaceholder="{}"} marker).
Review details
Suppressed comments (1)
localization/strings/en-US/Resources.resw:2214
- The
<comment>forWSLCCLI_DeprecatedArgumentWarningincludes two{FixedPlaceholder="{}"}tokens. The repo’s localization validator (tools/devops/validate-localization.pygenerate_string_comment()) expects at most one{FixedPlaceholder="{}"}marker when a string uses any{}inserts, so this is likely to fail comment validation in CI.
<data name="WSLCCLI_DeprecatedArgumentWarning" xml:space="preserve">
<value>'{}' is deprecated. Use {} instead.</value>
<comment>{FixedPlaceholder="{}"}{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
- Files reviewed: 25/27 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The review found at least one likely build-breaking unit-test issue (unused local variable) and a localization placeholder-lock inconsistency that should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 25/27 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The new WSLCCLI_DeprecatedArgumentWarning string has two {} placeholders but its .resw metadata only marks one fixed placeholder, which can break localization/validation expectations.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
localization/strings/en-US/Resources.resw:2215
WSLCCLI_DeprecatedArgumentWarninghas two{}placeholders, but the<comment>metadata only marks one fixed placeholder. Other multi-placeholder strings in this file include one{FixedPlaceholder="{}"}per placeholder, and tooling may rely on that to prevent argument tokens from being translated.
- Files reviewed: 25/27 changed files
- Comments generated: 0 new
- Review effort level: Lite
| THROW_HR_IF_MSG( | ||
| E_INVALIDARG, | ||
| !deprecatedTypes.emplace(deprecatedType).second, | ||
| "Argument type %d has multiple deprecation mappings", |
There was a problem hiding this comment.
Should this be localized?
There was a problem hiding this comment.
This happens several times below as well
There was a problem hiding this comment.
These are not user-facing. These are bad-configuration / state errors due to an invalid command configuration in the code.
I will change these to a more specific error code however that indicates the problem is internal state, not a user argument is invalid.
ggarzia-MSFT
left a comment
ggarzia-MSFT
left a comment
There was a problem hiding this comment.
Other than localization + minor nits lgtm
| // Check if this argument expects a value | ||
| if (firstArg->Kind() == Kind::Value) | ||
| { | ||
| const auto type = ResolveArgumentType(*firstArg); |
There was a problem hiding this comment.
nit: this declaration is the same as the declaration of firstType on line 517, can we move this declaration out of the if statement and remove the one on line 517?
|
|
||
| if (nextArg->Kind() == Kind::Value) | ||
| { | ||
| const auto type = ResolveArgumentType(*nextArg); |
There was a problem hiding this comment.
nit: same as above, this is the same as type on line 568
| auto wslcOutput = wslcResult.Stdout.value(); | ||
| const std::wstring usageNeedle = L"Usage: wslc"; | ||
| const std::wstring usageReplacement = L"Usage: container"; | ||
| const std::wstring usageNeedle = L"Usage: wslc"; |
There was a problem hiding this comment.
Is this for docker parity?
There was a problem hiding this comment.
Yes, there are two spaces in the Docker usage for clarity, just matching that.
…command configuration
There was a problem hiding this comment.
🔵 Needs a closer look
It modifies core CLI parsing and argument storage semantics across multiple layers, which warrants final human review despite the added test coverage.
Review details
- Files reviewed: 25/27 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core CLI parsing and argument storage semantics (unsupported/deprecated resolution) and should receive final human review despite strong test coverage.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/windows/wslc/arguments/ArgMap.h:179
HandleMapActioncurrently treats anyEnumBasedVariantMapActionother than Add/GetMutable/Remove as an implicit no-op via thedefault:case. SinceEnumBasedVariantMapActionincludes known read actions (Contains/Get/GetAll/Count), it would be clearer/safer to enumerate those explicitly and keep an assert/guard for truly unknown actions so future enum additions don’t silently skip cache invalidation logic.
- Files reviewed: 25/27 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary of the Pull Request
Adds command-level declarations for unsupported options and deprecated argument mappings. Unsupported options remain recognized for intentional WSLC/Docker divergences but are hidden from help and rejected through the normal argument-error path. Deprecated spellings act as hidden aliases for canonical replacement arguments and warn users to use the replacement.
This applies the unsupported option model to Docker-compatible
--platformoptions and provides reusable deprecation plumbing for compatibility syntax. Parsing, validation, and execution use only canonical arguments.PR Checklist
Detailed Description of the Pull Request / Additional comments
GetArguments()remains limited to executable arguments.ArgTypeand follow normal last-value or accumulation behavior.ArgTypevalues, and include hidden syntax in command-tree collision checks.--platformunsupported on 10 Docker-compatible commands and align generatedUsage:heading spacing with Docker.Unsupported positional arguments, unsupported commands, and replacementless deprecations are out of scope.
Validation Steps Performed
--platformon all 10 affected commands.