[clangd] Replace raw lexer code with token buffer in prepare rename.

[hokein]

Summary

there is a slight behavior change in this patch:

• before: in^t a;, returns our internal error message (no symbol at given location)
• after: `in^t a, returns null, and client displays their message (e.g. e.g. "the element can't be renamed" in vscode).

both are sensible according LSP, and we'd save one rename call in the later case.

[merge_guards_bot]

Unit tests: pass. 62257 tests passed, 0 failed and 827 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[sammccall]

Nice, thanks!

clang-tools-extra/clangd/ClangdServer.cpp

321

Agree that this behavior is OK per the spec, but do we actually prefer it/why the change?

Seems like clients are marginally more likely to handle null incorrectly/silently, and we give up control over the message.

323

this runs the lexer to compute token boundaries. We actually already have them: syntax::Token has endLocation/length

clang-tools-extra/clangd/SourceCode.h

302 ↗

(On Diff #241066)

I don't think this function improves things vs keeping it inline:

• wrapping clang functions to adapt them to our data types doesn't seem sustainable
• it's hard to draw the line, but this one neither hides logic (saves 2 trivial lines) nor is very commonly used (yet)
• this breaks a (minor) error-handling path: an invalid range is no longer reported to the client as an error

[hokein]

address comments

[hokein]

revert an accident change.

[hokein]

clang-tools-extra/clangd/ClangdServer.cpp

321

Agree that this behavior is OK per the spec, but do we actually prefer it/why the change?

I checked with VSCode, it seems fine (the element can't be renamed), and YCM (but it doesn't support prepare rename yet). Are you suggesting that we return our message here?

323

removed it.

[sammccall]

clang-tools-extra/clangd/ClangdServer.cpp

321

Doh, I had this confused with rename where the spec is more ambiguous (and e.g. eglot doesn't handle null well). Nevermind.

[merge_guards_bot]

Unit tests: pass. 62257 tests passed, 0 failed and 827 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[merge_guards_bot]

Unit tests: pass. 62257 tests passed, 0 failed and 827 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

clang-tools-extra/clangd/ClangdServer.cpp

 void ClangdServer::prepareRename(PathRef File, Position Pos,
                                  Callback<llvm::Optional<Range>> CB) {
   auto Action = [Pos, File = File.str(), CB = std::move(CB),
                  this](llvm::Expected<InputsAndAST> InpAST) mutable {
     if (!InpAST)
       return CB(InpAST.takeError());
     auto &AST = InpAST->AST;
     const auto &SM = AST.getSourceManager();
-    SourceLocation Loc =
-        SM.getMacroArgExpandedLocation(getBeginningOfIdentifier(
-            Pos, AST.getSourceManager(), AST.getLangOpts()));
-    auto Range = getTokenRange(SM, AST.getLangOpts(), Loc);
-    if (!Range)
-      return CB(llvm::None); // "rename" is not valid at the position.
+    auto Loc = sourceLocationInMainFile(SM, Pos);
+    if (!Loc)
+      return CB(Loc.takeError());
+    const auto *TouchingIdentifier =
+        spelledIdentifierTouching(*Loc, AST.getTokens());
+    if (!TouchingIdentifier)
+      return CB(llvm::None); // no rename on non-identifiers.
+
+    auto Range = halfOpenToRange(
+        SM, CharSourceRange::getCharRange(TouchingIdentifier->location(),
+                                          TouchingIdentifier->endLocation()));
 
     if (CrossFileRename)
       // FIXME: we now assume cross-file rename always succeeds, revisit this.
-      return CB(*Range);
+      return CB(Range);
 
     // Performing the local rename isn't substantially more expensive than
     // doing an AST-based check, so we just rename and throw away the results.
     auto Changes = clangd::rename({Pos, "dummy", AST, File, Index,
                                    /*AllowCrossFile=*/false,
                                    /*GetDirtyBuffer=*/nullptr});
     if (!Changes) {
       // LSP says to return null on failure, but that will result in a generic
       // failure message. If we send an LSP error response, clients can surface
       // the message to users (VSCode does).
       return CB(Changes.takeError());
     }
-    return CB(*Range);
+    return CB(Range);
   };
   WorkScheduler.runWithAST("PrepareRename", File, std::move(Action));
 }
 
 void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName,
                           bool WantFormat, Callback<FileEdits> CB) {
   // A snapshot of all file dirty buffers.
   llvm::StringMap<std::string> Snapshot = WorkScheduler.getAllFileContents();

clang-tools-extra/clangd/test/rename.test

 # CHECK-NEXT:      },
 # CHECK-NEXT:      "start": {
 # CHECK-NEXT:        "character": 4,
 # CHECK-NEXT:        "line": 0
 # CHECK-NEXT:      }
 # CHECK-NEXT:  }
 ---
 {"jsonrpc":"2.0","id":2,"method":"textDocument/prepareRename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":2}}}
-#      CHECK:  "error": {
-# CHECK-NEXT:    "code": -32001,
-# CHECK-NEXT:    "message": "Cannot rename symbol: there is no symbol at the given location"
-# CHECK-NEXT:  },
-# CHECK-NEXT:  "id": 2,
-# CHECK-NEXT:  "jsonrpc": "2.0"
+#      CHECK:  "id": 2,
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": null
 ---
 {"jsonrpc":"2.0","id":4,"method":"textDocument/rename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":2},"newName":"bar"}}
 #      CHECK:  "error": {
 # CHECK-NEXT:    "code": -32001,
 # CHECK-NEXT:    "message": "Cannot rename symbol: there is no symbol at the given location"
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "id": 4,
 # CHECK-NEXT:  "jsonrpc": "2.0"