Avoid mutable global state#52883
Conversation
|
@MiYanni ptal |
|
Please don't do this - we can't use subclasses to contain behavior because it introduces diamond dependencies, and we have several permutations of cross cutting S.CL extensibility behavior. |
|
Which other extensions do we have? |
There was a problem hiding this comment.
Pull request overview
This PR addresses a thread-safety issue in System.CommandLine.StaticCompletions where IsDynamic metadata was tracked via mutable global state. It replaces that approach with explicit symbol types (ExtendedArgument<T>, ExtendedOption<T>) that carry the IsDynamic flag on the symbol instance.
Changes:
- Added
IExtendedSymbolplusExtendedOption<T>/ExtendedArgument<T>to holdIsDynamicper symbol instance. - Removed the prior
DynamicSymbolExtensionsimplementation that used a global dictionary for dynamic symbol state. - Updated several CLI option/argument factories to construct
ExtendedOption<T>/ExtendedArgument<T>.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/System.CommandLine.StaticCompletions/shells/PowershellShellProvider.cs | Updates dynamic-argument detection to use IExtendedSymbol rather than the old extension property. |
| src/System.CommandLine.StaticCompletions/ExtendedSymbols.cs | Introduces IExtendedSymbol, ExtendedOption<T>, ExtendedArgument<T>, and a Symbol IsDynamic extension getter. |
| src/System.CommandLine.StaticCompletions/DynamicSymbolExtensions.cs | Removes the previous global-dictionary-based IsDynamic implementation. |
| src/Cli/Microsoft.DotNet.Cli.Definitions/Common/TargetPlatformOptions.cs | Switches runtime option creation to ExtendedOption<string>. |
| src/Cli/Microsoft.DotNet.Cli.Definitions/Common/CommonOptions.cs | Switches several common options to ExtendedOption<T>. |
| src/Cli/Microsoft.DotNet.Cli.Definitions/Common/CommonArguments.cs | Switches package identity arguments to ExtendedArgument<T>. |
| src/Cli/Microsoft.DotNet.Cli.Definitions/Commands/Restore/RestoreCommandDefinition.cs | Switches restore runtime option creation to ExtendedOption<IEnumerable<string>>. |
| src/Cli/Microsoft.DotNet.Cli.Definitions/Commands/Reference/ReferenceAddCommandDefinition.cs | Switches framework option creation to ExtendedOption<string>. |
| src/Cli/Microsoft.DotNet.Cli.Definitions/Commands/Package/PackageAddCommandDefinition.cs | Switches version option creation to ExtendedOption<string>. |
We've got the forwarding stuff at minimum, and help descriptions for the same symbol per-command IIRC? |
|
Closing because I merged #52887 |
The implementation of
IsDynamicextension property mutated global state and was not thread-safe.This could cause state corruption when commands are created in parallel, e.g. in unit tests.
Removes global state and instead defines
ExtendedArgument<T>andExtendedOption<T>that allow setting the flag on select symbols.