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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


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


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


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


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


================
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);
----------------
hokein wrote:
> 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.
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.


================
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
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263