Properly handle trailing commas in python arguments (#2780)

Fixes #807

## 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

---------

Co-authored-by: Phil Cohen <phillip@phillip.io>
Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com>

data/fixtures/scopes/python/argument.formal2.scope

@@ -0,0 +1,49 @@
+def foo(
+    aaa,
+    bbb,
+):
+    pass
+---
+
+[#1 Content] =
+[#1 Domain] = 1:4-1:7
+      >---<
+1|     aaa,
+
+[#1 Removal] = 1:4-2:4
+      >----
+1|     aaa,
+2|     bbb,
+   ----<
+
+[#1 Trailing delimiter] = 1:7-2:4
+         >-
+1|     aaa,
+2|     bbb,
+   ----<
+
+[#1 Insertion delimiter] = ",\n"
+
+
+[#2 Content] =
+[#2 Domain] = 2:4-2:7
+      >---<
+2|     bbb,
+
+[#2 Removal] = 1:7-2:7
+         >-
+1|     aaa,
+2|     bbb,
+   -------<
+
+[#2 Leading delimiter] = 1:7-2:4
+         >-
+1|     aaa,
+2|     bbb,
+   ----<
+
+[#2 Trailing delimiter] = 2:7-2:8
+         >-<
+2|     bbb,
+
+[#2 Insertion delimiter] = ",\n"

packages/cursorless-engine/src/languages/TreeSitterQuery/validateQueryCaptures.ts

@@ -5,7 +5,14 @@ const wildcard = "_";
 const captureNames = [wildcard, ...simpleScopeTypeTypes];
 
 const positionRelationships = ["prefix", "leading", "trailing"];
-const positionSuffixes = ["startOf", "endOf"];
+const positionSuffixes = [
+  "startOf",
+  "endOf",
+  "start.startOf",
+  "start.endOf",
+  "end.startOf",
+  "end.endOf",
+];
 
 const rangeRelationships = [
   "domain",

packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/BoundedScopeHandler.ts

@@ -4,6 +4,7 @@ import type {
   ScopeType,
   TextEditor,
 } from "@cursorless/common";
+import type { Target } from "../../../typings/target.types";
 import type { InteriorTarget } from "../../targets";
 import {
   BoundedParagraphTarget,
@@ -18,7 +19,7 @@ import type {
   ScopeIteratorRequirements,
 } from "./scopeHandler.types";
 import type { ScopeHandlerFactory } from "./ScopeHandlerFactory";
-import type { Target } from "../../../typings/target.types";
+import { isEveryScopeModifier } from "./util/isHintsEveryScope";
 
 abstract class BoundedBaseScopeHandler extends BaseScopeHandler {
   protected readonly isHierarchical = true;
@@ -91,11 +92,10 @@ abstract class BoundedBaseScopeHandler extends BaseScopeHandler {
         direction,
         {
           ...hints,
-          // For every (skipAncestorScopes=true) we don't want to go outside of the surrounding pair
-          containment:
-            hints.containment == null && hints.skipAncestorScopes
-              ? "required"
-              : hints.containment,
+          // For the every scope, we don't want to go outside of the surrounding pair
+          containment: isEveryScopeModifier(hints)
+            ? "required"
+            : hints.containment,
         },
       ),
     );

packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/CollectionItemScopeHandler/CollectionItemTextualScopeHandler.ts

@@ -14,6 +14,7 @@ import type {
   ScopeIteratorRequirements,
 } from "../scopeHandler.types";
 import type { ScopeHandlerFactory } from "../ScopeHandlerFactory";
+import { isEveryScopeModifier } from "../util/isHintsEveryScope";
 import { OneWayNestedRangeFinder } from "../util/OneWayNestedRangeFinder";
 import { OneWayRangeFinder } from "../util/OneWayRangeFinder";
 import { collectionItemTextualIterationScopeHandler } from "./collectionItemTextualIterationScopeHandler";
@@ -42,7 +43,7 @@ export class CollectionItemTextualScopeHandler extends BaseScopeHandler {
     direction: Direction,
     hints: ScopeIteratorRequirements,
   ): Iterable<TargetScope> {
-    const isEveryScope = hints.containment == null && hints.skipAncestorScopes;
+    const isEveryScope = isEveryScopeModifier(hints);
     const separatorRanges = getSeparatorOccurrences(editor.document);
     const interiorRanges = getInteriorRanges(
       this.scopeHandlerFactory,

packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/CollectionItemScopeHandler/createTargetScope.ts

@@ -1,7 +1,7 @@
 import { type TextEditor, Range } from "@cursorless/common";
-import { getRangeLength } from "../../../../util/rangeUtils";
 import { ScopeTypeTarget } from "../../../targets";
 import type { TargetScope } from "../scope.types";
+import { getCollectionItemRemovalRange } from "../util/getCollectionItemRemovalRange";
 
 export function createTargetScope(
   isEveryScope: boolean,
@@ -15,22 +15,19 @@ export function createTargetScope(
     previousRange != null
       ? new Range(previousRange.end, contentRange.start)
       : undefined;
+
   const trailingDelimiterRange =
     nextRange != null
       ? new Range(contentRange.end, nextRange.start)
       : undefined;
 
-  // We have both leading and trailing delimiter ranges
-  // If the leading one is longer/more specific, prefer to use that for removal;
-  // otherwise use undefined to fallback to the default behavior (often trailing)
-  const removalRange =
-    !isEveryScope &&
-    leadingDelimiterRange != null &&
-    trailingDelimiterRange != null &&
-    getRangeLength(editor, leadingDelimiterRange) >
-      getRangeLength(editor, trailingDelimiterRange)
-      ? contentRange.union(leadingDelimiterRange)
-      : undefined;
+  const removalRange = getCollectionItemRemovalRange(
+    isEveryScope,
+    editor,
+    contentRange,
+    leadingDelimiterRange,
+    trailingDelimiterRange,
+  );
 
   const insertionDelimiter = iterationRange.isSingleLine ? ", " : ",\n";
 

packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/TreeSitterScopeHandler/BaseTreeSitterScopeHandler.ts

@@ -8,6 +8,7 @@ import { BaseScopeHandler } from "../BaseScopeHandler";
 import { compareTargetScopes } from "../compareTargetScopes";
 import type { TargetScope } from "../scope.types";
 import type { ScopeIteratorRequirements } from "../scopeHandler.types";
+import { isEveryScopeModifier } from "../util/isHintsEveryScope";
 import { getQuerySearchRange } from "./getQuerySearchRange";
 import { mergeAdjacentBy } from "./mergeAdjacentBy";
 
@@ -24,6 +25,7 @@ export abstract class BaseTreeSitterScopeHandler extends BaseScopeHandler {
     hints: ScopeIteratorRequirements,
   ): Iterable<TargetScope> {
     const { document } = editor;
+    const isEveryScope = isEveryScopeModifier(hints);
 
     /** Narrow the range within which tree-sitter searches, for performance */
     const { start, end } = getQuerySearchRange(
@@ -35,7 +37,7 @@ export abstract class BaseTreeSitterScopeHandler extends BaseScopeHandler {
 
     const scopes = this.query
       .matches(document, start, end)
-      .map((match) => this.matchToScope(editor, match))
+      .map((match) => this.matchToScope(editor, match, isEveryScope))
       .filter((scope): scope is ExtendedTargetScope => scope != null)
       .sort((a, b) => compareTargetScopes(direction, position, a, b));
 
@@ -88,12 +90,14 @@ export abstract class BaseTreeSitterScopeHandler extends BaseScopeHandler {
    * relevant to this scope handler
    * @param editor The editor in which the match was found
    * @param match The match to convert to a scope
+   * @param isEveryScope Whether the scope is being used in an "every" modifier
    * @returns The scope, or undefined if the match is not relevant to this scope
    * handler
    */
   protected abstract matchToScope(
     editor: TextEditor,
     match: QueryMatch,
+    isEveryScope: boolean,
   ): ExtendedTargetScope | undefined;
 }
 

packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/TreeSitterScopeHandler/TreeSitterIterationScopeHandler.ts

@@ -34,6 +34,7 @@ export class TreeSitterIterationScopeHandler extends BaseTreeSitterScopeHandler
   protected matchToScope(
     editor: TextEditor,
     match: QueryMatch,
+    _isEveryScope: boolean,
   ): ExtendedTargetScope | undefined {
     const scopeTypeType = this.iterateeScopeType.type;
 

packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/TreeSitterScopeHandler/TreeSitterScopeHandler.ts

@@ -3,6 +3,7 @@ import type { TreeSitterQuery } from "../../../../languages/TreeSitterQuery";
 import type { QueryMatch } from "../../../../languages/TreeSitterQuery/QueryCapture";
 import { ScopeTypeTarget } from "../../../targets/ScopeTypeTarget";
 import type { CustomScopeType } from "../scopeHandler.types";
+import { getCollectionItemRemovalRange } from "../util/getCollectionItemRemovalRange";
 import type { ExtendedTargetScope } from "./BaseTreeSitterScopeHandler";
 import { BaseTreeSitterScopeHandler } from "./BaseTreeSitterScopeHandler";
 import { TreeSitterIterationScopeHandler } from "./TreeSitterIterationScopeHandler";
@@ -36,6 +37,7 @@ export class TreeSitterScopeHandler extends BaseTreeSitterScopeHandler {
   protected matchToScope(
     editor: TextEditor,
     match: QueryMatch,
+    isEveryScope: boolean,
   ): ExtendedTargetScope | undefined {
     const scopeTypeType = this.scopeType.type;
 
@@ -51,8 +53,6 @@ export class TreeSitterScopeHandler extends BaseTreeSitterScopeHandler {
     const domain =
       getRelatedRange(match, scopeTypeType, "domain", true) ?? contentRange;
 
-    const removalRange = getRelatedRange(match, scopeTypeType, "removal", true);
-
     const interiorRange = getRelatedRange(
       match,
       scopeTypeType,
@@ -81,6 +81,22 @@ export class TreeSitterScopeHandler extends BaseTreeSitterScopeHandler {
       true,
     )?.with(contentRange.end);
 
+    let removalRange = getRelatedRange(match, scopeTypeType, "removal", true);
+
+    if (
+      removalRange == null &&
+      (scopeTypeType === "collectionItem" ||
+        scopeTypeType === "argumentOrParameter")
+    ) {
+      removalRange = getCollectionItemRemovalRange(
+        isEveryScope,
+        editor,
+        contentRange,
+        leadingDelimiterRange,
+        trailingDelimiterRange,
+      );
+    }
+
     return {
       editor,
       domain,

packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/util/getCollectionItemRemovalRange.ts

@@ -0,0 +1,29 @@
+import type { Range, TextEditor } from "@cursorless/common";
+
+import { getRangeLength } from "../../../../util/rangeUtils";
+
+/**
+ * Picks which of the leading and trailing delimiter ranges to use for removal when both are present.
+ */
+export function getCollectionItemRemovalRange(
+  isEveryScope: boolean,
+  editor: TextEditor,
+  contentRange: Range,
+  leadingDelimiterRange: Range | undefined,
+  trailingDelimiterRange: Range | undefined,
+): Range | undefined {
+  if (isEveryScope) {
+    // Force a fallback to the default behavior (often trailing)
+    return undefined;
+  }
+  // If the leading one is longer/more specific, prefer to use that for removal
+  if (
+    leadingDelimiterRange != null &&
+    trailingDelimiterRange != null &&
+    getRangeLength(editor, leadingDelimiterRange) >
+      getRangeLength(editor, trailingDelimiterRange)
+  ) {
+    return contentRange.union(leadingDelimiterRange);
+  }
+  return undefined;
+}

packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/util/isHintsEveryScope.ts

@@ -0,0 +1,10 @@
+import type { ScopeIteratorRequirements } from "../scopeHandler.types";
+
+/**
+ * Returns whether the hints belong to the every scope modifier.
+ */
+export function isEveryScopeModifier(
+  hints: ScopeIteratorRequirements,
+): boolean {
+  return hints.containment == null && hints.skipAncestorScopes;
+}

queries/python.scm

@@ -589,7 +589,9 @@
     .
     (_) @argumentOrParameter
     .
-    (_)? @_.trailing.startOf
+    ","? @_.trailing.start.endOf
+    .
+    (_)? @_.trailing.end.startOf
   ) @_dummy
   (#not-type? @argumentOrParameter "comment")
   (#single-or-multi-line-delimiter! @argumentOrParameter @_dummy ", " ",\n")
@@ -603,7 +605,9 @@
     .
     (_) @argumentOrParameter
     .
-    (_)? @_.trailing.startOf
+    ","? @_.trailing.start.endOf
+    .
+    (_)? @_.trailing.end.startOf
   ) @_dummy
   (#not-type? @argumentOrParameter "comment")
   (#single-or-multi-line-delimiter! @argumentOrParameter @_dummy ", " ",\n")