[CodeComplete] Default parameter names of completed closure to internal names

ahoppen · ahoppen · commit 865e80f9c46b · 2021-04-01T19:14:19.000+02:00
If have a function that takes a trailing closure as follows
```
func sort(callback: (_ left: Int, _ right: Int) -&gt; Bool) {}
```
completing a call to `sort` and expanding the trailing closure results in
```
sort { &lt;#Int#&gt;, &lt;#Int#&gt; in
  &lt;#code#&gt;
}
```

We should be doing a better job here and defaulting the trailing closure's to the internal names specified in the function signature. I.e. the final result should be
```
sort { left, right in
  &lt;#code#&gt;
}
```

This commit does exactly that.

Firstly, it keeps track of the closure's internal names (as specified in the declaration of `sort`) in the closure's type through a new `InternalLabel` property in `AnyFunctionType::Param`. Once the type containing the parameter gets canonicalized, the internal label is dropped.

Secondly, it adds a new option to `ASTPrinter` to always try and print parameter labels. With this option set to true, it will always print external paramter labels and, if they are present, print the internal parameter label as `_ &lt;internalLabel&gt;`.

Finally, we can use this new printing mode to print the trailing closure’s type as
```
&lt;#T##callback: (Int, Int) -&gt; Bool##(_ left: Int, _ right: Int) -&gt; Bool#&gt;
```

