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

Add Rename Symbol Capability #915

Merged
merged 58 commits into from
Dec 27, 2023
Merged

Add Rename Symbol Capability #915

merged 58 commits into from
Dec 27, 2023

Conversation

mcecode
Copy link
Contributor

@mcecode mcecode commented Jul 19, 2023

Hi, I'd like to give implementing #161 a try, though it might take some time since I'm new to language servers and renaming in Bash doesn't seem to be as straightforward as in other languages. I have onPrepareRename working already, so hopefully onRenameRequest won't be too difficult to implement.

For global defined variables, I'm planning to base the rename on the includeAllWorkspaceSymbols config. If it's set to false, then the symbol would only be renamed within files linked by sourcing. If it's set to true, then the symbol would be renamed throughout the whole workspace.

For variables that are not defined, renaming would only happen within the file. I'm thinking this could be useful for renaming environment variables like if someone wants to change $HOME to $PWD for some reason.

For special variables, I'm thinking $1 to $9 could be renamed since it's common to assign these to variables with more descriptive names. It could be useful, for example, if someone created a quick script or function and now want to clean it up, they could just rename $1 to $myVar and assign myVar="$1" at the top of the script or function. I don't think other special variables like $@ or $# should be renamed since they already have meaning in and of themselves.

Feedback on the approach I'm thinking of taking and the work I've done so far would be much appreciated :)

Todos:

  • Add rename handlers
  • Implement onPrepareRename
  • Implement onRenameRequest for undeclared symbols
  • Implement onRenameRequest for scoped symbols
    • Local variables inside functions
    • Scoped variables and functions inside subshells
  • Implement onRenameRequest for global symbols
    • Within the file
    • Within the workspace
  • Test
    • Fix failing
    • onPrepareRename handler
    • onRenameRequest handler
  • Refactor
  • Update any docs that reference rename symbol functionality (e.g., README.md)

@skovhus
Copy link
Collaborator

skovhus commented Jul 19, 2023

Thanks for looking into this! And great that you are opening a draft PR, makes it easier to help you.

I do believe we have all the building blocks available to implement this feature – also note that a lot of the required functionality is available through https://github.com/bash-lsp/bash-language-server/blob/main/server/src/util/declarations.ts (which is what is used by the analyzer to get declarations). Declarations contain both environment variables, variables and functions.

For special variables, I'm thinking $1 to $9 could be renamed since it's common to assign these to variables with more descriptive names. It could be useful, for example, if someone created a quick script or function and now want to clean it up, they could just rename $1 to $myVar and assign myVar="$1" at the top of the script or function. I don't think other special variables like $@ or $# should be renamed since they already have meaning in and of themselves.

I suggest descoping that for the first version (unless this is easy to implement).

@mcecode
Copy link
Contributor Author

mcecode commented Jul 19, 2023

Thanks for the quick response! I'm still getting familiar with the code base so thanks for the tip on using the functionality in declarations.ts.

I'll take your advice and descope my ideas about special variables for now since I'm not sure how complex it is to implement.

@mcecode
Copy link
Contributor Author

mcecode commented Aug 16, 2023

Hi @skovhus, it took a bit of exploration and experimentation, but I think I'm more or less done with renaming not defined and scoped variables and functions, and I think renaming global variables and functions isn't too far off. Before I continue though, I'd like to get your feedback on what I've done and the approach I took so I know I'm going in the right direction.

Cases covered

  • Renaming not defined variables

    1-not-defined.webm
  • Renaming function scoped local variables

    2-scoped-function.webm
  • Renaming subshell scoped functions and variables

    3-scoped-subshell.webm
  • Some scope awareness

    4-scope-awareness-function.webm
    5-scope-awareness-subshell.webm
  • Differentiation between functions and variables with the same name

    6-differentiate-variables-and-functions.webm

Though I think function nesting, subshells, and such are rarely needed or used, I opted to support them so that in those cases where a user does use them, they won't get too janky of an experience.

