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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.


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


================
Comment at: clang-tools-extra/clangd/HeuristicResolver.h:64
+  // specifier.
+  const Type *resolveNestedNameSpecifierToType(const NestedNameSpecifier *NNS);
+
----------------
sammccall wrote:
> The contract is a bit fuzzy here, and both the caller and callee have to deal with NNS recursion. It could be narrowed by requiring the resolved parent type to be passed in...
I'm a bit unclear on what you mean by "both the caller and the callee have to deal with NNS recursion".

Looking through the callees, other than the recursive call in the implementation they all just make a single call to `resolveNestedNameSpecifiedToType()` and don't deal with NNS recursion?


================
Comment at: clang-tools-extra/clangd/ParsedAST.h:150
   CanonicalIncludes CanonIncludes;
+  std::unique_ptr<HeuristicResolver> Resolver;
 };
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92290