[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 ↗

(On Diff #225889)

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

182

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?
}
``

184

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

WDYT?

215

NIT: else is redundant

363

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.

426

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

270

s/fature/feature

271

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 ↗

(On Diff #225889)

Agreed. split it out.

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

182

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);

184

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.

363

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.

426

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

182

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

184

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.

363

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?

403

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?

426

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

184

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

363

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

403

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.

426

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

270

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

562

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?

587

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

270

Done. yes, we should make it hidden.

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

562

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?

587

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

281

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

285

Maybe remove this comment?

286

Why not extract this as a helper function?

295

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?

336

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

But up to you

351

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

562

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

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

106

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

281

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.

336

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

282

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

336

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–335

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...

349–359

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.

356

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

364

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

114

NIT: in both rename modes

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

33

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

40

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

49

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–335

Good point.

356

yes, obtained from the AST.

364

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

33

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

40

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

357

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

364

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

134

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

150

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

169

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?

173

Isn't the same true for local rename?

287

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?

301

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?

321

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

33

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

40

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

364

thanks.

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

150

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?

169

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.

173

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

287

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

301

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.

321

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

40

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

150

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?

169

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.

173

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)

301

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.

321

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

40

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

150

ah, I see. Done.

169

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

173

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

40

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

317

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

320

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

341–343

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

343

NIT: s/SnapShot/Snapshot

372

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

clang-tools-extra/clangd/TUScheduler.h

186

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

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

61

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))

175

nit: add braces to the outer if

311

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

364

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

412

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

446

NIT: remove <>

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

79–80

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

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

27

Could you document what this function is doing?

52

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

403–410

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

[hokein]

address comments.

[hokein]

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

412

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

412

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

412

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,
+      [File, 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 "Hover.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, 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);
+    const auto &SM = AST.getSourceManager();
+    SourceLocation Loc =
+        SM.getMacroArgExpandedLocation(getBeginningOfIdentifier(
+            Pos, AST.getSourceManager(), AST.getASTContext().getLangOpts()));
+    auto Range = getTokenRange(SM, AST.getASTContext().getLangOpts(), Loc);
+    if (!Range)
+      return CB(llvm::None); // "rename" is not valid at the position.
+
+    if (CrossFileRename)
+      // FIXME: we now assume cross-file rename always succeeds, revisit this.
+      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());
     }
-    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, Callback<FileEdits> CB) {
+  // A snapshot of all file dirty buffers.
+  llvm::StringMap<std::string> Snapshot = WorkScheduler.getAllFileContents();
   auto Action = [File = File.str(), NewName = NewName.str(), Pos, WantFormat,
-                 CB = std::move(CB),
+                 CB = std::move(CB), Snapshot = std::move(Snapshot),
                  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());
+    auto GetDirtyBuffer =
+        [&Snapshot](PathRef AbsPath) -> llvm::Optional<std::string> {
+      auto It = Snapshot.find(AbsPath);
+      if (It == Snapshot.end())
+        return llvm::None;
+      return It->second;
+    };
+    auto Edits = clangd::rename({Pos, NewName, InpAST->AST, File, Index,
+                                 CrossFileRename, GetDirtyBuffer});
+    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());
-    }
+      llvm::Error Err = llvm::Error::success();
+      for (auto &E : *Edits)
+        Err =
+            llvm::joinErrors(reformatEdit(E.getValue(), Style), 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));
+      if (Err)
+        return CB(std::move(Err));
+    }
+    return 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();

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;
 };
+/// A mapping from absolute file path (the one used for accessing the underlying
+/// VFS) to edits.
+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/TUScheduler.h

public:
   /// resources. Pending diagnostics for closed files may not be delivered, even
   /// if requested with WantDiags::Auto or WantDiags::Yes.
   void remove(PathRef File);
 
   /// Returns the current contents of the buffer for File, per last update().
   /// The returned StringRef may be invalidated by any write to TUScheduler.
   llvm::StringRef getContents(PathRef File) const;
 