Limitations

  1. Variable names not typed as variable_name by tree-sitter-bash can't and don't get renamed. Examples of this are variables inside arithmetic expansions and C-style for loops.

    arithmetic-expansion-whole

    In the arithmetic expansion above, the whole n+n is typed as a word with a command_name parent. Even if separated apart, each character would just be seen as separate words:

    arithmetic-expansion-separated

    To support these cases, we'd need to parse inside these constructs ourselves which would be quite complex to do.

  2. Scope awareness breaks down for complex nesting and scopes.

    In the example below, scope-wise, $var inside 3 should not be renamed when renaming var inside 1.

    7-limitation-complex-nesting.webm

    In this other example, $var inside calleeFunc is not renamed when renaming var inside callerFunc even if, scope-wise, they are the same variable.

    8-limitation-complex-scopes.webm

    This limitation comes from the heuristics I used when finding symbol instances. Though I could try to cover these and other edge cases, I think the effort-to-benefit ratio wouldn't be worth it right now as not a lot of users would run into them.

  3. Only the first variable_assignment is caught per declaration_command, so in the example below, only a="a" would be caught.

    local a="a" b="b" c="c"

    Truthfully, I just forgot that multiple variable_assignments can be done in one declaration_command while implementing Analyzer#findOriginalDeclaration, that's why it doesn't handle it. I don't think multiple variable_assignments in one declaration_command is used often, so this can be left as is, but I can add it if you think it would be useful for users.

Approach taken

The initial approach I took is as follows:

  1. Get the word at the given position using Analyzer#wordAtPointFromTextPosition.
  2. Find the word's declaration using Analyzer#findDeclarationsMatchingWord.
  3. Find all of the word's occurrences using Analyzer#findOccurrences.

I quickly learned that though this approach worked well enough for renaming not defined variables, it broke down when dealing with things like local declarations and scopes. So, I ended up slowly adding methods to Analyzer as I revised my approach, which became:

  1. Get the symbol, either a function or a variable, at the given position using Analyzer#symbolAtPointFromTextPosition
  2. Find the symbol's original definition within the scope it's in using Analyzer#findOriginalDeclaration.
  3. Find the definition's scope using Analyzer#findParentScope.
  4. Find all of the symbol's occurrences inside that scope using Analyzer#findOccurrencesWithin.

I added methods instead of changing what's already there since I didn't want to interfere with the current functionality and diagnostics provided by the language server and risk breaking the current experience for users. However, I do think what I added could be incorporated later on by other handlers. For example, I think Analyzer#findOccurrencesWithin's implementation could replace Analyzer#findOccurrences' which would give onDocumentHighlight and onReferences more accurate results.

That said, I do think I'm adding quite a bit of code, and I do realize that the onus of reviewing all of this would fall onto you. So, I wanted to know if you're alright with how I'm going about things and with all of the cases I'm trying to cover. Do you think I did too much? Should I tone down what I'm trying to support? Did I miss something and maybe I'm just reimplementing things that are already available in the code base? How could I make this easier for you to review later on?

Copy link
Collaborator

@skovhus skovhus left a comment

Choose a reason for hiding this comment

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

Amazing work here. 👏 👏 👏 I quickly looked over the code and came with some suggestions. But I'm really happy about the level of communication you do here and the videos.

I would love if you invested in writing some high level unit tests covering all the cases we handle and do not handle.

