[clangd] Implement cross-file rename.

[hokein]

Summary

This is the initial version. The cross-file rename is purely based on
the index (plus a text-match verification).

It is hidden under a command-line flag, and only available for a small set
of symbols.

[hokein]

tests are missing, but the patch should be sufficient for initial review.

[merge_guards_bot]

Build result: fail - 33608 tests passed, 1 failed and 462 were skipped.

failed: LLVM.tools/llvm-ar/mri-utf8.test

Log files: cmake-log.txt, ninja_check_all-log.txt, CMakeCache.txt

[ilya-biryukov]

Thanks for implementing this.
I think we could split it into multiple changes to make understanding it easier, see inline comments, I've tried to point out the places I find relevant.

Would definitely be nice to have some unit-tests for this.

Another important concern is surfacing errors to the users: silently dropping ranges for stale files is definitely not the nicest option, I'm afraid this will lead to non-explainable failure modes and users will be incredibly unhappy...

clang-tools-extra/clangd/ClangdLSPServer.cpp

106

NIT: rename to FE

clang-tools-extra/clangd/ClangdServer.h

334

NIT: this comment is redundant

clang-tools-extra/clangd/index/SymbolCollector.cpp

268

Not sure what was filtered out here.
I suggest moving this to a separate change, with unit-tests clearly describing the new symbols we do not filter out anymore and an explanation why it's necessary.

WDYT?

clang-tools-extra/clangd/refactor/Rename.cpp

146

So llvm::None means we do not reject?
Probably worth documenting that.

Maybe even add an enumerator ReasonToReject::Accept, indicating the symbol should be accepted and get rid of Optional altogether.

Otherwise this leads to code like:

auto R = renameableOutsideFile(N, I);
if (!R) { // negative condition.
 return doRename(N, I); // But we're running the rename anyway... WAT?
}
``

148

This function resembles renamableWithinFile.
We should probably share implementation and pass an extra flag. Something like isRenamable(..., bool IsCrossFile).

WDYT?

179

NIT: else is redundant

301

I feel this code is fundamental to the trade-offs we made for rename.
Could we move this to a separate function and add unit-tests for it?

You probably want to have something that handles just a single file rather than all edits in all files, to avoid mocking VFS in tests, etc.

364

Can we rely on the fact that dynamic index should not be stale for the current file instead?
Or don't we have this invariant?

We end up having two implementations now: index-based and AST-based. It'd be nice to have just one.
If that's not possible in the first revision, could you explain why?

Note that moving the current-file rename to index-based implementation is independent of doing cross-file renames. Maybe we should start with it?

clang-tools-extra/clangd/refactor/Rename.h

33

Have we considered post-filtering instead?
Or are we concerned with performance implications of openning too many files to compute the corrected edit ranges?

Probably worth documenting why we need it.

33

NIT: the comment is redundant

clang-tools-extra/clangd/tool/ClangdMain.cpp

269

s/fature/feature

270

Indicating the default is probably redundant, LLVM options-parsing library will indicate the default on command-line anyway.

clang/lib/Tooling/Refactoring/Rename/USRFindingAction.cpp

44 ↗

