Skip to content

Commit 865e80f

Browse files
ahoppenahoppen
committed
[CodeComplete] Default parameter names of completed closure to internal names
If have a function that takes a trailing closure as follows ``` func sort(callback: (_ left: Int, _ right: Int) -> Bool) {} ``` completing a call to `sort` and expanding the trailing closure results in ``` sort { <#Int#>, <#Int#> in <#code#> } ``` 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 <#code#> } ``` 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 `_ <internalLabel>`. Finally, we can use this new printing mode to print the trailing closure’s type as ``` <#T##callback: (Int, Int) -> Bool##(_ left: Int, _ right: Int) -> Bool#> ``` This is already correctly expanded by code-expand to the desired result. I also added a test case for that behaviour.
1 parent 74b5eb7 commit 865e80f

18 files changed

+161
-37
lines changed

‎include/swift/AST/PrintOptions.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -371,6 +371,12 @@ struct PrintOptions {
371371
/// Whether to print the extensions from conforming protocols.
372372
bool PrintExtensionFromConformingProtocols = false;
373373

374+
/// Whether to always try and print parameter labels. If present, print the
375+
/// external parameter name. Otherwise try printing the internal name as
376+
/// `_ <internalName>`, if an internal name exists. If neither an external nor
377+
/// an internal name exists, only print the parameter's type.
378+
bool AlwaysTryPrintParameterLabels = false;
379+
374380
std::shared_ptr<ShouldPrintChecker> CurrentPrintabilityChecker =
375381
std::make_shared<ShouldPrintChecker>();
376382

‎include/swift/AST/Types.h

Lines changed: 39 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2765,8 +2765,9 @@ class AnyFunctionType : public TypeBase {
27652765
public:
27662766
explicit Param(Type t,
27672767
Identifier l = Identifier(),
2768-
ParameterTypeFlags f = ParameterTypeFlags())
2769-
: Ty(t), Label(l), Flags(f) {
2768+
ParameterTypeFlags f = ParameterTypeFlags(),
2769+
Identifier internalLabel = Identifier())
2770+
: Ty(t), Label(l), InternalLabel(internalLabel), Flags(f) {
27702771
assert(t && "param type must be non-null");
27712772
assert(!t->is<InOutType>() && "set flags instead");
27722773
}
@@ -2776,8 +2777,18 @@ class AnyFunctionType : public TypeBase {
27762777
/// element type.
27772778
Type Ty;
27782779

2779-
// The label associated with the parameter, if any.
2780+
/// The label associated with the parameter, if any.
27802781
Identifier Label;
2782+
2783+
/// The internal label of the parameter, if explicitly specified, otherwise
2784+
/// empty. The internal label is considered syntactic sugar. It is not
2785+
/// considered part of the canonical type and is thus also ignored in \c
2786+
/// operator==.
2787+
/// E.g.
2788+
/// - `name name2: Int` has internal label `name2`
2789+
/// - `_ name2: Int` has internal label `name2`
2790+
/// - `name: Int` has no internal label
2791+
Identifier InternalLabel;
27812792

27822793
/// Parameter specific flags.
27832794
ParameterTypeFlags Flags = {};
@@ -2803,6 +2814,9 @@ class AnyFunctionType : public TypeBase {
28032814

28042815
bool hasLabel() const { return !Label.empty(); }
28052816
Identifier getLabel() const { return Label; }
2817+
2818+
bool hasInternalLabel() const { return !InternalLabel.empty(); }
2819+
Identifier getInternalLabel() const { return InternalLabel; }
28062820

28072821
ParameterTypeFlags getParameterFlags() const { return Flags; }
28082822

@@ -2831,23 +2845,34 @@ class AnyFunctionType : public TypeBase {
28312845
return Flags.getValueOwnership();
28322846
}
28332847

2848+
/// Returns \c true if the two \c Params are equal in their canonicalized
2849+
/// form.
2850+
/// Two \c Params are equal if their external label, flags and
2851+
/// *canonicalized* types match. The internal label and sugar types are
2852+
/// *not* considered for type equality.
28342853
bool operator==(Param const &b) const {
28352854
return (Label == b.Label &&
28362855
getPlainType()->isEqual(b.getPlainType()) &&
28372856
Flags == b.Flags);
28382857
}
28392858
bool operator!=(Param const &b) const { return !(*this == b); }
28402859

2841-
Param getWithoutLabel() const { return Param(Ty, Identifier(), Flags); }
2860+
/// Return the parameter without external and internal labels.
2861+
Param getWithoutLabels() const {
2862+
return Param(Ty, /*Label=*/Identifier(), Flags,
2863+
/*InternalLabel=*/Identifier());
2864+
}
28422865

28432866
Param withLabel(Identifier newLabel) const {
2844-
return Param(Ty, newLabel, Flags);
2867+
return Param(Ty, newLabel, Flags, InternalLabel);
28452868
}
28462869

2847-
Param withType(Type newType) const { return Param(newType, Label, Flags); }
2870+
Param withType(Type newType) const {
2871+
return Param(newType, Label, Flags, InternalLabel);
2872+
}
28482873

28492874
Param withFlags(ParameterTypeFlags flags) const {
2850-
return Param(Ty, Label, flags);
2875+
return Param(Ty, Label, flags, InternalLabel);
28512876
}
28522877
};
28532878

@@ -2968,14 +2993,19 @@ class AnyFunctionType : public TypeBase {
29682993
return composeInput(ctx, params.getOriginalArray(), canonicalVararg);
29692994
}
29702995

2971-
/// Given two arrays of parameters determine if they are equal.
2996+
/// Given two arrays of parameters determine if they are equal in their
2997+
/// canonicalized form. Internal labels and type sugar is *not* taken into
2998+
/// account.
29722999
static bool equalParams(ArrayRef<Param> a, ArrayRef<Param> b);
29733000

2974-
/// Given two arrays of parameters determine if they are equal.
3001+
/// Given two arrays of parameters determine if they are equal in their
3002+
/// canonicalized form. Internal labels and type sugar is *not* taken into
3003+
/// account.
29753004
static bool equalParams(CanParamArrayRef a, CanParamArrayRef b);
29763005

29773006
/// Given an array of parameters and an array of labels of the
29783007
/// same length, update each parameter to have the corresponding label.
3008+
/// The internal parameter labels remain the same.
29793009
static void relabelParams(MutableArrayRef<Param> params,
29803010
ArrayRef<Identifier> labels);
29813011

‎lib/AST/ASTContext.cpp

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3085,11 +3085,15 @@ getFunctionRecursiveProperties(ArrayRef<AnyFunctionType::Param> params,
30853085
}
30863086

30873087
static bool
3088-
isFunctionTypeCanonical(ArrayRef<AnyFunctionType::Param> params,
3088+
isAnyFunctionTypeCanonical(ArrayRef<AnyFunctionType::Param> params,
30893089
Type result) {
30903090
for (auto param : params) {
30913091
if (!param.getPlainType()->isCanonical())
30923092
return false;
3093+
if (!param.getInternalLabel().empty()) {
3094+
// Canonical types don't have internal labels
3095+
return false;
3096+
}
30933097
}
30943098

30953099
return result->isCanonical();
@@ -3128,6 +3132,10 @@ isGenericFunctionTypeCanonical(GenericSignature sig,
31283132
for (auto param : params) {
31293133
if (!sig->isCanonicalTypeInContext(param.getPlainType()))
31303134
return false;
3135+
if (!param.getInternalLabel().empty()) {
3136+
// Canonical types don't have internal labels
3137+
return false;
3138+
}
31313139
}
31323140

31333141
return sig->isCanonicalTypeInContext(result);
@@ -3231,17 +3239,21 @@ void AnyFunctionType::relabelParams(MutableArrayRef<Param> params,
32313239
assert(params.size() == labels.size());
32323240
for (auto i : indices(params)) {
32333241
auto &param = params[i];
3234-
param = AnyFunctionType::Param(param.getPlainType(),
3235-
labels[i],
3236-
param.getParameterFlags());
3242+
param = AnyFunctionType::Param(param.getPlainType(), labels[i],
3243+
param.getParameterFlags(),
3244+
param.getInternalLabel());
32373245
}
32383246
}
32393247

3248+
/// Profile \p params into \p ID. In contrast to \c == on \c Param, the profile
3249+
/// *does* take the internal label into account and *does not* canonicalize
3250+
/// the param's type.
32403251
static void profileParams(llvm::FoldingSetNodeID &ID,
32413252
ArrayRef<AnyFunctionType::Param> params) {
32423253
ID.AddInteger(params.size());
32433254
for (auto param : params) {
32443255
ID.AddPointer(param.getLabel().get());
3256+
ID.AddPointer(param.getInternalLabel().get());
32453257
ID.AddPointer(param.getPlainType().getPointer());
32463258
ID.AddInteger(param.getParameterFlags().toRaw());
32473259
}
@@ -3293,7 +3305,7 @@ FunctionType *FunctionType::get(ArrayRef<AnyFunctionType::Param> params,
32933305
>(params.size(), hasClangInfo ? 1 : 0, globalActor ? 1 : 0);
32943306
void *mem = ctx.Allocate(allocSize, alignof(FunctionType), arena);
32953307

3296-
bool isCanonical = isFunctionTypeCanonical(params, result);
3308+
bool isCanonical = isAnyFunctionTypeCanonical(params, result);
32973309
if (!clangTypeInfo.empty()) {
32983310
if (ctx.LangOpts.UseClangFunctionTypes)
32993311
isCanonical &= clangTypeInfo.getType()->isCanonicalUnqualified();

‎lib/AST/ASTDumper.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3807,6 +3807,8 @@ namespace {
38073807
PrintWithColorRAII(OS, TypeFieldColor) << "param";
38083808
if (param.hasLabel())
38093809
printField("name", param.getLabel().str());
3810+
if (param.hasInternalLabel())
3811+
printField("internal_name", param.getInternalLabel().str());
38103812
dumpParameterFlags(param.getParameterFlags());
38113813
printRec(param.getPlainType());
38123814
OS << ")";

‎lib/AST/ASTPrinter.cpp

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4759,10 +4759,23 @@ class TypePrinter : public TypeVisitor<TypePrinter> {
47594759
Printer.printStructurePost(PrintStructureKind::FunctionParameter);
47604760
};
47614761

4762-
if (printLabels && Param.hasLabel()) {
4762+
if ((Options.AlwaysTryPrintParameterLabels || printLabels) &&
4763+
Param.hasLabel()) {
4764+
// Label printing was requested and we have an external label. Print it
4765+
// and omit the internal label.
47634766
Printer.printName(Param.getLabel(),
47644767
PrintNameContext::FunctionParameterExternal);
47654768
Printer << ": ";
4769+
} else if (Options.AlwaysTryPrintParameterLabels &&
4770+
Param.hasInternalLabel()) {
4771+
// We didn't have an external parameter label but were requested to
4772+
// always try and print parameter labels. Print The internal label.
4773+
// If we have neither an external nor an internal label, only print the
4774+
// type.
4775+
Printer << "_ ";
4776+
Printer.printName(Param.getInternalLabel(),
4777+
PrintNameContext::FunctionParameterLocal);
4778+
Printer << ": ";
47664779
}
47674780

47684781
auto type = Param.getPlainType();

‎lib/AST/Decl.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2598,7 +2598,8 @@ static Type mapSignatureFunctionType(ASTContext &ctx, Type type,
25982598
newFlags = newFlags.withInOut(false);
25992599
}
26002600

2601-
AnyFunctionType::Param newParam(newParamType, param.getLabel(), newFlags);
2601+
AnyFunctionType::Param newParam(newParamType, param.getLabel(), newFlags,
2602+
param.getInternalLabel());
26022603
newParams.push_back(newParam);
26032604
}
26042605

@@ -6290,11 +6291,12 @@ AnyFunctionType::Param ParamDecl::toFunctionParam(Type type) const {
62906291
type = ParamDecl::getVarargBaseTy(type);
62916292

62926293
auto label = getArgumentName();
6294+
auto internalLabel = getParameterName();
62936295
auto flags = ParameterTypeFlags::fromParameterType(
62946296
type, isVariadic(), isAutoClosure(), isNonEphemeral(),
62956297
getValueOwnership(),
62966298
/*isNoDerivative*/ false);
6297-
return AnyFunctionType::Param(type, label, flags);
6299+
return AnyFunctionType::Param(type, label, flags, internalLabel);
62986300
}
62996301

63006302
Optional<Initializer *> ParamDecl::getCachedDefaultArgumentInitContext() const {

‎lib/AST/Type.cpp

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -833,7 +833,7 @@ Type TypeBase::removeArgumentLabels(unsigned numArgumentLabels) {
833833
llvm::SmallVector<AnyFunctionType::Param, 8> unlabeledParams;
834834
unlabeledParams.reserve(fnType->getNumParams());
835835
for (const auto &param : fnType->getParams())
836-
unlabeledParams.push_back(param.getWithoutLabel());
836+
unlabeledParams.push_back(param.getWithoutLabels());
837837

838838
auto result = fnType->getResult()
839839
->removeArgumentLabels(numArgumentLabels - 1);
@@ -1014,9 +1014,7 @@ rebuildSelfTypeWithObjectType(AnyFunctionType::Param selfParam,
10141014
if (selfParam.getPlainType()->getAs<MetatypeType>())
10151015
objectTy = MetatypeType::get(objectTy);
10161016

1017-
return AnyFunctionType::Param(objectTy,
1018-
selfParam.getLabel(),
1019-
selfParam.getParameterFlags());
1017+
return selfParam.withType(objectTy);
10201018
}
10211019

10221020
/// Returns a new function type exactly like this one but with the self
@@ -1191,9 +1189,11 @@ getCanonicalParams(AnyFunctionType *funcType,
11911189
SmallVectorImpl<AnyFunctionType::Param> &canParams) {
11921190
auto origParams = funcType->getParams();
11931191
for (auto param : origParams) {
1192+
// Canonicalize the type and drop the internal label to canonicalize the
1193+
// Param.
11941194
canParams.emplace_back(param.getPlainType()->getCanonicalType(genericSig),
1195-
param.getLabel(),
1196-
param.getParameterFlags());
1195+
param.getLabel(), param.getParameterFlags(),
1196+
/*InternalLabel=*/Identifier());
11971197
}
11981198
}
11991199

@@ -4776,6 +4776,7 @@ case TypeKind::Id:
47764776
auto type = param.getPlainType();
47774777
auto label = param.getLabel();
47784778
auto flags = param.getParameterFlags();
4779+
auto internalLabel = param.getInternalLabel();
47794780

47804781
auto substType = type.transformRec(fn);
47814782
if (!substType)
@@ -4794,7 +4795,7 @@ case TypeKind::Id:
47944795
flags = flags.withInOut(true);
47954796
}
47964797

4797-
substParams.emplace_back(substType, label, flags);
4798+
substParams.emplace_back(substType, label, flags, internalLabel);
47984799
}
47994800

48004801
// Transform result type.

‎lib/IDE/CodeCompletion.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1001,6 +1001,7 @@ void CodeCompletionResultBuilder::addCallParameter(Identifier Name,
10011001
PO.SkipAttributes = true;
10021002
PO.OpaqueReturnTypePrinting =
10031003
PrintOptions::OpaqueReturnTypePrintingMode::WithoutOpaqueKeyword;
1004+
PO.AlwaysTryPrintParameterLabels = true;
10041005
if (ContextTy)
10051006
PO.setBaseType(ContextTy);
10061007

@@ -1017,6 +1018,8 @@ void CodeCompletionResultBuilder::addCallParameter(Identifier Name,
10171018

10181019
if (param.hasLabel()) {
10191020
OS << param.getLabel();
1021+
} else if (param.hasInternalLabel()) {
1022+
OS << param.getInternalLabel();
10201023
} else {
10211024
OS << "<#";
10221025
if (param.isInOut())
@@ -2337,8 +2340,7 @@ class CompletionLookup final : public swift::VisibleDeclConsumer {
23372340
SmallVector<AnyFunctionType::Param, 8> erasedParams;
23382341
for (const auto &param : genericFuncType->getParams()) {
23392342
auto erasedTy = eraseArchetypes(param.getPlainType(), genericSig);
2340-
erasedParams.emplace_back(erasedTy, param.getLabel(),
2341-
param.getParameterFlags());
2343+
erasedParams.emplace_back(param.withType(erasedTy));
23422344
}
23432345
return GenericFunctionType::get(genericSig,
23442346
erasedParams,

‎lib/SIL/IR/Bridging.cpp

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,7 @@ TypeConverter::getBridgedParam(SILFunctionTypeRepresentation rep,
5353
llvm::report_fatal_error("unable to set up the ObjC bridge!");
5454
}
5555

56-
return AnyFunctionType::Param(bridged->getCanonicalType(),
57-
param.getLabel(),
58-
param.getParameterFlags());
56+
return param.withType(bridged->getCanonicalType());
5957
}
6058

6159
void TypeConverter::

‎lib/Sema/CSApply.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1040,8 +1040,9 @@ namespace {
10401040

10411041
// SILGen knows how to emit property-wrapped parameters, but the
10421042
// function type needs the backing wrapper type in its param list.
1043-
innerParamTypes.push_back(AnyFunctionType::Param(paramRef->getType(),
1044-
innerParam->getArgumentName()));
1043+
innerParamTypes.push_back(AnyFunctionType::Param(
1044+
paramRef->getType(), innerParam->getArgumentName(),
1045+
ParameterTypeFlags(), innerParam->getParameterName()));
10451046
} else {
10461047
// Rewrite the parameter ref if necessary.
10471048
if (outerParam->isInOut()) {

‎lib/Sema/CSDiagnostics.cpp

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4471,7 +4471,15 @@ bool MissingArgumentsFailure::diagnoseClosure(const ClosureExpr *closure) {
44714471
fixText += " ";
44724472
interleave(
44734473
funcType->getParams(),
4474-
[&fixText](const AnyFunctionType::Param &param) { fixText += '_'; },
4474+
[&fixText](const AnyFunctionType::Param &param) {
4475+
if (param.hasLabel()) {
4476+
fixText += param.getLabel().str();
4477+
} else if (param.hasInternalLabel()) {
4478+
fixText += param.getInternalLabel().str();
4479+
} else {
4480+
fixText += '_';
4481+
}
4482+
},
44754483
[&fixText] { fixText += ','; });
44764484
fixText += " in ";
44774485
}

‎lib/Sema/CSGen.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2423,7 +2423,7 @@ namespace {
24232423
// Remove parameter labels; they aren't used when matching cases,
24242424
// but outright conflicts will be checked during coercion.
24252425
for (auto &param : params) {
2426-
param = param.getWithoutLabel();
2426+
param = param.getWithoutLabels();
24272427
}
24282428

24292429
Type outputType = CS.createTypeVariable(

‎lib/Sema/ConstraintSystem.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1214,7 +1214,9 @@ unwrapPropertyWrapperParameterTypes(ConstraintSystem &cs, AbstractFunctionDecl *
12141214
auto *wrappedType = cs.createTypeVariable(cs.getConstraintLocator(locator), 0);
12151215
auto paramType = paramTypes[i].getParameterType();
12161216
auto paramLabel = paramTypes[i].getLabel();
1217-
adjustedParamTypes.push_back(AnyFunctionType::Param(wrappedType, paramLabel));
1217+
auto paramInternalLabel = paramTypes[i].getInternalLabel();
1218+
adjustedParamTypes.push_back(AnyFunctionType::Param(
1219+
wrappedType, paramLabel, ParameterTypeFlags(), paramInternalLabel));
12181220
cs.applyPropertyWrapperToParameter(paramType, wrappedType, paramDecl, argLabel,
12191221
ConstraintKind::Equal, locator);
12201222
}

‎lib/Sema/TypeCheckType.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2699,7 +2699,8 @@ TypeResolver::resolveASTFunctionTypeParams(TupleTypeRepr *inputRepr,
26992699
auto paramFlags = ParameterTypeFlags::fromParameterType(
27002700
ty, variadic, autoclosure, /*isNonEphemeral*/ false, ownership,
27012701
noDerivative);
2702-
elements.emplace_back(ty, Identifier(), paramFlags);
2702+
elements.emplace_back(ty, Identifier(), paramFlags,
2703+
inputRepr->getElementName(i));
27032704
}
27042705

27052706
// All non-`@noDerivative` parameters of `@differentiable` function types must

0 commit comments

Comments
 (0)