/**
* A more scope-aware version of findOccurrences.
*/
public findOccurrencesWithin({
Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't look to carefully here, but it might be we can reuse some functionality from utils/declarations.

parent = this.findParentScopeNode(uri, parent.startPosition, parent.endPosition)
}

// TODO: Handle global definitions
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean declarations across files? getGlobalDeclarations should help with this.

@mcecode
Copy link
Contributor Author

mcecode commented Aug 17, 2023

Thanks for the review, I appreciate it. I'll update the implementation based on your suggestions.

Don't worry about the tests, I've kept track of the cases covered and not and plan to write them in test form when the implementation is a bit more locked in. I've been adding and removing methods here and there during my exploration and didn't want to add and remove tests at the same time.

Since you seem to be alright with the general direction I'm going, I'll just ping you again when I'm done with the implementation + tests. Hopefully it won't take me so long this time 😅 Thanks again!

@skovhus
Copy link
Collaborator

skovhus commented Sep 25, 2023

It seems like this is pretty close! 👏

@mcecode
Copy link
Contributor Author

mcecode commented Sep 25, 2023

Hi! Yeah, it's pretty close now. Taking me a while since I've only been able to chip at it little by little and I had to think through a bit on how workspace-wide renames should work, but the implementation's pretty much done. I just have to fix the failing tests due to snapshots not expecting onPrepareRename and onRenameRequest handlers, add the tests for renaming, and probably refactor a bit since some of the methods I've added have become quite long and unwieldy.

I'll probably make an updated write-up of the cases covered, limitations, and approach taken when I'm done as some things have changed since the last one I commented. Heads up though, I tried to use as much of what's already in the code base, but I wasn't able to use what's in util/declarations.ts because it returns declarations based on the latest definition. The implementation I've come up with uses the original declaration/definition to gauge the scope of where to search for words that match. I'd be glad to know if I missed something and I can use it somewhere.

@skovhus
Copy link
Collaborator

skovhus commented Sep 25, 2023

he implementation I've come up with uses the original declaration/definition to gauge the scope of where to search for words that match. I'd be glad to know if I missed something and I can use it somewhere.

No worries.

Handle multiple variable assignments and declarations per
declaration command when searching inside functions
Handle multiple variable assignments and declarations per
declaration command when searching globally
Handle multiple variable assignments and declarations in one
declaration command
@mcecode
Copy link
Contributor Author

mcecode commented Oct 19, 2023

Hi @skovhus, I'm finally done. There are a lot of lines added, but most of those are from tests and snapshots. I'm not sure if it's overkill, but I wanted to document as much behavior as I reasonably could while taking into account some edge cases, and that's what I ended up with.

Now, onto the updates. I'll be skipping visuals this time since I think the tests and example cases in renaming.sh should cover that.

Cases covered

  • Renaming undeclared symbols
  • Renaming locally scoped symbols, local variables in functions and declared variables and functions in subshells, with some scope awareness
  • Renaming globally scoped symbols, both file-wide and workspace-wide, taking into account the includeAllWorkspaceSymbols flag
  • Differentiates between variables and functions with the same name
  • Handling requests with non-renamable symbols (onPrepareRename) and invalid symbol names (onRenameRequest)

Limitations

  • Not all variables are typed by tree-sitter-bash as variable_name, so those variables won't be renamed
  • Scope awareness breaks down for complex scopes and nesting
  • Only takes into account subshells created with ( and ), so, for example, subshell scopes created with the pipe (|) operator aren't recognized
  • Doesn't take into account sourcing location and scope, so, for example, sourcing inside a subshell will affect symbols outside of that subshell

You can find tests that expound and give more examples of these in the "Edge or not covered cases" under the "onRenameRequest" describe block and some parts of the "onPrepareRename" describe block. Honestly though, there are probably more limitations that I'm just not aware of since there are so many cases that can be created with Bash.

Approach taken

There are some changes from the original, but the overall steps are the same.

  1. Get the symbol, either a function or a variable, at the given position using Analyzer#symbolAtPointFromTextPosition.
  2. Find the symbol's original declaration and scope based on its original definition using Analyzer#findOriginalDeclaration. The general heuristic used here is that the original declaration should be within the same or at a higher scope as the symbol found in step 1 while also being higher up in the file.
  3. Based on the original declaration and scope found in step 2, find all of the symbol's occurrences inside the scope, the file, or the whole workspace using Analyzer#findOccurrencesWithin, taking into account things like scoping and the includeAllWorkspaceSymbols flag.

Thank you so much for your patience on this. Feel free to tell me if there's anything else I need to do or if I missed anything and I'll try to address it as soon as I can.

@mcecode mcecode marked this pull request as ready for review October 19, 2023 10:13
@mcecode mcecode changed the title [WIP] Add Rename Symbol Capability Add Rename Symbol Capability Oct 19, 2023
@Shane-XB-Qian
Copy link
Contributor

i did a rough test, work fine, thanks.
but i found a case: e.g while read a b; do echo $b; done here it cannot rename b
was that an expected case, or seems it's not in one of "Edge or not covered cases"

@mcecode
Copy link
Contributor Author

mcecode commented Oct 20, 2023

Hi @Shane-XB-Qian, thanks for testing it on your setup. Glad it works well.

Unfortunately, yes, that case is an expected case since b is not typed by tree-sitter-bash as variable_name but rather as word, so it falls on the first limitation that I listed. I've added a while read loop test case since that's a common use case and updated the first test's name in "Edge or not covered cases" to make the limitation being tested more explicit.

@skovhus skovhus mentioned this pull request Dec 27, 2023
@skovhus skovhus enabled auto-merge December 27, 2023 12:26
@skovhus
Copy link
Collaborator

skovhus commented Dec 27, 2023

Sorry for the late review. Amazing work here @mcecode – let me know if you have other ideas for improving the LSP. I would love some help maintaining this project...

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.

3 participants