cursorless icon indicating copy to clipboard operation
cursorless copied to clipboard

Initial next gen scope support for shellscript

Open fidgetingbits opened this issue 2 years ago β€’ 12 comments

What

Adds support for some next gen scopes to shellscript programming language.

This is something I did recently while working on some bash scripts, but haven't had time to finish it off yet.

Checklist

  • [x] Recorded tests for the new language
  • [x] Used "change" / "clear" instead of "take" for selection tests to make recorded tests easier to read
  • [x] Added a few specific tests that use "chuck" instead of "change" to test removal behaviour when it's interesting, especially:
    • [x] "chuck arg" with single argument in list
    • [x] "chuck arg" with multiple arguments in list
    • [x] "chuck item" with single argument in list
    • [x] "chuck item" with multiple arguments in list
  • [x] Added @textFragment captures. Usually you want to put these on comment and string nodes. This enables "take round" to work within comments and strings.
  • [ ] Added a test for "change round" inside a string, eg "hello (there)"
  • [-] Supported "type" both for type annotations (eg foo: string) and declarations (eg interface Foo {}) (and added tests for this behaviour 😊)
  • [ ] Supported "item" both for map pairs and list entries (with tests of course)

Scope Support

Legend: βœ“ Supported, βœ— Not supported, _ Not applicable

Supported Tested Term Capture Definition Comment
βœ“ βœ“ list @list List type equivalent -
βœ“ βœ“ inside list @list.interior Inside of a list -
_ _ map @map Dictionary type equivalent -
_ _ inside map @map.interior Inside of a dictionary -
_ _ key @collectionKey Dictionary key equivalent -
βœ“ βœ“ funk @namedFunction A named function declaration -
βœ“ βœ“ inside funk @namedFunction.interior The inside of a lambda declaration -
βœ“ βœ“ funk name @functionName Name of declared function -
_ _ lambda @anonymousFunction A lambda declaration -
_ _ inside lambda @anonymousFunction.interior The inside of a lambda declaration -
βœ“ βœ“ name @name Variable name -
βœ“ βœ“ value @value Right-hand-side value of an assignment -
_ _ value @value Value returned from a function -
value @value Value of a key-value pair -
βœ“ βœ“ state @statement Any single coded statement -
βœ“ βœ“ if state @ifStatement An if conditional block -
βœ“ βœ“ condition @condition Condition of an if block -
βœ“ condition @condition Condition of a while loop -
βœ“ condition @condition Condition of a do while style loop -
βœ“ condition @condition Condition of a for loop -
βœ“ condition @condition Condition of a switch case -
_ _ condition @condition Condition of a ternary expression -
βœ“ βœ“ branch @branch The resulting code associated with af conditional expression -
βœ“ βœ“ inside branch @branch.interior The statements associated with the body of a conditional expression -
βœ“ βœ“ comment @comment Code comment -
_ _ comment @comment Multi-line code comment -
inside comment @comment.interior The inside of a code comment -
βœ“ βœ“ string @string Single line strings -
_ _ string @string Multi-line strings
βœ“ βœ“ - @textFragment Used to capture string-type nodes (strings and comments) -
βœ“ βœ“ call @functionCall A function call (not a function definition) -
βœ“ βœ“ callee @functionCallee Name of the function being called -
_ _ arg @argumentOrParameter Arguments to function definition -
βœ“ βœ“ arg @argumentOrParameter Arguments to function call -
βœ“ βœ“ arg @argumentOrParameter Interpolated variables in strings -
_ _ class @class Class or structure declaration -
_ _ inside class @class.interior The inside of a class declaration -
_ _ class name @className Name of class or structure declaration -
_ _ type @type Type declarations -
βœ“ βœ“ regex @regex Regular expression -

fidgetingbits avatar Nov 27 '23 01:11 fidgetingbits

