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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:370
+      for (auto &E : *Edits) {
+        if (auto Err = reformatEdit(E.getValue(), Style))
+          elog("Failed to format replacements: {0}", std::move(Err));
----------------
ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > Maybe use `llvm::joinErrors` to combine multiple failures into a single error?
> > > Should simplify the code.
> > 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));
> >    }
> > }
> > ```
> 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));
> ```
thanks.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:107
+    if (!Index)
+      return NoIndexProvided;
+
----------------
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?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:128
+
+  // A whitelist of renamable symbols.
+  if (llvm::isa<CXXRecordDecl>(RenameDecl))
----------------
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**.


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


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:221
+  if (!Index)
+    return {};
+  RefsRequest RQuest;
----------------
ilya-biryukov wrote:
> 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?
moved the no-index handling to the caller, the caller code is still a bit fuzzy.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:235
+  });
+  return AffectedFiles;
+}
----------------
ilya-biryukov wrote:
> 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?
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.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:255
+    // !positionToOffset is O(N), it is okay at the moment since we only
+    // process at most 100 references.
+    auto RangeOffset = toRangeOffset(Occurrence, InitialCode);
----------------
ilya-biryukov wrote:
> 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.
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263