[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:107
+    if (!Index)
+      return NoIndexProvided;
+
----------------
ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > Why isn't this a scope enum in the first place?
> > 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?
> 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`?
ah, I see. Done.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:128
+
+  // A whitelist of renamable symbols.
+  if (llvm::isa<CXXRecordDecl>(RenameDecl))
----------------
ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > 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?
> > 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**.
> 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.
Re-thinking this, you are right. Also, the indexable symbol kinds have a large overlap with the renamable symbol kinds.  Changed to blacklist.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:132
+  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.
----------------
ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > Isn't the same true for local rename?
> > no, actually the local rename does handle this, it finds all overriden methods from the AST, and updates all of them when doing rename.
> 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)
> 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.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:40
+  // When set, used by the rename to get file content for all rename-related
+  // files (except the main file).
+  // If there is no corresponding dirty buffer, we will use the file content
----------------
ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > hokein wrote:
> > > > ilya-biryukov wrote:
> > > > > Why is `MainFile` special? Can it also be handled with the GetDirtyBuffer?
> > > > 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). 
> > > 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.
> > yeah, having a single source for the file content could help, but I think this helper is internal-only. 
> I might be missing something, but it's in the public API of rename, right?
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69263/new/

https://reviews.llvm.org/D69263