Skip to content

Commit 3e28bbb

Browse files
hamishknighthamishknight
committed
Update for review feedback
- Remove OriginalArguments in favor of storing the pre-rewritten argument list, which simplifies things nicely - Adopt llvm::indexed_accessor_iterator
1 parent a1f127f commit 3e28bbb

15 files changed

+374
-599
lines changed

‎include/swift/AST/ArgumentList.h‎

Lines changed: 193 additions & 265 deletions
Large diffs are not rendered by default.

‎include/swift/Sema/ConstraintSystem.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -662,7 +662,7 @@ class FunctionArgApplyInfo {
662662

663663
/// Whether the argument is a trailing closure.
664664
bool isTrailingClosure() const {
665-
return ArgList->isRawTrailingClosureIndex(ArgIdx);
665+
return ArgList->isTrailingClosureIndex(ArgIdx);
666666
}
667667

668668
/// \returns The interface type for the function being applied. Note that this

‎lib/AST/ArgumentList.cpp‎

Lines changed: 34 additions & 181 deletions
Original file line numberDiff line numberDiff line change
@@ -39,103 +39,11 @@ bool Argument::isInOut() const {
3939
return ArgExpr->isSemanticallyInOutExpr();
4040
}
4141

42-
/// Attempt to extract the underlying array expr from an implicit variadic
43-
/// expansion expr.
44-
static ArrayExpr *getVariadicArrayFrom(const Expr *E) {
45-
auto *vargExpr = dyn_cast<VarargExpansionExpr>(E);
46-
if (!vargExpr || !vargExpr->isImplicit())
47-
return nullptr;
48-
auto *vargArray = dyn_cast<ArrayExpr>(vargExpr->getSubExpr());
49-
if (!vargArray || !vargArray->isImplicit())
50-
return nullptr;
51-
return vargArray;
52-
}
53-
54-
namespace {
55-
struct TrailingClosureInfo {
56-
Optional<unsigned> ArgIndex;
57-
bool HasMultiple;
58-
59-
TrailingClosureInfo() : HasMultiple(false) {}
60-
TrailingClosureInfo(Optional<unsigned> argIndex, bool hasMultiple)
61-
: ArgIndex(argIndex), HasMultiple(hasMultiple) {}
62-
};
63-
} // end anonymous namespace
64-
65-
static bool argExprHasValidSourceRange(const Expr *expr) {
66-
// Default arguments may have source info, but it's not a valid argument
67-
// source range.
68-
return !isa<DefaultArgumentExpr>(expr) && expr->getSourceRange().isValid();
69-
}
70-
71-
/// Whether a given argument expression in a non-implicit arg list is either a
72-
/// trailing closure, or is a variadic expansion containing a trailing closure.
73-
static bool isTrailingArgOfNonImplicitArgList(const Expr *argExpr,
74-
SourceLoc rparenLoc) {
75-
// If the argument has no valid source info, we can't count it as a trailing
76-
// closure.
77-
if (!argExprHasValidSourceRange(argExpr))
78-
return false;
79-
80-
// If the right paren loc is invalid, this is something like 'fn {}'.
81-
if (rparenLoc.isInvalid())
82-
return true;
83-
84-
// Otherwise the argument must come after the r-paren. Note we check the end
85-
// loc of the argument to match against variadic args that contain both
86-
// non-trailing and trailing closures.
87-
return rparenLoc.getOpaquePointerValue() <
88-
argExpr->getEndLoc().getOpaquePointerValue();
89-
}
90-
91-
static TrailingClosureInfo
92-
computeTrailingClosureInfo(ArrayRef<Argument> args, SourceLoc rparenLoc,
93-
bool isImplicit) {
94-
// Implicit argument lists never have trailing closures.
95-
if (isImplicit)
96-
return {};
97-
98-
auto iter = llvm::find_if(args, [&](const auto &arg) {
99-
return isTrailingArgOfNonImplicitArgList(arg.getExpr(), rparenLoc);
100-
});
101-
if (iter == args.end()) {
102-
assert(rparenLoc.isValid() &&
103-
"Explicit argument list with no parens and no trailing closures?");
104-
return {};
105-
}
106-
auto argIdx = iter - args.begin();
107-
108-
// For the variadic case we need to dig into the variadic array to figure out
109-
// if we have multiple trailing closures.
110-
if (auto *array = getVariadicArrayFrom(args[argIdx].getExpr())) {
111-
auto numTrailing = llvm::count_if(array->getElements(), [&](auto *expr) {
112-
return isTrailingArgOfNonImplicitArgList(expr, rparenLoc);
113-
});
114-
assert(numTrailing > 0 && "Variadic args didn't have trailing closure?");
115-
if (numTrailing > 1)
116-
return TrailingClosureInfo(argIdx, /*HasMultiple*/ true);
117-
}
118-
119-
// Otherwise, look for the next trailing closure if any.
120-
auto restOfArgs = args.drop_front(argIdx + 1);
121-
auto nextIter = llvm::find_if(restOfArgs, [&](const auto &arg) {
122-
return isTrailingArgOfNonImplicitArgList(arg.getExpr(), rparenLoc);
123-
});
124-
return TrailingClosureInfo(argIdx, /*HasMultiple*/ nextIter != args.end());
125-
}
126-
12742
ArgumentList *ArgumentList::create(ASTContext &ctx, SourceLoc lParenLoc,
12843
ArrayRef<Argument> args, SourceLoc rParenLoc,
12944
Optional<unsigned> firstTrailingClosureIndex,
130-
bool hasMultipleTrailingClosures,
131-
bool isImplicit, AllocationArena arena) {
132-
#ifndef NDEBUG
133-
auto trailingInfo = computeTrailingClosureInfo(args, rParenLoc, isImplicit);
134-
assert(trailingInfo.ArgIndex == firstTrailingClosureIndex);
135-
assert(trailingInfo.HasMultiple == hasMultipleTrailingClosures);
136-
#endif
137-
138-
assert(lParenLoc.isValid() == rParenLoc.isValid());
45+
bool isImplicit, ArgumentList *originalArgs,
46+
AllocationArena arena) {
13947
SmallVector<Expr *, 4> exprs;
14048
SmallVector<Identifier, 4> labels;
14149
SmallVector<SourceLoc, 4> labelLocs;
@@ -156,12 +64,13 @@ ArgumentList *ArgumentList::create(ASTContext &ctx, SourceLoc lParenLoc,
15664
if (!hasLabelLocs)
15765
labelLocs.clear();
15866

159-
auto numBytes = totalSizeToAlloc<Expr *, Identifier, SourceLoc>(
160-
exprs.size(), labels.size(), labelLocs.size());
67+
auto numBytes =
68+
totalSizeToAlloc<Expr *, Identifier, SourceLoc, ArgumentList *>(
69+
exprs.size(), labels.size(), labelLocs.size(), originalArgs ? 1 : 0);
16170
auto *mem = ctx.Allocate(numBytes, alignof(ArgumentList), arena);
162-
auto *argList = new (mem) ArgumentList(
163-
lParenLoc, rParenLoc, args.size(), firstTrailingClosureIndex,
164-
hasMultipleTrailingClosures, isImplicit, hasLabels, hasLabelLocs);
71+
auto *argList = new (mem)
72+
ArgumentList(lParenLoc, rParenLoc, args.size(), firstTrailingClosureIndex,
73+
originalArgs, isImplicit, hasLabels, hasLabelLocs);
16574

16675
std::uninitialized_copy(exprs.begin(), exprs.end(),
16776
argList->getExprsBuffer().begin());
@@ -173,24 +82,35 @@ ArgumentList *ArgumentList::create(ASTContext &ctx, SourceLoc lParenLoc,
17382
std::uninitialized_copy(labelLocs.begin(), labelLocs.end(),
17483
argList->getLabelLocsBuffer().begin());
17584
}
85+
if (originalArgs) {
86+
*argList->getTrailingObjects<ArgumentList *>() = originalArgs;
87+
}
17688
return argList;
17789
}
17890

179-
ArgumentList *ArgumentList::create(ASTContext &ctx, SourceLoc lParenLoc,
180-
ArrayRef<Argument> args, SourceLoc rParenLoc,
181-
bool isImplicit, AllocationArena arena) {
182-
auto trailingClosureInfo =
183-
computeTrailingClosureInfo(args, rParenLoc, isImplicit);
184-
return ArgumentList::create(
185-
ctx, lParenLoc, args, rParenLoc, trailingClosureInfo.ArgIndex,
186-
trailingClosureInfo.HasMultiple, isImplicit, arena);
91+
ArgumentList *
92+
ArgumentList::createParsed(ASTContext &ctx, SourceLoc lParenLoc,
93+
ArrayRef<Argument> args, SourceLoc rParenLoc,
94+
Optional<unsigned> firstTrailingClosureIndex) {
95+
return create(ctx, lParenLoc, args, rParenLoc, firstTrailingClosureIndex,
96+
/*implicit*/ false);
97+
}
98+
99+
ArgumentList *ArgumentList::createTypeChecked(ASTContext &ctx,
100+
ArgumentList *originalArgs,
101+
ArrayRef<Argument> newArgs) {
102+
return create(ctx, originalArgs->getLParenLoc(), newArgs,
103+
originalArgs->getRParenLoc(), /*trailingClosureIdx*/ None,
104+
originalArgs->isImplicit(), originalArgs);
187105
}
188106

189107
ArgumentList *ArgumentList::createImplicit(ASTContext &ctx, SourceLoc lParenLoc,
190108
ArrayRef<Argument> args,
191109
SourceLoc rParenLoc,
192110
AllocationArena arena) {
193-
return create(ctx, lParenLoc, args, rParenLoc, /*implicit*/ true, arena);
111+
return create(ctx, lParenLoc, args, rParenLoc,
112+
/*firstTrailingClosureIdx*/ None, /*implicit*/ true,
113+
/*originalArgs*/ nullptr, arena);
194114
}
195115

196116
ArgumentList *ArgumentList::createImplicit(ASTContext &ctx,
@@ -258,24 +178,13 @@ SourceRange ArgumentList::getSourceRange() const {
258178
}
259179
}
260180
auto end = RParenLoc;
261-
if (hasAnyTrailingClosures()) {
262-
// If we have trailing closures, the end loc is the end loc of the last
263-
// trailing closure.
264-
for (auto arg : llvm::reverse(*this)) {
265-
if (isTrailingArgOfNonImplicitArgList(arg.getExpr(), RParenLoc)) {
266-
end = arg.getEndLoc();
267-
break;
268-
}
269-
}
270-
} else if (RParenLoc.isInvalid()) {
271-
// If we don't have trailing closures and the r-paren loc is invalid, this
272-
// is an implicit argument list. Take the last valid argument loc.
273-
assert(isImplicit());
274-
for (auto arg : llvm::reverse(*this)) {
275-
if (argExprHasValidSourceRange(arg.getExpr())) {
276-
end = arg.getEndLoc();
181+
if (hasAnyTrailingClosures() || RParenLoc.isInvalid()) {
182+
// Scan backward for the first valid source loc. We use getOriginalArgs to
183+
// filter out default arguments and get accurate trailing closure info.
184+
for (auto arg : llvm::reverse(*getOriginalArgs())) {
185+
end = arg.getEndLoc();
186+
if (end.isValid())
277187
break;
278-
}
279188
}
280189
}
281190
if (start.isInvalid() || end.isInvalid())
@@ -393,59 +302,3 @@ bool ArgumentList::matches(ArrayRef<AnyFunctionType::Param> params,
393302
}
394303
return true;
395304
}
396-
397-
OriginalArguments ArgumentList::getOriginalArguments() const {
398-
// We need to sort out the trailing closures separately to handle cases like:
399-
//
400-
// func foo(fn: () -> Void, x: Int = 0) {}
401-
// foo(x: 0) {}
402-
//
403-
// where we currently allow the re-ordering of a trailing closure argument
404-
// such that it appears as the first argument in the type-checked AST. To
405-
// remedy this, separate out the trailing closures and make sure to append
406-
// them after the regular arguments.
407-
OriginalArguments::Storage newArgs;
408-
SmallVector<Argument, 1> trailingClosures;
409-
410-
auto addArg = [&](Argument arg) {
411-
if (hasAnyTrailingClosures() &&
412-
isTrailingArgOfNonImplicitArgList(arg.getExpr(), getRParenLoc())) {
413-
trailingClosures.push_back(arg);
414-
return;
415-
}
416-
newArgs.push_back(arg);
417-
};
418-
for (auto arg : *this) {
419-
auto *expr = arg.getExpr();
420-
if (isa<DefaultArgumentExpr>(expr))
421-
continue;
422-
423-
if (auto *vargArray = getVariadicArrayFrom(expr)) {
424-
auto elts = vargArray->getElements();
425-
for (auto idx : indices(elts)) {
426-
// The first element in a variadic expansion takes the argument label,
427-
// the rest are unlabeled.
428-
if (idx == 0) {
429-
addArg(Argument(arg.getLabelLoc(), arg.getLabel(), elts[idx]));
430-
} else {
431-
addArg(Argument::unlabeled(elts[idx]));
432-
}
433-
}
434-
continue;
435-
}
436-
addArg(arg);
437-
}
438-
Optional<unsigned> trailingClosureIdx;
439-
if (!trailingClosures.empty())
440-
trailingClosureIdx = newArgs.size();
441-
442-
newArgs.append(trailingClosures.begin(), trailingClosures.end());
443-
auto origArgs = OriginalArguments(std::move(newArgs), trailingClosureIdx);
444-
#ifndef NDEBUG
445-
auto trailingInfo = computeTrailingClosureInfo(origArgs.getArray(),
446-
getRParenLoc(), isImplicit());
447-
assert(trailingInfo.ArgIndex == trailingClosureIdx);
448-
assert(trailingInfo.HasMultiple == origArgs.hasMultipleTrailingClosures());
449-
#endif
450-
return origArgs;
451-
}

‎lib/IDE/ExprContextAnalysis.cpp‎

Lines changed: 26 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -212,20 +212,32 @@ class CCExprRemover: public ASTWalker, public ExprVisitor<CCExprRemover, Expr *>
212212
CCExprRemover(ASTContext &Ctx) : Ctx(Ctx) {}
213213

214214
Expr *visitCallExpr(CallExpr *E) {
215-
auto *args = E->getArgs();
215+
auto *args = E->getArgs()->getOriginalArgs();
216216

217+
Optional<unsigned> newTrailingClosureIdx;
217218
SmallVector<Argument, 4> newArgs;
218-
for (auto arg : *args) {
219+
for (auto idx : indices(*args)) {
220+
// Update the trailing closure index if we have one.
221+
if (args->hasAnyTrailingClosures() &&
222+
idx == *args->getFirstTrailingClosureIndex()) {
223+
newTrailingClosureIdx = newArgs.size();
224+
}
225+
auto arg = args->get(idx);
219226
if (!isa<CodeCompletionExpr>(arg.getExpr()))
220227
newArgs.push_back(arg);
221228
}
222229
if (newArgs.size() == args->size())
223230
return E;
224231

232+
// If we ended up removing the last trailing closure, drop the index.
233+
if (newTrailingClosureIdx && *newTrailingClosureIdx == newArgs.size())
234+
newTrailingClosureIdx = None;
235+
225236
Removed = true;
226-
auto *argList = ArgumentList::create(Ctx, args->getLParenLoc(), newArgs,
227-
args->getRParenLoc(),
228-
args->isImplicit());
237+
238+
auto *argList = ArgumentList::create(
239+
Ctx, args->getLParenLoc(), newArgs, args->getRParenLoc(),
240+
newTrailingClosureIdx, E->isImplicit());
229241
return CallExpr::create(Ctx, E->getFn(), argList, E->isImplicit());
230242
}
231243

@@ -728,9 +740,8 @@ static bool getPositionInTuple(DeclContext &DC, TupleExpr *TE, Expr *CCExpr,
728740
/// \returns the position index number on success, \c None if \p CCExpr is not
729741
/// a part of \p Args.
730742
static Optional<unsigned>
731-
getPositionInParams(DeclContext &DC, const OriginalArguments &Args,
732-
Expr *CCExpr, ArrayRef<AnyFunctionType::Param> Params,
733-
bool Lenient) {
743+
getPositionInParams(DeclContext &DC, const ArgumentList *Args, Expr *CCExpr,
744+
ArrayRef<AnyFunctionType::Param> Params, bool Lenient) {
734745
auto &SM = DC.getASTContext().SourceMgr;
735746
unsigned PosInParams = 0;
736747
unsigned PosInArgs = 0;
@@ -740,11 +751,11 @@ getPositionInParams(DeclContext &DC, const OriginalArguments &Args,
740751
// For each argument, we try to find a matching parameter either by matching
741752
// argument labels, in which case PosInParams may be advanced by more than 1,
742753
// or by advancing PosInParams and PosInArgs both by 1.
743-
for (; PosInArgs < Args.size(); ++PosInArgs) {
744-
if (!SM.isBeforeInBuffer(Args[PosInArgs].getExpr()->getEndLoc(),
754+
for (; PosInArgs < Args->size(); ++PosInArgs) {
755+
if (!SM.isBeforeInBuffer(Args->getExpr(PosInArgs)->getEndLoc(),
745756
CCExpr->getStartLoc())) {
746757
// The arg is after the code completion position. Stop.
747-
if (LastParamWasVariadic && Args[PosInArgs].getLabel().empty()) {
758+
if (LastParamWasVariadic && Args->getLabel(PosInArgs).empty()) {
748759
// If the last parameter was variadic and this argument stands by itself
749760
// without a label, assume that it belongs to the previous vararg
750761
// list.
@@ -753,7 +764,7 @@ getPositionInParams(DeclContext &DC, const OriginalArguments &Args,
753764
break;
754765
}
755766

756-
auto ArgName = Args[PosInArgs].getLabel();
767+
auto ArgName = Args->getLabel(PosInArgs);
757768
// If the last parameter we matched was variadic, we claim all following
758769
// unlabeled arguments for that variadic parameter -> advance PosInArgs but
759770
// not PosInParams.
@@ -778,7 +789,7 @@ getPositionInParams(DeclContext &DC, const OriginalArguments &Args,
778789
}
779790
}
780791

781-
if (!AdvancedPosInParams && Args.isTrailingClosureIndex(PosInArgs)) {
792+
if (!AdvancedPosInParams && Args->isTrailingClosureIndex(PosInArgs)) {
782793
// If the argument is a trailing closure, it can't match non-function
783794
// parameters. Advance to the next function parameter.
784795
for (unsigned i = PosInParams; i < Params.size(); ++i) {
@@ -802,7 +813,7 @@ getPositionInParams(DeclContext &DC, const OriginalArguments &Args,
802813
}
803814
}
804815
}
805-
if (PosInArgs < Args.size() && PosInParams < Params.size()) {
816+
if (PosInArgs < Args->size() && PosInParams < Params.size()) {
806817
// We didn't search until the end, so we found a position in Params. Success
807818
return PosInParams;
808819
} else {
@@ -900,7 +911,7 @@ class ExprContextAnalyzer {
900911
llvm::SmallVector<Optional<unsigned>, 2> posInParams;
901912
{
902913
bool found = false;
903-
auto originalArgs = Args->getOriginalArguments();
914+
auto *originalArgs = Args->getOriginalArgs();
904915
for (auto &typeAndDecl : Candidates) {
905916
Optional<unsigned> pos = getPositionInParams(
906917
*DC, originalArgs, ParsedExpr, typeAndDecl.Type->getParams(),

‎lib/IDE/Formatting.cpp‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -666,8 +666,7 @@ class RangeWalker: protected ASTWalker {
666666
return Stop;
667667
}
668668
} else {
669-
auto OriginalArgs = Args->getOriginalArguments();
670-
handleImplicitRange(OriginalArgs.getTrailingSourceRange(),
669+
handleImplicitRange(Args->getOriginalArgs()->getTrailingSourceRange(),
671670
ContextLoc);
672671
}
673672
}
@@ -2692,7 +2691,7 @@ class FormatWalker : public ASTWalker {
26922691

26932692
return getIndentContextFrom(CE, ContextLoc);
26942693
}
2695-
auto ClosuresRange = Args->getOriginalArguments().getTrailingSourceRange();
2694+
auto ClosuresRange = Args->getOriginalArgs()->getTrailingSourceRange();
26962695
if (!overlapsTarget(ClosuresRange) && !TrailingTarget)
26972696
return None;
26982697

@@ -2731,8 +2730,7 @@ class FormatWalker : public ASTWalker {
27312730
}
27322731

27332732
ListAligner Aligner(SM, TargetLocation, ContextLoc, L, R);
2734-
auto OriginalArgs = Args->getOriginalArguments();
2735-
for (auto Arg : OriginalArgs.getNonTrailingArgs()) {
2733+
for (auto Arg : Args->getOriginalArgs()->getNonTrailingArgs()) {
27362734
SourceRange ElemRange = Arg.getLabelLoc();
27372735
auto *Elem = Arg.getExpr();
27382736
assert(Elem);

0 commit comments

Comments
 (0)