No automatic token expansion for "from"; stored target cleanup (#2170)

## Checklist - [x] I have added [tests](https://www.cursorless.org/docs/contributing/test-case-recorder/) - [-] I have updated the [docs](https://github.com/cursorless-dev/cursorless/tree/main/docs) and [cheatsheet](https://github.com/cursorless-dev/cursorless/tree/main/cursorless-talon/src/cheatsheet) - [-] I have not broken the cheatsheet
cursorless-dev · Jan 14, 2024 · f2c9f4a · f2c9f4a
1 parent 2ae73a7
commit f2c9f4a
Show file tree

Hide file tree

Showing 10 changed files with 144 additions and 57 deletions.
diff --git a/packages/common/src/testUtil/TestCaseSnapshot.ts b/packages/common/src/testUtil/TestCaseSnapshot.ts
@@ -1,29 +1,30 @@
+import { StoredTargetKey } from "../StoredTargetKey";
 import {
   RangePlainObject,
   SelectionPlainObject,
   SerializedMarks,
   TargetPlainObject,
 } from "../util/toPlainObject";
 
-export type TestCaseSnapshot = {
+type MarkKeys = {
+  [K in `${StoredTargetKey}Mark`]?: TargetPlainObject[];
+};
+
+export interface TestCaseSnapshot extends MarkKeys {
   documentContents: string;
   selections: SelectionPlainObject[];
   clipboard?: string;
   // FIXME Visible ranges are not asserted during testing, see:
   // https://github.com/cursorless-dev/cursorless/issues/160
   visibleRanges?: RangePlainObject[];
   marks?: SerializedMarks;
-  thatMark?: TargetPlainObject[];
-  sourceMark?: TargetPlainObject[];
-  instanceReferenceMark?: TargetPlainObject[];
   timeOffsetSeconds?: number;
 
   /**
    * Extra information about the snapshot. Must be json serializable
    */
   metadata?: unknown;
-};
-
+}
 export type ExtraSnapshotField = keyof TestCaseSnapshot;
 export type ExcludableSnapshotField = keyof TestCaseSnapshot;
 

diff --git a/packages/cursorless-engine/src/actions/Actions.ts b/packages/cursorless-engine/src/actions/Actions.ts
@@ -33,7 +33,7 @@ import Remove from "./Remove";
 import Replace from "./Replace";
 import Rewrap from "./Rewrap";
 import { ScrollToBottom, ScrollToCenter, ScrollToTop } from "./Scroll";
-import { SetInstanceReference } from "./SetInstanceReference";
+import { SetSpecialTarget } from "./SetSpecialTarget";
 import {
   SetSelection,
   SetSelectionAfter,
@@ -134,7 +134,9 @@ export class Actions implements ActionRecord {
   scrollToBottom = new ScrollToBottom();
   scrollToCenter = new ScrollToCenter();
   scrollToTop = new ScrollToTop();
-  ["experimental.setInstanceReference"] = new SetInstanceReference();
+  ["experimental.setInstanceReference"] = new SetSpecialTarget(
+    "instanceReference",
+  );
   setSelection = new SetSelection();
   setSelectionAfter = new SetSelectionAfter();
   setSelectionBefore = new SetSelectionBefore();

diff --git a/...ngine/src/actions/SetInstanceReference.ts → ...ss-engine/src/actions/SetSpecialTarget.ts b/...ngine/src/actions/SetInstanceReference.ts → ...ss-engine/src/actions/SetSpecialTarget.ts
@@ -1,15 +1,18 @@
+import { StoredTargetKey } from "@cursorless/common";
 import { Target } from "../typings/target.types";
 import { SimpleAction, ActionReturnValue } from "./actions.types";
 
-export class SetInstanceReference implements SimpleAction {
-  constructor() {
+export class SetSpecialTarget implements SimpleAction {
+  noAutomaticTokenExpansion = true;
+
+  constructor(private key: StoredTargetKey) {
     this.run = this.run.bind(this);
   }
 
   async run(targets: Target[]): Promise<ActionReturnValue> {
     return {
       thatTargets: targets,
-      instanceReferenceTargets: targets,
+      [`${this.key}Targets`]: targets,
     };
   }
 }
diff --git a/packages/cursorless-engine/src/actions/actions.types.ts b/packages/cursorless-engine/src/actions/actions.types.ts
@@ -62,6 +62,13 @@ export interface SimpleAction {
    * @param args Extra args to command
    */
   getFinalStages?(): ModifierStage[];
+
+  /**
+   * If `true`, don't perform automatic token expansion for "<action> this" with
+   * empty cursor. Used for actions like `setImplicitTarget` that are just
+   * loading up the pipeline.
+   */
+  noAutomaticTokenExpansion?: boolean;
 }
 
 /**

diff --git a/packages/cursorless-engine/src/core/commandRunner/CommandRunnerImpl.ts b/packages/cursorless-engine/src/core/commandRunner/CommandRunnerImpl.ts
@@ -18,6 +18,7 @@ import { selectionToStoredTarget } from "./selectionToStoredTarget";
 export class CommandRunnerImpl implements CommandRunner {
   private inferenceContext: InferenceContext;
   private finalStages: ModifierStage[] = [];
+  private noAutomaticTokenExpansion: boolean | undefined;
 
   constructor(
     private debug: Debug,
@@ -179,6 +180,8 @@ export class CommandRunnerImpl implements CommandRunner {
       default: {
         const action = this.actions[actionDescriptor.name];
         this.finalStages = action.getFinalStages?.() ?? [];
+        this.noAutomaticTokenExpansion =
+          action.noAutomaticTokenExpansion ?? false;
         return action.run(this.getTargets(actionDescriptor.target));
       }
     }
@@ -191,7 +194,10 @@ export class CommandRunnerImpl implements CommandRunner {
       partialTargetsDescriptor,
     );
 
-    return this.pipelineRunner.run(targetDescriptor, this.finalStages);
+    return this.pipelineRunner.run(targetDescriptor, {
+      actionFinalStages: this.finalStages,
+      noAutomaticTokenExpansion: this.noAutomaticTokenExpansion,
+    });
   }
 
   private getDestinations(

diff --git a/packages/cursorless-engine/src/processTargets/TargetPipelineRunner.ts b/packages/cursorless-engine/src/processTargets/TargetPipelineRunner.ts
@@ -21,6 +21,11 @@ import { ImplicitStage } from "./marks/ImplicitStage";
 import { ContainingTokenIfUntypedEmptyStage } from "./modifiers/ConditionalModifierStages";
 import { PlainTarget } from "./targets";
 
+interface TargetPipelineRunnerOpts {
+  actionFinalStages?: ModifierStage[];
+  noAutomaticTokenExpansion?: boolean;
+}
+
 export class TargetPipelineRunner {
   constructor(
     private modifierStageFactory: ModifierStageFactory,
@@ -40,12 +45,18 @@ export class TargetPipelineRunner {
    * document containing it, and potentially rich context information such as
    * how to remove the target
    */
-  run(target: TargetDescriptor, actionFinalStages?: ModifierStage[]): Target[] {
+  run(
+    target: TargetDescriptor,
+    {
+      actionFinalStages = [],
+      noAutomaticTokenExpansion = false,
+    }: TargetPipelineRunnerOpts = {},
+  ): Target[] {
     return new TargetPipeline(
       this.modifierStageFactory,
       this.markStageFactory,
       target,
-      actionFinalStages ?? [],
+      { actionFinalStages, noAutomaticTokenExpansion },
     ).run();
   }
 }
@@ -55,7 +66,7 @@ class TargetPipeline {
     private modifierStageFactory: ModifierStageFactory,
     private markStageFactory: MarkStageFactory,
     private target: TargetDescriptor,
-    private actionFinalStages: ModifierStage[],
+    private opts: Required<TargetPipelineRunnerOpts>,
   ) {}
 
   /**
@@ -218,11 +229,13 @@ class TargetPipeline {
      */
     const modifierStages = [
       ...targetModifierStages,
-      ...this.actionFinalStages,
+      ...this.opts.actionFinalStages,
 
       // This performs auto-expansion to token when you say eg "take this" with an
       // empty selection
-      new ContainingTokenIfUntypedEmptyStage(this.modifierStageFactory),
+      ...(this.opts.noAutomaticTokenExpansion
+        ? []
+        : [new ContainingTokenIfUntypedEmptyStage(this.modifierStageFactory)]),
     ];
 
     // Run all targets through the modifier stages

diff --git a/packages/cursorless-engine/src/testUtil/takeSnapshot.ts b/packages/cursorless-engine/src/testUtil/takeSnapshot.ts
@@ -11,7 +11,8 @@ import {
   TestCaseSnapshot,
   TextEditor,
 } from "@cursorless/common";
-import type { StoredTargetMap } from "../core/StoredTargets";
+import { type StoredTargetMap } from "../core/StoredTargets";
+import { storedTargetKeys } from "@cursorless/common";
 
 export async function takeSnapshot(
   storedTargets: StoredTargetMap | undefined,
@@ -45,26 +46,12 @@ export async function takeSnapshot(
     snapshot.visibleRanges = editor.visibleRanges.map(rangeToPlainObject);
   }
 
-  const thatMarkTargets = storedTargets?.get("that");
-  if (thatMarkTargets != null && !excludeFields.includes("thatMark")) {
-    snapshot.thatMark = thatMarkTargets.map((target) => target.toPlainObject());
-  }
-
-  const sourceMarkTargets = storedTargets?.get("source");
-  if (sourceMarkTargets != null && !excludeFields.includes("sourceMark")) {
-    snapshot.sourceMark = sourceMarkTargets.map((target) =>
-      target.toPlainObject(),
-    );
-  }
-
-  const instanceReferenceMarkTargets = storedTargets?.get("instanceReference");
-  if (
-    instanceReferenceMarkTargets != null &&
-    !excludeFields.includes("instanceReferenceMark")
-  ) {
-    snapshot.instanceReferenceMark = instanceReferenceMarkTargets.map(
-      (target) => target.toPlainObject(),
-    );
+  for (const storedTargetKey of storedTargetKeys) {
+    const targets = storedTargets?.get(storedTargetKey);
+    const key = `${storedTargetKey}Mark` as const;
+    if (targets != null && !excludeFields.includes(key)) {
+      snapshot[key] = targets.map((target) => target.toPlainObject());
+    }
   }
 
   if (extraFields.includes("timeOffsetSeconds")) {

diff --git a/...ages/cursorless-vscode-e2e/src/suite/fixtures/recorded/actions/changeNextInstanceChar.yml b/...ages/cursorless-vscode-e2e/src/suite/fixtures/recorded/actions/changeNextInstanceChar.yml
@@ -0,0 +1,42 @@
+languageId: plaintext
+command:
+  version: 6
+  spokenForm: change next instance char
+  action:
+    name: clearAndSetSelection
+    target:
+      type: primitive
+      modifiers:
+        - type: relativeScope
+          scopeType: {type: instance}
+          offset: 1
+          length: 1
+          direction: forward
+        - type: containingScope
+          scopeType: {type: character}
+  usePrePhraseSnapshot: true
+initialState:
+  documentContents: aba aaa
+  selections:
+    - anchor: {line: 0, character: 0}
+      active: {line: 0, character: 0}
+  marks: {}
+  instanceReferenceMark:
+    - type: UntypedTarget
+      contentRange:
+        start: {line: 0, character: 0}
+        end: {line: 0, character: 0}
+      isReversed: false
+      hasExplicitRange: false
+finalState:
+  documentContents: ba aaa
+  selections:
+    - anchor: {line: 0, character: 0}
+      active: {line: 0, character: 0}
+  thatMark:
+    - type: RawSelectionTarget
+      contentRange:
+        start: {line: 0, character: 0}
+        end: {line: 0, character: 0}
+      isReversed: false
+      hasExplicitRange: true
diff --git a/packages/cursorless-vscode-e2e/src/suite/fixtures/recorded/actions/fromThis.yml b/packages/cursorless-vscode-e2e/src/suite/fixtures/recorded/actions/fromThis.yml
@@ -0,0 +1,35 @@
+languageId: plaintext
+command:
+  version: 6
+  spokenForm: from this
+  action:
+    name: experimental.setInstanceReference
+    target:
+      type: primitive
+      mark: {type: cursor}
+  usePrePhraseSnapshot: true
+initialState:
+  documentContents: aba aaa
+  selections:
+    - anchor: {line: 0, character: 0}
+      active: {line: 0, character: 0}
+  marks: {}
+finalState:
+  documentContents: aba aaa
+  selections:
+    - anchor: {line: 0, character: 0}
+      active: {line: 0, character: 0}
+  thatMark:
+    - type: UntypedTarget
+      contentRange:
+        start: {line: 0, character: 0}
+        end: {line: 0, character: 0}
+      isReversed: false
+      hasExplicitRange: false
+  instanceReferenceMark:
+    - type: UntypedTarget
+      contentRange:
+        start: {line: 0, character: 0}
+        end: {line: 0, character: 0}
+      isReversed: false
+      hasExplicitRange: false
diff --git a/packages/cursorless-vscode-e2e/src/suite/recorded.vscode.test.ts b/packages/cursorless-vscode-e2e/src/suite/recorded.vscode.test.ts
@@ -18,6 +18,7 @@ import {
   splitKey,
   SpyIDE,
   spyIDERecordedValuesToPlainObject,
+  storedTargetKeys,
   TestCaseFixtureLegacy,
   TextEditor,
   TokenHat,
@@ -89,15 +90,10 @@ async function runTest(file: string, spyIde: SpyIDE) {
 
   editor.selections = fixture.initialState.selections.map(createSelection);
 
-  setStoredTarget(editor, "that", fixture.initialState.thatMark);
-
-  setStoredTarget(editor, "source", fixture.initialState.sourceMark);
-
-  setStoredTarget(
-    editor,
-    "instanceReference",
-    fixture.initialState.instanceReferenceMark,
-  );
+  for (const storedTargetKey of storedTargetKeys) {
+    const key = `${storedTargetKey}Mark` as const;
+    setStoredTarget(editor, storedTargetKey, fixture.initialState[key]);
+  }
 
   if (fixture.initialState.clipboard) {
     vscode.env.clipboard.writeText(fixture.initialState.clipboard);
@@ -162,16 +158,11 @@ async function runTest(file: string, spyIde: SpyIDE) {
     excludeFields.push("clipboard");
   }
 
-  if (fixture.finalState?.thatMark == null) {
-    excludeFields.push("thatMark");
-  }
-
-  if (fixture.finalState?.sourceMark == null) {
-    excludeFields.push("sourceMark");
-  }
-
-  if (fixture.finalState?.instanceReferenceMark == null) {
-    excludeFields.push("instanceReferenceMark");
+  for (const storedTargetKey of storedTargetKeys) {
+    const key = `${storedTargetKey}Mark` as const;
+    if (fixture.finalState?.[key] == null) {
+      excludeFields.push(key);
+    }
   }
 
   // FIXME Visible ranges are not asserted, see: