From b4c0cb71ddf7f02bdd0ade084985df0ffb6c73b7 Mon Sep 17 00:00:00 2001 From: Andreas Arvidsson Date: Sat, 21 Feb 2026 14:22:32 +0100 Subject: [PATCH 1/2] Working on bug fixes for surrounding pair --- .../parseTree/ruby/changePair2.yml | 23 ++++ .../getDelimiterOccurrences.ts | 14 +-- .../getSurroundingPairOccurrences.ts | 106 +++++++++++++----- .../SurroundingPairScopeHandler/types.ts | 4 +- 4 files changed, 110 insertions(+), 37 deletions(-) create mode 100644 data/fixtures/recorded/surroundingPair/parseTree/ruby/changePair2.yml diff --git a/data/fixtures/recorded/surroundingPair/parseTree/ruby/changePair2.yml b/data/fixtures/recorded/surroundingPair/parseTree/ruby/changePair2.yml new file mode 100644 index 0000000000..da9708eab0 --- /dev/null +++ b/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} diff --git a/packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/SurroundingPairScopeHandler/getDelimiterOccurrences.ts b/packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/SurroundingPairScopeHandler/getDelimiterOccurrences.ts index 80e8a4a1af..1ec74624e6 100644 --- a/packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/SurroundingPairScopeHandler/getDelimiterOccurrences.ts +++ b/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 + >((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 ?? diff --git a/packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/SurroundingPairScopeHandler/getSurroundingPairOccurrences.ts b/packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/SurroundingPairScopeHandler/getSurroundingPairOccurrences.ts index 7f33695a9c..20f9a0ede7 100644 --- a/packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/SurroundingPairScopeHandler/getSurroundingPairOccurrences.ts +++ b/packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/SurroundingPairScopeHandler/getSurroundingPairOccurrences.ts @@ -1,6 +1,16 @@ 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 ResolvedDelimiterOccurrence { + delimiterInfo: IndividualDelimiter; + range: Range; + textFragmentRange: Range | undefined; +} /** * Given a list of occurrences of delimiters, returns a list of occurrences of @@ -13,43 +23,45 @@ export function getSurroundingPairOccurrences( delimiterOccurrences: DelimiterOccurrence[], ): SurroundingPairOccurrence[] { const result: SurroundingPairOccurrence[] = []; - const openingDelimitersStack: DelimiterOccurrence[] = []; + const openingDelimitersStack: ResolvedDelimiterOccurrence[] = []; 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), - ); - - 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); - } - continue; - } + // 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 { delimiterInfo, openingDelimiterIndex } = + closestOpeningDelimiterMatch; const openingDelimiter = openingDelimitersStack[openingDelimiterIndex]; // Pop stack up to and including the opening delimiter openingDelimitersStack.length = openingDelimiterIndex; result.push({ - delimiterName: delimiterName, + delimiterName: delimiterInfo.delimiterName, openingDelimiterRange: openingDelimiter.range, - closingDelimiterRange: range, + closingDelimiterRange: occurrence.range, + }); + } else { + const openingDelimiterInfo = occurrence.delimiterInfos.find( + ({ side }) => side === "left" || side === "unknown", + ); + + // Pure closing delimiters with no matching opener are ignored. + if (openingDelimiterInfo == null) { + continue; + } + + // If this token can't close anything, treat it as an opener. + openingDelimitersStack.push({ + delimiterInfo: openingDelimiterInfo, + range: occurrence.range, + textFragmentRange: occurrence.textFragmentRange, }); } } @@ -57,6 +69,44 @@ export function getSurroundingPairOccurrences( return result; } +function getClosestOpeningDelimiterMatch( + occurrence: DelimiterOccurrence, + openingDelimitersStack: ResolvedDelimiterOccurrence[], +): + | { delimiterInfo: IndividualDelimiter; openingDelimiterIndex: number } + | undefined { + // When multiple interpretations are possible, choose the one whose opener is + // closest on the stack, which preserves normal nesting behavior. + return occurrence.delimiterInfos.reduce< + | { delimiterInfo: IndividualDelimiter; openingDelimiterIndex: number } + | undefined + >((closestMatch, delimiterInfo) => { + if (delimiterInfo.side === "left") { + return closestMatch; + } + + const openingDelimiterIndex = findLastIndex( + openingDelimitersStack, + (o) => + o.delimiterInfo.delimiterName === delimiterInfo.delimiterName && + isSameTextFragment(o.textFragmentRange, occurrence.textFragmentRange) && + isValidLine(delimiterInfo.isSingleLine, o.range, occurrence.range), + ); + + // Keep the existing candidate unless this interpretation found a valid + // opener that is strictly closer (larger stack index). + if ( + openingDelimiterIndex === -1 || + (closestMatch != null && + closestMatch.openingDelimiterIndex >= openingDelimiterIndex) + ) { + return closestMatch; + } + + return { delimiterInfo, openingDelimiterIndex }; + }, undefined); +} + function isSameTextFragment( a: Range | undefined, b: Range | undefined, diff --git a/packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/SurroundingPairScopeHandler/types.ts b/packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/SurroundingPairScopeHandler/types.ts index 458ee23142..520e9630fc 100644 --- a/packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/SurroundingPairScopeHandler/types.ts +++ b/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 From 249b61013074c39a26d8944923702d87b233e09d Mon Sep 17 00:00:00 2001 From: Andreas Arvidsson Date: Sat, 21 Feb 2026 14:57:41 +0100 Subject: [PATCH 2/2] Clean up --- .../parseTree/ruby/changePair3.yml | 23 ++++++ .../getSurroundingPairOccurrences.ts | 76 ++++++++++--------- 2 files changed, 64 insertions(+), 35 deletions(-) create mode 100644 data/fixtures/recorded/surroundingPair/parseTree/ruby/changePair3.yml diff --git a/data/fixtures/recorded/surroundingPair/parseTree/ruby/changePair3.yml b/data/fixtures/recorded/surroundingPair/parseTree/ruby/changePair3.yml new file mode 100644 index 0000000000..71519bff01 --- /dev/null +++ b/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} diff --git a/packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/SurroundingPairScopeHandler/getSurroundingPairOccurrences.ts b/packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/SurroundingPairScopeHandler/getSurroundingPairOccurrences.ts index 20f9a0ede7..0a031d0421 100644 --- a/packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/SurroundingPairScopeHandler/getSurroundingPairOccurrences.ts +++ b/packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/SurroundingPairScopeHandler/getSurroundingPairOccurrences.ts @@ -6,12 +6,17 @@ import type { SurroundingPairOccurrence, } from "./types"; -interface ResolvedDelimiterOccurrence { +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 * surrounding pairs by matching opening and closing delimiters. @@ -23,7 +28,7 @@ export function getSurroundingPairOccurrences( delimiterOccurrences: DelimiterOccurrence[], ): SurroundingPairOccurrence[] { const result: SurroundingPairOccurrence[] = []; - const openingDelimitersStack: ResolvedDelimiterOccurrence[] = []; + const openingDelimitersStack: OpeningDelimiterStackOccurrence[] = []; for (const occurrence of delimiterOccurrences) { // One token can represent multiple delimiters (eg ")" could close @@ -34,20 +39,7 @@ export function getSurroundingPairOccurrences( openingDelimitersStack, ); - if (closestOpeningDelimiterMatch != null) { - 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, - }); - } else { + if (closestOpeningDelimiterMatch == null) { const openingDelimiterInfo = occurrence.delimiterInfos.find( ({ side }) => side === "left" || side === "unknown", ); @@ -63,26 +55,37 @@ export function getSurroundingPairOccurrences( 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: ResolvedDelimiterOccurrence[], -): - | { delimiterInfo: IndividualDelimiter; openingDelimiterIndex: number } - | undefined { - // When multiple interpretations are possible, choose the one whose opener is - // closest on the stack, which preserves normal nesting behavior. - return occurrence.delimiterInfos.reduce< - | { delimiterInfo: IndividualDelimiter; openingDelimiterIndex: number } - | undefined - >((closestMatch, delimiterInfo) => { + openingDelimitersStack: OpeningDelimiterStackOccurrence[], +): OpeningDelimiterMatch | undefined { + let closestMatch: OpeningDelimiterMatch | undefined; + + for (const delimiterInfo of occurrence.delimiterInfos) { if (delimiterInfo.side === "left") { - return closestMatch; + continue; } const openingDelimiterIndex = findLastIndex( @@ -93,18 +96,21 @@ function getClosestOpeningDelimiterMatch( isValidLine(delimiterInfo.isSingleLine, o.range, occurrence.range), ); - // Keep the existing candidate unless this interpretation found a valid - // opener that is strictly closer (larger stack index). + // 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 ( - openingDelimiterIndex === -1 || - (closestMatch != null && - closestMatch.openingDelimiterIndex >= openingDelimiterIndex) + closestMatch == null || + openingDelimiterIndex > closestMatch.openingDelimiterIndex ) { - return closestMatch; + closestMatch = { delimiterInfo, openingDelimiterIndex }; } + } - return { delimiterInfo, openingDelimiterIndex }; - }, undefined); + return closestMatch; } function isSameTextFragment(