Support specializing snippet definitions by containing scope (#1487)

I thought this one would be a quick win, but deciding which matching
snippet should apply turned out to be complex

- Closes #802

Note that we don't support scope specialisation for custom snippets;
should be a fairly easy follow-up PR building on this machinery.

## Checklist

- [ ] 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

Author: pokey

cursorless-snippets/functionDeclaration.cursorless-snippets

@@ -21,6 +21,32 @@
           }
         }
       },
+      {
+        "scope": {
+          "langIds": [
+            "typescript",
+            "typescriptreact",
+            "javascript",
+            "javascriptreact"
+          ],
+          "scopeTypes": [
+            "class"
+          ],
+          "excludeDescendantScopeTypes": [
+            "namedFunction"
+          ]
+        },
+        "body": [
+          "$name($parameterList) {",
+          "\t$body",
+          "}"
+        ],
+        "variables": {
+          "name": {
+            "formatter": "camelCase"
+          }
+        }
+      },
       {
         "scope": {
           "langIds": [
@@ -36,6 +62,28 @@
             "formatter": "snakeCase"
           }
         }
+      },
+      {
+        "scope": {
+          "langIds": [
+            "python"
+          ],
+          "scopeTypes": [
+            "class"
+          ],
+          "excludeDescendantScopeTypes": [
+            "namedFunction"
+          ]
+        },
+        "body": [
+          "def $name(self${parameterList:, }):",
+          "\t$body"
+        ],
+        "variables": {
+          "name": {
+            "formatter": "snakeCase"
+          }
+        }
       }
     ],
     "description": "Function declaration",

packages/common/src/types/snippet.types.ts

@@ -13,6 +13,14 @@ export interface SnippetScope {
    * global scope.
    */
   scopeTypes?: SimpleScopeTypeType[];
+
+  /**
+   * Exclude regions of {@link scopeTypes} that are descendants of these scope
+   * types. For example, if you have a snippet that should be active in a class
+   * but not in a function within the class, you can specify
+   * `scopeTypes: ["class"], excludeDescendantScopeTypes: ["namedFunction"]`.
+   */
+  excludeDescendantScopeTypes?: SimpleScopeTypeType[];
 }
 
 export type SnippetBody = string[];

packages/cursorless-engine/src/actions/InsertSnippet.ts

@@ -23,6 +23,7 @@ import { Target } from "../typings/target.types";
 import { ensureSingleEditor } from "../util/targetUtils";
 import { Actions } from "./Actions";
 import { Action, ActionReturnValue } from "./actions.types";
