swiftlang / sourcekit-lsp

Language Server Protocol implementation for Swift and C-based languages
Apache License 2.0
3.32k stars 276 forks source link

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

Closed adam-fowler closed 8 months ago

adam-fowler commented 2 years ago
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 commented 2 years ago

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

ahoppen commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

rdar://95587948

adam-fowler commented 2 years ago

@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 commented 2 years ago

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

MartinLau7 commented 1 year ago

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

stackotter commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Could you try the following

stackotter commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@stackotter are you still pursuing this?

stackotter commented 1 year ago

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 commented 9 months ago

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?

image

ahoppen commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 8 months ago

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 commented 8 months ago

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