+  /// Returns a snapshot of all file buffer contents, per last update().
+  llvm::StringMap<std::string> getAllFileContents() const;
+
   /// Schedule an async task with no dependencies.
   void run(llvm::StringRef Name, llvm::unique_function<void()> Action);
 
   /// Schedule an async read of the AST. \p Action will be called when AST is
   /// ready. The AST passed to \p Action refers to the version of \p File
   /// tracked at the time of the call, even if new updates are received before
   /// \p Action is executed.
   /// If an error occurs during processing, it is forwarded to the \p Action
   /// callback.

clang-tools-extra/clangd/TUScheduler.cpp

llvm::StringRef TUScheduler::getContents(PathRef File) const {
   auto It = Files.find(File);
   if (It == Files.end()) {
     elog("getContents() for untracked file: {0}", File);
     return "";
   }
   return It->second->Contents;
 }
 
+llvm::StringMap<std::string> TUScheduler::getAllFileContents() const {
+  llvm::StringMap<std::string> Results;
+  for (auto &It : Files)
+    Results.try_emplace(It.getKey(), It.getValue()->Contents);
+  return Results;
+}
+
 void TUScheduler::run(llvm::StringRef Name,
                       llvm::unique_function<void()> Action) {
   if (!PreambleTasks)
     return Action();
   PreambleTasks->runAsync(Name, [Ctx = Context::current().clone(),
                                  Action = std::move(Action)]() mutable {
     WithContext WC(std::move(Ctx));
     Action();

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;
 
-/// 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);
+/// Gets dirty buffer for a given file \p AbsPath.
+/// Returns None if there is no dirty buffer for the given file.
+using DirtyBufferGetter =
+    llvm::function_ref<llvm::Optional<std::string>(PathRef AbsPath)>;
+
+struct RenameInputs {
+  Position Pos; // the position triggering the rename
+  llvm::StringRef NewName;
+
+  ParsedAST &AST;
+  llvm::StringRef MainFilePath;
+
+  const SymbolIndex *Index = nullptr;
+
+  bool AllowCrossFile = false;
+  // When set, used by the rename to get file content for all rename-related
+  // files.
+  // If there is no corresponding dirty buffer, we will use the file content
+  // from disk.
+  DirtyBufferGetter GetDirtyBuffer = nullptr;
+};
+
+/// Renames all occurrences of the symbol.
+/// If AllowCrossFile is false, returns an error if rename a symbol that's used
+/// in another file (per the index).
+llvm::Expected<FileEdits> rename(const RenameInputs &RInputs);
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_RENAME_H

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

 #include "ParsedAST.h"
 #include "Selection.h"
 #include "SourceCode.h"
 #include "index/SymbolCollector.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/Error.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
 llvm::Optional<std::string> filePath(const SymbolLocation &Loc,
                                      llvm::StringRef HintFilePath) {
   if (!Loc)

 // Query the index to find some other files where the Decl is referenced.
 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);
+  Req.IDs.insert(*getSymbolID(&D));
   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;
     }
   });
for (const auto *D :
     Result.insert(D);
   return Result;
 }
 
 enum ReasonToReject {
   NoSymbolFound,
   NoIndexProvided,
   NonIndexable,
-  UsedOutsideFile,
+  UsedOutsideFile, // for within-file rename only.
   UnsupportedSymbol,
   AmbiguousSymbol,
 };
 