+import { UntypedTarget } from "../processTargets/targets";
 
 interface NamedSnippetArg {
   type: "named";
@@ -94,6 +95,7 @@ export default class InsertSnippet implements Action {
       const snippet = this.snippets.getSnippetStrict(name);
 
       const definition = findMatchingSnippetDefinitionStrict(
+        this.modifierStageFactory,
         targets,
         snippet.definitions,
       );
@@ -124,9 +126,28 @@ export default class InsertSnippet implements Action {
   ): Promise<ActionReturnValue> {
     const editor = ide().getEditableTextEditor(ensureSingleEditor(targets));
 
+    await this.actions.editNew.run([targets]);
+
+    const targetSelectionInfos = editor.selections.map((selection) =>
+      getSelectionInfo(
+        editor.document,
+        selection,
+        RangeExpansionBehavior.openOpen,
+      ),
+    );
     const { body, formatSubstitutions } = this.getSnippetInfo(
       snippetDescription,
-      targets,
+      // Use new selection locations instead of original targets because
+      // that's where we'll be doing the snippet insertion
+      editor.selections.map(
+        (selection) =>
+          new UntypedTarget({
+            editor,
+            contentRange: selection,
+            isReversed: false,
+            hasExplicitRange: true,
+          }),
+      ),
     );
 
     const parsedSnippet = this.snippetParser.parse(body);
@@ -139,16 +160,6 @@ export default class InsertSnippet implements Action {
 
     const snippetString = parsedSnippet.toTextmateString();
 
-    await this.actions.editNew.run([targets]);
-
-    const targetSelectionInfos = editor.selections.map((selection) =>
-      getSelectionInfo(
-        editor.document,
-        selection,
-        RangeExpansionBehavior.openOpen,
-      ),
-    );
-
     // NB: We used the command "editor.action.insertSnippet" instead of calling editor.insertSnippet
     // because the latter doesn't support special variables like CLIPBOARD
     const [updatedTargetSelections] = await callFunctionAndUpdateSelectionInfos(

packages/cursorless-engine/src/actions/WrapWithSnippet.ts

@@ -86,6 +86,7 @@ export default class WrapWithSnippet implements Action {
       const snippet = this.snippets.getSnippetStrict(name);
 
       const definition = findMatchingSnippetDefinitionStrict(
+        this.modifierStageFactory,
         targets,
         snippet.definitions,
       );

packages/cursorless-engine/src/core/Snippets.ts

@@ -1,9 +1,10 @@
 import { showError, Snippet, SnippetMap, walkFiles } from "@cursorless/common";
 import { readFile, stat } from "fs/promises";
-import { cloneDeep, max, merge } from "lodash";
+import { max } from "lodash";
 import { join } from "path";
 import { ide } from "../singletons/ide.singleton";
 import { mergeStrict } from "../util/object";
+import { mergeSnippets } from "./mergeSnippets";
 
 const CURSORLESS_SNIPPETS_SUFFIX = ".cursorless-snippets";
 const SNIPPET_DIR_REFRESH_INTERVAL_MS = 1000;
@@ -21,7 +22,7 @@ interface DirectoryErrorMessage {
 export class Snippets {
   private coreSnippets!: SnippetMap;
   private thirdPartySnippets: Record<string, SnippetMap> = {};
-  private userSnippets!: SnippetMap;
+  private userSnippets!: SnippetMap[];
 
   private mergedSnippets!: SnippetMap;
 
@@ -133,7 +134,7 @@ export class Snippets {
         };
       }
 
-      this.userSnippets = {};
+      this.userSnippets = [];
       this.mergeSnippets();
 
       return;
@@ -154,34 +155,32 @@ export class Snippets {
 
     this.maxSnippetMtimeMs = maxSnippetMtime;
 
-    this.userSnippets = mergeStrict(
-      ...(await Promise.all(
-        snippetFiles.map(async (path) => {
-          try {
-            const content = await readFile(path, "utf8");
-
-            if (content.length === 0) {
-              // Gracefully handle an empty file
-              return {};
-            }
-
-            return JSON.parse(content);
-          } catch (err) {
-            showError(
-              ide().messages,
-              "snippetsFileError",
-              `Error with cursorless snippets file "${path}": ${
-                (err as Error).message
-              }`,
-            );
-
-            // We don't want snippets from all files to stop working if there is
-            // a parse error in one file, so we just effectively ignore this file
-            // once we've shown an error message
+    this.userSnippets = await Promise.all(
+      snippetFiles.map(async (path) => {
+        try {
+          const content = await readFile(path, "utf8");
+
+          if (content.length === 0) {
+            // Gracefully handle an empty file
             return {};
           }
-        }),
-      )),
+
+          return JSON.parse(content);
+        } catch (err) {
+          showError(
+            ide().messages,
+            "snippetsFileError",
+            `Error with cursorless snippets file "${path}": ${
+              (err as Error).message
+            }`,
+          );
+
+          // We don't want snippets from all files to stop working if there is
+          // a parse error in one file, so we just effectively ignore this file
+          // once we've shown an error message
+          return {};
+        }
+      }),
     );
 
     this.mergeSnippets();
@@ -206,34 +205,11 @@ export class Snippets {
    * party > core.
    */
   private mergeSnippets() {
-    this.mergedSnippets = {};
-
-    // We make a list of all entries from all sources, in order of increasing
-    // precedence: user > third party > core.
-    const entries = [
-      ...Object.entries(cloneDeep(this.coreSnippets)),
-      ...Object.values(this.thirdPartySnippets).flatMap((snippets) =>
-        Object.entries(cloneDeep(snippets)),
-      ),
-      ...Object.entries(cloneDeep(this.userSnippets)),
-    ];
-
-    entries.forEach(([key, value]) => {
-      if (Object.prototype.hasOwnProperty.call(this.mergedSnippets, key)) {
-        const { definitions, ...rest } = value;
-        const mergedSnippet = this.mergedSnippets[key];
-
-        // NB: We make sure that the new definitions appear before the previous
-        // ones so that they take precedence
-        mergedSnippet.definitions = definitions.concat(
-          ...mergedSnippet.definitions,
-        );
-
-        merge(mergedSnippet, rest);
-      } else {
-        this.mergedSnippets[key] = value;
-      }
-    });
+    this.mergedSnippets = mergeSnippets(
+      this.coreSnippets,
+      this.thirdPartySnippets,
+      this.userSnippets,
+    );
   }
 
   /**

packages/cursorless-engine/src/core/compareSnippetDefinitions.ts

@@ -0,0 +1,95 @@
+import {
+  SimpleScopeTypeType,
+  SnippetDefinition,
+  SnippetScope,
+} from "@cursorless/common";
+import { SnippetOrigin } from "./mergeSnippets";
+
+/**
+ * Compares two snippet definitions by how specific their scope, breaking
+ * ties by origin.
+ * @param a One of the snippet definitions to compare
+ * @param b The other snippet definition to compare
+ * @returns A negative number if a should come before b, a positive number if b
+ */
+export function compareSnippetDefinitions(
+  a: SnippetDefinitionWithOrigin,
+  b: SnippetDefinitionWithOrigin,
+): number {
+  const scopeComparision = compareSnippetScopes(
+    a.definition.scope,
+    b.definition.scope,
+  );
+
+  // Prefer the more specific snippet definitino, no matter the origin
+  if (scopeComparision !== 0) {
+    return scopeComparision;
+  }
+
+  // If the scopes are the same, prefer the snippet from the higher priority
+  // origin
+  return a.origin - b.origin;
+}
+
+function compareSnippetScopes(
+  a: SnippetScope | undefined,
+  b: SnippetScope | undefined,
+): number {
+  if (a == null && b == null) {
+    return 0;
+  }
+
+  // Prefer the snippet that has a scope at all
+  if (a == null) {
+    return -1;
+  }
+
+  if (b == null) {
+    return 1;
+  }
+
+  // Prefer the snippet that is language-specific, regardless of scope type
+  if (a.langIds == null && b.langIds != null) {
+    return -1;
+  }
+
+  if (b.langIds == null && a.langIds != null) {
+    return 1;
+  }
+
+  // If both snippets are language-specific, prefer the snippet that specifies
+  // scope types.  Note that this holds even if one snippet specifies more
+  // languages than the other.  The motivating use case is if you have a snippet
+  // for functions in js and ts, and a snippet for methods in js and ts.  If you
+  // override the function snippet for ts, you still want the method snippet to
+  // be used for ts methods.
+  const scopeTypesComparision = compareScopeTypes(a.scopeTypes, b.scopeTypes);
+
+  if (scopeTypesComparision !== 0) {
+    return scopeTypesComparision;
+  }
+
+  // If snippets both have scope types or both don't have scope types, prefer
+  // the snippet that specifies fewer languages
+  return a.langIds == null ? 0 : b.langIds!.length - a.langIds.length;
+}
+
+function compareScopeTypes(
+  a: SimpleScopeTypeType[] | undefined,
+  b: SimpleScopeTypeType[] | undefined,
+): number {
+  if (a == null && b != null) {
+    return -1;
+  }
+
+  if (b == null && a != null) {
+    return 1;
+  }
+
+  return 0;
+}
+
+interface SnippetDefinitionWithOrigin {
+  origin: SnippetOrigin;
+  definition: SnippetDefinition;
+}