From e87afbab7673ba68c073917109d2c2baeef7d4fc Mon Sep 17 00:00:00 2001 From: Xiaodi Wu <13952+xwu@users.noreply.github.com> Date: Fri, 8 May 2020 20:37:23 -0400 Subject: [PATCH 1/2] Allow optional labels on the first trailing closure --- include/swift/AST/Expr.h | 42 +++---- lib/AST/Expr.cpp | 20 +-- lib/IDE/SwiftSourceDocInfo.cpp | 2 +- lib/Parse/ParseExpr.cpp | 132 ++++++++++---------- lib/Sema/CSApply.cpp | 2 +- lib/Sema/CSGen.cpp | 20 +-- lib/Sema/CSSimplify.cpp | 172 ++++++++++++++------------ lib/Sema/ConstraintSystem.h | 6 +- utils/gyb_syntax_support/ExprNodes.py | 13 +- 9 files changed, 216 insertions(+), 193 deletions(-) diff --git a/include/swift/AST/Expr.h b/include/swift/AST/Expr.h index 3c13b029ac9aa..c52a8b8a0922a 100644 --- a/include/swift/AST/Expr.h +++ b/include/swift/AST/Expr.h @@ -544,8 +544,8 @@ class alignas(8) Expr { /// Given that this is a packed argument expression of the sort that /// would be produced from packSingleArgument, return the index of the - /// unlabeled trailing closure, if there is one. - Optional getUnlabeledTrailingClosureIndexOfPackedArgument() const; + /// first trailing closure, if there is one. + Optional getFirstTrailingClosureIndexOfPackedArgument() const; /// Produce a mapping from each subexpression to its parent /// expression, with the provided expression serving as the root of @@ -1234,9 +1234,9 @@ class ObjectLiteralExpr final return Bits.ObjectLiteralExpr.HasTrailingClosure; } - /// Return the index of the unlabeled trailing closure argument. - Optional getUnlabeledTrailingClosureIndex() const { - return getArg()->getUnlabeledTrailingClosureIndexOfPackedArgument(); + /// Return the index of the first trailing closure argument. + Optional getFirstTrailingClosureIndex() const { + return getArg()->getFirstTrailingClosureIndexOfPackedArgument(); } SourceLoc getSourceLoc() const { return PoundLoc; } @@ -1825,9 +1825,9 @@ class DynamicSubscriptExpr final return Bits.DynamicSubscriptExpr.HasTrailingClosure; } - /// Return the index of the unlabeled trailing closure argument. - Optional getUnlabeledTrailingClosureIndex() const { - return Index->getUnlabeledTrailingClosureIndexOfPackedArgument(); + /// Return the index of the first trailing closure argument. + Optional getFirstTrailingClosureIndex() const { + return Index->getFirstTrailingClosureIndexOfPackedArgument(); } SourceLoc getLoc() const { return Index->getStartLoc(); } @@ -1899,9 +1899,9 @@ class UnresolvedMemberExpr final return Bits.UnresolvedMemberExpr.HasTrailingClosure; } - /// Return the index of the unlabeled trailing closure argument. - Optional getUnlabeledTrailingClosureIndex() const { - return getArgument()->getUnlabeledTrailingClosureIndexOfPackedArgument(); + /// Return the index of the first trailing closure argument. + Optional getFirstTrailingClosureIndex() const { + return getArgument()->getFirstTrailingClosureIndexOfPackedArgument(); } SourceLoc getLoc() const { return NameLoc.getBaseNameLoc(); } @@ -2086,7 +2086,7 @@ class ParenExpr : public IdentityExpr { bool hasTrailingClosure() const { return Bits.ParenExpr.HasTrailingClosure; } Optional - getUnlabeledTrailingClosureIndexOfPackedArgument() const { + getFirstTrailingClosureIndexOfPackedArgument() const { return hasTrailingClosure() ? Optional(0) : None; } @@ -2185,7 +2185,7 @@ class TupleExpr final : public Expr, } Optional - getUnlabeledTrailingClosureIndexOfPackedArgument() const { + getFirstTrailingClosureIndexOfPackedArgument() const { return FirstTrailingArgumentAt; } @@ -2474,9 +2474,9 @@ class SubscriptExpr final : public LookupExpr, return Bits.SubscriptExpr.HasTrailingClosure; } - /// Return the index of the unlabeled trailing closure argument. - Optional getUnlabeledTrailingClosureIndex() const { - return getIndex()->getUnlabeledTrailingClosureIndexOfPackedArgument(); + /// Return the index of the first trailing closure argument. + Optional getFirstTrailingClosureIndex() const { + return getIndex()->getFirstTrailingClosureIndexOfPackedArgument(); } /// Determine whether this subscript reference should bypass the @@ -4324,8 +4324,8 @@ class ApplyExpr : public Expr { /// Whether this application was written using a trailing closure. bool hasTrailingClosure() const; - /// Return the index of the unlabeled trailing closure argument. - Optional getUnlabeledTrailingClosureIndex() const; + /// Return the index of the first trailing closure argument. + Optional getFirstTrailingClosureIndex() const; static bool classof(const Expr *E) { return E->getKind() >= ExprKind::First_ApplyExpr && @@ -4413,9 +4413,9 @@ class CallExpr final : public ApplyExpr, /// Whether this call with written with a single trailing closure. bool hasTrailingClosure() const { return Bits.CallExpr.HasTrailingClosure; } - /// Return the index of the unlabeled trailing closure argument. - Optional getUnlabeledTrailingClosureIndex() const { - return getArg()->getUnlabeledTrailingClosureIndexOfPackedArgument(); + /// Return the index of the first trailing closure argument. + Optional getFirstTrailingClosureIndex() const { + return getArg()->getFirstTrailingClosureIndexOfPackedArgument(); } using TrailingCallArguments::getArgumentLabels; diff --git a/lib/AST/Expr.cpp b/lib/AST/Expr.cpp index 3dd7586713141..f7396b4437993 100644 --- a/lib/AST/Expr.cpp +++ b/lib/AST/Expr.cpp @@ -1069,24 +1069,24 @@ Expr *swift::packSingleArgument(ASTContext &ctx, SourceLoc lParenLoc, argLabelLocs = argLabelLocsScratch; } - Optional unlabeledTrailingClosureIndex; - if (!trailingClosures.empty() && trailingClosures[0].Label.empty()) - unlabeledTrailingClosureIndex = args.size() - trailingClosures.size(); + Optional firstTrailingClosureIndex; + if (!trailingClosures.empty()) + firstTrailingClosureIndex = args.size() - trailingClosures.size(); auto arg = TupleExpr::create(ctx, lParenLoc, rParenLoc, args, argLabels, argLabelLocs, - unlabeledTrailingClosureIndex, + firstTrailingClosureIndex, /*Implicit=*/false); computeSingleArgumentType(ctx, arg, implicit, getType); return arg; } Optional -Expr::getUnlabeledTrailingClosureIndexOfPackedArgument() const { +Expr::getFirstTrailingClosureIndexOfPackedArgument() const { if (auto PE = dyn_cast(this)) - return PE->getUnlabeledTrailingClosureIndexOfPackedArgument(); + return PE->getFirstTrailingClosureIndexOfPackedArgument(); if (auto TE = dyn_cast(this)) - return TE->getUnlabeledTrailingClosureIndexOfPackedArgument(); + return TE->getFirstTrailingClosureIndexOfPackedArgument(); return None; } @@ -1675,9 +1675,9 @@ bool ApplyExpr::hasTrailingClosure() const { return false; } -Optional ApplyExpr::getUnlabeledTrailingClosureIndex() const { +Optional ApplyExpr::getFirstTrailingClosureIndex() const { if (auto call = dyn_cast(this)) - return call->getUnlabeledTrailingClosureIndex(); + return call->getFirstTrailingClosureIndex(); return None; } @@ -1731,7 +1731,7 @@ CallExpr *CallExpr::create(ASTContext &ctx, Expr *fn, SourceLoc lParenLoc, SmallVector argLabelsScratch; SmallVector argLabelLocsScratch; Expr *arg = packSingleArgument(ctx, lParenLoc, args, argLabels, argLabelLocs, - rParenLoc, trailingClosures, implicit, + rParenLoc, trailingClosures, implicit, argLabelsScratch, argLabelLocsScratch, getType); diff --git a/lib/IDE/SwiftSourceDocInfo.cpp b/lib/IDE/SwiftSourceDocInfo.cpp index f8211c437795a..1cf335a2689ce 100644 --- a/lib/IDE/SwiftSourceDocInfo.cpp +++ b/lib/IDE/SwiftSourceDocInfo.cpp @@ -887,7 +887,7 @@ std::vector swift::ide:: getCallArgInfo(SourceManager &SM, Expr *Arg, LabelRangeEndAt EndKind) { std::vector InfoVec; if (auto *TE = dyn_cast(Arg)) { - auto FirstTrailing = TE->getUnlabeledTrailingClosureIndexOfPackedArgument(); + auto FirstTrailing = TE->getFirstTrailingClosureIndexOfPackedArgument(); for (size_t ElemIndex: range(TE->getNumElements())) { Expr *Elem = TE->getElement(ElemIndex); if (isa(Elem)) diff --git a/lib/Parse/ParseExpr.cpp b/lib/Parse/ParseExpr.cpp index 0b3252d489c60..cce91a102a760 100644 --- a/lib/Parse/ParseExpr.cpp +++ b/lib/Parse/ParseExpr.cpp @@ -948,7 +948,7 @@ static bool isStartOfGetSetAccessor(Parser &P) { /// where the parser requires an expr-basic (which does not allow them). We /// handle this by doing some lookahead in common situations. And later, Sema /// will emit a diagnostic with a fixit to add wrapping parens. -static bool isValidTrailingClosure(bool isExprBasic, Parser &P){ +static bool isValidUnlabeledTrailingClosure(bool isExprBasic, Parser &P) { assert(P.Tok.is(tok::l_brace) && "Couldn't be a trailing closure"); // If this is the start of a get/set accessor, then it isn't a trailing @@ -1013,7 +1013,39 @@ static bool isValidTrailingClosure(bool isExprBasic, Parser &P){ } } +static bool isStartOfLabeledTrailingClosure(Parser &P) { + // Fast path: the next two tokens must be a label and a colon. + // But 'default:' is ambiguous with switch cases and we disallow it + // (unless escaped) even outside of switches. + if (!P.Tok.canBeArgumentLabel() || + P.Tok.is(tok::kw_default) || + !P.peekToken().is(tok::colon)) + return false; + // Do some tentative parsing to distinguish `label: { ... }` and + // `label: switch x { ... }`. + Parser::BacktrackingScope backtrack(P); + P.consumeToken(); + if (P.peekToken().is(tok::l_brace)) + return true; + // Parse editor placeholder as trailing closure so SourceKit can expand it to + // closure literal. + if (P.peekToken().is(tok::identifier) && + Identifier::isEditorPlaceholder(P.peekToken().getText())) + return true; + // Consider `label: ` that the user is trying to write a closure. + if (P.peekToken().is(tok::code_complete) && !P.peekToken().isAtStartOfLine()) + return true; + return false; +} + +static bool isStartOfFirstTrailingClosure(bool isExprBasic, Parser &P) { + if (P.Tok.is(tok::l_brace) && isValidUnlabeledTrailingClosure(isExprBasic, P)) + return true; + if (isStartOfLabeledTrailingClosure(P)) + return true; + return false; +} /// Map magic literal tokens such as #file to their /// MagicIdentifierLiteralExpr kind. @@ -1202,7 +1234,7 @@ Parser::parseExprPostfixSuffix(ParserResult Result, bool isExprBasic, } // Check for a trailing closure, if allowed. - if (Tok.is(tok::l_brace) && isValidTrailingClosure(isExprBasic, *this)) { + if (isStartOfFirstTrailingClosure(isExprBasic, *this)) { // FIXME: if Result has a trailing closure, break out. // Stop after literal expressions, which may never have trailing closures. @@ -1233,8 +1265,8 @@ Parser::parseExprPostfixSuffix(ParserResult Result, bool isExprBasic, /*implicit=*/false)); SyntaxContext->createNodeInPlace(SyntaxKind::FunctionCallExpr); - // We only allow a single trailing closure on a call. This could be - // generalized in the future, but needs further design. + // We only allow a single unlabeled trailing closure on a call. This + // could be generalized in the future, but needs further design. if (Tok.is(tok::l_brace)) break; continue; @@ -1626,7 +1658,7 @@ ParserResult Parser::parseExprPrimary(Diag<> ID, bool isExprBasic) { } // Check for a trailing closure, if allowed. - if (Tok.is(tok::l_brace) && isValidTrailingClosure(isExprBasic, *this)) { + if (isStartOfFirstTrailingClosure(isExprBasic, *this)) { if (SyntaxContext->isEnabled()) { // Add dummy blank argument list to the call expression syntax. SyntaxContext->addSyntax( @@ -3133,8 +3165,7 @@ ParserStatus Parser::parseExprList(tok leftTok, tok rightTok, // If we aren't interested in trailing closures, or there isn't a valid one, // we're done. - if (!isPostfix || Tok.isNot(tok::l_brace) || - !isValidTrailingClosure(isExprBasic, *this)) + if (!isPostfix || !isStartOfFirstTrailingClosure(isExprBasic, *this)) return status; // Parse the closure. @@ -3144,72 +3175,48 @@ ParserStatus Parser::parseExprList(tok leftTok, tok rightTok, return status; } -static bool isStartOfLabelledTrailingClosure(Parser &P) { - // Fast path: the next two tokens must be a label and a colon. - // But 'default:' is ambiguous with switch cases and we disallow it - // (unless escaped) even outside of switches. - if (!P.Tok.canBeArgumentLabel() || - P.Tok.is(tok::kw_default) || - !P.peekToken().is(tok::colon)) - return false; - - // Do some tentative parsing to distinguish `label: { ... }` and - // `label: switch x { ... }`. - Parser::BacktrackingScope backtrack(P); - P.consumeToken(); - if (P.peekToken().is(tok::l_brace)) - return true; - // Parse editor placeholder as trailing closure so SourceKit can expand it to - // closure literal. - if (P.peekToken().is(tok::identifier) && - Identifier::isEditorPlaceholder(P.peekToken().getText())) - return true; - // Consider `label: ` that the user is trying to write a closure. - if (P.peekToken().is(tok::code_complete) && !P.peekToken().isAtStartOfLine()) - return true; - return false; -} - ParserStatus Parser::parseTrailingClosures(bool isExprBasic, SourceRange calleeRange, SmallVectorImpl &closures) { - SourceLoc braceLoc = Tok.getLoc(); - - // Record the line numbers for the diagnostics below. - // Note that *do not* move this to after 'parseExprClosure()' it slows down - // 'getLineNumber()' call because of cache in SourceMgr. - auto origLine = SourceMgr.getLineNumber(calleeRange.End); - auto braceLine = SourceMgr.getLineNumber(braceLoc); - ParserStatus result; - // Parse the closure. - ParserResult closure = parseExprClosure(); - if (closure.isNull()) - return makeParserError(); + // Parse any unlabeled trailing closure. + if (Tok.is(tok::l_brace)) { + SourceLoc braceLoc = Tok.getLoc(); - result |= closure; + // Record the line numbers for the diagnostics below. + // Note that *do not* move this to after 'parseExprClosure()' it slows down + // 'getLineNumber()' call because of cache in SourceMgr. + auto origLine = SourceMgr.getLineNumber(calleeRange.End); + auto braceLine = SourceMgr.getLineNumber(braceLoc); - closures.push_back({closure.get()}); - - // Warn if the trailing closure is separated from its callee by more than - // one line. A single-line separation is acceptable for a trailing closure - // call, and will be diagnosed later only if the call fails to typecheck. - if (braceLine > origLine + 1) { - diagnose(braceLoc, diag::trailing_closure_after_newlines); - diagnose(calleeRange.Start, diag::trailing_closure_callee_here); + // Parse the closure. + ParserResult closure = parseExprClosure(); + if (closure.isNull()) + return makeParserError(); - auto *CE = dyn_cast(closures[0].ClosureExpr); - if (CE && CE->hasAnonymousClosureVars() && - CE->getParameters()->size() == 0) { - diagnose(braceLoc, diag::brace_stmt_suggest_do) - .fixItInsert(braceLoc, "do "); + result |= closure; + closures.push_back({closure.get()}); + + // Warn if the trailing closure is separated from its callee by more than + // one line. A single-line separation is acceptable for a trailing closure + // call, and will be diagnosed later only if the call fails to typecheck. + if (braceLine > origLine + 1) { + diagnose(braceLoc, diag::trailing_closure_after_newlines); + diagnose(calleeRange.Start, diag::trailing_closure_callee_here); + + auto *CE = dyn_cast(closures[0].ClosureExpr); + if (CE && CE->hasAnonymousClosureVars() && + CE->getParameters()->size() == 0) { + diagnose(braceLoc, diag::brace_stmt_suggest_do) + .fixItInsert(braceLoc, "do "); + } } } // Parse labeled trailing closures. while (true) { - if (!isStartOfLabelledTrailingClosure(*this)) { + if (!isStartOfLabeledTrailingClosure(*this)) { if (!Tok.is(tok::code_complete)) break; @@ -3241,7 +3248,7 @@ Parser::parseTrailingClosures(bool isExprBasic, SourceRange calleeRange, consumeToken(tok::identifier); } else if (Tok.is(tok::code_complete)) { assert(!Tok.isAtStartOfLine() && - "isStartOfLabelledTrailingClosure() should return false"); + "isStartOfLabeledTrailingClosure() should return false"); // Swallow code completion token after the label. // FIXME: Closure literal completion. consumeToken(tok::code_complete); @@ -3253,7 +3260,8 @@ Parser::parseTrailingClosures(bool isExprBasic, SourceRange calleeRange, result |= closure; closures.push_back({label, labelLoc, closure.get()}); - // Don't diagnose whitespace gaps before labelled closures. + // Don't diagnose whitespace gaps before labeled closures. + // TODO: Recover if multiple closures are separated by a comma. } SyntaxContext->collectNodesInPlace( SyntaxKind::MultipleTrailingClosureElementList); diff --git a/lib/Sema/CSApply.cpp b/lib/Sema/CSApply.cpp index 6bbc088b4bcbd..0a186f5b1d429 100644 --- a/lib/Sema/CSApply.cpp +++ b/lib/Sema/CSApply.cpp @@ -5515,7 +5515,7 @@ Expr *ExprRewriter::coerceCallArguments(Expr *arg, AnyFunctionType *funcType, SmallVector parameterBindings; bool failed = constraints::matchCallArguments(args, params, paramInfo, - arg->getUnlabeledTrailingClosureIndexOfPackedArgument(), + arg->getFirstTrailingClosureIndexOfPackedArgument(), /*allowFixes=*/false, listener, parameterBindings); diff --git a/lib/Sema/CSGen.cpp b/lib/Sema/CSGen.cpp index a4ea12163c394..922385fffae3b 100644 --- a/lib/Sema/CSGen.cpp +++ b/lib/Sema/CSGen.cpp @@ -994,7 +994,7 @@ namespace { Type addSubscriptConstraints( Expr *anchor, Type baseTy, Expr *index, ValueDecl *declOrNull, ArrayRef argLabels, - Optional unlabeledTrailingClosure, + Optional firstTrailingClosure, ConstraintLocator *locator = nullptr, SmallVectorImpl *addedTypeVars = nullptr) { // Locators used in this expression. @@ -1012,7 +1012,7 @@ namespace { ConstraintLocator::FunctionResult); associateArgumentLabels(memberLocator, - {argLabels, unlabeledTrailingClosure}); + {argLabels, firstTrailingClosure}); Type outputTy; @@ -1306,7 +1306,7 @@ namespace { auto *exprLoc = CS.getConstraintLocator(expr); associateArgumentLabels( exprLoc, {expr->getArgumentLabels(), - expr->getUnlabeledTrailingClosureIndex()}); + expr->getFirstTrailingClosureIndex()}); // If the expression has already been assigned a type; just use that type. if (expr->getType()) @@ -1591,7 +1591,7 @@ namespace { associateArgumentLabels( CS.getConstraintLocator(expr), {expr->getArgumentLabels(), - expr->getUnlabeledTrailingClosureIndex()}); + expr->getFirstTrailingClosureIndex()}); return baseTy; } @@ -1833,7 +1833,7 @@ namespace { return addSubscriptConstraints(expr, CS.getType(base), expr->getIndex(), decl, expr->getArgumentLabels(), - expr->getUnlabeledTrailingClosureIndex()); + expr->getFirstTrailingClosureIndex()); } Type visitArrayExpr(ArrayExpr *expr) { @@ -2125,7 +2125,7 @@ namespace { return addSubscriptConstraints(expr, CS.getType(expr->getBase()), expr->getIndex(), /*decl*/ nullptr, expr->getArgumentLabels(), - expr->getUnlabeledTrailingClosureIndex()); + expr->getFirstTrailingClosureIndex()); } Type visitTupleElementExpr(TupleElementExpr *expr) { @@ -2909,7 +2909,7 @@ namespace { associateArgumentLabels( CS.getConstraintLocator(expr), {expr->getArgumentLabels(scratch), - expr->getUnlabeledTrailingClosureIndex()}, + expr->getFirstTrailingClosureIndex()}, /*labelsArePermanent=*/isa(expr)); if (auto *UDE = dyn_cast(fnExpr)) { @@ -3458,12 +3458,12 @@ namespace { // re-type-check the constraints during failure diagnosis. case KeyPathExpr::Component::Kind::Subscript: { auto index = component.getIndexExpr(); - auto unlabeledTrailingClosureIndex = - index->getUnlabeledTrailingClosureIndexOfPackedArgument(); + auto firstTrailingClosureIndex = + index->getFirstTrailingClosureIndexOfPackedArgument(); base = addSubscriptConstraints(E, base, index, /*decl*/ nullptr, component.getSubscriptLabels(), - unlabeledTrailingClosureIndex, + firstTrailingClosureIndex, memberLocator, &componentTypeVars); break; diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp index 0f1279a2a578a..7b05b82deecf4 100644 --- a/lib/Sema/CSSimplify.cpp +++ b/lib/Sema/CSSimplify.cpp @@ -126,7 +126,7 @@ bool constraints::doesMemberRefApplyCurriedSelf(Type baseTy, static bool areConservativelyCompatibleArgumentLabels( OverloadChoice choice, SmallVectorImpl &args, - Optional unlabeledTrailingClosureArgIndex) { + Optional firstTrailingClosureArgIndex) { ValueDecl *decl = nullptr; switch (choice.getKind()) { case OverloadChoiceKind::Decl: @@ -168,7 +168,7 @@ static bool areConservativelyCompatibleArgumentLabels( SmallVector unusedParamBindings; return !matchCallArguments(args, params, paramInfo, - unlabeledTrailingClosureArgIndex, + firstTrailingClosureArgIndex, /*allow fixes*/ false, listener, unusedParamBindings); } @@ -220,13 +220,13 @@ bool constraints:: matchCallArguments(SmallVectorImpl &args, ArrayRef params, const ParameterListInfo ¶mInfo, - Optional unlabeledTrailingClosureArgIndex, + Optional firstTrailingClosureArgIndex, bool allowFixes, MatchCallArgumentListener &listener, SmallVectorImpl ¶meterBindings) { assert(params.size() == paramInfo.size() && "Default map does not match"); - assert(!unlabeledTrailingClosureArgIndex || - *unlabeledTrailingClosureArgIndex < args.size()); + assert(!firstTrailingClosureArgIndex || + *firstTrailingClosureArgIndex < args.size()); // Keep track of the parameter we're matching and what argument indices // got bound to each parameter. @@ -433,16 +433,28 @@ matchCallArguments(SmallVectorImpl &args, haveUnfulfilledParams = true; }; - // If we have an unlabeled trailing closure, we match trailing closure - // labels from the end, then match the trailing closure arg. - if (unlabeledTrailingClosureArgIndex) { - unsigned unlabeledArgIdx = *unlabeledTrailingClosureArgIndex; + // If we have any trailing closures, we match trailing closure labels from the + // end, then match any unlabeled trailing closure arg. + if (firstTrailingClosureArgIndex) { + unsigned firstArgIdx = *firstTrailingClosureArgIndex; // One past the next parameter index to look at. unsigned prevParamIdx = numParams; + bool hasUnlabeledFirstTrailingClosure = false; + // Scan backwards to match any labeled trailing closures. - for (unsigned argIdx = numArgs - 1; argIdx != unlabeledArgIdx; --argIdx) { + unsigned argIdx = numArgs; + while (argIdx > firstArgIdx) { + argIdx -= 1; + auto argLabel = args[argIdx].getLabel(); + + // We're done if we're at the first trailing closure and it's unlabeled. + if (argIdx == firstArgIdx && argLabel.empty()) { + hasUnlabeledFirstTrailingClosure = true; + break; + } + bool claimed = false; // We do this scan on a copy of prevParamIdx so that, if it fails, @@ -454,7 +466,7 @@ matchCallArguments(SmallVectorImpl &args, // Check for an exact label match. const auto ¶m = params[paramIdx]; - if (param.getLabel() == args[argIdx].getLabel()) { + if (param.getLabel() == argLabel) { parameterBindings[paramIdx].push_back(argIdx); claim(param.getLabel(), argIdx); @@ -474,81 +486,83 @@ matchCallArguments(SmallVectorImpl &args, } // Okay, we've matched all the labeled closures; scan backwards from - // there to match the unlabeled trailing closure. - Optional unlabeledParamIdx; - if (prevParamIdx > 0) { - unsigned paramIdx = prevParamIdx - 1; - - bool lastAcceptsTrailingClosure = - acceptsTrailingClosure(params[paramIdx]); - - // If the last parameter is defaulted, this might be - // an attempt to use a trailing closure with previous - // parameter that accepts a function type e.g. - // - // func foo(_: () -> Int, _ x: Int = 0) {} - // foo { 42 } - // - // FIXME: shouldn't this skip multiple arguments and look for - // something that acceptsTrailingClosure rather than just something - // with a function type? - if (!lastAcceptsTrailingClosure && paramIdx > 0 && - paramInfo.hasDefaultArgument(paramIdx)) { - auto paramType = params[paramIdx - 1].getPlainType(); - // If the parameter before defaulted last accepts. - if (paramType->is()) { - lastAcceptsTrailingClosure = true; - paramIdx -= 1; + // there to match the unlabeled first trailing closure, if there is one. + if (hasUnlabeledFirstTrailingClosure) { + Optional unlabeledParamIdx; + if (prevParamIdx > 0) { + unsigned paramIdx = prevParamIdx - 1; + + bool lastAcceptsTrailingClosure = + acceptsTrailingClosure(params[paramIdx]); + + // If the last parameter is defaulted, this might be + // an attempt to use a trailing closure with previous + // parameter that accepts a function type e.g. + // + // func foo(_: () -> Int, _ x: Int = 0) {} + // foo { 42 } + // + // FIXME: shouldn't this skip multiple arguments and look for + // something that acceptsTrailingClosure rather than just something + // with a function type? + if (!lastAcceptsTrailingClosure && paramIdx > 0 && + paramInfo.hasDefaultArgument(paramIdx)) { + auto paramType = params[paramIdx - 1].getPlainType(); + // If the parameter before defaulted last accepts. + if (paramType->is()) { + lastAcceptsTrailingClosure = true; + paramIdx -= 1; + } } + + if (lastAcceptsTrailingClosure) + unlabeledParamIdx = paramIdx; } - if (lastAcceptsTrailingClosure) - unlabeledParamIdx = paramIdx; - } - - // If there's no suitable last parameter to accept the trailing closure, - // notify the listener and bail if we need to. - if (!unlabeledParamIdx) { - // Try to use a specialized diagnostic for an extra closure. - bool isExtraClosure = false; - if (prevParamIdx == 0) { - isExtraClosure = true; - } else if (unlabeledArgIdx > 0) { - // Argument before the trailing closure. - unsigned prevArg = unlabeledArgIdx - 1; - auto &arg = args[prevArg]; - // If the argument before trailing closure matches - // last parameter, this is just a special case of - // an extraneous argument. - const auto param = params[prevParamIdx - 1]; - if (param.hasLabel() && param.getLabel() == arg.getLabel()) { + // If there's no suitable last parameter to accept the trailing closure, + // notify the listener and bail if we need to. + if (!unlabeledParamIdx) { + // Try to use a specialized diagnostic for an extra closure. + bool isExtraClosure = false; + if (prevParamIdx == 0) { isExtraClosure = true; + } else if (firstArgIdx > 0) { + // Argument before the trailing closure. + unsigned prevArg = firstArgIdx - 1; + auto &arg = args[prevArg]; + // If the argument before trailing closure matches + // last parameter, this is just a special case of + // an extraneous argument. + const auto param = params[prevParamIdx - 1]; + if (param.hasLabel() && param.getLabel() == arg.getLabel()) { + isExtraClosure = true; + } } - } - if (isExtraClosure) { - if (listener.extraArgument(unlabeledArgIdx)) - return true; - } else { - if (listener.trailingClosureMismatch(prevParamIdx - 1, - unlabeledArgIdx)) - return true; - } + if (isExtraClosure) { + if (listener.extraArgument(firstArgIdx)) + return true; + } else { + if (listener.trailingClosureMismatch(prevParamIdx - 1, + firstArgIdx)) + return true; + } - if (isExtraClosure) { - // Claim the unlabeled trailing closure without an associated - // parameter to suppress further complaints about it. - claim(Identifier(), unlabeledArgIdx, /*ignoreNameClash=*/true); - } else { - unlabeledParamIdx = prevParamIdx - 1; + if (isExtraClosure) { + // Claim the unlabeled trailing closure without an associated + // parameter to suppress further complaints about it. + claim(Identifier(), firstArgIdx, /*ignoreNameClash=*/true); + } else { + unlabeledParamIdx = prevParamIdx - 1; + } } - } - if (unlabeledParamIdx) { - // Claim the parameter/argument pair. - claim(params[*unlabeledParamIdx].getLabel(), unlabeledArgIdx, - /*ignoreNameClash=*/true); - parameterBindings[*unlabeledParamIdx].push_back(unlabeledArgIdx); + if (unlabeledParamIdx) { + // Claim the parameter/argument pair. + claim(params[*unlabeledParamIdx].getLabel(), firstArgIdx, + /*ignoreNameClash=*/true); + parameterBindings[*unlabeledParamIdx].push_back(firstArgIdx); + } } } @@ -1107,7 +1121,7 @@ ConstraintSystem::TypeMatchResult constraints::matchCallArguments( parameterBindings, locator); if (constraints::matchCallArguments( argsWithLabels, params, paramInfo, - argInfo->UnlabeledTrailingClosureIndex, + argInfo->FirstTrailingClosureIndex, cs.shouldAttemptFixes(), listener, parameterBindings)) return cs.getTypeMatchFailure(locator); @@ -8076,7 +8090,7 @@ bool ConstraintSystem::simplifyAppliedOverloadsImpl( if (!areConservativelyCompatibleArgumentLabels( choice, argsWithLabels, - argumentInfo->UnlabeledTrailingClosureIndex)) { + argumentInfo->FirstTrailingClosureIndex)) { labelMismatch = true; return false; } diff --git a/lib/Sema/ConstraintSystem.h b/lib/Sema/ConstraintSystem.h index a68185f4825f8..88dc14f1137a3 100644 --- a/lib/Sema/ConstraintSystem.h +++ b/lib/Sema/ConstraintSystem.h @@ -2183,7 +2183,7 @@ class ConstraintSystem { struct ArgumentInfo { ArrayRef Labels; - Optional UnlabeledTrailingClosureIndex; + Optional FirstTrailingClosureIndex; }; /// A mapping from the constraint locators for references to various @@ -4900,7 +4900,7 @@ class MatchCallArgumentListener { /// \param args The arguments. /// \param params The parameters. /// \param paramInfo Declaration-level information about the parameters. -/// \param unlabeledTrailingClosureIndex The index of an unlabeled trailing closure, +/// \param firstTrailingClosureIndex The index of the first trailing closure, /// if any. /// \param allowFixes Whether to allow fixes when matching arguments. /// @@ -4913,7 +4913,7 @@ class MatchCallArgumentListener { bool matchCallArguments(SmallVectorImpl &args, ArrayRef params, const ParameterListInfo ¶mInfo, - Optional unlabeledTrailingClosureIndex, + Optional firstTrailingClosureIndex, bool allowFixes, MatchCallArgumentListener &listener, SmallVectorImpl ¶meterBindings); diff --git a/utils/gyb_syntax_support/ExprNodes.py b/utils/gyb_syntax_support/ExprNodes.py index e746211f7bead..b4a0e90adcbd4 100644 --- a/utils/gyb_syntax_support/ExprNodes.py +++ b/utils/gyb_syntax_support/ExprNodes.py @@ -394,7 +394,7 @@ Child('Pattern', kind='Pattern'), ]), - # trailing-closure-element -> identifier ':' closure-expression + # trailing-closure-element -> identifier ':' closure-expr Node('MultipleTrailingClosureElement', kind='Syntax', children=[ Child('Label', kind='Token', @@ -409,8 +409,9 @@ Node('MultipleTrailingClosureElementList', kind='SyntaxCollection', element='MultipleTrailingClosureElement'), - # call-expr -> expr '(' call-argument-list ')' closure-expr? - # | expr closure-expr + # call-expr -> expr '(' call-argument-list ')' closure-expr? multiple-trailing-closure-element-list + # | expr closure-expr multiple-trailing-closure-element-list + # | expr trailing-closure-element multiple-trailing-closure-element-list Node('FunctionCallExpr', kind='Expr', children=[ Child('CalledExpression', kind='Expr'), @@ -424,11 +425,11 @@ is_optional=True), Child('AdditionalTrailingClosures', kind='MultipleTrailingClosureElementList', - collection_element_name='AdditionalTralingClosure', + collection_element_name='AdditionalTrailingClosure', is_optional=True), ]), - # subscript-expr -> expr '[' call-argument-list ']' closure-expr? + # subscript-expr -> expr '[' call-argument-list ']' closure-expr? multiple-trailing-closure-element-list Node('SubscriptExpr', kind='Expr', children=[ Child('CalledExpression', kind='Expr'), @@ -440,7 +441,7 @@ is_optional=True), Child('AdditionalTrailingClosures', kind='MultipleTrailingClosureElementList', - collection_element_name='AdditionalTralingClosure', + collection_element_name='AdditionalTrailingClosure', is_optional=True), ]), From c4eb8915b85e080217e2295395713306c9d177e2 Mon Sep 17 00:00:00 2001 From: Xiaodi Wu <13952+xwu@users.noreply.github.com> Date: Sun, 10 May 2020 12:34:02 -0400 Subject: [PATCH 2/2] Zap a warning when it's no longer necessary --- lib/Sema/MiscDiagnostics.cpp | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index f6bd269c6d072..fec7593bdcd20 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -3243,10 +3243,15 @@ static void checkStmtConditionTrailingClosure(ASTContext &ctx, const Expr *E) { if (!argsTy || argsTy->hasError()) return; SourceLoc closureLoc; - if (auto PE = dyn_cast(argsExpr)) - closureLoc = PE->getSubExpr()->getStartLoc(); - else if (auto TE = dyn_cast(argsExpr)) + if (auto TE = dyn_cast(argsExpr)) { + assert(TE->getNumElements() != 0 && "Unexpected empty TupleExpr"); + // Labeled trailing closure; this is fine. + // FIXME: It should also be fine even when the label is `_`. + if (TE->getElementNameLoc(0).isValid()) return; closureLoc = TE->getElements().back()->getStartLoc(); + } else if (auto PE = dyn_cast(argsExpr)) { + closureLoc = PE->getSubExpr()->getStartLoc(); + } Identifier closureLabel; if (auto TT = argsTy->getAs()) { @@ -3297,7 +3302,7 @@ static void checkStmtConditionTrailingClosure(ASTContext &ctx, const Expr *E) { /// Conditional statements, including 'for' or `switch` doesn't allow ambiguous /// trailing closures in these conditions part. Even if the parser can recover /// them, we force them to disambiguate. -// +/// /// E.g.: /// if let _ = arr?.map {$0+1} { ... } /// for _ in numbers.filter {$0 > 4} { ... }