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

Initial next gen scope support for shellscript #2058

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

fidgetingbits
Copy link
Collaborator

@fidgetingbits fidgetingbits commented Nov 27, 2023

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

  • Recorded tests for the new language
  • Used "change" / "clear" instead of "take" for selection tests to make recorded tests easier to read
  • Added a few specific tests that use "chuck" instead of "change" to test removal behaviour when it's interesting, especially:
    • "chuck arg" with single argument in list
    • "chuck arg" with multiple arguments in list
    • "chuck item" with single argument in list
    • "chuck item" with multiple arguments in list
  • 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 -

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

Once again fantastic work. Left a few minor comments

queries/shellscript.scm Show resolved Hide resolved
queries/shellscript.scm Outdated Show resolved Hide resolved

;;!! var="foo"
;;! ^^^^^
(string) @string @textFragment @argumentOrParameter.iteration
Copy link
Member

Choose a reason for hiding this comment

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

curious why this is iteration scope for "arg"? A test for that would be good

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There was a discussion that some of you had in another issue which I forget off hand, where iirc the consensus was that ${} expansions inside of strings were going to be referred to be as arg scopes. So I've been testing that on the shell script stuff and it seems to work pretty nicely. I can't remember offhand if I added it to nix as well, which uses similar expansions.

Copy link
Member

Choose a reason for hiding this comment

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

ohh right yeah if interpolations are args, then string should be iteration scope. Feels a bit surprising to get trapped in a string for "every arg" tho. cc/ @AndreasArvidsson

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I'm a bit hesitant to make strings the iteration scope for arguments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So there are at least 3 options I guess?

  1. Only add an iteration scope if the string actually has an interpolation inside, so it's less surprising
  2. Keep interpolation as args, but just don't have an iterarion scope for the container strings (I feel like being able to target every interpolation in a string could be nice though?)
  3. Create a new scope and don't use args

Copy link
Member

Choose a reason for hiding this comment

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

  1. is an interesting possibility. We don't usually restrict depending on whether the scope actually appears, but I could see an argument here

Re 2), you don't actually need to have an iteration scope to target every interpolation in a string. You'd just say "every arg string". Iteration scope is only used when there's no explicit range given for iteration. That being said, in #2128 (comment), we've started leaning towards requiring iteration scope between layers of any nested scope, so that would argue for having an iteration scope here cc/ @AndreasArvidsson

  1. is an interesting idea, and certainly I could see this being a fine-grained scope that gets rolled up into arg, tho we don't really support that yet

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now I just removed the iteration scope, but keep the interpolations as args. I'll mark this unresolved in case there is some other type of iteration scope I should add in light of 2).

queries/shellscript.scm Outdated Show resolved Hide resolved
queries/shellscript.scm Outdated Show resolved Hide resolved
@pokey
Copy link
Member

pokey commented Dec 6, 2023

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!

@fidgetingbits
Copy link
Collaborator Author

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.

@pokey
Copy link
Member

pokey commented Jan 19, 2024

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

@wenkokke
Copy link

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.

@pokey
Copy link
Member

pokey commented Jan 22, 2024

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?

@fidgetingbits
Copy link
Collaborator Author

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.

@pokey
Copy link
Member

pokey commented Jan 23, 2024

Is your cursorless talon up to date?

@pokey
Copy link
Member

pokey commented Jan 23, 2024

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

@fidgetingbits
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

The tailwind bug is from commit de37ebf. 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 fidgetingbits mentioned this pull request Jun 17, 2024
@fidgetingbits
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants