Refactor transformations and add name transformation #56

brxck · 2021-06-30T23:55:35Z

The scope of this PR has changed a bit!

This refactors the transformations code to increase reusability and lower the cost to support new transformations (modifiers). This also adds the name, function name, and class name transformations.

NodeMatchers are split into component functions NodeFinders and SelectionExtractors. Functions have been added to combine and compose these into new NodeMatchers.

Here's the first step of #27, relatively simple.

I'd be happy to look into extending this for "funk name," "funk class," etc, but I am not entirely sure what approach to take or if there are already provisions for this sort of thing.

This includes/depends on work in #32. I wish stacked PRs were a thing in GitHub, lmk if you want to handle to this kind of PR differently.

brxck · 2021-06-30T23:57:48Z

Oh this also includes a commit which adds some logging of the types of the selected node and its ancestors. This has been pretty handy developing these transformations. If you prefer, I can easily drop that commit.

brxck · 2021-07-02T21:02:51Z

@pokey I'm still a little hung up on how to go about implementing a function name transformation.

Selecting a function in both our languages is non trivial. For example, the Typescript implementation is three cascading matchers:

  namedFunction: cascadingMatcher(
    possiblyExportedDeclaration("function_declaration", "method_definition"),
    (editor: TextEditor, node: SyntaxNode) =>
      node.type === "public_field_definition" &&
      getValueNode(node)!.type === "arrow_function"
        ? simpleSelectionExtractor(node)
        : null,
    possiblyWrappedNode(
      (node) => node.type === "export_statement",
      isNamedArrowFunction,
      (node) => [getDeclarationNode(node)]
    )
  ),

This ends up being a substantial amount of logic to extract and duplicate for this one transformation. It feels like we should have the ability to compose/extend matchers if we want to implement more things like this.

If we separated finding a node and extracting a selection, then we could compose the node finder functions. Which could look very roughly like:

import { flowRight } from "lodash";

const nodeMatchers: Record<ScopeType, NodeMatcher> = {
  functionName: flowRight([simpleSelectionExtractor, findName, findNamedFunction]),
  namedFunction: flowRight([simpleSelectionExtractor, findNamedFunction])
}

Curious what you think about this!

pokey · 2021-07-02T22:41:17Z

Sounds good to me if you can make it work! My worry is that extracting the name will be different depending which of the cascading marchers hits. For example if the matcher hits an export, it returns the whole export statement, so I believe it would then have to drill back down to get the name. I think it's simpler to avoid that because the name extractor doesn't need to try to include the export, whereas the function matcher goes to great lengths to do so. So I think the function matching process will actually be a bit different if you just want the name

But if you think you can make it work give it a shot! Otherwise I'd just copy the matchers in the cascade and tear off the stuff you don't need and get the name from there. If after that it still looks really similar to the original then yeah maybe makes sense to abstract it

Not sure if any of this made sense it's pretty late here 😅

pokey

@brxck this is awesome!! great improvement. I left a bunch of nits. I don't feel strongly on findX vs xFinder but I do think we should try to be super consistent about our policy. Also, I'd update the PR description: def no longer a simple change 😄

pokey · 2021-07-05T18:23:22Z

src/Types.ts

+}
+
+export type NodeFinder = (node: SyntaxNode) => SyntaxNode | null;


I might move the NodeMatcher above down to right above this one; it got separated in the rebase

I also might add a docstring for this type and the next one as they're important abstractions

pokey · 2021-07-05T18:25:52Z

src/extension.ts

@@ -197,6 +198,25 @@ export async function activate(context: vscode.ExtensionContext) {
    addDecorations();
  };

+  function logBranchTypes(event: vscode.TextEditorSelectionChangeEvent) {


Mind moving this one to a separate file? Like debug.ts or something?

pokey · 2021-07-05T18:28:30Z

src/extension.ts

+    const getBranch = (branch: SyntaxNode[]): SyntaxNode[] => {
+      if (branch[0].parent) {
+        return getBranch([branch[0].parent, ...branch]);
+      }
+      return branch;
+    };


whoah recursion. this one is a bit tough to follow 😅. would getAncestors be a better name? Presuming I've understood the function correctly...

I think this one might be a bit clearer without recursion too. Just iterate until the parent is null and add the nodes as you go up. I also prefer == null comparisons to boolean transformations

Also I generally try to avoid nesting functions unless it needs to capture scope

Yeah I hear you, this was mostly meant for me at the time of writing. This is probably an artifact of the fact that my first intro to programming around syntax trees was in SML & Racket 😆 I can clean this one up and move it to a separate file.

pokey · 2021-07-05T18:29:49Z

src/extension.ts

+    branch.forEach((node, i) => console.log(">".repeat(i + 1), node.type));
+    const leafText = leaf.text.replace(/\s+/g, " ").substring(0, 100);
+    console.log(">".repeat(branch.length), `"${leafText}"`);
+  }


