migrate Go keys and values to treesitter #1839

josharian · 2023-09-01T00:31:33Z

This doesn't handle all uses of key and value.

Missing still are assignment statements (name, value),
function signatures (name, value), and possibly other
places (perhaps type declarations?).

It does handle all existing uses of key, value, and name,
though, so we can start here.

The handling of multiple return values is complicated,
particularly around the handling of comments and removal ranges.
It would be nice to find a simpler approach.

Checklist

I have added tests
I have updated the docs and cheatsheet
I have not broken the cheatsheet

This doesn't handle all uses of key, value, and name. Missing still are migrate Go key and value scope to treesitter This doesn't handle all uses of key and value. Missing still are assignment statements (name, value), function signatures (name, value), and possibly other places (perhaps type declarations?). It does handle all existing uses of key, value, and name, though, so we can start here. The handling of multiple return values is complicated, particularly around the handling of comments and removal ranges. It would be nice to find a simpler approach.

josharian · 2023-09-01T00:33:59Z

I haven't added any tests yet. Before I did that, I wanted to get a round of feedback about the behavior, so that I don't waste time adding tests with the wrong behavior.

There are a lot of tests to add.

To make it easier to get initial feedback I have included two videos of the scope visualizer. They go by fast but you can pause as needed. :)

A lot of the same considerations about comments, only-one vs many (and for many, first-vs-rest), removal ranges, etc. apply to the other uses of name and value, so it'd be good to get happy with the behavior and implementation before I continue with those.

(A surprising amount of development time is packed into not very many lines with .scm files. :P)

josharian · 2023-09-01T00:34:12Z

return values:

visualize_value.mov

josharian · 2023-09-01T00:36:37Z

keys and values in maps and lists:

visualize_key_value.mov

pokey · 2023-09-04T11:07:29Z

return values:

visualize_value.mov

Looking pretty good! One thing we're doing in other places is to have a scope that is the parent of the other scopes, whose domain is the entire statement and whose range is from the first return value to the last. See eg the name scope in .scm, and the comment proposing that behaviour #1631 (comment). Note that we only have that parent scope when there is more than one return value, otherwise you get useless nesting

Also, it's tough to tell what your iteration scopes are actually doing, but I think we'd want one iteration scope for iterating the values of one return statement when it has more than one return value, and a bigger iteration scope consisting of the function body

pokey · 2023-09-04T11:08:47Z

keys and values in maps and lists:

visualize_key_value.mov

this looks great!

pokey

Solid direction; left a bunch of minor inline comments, and see also my responses to your videos above 👍

pokey · 2023-09-01T07:29:39Z

packages/cursorless-engine/src/languages/TreeSitterQuery/checkCaptureStartEnd.ts

