[PATCH] D132797: [clangd] Support renaming virtual methods

[Tom Praschan via Phabricator via cfe-commits]

tom-anders marked 6 inline comments as done.
tom-anders added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:245
   trace::Span Tracer("FindOccurrencesWithinFile");
   assert(canonicalRenameDecl(&ND) == &ND &&
          "ND should be already canonicalized.");
----------------
sammccall wrote:
> Unfortunately we've uncovered a bug: this assertion fires (after this patch) on the following code:
> 
> ```
> template <typename> class Foo { virtual void m(); };
> class Bar : Foo<int> { void ^m() override; };
> ```
> 
> `canonicalRenameDecl` is supposed to be idempotent. However:`canonicalRenameDecl(Bar::m)` is `Foo<int>::m`, but `canonicalRenameDecl(Foo<int>::m)` is `Foo<T>::m`.
> 
> I think this is because the `getInstantiatedFromMemberFunction` check comes before the `overridden_methods` check, so if the override points to a member of an instantiation then it's too late to map it back to the member of the template.
> 
> Probably the best fix is to change line 103 to a recursive `return canonicalRenameDecl(...)`, as is done elsewhere in the function.
> 
> (Can you please add this case to the tests?)
I added the test case for this. However, your suggested fix doesn't seem to stop the assertion from being triggered. I'd be grateful if you can take a closer look at this in a separate review - Should I leave the test case in anyway? Is it okay if the test fails for a while until you found the fix?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:554
+  Req.Subjects = {Base};
+  Index.relations(Req, [&](const SymbolID &, const Symbol &Override) {
+    IDs.insert(Override.ID);
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > ilya-biryukov wrote:
> > > Should we put a limit on the number of requests we send during recursion here?
> > > 
> > > I see a few obvious failure modes:
> > > - infinite recursion in the relations due to parts of index being stale, corrupted input data or other reasons,
> > > - exponential blow up in hierarchies with multiple inheritance,
> > > - sending a lot of network requests in case of deep inheritance hierarchies for remote index implementations. Since all requests are sequential, the network latency might add up to substantial numbers.
> > > 
> > > We could address these in some other manner, this just seems to be the simplest option to protect against catastrophic outcomes (running the request indefinitely, crashing due to infinite recursion, etc).
> > > exponential blow up in hierarchies with multiple inheritance,
> > 
> > It seems with little loss of readability we could provide some useful bounds:
> > 
> > ```
> > DenseSet<SymbolID> Pending = {Base};
> > while (!Pending.empty()) {
> >   Req = {.Subjects = Pending};
> >   Pending.clear();
> >   Index.relations(Req, { IDs.insert(ID); Pending.insert(ID) });
> > }
> > ```
> > in this case the #requests is clearly bounded by the length of the shortest chain to the most distant SymbolID, and is IMO safe with no numeric cap.
> > 
> > whereas the current version could potentially get the same ID in multiple branches and so the bound on #requests is harder to determine.
> This looks good! Also avoids infinite recursion.
> Having a potentially large number of sequential network requests still looks unfortunate, but I doubt this will bite us in practice (at least not for remote indicies for LLVM and Chrome).
> 
> To solve it, we could allow recursive requests and implement the recursion inside the index, but this could be done with a follow-up when we actually hit this issue.
Done, hope I interpreted your sketch correctly


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132797