Skip to content

Commit 08771ab

Browse files
xedinxedin
authored
Merge pull request swiftlang#33364 from xedin/trailing-closure-warning
[Diagnostics] Extract backward scan deprecation warning into a fix/diagnostic
2 parents 155e20c + 00b22bf commit 08771ab

File tree

8 files changed

+216
-129
lines changed

8 files changed

+216
-129
lines changed

‎lib/Sema/CSApply.cpp‎

Lines changed: 0 additions & 125 deletions
Original file line numberDiff line numberDiff line change
@@ -5469,124 +5469,6 @@ static bool hasCurriedSelf(ConstraintSystem &cs, ConcreteDeclRef callee,
54695469
return false;
54705470
}
54715471

5472-
/// Attach a Fix-It to the given diagnostic to give the trailing closure
5473-
/// argument a label.
5474-
static void labelTrailingClosureArgument(
5475-
ASTContext &ctx, Expr *fn, Expr *arg, Identifier paramName,
5476-
Expr *trailingClosure, InFlightDiagnostic &diag) {
5477-
// Dig out source locations.
5478-
SourceLoc existingRParenLoc;
5479-
SourceLoc leadingCommaLoc;
5480-
if (auto tupleExpr = dyn_cast<TupleExpr>(arg)) {
5481-
existingRParenLoc = tupleExpr->getRParenLoc();
5482-
assert(tupleExpr->getNumElements() >= 2 && "Should be a ParenExpr?");
5483-
leadingCommaLoc = Lexer::getLocForEndOfToken(
5484-
ctx.SourceMgr,
5485-
tupleExpr->getElements()[tupleExpr->getNumElements()-2]->getEndLoc());
5486-
} else {
5487-
auto parenExpr = cast<ParenExpr>(arg);
5488-
existingRParenLoc = parenExpr->getRParenLoc();
5489-
}
5490-
5491-
// Figure out the text to be inserted before the trailing closure.
5492-
SmallString<16> insertionText;
5493-
SourceLoc insertionLoc;
5494-
if (leadingCommaLoc.isValid()) {
5495-
insertionText += ", ";
5496-
assert(existingRParenLoc.isValid());
5497-
insertionLoc = leadingCommaLoc;
5498-
} else if (existingRParenLoc.isInvalid()) {
5499-
insertionText += "(";
5500-
insertionLoc = Lexer::getLocForEndOfToken(
5501-
ctx.SourceMgr, fn->getEndLoc());
5502-
} else {
5503-
insertionLoc = existingRParenLoc;
5504-
}
5505-
5506-
// Add the label, if there is one.
5507-
if (!paramName.empty()) {
5508-
insertionText += paramName.str();
5509-
insertionText += ": ";
5510-
}
5511-
5512-
// If there is an existing right parentheses, remove it while we
5513-
// insert the new text.
5514-
if (existingRParenLoc.isValid()) {
5515-
SourceLoc afterExistingRParenLoc = Lexer::getLocForEndOfToken(
5516-
ctx.SourceMgr, existingRParenLoc);
5517-
diag.fixItReplaceChars(
5518-
insertionLoc, afterExistingRParenLoc, insertionText);
5519-
} else {
5520-
// Insert the appropriate prefix.
5521-
diag.fixItInsert(insertionLoc, insertionText);
5522-
}
5523-
5524-
// Insert a right parenthesis after the closing '}' of the trailing closure;
5525-
SourceLoc newRParenLoc = Lexer::getLocForEndOfToken(
5526-
ctx.SourceMgr, trailingClosure->getEndLoc());
5527-
diag.fixItInsert(newRParenLoc, ")");
5528-
}
5529-
5530-
/// Find the trailing closure argument of a tuple or parenthesized expression.
5531-
///
5532-
/// Due to a quirk of the backward scan that could allow reordering of
5533-
/// arguments in the presence of a trailing closure, it might not be the last
5534-
/// argument in the tuple.
5535-
static Expr *findTrailingClosureArgument(Expr *arg) {
5536-
if (auto parenExpr = dyn_cast<ParenExpr>(arg)) {
5537-
return parenExpr->getSubExpr();
5538-
}
5539-
5540-
auto tupleExpr = cast<TupleExpr>(arg);
5541-
SourceLoc endLoc = tupleExpr->getEndLoc();
5542-
for (Expr *elt : llvm::reverse(tupleExpr->getElements())) {
5543-
if (elt->getEndLoc() == endLoc)
5544-
return elt;
5545-
}
5546-
5547-
return tupleExpr->getElements().back();
5548-
}
5549-
5550-
/// Find the index of the parameter that binds the given argument.
5551-
static unsigned findParamBindingArgument(
5552-
ArrayRef<ParamBinding> parameterBindings, unsigned argIndex) {
5553-
for (unsigned paramIdx : indices(parameterBindings)) {
5554-
if (llvm::find(parameterBindings[paramIdx], argIndex)
5555-
!= parameterBindings[paramIdx].end())
5556-
return paramIdx;
5557-
}
5558-
5559-
llvm_unreachable("No parameter binds the argument?");
5560-
}
5561-
5562-
/// Warn about the use of the deprecated "backward" scan for matching the
5563-
/// unlabeled trailing closure. It was needed to properly type check, but
5564-
/// this code will break with a future version of Swift.
5565-
static void warnAboutTrailingClosureBackwardScan(
5566-
ConcreteDeclRef callee, Expr *fn, Expr *arg,
5567-
ArrayRef<AnyFunctionType::Param> params,
5568-
Optional<unsigned> unlabeledTrailingClosureIndex,
5569-
ArrayRef<ParamBinding> parameterBindings) {
5570-
5571-
Expr *trailingClosure = findTrailingClosureArgument(arg);
5572-
unsigned paramIdx = findParamBindingArgument(
5573-
parameterBindings, *unlabeledTrailingClosureIndex);
5574-
ASTContext &ctx = params[paramIdx].getPlainType()->getASTContext();
5575-
Identifier paramName = params[paramIdx].getLabel();
5576-
5577-
{
5578-
auto diag = ctx.Diags.diagnose(
5579-
trailingClosure->getStartLoc(),
5580-
diag::unlabeled_trailing_closure_deprecated, paramName);
5581-
labelTrailingClosureArgument(
5582-
ctx, fn, arg, paramName, trailingClosure, diag);
5583-
}
5584-
5585-
if (auto decl = callee.getDecl()) {
5586-
ctx.Diags.diagnose(decl, diag::decl_declared_here, decl->getName());
5587-
}
5588-
}
5589-
55905472
Expr *ExprRewriter::coerceCallArguments(
55915473
Expr *arg, AnyFunctionType *funcType,
55925474
ConcreteDeclRef callee,
@@ -5678,13 +5560,6 @@ Expr *ExprRewriter::coerceCallArguments(
56785560
// FIXME: Eventually, we want to enforce that we have either argTuple or
56795561
// argParen here.
56805562

5681-
// Warn about the backward scan being deprecated.
5682-
if (trailingClosureMatching == TrailingClosureMatching::Backward) {
5683-
warnAboutTrailingClosureBackwardScan(
5684-
callee, apply ? apply->getFn() : nullptr, arg, params,
5685-
unlabeledTrailingClosureIndex, parameterBindings);
5686-
}
5687-
56885563
SourceLoc lParenLoc, rParenLoc;
56895564
if (argTuple) {
56905565
lParenLoc = argTuple->getLParenLoc();

‎lib/Sema/CSDiagnostics.cpp‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6598,3 +6598,95 @@ SourceLoc MissingOptionalUnwrapKeyPathFailure::getLoc() const {
65986598
auto *SE = castToExpr<SubscriptExpr>(getAnchor());
65996599
return SE->getBase()->getEndLoc();
66006600
}
6601+
6602+
bool TrailingClosureRequiresExplicitLabel::diagnoseAsError() {
6603+
auto argInfo = *getFunctionArgApplyInfo(getLocator());
6604+
6605+
{
6606+
auto diagnostic = emitDiagnostic(
6607+
diag::unlabeled_trailing_closure_deprecated, argInfo.getParamLabel());
6608+
fixIt(diagnostic, argInfo);
6609+
}
6610+
6611+
if (auto *callee = argInfo.getCallee()) {
6612+
emitDiagnosticAt(callee, diag::decl_declared_here, callee->getName());
6613+
}
6614+
6615+
return true;
6616+
}
6617+
6618+
void TrailingClosureRequiresExplicitLabel::fixIt(
6619+
InFlightDiagnostic &diagnostic, const FunctionArgApplyInfo &info) const {
6620+
auto &ctx = getASTContext();
6621+
6622+
// Dig out source locations.
6623+
SourceLoc existingRParenLoc;
6624+
SourceLoc leadingCommaLoc;
6625+
6626+
auto anchor = getRawAnchor();
6627+
auto *arg = info.getArgListExpr();
6628+
Expr *fn = nullptr;
6629+
6630+
if (auto *applyExpr = getAsExpr<ApplyExpr>(anchor)) {
6631+
fn = applyExpr->getFn();
6632+
} else {
6633+
// Covers subscripts, unresolved members etc.
6634+
fn = getAsExpr(anchor);
6635+
}
6636+
6637+
if (!fn)
6638+
return;
6639+
6640+
auto *trailingClosure = info.getArgExpr();
6641+
6642+
if (auto tupleExpr = dyn_cast<TupleExpr>(arg)) {
6643+
existingRParenLoc = tupleExpr->getRParenLoc();
6644+
assert(tupleExpr->getNumElements() >= 2 && "Should be a ParenExpr?");
6645+
leadingCommaLoc = Lexer::getLocForEndOfToken(
6646+
ctx.SourceMgr,
6647+
tupleExpr->getElements()[tupleExpr->getNumElements() - 2]->getEndLoc());
6648+
} else {
6649+
auto parenExpr = cast<ParenExpr>(arg);
6650+
existingRParenLoc = parenExpr->getRParenLoc();
6651+
}
6652+
6653+
// Figure out the text to be inserted before the trailing closure.
6654+
SmallString<16> insertionText;
6655+
SourceLoc insertionLoc;
6656+
if (leadingCommaLoc.isValid()) {
6657+
insertionText += ", ";
6658+
assert(existingRParenLoc.isValid());
6659+
insertionLoc = leadingCommaLoc;
6660+
} else if (existingRParenLoc.isInvalid()) {
6661+
insertionText += "(";
6662+
insertionLoc = Lexer::getLocForEndOfToken(ctx.SourceMgr, fn->getEndLoc());
6663+
} else {
6664+
insertionLoc = existingRParenLoc;
6665+
}
6666+
6667+
// Add the label, if there is one.
6668+
auto paramName = info.getParamLabel();
6669+
if (!paramName.empty()) {
6670+
insertionText += paramName.str();
6671+
insertionText += ": ";
6672+
}
6673+
6674+
// If there is an existing right parentheses/brace, remove it while we
6675+
// insert the new text.
6676+
if (existingRParenLoc.isValid()) {
6677+
SourceLoc afterExistingRParenLoc =
6678+
Lexer::getLocForEndOfToken(ctx.SourceMgr, existingRParenLoc);
6679+
diagnostic.fixItReplaceChars(insertionLoc, afterExistingRParenLoc,
6680+
insertionText);
6681+
} else {
6682+
// Insert the appropriate prefix.
6683+
diagnostic.fixItInsert(insertionLoc, insertionText);
6684+
}
6685+
6686+
// Insert a right parenthesis/brace after the closing '}' of the trailing
6687+
// closure;
6688+
SourceLoc newRParenLoc =
6689+
Lexer::getLocForEndOfToken(ctx.SourceMgr, trailingClosure->getEndLoc());
6690+
diagnostic.fixItInsert(newRParenLoc,
6691+
isExpr<SubscriptExpr>(anchor) ? "]" : ")");
6692+
}

‎lib/Sema/CSDiagnostics.h‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2216,6 +2216,30 @@ class MissingOptionalUnwrapKeyPathFailure final : public ContextualFailure {
22162216
SourceLoc getLoc() const override;
22172217
};
22182218

2219+
/// Diagnose situations when trailing closure has been matched to a specific
2220+
/// parameter via a deprecated backward scan.
2221+
///
2222+
/// \code
2223+
/// func multiple_trailing_with_defaults(
2224+
/// duration: Int,
2225+
/// animations: (() -> Void)? = nil,
2226+
/// completion: (() -> Void)? = nil) {}
2227+
///
2228+
/// multiple_trailing_with_defaults(duration: 42) {} // picks `completion:`
2229+
/// \endcode
2230+
class TrailingClosureRequiresExplicitLabel final : public FailureDiagnostic {
2231+
public:
2232+
TrailingClosureRequiresExplicitLabel(const Solution &solution,
2233+
ConstraintLocator *locator)
2234+
: FailureDiagnostic(solution, locator) {}
2235+
2236+
bool diagnoseAsError() override;
2237+
2238+
private:
2239+
void fixIt(InFlightDiagnostic &diagnostic,
2240+
const FunctionArgApplyInfo &info) const;
2241+
};
2242+
22192243
} // end namespace constraints
22202244
} // end namespace swift
22212245

‎lib/Sema/CSFix.cpp‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1526,3 +1526,16 @@ UnwrapOptionalBaseKeyPathApplication::attempt(ConstraintSystem &cs, Type baseTy,
15261526
return new (cs.getAllocator())
15271527
UnwrapOptionalBaseKeyPathApplication(cs, baseTy, rootTy, locator);
15281528
}
1529+
1530+
bool SpecifyLabelToAssociateTrailingClosure::diagnose(const Solution &solution,
1531+
bool asNote) const {
1532+
TrailingClosureRequiresExplicitLabel failure(solution, getLocator());
1533+
return failure.diagnose(asNote);
1534+
}
1535+
1536+
SpecifyLabelToAssociateTrailingClosure *
1537+
SpecifyLabelToAssociateTrailingClosure::create(ConstraintSystem &cs,
1538+
ConstraintLocator *locator) {
1539+
return new (cs.getAllocator())
1540+
SpecifyLabelToAssociateTrailingClosure(cs, locator);
1541+
}

‎lib/Sema/CSFix.h‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,10 @@ enum class FixKind : uint8_t {
269269

270270
/// Unwrap optional base on key path application.
271271
UnwrapOptionalBaseKeyPathApplication,
272+
273+
/// Explicitly specify a label to match trailing closure to a certain
274+
/// parameter in the call.
275+
SpecifyLabelToAssociateTrailingClosure,
272276
};
273277

274278
class ConstraintFix {
@@ -1926,6 +1930,34 @@ class UnwrapOptionalBaseKeyPathApplication final : public ContextualMismatch {
19261930
ConstraintLocator *locator);
19271931
};
19281932

1933+
/// Diagnose situations when solver used old (backward scan) rule
1934+
/// to match trailing closure to a parameter.
1935+
///
1936+
/// \code
1937+
/// func multiple_trailing_with_defaults(
1938+
/// duration: Int,
1939+
/// animations: (() -> Void)? = nil,
1940+
/// completion: (() -> Void)? = nil) {}
1941+
///
1942+
/// multiple_trailing_with_defaults(duration: 42) {} // picks `completion:`
1943+
/// \endcode
1944+
class SpecifyLabelToAssociateTrailingClosure final : public ConstraintFix {
1945+
SpecifyLabelToAssociateTrailingClosure(ConstraintSystem &cs,
1946+
ConstraintLocator *locator)
1947+
: ConstraintFix(cs, FixKind::SpecifyLabelToAssociateTrailingClosure,
1948+
locator, /*isWarning=*/true) {}
1949+
1950+
public:
1951+
std::string getName() const override {
1952+
return "specify a label to associate trailing closure with parameter";
1953+
}
1954+
1955+
bool diagnose(const Solution &solution, bool asNote = false) const override;
1956+
1957+
static SpecifyLabelToAssociateTrailingClosure *
1958+
create(ConstraintSystem &cs, ConstraintLocator *locator);
1959+
};
1960+
19291961
} // end namespace constraints
19301962
} // end namespace swift
19311963

