Solve bug with ambiguous closing pair delimiter (#3185)

In ruby both of these surrounding pairs are valid
`( )`
`%Q( )`

The ambiguity that two surrounding pairs have the same closing delimiter
introduced a bug.

Author: AndreasArvidsson

data/fixtures/recorded/surroundingPair/parseTree/ruby/changePair2.yml

@@ -0,0 +1,23 @@
+languageId: ruby
+command:
+  version: 7
+  spokenForm: change pair
+  action:
+    name: clearAndSetSelection
+    target:
+      type: primitive
+      modifiers:
+        - type: containingScope
+          scopeType: {type: surroundingPair, delimiter: any}
+  usePrePhraseSnapshot: false
+initialState:
+  documentContents: "%Q(hello)"
+  selections:
+    - anchor: {line: 0, character: 3}
+      active: {line: 0, character: 3}
+  marks: {}
+finalState:
+  documentContents: ""
+  selections:
+    - anchor: {line: 0, character: 0}
+      active: {line: 0, character: 0}

data/fixtures/recorded/surroundingPair/parseTree/ruby/changePair3.yml

@@ -0,0 +1,23 @@
+languageId: ruby
+command:
+  version: 7
+  spokenForm: change pair
+  action:
+    name: clearAndSetSelection
+    target:
+      type: primitive
+      modifiers:
+        - type: containingScope
+          scopeType: {type: surroundingPair, delimiter: any}
+  usePrePhraseSnapshot: false
+initialState:
+  documentContents: foo(%Q(hello))
+  selections:
+    - anchor: {line: 0, character: 4}
+      active: {line: 0, character: 4}
+  marks: {}
+finalState:
+  documentContents: foo()
+  selections:
+    - anchor: {line: 0, character: 4}
+      active: {line: 0, character: 4}

packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/SurroundingPairScopeHandler/getDelimiterOccurrences.ts

@@ -34,12 +34,12 @@ export function getDelimiterOccurrences(
     getSortedCaptures(capturesMap.textFragment),
   );
 
-  const delimiterTextToDelimiterInfoMap = Object.fromEntries(
-    individualDelimiters.map((individualDelimiter) => [
-      individualDelimiter.text,
-      individualDelimiter,
-    ]),
-  );
+  const delimiterTextToDelimiterInfoMap = individualDelimiters.reduce<
+    Record<string, IndividualDelimiter[]>
+  >((acc, individualDelimiter) => {
+    (acc[individualDelimiter.text] ??= []).push(individualDelimiter);
+    return acc;
+  }, {});
 
   const regexMatches = matchAllIterator(
     document.getText(),
@@ -64,7 +64,7 @@ export function getDelimiterOccurrences(
     }
 
     results.push({
-      delimiterInfo: delimiterTextToDelimiterInfoMap[text],
+      delimiterInfos: delimiterTextToDelimiterInfoMap[text],
       textFragmentRange: textFragments.getSmallestContaining(matchRange)?.range,
       range:
         ifNoErrors(pairDelimiters.getContaining(matchRange))?.range ??

packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/SurroundingPairScopeHandler/getSurroundingPairOccurrences.ts

@@ -1,6 +1,21 @@
 import type { Range } from "@cursorless/common";
 import findLastIndex from "lodash-es/findLastIndex";
-import type { DelimiterOccurrence, SurroundingPairOccurrence } from "./types";
+import type {
+  DelimiterOccurrence,
+  IndividualDelimiter,
+  SurroundingPairOccurrence,
+} from "./types";
+
+interface OpeningDelimiterStackOccurrence {
+  delimiterInfo: IndividualDelimiter;
+  range: Range;
+  textFragmentRange: Range | undefined;
+}
+
+interface OpeningDelimiterMatch {
+  delimiterInfo: IndividualDelimiter;
+  openingDelimiterIndex: number;
+}
 
 /**
  * Given a list of occurrences of delimiters, returns a list of occurrences of
@@ -13,50 +28,91 @@ export function getSurroundingPairOccurrences(
   delimiterOccurrences: DelimiterOccurrence[],
 ): SurroundingPairOccurrence[] {
   const result: SurroundingPairOccurrence[] = [];
-  const openingDelimitersStack: DelimiterOccurrence[] = [];
+  const openingDelimitersStack: OpeningDelimiterStackOccurrence[] = [];
 
   for (const occurrence of delimiterOccurrences) {
-    const {
-      delimiterInfo: { delimiterName, side, isSingleLine },
-      textFragmentRange,
-      range,
-    } = occurrence;
-
-    if (side === "left") {
-      openingDelimitersStack.push(occurrence);
-    } else {
-      const openingDelimiterIndex = findLastIndex(
-        openingDelimitersStack,
-        (o) =>
-          o.delimiterInfo.delimiterName === delimiterName &&
-          isSameTextFragment(o.textFragmentRange, textFragmentRange) &&
-          isValidLine(isSingleLine, o.range, range),
+    // One token can represent multiple delimiters (eg ")" could close
+    // `parentheses` or Ruby `%Q(`), so pick the best closing interpretation
+    // based on currently open delimiters.
+    const closestOpeningDelimiterMatch = getClosestOpeningDelimiterMatch(
+      occurrence,
+      openingDelimitersStack,
+    );
+
+    if (closestOpeningDelimiterMatch == null) {
+      const openingDelimiterInfo = occurrence.delimiterInfos.find(
+        ({ side }) => side === "left" || side === "unknown",
       );
 
-      if (openingDelimiterIndex === -1) {
-        // When side is unknown and we can't find an opening delimiter, that means this *is* the opening delimiter.
-        if (side === "unknown") {
-          openingDelimitersStack.push(occurrence);
-        }
+      // Pure closing delimiters with no matching opener are ignored.
+      if (openingDelimiterInfo == null) {
         continue;
       }
 
-      const openingDelimiter = openingDelimitersStack[openingDelimiterIndex];
-
-      // Pop stack up to and including the opening delimiter
-      openingDelimitersStack.length = openingDelimiterIndex;
-
-      result.push({
-        delimiterName: delimiterName,
-        openingDelimiterRange: openingDelimiter.range,
-        closingDelimiterRange: range,
+      // If this token can't close anything, treat it as an opener.
+      openingDelimitersStack.push({
+        delimiterInfo: openingDelimiterInfo,
+        range: occurrence.range,
+        textFragmentRange: occurrence.textFragmentRange,
       });
+      continue;
     }
+
+    const { delimiterInfo, openingDelimiterIndex } =
+      closestOpeningDelimiterMatch;
+    const openingDelimiter = openingDelimitersStack[openingDelimiterIndex];
+
+    // Pop stack up to and including the opening delimiter
+    openingDelimitersStack.length = openingDelimiterIndex;
+
+    result.push({
+      delimiterName: delimiterInfo.delimiterName,
+      openingDelimiterRange: openingDelimiter.range,
+      closingDelimiterRange: occurrence.range,
+    });
   }
 
   return result;
 }
 
+// When multiple interpretations are possible, choose the one whose opener is
+// closest on the stack, which preserves normal nesting behavior.
+function getClosestOpeningDelimiterMatch(
+  occurrence: DelimiterOccurrence,
+  openingDelimitersStack: OpeningDelimiterStackOccurrence[],
+): OpeningDelimiterMatch | undefined {
+  let closestMatch: OpeningDelimiterMatch | undefined;
+
+  for (const delimiterInfo of occurrence.delimiterInfos) {
+    if (delimiterInfo.side === "left") {
+      continue;
+    }
+
+    const openingDelimiterIndex = findLastIndex(
+      openingDelimitersStack,
+      (o) =>
+        o.delimiterInfo.delimiterName === delimiterInfo.delimiterName &&
+        isSameTextFragment(o.textFragmentRange, occurrence.textFragmentRange) &&
+        isValidLine(delimiterInfo.isSingleLine, o.range, occurrence.range),
+    );
+
+    // No opening delimiter found for this interpretation, so skip it
+    if (openingDelimiterIndex === -1) {
+      continue;
+    }
+
+    // If this is the closest opening delimiter so far, remember it
+    if (
+      closestMatch == null ||
+      openingDelimiterIndex > closestMatch.openingDelimiterIndex
+    ) {
+      closestMatch = { delimiterInfo, openingDelimiterIndex };
+    }
+  }
+
+  return closestMatch;
+}
+
 function isSameTextFragment(
   a: Range | undefined,
   b: Range | undefined,

packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/SurroundingPairScopeHandler/types.ts

@@ -38,9 +38,9 @@ export interface IndividualDelimiter {
  */
 export interface DelimiterOccurrence {
   /**
-   * Information about the delimiter itself
+   * Possible delimiter interpretations for this text occurrence
    */
-  delimiterInfo: IndividualDelimiter;
+  delimiterInfos: IndividualDelimiter[];
 
   /**
    * The range of the delimiter in the document