[SR-16104] SourceKit-LSP: Add support for RenameRequest

[adam-fowler]

Previous ID	SR-16104
Radar	None
Original Reporter	@adam-fowler
Type	Improvement

Additional Detail from JIRA

Votes	0
Component/s	SourceKit-LSP
Labels	Improvement
Assignee	None
Priority	Medium

md5: 8028e8b5f474f8ce22efadde5bd6c30a

Issue Description:

Add support for RenameRequest message.

[MozarellaMan]

I'd like to try working on this. Would I be correct in saying it would be very similar to references in SourceKitServer?

[ahoppen]

references only reports references to the symbol within the same file but I would expect RenameRequest to rename all occurrences of the symbol across files, which sourcekitd doesn’t currently support. We would need to query the index database (SourceKitLSP.Workspace.index) for all references to the renamed symbol across all files but I think this should definitely be possible.

[resolritter]

references only reports references to the symbol within the same file but I would expect RenameRequest to rename all occurrences of the symbol across files

@ahoppen does that mean symbol.usr is not used across modules? At the moment I'm a bit confused by this statement of "references only reports references to the symbol within the same file" because I don't see any filtering being done in https://github.com/apple/sourcekit-lsp/blob/0329b49735083c374a56a479c990888d924806a9/Sources/SourceKitLSP/SourceKitServer.swift#L1315-L1325 for the locations of req.params.textDocument. I'm also trying to grok addRelation and nameAndUSRCache from https://github.com/apple/swift/blob/0b20f2117c6d6115a5e055782105711d654dd6c2/lib/Index/Index.cpp#L529 but haven't quite found where the per-file scoping happens.

[ahoppen]

The SourceKit-LSP textDocument/references request is backed by the sourcekitd source.request.cursorinfo request, which walks the AST of the the current file and reports any usages of the variable that’s currently highlighted within it. It does not do an index lookup and thus can’t find usages of the variable in other files.

[ahoppen]

rdar://95587948

[adam-fowler]

@ahoppen I was just looking at the textDocument/references request and it seems to be returning all references of a symbol across all packages.

[ahoppen]

Oh, you’re right. I didn’t realize we were doing an index lookup for textDocument/references.

[MartinLau7]

Hi all, is there any new progress on this function request?

[stackotter]

I’m interested in trying to implement this, because I don’t use Xcode and it bothers me every day how many must-have LSP features are missing. Is anyone actively working on this already, and are there any roadblocks I should be aware of?

[ahoppen]

I don’t think anyone is actively working on this and I can’t think of any roadblocks that I know of.

@stackotter If you want to work on this, that would be great. If you’ve got any questions, don’t hesitate to ask! The comments I added last year should also still be up-to date and give you the first pointers on where to start.

[stackotter]

Awesome! I've started working on trying to get a very crude implementation of RenameRequest working based off SourceKitServer.references (and ignoring PrepareRenameRequest for now).

I'm running into an issue where self.workspaceForDocument(uri: req.params.textDocument.uri)?.index is nil (to be clear, index is nil, not the workspace). And I don't know how to get the language server to produce an index. I have tried building the project and that didn't seem to help at all. By the way I'm not using an Xcode project, I'm testing this on a Swift package in Neovim.

It seems like all multi-file functionality just plain isn't working (cross-file code completion etc.). Not really sure how to go about fixing this. I think it's probably an issue with my development setup and mismatching versions or something?

Swift toolchain: swift-DEVELOPMENT-SNAPSHOT-2023-02-27-a.xctoolchain
sourcekit-lsp env: "SOURCEKIT_TOOLCHAIN_PATH": "/Library/Developer/Toolchains/swift-DEVELOPMENT-SNAPSHOT-2023-02-27-a.xctoolchain/"
sourcekit-lsp branch: main
Build command for my test project: /Library/Developer/Toolchains/swift-DEVELOPMENT-SNAPSHOT-2023-02-27-a.xctoolchain/usr/bin/swift build --enable-index-store

EDIT

I compared the build dirs of two projects (one built with 5.7 and one built with the latest dev toolchain) and it looks like the one built with the dev toolchain is missing a db directory in .build/debug/index. Both projects do have a .build/debug/index/store directory. Don't know if this is a change of behaviour between 5.7 and 5.8 but it seems related.

[ahoppen]

Hey @stackotter,

Sorry for the delayed reply.

I just double-checked and the db directory is created by indexstore-db once the index is opened and not during the compilation itself. So the fact that the db directory is missing is not the cause of the problem but a symptom.

That being said, it is odd that the index is nil here. If you have instructions on how to reproduce the problem (branch of SourceKit-LSP to build, project to open), I can try debugging it. I only have a setup of SourceKit-LSP using VSCode using the Swift extensions, so if the instructions could be based on VSCode that would really make it a lot simpler for me to reproduce it locally.

[ahoppen]

Also: Could you try running SourceKit-LSP with an environment variable SOURCEKIT_LOGGING=5? In VSCode at least you can set this in the “Swift Environment Variables” section.

[stackotter]

In trying to set up a reproduction case, the index stopped being nil 😅 it was probably a misconfiguration in neovim that I didn't have in VSCode.