Oh also fwiw the infrastructure to formalize those test facets is now in place. Feel free to try it out!

  1. Ensure the facet you care about exists in https://github.com/cursorless-dev/cursorless/blob/main/packages/common/src/scopeSupportFacets/scopeSupportFacets.types.ts. Add it if not (tho feel free to ask us if unsure whether it makes sense)
  2. Add a facet support file for your language. See eg https://github.com/cursorless-dev/cursorless/blob/main/packages/common/src/scopeSupportFacets/javascript.ts
  3. Expose your facet support info in https://github.com/cursorless-dev/cursorless/blob/main/packages/common/src/scopeSupportFacets/getLanguageScopeSupport.ts
  4. Create a subdirectory for your language in the test dir.
  5. Drop a file in there named after the facet. Eg packages/cursorless-vscode-e2e/src/suite/fixtures/scopes/shellscript/value.assignment.scope. Just include your desired test code, followed by a line containing just ---
  6. Run the Update test fixtures launch config. It should dump a nice pretty representation of the scopes found there
  7. Repeat step 5 as many times as desired, followed by step 6 again!

pokey avatar Dec 06 '23 20:12 pokey

All issues from initial review pass are fixed. I have support for a few node types that are dependent on an updated tree-sitter-bash (vscode-parse-tree PR #77). I've also fixed lots of other quirks and edge cases I ran into.

I added the initial scope facet files, but running the Update Fixtures config throws a bunch of errors (not sure if nix-related or not), and I haven't dug into it quite yet. So I don't see any updated content in the test .scope file I created, but still pushed it anyway.

fidgetingbits avatar Jan 02 '24 15:01 fidgetingbits

I switched to tip of bash branch, and noticed some tests breaking. I'm not sure if that was the cause, but I fixed them. Along the way I cleaned up a bit and added / updated facet tests. The auto facet update worked fine for me fwiw

See my fixes; haven't looked at all the code, but possible some of the techniques there apply elsewhere? But if not limk and I'll continue reviewing

pokey avatar Jan 19 '24 17:01 pokey

Oh also fwiw the infrastructure to formalize those test facets is now in place. Feel free to try it out!

  1. Ensure the facet you care about exists in https://github.com/cursorless-dev/cursorless/blob/main/packages/common/src/scopeSupportFacets/scopeSupportFacets.types.ts. Add it if not (tho feel free to ask us if unsure whether it makes sense)
  2. Add a facet support file for your language. See eg https://github.com/cursorless-dev/cursorless/blob/main/packages/common/src/scopeSupportFacets/javascript.ts
  3. Expose your facet support info in https://github.com/cursorless-dev/cursorless/blob/main/packages/common/src/scopeSupportFacets/getLanguageScopeSupport.ts
  4. Create a subdirectory for your language in the test dir.
  5. Drop a file in there named after the facet. Eg packages/cursorless-vscode-e2e/src/suite/fixtures/scopes/shellscript/value.assignment.scope. Just include your desired test code, followed by a line containing just ---
  6. Run the Update test fixtures launch config. It should dump a nice pretty representation of the scopes found there
  7. Repeat step 5 as many times as desired, followed by step 6 again!

How do I actually run this for a subset? There is very little documentation for the regular expressions, and the example folders don't seem to actually exist.

wenkokke avatar Jan 22 '24 14:01 wenkokke

How do I actually run this for a subset? There is very little documentation for the regular expressions

See https://www.cursorless.org/docs/contributing/#running-a-subset-of-tests

Thought the regex was documented somewhere. It’s just the one supported by mocha cli. A PR to add a link to that to those docs would be much appreciated ☺️

(pasted from slack form posterity)

the example folders don't seem to actually exist.

Sorry, not sure I follow. Which example folders?

pokey avatar Jan 22 '24 14:01 pokey

My problem was the regex. I realized after I ran the non subset and it worked. My regex was set to languages/shellscript because that's what I had been doing for the test cases (not sure why I prefixed languages/ in the first place), and of course scope doesn't have the languages/ subdir.

Fwiw, one error I get when running tests (though doesn't impact the scopes updating):

rejected promise not handled within 1 second: NeedsInitialTalonUpdateError: Custom spoken forms file not found at /tmp/ffa46ea4b829416627bdcd0114572b73/state.json. Using default spoken forms.
extensionHostProcess.js:131
stack trace: NeedsInitialTalonUpdateError: Custom spoken forms file not found at /tmp/ffa46ea4b829416627bdcd0114572b73/state.json. Using default spoken forms.
	at TalonSpokenFormsJsonReader.getSpokenFormEntries (/home/aa/persist/source/fidgetingbits-cursorless/packages/cursorless-vscode/dist/extension.cjs:50288:15)
	at async CustomSpokenForms.updateSpokenFormMaps (/home/aa/persist/source/fidgetingbits-cursorless/packages/cursorless-vscode/dist/extension.cjs:34339:26)

Not sure if this is another issue with TMPDIR style stuff or what. Didn't get time to dig into it yet.

fidgetingbits avatar Jan 23 '24 02:01 fidgetingbits

Is your cursorless talon up to date?

pokey avatar Jan 23 '24 10:01 pokey

lmk if you want me to review the code or if you wanted to do another pass first

pokey avatar Jan 23 '24 13:01 pokey

I'll take a look at it tomorrow to see if I spot any candidates for using removal, and also add the fixtures I've been working on.

fidgetingbits avatar Jan 23 '24 14:01 fidgetingbits

Trying to update this in light of the conflict from getLanguageScopeSupport being deleted, and seeing new pre-commit errors I've not run into before:

eslint...................................................................Passed
prettier.................................................................Failed
- hook id: prettier
- exit code: 1

[error] Cannot find package 'prettier-plugin-tailwindcss' imported from /home/aa/dev/talon/fidgetingbits-curs
orless/noop.js
[error] Cannot find package 'prettier-plugin-tailwindcss' imported from /home/aa/dev/talon/fidgetingbits-curs
orless/noop.js
[error] Cannot find package 'prettier-plugin-tailwindcss' imported from /home/aa/dev/talon/fidgetingbits-curs
orless/noop.js
[error] Cannot find package 'prettier-plugin-tailwindcss' imported from /home/aa/dev/talon/fidgetingbits-curs
orless/noop.js
[error] Cannot find package 'prettier-plugin-tailwindcss' imported from /home/aa/dev/talon/fidgetingbits-curs
orless/noop.js
[error] Cannot find package 'prettier-plugin-tailwindcss' imported from /home/aa/dev/talon/fidgetingbits-curs
orless/noop.js
[error] Cannot find package 'prettier-plugin-tailwindcss' imported from /home/aa/dev/talon/fidgetingbits-curs
orless/noop.js
[error] Cannot find package 'prettier-plugin-tailwindcss' imported from /home/aa/dev/talon/fidgetingbits-curs
orless/noop.js
[error] Cannot find package 'prettier-plugin-tailwindcss' imported from /home/aa/dev/talon/fidgetingbits-curs
orless/noop.js
[error] Cannot find package 'prettier-plugin-tailwindcss' imported from /home/aa/dev/talon/fidgetingbits-curs
orless/noop.js
[error] Cannot find package 'prettier-plugin-tailwindcss' imported from /home/aa/dev/talon/fidgetingbits-curs
orless/noop.js

format-recorded-tests....................................................Failed
- hook id: format-recorded-tests
- exit code: 1

✘ [ERROR] Could not resolve "chai"

    packages/common/src/testUtil/runRecordedTest.ts:34:23:
      34 β”‚ import { assert } from "chai";
         β•΅                        ~~~~~~

  You can mark the path "chai" as external to exclude it from the bundle, which will remove this error.

Will look into it, but just throwing it out here in case you happen to know what to do.

fidgetingbits avatar Jun 17 '24 02:06 fidgetingbits

The tailwind bug is from commit de37ebf77834afad9a102cd5a4235886c72ece65. This adds an external pnpm package dependency that pre-commit itself won't install, so pre-commit install is no longer enough. Nixos doesn't have a prettier-plugin-tailwindcss npm package, so atm I have to manually pnpm install -w -D prettier-plugin-tailwindcss, which then changes the pnpm-lock.yaml file lol. :/ I will create a nix package for prettier-plugin-tailwindcss and get it upstreamed, but in the meantime I'll have to use a bit of a hack to get that to work I guess (will just install the package in the dev shell and then git restore the lock file..)

The second bug seems to be related to me recently changing the flake.nix file to use corepack per auscompgeek suggestion. Reverting it back to using hardcoded nodejs 18 makes it go away, so I will revert that latest commit in I guess, as using pre-commit wasn't something I had tested.

fidgetingbits avatar Jun 17 '24 03:06 fidgetingbits

Looks like a bunch of tests are breaking related to @dummy and other things which I vaguely recall fixing a long time ago, so not sure what's up, but will investigate.

fidgetingbits avatar Jun 17 '24 05:06 fidgetingbits