[ts-command-line] Prevent ambiguous abbreviations of parameters defined on parent tool or action

apps/heft/package.json

@@ -40,7 +40,6 @@
     "@rushstack/rig-package": "workspace:*",
     "@rushstack/ts-command-line": "workspace:*",
     "@types/tapable": "1.0.6",
-    "argparse": "~1.0.9",
     "chokidar": "~3.4.0",
     "fast-glob": "~3.3.1",
     "git-repo-info": "~2.1.0",
@@ -54,7 +53,6 @@
     "local-eslint-config": "workspace:*",
     "@rushstack/heft": "0.62.0",
     "@rushstack/heft-node-rig": "2.3.2",
-    "@types/argparse": "1.0.38",
     "@types/heft-jest": "1.0.1",
     "@types/node": "18.17.15",
     "@types/watchpack": "2.4.0",

apps/heft/src/cli/HeftCommandLineParser.ts

@@ -1,7 +1,6 @@
 // Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license.
 // See LICENSE in the project root for license information.
 
-import { ArgumentParser } from 'argparse';
 import {
   CommandLineParser,
   type AliasCommandLineAction,
@@ -20,12 +19,13 @@ import { MetricsCollector } from '../metrics/MetricsCollector';
 import { HeftConfiguration } from '../configuration/HeftConfiguration';
 import { InternalHeftSession } from '../pluginFramework/InternalHeftSession';
 import { LoggingManager } from '../pluginFramework/logging/LoggingManager';
-import { Constants } from '../utilities/Constants';
 import { CleanAction } from './actions/CleanAction';
 import { PhaseAction } from './actions/PhaseAction';
 import { RunAction } from './actions/RunAction';
 import type { IHeftActionOptions } from './actions/IHeftAction';
 import { AliasAction } from './actions/AliasAction';
+import { getToolParameterNamesFromArgs } from '../utilities/CliUtilities';
+import { Constants } from '../utilities/Constants';
 
 /**
  * This interfaces specifies values for parameters that must be parsed before the CLI
@@ -233,13 +233,11 @@ export class HeftCommandLineParser extends CommandLineParser {
       throw new InternalError('onDefineParameters() has not yet been called.');
     }
 
-    // This is a rough parsing of the --debug parameter
-    const parser: ArgumentParser = new ArgumentParser({ addHelp: false });
-    parser.addArgument(this._debugFlag.longName, { dest: 'debug', action: 'storeTrue' });
-    parser.addArgument(this._unmanagedFlag.longName, { dest: 'unmanaged', action: 'storeTrue' });
-
-    const [result]: IPreInitializationArgumentValues[] = parser.parseKnownArgs(args);
-    return result;
+    const toolParameters: Set<string> = getToolParameterNamesFromArgs(args);
+    return {
+      debug: toolParameters.has(this._debugFlag.longName),
+      unmanaged: toolParameters.has(this._unmanagedFlag.longName)
+    };
   }
 
   private async _reportErrorAndSetExitCode(error: Error): Promise<void> {

apps/heft/src/cli/actions/RunAction.ts

@@ -78,21 +78,18 @@ export function definePhaseScopingParameters(action: IHeftAction): IScopingParam
   return {
     toParameter: action.defineStringListParameter({
       parameterLongName: Constants.toParameterLongName,
-      parameterShortName: Constants.toParameterShortName,
       description: `The phase to ${action.actionName} to, including all transitive dependencies.`,
       argumentName: 'PHASE',
       parameterGroup: ScopedCommandLineAction.ScopingParameterGroup
     }),
     toExceptParameter: action.defineStringListParameter({
       parameterLongName: Constants.toExceptParameterLongName,
-      parameterShortName: Constants.toExceptParameterShortName,
       description: `The phase to ${action.actionName} to (but not include), including all transitive dependencies.`,
       argumentName: 'PHASE',
       parameterGroup: ScopedCommandLineAction.ScopingParameterGroup
     }),
     onlyParameter: action.defineStringListParameter({
       parameterLongName: Constants.onlyParameterLongName,
-      parameterShortName: Constants.onlyParameterShortName,
       description: `The phase to ${action.actionName}.`,
       argumentName: 'PHASE',
       parameterGroup: ScopedCommandLineAction.ScopingParameterGroup

apps/heft/src/startWithVersionSelector.ts

@@ -8,6 +8,7 @@
 import * as path from 'path';
 import * as fs from 'fs';
 import type { IPackageJson } from '@rushstack/node-core-library';
+import { getToolParameterNamesFromArgs } from './utilities/CliUtilities';
 import { Constants } from './utilities/Constants';
 
 const HEFT_PACKAGE_NAME: string = '@rushstack/heft';
@@ -49,14 +50,15 @@ function tryGetPackageFolderFor(resolvedFileOrFolderPath: string): string | unde
  * Use "heft --unmanaged" to bypass this feature.
  */
 function tryStartLocalHeft(): boolean {
-  if (process.argv.indexOf(Constants.unmanagedParameterLongName) >= 0) {
+  const toolParameters: Set<string> = getToolParameterNamesFromArgs();
+  if (toolParameters.has(Constants.unmanagedParameterLongName)) {
     console.log(
       `Bypassing the Heft version selector because ${JSON.stringify(Constants.unmanagedParameterLongName)} ` +
         'was specified.'
     );
     console.log();
     return false;
-  } else if (process.argv.indexOf(Constants.debugParameterLongName) >= 0) {
+  } else if (toolParameters.has(Constants.debugParameterLongName)) {
     // The unmanaged flag could be undiscoverable if it's not in their locally installed version
     console.log(
       'Searching for a locally installed version of Heft. Use the ' +

apps/heft/src/utilities/CliUtilities.ts

@@ -0,0 +1,22 @@
+// Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license.
+// See LICENSE in the project root for license information.
+
+/**
+ * Parse the arguments to the tool being executed and return the tool argument names.
+ *
+ * @param argv - The arguments to parse. Defaults to `process.argv`.
+ */
+export function getToolParameterNamesFromArgs(argv: string[] = process.argv): Set<string> {
+  const toolParameters: Set<string> = new Set();
+  // Skip the first two arguments, which are the path to the Node executable and the path to the Heft
+  // entrypoint. The remaining arguments are the tool arguments. Grab them until we reach a non-"-"-prefixed
+  // argument. We can do this simple parsing because the Heft tool only has simple optional flags.
+  for (let i: number = 2; i < argv.length; ++i) {
+    const arg: string = argv[i];
+    if (!arg.startsWith('-')) {
+      break;
+    }
+    toolParameters.add(arg);
+  }
+  return toolParameters;
+}

apps/heft/src/utilities/Constants.ts

@@ -20,18 +20,12 @@ export class Constants {
 
   public static onlyParameterLongName: string = '--only';
 
-  public static onlyParameterShortName: string = '-o';
-
   public static productionParameterLongName: string = '--production';
 
   public static toParameterLongName: string = '--to';
 
-  public static toParameterShortName: string = '-t';
-
   public static toExceptParameterLongName: string = '--to-except';
 
-  public static toExceptParameterShortName: string = '-T';
-
   public static unmanagedParameterLongName: string = '--unmanaged';
 
   public static verboseParameterLongName: string = '--verbose';

common/changes/@rushstack/heft/user-danade-PreventAmbiguousAbbreviations_2023-10-23-20-01.json

@@ -0,0 +1,10 @@
+{
+  "changes": [
+    {
+      "packageName": "@rushstack/heft",
+      "comment": "Fix an issue with parsing of the \"--debug\" and \"--unmanaged\" flags for Heft",
+      "type": "patch"
+    }
+  ],
+  "packageName": "@rushstack/heft"
+}

common/changes/@rushstack/heft/user-danade-PreventAmbiguousAbbreviations_2023-10-24-18-11.json

@@ -0,0 +1,10 @@
+{
+  "changes": [
+    {
+      "packageName": "@rushstack/heft",
+      "comment": "[BREAKING CHANGE] Remove \"heft run\" short-parameters for \"--to\" (\"-t\"), \"--to-except\" (\"-T\"), and \"--only\" (\"-o\").",
+      "type": "minor"
+    }
+  ],
+  "packageName": "@rushstack/heft"
+}

common/changes/@rushstack/ts-command-line/user-danade-PreventAmbiguousAbbreviations_2023-10-23-20-01.json

@@ -0,0 +1,10 @@
+{
+  "changes": [
+    {
+      "packageName": "@rushstack/ts-command-line",
+      "comment": "Consider parent tool and action parameters when determining ambiguous abbreviations. For example, if a CLI tool `mytool` has a parameter `--myparam` and an action `myaction`, then `myaction` would not accept a parameter named `--myparam` (i.e. - `mytool --myparam myaction` is valid, `mytool myaction --myparam` is not). Additionally, any parameter that can be abbreviated to `--myparam` must be uniquely provided (i.e. - `--myparam-2` can only be abbreviated to `--myparam-`, since any shorter abbreviation would be ambiguous with the original `--myparam` on the tool).",
+      "type": "minor"
+    }
+  ],
+  "packageName": "@rushstack/ts-command-line"
+}

common/reviews/api/ts-command-line.api.md

@@ -14,7 +14,7 @@ export class AliasCommandLineAction extends CommandLineAction {
     // @internal
     _processParsedData(parserOptions: ICommandLineParserOptions, data: _ICommandLineParserData): void;
     // @internal (undocumented)
-    _registerDefinedParameters(): void;
+    _registerDefinedParameters(state: _IRegisterDefinedParametersState): void;
     readonly targetAction: CommandLineAction;
 }
 
@@ -121,8 +121,6 @@ export abstract class CommandLineParameter {
     constructor(definition: IBaseCommandLineDefinition);
     abstract appendToArgList(argList: string[]): void;
     readonly description: string;
-    // @internal
-    _disableShortName(): void;
     readonly environmentVariable: string | undefined;
     // @internal
     _getSupplementaryNotes(supplementaryNotes: string[]): void;
@@ -158,6 +156,10 @@ export enum CommandLineParameterKind {
 export abstract class CommandLineParameterProvider {
     // @internal
     constructor();
+    // @internal (undocumented)
+    readonly _ambiguousParameterParserKeysByName: Map<string, string>;
+    // @internal (undocumented)
+    protected _defineAmbiguousParameter(name: string): string;
     defineChoiceListParameter(definition: ICommandLineChoiceListDefinition): CommandLineChoiceListParameter;
     defineChoiceParameter(definition: ICommandLineChoiceDefinition): CommandLineChoiceParameter;
     defineCommandLineRemainder(definition: ICommandLineRemainderDefinition): CommandLineRemainder;
@@ -184,10 +186,14 @@ export abstract class CommandLineParameterProvider {
     parseScopedLongName(scopedLongName: string): IScopedLongNameParseResult;
     // @internal (undocumented)
     protected _processParsedData(parserOptions: ICommandLineParserOptions, data: _ICommandLineParserData): void;
+    // (undocumented)
+    protected _registerAmbiguousParameter(name: string, parserKey: string): void;
+    // @internal (undocumented)
+    _registerDefinedParameters(state: _IRegisterDefinedParametersState): void;
     // @internal (undocumented)
-    _registerDefinedParameters(): void;
+    protected readonly _registeredParameterParserKeysByName: Map<string, string>;
     // @internal (undocumented)
-    protected _registerParameter(parameter: CommandLineParameter, useScopedLongName: boolean): void;
+    protected _registerParameter(parameter: CommandLineParameter, useScopedLongName: boolean, ignoreShortName: boolean): void;
     get remainder(): CommandLineRemainder | undefined;
     renderHelpText(): string;
     renderUsageText(): string;
@@ -213,7 +219,7 @@ export abstract class CommandLineParser extends CommandLineParameterProvider {
     protected _getArgumentParser(): argparse.ArgumentParser;
     protected onExecute(): Promise<void>;
     // @internal (undocumented)
-    _registerDefinedParameters(): void;
+    _registerDefinedParameters(state: _IRegisterDefinedParametersState): void;
     selectedAction: CommandLineAction | undefined;
     tryGetAction(actionName: string): CommandLineAction | undefined;
 }
@@ -360,6 +366,11 @@ export interface ICommandLineStringDefinition extends IBaseCommandLineDefinition
 export interface ICommandLineStringListDefinition extends IBaseCommandLineDefinitionWithArgument {
 }
 
+// @internal
+export interface _IRegisterDefinedParametersState {
+    parentParameterNames: Set<string>;
+}
+
 // @public
 export interface IScopedLongNameParseResult {
     longName: string;
@@ -382,6 +393,8 @@ export abstract class ScopedCommandLineAction extends CommandLineAction {
     get parameters(): ReadonlyArray<CommandLineParameter>;
     // @internal
     _processParsedData(parserOptions: ICommandLineParserOptions, data: _ICommandLineParserData): void;
+    // @internal (undocumented)
+    _registerDefinedParameters(state: _IRegisterDefinedParametersState): void;
     static readonly ScopingParameterGroup: typeof SCOPING_PARAMETER_GROUP;
 }
 

libraries/ts-command-line/src/index.ts

@@ -7,12 +7,15 @@
  * @packageDocumentation
  */
 
-export { CommandLineAction, ICommandLineActionOptions } from './providers/CommandLineAction';
+export { CommandLineAction, type ICommandLineActionOptions } from './providers/CommandLineAction';
 export { DynamicCommandLineAction } from './providers/DynamicCommandLineAction';
 export { ScopedCommandLineAction } from './providers/ScopedCommandLineAction';
-export { AliasCommandLineAction, IAliasCommandLineActionOptions } from './providers/AliasCommandLineAction';
-
 export {
+  AliasCommandLineAction,
+  type IAliasCommandLineActionOptions
+} from './providers/AliasCommandLineAction';
+
+export type {
   IBaseCommandLineDefinition,
   IBaseCommandLineDefinitionWithArgument,
   ICommandLineFlagDefinition,
@@ -42,11 +45,12 @@ export { CommandLineRemainder } from './parameters/CommandLineRemainder';
 
 export {
   CommandLineParameterProvider,
-  IScopedLongNameParseResult,
-  ICommandLineParserData as _ICommandLineParserData
+  type IScopedLongNameParseResult,
+  type ICommandLineParserData as _ICommandLineParserData,
+  type IRegisterDefinedParametersState as _IRegisterDefinedParametersState
 } from './providers/CommandLineParameterProvider';
 
-export { ICommandLineParserOptions, CommandLineParser } from './providers/CommandLineParser';
+export { CommandLineParser, type ICommandLineParserOptions } from './providers/CommandLineParser';
 export { DynamicCommandLineParser } from './providers/DynamicCommandLineParser';
 
 export { CommandLineConstants } from './Constants';

libraries/ts-command-line/src/parameters/BaseClasses.ts

@@ -179,14 +179,6 @@ export abstract class CommandLineParameter {
    */
   public abstract _setValue(data: any): void; // eslint-disable-line @typescript-eslint/no-explicit-any
 
-  /**
-   * Called internally by CommandLineParameterProvider._registerDefinedParameters()
-   * @internal
-   */
-  public _disableShortName(): void {
-    this._shortNameValue = undefined;
-  }
-
   /**
    * Returns additional text used by the help formatter.
    * @internal

libraries/ts-command-line/src/providers/AliasCommandLineAction.ts

@@ -5,7 +5,7 @@ import * as argparse from 'argparse';
 
 import { CommandLineAction } from './CommandLineAction';
 import { CommandLineParameterKind, type CommandLineParameter } from '../parameters/BaseClasses';
-import type { ICommandLineParserData } from './CommandLineParameterProvider';
+import type { ICommandLineParserData, IRegisterDefinedParametersState } from './CommandLineParameterProvider';
 import type { ICommandLineParserOptions } from './CommandLineParser';
 import type { CommandLineChoiceParameter } from '../parameters/CommandLineChoiceParameter';
 import type { CommandLineFlagParameter } from '../parameters/CommandLineFlagParameter';
@@ -85,7 +85,7 @@ export class AliasCommandLineAction extends CommandLineAction {
   }
 
   /** @internal */
-  public _registerDefinedParameters(): void {
+  public _registerDefinedParameters(state: IRegisterDefinedParametersState): void {
     /* override */
     // All parameters are going to be defined by the target action. Re-use the target action parameters
     // for this action.
@@ -137,6 +137,7 @@ export class AliasCommandLineAction extends CommandLineAction {
         default:
           throw new Error(`Unsupported parameter kind: ${parameter.kind}`);
       }
+
       // We know the parserKey is defined because the underlying _defineParameter method sets it,
       // and all parameters that we have access to have already been defined.
       this._parameterKeyMap.set(aliasParameter._parserKey!, parameter._parserKey!);
@@ -149,8 +150,23 @@ export class AliasCommandLineAction extends CommandLineAction {
       this._parameterKeyMap.set(argparse.Const.REMAINDER, argparse.Const.REMAINDER);
     }
 
-    // Finally, register the parameters with the parser.
-    super._registerDefinedParameters();
+    // Finally, register the parameters with the parser. We need to make sure that the target action
+    // is registered, since we need to re-use its parameters, and ambiguous parameters are discovered
+    // during registration. This will no-op if the target action is already registered.
+    this.targetAction._registerDefinedParameters(state);
+    super._registerDefinedParameters(state);
+
+    // We need to re-map the ambiguous parameters after they are defined by calling
+    // super._registerDefinedParameters()
+    for (const [ambiguousParameterName, parserKey] of this._ambiguousParameterParserKeysByName) {
+      const targetParserKey: string | undefined =
+        this.targetAction._ambiguousParameterParserKeysByName.get(ambiguousParameterName);
+
+      // If we have a mapping for the specified key, then use it. Otherwise, use the key as-is.
+      if (targetParserKey) {
+        this._parameterKeyMap.set(parserKey, targetParserKey);
+      }
+    }
   }
 
   /**