-// 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 modes.
   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) {
     auto &ASTCtx = RenameDecl.getASTContext();
     const auto &SM = ASTCtx.getSourceManager();
-  bool MainFileIsHeader = isHeaderFile(MainFile, ASTCtx.getLangOpts());
+    bool MainFileIsHeader = isHeaderFile(MainFilePath, ASTCtx.getLangOpts());
     bool DeclaredInMainFile = isInsideMainFile(RenameDecl.getBeginLoc(), SM);
 
     if (!DeclaredInMainFile)
       // We are sure the symbol is used externally, bail out early.
-    return UsedOutsideFile;
+      return ReasonToReject::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;
 
-  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 ReasonToReject::NoIndexProvided;
+
+  // Blacklist symbols that are not supported yet in cross-file mode due to the
+  // limitations of our index.
+  // FIXME: renaming templates requries to rename all related specializations,
+  //        our index doesn't have this information.
+  if (RenameDecl.getDescribedTemplate())
+    return ReasonToReject::UnsupportedSymbol;
+
+  // FIXME: renaming virtual methods requires to rename all overridens in
+  //        subclasses, our index doesn't have this information.
+  // Note: within-file rename does support this through the AST.
+  if (const auto *S = llvm::dyn_cast<CXXMethodDecl>(&RenameDecl)) {
+    if (S->isVirtual())
+      return ReasonToReject::UnsupportedSymbol;
+  }
+  return None;
+}
+
 llvm::Error makeError(ReasonToReject Reason) {
   auto Message = [](ReasonToReject Reason) {
     switch (Reason) {
-    case NoSymbolFound:
+    case ReasonToReject::NoSymbolFound:
       return "there is no symbol at the given location";
-    case NoIndexProvided:
+    case ReasonToReject::NoIndexProvided:
       return "symbol may be used in other files (no index available)";
-    case UsedOutsideFile:
+    case ReasonToReject::UsedOutsideFile:
       return "the symbol is used outside main file";
-    case NonIndexable:
+    case ReasonToReject::NonIndexable:
       return "symbol may be used in other files (not eligible for indexing)";
-    case UnsupportedSymbol:
+    case ReasonToReject::UnsupportedSymbol:
       return "symbol is not a supported kind (e.g. namespace, macro)";
     case AmbiguousSymbol:
       return "there are multiple symbols at the given location";
     }
     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.
 std::vector<SourceLocation> findOccurrencesWithinFile(ParsedAST &AST,
                                                       const NamedDecl &ND) {
   // In theory, locateDeclAt should return the primary template. However, if the
   // cursor is under the underlying CXXRecordDecl of the ClassTemplateDecl, ND
   // will be the CXXRecordDecl, for this case, we need to get the primary
   // template maunally.
   const auto &RenameDecl =
       ND.getDescribedTemplate() ? *ND.getDescribedTemplate() : ND;
   // getUSRsForDeclaration will find other related symbols, e.g. virtual and its
   // overriddens, primary template and all explicit specializations.
   // FIXME: get rid of the remaining tooling APIs.
   std::vector<std::string> RenameUSRs = tooling::getUSRsForDeclaration(
       tooling::getCanonicalSymbolDeclaration(&RenameDecl), AST.getASTContext());
   llvm::DenseSet<SymbolID> TargetIDs;
   for (auto &USR : RenameUSRs)
     TargetIDs.insert(SymbolID(USR));
 
   std::vector<SourceLocation> Results;
   for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {
     findExplicitReferences(TopLevelDecl, [&](ReferenceLoc Ref) {
       if (Ref.Targets.empty())
         return;
       for (const auto *Target : Ref.Targets) {
         auto ID = getSymbolID(Target);
         if (!ID || TargetIDs.find(*ID) == TargetIDs.end())
           return;
       }
       Results.push_back(Ref.NameLoc);
     });
   }
 
   return Results;
 }
 
-} // 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) {
+renameWithinFile(ParsedAST &AST, const NamedDecl &RenameDecl,
+                 llvm::StringRef NewName) {
   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);
-
-  auto DeclsUnderCursor = locateDeclAt(AST, SourceLocationBeg);
-  if (DeclsUnderCursor.empty())
-    return makeError(NoSymbolFound);
-  if (DeclsUnderCursor.size() > 1)
-    return makeError(AmbiguousSymbol);
-
-  const auto *RenameDecl = llvm::dyn_cast<NamedDecl>(*DeclsUnderCursor.begin());
-  if (!RenameDecl)
-    return makeError(UnsupportedSymbol);
-
-  if (auto Reject =
-          renamableWithinFile(*RenameDecl->getCanonicalDecl(), File, Index))
-    return makeError(*Reject);
 
   tooling::Replacements FilteredChanges;
-  for (SourceLocation Loc : findOccurrencesWithinFile(AST, *RenameDecl)) {
+  for (SourceLocation Loc : findOccurrencesWithinFile(AST, RenameDecl)) {
     SourceLocation RenameLoc = Loc;
     // We don't rename in any macro bodies, but we allow rename the symbol
     // spelled in a top-level macro argument in the main file.
     if (RenameLoc.isMacroID()) {
       if (isInMacroBody(SM, RenameLoc))
         continue;
       RenameLoc = SM.getSpellingLoc(Loc);
     }
     // Filter out locations not from main file.
     // We traverse only main file decls, but locations could come from an
     // non-preamble #include file e.g.
     //   void test() {
     //     int f^oo;
     //     #include "use_foo.inc"
     //   }
     if (!isInsideMainFile(RenameLoc, SM))
       continue;
     if (auto Err = FilteredChanges.add(tooling::Replacement(
             SM, CharSourceRange::getTokenRange(RenameLoc), NewName)))
       return std::move(Err);
   }
   return FilteredChanges;
 }
 
+Range 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;
+};
+
+// Return all rename occurrences (per the index) outside of the main file,
+// grouped by the absolute file path.
+llvm::StringMap<std::vector<Range>>
+findOccurrencesOutsideFile(const NamedDecl &RenameDecl,
+                           llvm::StringRef MainFile, const SymbolIndex &Index) {
+  RefsRequest RQuest;
+  RQuest.IDs.insert(*getSymbolID(&RenameDecl));
+
+  // Absolute file path => rename ocurrences in that file.
+  llvm::StringMap<std::vector<Range>> AffectedFiles;
+  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) {
+    // FIXME: !positionToOffset is O(N), optimize it.
+    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.
+//
+// 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.
+llvm::Expected<FileEdits> renameOutsideFile(
+    const NamedDecl &RenameDecl, llvm::StringRef MainFilePath,
+    llvm::StringRef NewName, const SymbolIndex &Index,
+    llvm::function_ref<llvm::Expected<std::string>(PathRef)> GetFileContent) {
+  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());
+
+  FileEdits Results;
+  for (const auto &FileAndOccurrences : AffectedFiles) {
+    llvm::StringRef FilePath = FileAndOccurrences.first();
+
+    auto AffectedFileCode = GetFileContent(FilePath);
+    if (!AffectedFileCode) {
+      elog("Fail to read file content: {0}", AffectedFileCode.takeError());
+      continue;
+    }
+
+    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(const RenameInputs &RInputs) {
+  ParsedAST &AST = RInputs.AST;
+  const SourceManager &SM = AST.getSourceManager();
+  llvm::StringRef MainFileCode = SM.getBufferData(SM.getMainFileID());
+  auto GetFileContent = [&RInputs,
+                         &SM](PathRef AbsPath) -> llvm::Expected<std::string> {
+    llvm::Optional<std::string> DirtyBuffer;
+    if (RInputs.GetDirtyBuffer &&
+        (DirtyBuffer = RInputs.GetDirtyBuffer(AbsPath)))
+      return std::move(*DirtyBuffer);
+
+    auto Content =
+        SM.getFileManager().getVirtualFileSystem().getBufferForFile(AbsPath);
+    if (!Content)
+      return llvm::createStringError(
+          llvm::inconvertibleErrorCode(),
+          llvm::formatv("Fail to open file {0}: {1}", AbsPath,
+                        Content.getError().message()));
+    if (!*Content)
+      return llvm::createStringError(
+          llvm::inconvertibleErrorCode(),
+          llvm::formatv("Got no buffer for file {0}", AbsPath));
+
+    return (*Content)->getBuffer().str();
+  };
+  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(ReasonToReject::UnsupportedSymbol);
+
+  auto DeclsUnderCursor = locateDeclAt(AST, SourceLocationBeg);
+  if (DeclsUnderCursor.empty())
+    return makeError(ReasonToReject::NoSymbolFound);
+  if (DeclsUnderCursor.size() > 1)
+    return makeError(ReasonToReject::AmbiguousSymbol);
+
+  const auto *RenameDecl = llvm::dyn_cast<NamedDecl>(*DeclsUnderCursor.begin());
+  if (!RenameDecl)
+    return makeError(ReasonToReject::UnsupportedSymbol);
+
+  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{MainFileCode, std::move(*MainFileRenameEdit)})});
+  }
+
+  FileEdits Results;
+  // renameable safely guards us that at this point we are renaming a local
+  // symbol if we don't have index,
+  if (RInputs.Index) {
+    auto OtherFilesEdits =
+        renameOutsideFile(*RenameDecl, RInputs.MainFilePath, RInputs.NewName,
+                          *RInputs.Index, GetFileContent);
+    if (!OtherFilesEdits)
+      return OtherFilesEdits.takeError();
+    Results = std::move(*OtherFilesEdits);
+  }
+  // Attach the rename edits for the main file.
+  Results.try_emplace(RInputs.MainFilePath, MainFileCode,
+                      std::move(*MainFileRenameEdit));
+  return Results;
+}
+
 } // namespace clangd
 } // namespace clang

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