(On Diff #225889)

Could you move this to a separate change and add corresponding unit-tests?

[hokein]

Thanks for the comments.

In D69263#1716760, @ilya-biryukov wrote:

Another important concern is surfacing errors to the users: silently dropping ranges for stale files is definitely not the nicest option, I'm afraid this will lead to non-explainable failure modes and users will be incredibly unhappy...

Agree, I think we should surface this error to users when the index is stale or we don't have enough confident to perform the rename.

Thinking more about this -- we have a dynamic index (for all opened files) which is overlaid on a static index (which is a background index in open-source world), so for all affected files, they are either in

1. an open state -- we can rely on the dynamic index, I think it is safe to assume that index always returns up-to-date results;
2. a non-open state -- rely on the background index, however background index has more chance to be stale (especially we don't detect file-change events at the moment), we could do a range patch heuristically to mitigate this stale issue. Failing that, we surface the error to users.

[ilya-biryukov]

In D69263#1717985, @hokein wrote:

Thinking more about this -- we have a dynamic index (for all opened files) which is overlaid on a static index (which is a background index in open-source world), so for all affected files, they are either in

1. an open state -- we can rely on the dynamic index, I think it is safe to assume that index always returns up-to-date results;

Not sure that holds. What if the file in question is being rebuilt right now? We do not wait until all ASTs are built (and the dynamic index gets the new results).
Open files actually pose an interesting problem: their contents do not correspond to the file contents on disk.
We should choose which of those we update on rename: dirty buffers or file contents? (I believe we should do dirty buffers, but I believe @sammccall has the opposite opinion, so we should sync on this)

2. a non-open state -- rely on the background index, however background index has more chance to be stale (especially we don't detect file-change events at the moment), we could do a range patch heuristically to mitigate this stale issue. Failing that, we surface the error to users.

To summarize, I think files can be stale in both cases and we should patch the ranges in both cases.

[hokein]

• address comments;
• add more FIXMEs;

[hokein]

In D69263#1718525, @ilya-biryukov wrote:

In D69263#1717985, @hokein wrote:

Thinking more about this -- we have a dynamic index (for all opened files) which is overlaid on a static index (which is a background index in open-source world), so for all affected files, they are either in

1. an open state -- we can rely on the dynamic index, I think it is safe to assume that index always returns up-to-date results;

Not sure that holds. What if the file in question is being rebuilt right now? We do not wait until all ASTs are built (and the dynamic index gets the new results).

I'm not sure this would happen quite often in practice (probably depends on users' behavior). but yes, patching ranges would mitigate this issue.

Open files actually pose an interesting problem: their contents do not correspond to the file contents on disk.
We should choose which of those we update on rename: dirty buffers or file contents? (I believe we should do dirty buffers, but I believe @sammccall has the opposite opinion, so we should sync on this)

I have the same feeling of using dirty buffers for open files, let's discuss offline.

2. a non-open state -- rely on the background index, however background index has more chance to be stale (especially we don't detect file-change events at the moment), we could do a range patch heuristically to mitigate this stale issue. Failing that, we surface the error to users.

To summarize, I think files can be stale in both cases and we should patch the ranges in both cases.

clang-tools-extra/clangd/index/SymbolCollector.cpp

268

Agreed. split it out.

clang-tools-extra/clangd/refactor/Rename.cpp

146

yeah, returning None means that we accept the rename.

Adding Accept is not aligning with the mental model, Accept doesn't belong to ReasonToReject I think. I agree the above given code is misleading, but looking at the current code in this file, it doesn't seem too bad, I think?

if (auto Reject = renameableOutsideFile(*RenameDecl, RInputs.Index))
   return makeError(*Reject);

148

though they look similar, the logic is quite different, for example, we query the index in within-file rename to detect a symbol is used outside of the file which is not needed for cross-file rename, and cross-file rename allows fewer symbols (as our index doesn't support them), so I don't think we can share a lot of implementation.

301

Agree, especially when we start implementing complicated stuff, e.g. range patching heuristics.

Moved it out, but note that I don't plan to add more stuff in this initial patch, so the logic is pretty straightforward, just assembling rename replacement from occurrences.

364

I think we can't get rid of the AST-based rename -- mainly for renaming local symbols (e.g. local variable in function body), as we don't index these symbols...

clang-tools-extra/clangd/refactor/Rename.h

33

Performance is one of the concern. I think the main purpose of this flag is to avoid any regressions in our existing within-file rename while developing the new cross-file rename.

[merge_guards_bot]

Build result: fail - 59475 tests passed, 2 failed and 805 were skipped.

failed: Clangd.Clangd/rename.test
failed: LLVM.tools/llvm-ar/mri-utf8.test

Log files: cmake-log.txt, ninja_check_all-log.txt, CMakeCache.txt

[merge_guards_bot]

Build result: fail - 59518 tests passed, 1 failed and 763 were skipped.

failed: Clangd.Clangd/rename.test

Log files: cmake-log.txt, ninja_check_all-log.txt, CMakeCache.txt

[ilya-biryukov]

clang-tools-extra/clangd/refactor/Rename.cpp

146

Agreed this aligns with the other functions we have in the file, so let's keep as is.

148

Can this be handled with a special flag:

bool renameable(NamedDecl &Decl, const SymbolIndex *Index, bool CrossFile) {
  if (CrossFileRename) {
    // check something special...
  }
}

Sharing implementation in the same function makes the differences between cross-file and single-file case obvious and also ensures any things that need to be updated for both are shared.

301

but note that I don't plan to add more stuff in this initial patch

Should we also check whether the replaced text matches the expected identifier and add unit-tests for this?
Or would you prefer to move all changes to this function to a separate patch?

341

Can we avoid duplicating calls to renamebleWithinFile? It should be possible to only call renameOutsideFile when CrossFileRename is enabled and always call renameWithinFile. Or is there something I'm missing?

364

Thanks, I believe you're right... We can't unify these implementations, at least not in the short term.

So renameOutsideFile fails on local symbols and renameWithinFile should handle that, right?
Also worth noting that renameWithinFile is AST-based and renameOutsideFile is index-based.
Could we document that?

[hokein]

• simplify the code, addressing comments;
• add unittests;

[hokein]

In D69263#1718525, @ilya-biryukov wrote:

Not sure that holds. What if the file in question is being rebuilt right now? We do not wait until all ASTs are built (and the dynamic index gets the new results).
Open files actually pose an interesting problem: their contents do not correspond to the file contents on disk.
We should choose which of those we update on rename: dirty buffers or file contents? (I believe we should do dirty buffers, but I believe @sammccall has the opposite opinion, so we should sync on this)

Based on the offline discussion, we decide to use dirty buffers for opened files.

clang-tools-extra/clangd/refactor/Rename.cpp

148

Done. I tried my best to unify them, please take a look on the new code.

301

I prefer to do it afterwards as the patch is relatively big now.

341

I was attempting to keep two branches (within-file rename, cross-file rename) as separately as possible, but I agree with your suggestion, it does simplify the code.

364

yes, exactly. I have simplified the code in this function, and added documentations.

[merge_guards_bot]

Build result: fail - 59520 tests passed, 1 failed and 763 were skipped.

failed: Clangd.Clangd/rename.test

Log files: cmake-log.txt, ninja_check_all-log.txt, CMakeCache.txt

[ilya-biryukov]

clang-tools-extra/clangd/tool/ClangdMain.cpp

269

Could you document that the feature is highly experimental and may lead to broken code or incomplete renames for the time being?

Also, maybe make it hidden for now?
At least until we have basic validation of ranges from the index. In the current state we can easily produce edits to unrelated parts of the file...

clang-tools-extra/clangd/unittests/RenameTests.cpp

277

Thinking whether we can encode these transformations in a nicer way?
If we could convert the contents dirty buffer and replacements to something like:

class [[Foo |-> newName]] {};

Just a proposal, maybe there's a better syntax here.

We can easily match strings instead of matching ranges in the tests. This has the advantage of having textual diffs in case something goes wrong - much more pleasant than looking at the offsets in the ranges.

WDYT?

302

Instead of defining custom matchers, could we convert to standard types and use existing gtest matchers?
Something like std::vector<std::pair</*Path*/std::string, Edit>> should work just fine.

Huge advantage we'll get is better output in case of errors.

[hokein]

address comments:

• make the command-line flag hidden;
• use the gtesting matcher;

clang-tools-extra/clangd/tool/ClangdMain.cpp

269

Done. yes, we should make it hidden.

clang-tools-extra/clangd/unittests/RenameTests.cpp

277

I agree textual diff would give a more friendly results when there are failures in the tests.

I don't have a better way to encode the transformation in the annotation code, I think a better way maybe to use a hard-coded new name, and applying the actual replacements on the testing code, and verify the the text diffs.

If we want to do this change, I'd prefer to do it in a followup, since it would change the existing testcases as well. What do you think?

302

yeah, converting the llvm::StringMap to standard types could work, but we have to pay the cost of transformation (that was something I tried to avoid).
I think there is no strong reason to use llvm::StringMap as the FileEdits, what do you think if we change the FileEdits type to std::vector<pair<string, Edit>>?

[merge_guards_bot]

Build result: pass - 59701 tests passed, 0 failed and 763 were skipped.
Log files: cmake-log.txt, ninja_check_all-log.txt, CMakeCache.txt

[ilya-biryukov]

Thanks, mostly LG!

The only important comment I have left is about limiting the number of references. Others are NITs.

clang-tools-extra/clangd/refactor/Rename.cpp

219

Why do we need to limit the number of references in the first place?

223

Maybe remove this comment?

224

Why not extract this as a helper function?

233

NIT: maybe put this comment on the line before the declaration?
Should lead to better formatting

Also, this comment would be most useful on the function declaration. Maybe move it there?

274

NIT: Maybe call it renameWithIndex instead?
It should capture what this function is doing better...

But up to you

289

Maybe move this comment to the function itself?
It definitely does a great job of capturing what this function is doing and what the trade-offs are.

clang-tools-extra/clangd/unittests/RenameTests.cpp

277

Maybe I'm overthinking it... For rename having just the range is probably enough.

clang-tools-extra/clangd/unittests/SyncAPI.cpp

103

s/DirtyBuffaer/DirtyBuffer

also use /*DirtryBuffer=*/ to be consistent with /*WantFormat=*/ from the previous line

[hokein]

address comments.

[hokein]

clang-tools-extra/clangd/refactor/Rename.cpp

219

I was thinking this is for performance, if the index returns too many results (the number of affected files >> our limit), we will ends up doing many unnecessary work (like resolving URIs). Removed it now, it seems a premature optimization, we can revisit this afterwards.

274

I would keep the current name, as it is symmetrical to the renameWithinFile.

[merge_guards_bot]

Build result: pass - 59816 tests passed, 0 failed and 762 were skipped.
Log files: console-log.txt, CMakeCache.txt

[ilya-biryukov]

clang-tools-extra/clangd/refactor/Rename.cpp

220

NIT: grouped by the absolute file name? Or is there a better term than "absolute" to describe this?

274

Yeah, those go together. Using renameWithIndex would mean the other one should be renameWithAST (or similar).
Feel free to keep as is too.

clang-tools-extra/clangd/refactor/Rename.h

23

Could you document what does it mean for this function to return llvm::None?
I assume we read the contents from disk instead.

Also worth documenting what should be returned for MainFilePath? llvm::None? same value as MainFileCode?

24

NIT: maybe use function_ref? We definitely don't store this function anywhere.

[hokein]

Add comments for DirtyBufferGetter.

[merge_guards_bot]

Build result: pass - 59816 tests passed, 0 failed and 762 were skipped.
Log files: console-log.txt, CMakeCache.txt

[ilya-biryukov]

clang-tools-extra/clangd/ClangdLSPServer.cpp

766

NIT: is capture of this redundant here?

I could be missing something, though.

clang-tools-extra/clangd/ClangdServer.cpp

334

Why not do this before running rename? This would allow to:

1. return result without doing the actual (expensive) rename if there's no identifier under the cursor
2. reduce nesting of the (now large) large piece of code that runs rename:

auto Loc = getBeginningOfIdentifier(...);
auto Range = ...;
if (!Range)
  return llvm::None;
if (!CrossFileRename)
  return *Range; // FIXME: assume cross-file rename always succeeds
// do the rename...

353–364

Could we get a helper function (or constructor) to keep the call sites as simple as they used to be?
RenameInputs{Contents, File, NewName, Pos,...} should also work just fine and I don't think it hurts readability, since all the parameters here are named the same as the fields and most of them have different types.

360

This can always be obtained from the AST, right?
Why do we need it separately?

370

Maybe use llvm::joinErrors to combine multiple failures into a single error?
Should simplify the code.

clang-tools-extra/clangd/SourceCode.h

226

We should document this typedef, otherwise it does not provide much value.
A doc comment from ApplyEdits should fit just nicely here.

clang-tools-extra/clangd/refactor/Rename.h

23

What does it return for MainFilePath? llvm::None? MainFileCode?

[ilya-biryukov]

clang-tools-extra/clangd/refactor/Rename.cpp

75

NIT: in both rename modes

clang-tools-extra/clangd/refactor/Rename.h

51

MainFileCode can be obtained from the AST. Why not do it?

58

Why is MainFile special? Can it also be handled with the GetDirtyBuffer?

67

Why isn't AST part of the RenameInputs?

[hokein]

address comments:

• use VFS from AST;
• simplify the interfaces;

clang-tools-extra/clangd/ClangdLSPServer.cpp

766

it is not redundant, we access the this->DraftMgr in the lambda body.

clang-tools-extra/clangd/ClangdServer.cpp

334

Good point.

360

yes, obtained from the AST.

370

I don't see using llvm::joinErrors can simplify the code here, joinErrors accepts two Error objects, and there is no way to create an empty Error object, we may end up with more code like:

llvm::Optional<llvm::Error> Err;
for (auto &E : *Edits) {
   if (auto E = reformatEdit(E.getValue(), Style)) {
      if (!Err) Err = std::move(E);
      else Err = joinErrors(std::move(*Err), std::move(Err));
   }
}

clang-tools-extra/clangd/refactor/Rename.h

51

ParsedAST doesn't provide such API to get the file code, I assume you mean InputsAndAST? The MainFileCode is from InputsAndAST.

58

I believe we should use the content from InputsAndAST, which is corresponding to the main AST (the content from GetDiryBuffer may not be reflected to the AST).

[merge_guards_bot]

Build result: pass - 59816 tests passed, 0 failed and 762 were skipped.
Log files: console-log.txt, CMakeCache.txt

[ilya-biryukov]

clang-tools-extra/clangd/ClangdServer.cpp

361

NIT: inlining RInputs should save quite a few lines of code

370

Works nicely if you start with Error::success

auto Err = Error::success();
for (...) {
  Err = llvm::joinErrors(reformatEdit(E.getValue(), Style));
}
if (Err)
  return CB(std::move(Err));

clang-tools-extra/clangd/refactor/Rename.cpp

95

NIT: this comment is redundant, previous line says the same thing

111

Why isn't this a scope enum in the first place?

132

Why not a blacklist? I'd expect us to blacklist:

• things with custom names (operators, constructors, etc)
• virtual methods

Probably some things are missing from the list, but fundamentally most that have names are pretty similar, right?

136

Isn't the same true for local rename?

225

This behavior is very surprising... Obviously, returning empty results is incorrect and the callers have to handle the no-index case in a custom manner.

Maybe accept only non-null index and handle in the caller?

239

Again, it looks like the function returns incorrect results and the callers should actually handle the case where SymbolID cannot be obtained in a custom manner.

Maybe accept a SymbolID as a parameter instead of NamedDecl* and the callers handle this case gracefully?

259

This is not true anymore, right?
We should probably try getting to O(N) to avoid slowing things down

Could be a FIXME for now, but have to fix it soon.

clang-tools-extra/clangd/refactor/Rename.h

51

SourceManager has access to the input buffer.
StringRef MainFileCode = SM.getBufferData(SM.getMainFileID());

58

Does that mean we have three states of files that rename should be aware of?

• Dirty buffer
• Main file contents
• Contents on disk

That definitely looks very complicated... Is there a chance we could abstract it away and just have a single source of truth for file contents?
I am thinking of a function that handles getting the file buffer by name, so we have all the complicated logic in one place. Would look something like:

std::function<std::string(PathRef /*Path*/)> GetFileContents;

Implementation could try to do the following in order:

1. try to get contents of the main file.
2. if failed, get contents from the dirty buffers.
3. if failed, get contents from disk.

Could be a helper function accepting RenameInputs, but important to use it everywhere we get the file contents.

[hokein]

address review comments.

[hokein]

clang-tools-extra/clangd/ClangdServer.cpp

370

thanks.

clang-tools-extra/clangd/refactor/Rename.cpp

111

this is tricky, the reason why I put it here and another copy below is to avoid regression of local rename (keep existing tests passed), if we move it in the first place, the error message of local rename may be changed, e.g. when renaming an external symbol (we know that from AST) without Index support, we'd like to return "used outside of the main file" rather than "no index provided".

thinking more about this, the no-index case is making our code complicated (within-file rename with no-index, cross-file rename with no-index), I believe the no-index case is not important, and is rare in practice, we could change this behavior, or assume the index is always provided, this would help to simplify the code, WDYT?

132

I believe our plan is to support major kinds of symbols (class, functions, enums, typedef, alias) , I feel scary to allow renaming nearly all NamedDecl (in theory, it should work but we're uncertain, for our experience of local rename, it sometimes leads to very surprising results). Having a whitelist can lead us to a better state, these symbols are officially supported.

136

no, actually the local rename does handle this, it finds all overriden methods from the AST, and updates all of them when doing rename.

225

moved the no-index handling to the caller, the caller code is still a bit fuzzy.

239

hmm, is this critical? I think it is safe to assume getSymbolID should always succeed, I believe we make this assumption in other code parts in clangd as well.

259

yes, I think it is not too bad to leave it now (as we limit the max number of affected number to 50), the idea to fix this is to build a line table. Updated the FIXME.

clang-tools-extra/clangd/refactor/Rename.h

58

yeah, having a single source for the file content could help, but I think this helper is internal-only.

[merge_guards_bot]

Build result: fail - 59795 tests passed, 21 failed and 762 were skipped.

failed: lld.ELF/linkerscript/filename-spec.s
failed: Clang.Index/index-module-with-vfs.m
failed: Clang.Modules/double-quotes.m
failed: Clang.Modules/framework-public-includes-private.m
failed: Clang.VFS/external-names.c
failed: Clang.VFS/framework-import.m
failed: Clang.VFS/implicit-include.c
failed: Clang.VFS/include-mixed-real-and-virtual.c
failed: Clang.VFS/include-real-from-virtual.c
failed: Clang.VFS/include-virtual-from-real.c
failed: Clang.VFS/include.c
failed: Clang.VFS/incomplete-umbrella.m
failed: Clang.VFS/module-import.m
failed: Clang.VFS/module_missing_vfs.m
failed: Clang.VFS/real-path-found-first.m
failed: Clang.VFS/relative-path.c
failed: Clang.VFS/test_nonmodular.c
failed: Clang.VFS/umbrella-framework-import-skipnonexist.m
failed: Clang.VFS/vfsroot-include.c
failed: Clang.VFS/vfsroot-module.m
failed: Clang.VFS/vfsroot-with-overlay.c

Log files: console-log.txt, CMakeCache.txt

[ilya-biryukov]

clang-tools-extra/clangd/refactor/Rename.cpp

111

Agree, we shouldn't bother about the no-index case too much.
My comment here was referring to the lack of ReasonToReject:: qualifier of the name.

Maybe make ReasonToReject a scoped enum, i.e. enum class ReasonToReject rather than enum ReasonToReject?

132

I disagree here, I don't see how NamedDecl is special if its name is an identifier unless we something smart about it.
My recollection is that local rename issues come from the fact that there are certain kinds of references, which are not handled in the corresponding visitor.

In any case, I'd suggest blacklisting symbols we know are broken, e.g. IIRC we don't index references to namespaces, so they're definitely broken. And finding the issues with the rest of the symbols and fixing those.
In practice, this should improve both rename and cross-references - whenever we have broken cross-file rename, there's a good chance that the indexer is also broken.

136

Do we properly check overriden methods are not used outside the main file, though?
We might be assuming rename is "local" because there are not usages of the original function, but there may still be usages of the overriden function.

Worth adding a comment that local rename handles that through the AST.
Also useful to keep in mind that this is a regression, compared to the local rename (i.e. if we switch to cross-file rename by default, we'll break this use-case)

239

Should we maybe assert it succeeds instead?
If we make this assumption in the code, why not make it explicit?
If not, we should probably add tests where it fails.

259

Agree it's ok to keep as is for now.
But we should fix it before switching this as a default. It's not uncommon to have a single large file with lots of occurrences. We really don't have O(N^2) in those cases.

clang-tools-extra/clangd/refactor/Rename.h

58

I might be missing something, but it's in the public API of rename, right?

[hokein]

• use blacklist
• change to enum scope

[hokein]

clang-tools-extra/clangd/refactor/Rename.cpp

111

ah, I see. Done.

132

Re-thinking this, you are right. Also, the indexable symbol kinds have a large overlap with the renamable symbol kinds. Changed to blacklist.

136

Do we properly check overriden methods are not used outside the main file, though?
We might be assuming rename is "local" because there are not usages of the original function, but there may still be usages of the overriden function.

No, we only check the original function, and we may encounter false positives, but we don't see any reports about this.

Worth adding a comment that local rename handles that through the AST.
Also useful to keep in mind that this is a regression, compared to the local rename (i.e. if we switch to cross-file rename by default, we'll break this use-case)

exactly, a way to avoid this regression when turning on cross-file rename by default is to first detect whether it is safe&sufficient to perform local rename, if yes, we just perform the local-rename.

clang-tools-extra/clangd/refactor/Rename.h

58

my concern is that choosing which source of the file content is an implementation detail and coupled with how the rename is implemented, why should the caller (ClangdServer?) create this complicated function?

[merge_guards_bot]

Build result: fail - 59861 tests passed, 21 failed and 763 were skipped.

failed: lld.ELF/linkerscript/filename-spec.s
failed: Clang.Index/index-module-with-vfs.m
failed: Clang.Modules/double-quotes.m
failed: Clang.Modules/framework-public-includes-private.m
failed: Clang.VFS/external-names.c
failed: Clang.VFS/framework-import.m
failed: Clang.VFS/implicit-include.c
failed: Clang.VFS/include-mixed-real-and-virtual.c
failed: Clang.VFS/include-real-from-virtual.c
failed: Clang.VFS/include-virtual-from-real.c
failed: Clang.VFS/include.c
failed: Clang.VFS/incomplete-umbrella.m
failed: Clang.VFS/module-import.m
failed: Clang.VFS/module_missing_vfs.m
failed: Clang.VFS/real-path-found-first.m
failed: Clang.VFS/relative-path.c
failed: Clang.VFS/test_nonmodular.c
failed: Clang.VFS/umbrella-framework-import-skipnonexist.m
failed: Clang.VFS/vfsroot-include.c
failed: Clang.VFS/vfsroot-module.m
failed: Clang.VFS/vfsroot-with-overlay.c

Log files: console-log.txt, CMakeCache.txt

[ilya-biryukov]

Thanks, this is in a good shape!
The only concern I have is the racy way to get dirty buffers, please see the corresponding comment.

clang-tools-extra/clangd/ClangdLSPServer.cpp

764

We should probably get a snapshot of all dirty buffers here instead.
A racy way (rename is run on a separate thread, new files updates might come in in the meantime) to get contents of the file looks like a bad idea, this will lead to hard-to-debug failures...

Note that ClangdServer also has access to all contents of all the files, they are stored in TUScheduler, so we don't need to pass GetDirtyBuffer callback up until the final run of rename

clang-tools-extra/clangd/ClangdServer.cpp

316

NIT: comment could be shortened to

/// "rename" is not valid at the position.

Or even removed.
Both would allow saving a line (if we put the comment on the same line as return

319

NIT: s/assueme/assume

also, in FIXME we normally give the final state, so it should probably be

/// FIXME: do not assume cross-file rename always succeeds

346–347

ClangdServer can obtain the dirty buffers via TUScheduler::getContents

[hokein]

• get dirty buffer in clangdServer layer;
• save a snpatshot for all dirty buffer;

[hokein]

clang-tools-extra/clangd/ClangdLSPServer.cpp

764

thanks, I like the idea. This could simplify the code, with this approach, the dirty buffer of the main file would be the same as the one used for building the AST.

[ilya-biryukov]

clang-tools-extra/clangd/ClangdServer.cpp

348

NIT: s/SnapShot/Snapshot

377

NIT: return CB(std::move(*Edits));
to keep all calls to CB consistent.

clang-tools-extra/clangd/TUScheduler.h

186 ↗

(On Diff #228213)

NIT: seems like a leftover from the previous version. Maybe remove?

clang-tools-extra/clangd/refactor/Rename.cpp

51

NIT: this assert is redundant, Optional asserts it has a value when operator* is called.
The code could be simplified to the equivalent: Refs.insert(*getSymbolID(&D))

145

nit: add braces to the outer if

249

NIT: remove <>. should still work, right?

302

We don't seem to use it. Remove? Or am I missing the usage?

350

Why is this different from prepareRename, which does not call getMacroArgExpandedLocation?

384

NIT: remove <>

clang-tools-extra/clangd/refactor/Tweak.h

76

The comment is now redundant, since the typedef has the same comment.

clang-tools-extra/clangd/unittests/RenameTests.cpp

33

Could you document what this function is doing?

58

NIT: I suggest to call it applyEdits, there's nothing rename-specific in this function.

139–146

NIT: maybe simplify to EXPECT_EQ(applyRename(...).second, expectedResult(Code, NewName))?

[hokein]

address comments.

[hokein]

clang-tools-extra/clangd/refactor/Rename.cpp

350

I didn't change it in this patch, but you raise a good point, prepareRename should call getMacroArgExpandedLocation.

[ilya-biryukov]

LGTM.

It's probably worth collecting a list of things we need to fix before enabling cross-file rename and putting it somewhere (a GitHub issue, maybe?)
Important things that immediately come to mind:

• add range-patching heuristics, measure how good they are
• avoid O(N^2) when computing edits (converting from positions to offsets)
• handle implicit references from the index

There are definitely more.

clang-tools-extra/clangd/refactor/Rename.cpp

350

Yep, makes sense to change it later. Could you put a FIXME, so that we don't forget about it?

[hokein]

rebase and call getMacroArgExpandedLocation in prepareRename.

[hokein]

In D69263#1738289, @ilya-biryukov wrote:

LGTM.

It's probably worth collecting a list of things we need to fix before enabling cross-file rename and putting it somewhere (a GitHub issue, maybe?)
Important things that immediately come to mind:

• add range-patching heuristics, measure how good they are
• avoid O(N^2) when computing edits (converting from positions to offsets)
• handle implicit references from the index

There are definitely more.

Thanks, filed at issues#193.

clang-tools-extra/clangd/refactor/Rename.cpp

350

Done, fixed this in this patch, it is trivial.

[hokein]

Rebase.

[merge_guards_bot]

Build result: pass - 60175 tests passed, 0 failed and 732 were skipped.
Log files: console-log.txt, CMakeCache.txt

clang-tools-extra/clangd/ClangdLSPServer.cpp

std::vector<std::vector<std::string>> buildHighlightScopeLookupTable() {
   std::vector<std::vector<std::string>> LookupTable;
   // HighlightingKind is using as the index.
   for (int KindValue = 0; KindValue <= (int)HighlightingKind::LastKind;
        ++KindValue)
     LookupTable.push_back({toTextMateScope((HighlightingKind)(KindValue))});
   return LookupTable;
 }
 
-// Makes sure edits in \p E are applicable to latest file contents reported by
+// Makes sure edits in \p FE are applicable to latest file contents reported by
 // editor. If not generates an error message containing information about files
 // that needs to be saved.
-llvm::Error validateEdits(const DraftStore &DraftMgr, const Tweak::Effect &E) {
+llvm::Error validateEdits(const DraftStore &DraftMgr, const FileEdits &FE) {
   size_t InvalidFileCount = 0;
   llvm::StringRef LastInvalidFile;
-  for (const auto &It : E.ApplyEdits) {
+  for (const auto &It : FE) {
     if (auto Draft = DraftMgr.getDraft(It.first())) {
       // If the file is open in user's editor, make sure the version we
       // saw and current version are compatible as this is the text that
       // will be replaced by editors.
       if (!It.second.canApplyTo(*Draft)) {
         ++InvalidFileCount;
         LastInvalidFile = It.first();
       }
auto Action = [this, ApplyEdit, Reply = std::move(Reply),
         Msg.message = *R->ShowMessage;
         Msg.type = MessageType::Info;
         notify("window/showMessage", Msg);
       }
       // When no edit is specified, make sure we Reply().
       if (R->ApplyEdits.empty())
         return Reply("Tweak applied.");
 
-      if (auto Err = validateEdits(DraftMgr, *R))
+      if (auto Err = validateEdits(DraftMgr, R->ApplyEdits))
         return Reply(std::move(Err));
 
       WorkspaceEdit WE;
       WE.changes.emplace();
       for (const auto &It : R->ApplyEdits) {
         (*WE.changes)[URI::createFile(It.first()).toString()] =
             It.second.asTextEdits();
       }

 
 void ClangdLSPServer::onRename(const RenameParams &Params,
                                Callback<WorkspaceEdit> Reply) {
   Path File = Params.textDocument.uri.file();
   llvm::Optional<std::string> Code = DraftMgr.getDraft(File);
   if (!Code)
     return Reply(llvm::make_error<LSPError>(
         "onRename called for non-added file", ErrorCode::InvalidParams));
-
-  Server->rename(File, Params.position, Params.newName, /*WantFormat=*/true,
-                 [File, Code, Params, Reply = std::move(Reply)](
-                     llvm::Expected<std::vector<TextEdit>> Edits) mutable {
-                   if (!Edits)
-                     return Reply(Edits.takeError());
-
-                   WorkspaceEdit WE;
-                   WE.changes = {{Params.textDocument.uri.uri(), *Edits}};
-                   Reply(WE);
-                 });
+  Server->rename(
+      File, Params.position, Params.newName,
+      /*WantFormat=*/true,
+      [this](PathRef File) { return DraftMgr.getDraft(File); },
+      [File, Code, Params, Reply = std::move(Reply),
+       this](llvm::Expected<FileEdits> Edits) mutable {
+        if (auto Err = validateEdits(DraftMgr, *Edits))
+          return Reply(std::move(Err));
+        WorkspaceEdit Result;
+        Result.changes.emplace();
+        for (const auto &Rep : *Edits) {
+          (*Result.changes)[URI::createFile(Rep.first()).toString()] =
+              Rep.second.asTextEdits();
+        }
+        Reply(Result);
 }
 
 void ClangdLSPServer::onDocumentDidClose(
     const DidCloseTextDocumentParams &Params) {
   PathRef File = Params.textDocument.uri.file();
   DraftMgr.removeDraft(File);
   Server->removeDocument(File);
 

clang-tools-extra/clangd/ClangdServer.h

 #include "GlobalCompilationDatabase.h"
 #include "Protocol.h"
 #include "SemanticHighlighting.h"
 #include "TUScheduler.h"
 #include "XRefs.h"
 #include "index/Background.h"
 #include "index/FileIndex.h"
 #include "index/Index.h"
+#include "refactor/Rename.h"
 #include "refactor/Tweak.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/FunctionExtras.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include <functional>
struct Options {
 
     /// Clangd will execute compiler drivers matching one of these globs to
     /// fetch system include path.
     std::vector<std::string> QueryDriverGlobs;
 
     /// Enable semantic highlighting features.
     bool SemanticHighlighting = false;
 
+    /// Enable cross-file rename feature.
+    bool CrossFileRename = false;
+
     /// Returns true if the tweak should be enabled.
     std::function<bool(const Tweak &)> TweakFilter = [](const Tweak &T) {
       return !T.hidden(); // only enable non-hidden tweaks.
     };
   };
   // Sensible default options for use in tests.
   // Features like indexing must be enabled if desired.
   static Options optsForTest();
void prepareRename(PathRef File, Position Pos,
                      Callback<llvm::Optional<Range>> CB);
 
   /// Rename all occurrences of the symbol at the \p Pos in \p File to
   /// \p NewName.
   /// If WantFormat is false, the final TextEdit will be not formatted,
   /// embedders could use this method to get all occurrences of the symbol (e.g.
   /// highlighting them in prepare stage).
   void rename(PathRef File, Position Pos, llvm::StringRef NewName,
-              bool WantFormat, Callback<std::vector<TextEdit>> CB);
+              bool WantFormat, DirtyBufferGetter GetDirtyBuffer,
+              Callback<FileEdits> CB);
 
   struct TweakRef {
     std::string ID;    /// ID to pass for applyTweak.
     std::string Title; /// A single-line message to show in the UI.
     Tweak::Intent Intent;
   };
   /// Enumerate the code tweaks available to the user at a specified point.
   void enumerateTweaks(PathRef File, Range Sel,
private:
 
   // When set, provides clang-tidy options for a specific file.
   ClangTidyOptionsBuilder GetClangTidyOptions;
 
   // If this is true, suggest include insertion fixes for diagnostic errors that
   // can be caused by missing includes (e.g. member access in incomplete type).
   bool SuggestMissingIncludes = false;
 
+  bool CrossFileRename = false;
+
   std::function<bool(const Tweak &)> TweakFilter;
 
   // GUARDED_BY(CachedCompletionFuzzyFindRequestMutex)
   llvm::StringMap<llvm::Optional<FuzzyFindRequest>>
       CachedCompletionFuzzyFindRequestByFile;
   mutable std::mutex CachedCompletionFuzzyFindRequestMutex;
 
   llvm::Optional<std::string> WorkspaceRoot;

clang-tools-extra/clangd/ClangdServer.cpp

ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
                            DiagnosticsConsumer &DiagConsumer,
                            const Options &Opts)
     : FSProvider(FSProvider),
       DynamicIdx(Opts.BuildDynamicSymbolIndex
                      ? new FileIndex(Opts.HeavyweightDynamicSymbolIndex)
                      : nullptr),
       GetClangTidyOptions(Opts.GetClangTidyOptions),
       SuggestMissingIncludes(Opts.SuggestMissingIncludes),
-      TweakFilter(Opts.TweakFilter), WorkspaceRoot(Opts.WorkspaceRoot),
+      CrossFileRename(Opts.CrossFileRename), TweakFilter(Opts.TweakFilter),
+      WorkspaceRoot(Opts.WorkspaceRoot),
       // Pass a callback into `WorkScheduler` to extract symbols from a newly
       // parsed file and rebuild the file index synchronously each time an AST
       // is parsed.
       // FIXME(ioeric): this can be slow and we may be able to index on less
       // critical paths.
       WorkScheduler(
           CDB, Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory,
           std::make_unique<UpdateIndexCallbacks>(DynamicIdx.get(), DiagConsumer,

 
 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;
-    // Performing the rename isn't substantially more expensive than doing an
-    // AST-based check, so we just rename and throw away the results. We may
-    // have to revisit this when we support cross-file rename.
-    auto Changes = renameWithinFile(AST, File, Pos, "dummy", Index);
+    // FIXME: for cross-file rename, we presume prepareRename always succeeds,
+    // revisit this strategy.
+    if (!CrossFileRename) {
+      // Performing the local rename isn't substantially more expensive than
+      // doing an AST-based check, so we just rename and throw away the results.
+      RenameInputs RInputs;
+      RInputs.MainFileCode = InpAST->Inputs.Contents;
+      RInputs.MainFilePath = File;
+      RInputs.NewName = "dummy";
+      RInputs.Pos = Pos;
+      RInputs.AllowCrossFile = false;
+      RInputs.Index = Index;
+      RInputs.VFS = FSProvider.getFileSystem();
+      auto Changes = clangd::rename(AST, RInputs); // within-file rename.
       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).
+        // failure message. If we send an LSP error response, clients can
+        // surface the message to users (VSCode does).
         return CB(Changes.takeError());
       }
+    }
+
     SourceLocation Loc = getBeginningOfIdentifier(
         Pos, AST.getSourceManager(), AST.getASTContext().getLangOpts());
     if (auto Range = getTokenRange(AST.getSourceManager(),
                                    AST.getASTContext().getLangOpts(), Loc))
       return CB(*Range);
     // Return null if the "rename" is not valid at the position.
     CB(llvm::None);
   };
   WorkScheduler.runWithAST("PrepareRename", File, std::move(Action));
 }
 
 void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName,
-                          bool WantFormat, Callback<std::vector<TextEdit>> CB) {
+                          bool WantFormat, DirtyBufferGetter GetDirtyBuffer,
+                          Callback<FileEdits> CB) {
   auto Action = [File = File.str(), NewName = NewName.str(), Pos, WantFormat,
-                 CB = std::move(CB),
+                 GetDirtyBuffer = std::move(GetDirtyBuffer), CB = std::move(CB),
                  this](llvm::Expected<InputsAndAST> InpAST) mutable {
     if (!InpAST)
       return CB(InpAST.takeError());
-    auto Changes = renameWithinFile(InpAST->AST, File, Pos, NewName, Index);
-    if (!Changes)
-      return CB(Changes.takeError());
+    RenameInputs RInputs;
+    RInputs.MainFileCode = InpAST->Inputs.Contents;
+    RInputs.MainFilePath = File;
+    RInputs.NewName = NewName;
+    RInputs.Pos = Pos;
+    RInputs.AllowCrossFile = CrossFileRename;
+    RInputs.Index = Index;
+    RInputs.VFS = FSProvider.getFileSystem();
+    RInputs.GetDirtyBuffer = std::move(GetDirtyBuffer);
+    auto Edits = clangd::rename(InpAST->AST, RInputs);
+    if (!Edits)
+      return CB(Edits.takeError());
 
     if (WantFormat) {
       auto Style = getFormatStyleForFile(File, InpAST->Inputs.Contents,
                                          InpAST->Inputs.FS.get());
-      if (auto Formatted =
-              cleanupAndFormat(InpAST->Inputs.Contents, *Changes, Style))
-        *Changes = std::move(*Formatted);
-      else
-        elog("Failed to format replacements: {0}", Formatted.takeError());
+      for (auto &E : *Edits) {
+        if (auto Err = reformatEdit(E.getValue(), Style))
+          elog("Failed to format replacements: {0}", std::move(Err));
       }
-
-    std::vector<TextEdit> Edits;
-    for (const auto &Rep : *Changes)
-      Edits.push_back(replacementToEdit(InpAST->Inputs.Contents, Rep));
-    return CB(std::move(Edits));
+    }
+    CB(std::move(*Edits));
   };
-
   WorkScheduler.runWithAST("Rename", File, std::move(Action));
 }
 
 static llvm::Expected<Tweak::Selection>
 tweakSelection(const Range &Sel, const InputsAndAST &AST) {
   auto Begin = positionToOffset(AST.Inputs.Contents, Sel.start);
   if (!Begin)
     return Begin.takeError();
   auto End = positionToOffset(AST.Inputs.Contents, Sel.end);
   if (!End)

clang-tools-extra/clangd/SourceCode.h

struct Edit {
   llvm::Expected<std::string> apply() const;
 
   /// Represents Replacements as TextEdits that are available for use in LSP.
   std::vector<TextEdit> asTextEdits() const;
 
   /// Checks whether the Replacements are applicable to given Code.
   bool canApplyTo(llvm::StringRef Code) const;
 };
+using FileEdits = llvm::StringMap<Edit>;
 
 /// Formats the edits and code around it according to Style. Changes
 /// Replacements to formatted ones if succeeds.
 llvm::Error reformatEdit(Edit &E, const format::FormatStyle &Style);
 
 /// Collects identifiers with counts in the source code.
 llvm::StringMap<unsigned> collectIdentifiers(llvm::StringRef Content,
                                              const format::FormatStyle &Style);

clang-tools-extra/clangd/index/SymbolCollector.cpp

if (D->getLocation().isInvalid())
     D = ASTNode.OrigD;
   // If OrigD is an declaration associated with a friend declaration and it's
   // not a definition, skip it. Note that OrigD is the occurrence that the
   // collector is currently visiting.
   if ((ASTNode.OrigD->getFriendObjectKind() !=
        Decl::FriendObjectKind::FOK_None) &&
       !(Roles & static_cast<unsigned>(index::SymbolRole::Definition)))
     return true;
-  // Skip non-semantic references, we should start processing these when we
-  // decide to implement renaming with index support.
-  if ((Roles & static_cast<unsigned>(index::SymbolRole::NameReference)))
-    return true;
   // A declaration created for a friend declaration should not be used as the
   // canonical declaration in the index. Use OrigD instead, unless we've already
   // picked a replacement for D
   if (D->getFriendObjectKind() != Decl::FriendObjectKind::FOK_None)
     D = CanonicalDecls.try_emplace(D, ASTNode.OrigD).first->second;
   const NamedDecl *ND = dyn_cast<NamedDecl>(D);
   if (!ND)
     return true;

clang-tools-extra/clangd/refactor/Rename.h

 //===--- Rename.h - Symbol-rename refactorings -------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_RENAME_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_RENAME_H
 
+#include "Path.h"
 #include "Protocol.h"
+#include "SourceCode.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/Support/Error.h"
 
 namespace clang {
 namespace clangd {
 class ParsedAST;
 class SymbolIndex;
 
+using DirtyBufferGetter =
+    std::function<llvm::Optional<std::string>(PathRef Path)>;
+
+struct RenameInputs {
+  Position Pos; // the position triggering the rename
+  llvm::StringRef NewName;
+
+  llvm::StringRef MainFilePath;
+  llvm::StringRef MainFileCode;
+
+  llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS;
+  const SymbolIndex *Index = nullptr;
+
+  bool AllowCrossFile = false;
+  DirtyBufferGetter GetDirtyBuffer;
+};
+
 /// Renames all occurrences of the symbol at \p Pos to \p NewName.
-/// Occurrences outside the current file are not modified.
-/// Returns an error if rename a symbol that's used in another file (per the
-/// index).
-llvm::Expected<tooling::Replacements>
-renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos,
-                 llvm::StringRef NewName, const SymbolIndex *Index = nullptr);
+/// If AllowCrossFile is false, returns an error if rename a symbol that's used
+/// in another file (per the index).
+llvm::Expected<FileEdits> rename(ParsedAST &AST, const RenameInputs &RInputs);
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_RENAME_H

clang-tools-extra/clangd/refactor/Rename.cpp

llvm::Optional<std::string> getOtherRefFile(const Decl &D, StringRef MainFile,
                                             const SymbolIndex &Index) {
   RefsRequest Req;
   // We limit the number of results, this is a correctness/performance
   // tradeoff. We expect the number of symbol references in the current file
   // is smaller than the limit.
   Req.Limit = 100;
   if (auto ID = getSymbolID(&D))
     Req.IDs.insert(*ID);
   llvm::Optional<std::string> OtherFile;
   Index.refs(Req, [&](const Ref &R) {
     if (OtherFile)
       return;
     if (auto RefFilePath = filePath(R.Location, /*HintFilePath=*/MainFile)) {
       if (*RefFilePath != MainFile)
         OtherFile = *RefFilePath;
     }
   });
   return OtherFile;
 }
 
 enum ReasonToReject {
   NoSymbolFound,
   NoIndexProvided,
   NonIndexable,
-  UsedOutsideFile,
+  UsedOutsideFile, // for within-file rename only.
   UnsupportedSymbol,
 };
 
-// Check the symbol Decl is renameable (per the index) within the file.
-llvm::Optional<ReasonToReject> renamableWithinFile(const Decl &RenameDecl,
-                                                   StringRef MainFile,
-                                                   const SymbolIndex *Index) {
+llvm::Optional<ReasonToReject> renameable(const Decl &RenameDecl,
+                                          StringRef MainFilePath,
+                                          const SymbolIndex *Index,
+                                          bool CrossFile) {
+  // Filter out symbols that are unsupported in both rename mode.
   if (llvm::isa<NamespaceDecl>(&RenameDecl))
     return ReasonToReject::UnsupportedSymbol;
   if (const auto *FD = llvm::dyn_cast<FunctionDecl>(&RenameDecl)) {
     if (FD->isOverloadedOperator())
       return ReasonToReject::UnsupportedSymbol;
   }
+  // function-local symbols is safe to rename.
+  if (RenameDecl.getParentFunctionOrMethod())
+    return None;
+
+  bool IsIndexable =
+      isa<NamedDecl>(RenameDecl) &&
+      SymbolCollector::shouldCollectSymbol(
+          cast<NamedDecl>(RenameDecl), RenameDecl.getASTContext(),
+          SymbolCollector::Options(), CrossFile);
+  if (!IsIndexable) // If the symbol is not indexable, we disallow rename.
+    return ReasonToReject::NonIndexable;
+
+  if (!CrossFile) {
+    // Within-file rename.
     auto &ASTCtx = RenameDecl.getASTContext();
     const auto &SM = ASTCtx.getSourceManager();
     bool MainFileIsHeader = ASTCtx.getLangOpts().IsHeaderFile;
     bool DeclaredInMainFile = isInsideMainFile(RenameDecl.getBeginLoc(), SM);
 
     if (!DeclaredInMainFile)
       // We are sure the symbol is used externally, bail out early.
       return UsedOutsideFile;
 
     // If the symbol is declared in the main file (which is not a header), we
     // rename it.
     if (!MainFileIsHeader)
       return None;
 
-  // Below are cases where the symbol is declared in the header.
-  // If the symbol is function-local, we rename it.
-  if (RenameDecl.getParentFunctionOrMethod())
-    return None;
-
     if (!Index)
-    return ReasonToReject::NoIndexProvided;
+      return NoIndexProvided;
 
-  bool IsIndexable = isa<NamedDecl>(RenameDecl) &&
-                     SymbolCollector::shouldCollectSymbol(
-                         cast<NamedDecl>(RenameDecl), ASTCtx, {}, false);
-  // If the symbol is not indexable, we disallow rename.
-  if (!IsIndexable)
-    return ReasonToReject::NonIndexable;
-  auto OtherFile = getOtherRefFile(RenameDecl, MainFile, *Index);
+    auto OtherFile = getOtherRefFile(RenameDecl, MainFilePath, *Index);
     // If the symbol is indexable and has no refs from other files in the index,
     // we rename it.
     if (!OtherFile)
       return None;
     // If the symbol is indexable and has refs from other files in the index,
     // we disallow rename.
     return ReasonToReject::UsedOutsideFile;
   }
 
+  assert(CrossFile);
+  if (!Index)
+    return NoIndexProvided;
+
+  // FIXME: renaming templates requries to rename all related specializations,
+  // which is not supported in our index.
+  if (RenameDecl.getDescribedTemplate())
+    return ReasonToReject::UnsupportedSymbol;
+
+  // A whitelist of renamable symbols.
+  if (llvm::isa<CXXRecordDecl>(RenameDecl))
+    return None;
+  if (const auto *S = llvm::dyn_cast<CXXMethodDecl>(&RenameDecl)) {
+    // FIXME: renaming virtual methods requires to rename all overridens in
+    // subclasses, which our index doesn't support yet.
+    if (!S->isVirtual())
+      return None;
+  } else if (isa<FunctionDecl>(&RenameDecl)) {
+    return None;
+  }
+  return UnsupportedSymbol;
+}
+
 llvm::Error makeError(ReasonToReject Reason) {
   auto Message = [](ReasonToReject Reason) {
     switch (Reason) {
     case NoSymbolFound:
       return "there is no symbol at the given location";
     case NoIndexProvided:
       return "symbol may be used in other files (no index available)";
     case UsedOutsideFile:
       return "the symbol is used outside main file";
     case NonIndexable:
       return "symbol may be used in other files (not eligible for indexing)";
     case UnsupportedSymbol:
       return "symbol is not a supported kind (e.g. namespace, macro)";
     }
     llvm_unreachable("unhandled reason kind");
   };
   return llvm::make_error<llvm::StringError>(
       llvm::formatv("Cannot rename symbol: {0}", Message(Reason)),
       llvm::inconvertibleErrorCode());
 }
 
-// Return all rename occurrences in the main file.
+// Find all rename occurrences in the main file based on the MainAST.
 tooling::SymbolOccurrences
 findOccurrencesWithinFile(ParsedAST &AST, const NamedDecl *RenameDecl) {
   const NamedDecl *CanonicalRenameDecl =
       tooling::getCanonicalSymbolDeclaration(RenameDecl);
   assert(CanonicalRenameDecl && "RenameDecl must be not null");
   std::vector<std::string> RenameUSRs =
       tooling::getUSRsForDeclaration(CanonicalRenameDecl, AST.getASTContext());
   std::string OldName = CanonicalRenameDecl->getNameAsString();
   tooling::SymbolOccurrences Result;
   for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {
     tooling::SymbolOccurrences RenameInDecl =
         tooling::getOccurrencesOfUSRs(RenameUSRs, OldName, TopLevelDecl);
     Result.insert(Result.end(), std::make_move_iterator(RenameInDecl.begin()),
                   std::make_move_iterator(RenameInDecl.end()));
   }
   return Result;
 }
 
-} // namespace
-
+// AST-based rename, it renames all occurrences in the main file.
 llvm::Expected<tooling::Replacements>
-renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos,
-                 llvm::StringRef NewName, const SymbolIndex *Index) {
-  const SourceManager &SM = AST.getSourceManager();
-  SourceLocation SourceLocationBeg = SM.getMacroArgExpandedLocation(
-      getBeginningOfIdentifier(Pos, SM, AST.getASTContext().getLangOpts()));
-  // FIXME: renaming macros is not supported yet, the macro-handling code should
-  // be moved to rename tooling library.
-  if (locateMacroAt(SourceLocationBeg, AST.getPreprocessor()))
-    return makeError(UnsupportedSymbol);
-
-  const auto *RenameDecl =
-      tooling::getNamedDeclAt(AST.getASTContext(), SourceLocationBeg);
-  if (!RenameDecl)
-    return makeError(NoSymbolFound);
-
-  if (auto Reject =
-          renamableWithinFile(*RenameDecl->getCanonicalDecl(), File, Index))
-    return makeError(*Reject);
-
+renameWithinFile(ParsedAST &AST, const NamedDecl *RenameDecl,
+                 llvm::StringRef NewName) {
   // Rename sometimes returns duplicate edits (which is a bug). A side-effect of
   // adding them to a single Replacements object is these are deduplicated.
   tooling::Replacements FilteredChanges;
   for (const tooling::SymbolOccurrence &Rename :
        findOccurrencesWithinFile(AST, RenameDecl)) {
     // Currently, we only support normal rename (one range) for C/C++.
     // FIXME: support multiple-range rename for objective-c methods.
     if (Rename.getNameRanges().size() > 1)
       continue;
     // We shouldn't have conflicting replacements. If there are conflicts, it
     // means that we have bugs either in clangd or in Rename library, therefore
     // we refuse to perform the rename.
     if (auto Err = FilteredChanges.add(tooling::Replacement(
             AST.getASTContext().getSourceManager(),
             CharSourceRange::getCharRange(Rename.getNameRanges()[0]), NewName)))
       return std::move(Err);
   }
   return FilteredChanges;
 }
 
+// Return all rename occurrences (per the index) outside of the main file.
+llvm::StringMap<std::vector<Range>>
+findOccurrencesOutsideFile(const NamedDecl *RenameDecl,
+                           llvm::StringRef MainFile, const SymbolIndex *Index) {
+  if (!Index)
+    return {};
+  RefsRequest RQuest;
+  // FIXME: revisit the limit, and our index should have a way to inform
+  // us whether the ref results is completed.
+  RQuest.Limit = 100;
+  if (auto ID = getSymbolID(RenameDecl))
+    RQuest.IDs.insert(*ID);
+
+  // Helper.
+  auto ToRange = [](const SymbolLocation &L) {
+    Range R;
+    R.start.line = L.Start.line();
+    R.start.character = L.Start.column();
+    R.end.line = L.End.line();
+    R.end.character = L.End.column();
+    return R;
+  };
+  llvm::StringMap<std::vector<Range>>
+      AffectedFiles; // absolute file path => rename ocurrences in that file.
+  Index->refs(RQuest, [&](const Ref &R) {
+    if (auto RefFilePath = filePath(R.Location, /*HintFilePath=*/MainFile)) {
+      if (*RefFilePath != MainFile)
+        AffectedFiles[*RefFilePath].push_back(ToRange(R.Location));
+    }
+  });
+  return AffectedFiles;
+}
+
+llvm::Expected<std::pair<size_t, size_t>> toRangeOffset(const clangd::Range &R,
+                                                        llvm::StringRef Code) {
+  auto StartOffset = positionToOffset(Code, R.start);
+  if (!StartOffset)
+    return StartOffset.takeError();
+  auto EndOffset = positionToOffset(Code, R.end);
+  if (!EndOffset)
+    return EndOffset.takeError();
+  return std::make_pair<>(*StartOffset, *EndOffset);
+};
+
+llvm::Expected<Edit> buildRenameEdit(llvm::StringRef InitialCode,
+                                     const std::vector<Range> &Occurrences,
+                                     llvm::StringRef NewName) {
+  tooling::Replacements RenameEdit;
+  for (const Range &Occurrence : Occurrences) {
+    // !positionToOffset is O(N), it is okay at the moment since we only
+    // process at most 100 references.
+    auto RangeOffset = toRangeOffset(Occurrence, InitialCode);
+    if (!RangeOffset)
+      return RangeOffset.takeError();
+    auto ByteLength = RangeOffset->second - RangeOffset->first;
+    if (auto Err = RenameEdit.add(tooling::Replacement(
+            InitialCode, RangeOffset->first, ByteLength, NewName)))
+      return std::move(Err);
+  }
+  return Edit(InitialCode, std::move(RenameEdit));
+}
+
+// Index-based rename, it renames all occurrences outside of the main file.
+llvm::Expected<FileEdits>
+renameOutsideFile(const NamedDecl *RenameDecl, llvm::StringRef MainFilePath,
+                  llvm::StringRef NewName, const SymbolIndex *Index,
+                  llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
+                  const DirtyBufferGetter &GetDirtyBuffer) {
+  auto AffectedFiles =
+      findOccurrencesOutsideFile(RenameDecl, MainFilePath, Index);
+  // FIXME: make the limit customizable.
+  static constexpr size_t MaxLimitFiles = 50;
+  if (AffectedFiles.size() >= MaxLimitFiles)
+    return llvm::make_error<llvm::StringError>(
+        llvm::formatv(
+            "The number of affected files exceeds the max limit {0}: {1}",
+            MaxLimitFiles, AffectedFiles.size()),
+        llvm::inconvertibleErrorCode());
+
+  // The cross-file rename is purely based on the index, as we don't want to
+  // build all ASTs for affected files, which may cause a performance hit.
+  // We choose to trade off some correctness for performance and scalability.
+  //
+  // Clangd builds a dynamic index for all opened files on top of the static
+  // index of the whole codebase. Dynamic index is up-to-date (respects dirty
+  // buffers) as long as clangd finishes processing opened files, while static
+  // index (background index) is relatively stale. We choose the dirty buffers
+  // as the file content we rename on, and fallback to file content on disk if
+  // there is no dirty buffer.
+  //
+  // FIXME: add range patching heuristics to detect staleness of the index, and
+  // report to users.
+  // FIXME: our index may return implicit references, which are non-eligitble
+  // for rename, we should filter out these references.
+  FileEdits Results;
+  std::string OldName = RenameDecl->getNameAsString();
+  for (const auto &FileAndOccurrences : AffectedFiles) {
+    llvm::StringRef FilePath = FileAndOccurrences.first();
+
+    std::string AffectedFileCode;
+    llvm::Optional<std::string> DirtyBuffer;
+    if (GetDirtyBuffer && (DirtyBuffer = GetDirtyBuffer(FilePath))) {
+      AffectedFileCode = std::move(*DirtyBuffer);
+    } else {
+      auto Content = VFS->getBufferForFile(FilePath);
+      if (!Content) {
+        elog("Fail to read file {0}: {1}", FilePath,
+             Content.getError().message());
+        continue;
+      }
+      if (!*Content)
+        continue;
+      AffectedFileCode = (*Content)->getBuffer().str();
+    }
+    auto RenameEdit = buildRenameEdit(AffectedFileCode,
+                                      FileAndOccurrences.getValue(), NewName);
+    if (!RenameEdit)
+      return RenameEdit.takeError();
+    if (!RenameEdit->Replacements.empty())
+      Results.insert({FilePath, std::move(*RenameEdit)});
+  }
+  return Results;
+}
+
+} // namespace
+
+llvm::Expected<FileEdits> rename(ParsedAST &AST, const RenameInputs &RInputs) {
+  const SourceManager &SM = AST.getSourceManager();
+  SourceLocation SourceLocationBeg =
+      SM.getMacroArgExpandedLocation(getBeginningOfIdentifier(
+          RInputs.Pos, SM, AST.getASTContext().getLangOpts()));
+  // FIXME: renaming macros is not supported yet, the macro-handling code should
+  // be moved to rename tooling library.
+  if (locateMacroAt(SourceLocationBeg, AST.getPreprocessor()))
+    return makeError(UnsupportedSymbol);
+
+  const NamedDecl *RenameDecl = tooling::getCanonicalSymbolDeclaration(
+      tooling::getNamedDeclAt(AST.getASTContext(), SourceLocationBeg));
+  if (!RenameDecl)
+    return makeError(NoSymbolFound);
+
+  auto Reject =
+      renameable(*RenameDecl->getCanonicalDecl(), RInputs.MainFilePath,
+                 RInputs.Index, RInputs.AllowCrossFile);
+  if (Reject)
+    return makeError(*Reject);
+
+  // We have two implemenations of the rename:
+  //   - AST-based rename: used for renaming local symbols, e.g. variables
+  //     defined in a function body;
+  //   - index-based rename: used for renaming non-local symbols, and not
+  //     feasible for local symbols (as by design our index don't index these
+  //     symbols by design;
+  // To make cross-file rename work for local symbol, we use a hybrid solution:
+  //   - run AST-based rename on the main file;
+  //   - run index-based rename on other affected files;
+  auto MainFileRenameEdit = renameWithinFile(AST, RenameDecl, RInputs.NewName);
+  if (!MainFileRenameEdit)
+    return MainFileRenameEdit.takeError();
+
+  if (!RInputs.AllowCrossFile) {
+    // within-file rename, just return the main file results.
+    return FileEdits({std::make_pair<>(
+        RInputs.MainFilePath,
+        Edit{RInputs.MainFileCode, std::move(*MainFileRenameEdit)})});
+  }
+
+  // Cross-file rename.
+  auto Results =
+      renameOutsideFile(RenameDecl, RInputs.MainFilePath, RInputs.NewName,
+                        RInputs.Index, RInputs.VFS, RInputs.GetDirtyBuffer);
+  if (!Results)
+    return Results.takeError();
+  // Attach the rename edits for the main file.
+  Results->try_emplace(RInputs.MainFilePath, RInputs.MainFileCode,
+                       std::move(*MainFileRenameEdit));
+  return Results;
+}
+
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/refactor/Tweak.h

enum Intent {
     /// Apply changes that preserve the behavior of the code.
     Refactor,
     /// Provide information to the user.
     Info,
   };
   struct Effect {
     /// A message to be displayed to the user.
     llvm::Optional<std::string> ShowMessage;
     /// A mapping from file path(the one used for accessing the underlying VFS)
     /// to edits.
-    llvm::StringMap<Edit> ApplyEdits;
+    FileEdits ApplyEdits;
 
     static Effect showMessage(StringRef S) {
       Effect E;
       E.ShowMessage = S;
       return E;
     }
 
     /// Path is the absolute, symlink-resolved path for the file pointed by FID

clang-tools-extra/clangd/tool/ClangdMain.cpp

 list<std::string> TweakList{
     "tweaks",
     cat(Features),
     desc("Specify a list of Tweaks to enable (only for clangd developers)."),
     Hidden,
     CommaSeparated,
 };
 
+opt<bool> CrossFileRename{
+    "cross-file-rename",
+    cat(Features),
+    desc("Enable cross-file rename feature."),
+    init(false),
+};
+
 opt<unsigned> WorkerThreadsCount{
     "j",
     cat(Misc),
     desc("Number of async workers used by clangd. Background index also "
          "uses this many workers."),
     init(getDefaultAsyncThreadsCount()),
 };
 
AsyncIndexLoad = runAsync<void>([Placeholder] {
       if (auto Idx = loadIndex(IndexFile, /*UseDex=*/true))
         Placeholder->reset(std::move(Idx));
     });
     if (Sync)
       AsyncIndexLoad.wait();
   }
   Opts.StaticIndex = StaticIdx.get();
   Opts.AsyncThreadsCount = WorkerThreadsCount;
+  Opts.CrossFileRename = CrossFileRename;
 
   clangd::CodeCompleteOptions CCOpts;
   CCOpts.IncludeIneligibleResults = IncludeIneligibleResults;
   CCOpts.Limit = LimitResults;
   if (CompletionStyle.getNumOccurrences())
     CCOpts.BundleOverloads = CompletionStyle != Detailed;
   CCOpts.ShowOrigins = ShowOrigins;
   CCOpts.InsertIncludes = HeaderInsertion;

clang-tools-extra/clangd/unittests/RenameTests.cpp

 //===-- RenameTests.cpp -----------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Annotations.h"
 #include "TestFS.h"
 #include "TestTU.h"
+#include "index/Ref.h"
 #include "refactor/Rename.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
+using testing::Eq;
+using testing::Matches;
+MATCHER_P2(PairMatcher, KeyMatcher, ValueMatcher, "") {
+  return Matches(KeyMatcher)(arg.first()) && Matches(ValueMatcher)(arg.second);
+}
 MATCHER_P2(RenameRange, Code, Range, "") {
   return replacementToEdit(Code, arg).range == Range;
 }
+MATCHER_P2(EqualFileEdit, Code, Ranges, "") {
+  using ReplacementMatcher = testing::Matcher<tooling::Replacement>;
+  std::vector<ReplacementMatcher> Expected;
+  for (const auto &R : Ranges)
+    Expected.push_back(RenameRange(Code, R));
+  auto Matcher =
+      testing::internal::UnorderedElementsAreArrayMatcher<ReplacementMatcher>(
+          Expected.begin(), Expected.end());
+  return arg.InitialCode == Code && Matches(Matcher)(arg.Replacements);
+}
+
+std::unique_ptr<RefSlab> buildRefSlab(const Annotations &Code,
+                                      llvm::StringRef SymbolName,
+                                      llvm::StringRef Path) {
+  RefSlab::Builder Builder;
+  TestTU TU;
+  TU.HeaderCode = Code.code();
+  auto Symbols = TU.headerSymbols();
+  const auto &SymbolID = findSymbol(Symbols, SymbolName).ID;
+  for (const auto &Range : Code.ranges()) {
+    Ref R;
+    R.Kind = RefKind::Reference;
+    R.Location.Start.setLine(Range.start.line);
+    R.Location.Start.setColumn(Range.start.character);
+    R.Location.End.setLine(Range.end.line);
+    R.Location.End.setColumn(Range.end.character);
+    auto U = URI::create(Path).toString();
+    R.Location.FileURI = U.c_str();
+    Builder.insert(SymbolID, R);
+  }
+
+  return std::make_unique<RefSlab>(std::move(Builder).build());
+}
+
+RenameInputs renameInputs(const Annotations &Code, llvm::StringRef NewName,
+                          llvm::StringRef Path,
+                          const SymbolIndex *Index = nullptr,
+                          bool CrossFile = false) {
+  RenameInputs Inputs;
+  Inputs.Pos = Code.point();
+  Inputs.MainFileCode = Code.code();
+  Inputs.MainFilePath = Path;
+  Inputs.NewName = NewName;
+  Inputs.Index = Index;
+  Inputs.AllowCrossFile = CrossFile;
+  return Inputs;
+}
 
 TEST(RenameTest, SingleFile) {
   struct Test {
     const char* Before;
     const char* After;
   } Tests[] = {
       // Rename function.
       {
const char* After;
           )cpp",
       },
   };
   for (const Test &T : Tests) {
     Annotations Code(T.Before);
     auto TU = TestTU::withCode(Code.code());
     auto AST = TU.build();
     auto RenameResult =
-        renameWithinFile(AST, testPath(TU.Filename), Code.point(), "abcde");
+        rename(AST, renameInputs(Code, "abcde", testPath(TU.Filename)));
+
     ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
-    auto ApplyResult =
-        tooling::applyAllReplacements(Code.code(), *RenameResult);
+    ASSERT_TRUE(RenameResult->size() == 1);
+    EXPECT_EQ(Code.code(), RenameResult->begin()->second.InitialCode);
+    auto ApplyResult = tooling::applyAllReplacements(
+        Code.code(), RenameResult->begin()->second.Replacements);
     ASSERT_TRUE(bool(ApplyResult)) << ApplyResult.takeError();
 
     EXPECT_EQ(T.After, *ApplyResult) << T.Before;
   }
 }
 
 TEST(RenameTest, Renameable) {
   struct Case {
     const char *Code;
     const char* ErrorMessage; // null if no error
     bool IsHeaderFile;
for (const auto& Case : Cases) {
     if (Case.IsHeaderFile) {
       // We open the .h file as the main file.
       TU.Filename = "test.h";
       // Parsing the .h file as C++ include.
       TU.ExtraArgs.push_back("-xobjective-c++-header");
     }
     auto AST = TU.build();
 
-    auto Results = renameWithinFile(AST, testPath(TU.Filename), T.point(),
-                                    "dummyNewName", Case.Index);
+    auto Results = rename(AST, renameInputs(T, "dummyNewName",
+                                            testPath(TU.Filename), Case.Index));
     bool WantRename = true;
     if (T.ranges().empty())
       WantRename = false;
     if (!WantRename) {
       assert(Case.ErrorMessage && "Error message must be set!");
-      EXPECT_FALSE(Results) << "expected renameWithinFile returned an error: "
-                            << T.code();
+      EXPECT_FALSE(Results)
+          << "expected rename returned an error: " << T.code();
       auto ActualMessage = llvm::toString(Results.takeError());
       EXPECT_THAT(ActualMessage, testing::HasSubstr(Case.ErrorMessage));
     } else {
-      EXPECT_TRUE(bool(Results)) << "renameWithinFile returned an error: "
+      EXPECT_TRUE(bool(Results)) << "rename returned an error: "
                                  << llvm::toString(Results.takeError());
-      std::vector<testing::Matcher<tooling::Replacement>> Expected;
-      for (const auto &R : T.ranges())
-        Expected.push_back(RenameRange(TU.Code, R));
-      EXPECT_THAT(*Results, UnorderedElementsAreArray(Expected));
+      EXPECT_THAT(*Results,
+                  testing::UnorderedElementsAre(PairMatcher(
+                      testing::_, EqualFileEdit(T.code(), T.ranges()))));
     }
   }
 }
 
+TEST(RenameTests, CrossFile) {
+  Annotations FooCode("class [[Foo]] {};");
+  std::string FooPath = testPath("foo.cc");
+  std::string FooDirtyBuffer =
+      (FooCode.code() + "\n// this is dirty buffer").str();
+  Annotations BarCode("void [[Bar]]() {}");
+  std::string BarPath = testPath("bar.cc");
+  // Build the index, the index has "Foo" references in foo.cc and "Bar"
+  // references from bar.cc.
+  FileSymbols FSymbols;
+  FSymbols.update(FooPath, nullptr, buildRefSlab(FooCode, "Foo", FooPath),
+                  nullptr, false);
+  FSymbols.update(BarPath, nullptr, buildRefSlab(BarCode, "Bar", BarPath),
+                  nullptr, false);
+  auto Index = FSymbols.buildIndex(IndexType::Light);
+
+  // Prepare the rename environment:
+  //   - dirty buffer for foo.cc;
+  //   - file on VFS for bar.cc;
+  Annotations MainCode("class  [[Fo^o]] {};");
+  auto MainFilePath = testPath("main.cc");
+  RenameInputs Input =
+      renameInputs(MainCode, "newName", MainFilePath, Index.get(),
+                   /*CrossFile*/ true);
+  Input.GetDirtyBuffer = [&](PathRef Path) -> llvm::Optional<std::string> {
+    if (Path == FooPath)
+      return FooDirtyBuffer;
+    return llvm::None;
+  };
+  llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> MemFS(
+      new llvm::vfs::InMemoryFileSystem);
+  MemFS->addFile(testPath("bar.cc"), 0,
+                 llvm::MemoryBuffer::getMemBuffer(BarCode.code()));
+  Input.VFS = MemFS;
+
+  // Run rename on Foo, there is a dirty buffer for foo.cc, rename should
+  // respect the dirty buffer.
+  TestTU TU = TestTU::withCode(MainCode.code());
+  auto AST = TU.build();
+  auto Results = rename(AST, Input);
+  ASSERT_TRUE(bool(Results)) << Results.takeError();
+  EXPECT_THAT(
+      *Results,
+      testing::UnorderedElementsAre(
+          PairMatcher(Eq(FooPath),
+                      EqualFileEdit(FooDirtyBuffer, FooCode.ranges())),
+          PairMatcher(Eq(MainFilePath),
+                      EqualFileEdit(MainCode.code(), MainCode.ranges()))));
+
+  // Run rename on Bar, there is no dirty buffer for the affected file bar.cc,
+  // so we should read file content from VFS.
+  MainCode = Annotations("void [[Bar]]() { [[B^ar]](); }");
+  Input.MainFileCode = MainCode.code();
+  Input.Pos = MainCode.point();
+  TU = TestTU::withCode(MainCode.code());
+  AST = TU.build();
+  Results = rename(AST, Input);
+  ASSERT_TRUE(bool(Results)) << Results.takeError();
+  EXPECT_THAT(
+      *Results,
+      testing::UnorderedElementsAre(
+          PairMatcher(Eq(BarPath),
+                      EqualFileEdit(BarCode.code(), BarCode.ranges())),
+          PairMatcher(Eq(MainFilePath),
+                      EqualFileEdit(MainCode.code(), MainCode.ranges()))));
+}
+
+TEST(CrossFileRenameTests, CrossFileOnLocalSymbol) {
+  // cross-file rename should work for function-local symbols, even there is no
+  // index provided.
+  Annotations Code("void f(int [[abc]]) { [[a^bc]] = 3; }");
+  auto TU = TestTU::withCode(Code.code());
+  auto Path = testPath(TU.Filename);
+  auto AST = TU.build();
+  auto Results = rename(AST, renameInputs(Code, "abcde", Path));
+  ASSERT_TRUE(bool(Results)) << Results.takeError();
+  EXPECT_THAT(*Results,
+              testing::UnorderedElementsAre(PairMatcher(
+                  Eq(Path), EqualFileEdit(Code.code(), Code.ranges()))));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/SyncAPI.h

llvm::Expected<SignatureHelp> runSignatureHelp(ClangdServer &Server,
                                                PathRef File, Position Pos);
 
 llvm::Expected<std::vector<LocatedSymbol>>
 runLocateSymbolAt(ClangdServer &Server, PathRef File, Position Pos);
 
 llvm::Expected<std::vector<DocumentHighlight>>
 runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos);
 
-llvm::Expected<std::vector<TextEdit>>
-runRename(ClangdServer &Server, PathRef File, Position Pos, StringRef NewName);
+llvm::Expected<FileEdits> runRename(ClangdServer &Server, PathRef File,
+                                    Position Pos, StringRef NewName);
 
 std::string runDumpAST(ClangdServer &Server, PathRef File);
 
 llvm::Expected<std::vector<SymbolInformation>>
 runWorkspaceSymbols(ClangdServer &Server, StringRef Query, int Limit);
 
 Expected<std::vector<DocumentSymbol>> runDocumentSymbols(ClangdServer &Server,
                                                          PathRef File);

clang-tools-extra/clangd/unittests/SyncAPI.cpp

 
 llvm::Expected<std::vector<DocumentHighlight>>
 runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos) {
   llvm::Optional<llvm::Expected<std::vector<DocumentHighlight>>> Result;
   Server.findDocumentHighlights(File, Pos, capture(Result));
   return std::move(*Result);
 }
 
-llvm::Expected<std::vector<TextEdit>> runRename(ClangdServer &Server,
-                                                PathRef File, Position Pos,
-                                                llvm::StringRef NewName) {
-  llvm::Optional<llvm::Expected<std::vector<TextEdit>>> Result;
-  Server.rename(File, Pos, NewName, /*WantFormat=*/true, capture(Result));
+llvm::Expected<FileEdits> runRename(ClangdServer &Server, PathRef File,
+                                    Position Pos, llvm::StringRef NewName) {
+  llvm::Optional<llvm::Expected<FileEdits>> Result;
+  Server.rename(File, Pos, NewName, /*WantFormat=*/true,
+                /*DirtyBuffaer*/ nullptr, capture(Result));
   return std::move(*Result);
 }
 
 std::string runDumpAST(ClangdServer &Server, PathRef File) {
   llvm::Optional<std::string> Result;
   Server.dumpAST(File, capture(Result));
   return std::move(*Result);
 }