Skip to content

Commit f39a889

Browse files
DougGregorDougGregor
committed
[Type checker] Improve diagnostics for trailing closures.
The change to the forward-scanning rule regressed some diagnostics, because we no longer generated the special "trailing closure mismatch" diagnostic. Reinstate the special-case "trailing closure mismatch" diagnostic, but this time do so as part of the normal argument mismatch diagnostics so it is based on type information. While here, clean up the handling of missing-argument diagnostics to deal with (multiple) trailing closures properly, so that we can (e.g) suggest adding a new labeled trailing closure at the end, rather than producing nonsensical Fix-Its. And, note that SR-12291 is broken (again) by the forward-scan matching rules.
1 parent 3243c52 commit f39a889

File tree

10 files changed

+156
-93
lines changed

10 files changed

+156
-93
lines changed

‎lib/Sema/CSDiagnostics.cpp

Lines changed: 100 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -3943,9 +3943,9 @@ bool MissingArgumentsFailure::diagnoseAsError() {
39433943
Expr *fnExpr = nullptr;
39443944
Expr *argExpr = nullptr;
39453945
unsigned numArguments = 0;
3946-
bool hasTrailingClosure = false;
3946+
Optional<unsigned> firstTrailingClosure = None;
39473947

3948-
std::tie(fnExpr, argExpr, numArguments, hasTrailingClosure) =
3948+
std::tie(fnExpr, argExpr, numArguments, firstTrailingClosure) =
39493949
getCallInfo(getRawAnchor());
39503950

39513951
// TODO(diagnostics): We should be able to suggest this fix-it
@@ -4008,46 +4008,66 @@ bool MissingArgumentsFailure::diagnoseSingleMissingArgument() const {
40084008
auto position = argument.first;
40094009
auto label = argument.second.getLabel();
40104010

4011-
SmallString<32> insertBuf;
4012-
llvm::raw_svector_ostream insertText(insertBuf);
4013-
4014-
if (position != 0)
4015-
insertText << ", ";
4016-
4017-
forFixIt(insertText, argument.second);
4018-
40194011
Expr *fnExpr = nullptr;
40204012
Expr *argExpr = nullptr;
4021-
unsigned insertableEndIdx = 0;
4022-
bool hasTrailingClosure = false;
4013+
unsigned numArgs = 0;
4014+
Optional<unsigned> firstTrailingClosure = None;
40234015

4024-
std::tie(fnExpr, argExpr, insertableEndIdx, hasTrailingClosure) =
4016+
std::tie(fnExpr, argExpr, numArgs, firstTrailingClosure) =
40254017
getCallInfo(anchor);
40264018

4027-
if (!argExpr)
4019+
if (!argExpr) {
40284020
return false;
4021+
}
4022+
4023+
// Will the parameter accept a trailing closure?
4024+
Type paramType = resolveType(argument.second.getPlainType());
4025+
bool paramAcceptsTrailingClosure = paramType
4026+
->lookThroughAllOptionalTypes()->is<AnyFunctionType>();
40294027

4030-
if (hasTrailingClosure)
4031-
insertableEndIdx -= 1;
4028+
// Determine whether we're inserting as a trailing closure.
4029+
bool insertingTrailingClosure =
4030+
firstTrailingClosure && position > *firstTrailingClosure;
4031+
4032+
SmallString<32> insertBuf;
4033+
llvm::raw_svector_ostream insertText(insertBuf);
40324034

4033-
if (position == 0 && insertableEndIdx != 0)
4035+
if (insertingTrailingClosure)
4036+
insertText << " ";
4037+
else if (position != 0)
4038+
insertText << ", ";
4039+
4040+
forFixIt(insertText, argument.second);
4041+
4042+
if (position == 0 && numArgs > 0 &&
4043+
(!firstTrailingClosure || position < *firstTrailingClosure))
40344044
insertText << ", ";
40354045

40364046
SourceLoc insertLoc;
4037-
if (auto *TE = dyn_cast<TupleExpr>(argExpr)) {
4038-
// fn():
4039-
// fn([argMissing])
4047+
4048+
if (position >= numArgs && insertingTrailingClosure) {
4049+
// Add a trailing closure to the end.
4050+
4051+
// fn { closure }:
4052+
// fn {closure} label: [argMissing]
4053+
// fn() { closure }:
4054+
// fn() {closure} label: [argMissing]
4055+
// fn(argX) { closure }:
4056+
// fn(argX) { closure } label: [argMissing]
4057+
insertLoc = Lexer::getLocForEndOfToken(
4058+
ctx.SourceMgr, argExpr->getEndLoc());
4059+
} else if (auto *TE = dyn_cast<TupleExpr>(argExpr)) {
40404060
// fn(argX, argY):
40414061
// fn([argMissing, ]argX, argY)
40424062
// fn(argX[, argMissing], argY)
4043-
// fn(argX, argY[, argMissing])
40444063
// fn(argX) { closure }:
40454064
// fn([argMissing, ]argX) { closure }
40464065
// fn(argX[, argMissing]) { closure }
4047-
// fn(argX[, closureLabel: ]{closure}[, argMissing)] // Not impl.
4048-
if (insertableEndIdx == 0)
4066+
// fn(argX, argY):
4067+
// fn(argX, argY[, argMissing])
4068+
if (numArgs == 0) {
40494069
insertLoc = TE->getRParenLoc();
4050-
else if (position != 0) {
4070+
} else if (position != 0) {
40514071
auto argPos = std::min(TE->getNumElements(), position) - 1;
40524072
insertLoc = Lexer::getLocForEndOfToken(
40534073
ctx.SourceMgr, TE->getElement(argPos)->getEndLoc());
@@ -4059,25 +4079,25 @@ bool MissingArgumentsFailure::diagnoseSingleMissingArgument() const {
40594079
} else {
40604080
auto *PE = cast<ParenExpr>(argExpr);
40614081
if (PE->getRParenLoc().isValid()) {
4082+
// fn():
4083+
// fn([argMissing])
40624084
// fn(argX):
4063-
// fn([argMissing, ]argX)
40644085
// fn(argX[, argMissing])
4086+
// fn([argMissing, ]argX)
40654087
// fn() { closure }:
40664088
// fn([argMissing]) {closure}
4067-
// fn([closureLabel: ]{closure}[, argMissing]) // Not impl.
4068-
if (insertableEndIdx == 0)
4069-
insertLoc = PE->getRParenLoc();
4070-
else if (position == 0)
4071-
insertLoc = PE->getSubExpr()->getStartLoc();
4072-
else
4089+
if (position == 0) {
40734090
insertLoc = Lexer::getLocForEndOfToken(ctx.SourceMgr,
4074-
PE->getSubExpr()->getEndLoc());
4091+
PE->getLParenLoc());
4092+
} else {
4093+
insertLoc = Lexer::getLocForEndOfToken(
4094+
ctx.SourceMgr, PE->getSubExpr()->getEndLoc());
4095+
}
40754096
} else {
40764097
// fn { closure }:
40774098
// fn[(argMissing)] { closure }
4078-
// fn[(closureLabel:] { closure }[, missingArg)] // Not impl.
40794099
assert(!isExpr<SubscriptExpr>(anchor) && "bracket less subscript");
4080-
assert(PE->hasTrailingClosure() &&
4100+
assert(firstTrailingClosure &&
40814101
"paren less ParenExpr without trailing closure");
40824102
insertBuf.insert(insertBuf.begin(), '(');
40834103
insertBuf.insert(insertBuf.end(), ')');
@@ -4089,16 +4109,28 @@ bool MissingArgumentsFailure::diagnoseSingleMissingArgument() const {
40894109
if (insertLoc.isInvalid())
40904110
return false;
40914111

4112+
// If we are trying to insert a trailing closure but the parameter
4113+
// corresponding to the missing argument doesn't support a trailing closure,
4114+
// don't provide a Fix-It.
4115+
// FIXME: It's possible to parenthesize and relabel the argument list to
4116+
// accomodate this, but it's tricky.
4117+
bool shouldEmitFixIt =
4118+
!(insertingTrailingClosure && !paramAcceptsTrailingClosure);
4119+
40924120
if (label.empty()) {
4093-
emitDiagnosticAt(insertLoc, diag::missing_argument_positional, position + 1)
4094-
.fixItInsert(insertLoc, insertText.str());
4121+
auto diag = emitDiagnosticAt(
4122+
insertLoc, diag::missing_argument_positional, position + 1);
4123+
if (shouldEmitFixIt)
4124+
diag.fixItInsert(insertLoc, insertText.str());
40954125
} else if (isPropertyWrapperInitialization()) {
40964126
auto *TE = cast<TypeExpr>(fnExpr);
40974127
emitDiagnosticAt(TE->getLoc(), diag::property_wrapper_missing_arg_init,
40984128
label, resolveType(TE->getInstanceType())->getString());
40994129
} else {
4100-
emitDiagnosticAt(insertLoc, diag::missing_argument_named, label)
4101-
.fixItInsert(insertLoc, insertText.str());
4130+
auto diag = emitDiagnosticAt(
4131+
insertLoc, diag::missing_argument_named, label);
4132+
if (shouldEmitFixIt)
4133+
diag.fixItInsert(insertLoc, insertText.str());
41024134
}
41034135

41044136
if (auto selectedOverload =
@@ -4327,23 +4359,24 @@ bool MissingArgumentsFailure::isMisplacedMissingArgument(
43274359
return TypeChecker::isConvertibleTo(argType, paramType, cs.DC);
43284360
}
43294361

4330-
std::tuple<Expr *, Expr *, unsigned, bool>
4362+
std::tuple<Expr *, Expr *, unsigned, Optional<unsigned>>
43314363
MissingArgumentsFailure::getCallInfo(TypedNode anchor) const {
43324364
if (auto *call = getAsExpr<CallExpr>(anchor)) {
43334365
return std::make_tuple(call->getFn(), call->getArg(),
4334-
call->getNumArguments(), call->hasTrailingClosure());
4366+
call->getNumArguments(),
4367+
call->getUnlabeledTrailingClosureIndex());
43354368
} else if (auto *UME = getAsExpr<UnresolvedMemberExpr>(anchor)) {
43364369
return std::make_tuple(UME, UME->getArgument(), UME->getNumArguments(),
4337-
UME->hasTrailingClosure());
4370+
UME->getUnlabeledTrailingClosureIndex());
43384371
} else if (auto *SE = getAsExpr<SubscriptExpr>(anchor)) {
43394372
return std::make_tuple(SE, SE->getIndex(), SE->getNumArguments(),
4340-
SE->hasTrailingClosure());
4373+
SE->getUnlabeledTrailingClosureIndex());
43414374
} else if (auto *OLE = getAsExpr<ObjectLiteralExpr>(anchor)) {
43424375
return std::make_tuple(OLE, OLE->getArg(), OLE->getNumArguments(),
4343-
OLE->hasTrailingClosure());
4376+
OLE->getUnlabeledTrailingClosureIndex());
43444377
}
43454378

4346-
return std::make_tuple(nullptr, nullptr, 0, false);
4379+
return std::make_tuple(nullptr, nullptr, 0, None);
43474380
}
43484381

43494382
void MissingArgumentsFailure::forFixIt(
@@ -5527,6 +5560,9 @@ bool ArgumentMismatchFailure::diagnoseAsError() {
55275560
if (diagnosePropertyWrapperMismatch())
55285561
return true;
55295562

5563+
if (diagnoseTrailingClosureMismatch())
5564+
return true;
5565+
55305566
auto argType = getFromType();
55315567
auto paramType = getToType();
55325568

@@ -5759,6 +5795,26 @@ bool ArgumentMismatchFailure::diagnosePropertyWrapperMismatch() const {
57595795
return true;
57605796
}
57615797

5798+
bool ArgumentMismatchFailure::diagnoseTrailingClosureMismatch() const {
5799+
if (!Info.isTrailingClosure())
5800+
return false;
5801+
5802+
auto paramType = getToType();
5803+
if (paramType->lookThroughAllOptionalTypes()->is<AnyFunctionType>())
5804+
return false;
5805+
5806+
emitDiagnostic(diag::trailing_closure_bad_param, paramType)
5807+
.highlight(getSourceRange());
5808+
5809+
if (auto overload = getCalleeOverloadChoiceIfAvailable(getLocator())) {
5810+
if (auto *decl = overload->choice.getDeclOrNull()) {
5811+
emitDiagnosticAt(decl, diag::decl_declared_here, decl->getName());
5812+
}
5813+
}
5814+
5815+
return true;
5816+
}
5817+
57625818
void ExpandArrayIntoVarargsFailure::tryDropArrayBracketsFixIt(
57635819
const Expr *anchor) const {
57645820
// If this is an array literal, offer to remove the brackets and pass the

‎lib/Sema/CSDiagnostics.h

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1283,10 +1283,10 @@ class MissingArgumentsFailure final : public FailureDiagnostic {
12831283
bool isPropertyWrapperInitialization() const;
12841284

12851285
/// Gather information associated with expression that represents
1286-
/// a call - function, arguments, # of arguments and whether it has
1287-
/// a trailing closure.
1288-
std::tuple<Expr *, Expr *, unsigned, bool>
1289-
getCallInfo(TypedNode anchor) const;
1286+
/// a call - function, arguments, # of arguments and the position of
1287+
/// the first trailing closure.
1288+
std::tuple<Expr *, Expr *, unsigned, Optional<unsigned>>
1289+
getCallInfo(TypedNode anchor) const;
12901290

12911291
/// Transform given argument into format suitable for a fix-it
12921292
/// text e.g. `[<label>:]? <#<type#>`
@@ -1806,6 +1806,10 @@ class ArgumentMismatchFailure : public ContextualFailure {
18061806
/// or now deprecated `init(initialValue:)`.
18071807
bool diagnosePropertyWrapperMismatch() const;
18081808

1809+
/// Tailored diagnostics for argument mismatches associated with trailing
1810+
/// closures being passed to non-closure parameters.
1811+
bool diagnoseTrailingClosureMismatch() const;
1812+
18091813
protected:
18101814
/// \returns The position of the argument being diagnosed, starting at 1.
18111815
unsigned getArgPosition() const { return Info.getArgPosition(); }

‎lib/Sema/CSSimplify.cpp

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -420,7 +420,8 @@ matchCallArguments(SmallVectorImpl<AnyFunctionType::Param> &args,
420420

421421
// If the parameter we are looking at does not support the (unlabeled)
422422
// trailing closure argument, this parameter is unfulfilled.
423-
if (!paramInfo.acceptsUnlabeledTrailingClosureArgument(paramIdx)) {
423+
if (!paramInfo.acceptsUnlabeledTrailingClosureArgument(paramIdx) &&
424+
!ignoreNameMismatch) {
424425
haveUnfulfilledParams = true;
425426
return;
426427
}
@@ -738,7 +739,10 @@ matchCallArguments(SmallVectorImpl<AnyFunctionType::Param> &args,
738739
// one argument requires label and another one doesn't, but caller
739740
// doesn't provide either, problem is going to be identified as
740741
// out-of-order argument instead of label mismatch.
741-
const auto expectedLabel = params[paramIdx].getLabel();
742+
const auto expectedLabel =
743+
fromArgIdx == unlabeledTrailingClosureArgIndex
744+
? Identifier()
745+
: params[paramIdx].getLabel();
742746
const auto argumentLabel = args[fromArgIdx].getLabel();
743747

744748
if (argumentLabel != expectedLabel) {

‎lib/Sema/ConstraintSystem.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -574,6 +574,15 @@ class FunctionArgApplyInfo {
574574
return StringRef(scratch.data(), scratch.size());
575575
}
576576

577+
/// Whether the argument is a trailing closure.
578+
bool isTrailingClosure() const {
579+
if (auto trailingClosureArg =
580+
ArgListExpr->getUnlabeledTrailingClosureIndexOfPackedArgument())
581+
return ArgIdx >= *trailingClosureArg;
582+
583+
return false;
584+
}
585+
577586
/// \returns The interface type for the function being applied. Note that this
578587
/// may not a function type, for example it could be a generic parameter.
579588
Type getFnInterfaceType() const { return FnInterfaceType; }

‎test/Constraints/closures.swift

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1006,11 +1006,13 @@ func rdar52204414() {
10061006
// expected-error@-1 {{declared closure result 'Int' is incompatible with contextual type 'Void'}}
10071007
}
10081008

1009-
// SR-12291 - trailing closure is used as an argument to the last (positionally) parameter
1010-
func overloaded_with_default(a: () -> Int, b: Int = 0, c: Int = 0) {}
1011-
func overloaded_with_default(b: Int = 0, c: Int = 0, a: () -> Int) {}
1009+
// SR-12291 - trailing closure is used as an argument to the last (positionally) parameter.
1010+
// Note that this was accepted prior to Swift 5.3. SE-0286 changed the
1011+
// order of argument resolution and made it ambiguous.
1012+
func overloaded_with_default(a: () -> Int, b: Int = 0, c: Int = 0) {} // expected-note{{found this candidate}}
1013+
func overloaded_with_default(b: Int = 0, c: Int = 0, a: () -> Int) {} // expected-note{{found this candidate}}
10121014

1013-
overloaded_with_default { 0 } // Ok (could be ambiguous if trailing was allowed to match `a:` in first overload)
1015+
overloaded_with_default { 0 } // expected-error{{ambiguous use of 'overloaded_with_default'}}
10141016

10151017
func overloaded_with_default_and_autoclosure<T>(_ a: @autoclosure () -> T, b: Int = 0) {}
10161018
func overloaded_with_default_and_autoclosure<T>(b: Int = 0, c: @escaping () -> T?) {}

‎test/Constraints/diag_missing_arg.swift

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -35,11 +35,9 @@ trailingClosureSingle1() { 1 } // expected-error {{missing argument for paramete
3535

3636
func trailingClosureSingle2(x: () -> Int, y: Int) {} // expected-note * {{here}}
3737
trailingClosureSingle2 { 1 }
38-
// expected-error@-1 {{missing argument for parameter 'x' in call}} {{23-23=(x: <#() -> Int#>)}}
39-
// expected-error@-2 {{trailing closure passed to parameter of type 'Int' that does not accept a closure}}
38+
// expected-error@-1 {{missing argument for parameter 'y' in call}} {{none}}
4039
trailingClosureSingle2() { 1 }
41-
// expected-error@-1 {{missing argument for parameter 'x' in call}} {{24-24=x: <#() -> Int#>}}
42-
// expected-error@-2 {{trailing closure passed to parameter of type 'Int' that does not accept a closure}}
40+
// expected-error@-1 {{missing argument for parameter 'y' in call}} {{none}}
4341

4442
func trailingClosureMulti1(x: Int, y: Int, z: () -> Int) {} // expected-note * {{here}}
4543
trailingClosureMulti1(y: 1) { 1 } // expected-error {{missing argument for parameter 'x' in call}} {{23-23=x: <#Int#>, }}
@@ -48,14 +46,17 @@ trailingClosureMulti1(x: 1, y: 1) // expected-error {{missing argument for param
4846

4947
func trailingClosureMulti2(x: Int, y: () -> Int, z: Int) {} // expected-note * {{here}}
5048
trailingClosureMulti2 { 1 }
51-
// expected-error@-1 {{missing arguments for parameters 'x', 'y' in call}}
52-
// expected-error@-2 {{trailing closure passed to parameter of type 'Int' that does not accept a closure}}
49+
// expected-error@-1 {{missing arguments for parameters 'x', 'z' in call}}
5350
trailingClosureMulti2() { 1 }
54-
// expected-error@-1 {{missing arguments for parameters 'x', 'y' in call}}
55-
// expected-error@-2 {{trailing closure passed to parameter of type 'Int' that does not accept a closure}}
51+
// expected-error@-1 {{missing arguments for parameters 'x', 'z' in call}}
5652
trailingClosureMulti2(x: 1) { 1 }
57-
// expected-error@-1 {{missing argument for parameter 'y' in call}} {{27-27=, y: <#() -> Int#>}}
58-
// expected-error@-2 {{trailing closure passed to parameter of type 'Int' that does not accept a closure}}
53+
// expected-error@-1 {{missing argument for parameter 'z' in call}} {{none}}
54+
55+
func trailingClosureMulti3(x: (Int) -> Int, y: (Int) -> Int, z: (Int) -> Int) {} // expected-note 2 {{declared here}}
56+
trailingClosureMulti3 { $0 } y: { $0 } // expected-error{{missing argument for parameter 'z' in call}}{{39-39= z: <#(Int) -> Int#>}}
57+
58+
trailingClosureMulti3 { $0 } z: { $0 } // expected-error{{missing argument for parameter 'y' in call}}{{29-29= y: <#(Int) -> Int#>}}
59+
5960

6061
func param2Func(x: Int, y: Int) {} // expected-note * {{here}}
6162
param2Func(x: 1) // expected-error {{missing argument for parameter 'y' in call}} {{16-16=, y: <#Int#>}}

‎test/Constraints/overload_filtering.swift

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -38,14 +38,3 @@ func testUnresolvedMember(i: Int) -> X {
3838
// CHECK-NEXT: introducing single enabled disjunction term {{.*}} bound to decl overload_filtering.(file).X.init(_:_:)
3939
return .init(i, i)
4040
}
41-
42-
func trailing(x: Int = 0, y: () -> Void) { }
43-
func trailing(x: Int = 0, z: Float) { }
44-
45-
func testTrailing() {
46-
// CHECK: disabled disjunction term {{.*}} bound to decl overload_filtering.(file).trailing(x:z:)
47-
trailing() { }
48-
49-
// CHECK: disabled disjunction term {{.*}} bound to decl overload_filtering.(file).trailing(x:z:)
50-
trailing(x: 5) { }
51-
}

0 commit comments

Comments
 (0)