public:
   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. Note that this feature is "
+         "experimental and may lead to broken code or incomplete rename "
+         "results"),
+    init(false),
+    Hidden,
+};
+
 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 {
 
-MATCHER_P2(RenameRange, Code, Range, "") {
-  return replacementToEdit(Code, arg).range == Range;
+using testing::Eq;
+using testing::Pair;
+using testing::UnorderedElementsAre;
+
+// Build a RefSlab from all marked ranges in the annotation. The ranges are
+// assumed to associate with the given SymbolName.
+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());
+}
+
+std::vector<
+    std::pair</*InitialCode*/ std::string, /*CodeAfterRename*/ std::string>>
+applyEdits(FileEdits FE) {
+  std::vector<std::pair<std::string, std::string>> Results;
+  for (auto &It : FE)
+    Results.emplace_back(
+        It.first().str(),
+        llvm::cantFail(tooling::applyAllReplacements(
+            It.getValue().InitialCode, It.getValue().Replacements)));
+  return Results;
 }
 
 // Generates an expected rename result by replacing all ranges in the given
 // annotation with the NewName.
 std::string expectedResult(Annotations Test, llvm::StringRef NewName) {
   std::string Result;
   unsigned NextChar = 0;
   llvm::StringRef Code = Test.code();
llvm::StringRef Tests[] = {
       )cpp",
   };
   for (const auto T : Tests) {
     Annotations Code(T);
     auto TU = TestTU::withCode(Code.code());
     TU.ExtraArgs.push_back("-fno-delayed-template-parsing");
     auto AST = TU.build();
     llvm::StringRef NewName = "abcde";
     for (const auto &RenamePos : Code.points()) {
       auto RenameResult =
-          renameWithinFile(AST, testPath(TU.Filename), RenamePos, NewName);
-      ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError() << T;
-      auto ApplyResult = llvm::cantFail(
-          tooling::applyAllReplacements(Code.code(), *RenameResult));
-      EXPECT_EQ(expectedResult(Code, NewName), ApplyResult);
+          rename({RenamePos, NewName, AST, testPath(TU.Filename)});
+      ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
+      ASSERT_EQ(1u, RenameResult->size());
+      EXPECT_EQ(applyEdits(std::move(*RenameResult)).front().second,
+                expectedResult(Code, NewName));
     }
   }
 }
 
 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();
     llvm::StringRef NewName = "dummyNewName";
