Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

migrate Go keys and values to treesitter #1839

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions data/playground/go/values.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package p

func returnZero() {
return
}

func returnOne() string {
return "lorem"
}

func returnOneWithCommentBefore() string {
return /* comment */ "lorem"
}

func returnOneWithCommentAfter() string {
return "lorem" /* comment */
}


func returnTwo() (string, string) {
return "lorem", "ipsum"
}

func returnTwoWithCommentInside() (string, string) {
return "lorem" /* comment */, "ipsum"
}

func returnTwoWithCommentBefore() (string, string) {
return /* comment */ "lorem", "ipsum"
}

func returnTwoWithCommentAfter() (string, string) {
return "lorem", "ipsum" /* comment */
}


func returnThree() (string, string, string) {
return "lorem", "ipsum", "blah"
}

func returnThreeWithCommentInside() (string, string, string) {
return "lorem" /* comment */, "ipsum", "blah"
}

func returnThreeWithCommentInside2() (string, string, string) {
return "lorem", "ipsum" /* comment */, "blah"
}

func returnThreeWithCommentBefore() (string, string, string) {
return /* comment */ "lorem", "ipsum", "blah"
}

func returnThreeWithCommentAfter() (string, string, string) {
return "lorem", "ipsum", "blah" /* comment */
}
12 changes: 12 additions & 0 deletions packages/common/src/types/Position.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
12 changes: 12 additions & 0 deletions packages/common/src/types/Range.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
12 changes: 12 additions & 0 deletions packages/common/src/types/Selection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the conversion to string not happen automatically?

)}`,
);
shownError = true;
}
Expand All @@ -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(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

)}`,
);
shownError = true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,22 @@ class HasMultipleChildrenOfType extends QueryPredicateOperator<HasMultipleChildr
}
}

/**
* A predicate operator that returns true if the node has more than 1 child not of
* type {@link type} (inclusive). For example, `(has-multiple-children-not-of-type?
* @foo bar)` will accept the match if the `@foo` capture has 2 or more children
* not of type `bar`.
*/
class HasMultipleChildrenNotOfType extends QueryPredicateOperator<HasMultipleChildrenNotOfType> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dsl ftw! :D

name = "has-multiple-children-not-of-type?" as const;
schema = z.tuple([q.node, q.string]);

run({ node }: MutableQueryCapture, type: string) {
const count = node.children.filter((n) => n.type !== type).length;
return count > 1;
}
}

class ChildRange extends QueryPredicateOperator<ChildRange> {
name = "child-range!" as const;
schema = z.union([
Expand Down Expand Up @@ -187,4 +203,5 @@ export const queryPredicateOperators = [
new AllowMultiple(),
new InsertionDelimiter(),
new HasMultipleChildrenOfType(),
new HasMultipleChildrenNotOfType(),
];
5 changes: 0 additions & 5 deletions packages/cursorless-engine/src/languages/go.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,6 @@ const nodeMatchers: Partial<
patternMatcher("parameter_declaration"),
patternMatcher("argument_declaration"),
),
collectionKey: "keyed_element[0]",
value: cascadingMatcher(
patternMatcher("keyed_element[1]"),
patternMatcher("return_statement.expression_list!"),
),
};

export default createPatternMatchers(nodeMatchers);
84 changes: 84 additions & 0 deletions queries/go.scm
Original file line number Diff line number Diff line change
Expand Up @@ -222,3 +222,87 @@
.
)
) @anonymousFunction @namedFunction

;; keys in maps
(literal_value
"{" @collectionKey.iteration.start.endOf
(keyed_element
(_) @collectionKey @collectionKey.trailing.start.endOf
":"
(_) @collectionKey.trailing.end.startOf
) @collectionKey.domain
"}" @collectionKey.iteration.end.startOf
)
Copy link
Member

@pokey pokey Sep 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


;; values in maps
(literal_value
"{" @value.iteration.start.endOf
(keyed_element
(_) @value.leading.start.endOf
":"
(_) @value @value.leading.end.startOf
) @value.domain
"}" @value.iteration.end.startOf
)

;; values in return statements

;; one value within a return statement
(return_statement
(expression_list
.
(_)
.
) @value
Comment on lines +252 to +256
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens with comments here?

(#insertion-delimiter! @value ", ")
) @value.domain @value.iteration

;; multiple values within a return statement

;; NB: gofmt puts the comma after block comments in lists of things
;;
;; Like this:
;; return "lorem" /* comment */, "ipsum"
;; Not like this:
;; return "lorem", /* comment */ "ipsum"
;;
;; It's really hard to deal with commas both before and after,
;; and they're rare anyway, so assume gofmt for now.
;; Non-gofmt commas will mess up removal ranges.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally lean slightly towards being conservative wrt removing comments, but this behaviour might be nice if it's almost always what you want. I'll defer to you on this one


;; the first value

;; BUG: in this code:
;; return "lorem" /* comment */ , "ipsum"
;; the comment is included in the removal range of "lorem".
Comment on lines +275 to +277
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

;; This is too hard to fix now, because it would require
;; disjoint removal ranges. And it is rare anyway.

;; the first of many return values...
(return_statement
(expression_list
.
(_) @value @value.trailing.start.endOf
(#insertion-delimiter! @value ", ")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting to put predicate right after the capture itself

.
(comment)* @value.trailing.omit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.
","
.
(_) @value.trailing.end.startOf
) @_exprlist @value.leading.start.startOf
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me

(#not-type? @value comment)
(#has-multiple-children-not-of-type? @_exprlist comment)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷‍♂️

) @value.iteration

;; ...and the rest of the values
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

(return_statement
(expression_list
"," @value.leading.start.startOf
.
(_) @value @value.leading.start.startOf
(#insertion-delimiter! @value ", ")
) @_exprlist
(#not-type? @value comment)
(#has-multiple-children-not-of-type? @_exprlist comment)
) @value.iteration