@@ -60,7 +60,9 @@ export function checkCaptureStartEnd(
    showError(
      messages,
      "TreeSitterQuery.checkCaptures.mixRegularStartEnd",
-      `Please do not mix regular captures and start/end captures: ${captures}`,
+      `Please do not mix regular captures and start/end captures: ${captures.map(
+        ({ name, range }) => name + "@" + range.toString(),


Does the conversion to string not happen automatically?

pokey · 2023-09-01T14:48:39Z

packages/cursorless-engine/src/languages/TreeSitterQuery/checkCaptureStartEnd.ts

@@ -71,7 +73,7 @@ export function checkCaptureStartEnd(
      messages,
      "TreeSitterQuery.checkCaptures.duplicate",
      `A capture with the same name may only appear once in a single pattern: ${captures.map(
-        ({ name }) => name,
+        ({ name, range }) => name + "@" + range.toString(),


pokey · 2023-09-01T14:52:13Z

queries/go.scm

+    (_) @value @value.trailing.start.endOf
+    (#insertion-delimiter! @value ", ")
+    .
+    (comment)* @value.trailing.omit


what does this .omit do? Is this some hypothetical capture modifier you'd like to support in the future?

pokey · 2023-09-04T11:09:39Z

packages/cursorless-engine/src/languages/TreeSitterQuery/queryPredicateOperators.ts

+ * @foo bar)` will accept the match if the `@foo` capture has 2 or more children
+ * not of type `bar`.
+ */
+class HasMultipleChildrenNotOfType extends QueryPredicateOperator<HasMultipleChildrenNotOfType> {


Wow this really feels like a slippery slope 😅. Getting to the point where I wonder if we want to create our own mini-language

dsl ftw! :D

pokey · 2023-09-04T11:12:48Z

queries/go.scm

+  (expression_list
+    .
+    (_)
+    .
+  ) @value


what happens with comments here?

pokey · 2023-09-04T11:17:43Z

queries/go.scm

+;; BUG: in this code:
+;;   return "lorem" /* comment */ , "ipsum"
+;; the comment is included in the removal range of "lorem".


is this a bug? I might argue the comment is part of "lorem", tho as mentioned above, I'd lean towards being conservative about deleting people's comments

pokey · 2023-09-04T11:18:57Z

queries/go.scm

+  (expression_list
+    .
+    (_) @value @value.trailing.start.endOf
+    (#insertion-delimiter! @value ", ")


interesting to put predicate right after the capture itself

pokey · 2023-09-04T11:20:21Z

queries/go.scm

+    ","
+    .
+    (_) @value.trailing.end.startOf
+  ) @_exprlist @value.leading.start.startOf


the _ prefix is your convention for things that are just dummy captures used for predicates? We've been using dummy, but I kinda like this _ prefix convention. wdyt @AndreasArvidsson

Sounds good to me

pokey · 2023-09-04T11:21:53Z

queries/go.scm

+    (_) @value.trailing.end.startOf
+  ) @_exprlist @value.leading.start.startOf
+  (#not-type? @value comment)
+  (#has-multiple-children-not-of-type? @_exprlist comment)


it's a bit annoying we need to use negative specifier here rather than specifying the type itself, but I'm assuming the individual expressions have no wrapper nodes of uniform type, so I'm not sure what else we can do 🤷‍♂️

pokey · 2023-09-04T11:22:45Z

queries/go.scm

+  (#has-multiple-children-not-of-type? @_exprlist comment)
+) @value.iteration
+
+;; ...and the rest of the values


would be nice to have a comment explaining why you need to handle first / rest separately

pokey · 2023-09-09T16:12:45Z

queries/go.scm

+    (_) @collectionKey.trailing.end.startOf
+  ) @collectionKey.domain
+  "}" @collectionKey.iteration.end.startOf
+)


I have mixed feelings about defining a scope’s pattern and its iteration scope’s pattern in one go. I think it's often clearer to define them separately. You could arguably define collectionKey.iteration and @value.iteration in one pattern. I think that might be clearer

That being said, it's more of a rule of thumb than a hard rule; if you feel defining collectionKey and collectionKey.iteration in the same pattern is clearer in this case, I don't have a strong objection

There is a slight performance implication as well fwiw, because this pattern matches once per collectionKey scope instance, and thus will yield the iteration scope once per scope instance, and then we silently merge them later. Defining the pattern separately will only yield the iteration scope once

josharian · 2023-10-31T23:28:42Z

I need to take another pass over this, marking as draft in the interim.

josharian requested a review from pokey as a code owner September 1, 2023 00:31

pokey reviewed Sep 4, 2023

View reviewed changes

pokey mentioned this pull request Sep 4, 2023

Elixir support #1844

Open

11 tasks

auscompgeek added lang-go Issues related to Go programming language support scope-migration Migrating scopes to next-gen scope implementation labels Sep 9, 2023

pokey reviewed Sep 9, 2023

View reviewed changes

josharian marked this pull request as draft October 31, 2023 23:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate Go keys and values to treesitter #1839

migrate Go keys and values to treesitter #1839

josharian commented Sep 1, 2023 •

edited

josharian commented Sep 1, 2023 •

edited

josharian commented Sep 1, 2023

josharian commented Sep 1, 2023

pokey commented Sep 4, 2023

pokey commented Sep 4, 2023

pokey left a comment

pokey Sep 1, 2023

pokey Sep 1, 2023

pokey Sep 1, 2023

pokey Sep 4, 2023

AndreasArvidsson Sep 4, 2023

pokey Sep 4, 2023

pokey Sep 4, 2023

pokey Sep 4, 2023

pokey Sep 4, 2023

AndreasArvidsson Sep 4, 2023

pokey Sep 4, 2023

pokey Sep 4, 2023

pokey Sep 9, 2023 •

edited

josharian commented Oct 31, 2023

migrate Go keys and values to treesitter #1839

Are you sure you want to change the base?

migrate Go keys and values to treesitter #1839

Conversation

josharian commented Sep 1, 2023 • edited

Checklist

josharian commented Sep 1, 2023 • edited

josharian commented Sep 1, 2023

josharian commented Sep 1, 2023

pokey commented Sep 4, 2023

pokey commented Sep 4, 2023

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

pokey Sep 9, 2023 • edited

Choose a reason for hiding this comment

josharian commented Oct 31, 2023

josharian commented Sep 1, 2023 •

edited

josharian commented Sep 1, 2023 •

edited

pokey Sep 9, 2023 •

edited