-    auto Results = renameWithinFile(AST, testPath(TU.Filename), T.point(),
-                                    NewName, Case.Index);
+    auto Results =
+        rename({T.point(), NewName, AST, 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();
+          << "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());
-      auto ApplyResult =
-          llvm::cantFail(tooling::applyAllReplacements(T.code(), *Results));
-      EXPECT_EQ(expectedResult(T, NewName), ApplyResult);
+      ASSERT_EQ(1u, Results->size());
+      EXPECT_EQ(applyEdits(std::move(*Results)).front().second,
+                expectedResult(T, NewName));
     }
   }
 }
 
 TEST(RenameTest, MainFileReferencesOnly) {
   // filter out references not from main file.
   llvm::StringRef Test =
       R"cpp(
         void test() {
           int [[fo^o]] = 1;
           // rename references not from main file are not included.
           #include "foo.inc"
         })cpp";
 
   Annotations Code(Test);
   auto TU = TestTU::withCode(Code.code());
   TU.AdditionalFiles["foo.inc"] = R"cpp(
       #define Macro(X) X
       &Macro(foo);
       &foo;
     )cpp";
   auto AST = TU.build();
   llvm::StringRef NewName = "abcde";
 
   auto RenameResult =
-      renameWithinFile(AST, testPath(TU.Filename), Code.point(), NewName);
+      rename({Code.point(), NewName, AST, testPath(TU.Filename)});
   ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError() << Code.point();