nice. yeah this one could be useful. I think I might use console.debug for these tho

pokey · 2021-07-05T18:34:51Z

src/nodeMatchers.ts

+  return function (editor: TextEditor, initialNode: SyntaxNode) {
+    let returnNode: SyntaxNode | null = initialNode;
+    for (const finder of finders) {
+      returnNode = returnNode ? finder(returnNode) : null;


should we not short-circuit if one of the finders returns null?

I think it makes more sense to short circuit. Otherwise you could be effectively skipping a finder, which could yield the incorrect node.

For instance, take this completely made up example. We want to get the return type of a function:

returnType: composedMatcher([ findNodeOfType("function_declaration"), findNodeOfType("return_type"), getTypeNode ])

Imagine a situation where we find a declaration but we don't find anything for findNodeOfType("return_type"). If we continue to the next finder, we could match any other encountered type (like a parameter type) in the function declaration, returning an incorrect node.

ah oh right you are short-circuiting. Missed that. I think maybe would be a bit easier to understand if you just had a return statement when you get a null? Then it's really obvious you're short-cirtuiting and can remove ternary at bottom

pokey · 2021-07-05T18:47:16Z

src/languages/getPojoMatchers.ts

-        listTypes.includes(node.parent?.type ?? "") && listElementMatcher(node),
-      (node) => node.type === "," || node.type === "[" || node.type === "]",
-      ", "
+    pairKey: composedMatcher([findNodeOfType("pair"), getKeyNode]),


oooh shiny 😊

pokey · 2021-07-05T18:49:15Z

src/nodeMatchers.ts

-  nodeMatches: (node: SyntaxNode) => boolean,
-  isDelimiterNode: (node: SyntaxNode) => boolean,
-  defaultDelimiter: string
+export function composedMatcher(


hmm i tend to like variadic signatures for this type of thing, so you can say eg composedMatcher(findNodeOfType("pair"), getKeyNode) (notice no square brackets). But obv that doesn't work with this selector param. Not sure what the right answer is here. Prob fine as you have it

Yeah I would have preferred that too.

I think maybe it can actually be done (see this PR for examples), but when I tried it seemed to break type inference within the function so I backed off. I did not spend much time looking into this though.

It would look something like this (?):

export function composedMatcher( ...args: [...NodeFinder[], SelectionExtractor] )

Hmm I think that's potentially more confusing. Maybe let's just leave it

pokey · 2021-07-05T18:51:46Z

src/languages/python.ts

+    matcher(getNameNode),
+    matcher((node) => (node.type === "assignment" ? getLeftNode(node) : null))
+  ),
+  functionName: notSupported,


Would it be hard to implement this one?

I doubt it would! I think I just got distracted by what this PR turned into 😅

ha fair enough; there was a lot going on in this PR 😄

pokey · 2021-07-05T18:55:27Z

src/languages/typescript.ts

-      isNamedArrowFunction,
-      (node) => [getDeclarationNode(node)]
+    matcher(
+      findPossiblyWrappedNode(


I wonder why this one can't use possiblyExportedDeclaration 🤔. Looks like it was already this way but is a bit curious

pokey · 2021-07-05T18:57:04Z

src/languages/typescript.ts

+  statement: matcher(possiblyExportedDeclaration(...STATEMENT_TYPES)),
+  arrowFunction: typeMatcher("arrow_function"),
+  functionCall: typeMatcher("call_expression", "new_expression"),
+  functionName: cascadingMatcher(


Do you not also support name for typescript?

It's using the one defined in getPojoMatchers. Unsure if it should stay there? Currently typescript is the only language using it, but it's the most generic implementation possible.

Yeah I don't think it should be in pojoMatchers. "Pojo" stands for "plain old javascript object", so more or less should only contain json stuff. But if you want to do a rename / refactor I'm fine with it

pokey · 2021-07-06T17:19:23Z

Do we also want to implement class names as part of this PR or leave that for follow on work?

pokey

Ok looking really good! Left a couple more minor comments. Also don't forget to update PR description. Nothing fancy just it's a bit outdated

pokey · 2021-07-06T20:50:06Z

src/debug.ts

+import * as vscode from "vscode";
+import { SyntaxNode } from "web-tree-sitter";
+
+export function logBranchTypes(getNodeAtLocation: any) {


I generally try to avoid any, tho I recognize this function is just for debugging. Should be a pretty straightforward signature tho no?

Also, I'd probably opt for a class here rather than returning a function, but let's leave for now as it's just for debugging

pokey · 2021-07-06T20:52:15Z

src/debug.ts

+    const ancestors: SyntaxNode[] = [];
+    let node: SyntaxNode = getNodeAtLocation(location);
+    while (node.parent) {
+      ancestors.unshift(node.parent);
+      node = node.parent;
+    }


Much cleaner 😊. I still prefer node.parent != null, even tho slightly more verbose

Dang, I forgot about that! I will probably continue to forget about it unless an eslint rule is instituted 😅

haha yeah good point. I think this would do the trick?

pokey · 2021-07-06T20:57:38Z

src/languages/python.ts

+  functionName: composedMatcher([
+    possiblyDecoratedDefinition("function_definition"),
+    getNameNode,
+  ]),
+  className: composedMatcher([
+    possiblyDecoratedDefinition("class_definition"),
+    getNameNode,
+  ]),


well these were too easy 😂. did you try it on a decorated definition? I'd think it would not be able to find the name on the decoration node

pokey · 2021-07-06T20:58:38Z

src/languages/typescript.ts

      getNameNode,
    ]),
    composedMatcher([findClassPropertyArrowFunction, getNameNode]),
    composedMatcher([findNamedArrowFunction, getNameNode])
  ),
+  className: composedMatcher([


nice this one was pretty easy too 😊. also, did you test this one on something that was exported? also thinking it might not find the name on the export statement

pokey · 2021-07-06T20:59:18Z

src/nodeFinders.ts

-export const findNode =
-  (isTargetNode: (node: SyntaxNode) => boolean): NodeFinder =>
-  (node: SyntaxNode) => {
+export const findNode = (


Should this be nodeFinder or predicateNodeFinder or something for consistency with new naming scheme?

brxck · 2021-07-07T19:04:10Z

Thank you for the solid review @pokey! Think I've addressed everything now.

brxck force-pushed the name-transform branch from 9f6e61a to bd7dd89 Compare July 5, 2021 17:57

pokey approved these changes Jul 5, 2021

View reviewed changes

pokey added this to the First release for new users milestone Jul 6, 2021

brxck and others added 9 commits July 6, 2021 12:31

Add name field transformation

12ba0df

Add AST debug logging

79ed944

Add basic name transformation

0a96484

Refactor matchers to add function name transform

89e0c85

Add missing function name

cd7c36a

Clean up transformations refactor

b1061eb

Refactor branch debug logging

c77d9f0

Fix purple color setting name

9c3b2f1

Add function and class name scopes

3a79d5d

brxck force-pushed the name-transform branch from bd7dd89 to 3a79d5d Compare July 6, 2021 20:13

brxck requested a review from pokey July 6, 2021 20:14

pokey reviewed Jul 6, 2021

View reviewed changes

brxck changed the title ~~Add name transformation~~ Refactor transformations and add name transformation Jul 7, 2021

brxck added 2 commits July 7, 2021 12:02

Fixed name matching for exported/decorated nodes

8365b2c

Clean up transformations refactor (again)

91f4ade

pokey mentioned this pull request Jul 7, 2021

Set up strict boolean checks in linter #80

Open

pokey merged commit 99eea68 into cursorless-dev:master Jul 7, 2021

pokey mentioned this pull request Jul 9, 2021

Support "name" transformation #27

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor transformations and add name transformation #56

Refactor transformations and add name transformation #56

brxck commented Jun 30, 2021 •

edited

Loading

brxck commented Jun 30, 2021

brxck commented Jul 2, 2021

pokey commented Jul 2, 2021

pokey left a comment

pokey Jul 5, 2021

pokey Jul 5, 2021

pokey Jul 5, 2021

brxck Jul 5, 2021

pokey Jul 5, 2021

pokey Jul 5, 2021

brxck Jul 5, 2021

pokey Jul 5, 2021

pokey Jul 5, 2021

pokey Jul 5, 2021

brxck Jul 5, 2021

pokey Jul 5, 2021

pokey Jul 5, 2021

brxck Jul 5, 2021

pokey Jul 5, 2021

pokey Jul 5, 2021

pokey Jul 5, 2021

brxck Jul 6, 2021

pokey Jul 6, 2021

pokey commented Jul 6, 2021

pokey left a comment •

edited

Loading

pokey Jul 6, 2021

pokey Jul 6, 2021

brxck Jul 7, 2021 •

edited

Loading

pokey Jul 7, 2021

pokey Jul 6, 2021

pokey Jul 6, 2021

pokey Jul 6, 2021

brxck commented Jul 7, 2021

		}

		export type NodeFinder = (node: SyntaxNode) => SyntaxNode \| null;

Refactor transformations and add name transformation #56

Refactor transformations and add name transformation #56

Conversation

brxck commented Jun 30, 2021 • edited Loading

brxck commented Jun 30, 2021

brxck commented Jul 2, 2021

pokey commented Jul 2, 2021

pokey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pokey commented Jul 6, 2021

pokey left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brxck Jul 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brxck commented Jul 7, 2021

brxck commented Jun 30, 2021 •

edited

Loading

pokey left a comment •

edited

Loading

brxck Jul 7, 2021 •

edited

Loading