Skip to content

Commit 5dd5d82

Browse files
tkremenektkremenek
authored
Merge pull request #4809 from jrose-apple/swift-3-fix-it-trailing-closure-ambiguity
Offer fix-its to disambiguate based on a trailing closure's label
2 parents a2e3e7c + 49d52c4 commit 5dd5d82

File tree

4 files changed

+186
-5
lines changed

4 files changed

+186
-5
lines changed

‎include/swift/AST/DiagnosticsSema.def

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2225,6 +2225,10 @@ ERROR(ambiguous_decl_ref,none,
22252225
"ambiguous use of %0", (DeclName))
22262226
ERROR(ambiguous_operator_ref,none,
22272227
"ambiguous use of operator %0", (DeclName))
2228+
NOTE(ambiguous_because_of_trailing_closure,none,
2229+
"%select{use an explicit argument label instead of a trailing closure|"
2230+
"avoid using a trailing closure}0 to call %1",
2231+
(bool, DeclName))
22282232

22292233
// Cannot capture inout-ness of a parameter
22302234
// Partial application of foreign functions not supported

‎lib/Sema/CSDiag.cpp

Lines changed: 109 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -312,12 +312,114 @@ static unsigned countDistinctOverloads(ArrayRef<OverloadChoice> choices) {
312312

313313
/// \brief Determine the name of the overload in a set of overload choices.
314314
static DeclName getOverloadChoiceName(ArrayRef<OverloadChoice> choices) {
315+
DeclName name;
315316
for (auto choice : choices) {
316-
if (choice.isDecl())
317-
return choice.getDecl()->getFullName();
317+
if (!choice.isDecl())
318+
continue;
319+
320+
DeclName nextName = choice.getDecl()->getFullName();
321+
if (!name) {
322+
name = nextName;
323+
continue;
324+
}
325+
326+
if (name != nextName) {
327+
// Assume all choices have the same base name and only differ in
328+
// argument labels. This may not be a great assumption, but we don't
329+
// really have a way to recover for diagnostics otherwise.
330+
return name.getBaseName();
331+
}
318332
}
319333

320-
return DeclName();
334+
return name;
335+
}
336+
337+
/// Returns true if any diagnostics were emitted.
338+
static bool
339+
tryDiagnoseTrailingClosureAmbiguity(TypeChecker &tc, const Expr *expr,
340+
ArrayRef<OverloadChoice> choices) {
341+
auto *callExpr = dyn_cast<CallExpr>(expr);
342+
if (!callExpr)
343+
return false;
344+
if (!callExpr->hasTrailingClosure())
345+
return false;
346+
347+
llvm::SmallMapVector<Identifier, const ValueDecl *, 8> choicesByLabel;
348+
for (const OverloadChoice &choice : choices) {
349+
auto *callee = dyn_cast<AbstractFunctionDecl>(choice.getDecl());
350+
if (!callee)
351+
return false;
352+
353+
const ParameterList *paramList = callee->getParameterLists().back();
354+
const ParamDecl *param = paramList->getArray().back();
355+
356+
// Sanity-check that the trailing closure corresponds to this parameter.
357+
if (!param->getType()->is<AnyFunctionType>())
358+
return false;
359+
360+
Identifier trailingClosureLabel = param->getArgumentName();
361+
auto &choiceForLabel = choicesByLabel[trailingClosureLabel];
362+
363+
// FIXME: Cargo-culted from diagnoseAmbiguity: apparently the same decl can
364+
// appear more than once?
365+
if (choiceForLabel == callee)
366+
continue;
367+
368+
// If just providing the trailing closure label won't solve the ambiguity,
369+
// don't bother offering the fix-it.
370+
if (choiceForLabel != nullptr)
371+
return false;
372+
373+
choiceForLabel = callee;
374+
}
375+
376+
// If we got here, then all of the choices have unique labels. Offer them in
377+
// order.
378+
SourceManager &sourceMgr = tc.Context.SourceMgr;
379+
SourceLoc replaceStartLoc;
380+
SourceLoc closureStartLoc;
381+
bool hasOtherArgs, needsParens;
382+
if (auto tupleArgs = dyn_cast<TupleExpr>(callExpr->getArg())) {
383+
assert(tupleArgs->getNumElements() >= 2);
384+
const Expr *lastBeforeClosure =
385+
tupleArgs->getElements().drop_back(1).back();
386+
replaceStartLoc =
387+
Lexer::getLocForEndOfToken(sourceMgr, lastBeforeClosure->getEndLoc());
388+
closureStartLoc = tupleArgs->getElements().back()->getStartLoc();
389+
hasOtherArgs = true;
390+
needsParens = false;
391+
} else {
392+
auto parenArgs = cast<ParenExpr>(callExpr->getArg());
393+
replaceStartLoc = parenArgs->getRParenLoc();
394+
closureStartLoc = parenArgs->getSubExpr()->getStartLoc();
395+
hasOtherArgs = false;
396+
needsParens = parenArgs->getRParenLoc().isInvalid();
397+
}
398+
if (!replaceStartLoc.isValid()) {
399+
assert(callExpr->getFn()->getEndLoc().isValid());
400+
replaceStartLoc =
401+
Lexer::getLocForEndOfToken(sourceMgr, callExpr->getFn()->getEndLoc());
402+
}
403+
404+
for (const auto &choicePair : choicesByLabel) {
405+
SmallString<64> insertionString;
406+
if (needsParens)
407+
insertionString += "(";
408+
else if (hasOtherArgs)
409+
insertionString += ", ";
410+
411+
if (!choicePair.first.empty()) {
412+
insertionString += choicePair.first.str();
413+
insertionString += ": ";
414+
}
415+
416+
tc.diagnose(expr->getLoc(), diag::ambiguous_because_of_trailing_closure,
417+
choicePair.first.empty(), choicePair.second->getFullName())
418+
.fixItReplaceChars(replaceStartLoc, closureStartLoc, insertionString)
419+
.fixItInsertAfter(callExpr->getEndLoc(), ")");
420+
}
421+
422+
return true;
321423
}
322424

323425
static bool diagnoseAmbiguity(ConstraintSystem &cs,
@@ -410,9 +512,12 @@ static bool diagnoseAmbiguity(ConstraintSystem &cs,
410512
: diag::ambiguous_decl_ref,
411513
name);
412514

515+
if (tryDiagnoseTrailingClosureAmbiguity(tc, expr, overload.choices))
516+
return true;
517+
413518
// Emit candidates. Use a SmallPtrSet to make sure only emit a particular
414519
// candidate once. FIXME: Why is one candidate getting into the overload
415-
// set multiple times?
520+
// set multiple times? (See also tryDiagnoseTrailingClosureAmbiguity.)
416521
SmallPtrSet<Decl*, 8> EmittedDecls;
417522
for (auto choice : overload.choices) {
418523
switch (choice.getKind()) {

‎test/expr/closure/trailing.swift

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,3 +180,71 @@ func r23036383(foo: Foo23036383?, obj: Foo23036383) {
180180
// expected-error@-1 {{trailing closure requires parentheses for disambiguation in this context}} {{22-23=(}} {{28-28=)}}
181181
// expected-error@-2 {{trailing closure requires parentheses for disambiguation in this context}} {{37-38=(x: }} {{43-43=)}}
182182
}
183+
184+
func overloadOnLabel(a: () -> Void) {}
185+
func overloadOnLabel(b: () -> Void) {}
186+
func overloadOnLabel(c: () -> Void) {}
187+
188+
func overloadOnLabel2(a: () -> Void) {}
189+
func overloadOnLabel2(_: () -> Void) {}
190+
191+
func overloadOnLabelArgs(_: Int, a: () -> Void) {}
192+
func overloadOnLabelArgs(_: Int, b: () -> Void) {}
193+
194+
func overloadOnLabelArgs2(_: Int, a: () -> Void) {}
195+
func overloadOnLabelArgs2(_: Int, _: () -> Void) {}
196+
197+
func overloadOnLabelDefaultArgs(x: Int = 0, a: () -> Void) {}
198+
func overloadOnLabelDefaultArgs(x: Int = 1, b: () -> Void) {}
199+
200+
func overloadOnLabelSomeDefaultArgs(_: Int, x: Int = 0, a: () -> Void) {}
201+
func overloadOnLabelSomeDefaultArgs(_: Int, x: Int = 1, b: () -> Void) {}
202+
203+
func overloadOnDefaultArgsOnly(x: Int = 0, a: () -> Void) {} // expected-note 2 {{found this candidate}}
204+
func overloadOnDefaultArgsOnly(y: Int = 1, a: () -> Void) {} // expected-note 2 {{found this candidate}}
205+
206+
func overloadOnDefaultArgsOnly2(x: Int = 0, a: () -> Void) {} // expected-note 2 {{found this candidate}}
207+
func overloadOnDefaultArgsOnly2(y: Bool = true, a: () -> Void) {} // expected-note 2 {{found this candidate}}
208+
209+
func overloadOnDefaultArgsOnly3(x: Int = 0, a: () -> Void) {} // expected-note 2 {{found this candidate}}
210+
func overloadOnDefaultArgsOnly3(x: Bool = true, a: () -> Void) {} // expected-note 2 {{found this candidate}}
211+
212+
func overloadOnSomeDefaultArgsOnly(_: Int, x: Int = 0, a: () -> Void) {} // expected-note {{found this candidate}}
213+
func overloadOnSomeDefaultArgsOnly(_: Int, y: Int = 1, a: () -> Void) {} // expected-note {{found this candidate}}
214+
215+
func overloadOnSomeDefaultArgsOnly2(_: Int, x: Int = 0, a: () -> Void) {} // expected-note {{found this candidate}}
216+
func overloadOnSomeDefaultArgsOnly2(_: Int, y: Bool = true, a: () -> Void) {} // expected-note {{found this candidate}}
217+
218+
func overloadOnSomeDefaultArgsOnly3(_: Int, x: Int = 0, a: () -> Void) {} // expected-note {{found this candidate}}
219+
func overloadOnSomeDefaultArgsOnly3(_: Int, x: Bool = true, a: () -> Void) {} // expected-note {{found this candidate}}
220+
221+
func testOverloadAmbiguity() {
222+
overloadOnLabel {} // expected-error {{ambiguous use of 'overloadOnLabel'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabel(a:)'}} {{18-19=(a: }} {{21-21=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabel(b:)'}} {{18-19=(b: }} {{21-21=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabel(c:)'}} {{18-19=(c: }} {{21-21=)}}
223+
overloadOnLabel() {} // expected-error {{ambiguous use of 'overloadOnLabel'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabel(a:)'}} {{19-21=a: }} {{23-23=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabel(b:)'}} {{19-21=b: }} {{23-23=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabel(c:)'}} {{19-21=c: }} {{23-23=)}}
224+
overloadOnLabel2 {} // expected-error {{ambiguous use of 'overloadOnLabel2'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabel2(a:)'}} {{19-20=(a: }} {{22-22=)}} expected-note {{avoid using a trailing closure to call 'overloadOnLabel2'}} {{19-20=(}} {{22-22=)}}
225+
overloadOnLabel2() {} // expected-error {{ambiguous use of 'overloadOnLabel2'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabel2(a:)'}} {{20-22=a: }} {{24-24=)}} expected-note {{avoid using a trailing closure to call 'overloadOnLabel2'}} {{20-22=}} {{24-24=)}}
226+
overloadOnLabelArgs(1) {} // expected-error {{ambiguous use of 'overloadOnLabelArgs'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelArgs(_:a:)'}} {{24-26=, a: }} {{28-28=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelArgs(_:b:)'}} {{24-26=, b: }} {{28-28=)}}
227+
overloadOnLabelArgs2(1) {} // expected-error {{ambiguous use of 'overloadOnLabelArgs2'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelArgs2(_:a:)'}} {{25-27=, a: }} {{29-29=)}} expected-note {{avoid using a trailing closure to call 'overloadOnLabelArgs2'}} {{25-27=, }} {{29-29=)}}
228+
overloadOnLabelDefaultArgs {} // expected-error {{ambiguous use of 'overloadOnLabelDefaultArgs'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelDefaultArgs(x:a:)'}} {{29-30=(a: }} {{32-32=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelDefaultArgs(x:b:)'}} {{29-30=(b: }} {{32-32=)}}
229+
overloadOnLabelDefaultArgs() {} // expected-error {{ambiguous use of 'overloadOnLabelDefaultArgs'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelDefaultArgs(x:a:)'}} {{30-32=a: }} {{34-34=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelDefaultArgs(x:b:)'}} {{30-32=b: }} {{34-34=)}}
230+
overloadOnLabelDefaultArgs(x: 2) {} // expected-error {{ambiguous use of 'overloadOnLabelDefaultArgs'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelDefaultArgs(x:a:)'}} {{34-36=, a: }} {{38-38=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelDefaultArgs(x:b:)'}} {{34-36=, b: }} {{38-38=)}}
231+
overloadOnLabelSomeDefaultArgs(1) {} // expected-error {{ambiguous use of 'overloadOnLabelSomeDefaultArgs'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelSomeDefaultArgs(_:x:a:)'}} {{35-37=, a: }} {{39-39=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelSomeDefaultArgs(_:x:b:)'}} {{35-37=, b: }} {{39-39=)}}
232+
overloadOnLabelSomeDefaultArgs(1, x: 2) {} // expected-error {{ambiguous use of 'overloadOnLabelSomeDefaultArgs'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelSomeDefaultArgs(_:x:a:)'}} {{41-43=, a: }} {{45-45=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelSomeDefaultArgs(_:x:b:)'}} {{41-43=, b: }} {{45-45=)}}
233+
234+
overloadOnLabelSomeDefaultArgs( // expected-error {{ambiguous use of 'overloadOnLabelSomeDefaultArgs'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelSomeDefaultArgs(_:x:a:)'}} {{12-5=, a: }} {{4-4=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelSomeDefaultArgs(_:x:b:)'}} {{12-5=, b: }} {{4-4=)}}
235+
1, x: 2
236+
) {
237+
// some
238+
}
239+
240+
overloadOnDefaultArgsOnly {} // expected-error {{ambiguous use of 'overloadOnDefaultArgsOnly'}}
241+
overloadOnDefaultArgsOnly() {} // expected-error {{ambiguous use of 'overloadOnDefaultArgsOnly'}}
242+
overloadOnDefaultArgsOnly2 {} // expected-error {{ambiguous use of 'overloadOnDefaultArgsOnly2'}}
243+
overloadOnDefaultArgsOnly2() {} // expected-error {{ambiguous use of 'overloadOnDefaultArgsOnly2'}}
244+
overloadOnDefaultArgsOnly3 {} // expected-error {{ambiguous use of 'overloadOnDefaultArgsOnly3(x:a:)'}}
245+
overloadOnDefaultArgsOnly3() {} // expected-error {{ambiguous use of 'overloadOnDefaultArgsOnly3(x:a:)'}}
246+
247+
overloadOnSomeDefaultArgsOnly(1) {} // expected-error {{ambiguous use of 'overloadOnSomeDefaultArgsOnly'}}
248+
overloadOnSomeDefaultArgsOnly2(1) {} // expected-error {{ambiguous use of 'overloadOnSomeDefaultArgsOnly2'}}
249+
overloadOnSomeDefaultArgsOnly3(1) {} // expected-error {{ambiguous use of 'overloadOnSomeDefaultArgsOnly3(_:x:a:)'}}
250+
}

‎test/expr/unary/selector/selector.swift

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@ class C1 {
1515
@objc class func method3(_ a: A, b: B) { } // expected-note{{found this candidate}}
1616
@objc class func method3(a: A, b: B) { } // expected-note{{found this candidate}}
1717

18+
@objc(ambiguous1:b:) class func ambiguous(a: A, b: A) { } // expected-note{{found this candidate}}
19+
@objc(ambiguous2:b:) class func ambiguous(a: A, b: B) { } // expected-note{{found this candidate}}
20+
1821
@objc func getC1() -> AnyObject { return self }
1922

2023
@objc func testUnqualifiedSelector(_ a: A, b: B) {
@@ -83,7 +86,8 @@ func testSelector(_ c1: C1, p1: P1, obj: AnyObject) {
8386
}
8487

8588
func testAmbiguity() {
86-
_ = #selector(C1.method3) // expected-error{{ambiguous use of 'method3(_:b:)'}}
89+
_ = #selector(C1.method3) // expected-error{{ambiguous use of 'method3'}}
90+
_ = #selector(C1.ambiguous) // expected-error{{ambiguous use of 'ambiguous(a:b:)'}}
8791
}
8892

8993
func testUnusedSelector() {

0 commit comments

Comments
 (0)