-  auto ApplyResult =
-      llvm::cantFail(tooling::applyAllReplacements(Code.code(), *RenameResult));
-  EXPECT_EQ(expectedResult(Code, NewName), ApplyResult);
+  ASSERT_EQ(1u, RenameResult->size());
+  EXPECT_EQ(applyEdits(std::move(*RenameResult)).front().second,
+            expectedResult(Code, NewName));
+}
+
+TEST(RenameTests, CrossFile) {
+  Annotations FooCode("class [[Foo]] {};");
+  std::string FooPath = testPath("foo.cc");
+  Annotations FooDirtyBuffer("class [[Foo]] {};\n// this is dirty buffer");
+  Annotations BarCode("void [[Bar]]() {}");
+  std::string BarPath = testPath("bar.cc");
+  // Build the index, the index has "Foo" references from 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);
+
+  Annotations MainCode("class  [[Fo^o]] {};");
+  auto MainFilePath = testPath("main.cc");
+  // Dirty buffer for foo.cc.
+  auto GetDirtyBuffer = [&](PathRef Path) -> llvm::Optional<std::string> {
+    if (Path == FooPath)
+      return FooDirtyBuffer.code().str();
+    return llvm::None;
+  };
+
+  // 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();
+  llvm::StringRef NewName = "newName";
+  auto Results = rename({MainCode.point(), NewName, AST, MainFilePath,
+                         Index.get(), /*CrossFile=*/true, GetDirtyBuffer});
+  ASSERT_TRUE(bool(Results)) << Results.takeError();
+  EXPECT_THAT(
+      applyEdits(std::move(*Results)),
+      UnorderedElementsAre(
+          Pair(Eq(FooPath), Eq(expectedResult(FooDirtyBuffer, NewName))),
+          Pair(Eq(MainFilePath), Eq(expectedResult(MainCode, NewName)))));
+
+  // 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]](); }");
+  TU = TestTU::withCode(MainCode.code());
+  // Set a file "bar.cc" on disk.
+  TU.AdditionalFiles["bar.cc"] = BarCode.code();
+  AST = TU.build();
+  Results = rename({MainCode.point(), NewName, AST, MainFilePath, Index.get(),
+                    /*CrossFile=*/true, GetDirtyBuffer});
+  ASSERT_TRUE(bool(Results)) << Results.takeError();
+  EXPECT_THAT(
+      applyEdits(std::move(*Results)),
+      UnorderedElementsAre(
+          Pair(Eq(BarPath), Eq(expectedResult(BarCode, NewName))),
+          Pair(Eq(MainFilePath), Eq(expectedResult(MainCode, NewName)))));
+}
+
+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();
+  llvm::StringRef NewName = "newName";
+  auto Results = rename({Code.point(), NewName, AST, Path});
+  ASSERT_TRUE(bool(Results)) << Results.takeError();
+  EXPECT_THAT(
+      applyEdits(std::move(*Results)),
+      UnorderedElementsAre(Pair(Eq(Path), Eq(expectedResult(Code, NewName)))));
 }
 
 } // 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;
+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, 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);
 }
 
 llvm::Expected<std::vector<SymbolInformation>>
 runWorkspaceSymbols(ClangdServer &Server, llvm::StringRef Query, int Limit) {
   llvm::Optional<llvm::Expected<std::vector<SymbolInformation>>> Result;