diff --git a/packages/common/src/types/Position.ts b/packages/common/src/types/Position.ts index 79f4d9063..e4b423218 100644 --- a/packages/common/src/types/Position.ts +++ b/packages/common/src/types/Position.ts @@ -138,4 +138,16 @@ export class Position { public toEmptyRange(): Range { return new Range(this, this); } + + /** + * Return a concise string representation of the position. + * @returns concise representation + **/ + public concise(): string { + return `${this.line}:${this.character}`; + } + + public toString(): string { + return this.concise(); + } } diff --git a/packages/common/src/types/Range.ts b/packages/common/src/types/Range.ts index 55ea3b402..b0ffe057f 100644 --- a/packages/common/src/types/Range.ts +++ b/packages/common/src/types/Range.ts @@ -146,4 +146,16 @@ export class Range { ? new Selection(this.end, this.start) : new Selection(this.start, this.end); } + + /** + * Return a concise string representation of the range + * @returns concise representation + **/ + public concise(): string { + return `${this.start.concise()}-${this.end.concise()}`; + } + + public toString(): string { + return this.concise(); + } } diff --git a/packages/common/src/types/Selection.ts b/packages/common/src/types/Selection.ts index f54ac0678..a202c4725 100644 --- a/packages/common/src/types/Selection.ts +++ b/packages/common/src/types/Selection.ts @@ -72,4 +72,16 @@ export class Selection extends Range { this.anchor.isEqual(other.anchor) && this.active.isEqual(other.active) ); } + + /** + * Return a concise string representation of the selection + * @returns concise representation + **/ + public concise(): string { + return `${this.anchor.concise()}->${this.active.concise()}`; + } + + public toString(): string { + return this.concise(); + } } diff --git a/packages/cursorless-engine/package.json b/packages/cursorless-engine/package.json index 717d13ad2..27cd57db2 100644 --- a/packages/cursorless-engine/package.json +++ b/packages/cursorless-engine/package.json @@ -27,6 +27,7 @@ "@types/mocha": "^8.0.4", "@types/sbd": "^1.0.3", "@types/sinon": "^10.0.2", + "fast-check": "3.12.0", "js-yaml": "^4.1.0", "mocha": "^10.2.0", "sinon": "^11.1.1" diff --git a/packages/cursorless-engine/src/processTargets/TargetPipelineRunner.ts b/packages/cursorless-engine/src/processTargets/TargetPipelineRunner.ts index c5f147d8b..407d6b93a 100644 --- a/packages/cursorless-engine/src/processTargets/TargetPipelineRunner.ts +++ b/packages/cursorless-engine/src/processTargets/TargetPipelineRunner.ts @@ -5,7 +5,7 @@ import { Range, ScopeType, } from "@cursorless/common"; -import { uniqWith, zip } from "lodash"; +import { zip } from "lodash"; import { PrimitiveTargetDescriptor, RangeTargetDescriptor, @@ -18,6 +18,7 @@ import { MarkStage, ModifierStage } from "./PipelineStages.types"; import ImplicitStage from "./marks/ImplicitStage"; import { ContainingTokenIfUntypedEmptyStage } from "./modifiers/ConditionalModifierStages"; import { PlainTarget } from "./targets"; +import { uniqWithHash } from "../util/uniqWithHash"; export class TargetPipelineRunner { constructor( @@ -287,7 +288,11 @@ function calcIsReversed(anchor: Target, active: Target) { } function uniqTargets(array: Target[]): Target[] { - return uniqWith(array, (a, b) => a.isEqual(b)); + return uniqWithHash( + array, + (a, b) => a.isEqual(b), + (a) => a.contentRange.concise(), + ); } function ensureSingleEditor(anchorTarget: Target, activeTarget: Target) { diff --git a/packages/cursorless-engine/src/util/setSelectionsAndFocusEditor.ts b/packages/cursorless-engine/src/util/setSelectionsAndFocusEditor.ts index 2b8b318a2..404daefc5 100644 --- a/packages/cursorless-engine/src/util/setSelectionsAndFocusEditor.ts +++ b/packages/cursorless-engine/src/util/setSelectionsAndFocusEditor.ts @@ -1,6 +1,6 @@ import { EditableTextEditor, Selection } from "@cursorless/common"; -import uniqDeep from "./uniqDeep"; +import { uniqWithHash } from "./uniqWithHash"; export async function setSelectionsAndFocusEditor( editor: EditableTextEditor, @@ -22,5 +22,9 @@ export function setSelectionsWithoutFocusingEditor( editor: EditableTextEditor, selections: Selection[], ) { - editor.selections = uniqDeep(selections); + editor.selections = uniqWithHash( + selections, + (a, b) => a.isEqual(b), + (s) => s.concise(), + ); } diff --git a/packages/cursorless-engine/src/util/uniqDeep.ts b/packages/cursorless-engine/src/util/uniqDeep.ts deleted file mode 100644 index 182226a01..000000000 --- a/packages/cursorless-engine/src/util/uniqDeep.ts +++ /dev/null @@ -1,5 +0,0 @@ -import { uniqWith, isEqual } from "lodash"; - -export default (array: T[]): T[] => { - return uniqWith(array, isEqual); -}; diff --git a/packages/cursorless-engine/src/util/uniqWithHash.test.ts b/packages/cursorless-engine/src/util/uniqWithHash.test.ts new file mode 100644 index 000000000..5e6be110e --- /dev/null +++ b/packages/cursorless-engine/src/util/uniqWithHash.test.ts @@ -0,0 +1,65 @@ +import * as assert from "assert"; +import * as fc from "fast-check"; +import { uniqWith } from "lodash"; +import { uniqWithHash } from "./uniqWithHash"; + +// known good but slow (quadratic!) +function knownGoodUniqWithHash( + array: T[], + fn: (a: T, b: T) => boolean, + _: (t: T) => string, +): T[] { + return uniqWith(array, fn); +} + +suite("uniqWithHash", () => { + test("uniqWithHash", () => { + // believe it or not, all these test cases are important + const testCases: number[][] = [ + [], + [0], + [1], + [0, 1], + [1, 0], + [0, 0], + [1, 1], + [0, 1, 0, 1], + [1, 0, 1, 0], + [0, 1, 1, 0], + [1, 0, 0, 1], + [0, 1, 2, 3], + [0, 1, 2, 3, 0, 1, 2, 3], + [0, 0, 1, 1, 2, 2, 3, 3], + [0, 1, 2, 3, 0, 1, 2, 3, 0, 1, 2, 3], + [0, 0, 1, 1, 2, 2, 3, 3, 0, 0, 1, 1, 2, 2, 3, 3], + [0, 1, 2, 3, 4, 5, 0, 1, 2, 3], + [0, 1, 2, 3, 4, 5, 0, 1, 2, 3, 0, 1, 2, 3], + [0, 0, 1, 1, 2, 2, 3, 3, 4, 4], + [0, 0, 1, 1, 2, 2, 3, 3, 4, 4, 0, 0, 1, 1, 2, 2, 3, 3, 4, 4], + ]; + + const hashFunctions = [ + (x: number) => x.toString(), + (x: number) => (x % 2).toString(), + (_: number) => "0", + ]; + + hashFunctions.forEach((hash) => { + const check = (testCase: number[]) => { + const actual = uniqWithHash(testCase, (a, b) => a === b, hash); + const expected = knownGoodUniqWithHash( + testCase, + (a, b) => a === b, + hash, + ); + assert.deepStrictEqual(actual, expected); + }; + + testCases.forEach(check); + + // max length 50 because the known good implementation is quadratic + const randomNumbers = fc.array(fc.integer(), { maxLength: 50 }); + fc.assert(fc.property(randomNumbers, check)); + }); + }); +}); diff --git a/packages/cursorless-engine/src/util/uniqWithHash.ts b/packages/cursorless-engine/src/util/uniqWithHash.ts new file mode 100644 index 000000000..16221ce7b --- /dev/null +++ b/packages/cursorless-engine/src/util/uniqWithHash.ts @@ -0,0 +1,81 @@ +import { uniqWith } from "lodash"; + +/** + * Like lodash.uniqWith, but uses a hash function to (mostly) avoid quadratic runtime. + * @param array The array to uniq + * @param isEqual The equality function + * @param hash The hash function. It must be a valid hash function, insofar as it must return the same value for equal items. A hash function that returns a constant value is equivalent to lodash.uniqWith. + * @returns The uniq array + */ +export function uniqWithHash( + array: T[], + isEqual: (a: T, b: T) => boolean, + hash: (t: T) => string, +): T[] { + // Handle the common, tiny cases without allocating anything extra. + if (array.length < 2) { + return [...array]; + } + if (array.length === 2) { + if (isEqual(array[0], array[1])) { + return [array[0]]; + } + return [...array]; + } + // First, split up the array using the hash function. + // This keeps the sets of items passed to uniqWith small, + // so that the quadratic runtime of uniqWith less of a problem. + + /* Keep track of which sets have multiple items, so that we can uniq them. */ + const needsUniq: string[] = []; + const hashToItems: Map = array.reduce((acc, item) => { + const key = hash(item); + const items = acc.get(key); + if (items == null) { + acc.set(key, [item]); + return acc; + } + + acc.get(key)!.push(item); + if (items.length === 2) { + needsUniq.push(key); + } + return acc; + }, new Map()); + + // Another common case: Everything is unique. + if (needsUniq.length === 0) { + return [...array]; + } + + // For hash collisions, uniq the items, + // letting uniqWith provide correct semantics. + needsUniq.forEach((key) => { + hashToItems.set(key, uniqWith(hashToItems.get(key)!, isEqual)); + }); + + // To preserve order, step through the original items + // one at a time, returning it as appropriate. + // We need to do this because uniqWith preserves order, + // and we are mimicking its semantics. + // Note that items were added in order, + // and uniqWith preserved that order. + return array.flatMap((item) => { + const key = hash(item); + const items = hashToItems.get(key)!; + if (items == null || items.length === 0) { + // Removed by uniqWith. + return []; + } + const first = items[0]!; + if (!isEqual(first, item)) { + // Removed by uniqWith. + // Note that it is sufficient to check the first item, + // because uniqWith preserves order. + return []; + } + // Emit item. + items.shift(); + return first; + }); +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index d56301c47..0aabb70b6 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -257,6 +257,9 @@ importers: '@types/sinon': specifier: ^10.0.2 version: 10.0.13 + fast-check: + specifier: 3.12.0 + version: 3.12.0 js-yaml: specifier: ^4.1.0 version: 4.1.0 @@ -8090,6 +8093,13 @@ packages: iconv-lite: 0.4.24 tmp: 0.0.33 + /fast-check@3.12.0: + resolution: {integrity: sha512-SqahE9mlL3+lhjJ39joMLwcj6F+24hfZdf/tchlNO8sHcTdrUUdA5P/ZbSFZM9Xpzs36XaneGwE0FWepm/zyOA==} + engines: {node: '>=8.0.0'} + dependencies: + pure-rand: 6.0.0 + dev: true + /fast-deep-equal@3.1.3: resolution: {integrity: sha512-f3qQ9oQy9j2AhBe/H9VC91wLmKBCCU/gDOnKNAYG5hswO7BLKj09Hc5HYNz9cGI++xlpDCIgDaitVs03ATR84Q==}