[CodeComplete] Match argument labels when completing function arguments

Previously, we always assumed that the position in the arguments would match the position in the parameter, which isn’t true in case of defaulted arguments. Try matching parameters based on argument labels to give better completion results.

Fixes rdar://60346573

Author: ahoppen

lib/IDE/ExprContextAnalysis.cpp

@@ -745,6 +745,79 @@ static bool getPositionInArgs(DeclContext &DC, Expr *Args, Expr *CCExpr,
   return false;
 }
 
+/// Get index of \p CCExpr in \p Params. Note that the position in \p Params may
+/// be different than the position in \p Args if there are defaulted arguments
+/// in \p Params which don't occur in \p Args.
+///
+/// \returns \c true if success, \c false if \p CCExpr is not a part of \p Args.
+static bool getPositionInParams(DeclContext &DC, Expr *Args, Expr *CCExpr,
+                                ArrayRef<AnyFunctionType::Param> Params,
+                                unsigned &PosInParams) {
+  if (isa<ParenExpr>(Args)) {
+    PosInParams = 0;
+    return true;
+  }
+
+  auto *tuple = dyn_cast<TupleExpr>(Args);
+  if (!tuple) {
+    return false;
+  }
+
+  auto &SM = DC.getASTContext().SourceMgr;
+  PosInParams = 0;
+  unsigned PosInArgs = 0;
+  bool LastParamWasVariadic = false;
+  // We advance PosInArgs until we find argument that is after the code
+  // completion token, which is when we stop.
+  // For each argument, we try to find a matching parameter either by matching
+  // argument labels, in which case PosInParams may be advanced by more than 1,
+  // or by advancing PosInParams and PosInArgs both by 1.
+  for (; PosInArgs < tuple->getNumElements(); ++PosInArgs) {
+    if (!SM.isBeforeInBuffer(tuple->getElement(PosInArgs)->getEndLoc(),
+                             CCExpr->getStartLoc())) {
+      // The arg is after the code completion position. Stop.
+      break;
+    }
+
+    auto ArgName = tuple->getElementName(PosInArgs);
+    // If the last parameter we matched was variadic, we claim all following
+    // unlabeled arguments for that variadic parameter -> advance PosInArgs but
+    // not PosInParams.
+    if (LastParamWasVariadic && ArgName.empty()) {
+      continue;
+    } else {
+      LastParamWasVariadic = false;
+    }
+
+    // Look for a matching parameter label.
+    bool FoundLabelMatch = false;
+    for (unsigned i = PosInParams; i < Params.size(); ++i) {
+      if (Params[i].getLabel() == ArgName) {
+        // We have found a label match. Advance the position in the params
+        // to point to the param after the one with this label.
+        PosInParams = i + 1;
+        FoundLabelMatch = true;
+        if (Params[i].isVariadic()) {
+          LastParamWasVariadic = true;
+        }
+        break;
+      }
+    }
+
+    if (!FoundLabelMatch) {
+      // We haven't found a matching argument label. Assume the current one is
+      // named incorrectly and advance by one.
+      ++PosInParams;
+    }
+  }
+  if (PosInArgs < tuple->getNumElements() && PosInParams < Params.size()) {
+    // We didn't search until the end, so we found a position in Params. Success
+    return true;
+  } else {
+    return false;
+  }
+}
+
 /// Given an expression and its context, the analyzer tries to figure out the
 /// expected type of the expression by analyzing its context.
 class ExprContextAnalyzer {
@@ -791,14 +864,12 @@ class ExprContextAnalyzer {
     PossibleCallees.assign(Candidates.begin(), Candidates.end());
 
     // Determine the position of code completion token in call argument.
-    unsigned Position;
+    unsigned PositionInArgs;
     bool HasName;
-    if (!getPositionInArgs(*DC, Arg, ParsedExpr, Position, HasName))
+    if (!getPositionInArgs(*DC, Arg, ParsedExpr, PositionInArgs, HasName))
       return false;
 
     // Collect possible types (or labels) at the position.
-    // FIXME: Take variadic and optional parameters into account. We need to do
-    //        something equivalent to 'constraints::matchCallArguments'
     {
       bool MayNeedName = !HasName && !E->isImplicit() &&
                          (isa<CallExpr>(E) | isa<SubscriptExpr>(E) ||
@@ -811,13 +882,22 @@ class ExprContextAnalyzer {
           memberDC = typeAndDecl.Decl->getInnermostDeclContext();
 
         auto Params = typeAndDecl.Type->getParams();
+        unsigned PositionInParams;
+        if (!getPositionInParams(*DC, Arg, ParsedExpr, Params,
+                                 PositionInParams)) {
+          // If the argument doesn't have a matching position in the parameters,
+          // indicate that with optional nullptr param.
+          if (seenArgs.insert({Identifier(), CanType()}).second)
+            recordPossibleParam(nullptr, /*isRequired=*/false);
+          continue;
+        }
         ParameterList *paramList = nullptr;
         if (auto VD = typeAndDecl.Decl) {
           paramList = getParameterList(VD);
           if (paramList && paramList->size() != Params.size())
             paramList = nullptr;
         }
-        for (auto Pos = Position; Pos < Params.size(); ++Pos) {
+        for (auto Pos = PositionInParams; Pos < Params.size(); ++Pos) {
           const auto &paramType = Params[Pos];
           Type ty = paramType.getPlainType();
           if (memberDC && ty->hasTypeParameter())
@@ -843,12 +923,6 @@ class ExprContextAnalyzer {
           if (!canSkip)
             break;
         }
-        // If the argument position is out of expeceted number, indicate that
-        // with optional nullptr param.
-        if (Position >= Params.size()) {
-          if (seenArgs.insert({Identifier(), CanType()}).second)
-            recordPossibleParam(nullptr, /*isRequired=*/false);
-        }
       }
     }
     return !PossibleTypes.empty() || !PossibleParams.empty();

test/IDE/complete_call_arg.swift

@@ -116,6 +116,10 @@
 
 // RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=LABEL_IN_SELF_DOT_INIT | %FileCheck %s -check-prefix=LABEL_IN_SELF_DOT_INIT
 
+// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=MISSING_REQUIRED_PARAM | %FileCheck %s -check-prefix=MISSING_REQUIRED_PARAM
+
+// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=NAMED_PARAMETER_WITH_LEADING_VARIADIC | %FileCheck %s -check-prefix=NAMED_PARAMETER_WITH_LEADING_VARIADIC
+
 var i1 = 1
 var i2 = 2
 var oi1 : Int?
@@ -700,9 +704,7 @@ func testStaticMemberCall() {
 // STATIC_METHOD_SECOND: End completions
 
   let _ = TestStaticMemberCall.create2(1, arg3: 2, #^STATIC_METHOD_SKIPPED^#)
-// STATIC_METHOD_SKIPPED: Begin completions, 2 items
-// FIXME: 'arg3' shouldn't be suggested.
-// STATIC_METHOD_SKIPPED: Pattern/ExprSpecific: {#arg3: Int#}[#Int#];
+// STATIC_METHOD_SKIPPED: Begin completions, 1 item
 // STATIC_METHOD_SKIPPED: Pattern/ExprSpecific: {#arg4: Int#}[#Int#];
 // STATIC_METHOD_SKIPPED: End completions
 
@@ -739,9 +741,7 @@ func testImplicitMember() {
 // IMPLICIT_MEMBER_SECOND: End completions
 
   let _: TestStaticMemberCall = .create2(1, arg3: 2, #^IMPLICIT_MEMBER_SKIPPED^#)
-// IMPLICIT_MEMBER_SKIPPED: Begin completions, 2 items
-// FIXME: 'arg3' shouldn't be suggested.
-// IMPLICIT_MEMBER_SKIPPED: Pattern/ExprSpecific: {#arg3: Int#}[#Int#];
+// IMPLICIT_MEMBER_SKIPPED: Begin completions, 1 item
 // IMPLICIT_MEMBER_SKIPPED: Pattern/ExprSpecific: {#arg4: Int#}[#Int#];
 // IMPLICIT_MEMBER_SKIPPED: End completions
 
@@ -923,3 +923,27 @@ func testLabelsInSelfDotInit() {
     }
   }
 }
+
+func testMissingRequiredParameter() {
+  class C {
+    func foo(x: Int, y: Int, z: Int)  {}
+  }
+  func test(c: C) {
+    c.foo(y: 1, #^MISSING_REQUIRED_PARAM^#)
+// MISSING_REQUIRED_PARAM: Begin completions, 1 item
+// MISSING_REQUIRED_PARAM-DAG: Pattern/ExprSpecific:               {#z: Int#}[#Int#]
+// MISSING_REQUIRED_PARAM: End completions
+  }
+}
+
+func testAfterVariadic() {
+  class C {
+    func foo(x: Int..., y: Int, z: Int)  {}
+  }
+  func test(c: C) {
+    c.foo(x: 10, 20, 30, y: 40, #^NAMED_PARAMETER_WITH_LEADING_VARIADIC^#)
+// NAMED_PARAMETER_WITH_LEADING_VARIADIC: Begin completions, 1 item
+// NAMED_PARAMETER_WITH_LEADING_VARIADIC-DAG: Pattern/ExprSpecific:               {#z: Int#}[#Int#]
+// NAMED_PARAMETER_WITH_LEADING_VARIADIC: End completions
+  }
+}

test/IDE/complete_call_as_function.swift

@@ -103,10 +103,8 @@ func testCallAsFunctionOverloaded(fn: Functor) {
 //OVERLOADED_PAREN: End completions
 
   fn(h: .left, #^OVERLOADED_ARG2_LABEL^#)
-// FIXME: Should only suggest 'v:' (rdar://problem/60346573).
-//OVERLOADED_ARG2_LABEL: Begin completions, 2 items
+//OVERLOADED_ARG2_LABEL: Begin completions, 1 item
 //OVERLOADED_ARG2_LABEL-DAG: Pattern/ExprSpecific:               {#v: Functor.Vertical#}[#Functor.Vertical#];
-//OVERLOADED_ARG2_LABEL-DAG: Pattern/ExprSpecific:               {#h: Functor.Horizontal#}[#Functor.Horizontal#];
 //OVERLOADED_ARG2_LABEL: End completions
 
   fn(h: .left, v: .#^OVERLOADED_ARG2_VALUE^#)