[PATCH] D92290: [clangd] Factor out the heuristic resolver code into its own class

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Ok, I've made `HeuristicResolver` nullable, in the sense that I've added null checks arounds its use in `TargetFinder`.

I haven't refactored the tests to pass in a null resolver / annotate the ones that do or don't need one. Would you like that done in this patch, or a follow-up?



================
Comment at: clang-tools-extra/clangd/HeuristicResolver.h:60
+  // or more declarations that it likely references.
+  std::vector<const NamedDecl *> resolveExprToDecls(const Expr *E);
+
----------------
sammccall wrote:
> nridge wrote:
> > sammccall wrote:
> > > This is pretty vaguely defined. From reading the implementation, this can choose to return two things:
> > > 
> > > 1. a ValueDecl representing the value of E
> > > 2. a TypeDecl representing the type of E
> > > 
> > > Case 1 is used by heuristic go-to-def, which never hits case 2 because it only passes certain kinds of E.
> > > Case 1+2 are used internally by the heuristic resolver to resolve base types, and are unified by resolveDeclsToType which handles them as two cases.
> > > 
> > > This doesn't seem like a clean public API :-) It also seems to lead to some convolution internally.
> > > 
> > > --- 
> > > 
> > > I think we should distribute the work of this function into a few smaller functions with couple of flavors.
> > > 
> > > Some that deal with specific node types, extracted from resolveExprToDecls:
> > >  - `Decl* memberExprTarget(CXXDependentMemberExpr*)`. Uses exprToType (for the base type), pointee (for arrow exprs), resolveDependentMember (the final lookup).
> > >  - `Type* callExprType(CallExpr*)`. Uses exprToType (for the callee type).
> > >  - `Decl* declRefExprTarget(DependentScopeDeclRefExpr*)` - this is currently a trivial wrapper for resolveDependentMember.
> > > 
> > > And some that are more generic building blocks:
> > >  - `Type* exprType(Expr*)`, which tries to handle any expr, dispatching to the node-specific function and falling back to Expr::getType().
> > >  - `getPointeeType(Type*)` and `resolveDependentMember(...)` as now
> > > 
> > > It's not entirely clear what should be public vs private, but one idea I quite like is: all the node-specific methods are public, and all the building blocks are private.
> > > - This would mean adding some more node-specific methods to cover the external callers of `resolveDependentMember`, but in each case I looked at this seems to make sense.
> > >   (This neatly deals with the slightly grungy signature of resolveDependentMember by hiding it)
> > > - it also mostly answers the question about contracts on non-dependent code: most non-dependent node types simply can't be passed in
> > Thanks, you make some good points about this method being messy (and particularly that we're duplicating some "resolve the type of a non-dependent expression" logic, which has bothered me as well).
> > 
> > I like your proposed breakdown into smaller methods, and I partially implemented it.
> > 
> > A couple of notes:
> > 
> >  * I had to keep the [MemberExpr case](https://searchfox.org/llvm/rev/b3d7e761e347d562333893652dcf3837fa55d777/clang-tools-extra/clangd/FindTarget.cpp#218) around. It could not be subsumed by `Expr::getType()`, because for a `MemberExpr` that returns [this thing](https://searchfox.org/llvm/rev/b3d7e761e347d562333893652dcf3837fa55d777/clang/include/clang/AST/BuiltinTypes.def#283), while `getMemberDecl()->getType()` returns the function type of the member function, and we want the latter (for [this call site](https://searchfox.org/llvm/rev/b3d7e761e347d562333893652dcf3837fa55d777/clang-tools-extra/clangd/FindTarget.cpp#205)).
> >  * We don't quite achieve "non-dependent node types simply can't be passed in", because `resolveCallExpr()` can be called with a non-dependent call expr. However, it currently has no external callers -- perhaps we should make it private?
> > 
> > I left `resolveDependentMember()` public (and left its external callers alone) for now. I can make it private and add additional public wrappers if you'd like, but I'm wondering what the motivation is (especially as you've described it as "a really nice example of a heuristic lookup with a clear contract" :-)).
> Those notes/caveats seem totally fine to me.
> 
> > resolveCallExpr ... perhaps we should make it private?
> >  left resolveDependentMember() public ... I can make it private... I'm wondering what the motivation is
> 
> I just think it's a little easier to understand how to use this class, and how to maintain/extend it, if the public methods form a consistent set with the same level of abstraction. Having all the public methods handle node types is tempting from this point of view.
> 
> I won't insist on this. We can inline the handling of UnusedUsingValueDecl into the caller as it happens to be simple, but I don't see a strong reason to do so.
Your argument for consistency, together with the fact that this allows moving the `Filter` lambdas out of the header file, has convinced me. Done!


================
Comment at: clang-tools-extra/clangd/ParsedAST.h:113
 
+  HeuristicResolver *getHeuristicResolver() const { return Resolver.get(); }
+
----------------
sammccall wrote:
> is this intended to be nullable? (If not, why the pointer type?)
I changed this to return `const HeuristicResolver *`, but kept it as a pointer for a couple of reasons:

 * I couldn't make `ParsedAST` store `HeuristicResolver` by value because it would make `ParsedAST` non-movable (`HeuristicResolver` is non-movable because it has a member of reference type (`ASTContext&`)).
 * The consumer functions (targetDecl() etc.) expect a pointer, so this avoids writing `&AST->getHeuristicResolver()` at every call site).


================
Comment at: clang-tools-extra/clangd/ParsedAST.h:150
   CanonicalIncludes CanonIncludes;
+  std::unique_ptr<HeuristicResolver> Resolver;
 };
----------------
nridge wrote:
> sammccall wrote:
> > Optional<HeuristicResolver> or simply HeuristicResolver?
> I wanted to make it just `HeuristicResolver` but it's accessed through a `const ParsedAST` which would only expose a `const HeuristicResolver *`, and I had already sprinkled `HeuristicResolver *` (without the const) everywhere :)
> 
> I'll clean this up after we settle the nullability question.
See previous comment for why I couldn't make it `HeuristicResolver`. (`Optional` doesn't work for the same reason.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92290