[clangd] Refactor forwarding call detection logic

[ilya-biryukov]

The new test fails in the pre merge checks.
Maybe the diff is missing some more changes?

clang-tools-extra/clangd/AST.cpp

793–794

NIT: maybe rename to PackLocation?
now that there is only 1 variable we could use a shorter name without any confusion

828

NIT: doesn't this work without dyn_cast? looks redundant, I expect derived-to-base conversion to do the right thing.

871–874

Could you update the comment to account for the new semantics?

clang-tools-extra/clangd/unittests/InlayHintTests.cpp

1446 ↗

(On Diff #446457)

NIT: maybe test for the case with a single expansion here:

foo(Foo(args...), 1);
foo({args...}, 1);

?

testing multiple expansions is also interesting, but seems orthogonal to the change being made here.
E.g. tests for it would probably benefit from more than 2 appearances of args and more complicated nesting structures.

[kadircet]

• Address comments

clang-tools-extra/clangd/unittests/InlayHintTests.cpp

1446 ↗

(On Diff #446457)

this was mostly a "sugar" patch, the tests are part of D130260 but I didn't do a good job of splitting the change 😅

[ilya-biryukov]

LGTM

clang-tools-extra/clangd/AST.cpp

 private:
   // inspects the given callee with the given args to check whether it
   // contains Parameters, and sets Info accordingly.
   void handleCall(FunctionDecl *Callee, typename CallExpr::arg_range Args) {
     if (std::any_of(Args.begin(), Args.end(), [](const Expr *E) {
           return dyn_cast<PackExpansionExpr>(E) != nullptr;
         })) {
       return;
     }
-    auto OptPackLocation = findPack(Args);
-    if (OptPackLocation) {
-      size_t PackLocation = OptPackLocation.value();
-      ArrayRef<ParmVarDecl *> MatchingParams =
-          Callee->parameters().slice(PackLocation, Parameters.size());
-      // Check whether the function has a parameter pack as the last template
-      // parameter
-      if (const auto *TTPT = getFunctionPackType(Callee)) {
-        // In this case: Separate the parameters into head, pack and tail
-        auto IsExpandedPack = [&](const ParmVarDecl *P) {
-          return getUnderylingPackType(P) == TTPT;
-        };
-        ForwardingInfo FI;
-        FI.Head = MatchingParams.take_until(IsExpandedPack);
-        FI.Pack = MatchingParams.drop_front(FI.Head.size())
-                      .take_while(IsExpandedPack);
-        FI.Tail = MatchingParams.drop_front(FI.Head.size() + FI.Pack.size());
-        FI.PackTarget = Callee;
-        Info = FI;
-        return;
-      }
-      // Default case: assume all parameters were fully resolved
-      ForwardingInfo FI;
-      FI.Head = MatchingParams;
-      Info = FI;
-    }
-  }
+    auto PackLocation = findPack(Args);
+    if (!PackLocation)
+      return;
+        Callee->parameters().slice(*PackLocation, Parameters.size());
+      FI.Pack =
+          MatchingParams.drop_front(FI.Head.size()).take_while(IsExpandedPack);
 
   // Returns the beginning of the expanded pack represented by Parameters
   // in the given arguments, if it is there.
   llvm::Optional<size_t> findPack(typename CallExpr::arg_range Args) {
     // find the argument directly referring to the first parameter
-    auto FirstMatch = std::find_if(Args.begin(), Args.end(), [&](Expr *Arg) {
-      const auto *RefArg = unwrapArgument(Arg);
-      if (RefArg) {
-        if (Parameters.front() == dyn_cast<ParmVarDecl>(RefArg->getDecl())) {
-          return true;
-        }
-      }
-      return false;
-    });
-    if (FirstMatch == Args.end()) {
-      return llvm::None;
-    }
-    return std::distance(Args.begin(), FirstMatch);
-  }
+    for (auto It = Args.begin(); It != Args.end(); ++It) {
+      const Expr *Arg = *It;
+      if (const auto *RefArg = unwrapForward(Arg)) {
+        if (Parameters.front() != RefArg->getDecl())
+          continue;
+        return std::distance(Args.begin(), It);
 
   static FunctionDecl *getCalleeDeclOrUniqueOverload(CallExpr *E) {
     Decl *CalleeDecl = E->getCalleeDecl();
     auto *Callee = dyn_cast_or_null<FunctionDecl>(CalleeDecl);
     if (!Callee) {
       if (auto *Lookup = dyn_cast<UnresolvedLookupExpr>(E->getCallee())) {
         Callee = resolveOverload(Lookup, E);
       }
     }
     // Ignore the callee if the number of arguments is wrong (deal with va_args)
-    if (Callee->getNumParams() == E->getNumArgs())
+    if (Callee && Callee->getNumParams() == E->getNumArgs())
       return Callee;
     return nullptr;
   }
 
   static FunctionDecl *resolveOverload(UnresolvedLookupExpr *Lookup,
                                        CallExpr *E) {
     FunctionDecl *MatchingDecl = nullptr;
     if (!Lookup->requiresADL()) {
if (!Lookup->requiresADL()) {
             MatchingDecl = FuncCandidate;
           }
         }
       }
     }
     return MatchingDecl;
   }
 
-  // Removes any implicit cast expressions around the given expression.
-  static const Expr *unwrapImplicitCast(const Expr *E) {
-    while (const auto *Cast = dyn_cast<ImplicitCastExpr>(E)) {
-      E = Cast->getSubExpr();
-    }
-    return E;
-  }
-
-  // Maps std::forward(E) to E, nullptr otherwise
-  static const Expr *unwrapForward(const Expr *E) {
+  // Tries to get to the underlying argument by unwrapping implicit nodes and
+  // std::forward.
+  static const DeclRefExpr *unwrapForward(const Expr *E) {
+    E = E->IgnoreImplicitAsWritten();
     if (const auto *Call = dyn_cast<CallExpr>(E)) {
       const auto Callee = Call->getBuiltinCallee();
       if (Callee == Builtin::BIforward) {
-        return Call->getArg(0);
-      }
+        return dyn_cast<DeclRefExpr>(
+            Call->getArg(0)->IgnoreImplicitAsWritten());
       }
-    return E;
     }
-
-  // Maps std::forward(DeclRefExpr) to DeclRefExpr, removing any intermediate
-  // implicit casts, nullptr otherwise
-  static const DeclRefExpr *unwrapArgument(const Expr *E) {
-    E = unwrapImplicitCast(E);
-    E = unwrapForward(E);
-    E = unwrapImplicitCast(E);
     return dyn_cast<DeclRefExpr>(E);
   }
 };
 
 } // namespace
 
 SmallVector<const ParmVarDecl *>
 resolveForwardingParameters(const FunctionDecl *D, unsigned MaxDepth) {