‎lib/Sema/CSSimplify.cpp‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1208,6 +1208,9 @@ ConstraintSystem::TypeMatchResult constraints::matchCallArguments(
12081208

12091209
// Match up the call arguments to the parameters.
12101210
SmallVector<ParamBinding, 4> parameterBindings;
1211+
TrailingClosureMatching selectedTrailingMatching =
1212+
TrailingClosureMatching::Forward;
1213+
12111214
{
12121215
ArgumentFailureTracker listener(cs, argsWithLabels, params, locator);
12131216
auto callArgumentMatch = constraints::matchCallArguments(
@@ -1224,10 +1227,10 @@ ConstraintSystem::TypeMatchResult constraints::matchCallArguments(
12241227
return cs.getTypeMatchAmbiguous();
12251228
}
12261229

1230+
selectedTrailingMatching = callArgumentMatch->trailingClosureMatching;
12271231
// Record the direction of matching used for this call.
1228-
cs.recordTrailingClosureMatch(
1229-
cs.getConstraintLocator(locator),
1230-
callArgumentMatch->trailingClosureMatching);
1232+
cs.recordTrailingClosureMatch(cs.getConstraintLocator(locator),
1233+
selectedTrailingMatching);
12311234

12321235
// If there was a disjunction because both forward and backward were
12331236
// possible, increase the score for forward matches to bias toward the
@@ -1316,6 +1319,15 @@ ConstraintSystem::TypeMatchResult constraints::matchCallArguments(
13161319
}
13171320
}
13181321

1322+
// In case solver matched trailing based on the backward scan,
1323+
// let's produce a warning which would suggest to add a label
1324+
// to disambiguate in the future.
1325+
if (selectedTrailingMatching == TrailingClosureMatching::Backward &&
1326+
argIdx == *argInfo->UnlabeledTrailingClosureIndex) {
1327+
cs.recordFix(SpecifyLabelToAssociateTrailingClosure::create(
1328+
cs, cs.getConstraintLocator(loc)));
1329+
}
1330+
13191331
// If argument comes for declaration it should loose
13201332
// `@autoclosure` flag, because in context it's used
13211333
// as a function type represented by autoclosure.
@@ -9967,7 +9979,8 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
99679979
case FixKind::AllowKeyPathRootTypeMismatch:
99689980
case FixKind::UnwrapOptionalBaseKeyPathApplication:
99699981
case FixKind::AllowCoercionToForceCast:
9970-
case FixKind::SpecifyKeyPathRootType: {
9982+
case FixKind::SpecifyKeyPathRootType:
9983+
case FixKind::SpecifyLabelToAssociateTrailingClosure: {
99719984
return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;
99729985
}
99739986

0 commit comments

Comments
 (0)