Contiguous scope #2101

AndreasArvidsson · 2023-12-06T05:34:17Z

Now implemented as a modifier of the comment scope

I decided to implement this as a scope type instead of a modifier. Now you can say things like chuck fat comment or changed next fat alpha. This behavior of having alternatives to existing scopes is already something we started with BoundedNonWhitespaceSequenceStage albeit in a less capacity.

Checklist

I have added tests
[-] I have updated the docs and cheatsheet
I have not broken the cheatsheet

pokey

seems like a good implementation approach. left some comments. also, please add some tests with "next" / "previous", and be sure to test boundaries on edges of scopes and between constituent scopes

packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/ContiguousScopeHandler.ts

packages/cursorless-engine/src/scopeProviders/ScopeInfoProvider.ts

…andlers/ContiguousScopeHandler.ts Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>

packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/ContiguousScopeHandler.ts

…ss into contiguous_scope

pokey

Well this code simplified nicely 😄. Left a couple more comments

pokey · 2023-12-12T15:14:44Z

packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/ContiguousScopeHandler.ts

+  };
+}
+
+function* generateTargetRangesInDirection(


I'm not convinced this function is correct. Eg corner cases like single scope not adjacent to others that's last in the file. To me it looks like if you've already yielded anything, then the final yield after the loop body won't fire.

Some proper unit tests for ContiguousScopeHandler would help. See https://github.com/cursorless-dev/cursorless/blob/main/packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/BaseScopeHandler.test.ts if you're not sure how to do unit tests for this kinda thing. There's a limit to how far you can get using existing scopes

Actually might make sense to hold off on unit tests for the following reasons:

Your code actually looks at document text, unlike the test I linked, so might be harder to mock that properly

On second thought, I think your code is probably correct

I'm actually not convinced of the utility of this contiguous scope thing 😅. The only example I saw that was actually useful was "comment", and tbh I'd be tempted to just automatically merge adjacent single-line comments, as we could target a single one using "line". And I worry users will try "fat arg" to get something like Support "all" modifier #473, and then be surprised when it breaks if there's a comment between args. So maybe we ship this with slightly fewer tests, maybe marked private? idk

Update from discussion today:

We agreed "comment" is the only use case we can come up with, and we probably just want that to be default behaviour for line comments

If @josharian wants to fight for supporting "fat" for arbitrary scopes, then we might keep it; otherwise it dies

We potentially could reuse this code to make contiguous be default behaviour for comment

If @josharian wants to fight for supporting "fat" for arbitrary scopes, then we might keep it; otherwise it diesau

looking at these test cases, i think it should die

Sounds good. Any opinion on making single-line comments behave this way by default? (Ie auto expand to all comments that aren't separated by an empty line)

Update from meet-up: let's do the following:

( (comment) @comment (#match? @comment "^//") (#contiguous! @comment) ) (comment) @comment

We both don't love this option or the option in this PR, but it's the best we can do with what we have today. Fwiw we would prefer something like

( (comment) @comment ($if (#match? @comment "^//") (#contiguous! @comment) ) )

but that's a bit out of scope here 😅

pokey · 2023-12-12T15:16:41Z

packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/ContiguousScopeHandler.ts

+  const [startTarget, endTarget] = getTargetsInDocumentOrder(
+    scope1.getTargets(false)[0],
+    scope2.getTargets(false)[0],
+  );


do we want to throw an error if there's more than one target? Should we have a utility function function ensureSingleTarget(scope: Scope): Target that checks there's only one target and returns it? That worked out well for ensureSingleEditor I think

Reused our existing and shore single target. They should really never trigger with our current implementation so am fine with using an error message that might not be perfect.

packages/cursorless-engine/src/spokenForms/defaultSpokenFormMapCore.ts

AndreasArvidsson · 2023-12-14T15:06:51Z

@pokey @josharian The current implementation now only effects the command scope type and there is no spoken form for it.

AndreasArvidsson · 2023-12-14T15:15:03Z

packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/ContiguousScopeHandler.ts

+    return true;
+  }
+
+  const [startTarget, endTarget] = getTargetsInDocumentOrder(


This file is now general enough to be used with most scopes. If we wanted to we could remove a bunch of code here and rely on the fact that we only use it for comments. I could go either way.

pokey · 2023-12-14T17:12:20Z

Won't this cause adjacent multiline comments to be merged as well? eg

/*
 * foo
 */
/*
 * bar
 */

Looking at this, it feels like not a particularly big deal 😅. But figured I'd point it out as I don't think it quite matches what we discussed

AndreasArvidsson · 2023-12-14T17:17:29Z

Yes unless we make this more comment specific and check for single line ranges. Ideally we would have single and multi-line as separate scopes and only use this for single line.

Or add a new predicate for contiguous scope. That is probably the best solution?

pokey · 2023-12-14T17:20:32Z

hmm so the predicate would just set a flag on the capture. I think that sounds reasonable?

AndreasArvidsson · 2023-12-14T17:24:35Z

Yes exactly.

AndreasArvidsson · 2023-12-15T07:35:01Z

There is no problem really with this implementation except for the fact that in javascript there is no Tree sitter difference between block and line comments. I guess that contiguous predicate could take a boolean that requires single line ranges? Or a regex pattern?

edit: Solved with a regex pattern

pokey

left a few comments

pokey · 2023-12-15T14:48:04Z

packages/cursorless-engine/src/languages/LanguageDefinition.ts

+
+/**
+ * Returns true if the given scope type should use a contiguous scope handler.
+ */
+function useContiguousScopeHandler(scopeType: ScopeType): boolean {
+  switch (scopeType.type) {
+    case "comment":
+      return true;
+    default:
+      return false;
+  }
+}


I thought you were using a query predicate for this now

We use both. I don't know how to check it at this stage. When we actually fetch the matched scope it's to late to inject this handler. Not without a lot of rewrites at least.

Sorry I'm not following how you would use both. When do you use one vs the other?

This code decides to use the handler. Then the scope.contiguous is used to determine when to merge two scopes. That way we can ignore block comments.

cursorless/packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/ContiguousScopeHandler.ts

Line 119 in b6ac60f

if (!scope1.contiguous || !scope2.contiguous) {

pokey · 2023-12-15T14:49:41Z

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

+  name = "contiguous!" as const;
+  schema = z.union([z.tuple([q.node]), z.tuple([q.node, q.string])]);
+
+  run(nodeInfo: MutableQueryCapture, pattern?: string) {


Yikes. This is a slippery slope. This operator merges to completely orthogonal concepts. I fear we're sleepwalking into the same problem we had with our old typescript-based matchers, where we had 5 custom functions per language.

Maybe we could add a predicate that rejects a pattern if it doesn't match a regex, and then define the pattern twice: once with positive match, one negative?

This is still not language specific. Probably more languages that dont separate block comments in tree sitter.

That could also work, but is that really easier to read and maintain?

In what way is the concept of regex pattern related to whether we want a contiguous scope? Feels like we're starting to invent a predicate for every arbitrary combination of abstractions. This feels like the next step from the multi-line ternary thing towards a combinatorial explosion of predicates

I think that would be easier to read, because otherwise I would look at the predicate and be like "why does this have anything to do with a regex pattern?"

Happy to discuss if you'd like

Because the pattern matches a single line comment? look how I use it in js.

Lets.

to discuss Plan to discuss at meet-up

Update from meet-up:

Remove pattern arg from this predicate

use the built-in tree-sitter match predicate, and duplicate the pattern, once with contiguous and once without

packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/scope.types.ts

AndreasArvidsson added 5 commits December 5, 2023 15:21

Started working on contiguous scope modifier

fdcd03e

Merge branch 'main' into contiguous_scope

f223d84

more work

b549ca5

Change continuous scope modifier into continuous scope type

a71f017

Added tests

da8d6ec

AndreasArvidsson requested a review from pokey as a code owner December 6, 2023 05:34

pre-commit-ci-lite bot and others added 5 commits December 6, 2023 05:35

[pre-commit.ci lite] apply automatic fixes

a3fa351

Moved tests

53351e8

Added comment

d370690

cleanup

48f97ee

more cleanup

e2ec5c8

pokey reviewed Dec 6, 2023

View reviewed changes

AndreasArvidsson and others added 11 commits December 6, 2023 16:21

Update packages/cursorless-engine/src/processTargets/modifiers/scopeH…

d1020a8

…andlers/ContiguousScopeHandler.ts Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>

Import

02a295e

Update packages/cursorless-engine/src/processTargets/modifiers/scopeH…

9d75ead

…andlers/ContiguousScopeHandler.ts Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>

Update packages/cursorless-engine/src/processTargets/modifiers/scopeH…

48c7a65

…andlers/ContiguousScopeHandler.ts Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>

Update packages/cursorless-engine/src/processTargets/modifiers/scopeH…

fa57ab2

…andlers/ContiguousScopeHandler.ts Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>

Update packages/cursorless-engine/src/processTargets/modifiers/scopeH…

c943542

…andlers/ContiguousScopeHandler.ts Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>

Update packages/cursorless-engine/src/processTargets/modifiers/scopeH…

c26d704

…andlers/ContiguousScopeHandler.ts Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>

[pre-commit.ci lite] apply automatic fixes

e0b6b5b

rename

2988adb

Merge

c2f942b

update

34cbbb1

pokey reviewed Dec 6, 2023

View reviewed changes

packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/ContiguousScopeHandler.ts Outdated Show resolved Hide resolved

AndreasArvidsson added 6 commits December 7, 2023 05:09

Use proximal and distal

8b5a95f

Continues target

bd86a64

Added tests

5a905ea

cleanup

6612818

Merge branch 'main' into contiguous_scope

2852e37

clean up

d3eb687

Merge branch 'contiguous_scope' of github.com:cursorless-dev/cursorle…

fe7e8e5

…ss into contiguous_scope

pokey reviewed Dec 12, 2023

View reviewed changes

AndreasArvidsson added 4 commits December 14, 2023 15:46

Only do contiguous for comments

2f15dc6

cleanup

0ccdcc6

Merge branch 'main' into contiguous_scope

fd93230

cleanup

d898f26

Update test

ea5a542

AndreasArvidsson commented Dec 14, 2023

View reviewed changes

AndreasArvidsson added 3 commits December 15, 2023 08:45

Added predicate

66731e4

Merge branch 'main' into contiguous_scope

58bfb9a

Added comment facets

b6ac60f

pokey reviewed Dec 15, 2023

View reviewed changes

AndreasArvidsson added the to discuss Plan to discuss at meet-up label Dec 15, 2023

pokey removed the to discuss Plan to discuss at meet-up label Jan 4, 2024

AndreasArvidsson added 3 commits February 26, 2024 07:24

Remove pattern argument from contiguous predicate

17b0507

Merge branch 'main' into contiguous_scope

6f0de5c

Update scope fixtures

9f156a6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Contiguous scope #2101

Contiguous scope #2101

AndreasArvidsson commented Dec 6, 2023 •

edited

pokey left a comment

pokey left a comment

pokey Dec 12, 2023

pokey Dec 12, 2023

pokey Dec 12, 2023

josharian Dec 12, 2023

pokey Dec 12, 2023

pokey Jan 4, 2024 •

edited

pokey Dec 12, 2023

AndreasArvidsson Dec 14, 2023

AndreasArvidsson commented Dec 14, 2023 •

edited

AndreasArvidsson Dec 14, 2023

pokey commented Dec 14, 2023 •

edited

AndreasArvidsson commented Dec 14, 2023

pokey commented Dec 14, 2023

AndreasArvidsson commented Dec 14, 2023

AndreasArvidsson commented Dec 15, 2023 •

edited

pokey left a comment

pokey Dec 15, 2023

AndreasArvidsson Dec 15, 2023

pokey Dec 15, 2023

AndreasArvidsson Dec 15, 2023 •

edited

pokey Dec 15, 2023

AndreasArvidsson Dec 15, 2023

pokey Dec 15, 2023

AndreasArvidsson Dec 15, 2023

pokey Dec 15, 2023

pokey Feb 25, 2024

AndreasArvidsson Feb 26, 2024

Contiguous scope #2101

Are you sure you want to change the base?

Contiguous scope #2101

Conversation

AndreasArvidsson commented Dec 6, 2023 • edited

Checklist

pokey left a comment

Choose a reason for hiding this comment

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

pokey Jan 4, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasArvidsson commented Dec 14, 2023 • edited

Choose a reason for hiding this comment

pokey commented Dec 14, 2023 • edited

AndreasArvidsson commented Dec 14, 2023

pokey commented Dec 14, 2023

AndreasArvidsson commented Dec 14, 2023

AndreasArvidsson commented Dec 15, 2023 • edited

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

AndreasArvidsson Dec 15, 2023 • edited

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

AndreasArvidsson commented Dec 6, 2023 •

edited

pokey Jan 4, 2024 •

edited

AndreasArvidsson commented Dec 14, 2023 •

edited

pokey commented Dec 14, 2023 •

edited

AndreasArvidsson commented Dec 15, 2023 •

edited

AndreasArvidsson Dec 15, 2023 •

edited