This is already correctly expanded by code-expand to the desired result. I also added a test case for that behaviour.
diff --git a/include/swift/AST/PrintOptions.h b/include/swift/AST/PrintOptions.h
@@ -371,6 +371,12 @@ struct PrintOptions {
   /// Whether to print the extensions from conforming protocols.
   bool PrintExtensionFromConformingProtocols = false;
 
+  /// Whether to always try and print parameter labels. If present, print the
+  /// external parameter name. Otherwise try printing the internal name as
+  /// `_ <internalName>`, if an internal name exists. If neither an external nor
+  /// an internal name exists, only print the parameter's type.
+  bool AlwaysTryPrintParameterLabels = false;
+
   std::shared_ptr<ShouldPrintChecker> CurrentPrintabilityChecker =
     std::make_shared<ShouldPrintChecker>();
 
diff --git a/include/swift/AST/Types.h b/include/swift/AST/Types.h
@@ -2765,8 +2765,9 @@ class AnyFunctionType : public TypeBase {
   public:
     explicit Param(Type t,
                    Identifier l = Identifier(),
-                   ParameterTypeFlags f = ParameterTypeFlags())
-        : Ty(t), Label(l), Flags(f) {
+                   ParameterTypeFlags f = ParameterTypeFlags(),
+                   Identifier internalLabel = Identifier())
+        : Ty(t), Label(l), InternalLabel(internalLabel), Flags(f) {
       assert(t && "param type must be non-null");
       assert(!t->is<InOutType>() && "set flags instead");
     }
@@ -2776,8 +2777,18 @@ class AnyFunctionType : public TypeBase {
     /// element type.
     Type Ty;
     
-    // The label associated with the parameter, if any.
+    /// The label associated with the parameter, if any.
     Identifier Label;
+
+    /// The internal label of the parameter, if explicitly specified, otherwise
+    /// empty. The internal label is considered syntactic sugar. It is not
+    /// considered part of the canonical type and is thus also ignored in \c
+    /// operator==.
+    /// E.g.
+    ///  - `name name2: Int` has internal label `name2`
+    ///  - `_ name2: Int` has internal label `name2`
+    ///  - `name: Int` has no internal label
+    Identifier InternalLabel;
     
     /// Parameter specific flags.
     ParameterTypeFlags Flags = {};
@@ -2803,6 +2814,9 @@ class AnyFunctionType : public TypeBase {
 
     bool hasLabel() const { return !Label.empty(); }
     Identifier getLabel() const { return Label; }
+
+    bool hasInternalLabel() const { return !InternalLabel.empty(); }
+    Identifier getInternalLabel() const { return InternalLabel; }
     
     ParameterTypeFlags getParameterFlags() const { return Flags; }
 
@@ -2831,23 +2845,34 @@ class AnyFunctionType : public TypeBase {
       return Flags.getValueOwnership();
     }
 
+    /// Returns \c true if the two \c Params are equal in their canonicalized
+    /// form.
+    /// Two \c Params are equal if their external label, flags and
+    /// *canonicalized* types match. The internal label and sugar types are
+    /// *not* considered for type equality.
     bool operator==(Param const &b) const {
       return (Label == b.Label &&
               getPlainType()->isEqual(b.getPlainType()) &&
               Flags == b.Flags);
     }
     bool operator!=(Param const &b) const { return !(*this == b); }
 
-    Param getWithoutLabel() const { return Param(Ty, Identifier(), Flags); }
+    /// Return the parameter without external and internal labels.
+    Param getWithoutLabels() const {
+      return Param(Ty, /*Label=*/Identifier(), Flags,
+                   /*InternalLabel=*/Identifier());
+    }
 
     Param withLabel(Identifier newLabel) const {
-      return Param(Ty, newLabel, Flags);
+      return Param(Ty, newLabel, Flags, InternalLabel);
     }
 
-    Param withType(Type newType) const { return Param(newType, Label, Flags); }
+    Param withType(Type newType) const {
+      return Param(newType, Label, Flags, InternalLabel);
+    }
 
     Param withFlags(ParameterTypeFlags flags) const {
-      return Param(Ty, Label, flags);
+      return Param(Ty, Label, flags, InternalLabel);
     }
   };
 
@@ -2968,14 +2993,19 @@ class AnyFunctionType : public TypeBase {
     return composeInput(ctx, params.getOriginalArray(), canonicalVararg);
   }
 
-  /// Given two arrays of parameters determine if they are equal.
+  /// Given two arrays of parameters determine if they are equal in their
+  /// canonicalized form. Internal labels and type sugar is *not* taken into
+  /// account.
   static bool equalParams(ArrayRef<Param> a, ArrayRef<Param> b);
 
-  /// Given two arrays of parameters determine if they are equal.
+  /// Given two arrays of parameters determine if they are equal in their
+  /// canonicalized form. Internal labels and type sugar is *not* taken into
+  /// account.
   static bool equalParams(CanParamArrayRef a, CanParamArrayRef b);
 
   /// Given an array of parameters and an array of labels of the
   /// same length, update each parameter to have the corresponding label.
+  /// The internal parameter labels remain the same.
   static void relabelParams(MutableArrayRef<Param> params,
                             ArrayRef<Identifier> labels);
 
diff --git a/lib/AST/ASTContext.cpp b/lib/AST/ASTContext.cpp
@@ -3085,11 +3085,15 @@ getFunctionRecursiveProperties(ArrayRef<AnyFunctionType::Param> params,
 }
 
 static bool
-isFunctionTypeCanonical(ArrayRef<AnyFunctionType::Param> params,
+isAnyFunctionTypeCanonical(ArrayRef<AnyFunctionType::Param> params,
                         Type result) {
   for (auto param : params) {
     if (!param.getPlainType()->isCanonical())
       return false;
+    if (!param.getInternalLabel().empty()) {
+      // Canonical types don't have internal labels
+      return false;
+    }
   }
 
   return result->isCanonical();
@@ -3128,6 +3132,10 @@ isGenericFunctionTypeCanonical(GenericSignature sig,
   for (auto param : params) {
     if (!sig->isCanonicalTypeInContext(param.getPlainType()))
       return false;
+    if (!param.getInternalLabel().empty()) {
+      // Canonical types don't have internal labels
+      return false;
+    }
   }
 
   return sig->isCanonicalTypeInContext(result);
@@ -3231,17 +3239,21 @@ void AnyFunctionType::relabelParams(MutableArrayRef<Param> params,
   assert(params.size() == labels.size());
   for (auto i : indices(params)) {
     auto &param = params[i];
-    param = AnyFunctionType::Param(param.getPlainType(),
-                                   labels[i],
-                                   param.getParameterFlags());
+    param = AnyFunctionType::Param(param.getPlainType(), labels[i],
+                                   param.getParameterFlags(),
+                                   param.getInternalLabel());
   }
 }
 
+/// Profile \p params into \p ID. In contrast to \c == on \c Param, the profile
+/// *does* take the internal label into account and *does not* canonicalize
+/// the param's type.
 static void profileParams(llvm::FoldingSetNodeID &ID,
                           ArrayRef<AnyFunctionType::Param> params) {
   ID.AddInteger(params.size());
   for (auto param : params) {
     ID.AddPointer(param.getLabel().get());
+    ID.AddPointer(param.getInternalLabel().get());
     ID.AddPointer(param.getPlainType().getPointer());
     ID.AddInteger(param.getParameterFlags().toRaw());
   }
@@ -3293,7 +3305,7 @@ FunctionType *FunctionType::get(ArrayRef<AnyFunctionType::Param> params,
     >(params.size(), hasClangInfo ? 1 : 0, globalActor ? 1 : 0);
   void *mem = ctx.Allocate(allocSize, alignof(FunctionType), arena);
 
-  bool isCanonical = isFunctionTypeCanonical(params, result);
+  bool isCanonical = isAnyFunctionTypeCanonical(params, result);
   if (!clangTypeInfo.empty()) {
     if (ctx.LangOpts.UseClangFunctionTypes)
       isCanonical &= clangTypeInfo.getType()->isCanonicalUnqualified();
diff --git a/lib/AST/ASTDumper.cpp b/lib/AST/ASTDumper.cpp
@@ -3807,6 +3807,8 @@ namespace {
         PrintWithColorRAII(OS, TypeFieldColor) << "param";
         if (param.hasLabel())
           printField("name", param.getLabel().str());
+        if (param.hasInternalLabel())
+          printField("internal_name", param.getInternalLabel().str());
         dumpParameterFlags(param.getParameterFlags());
         printRec(param.getPlainType());
         OS << ")";
diff --git a/lib/AST/ASTPrinter.cpp b/lib/AST/ASTPrinter.cpp
@@ -4759,10 +4759,23 @@ class TypePrinter : public TypeVisitor<TypePrinter> {
         Printer.printStructurePost(PrintStructureKind::FunctionParameter);
       };
 
-      if (printLabels && Param.hasLabel()) {
+      if ((Options.AlwaysTryPrintParameterLabels || printLabels) &&
+          Param.hasLabel()) {
+        // Label printing was requested and we have an external label. Print it
+        // and omit the internal label.
         Printer.printName(Param.getLabel(),
                           PrintNameContext::FunctionParameterExternal);
         Printer << ": ";
+      } else if (Options.AlwaysTryPrintParameterLabels &&
+                 Param.hasInternalLabel()) {
+        // We didn't have an external parameter label but were requested to
+        // always try and print parameter labels. Print The internal label.
+        // If we have neither an external nor an internal label, only print the
+        // type.
+        Printer << "_ ";
+        Printer.printName(Param.getInternalLabel(),
+                          PrintNameContext::FunctionParameterLocal);
+        Printer << ": ";
       }
 
       auto type = Param.getPlainType();
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
@@ -2598,7 +2598,8 @@ static Type mapSignatureFunctionType(ASTContext &ctx, Type type,
       newFlags = newFlags.withInOut(false);
     }
 
-    AnyFunctionType::Param newParam(newParamType, param.getLabel(), newFlags);
+    AnyFunctionType::Param newParam(newParamType, param.getLabel(), newFlags,
+                                    param.getInternalLabel());
     newParams.push_back(newParam);
   }
 
@@ -6290,11 +6291,12 @@ AnyFunctionType::Param ParamDecl::toFunctionParam(Type type) const {
     type = ParamDecl::getVarargBaseTy(type);
 
   auto label = getArgumentName();
+  auto internalLabel = getParameterName();
   auto flags = ParameterTypeFlags::fromParameterType(
       type, isVariadic(), isAutoClosure(), isNonEphemeral(),
       getValueOwnership(),
       /*isNoDerivative*/ false);
-  return AnyFunctionType::Param(type, label, flags);
+  return AnyFunctionType::Param(type, label, flags, internalLabel);
 }
 
 Optional<Initializer *> ParamDecl::getCachedDefaultArgumentInitContext() const {
diff --git a/lib/AST/Type.cpp b/lib/AST/Type.cpp
@@ -833,7 +833,7 @@ Type TypeBase::removeArgumentLabels(unsigned numArgumentLabels) {
   llvm::SmallVector<AnyFunctionType::Param, 8> unlabeledParams;
   unlabeledParams.reserve(fnType->getNumParams());
   for (const auto &param : fnType->getParams())
-    unlabeledParams.push_back(param.getWithoutLabel());
+    unlabeledParams.push_back(param.getWithoutLabels());
 
   auto result = fnType->getResult()
                       ->removeArgumentLabels(numArgumentLabels - 1);
@@ -1014,9 +1014,7 @@ rebuildSelfTypeWithObjectType(AnyFunctionType::Param selfParam,
   if (selfParam.getPlainType()->getAs<MetatypeType>())
     objectTy = MetatypeType::get(objectTy);
 
-  return AnyFunctionType::Param(objectTy,
-                                selfParam.getLabel(),
-                                selfParam.getParameterFlags());
+  return selfParam.withType(objectTy);
 }
 
 /// Returns a new function type exactly like this one but with the self
@@ -1191,9 +1189,11 @@ getCanonicalParams(AnyFunctionType *funcType,
                    SmallVectorImpl<AnyFunctionType::Param> &canParams) {
   auto origParams = funcType->getParams();
   for (auto param : origParams) {
+    // Canonicalize the type and drop the internal label to canonicalize the
+    // Param.
     canParams.emplace_back(param.getPlainType()->getCanonicalType(genericSig),
-                           param.getLabel(),
-                           param.getParameterFlags());
+                           param.getLabel(), param.getParameterFlags(),
+                           /*InternalLabel=*/Identifier());
   }
 }
 
@@ -4776,6 +4776,7 @@ case TypeKind::Id:
       auto type = param.getPlainType();
       auto label = param.getLabel();
       auto flags = param.getParameterFlags();
+      auto internalLabel = param.getInternalLabel();
 
       auto substType = type.transformRec(fn);
       if (!substType)
@@ -4794,7 +4795,7 @@ case TypeKind::Id:
         flags = flags.withInOut(true);
       }
 
-      substParams.emplace_back(substType, label, flags);
+      substParams.emplace_back(substType, label, flags, internalLabel);
     }
 
     // Transform result type.
diff --git a/lib/IDE/CodeCompletion.cpp b/lib/IDE/CodeCompletion.cpp
@@ -1001,6 +1001,7 @@ void CodeCompletionResultBuilder::addCallParameter(Identifier Name,
     PO.SkipAttributes = true;
     PO.OpaqueReturnTypePrinting =
         PrintOptions::OpaqueReturnTypePrintingMode::WithoutOpaqueKeyword;
+    PO.AlwaysTryPrintParameterLabels = true;
     if (ContextTy)
       PO.setBaseType(ContextTy);
 
@@ -1017,6 +1018,8 @@ void CodeCompletionResultBuilder::addCallParameter(Identifier Name,
 
         if (param.hasLabel()) {
           OS << param.getLabel();
+        } else if (param.hasInternalLabel()) {
+          OS << param.getInternalLabel();
         } else {
           OS << "<#";
           if (param.isInOut())
@@ -2337,8 +2340,7 @@ class CompletionLookup final : public swift::VisibleDeclConsumer {
       SmallVector<AnyFunctionType::Param, 8> erasedParams;
       for (const auto &param : genericFuncType->getParams()) {
         auto erasedTy = eraseArchetypes(param.getPlainType(), genericSig);
-        erasedParams.emplace_back(erasedTy, param.getLabel(),
-                                  param.getParameterFlags());
+        erasedParams.emplace_back(param.withType(erasedTy));
       }
       return GenericFunctionType::get(genericSig,
           erasedParams,
diff --git a/lib/SIL/IR/Bridging.cpp b/lib/SIL/IR/Bridging.cpp
@@ -53,9 +53,7 @@ TypeConverter::getBridgedParam(SILFunctionTypeRepresentation rep,
      llvm::report_fatal_error("unable to set up the ObjC bridge!");
   }
 
-  return AnyFunctionType::Param(bridged->getCanonicalType(),
-                                param.getLabel(),
-                                param.getParameterFlags());
+  return param.withType(bridged->getCanonicalType());
 }
 
 void TypeConverter::
diff --git a/lib/Sema/CSApply.cpp b/lib/Sema/CSApply.cpp
@@ -1040,8 +1040,9 @@ namespace {
 
           // SILGen knows how to emit property-wrapped parameters, but the
           // function type needs the backing wrapper type in its param list.
-          innerParamTypes.push_back(AnyFunctionType::Param(paramRef->getType(),
-                                                           innerParam->getArgumentName()));
+          innerParamTypes.push_back(AnyFunctionType::Param(
+              paramRef->getType(), innerParam->getArgumentName(),
+              ParameterTypeFlags(), innerParam->getParameterName()));
         } else {
           // Rewrite the parameter ref if necessary.
           if (outerParam->isInOut()) {
diff --git a/lib/Sema/CSDiagnostics.cpp b/lib/Sema/CSDiagnostics.cpp
@@ -4471,7 +4471,15 @@ bool MissingArgumentsFailure::diagnoseClosure(const ClosureExpr *closure) {
       fixText += " ";
       interleave(
           funcType->getParams(),
-          [&fixText](const AnyFunctionType::Param &param) { fixText += '_'; },
+          [&fixText](const AnyFunctionType::Param &param) {
+            if (param.hasLabel()) {
+              fixText += param.getLabel().str();
+            } else if (param.hasInternalLabel()) {
+              fixText += param.getInternalLabel().str();
+            } else {
+              fixText += '_';
+            }
+          },
           [&fixText] { fixText += ','; });
       fixText += " in ";
     }
diff --git a/lib/Sema/CSGen.cpp b/lib/Sema/CSGen.cpp
@@ -2423,7 +2423,7 @@ namespace {
           // Remove parameter labels; they aren't used when matching cases,
           // but outright conflicts will be checked during coercion.
           for (auto &param : params) {
-            param = param.getWithoutLabel();
+            param = param.getWithoutLabels();
           }
 
           Type outputType = CS.createTypeVariable(
diff --git a/lib/Sema/ConstraintSystem.cpp b/lib/Sema/ConstraintSystem.cpp
@@ -1214,7 +1214,9 @@ unwrapPropertyWrapperParameterTypes(ConstraintSystem &cs, AbstractFunctionDecl *
     auto *wrappedType = cs.createTypeVariable(cs.getConstraintLocator(locator), 0);
     auto paramType = paramTypes[i].getParameterType();
     auto paramLabel = paramTypes[i].getLabel();
-    adjustedParamTypes.push_back(AnyFunctionType::Param(wrappedType, paramLabel));
+    auto paramInternalLabel = paramTypes[i].getInternalLabel();
+    adjustedParamTypes.push_back(AnyFunctionType::Param(
+        wrappedType, paramLabel, ParameterTypeFlags(), paramInternalLabel));
     cs.applyPropertyWrapperToParameter(paramType, wrappedType, paramDecl, argLabel,
                                        ConstraintKind::Equal, locator);
   }
diff --git a/lib/Sema/TypeCheckType.cpp b/lib/Sema/TypeCheckType.cpp
@@ -2699,7 +2699,8 @@ TypeResolver::resolveASTFunctionTypeParams(TupleTypeRepr *inputRepr,
     auto paramFlags = ParameterTypeFlags::fromParameterType(
         ty, variadic, autoclosure, /*isNonEphemeral*/ false, ownership,
         noDerivative);
-    elements.emplace_back(ty, Identifier(), paramFlags);
+    elements.emplace_back(ty, Identifier(), paramFlags,
+                          inputRepr->getElementName(i));
   }
 
   // All non-`@noDerivative` parameters of `@differentiable` function types must
diff --git a/test/IDE/complete_call_arg.swift b/test/IDE/complete_call_arg.swift
diff --git a/test/IDE/complete_multiple_trailingclosure_signatures.swift b/test/IDE/complete_multiple_trailingclosure_signatures.swift
diff --git a/test/IDE/complete_value_expr.swift b/test/IDE/complete_value_expr.swift
diff --git a/test/SourceKit/CodeExpand/code-expand.swift b/test/SourceKit/CodeExpand/code-expand.swift

Original file line number	Diff line number	Diff line change
`@@ -2598,7 +2598,8 @@ static Type mapSignatureFunctionType(ASTContext &ctx, Type type,`
`2598`	`2598`	`newFlags = newFlags.withInOut(false);`
`2599`	`2599`	`}`
`2600`	`2600`
`2601`		`- AnyFunctionType::Param newParam(newParamType, param.getLabel(), newFlags);`
	`2601`	`+ AnyFunctionType::Param newParam(newParamType, param.getLabel(), newFlags,`
	`2602`	`+ param.getInternalLabel());`
`2602`	`2603`	`newParams.push_back(newParam);`
`2603`	`2604`	`}`
`2604`	`2605`
`@@ -6290,11 +6291,12 @@ AnyFunctionType::Param ParamDecl::toFunctionParam(Type type) const {`
`6290`	`6291`	`type = ParamDecl::getVarargBaseTy(type);`
`6291`	`6292`
`6292`	`6293`	`auto label = getArgumentName();`
	`6294`	`+ auto internalLabel = getParameterName();`
`6293`	`6295`	`auto flags = ParameterTypeFlags::fromParameterType(`
`6294`	`6296`	`type, isVariadic(), isAutoClosure(), isNonEphemeral(),`
`6295`	`6297`	`getValueOwnership(),`
`6296`	`6298`	`/isNoDerivative/ false);`
`6297`		`- return AnyFunctionType::Param(type, label, flags);`
	`6299`	`+ return AnyFunctionType::Param(type, label, flags, internalLabel);`
`6298`	`6300`	`}`
`6299`	`6301`
`6300`	`6302`	`Optional<Initializer *> ParamDecl::getCachedDefaultArgumentInitContext() const {`
Original file line number	Diff line number	Diff line change
`@@ -53,9 +53,7 @@ TypeConverter::getBridgedParam(SILFunctionTypeRepresentation rep,`
`53`	`53`	`llvm::report_fatal_error("unable to set up the ObjC bridge!");`
`54`	`54`	`}`
`55`	`55`
`56`		`- return AnyFunctionType::Param(bridged->getCanonicalType(),`
`57`		`- param.getLabel(),`
`58`		`- param.getParameterFlags());`
	`56`	`+ return param.withType(bridged->getCanonicalType());`
`59`	`57`	`}`
`60`	`58`
`61`	`59`	`void TypeConverter::`