However, now I'm running into the issue of occurs in extractIndexedOccurrences being [] no matter which symbol I select for renaming. All the code before that seems to be working (the selected symbol's usr and location are identified), but for some reason the index lookup is failing to find any results (I'm using SymbolRole.all).

I investigated why go to definition (which also uses extractIndexedOccurrences) is managing to work, and it seems like the only reason it's working is because it's falling back on the bestLocalDeclaration value from the symbol info request (which isn't an option in the case of renaming, because we need references).

It's almost as if the index is empty?

I'm on commit 24ce1be of sourcekit-lsp and I'm using the swift-DEVELOPMENT-SNAPSHOT-2023-02-27-a Swift toolchain. I have included a zip of the example package I'm using.

Example.zip

[ahoppen]

Could it be that you haven’t built your package yet when extractIndexedOccurrences returns no results?

Could you try the following

• Add the list of files in .build/<architecture>/debug/index/store/v5/units to this issue (the actual contents aren’t important right now)
• If this list does not contain A.swift.o-<something>, try the following and see if any of the steps make the file appear
  • path/to/swift build
  • path/to/swift build --enable-index-store
  • Remove the .build directory and try the steps again.

[stackotter]

I hadn't built it after the last time I cleaned it 🤦‍♂️ It seems that it works in VSCode now (ish). Thanks for the tips :)

I can successfully get a list of all references to a given non-local symbol now (not sure of the formal word for that, I mean things that aren't local vars). For renaming of local variables I assume that I'll need to deal with the open file's AST.

Annoyingly, the ranges of all of the symbols returned are wrong (the start and end locations are both just the start location of the symbol). I think VSCode doesn't like this because it completely ignores any TextEdits I return. However, I've even tried hardcoding an end index adjustment specifically for the symbol I'm attempting to rename, and it still ignores the renames. I'll look into it a bit more later this week, but if you have any insight from your experience of sourcekit-lsp internals that would be useful. It would also be handy if there was a way to inspect why the lsp client is silently ignoring the changes in the rename responses, but I don't know if there is.

I've included my current rename implementation in case there's something obviously wrong

func rename(
  _ req: Request<RenameRequest>,
  workspace: Workspace,
  languageService: ToolchainLanguageServer
) {
  let symbolInfo = SymbolInfoRequest(textDocument: req.params.textDocument, position: req.params.position)
  let index = self.workspaceForDocument(uri: req.params.textDocument.uri)?.index
  let callback = callbackOnQueue(self.queue) { (result: LSPResult<SymbolInfoRequest.Response>) in
    let extractedResult = self.extractIndexedOccurrences(result: result, index: index) { (usr, index) in
      let roles: SymbolRole = .all
      return index.occurrences(ofUSR: usr, roles: roles)
    }

    let workspaceEdit: Result<WorkspaceEdit?, ResponseError> = extractedResult.map { result in
      var changes: [DocumentURI: [TextEdit]] = [:]
      for occurence in result {
        var documentEdits = changes[occurence.location.uri] ?? []
        documentEdits.append(TextEdit(
          range: occurence.location.range,
          newText: req.params.newName
        ))
        changes[occurence.location.uri] = documentEdits
      }

      return WorkspaceEdit(
        changes: changes,
        documentChanges: [], // TODO: Implement file renaming when relevant?
        changeAnnotation: [:]
      )
    }

    req.reply(workspaceEdit)
  }
  let request = Request(symbolInfo, id: req.id, clientID: ObjectIdentifier(self),
                        cancellation: req.cancellationToken, reply: callback)
  languageService.symbolInfo(request)
}

[ahoppen]

I hadn't built it after the last time I cleaned it 🤦‍♂️ It seems that it works in VSCode now (ish). Thanks for the tips :)

That’s good to hear. I was worried something was wrong with how we build the index.

I can successfully get a list of all references to a given non-local symbol now (not sure of the formal word for that, I mean things that aren't local vars). For renaming of local variables I assume that I'll need to deal with the open file's AST.

Yes, that sounds about right to me.

Annoyingly, the ranges of all of the symbols returned are wrong (the start and end locations are both just the start location of the symbol). I think VSCode doesn't like this because it completely ignores any TextEdits I return. However, I've even tried hardcoding an end index adjustment specifically for the symbol I'm attempting to rename, and it still ignores the renames. I'll look into it a bit more later this week, but if you have any insight from your experience of sourcekit-lsp internals that would be useful. It would also be handy if there was a way to inspect why the lsp client is silently ignoring the changes in the rename responses, but I don't know if there is.

I think the index only contains the start location of an occurrence, not the entire range. You will need to determine the token’s range yourself. I would imagine that the easiest way to do it would be to inspect the SwiftSyntax tree (I think there should already be one available in DocumentTokens), search it for the token that contains the start location and get that token’s range.

I've included my current rename implementation in case there's something obviously wrong

Looks good to me. Nothing major that jumps to my mind.

[douglas-reid]

@stackotter are you still pursuing this?

[stackotter]

Nah sorry, I got busy with other projects and uni, I’m happy for someone else to try, or else I might get back to it at some point.

[benjiwolff]

Looks like you are working on this now @ahoppen? I would love to build LSP to check it out, however I am having issues. Is it possible that sourcekitd in is still outdated in the toolchain?

[ahoppen]

Which sourcekitd are you using? Only main development snapshots of sourcekitd (Link) support rename from sourcekit-lsp. If you download a main development snapshot, rename in Swift files should work.

[benjiwolff]

I have installed the latest version of the main development snapshot. However, I have also installed Xcode which I assume came with the toolchain from the lastest release. How can I tell sourcekit-lsp which toolchain it should use?

[ahoppen]

You need to point the Swift VS Code extension to the open source toolchain snapshot. For example by adding the following to your settings.json (replacing the date with the toolchain that you actually downloaded).

"swift.path": "/Library/Developer/Toolchains/swift-DEVELOPMENT-SNAPSHOT-2024-01-08-a.xctoolchain/usr/bin",

[wojciech-kulik]

Thank you for this feature! I tested it using development snapshot and it worked 🔥 Do you know when it should be available in Xcode? I'm not sure what's the cycle? Is it going to be included in the next Xcode version?

[ahoppen]

Rename is merged into the main, which will be part of Swift 5.11.