Skip to content

Commit f70c43a

Browse files
xedinxedin
authored
Merge pull request swiftlang#23085 from xedin/diag-OoO-arguments
[ConstraintSystem] Diagnose out-of-order arguments via fixes
2 parents da50d66 + 2e14bec commit f70c43a

File tree

8 files changed

+246
-138
lines changed

8 files changed

+246
-138
lines changed

‎lib/Sema/CSDiag.cpp‎

Lines changed: 6 additions & 115 deletions
Original file line numberDiff line numberDiff line change
@@ -3369,8 +3369,6 @@ class ArgumentMatcher : public MatchCallArgumentListener {
33693369
// Stores parameter bindings determined by call to matchCallArguments.
33703370
SmallVector<ParamBinding, 4> Bindings;
33713371

3372-
unsigned NumLabelFailures = 0;
3373-
33743372
public:
33753373
ArgumentMatcher(Expr *fnExpr, Expr *argExpr,
33763374
ArrayRef<AnyFunctionType::Param> &params,
@@ -3547,130 +3545,23 @@ class ArgumentMatcher : public MatchCallArgumentListener {
35473545
}
35483546

35493547
bool missingLabel(unsigned paramIdx) override {
3550-
++NumLabelFailures;
35513548
return false;
35523549
}
35533550

35543551
bool extraneousLabel(unsigned paramIdx) override {
3555-
++NumLabelFailures;
35563552
return false;
35573553
}
35583554

35593555
bool incorrectLabel(unsigned paramIdx) override {
3560-
++NumLabelFailures;
35613556
return false;
35623557
}
35633558

3564-
void outOfOrderArgument(unsigned argIdx, unsigned prevArgIdx) override {
3565-
auto tuple = cast<TupleExpr>(ArgExpr);
3566-
Identifier first = tuple->getElementName(argIdx);
3567-
Identifier second = tuple->getElementName(prevArgIdx);
3568-
3569-
// If we've seen label failures and now there is an out-of-order
3570-
// parameter (or even worse - OoO parameter with label re-naming),
3571-
// we most likely have no idea what would be the best
3572-
// diagnostic for this situation, so let's just try to re-label.
3573-
auto shouldDiagnoseOoO = [&](Identifier newLabel, Identifier oldLabel) {
3574-
if (NumLabelFailures > 0)
3575-
return false;
3576-
3577-
unsigned actualIndex = prevArgIdx;
3578-
for (; actualIndex != argIdx; ++actualIndex) {
3579-
// Looks like new position (excluding defaulted parameters),
3580-
// has a valid label.
3581-
if (newLabel == Parameters[actualIndex].getLabel())
3582-
break;
3583-
3584-
// If we are moving the the position with a different label
3585-
// and there is no default value for it, can't diagnose the
3586-
// problem as a simple re-ordering.
3587-
if (!DefaultMap.test(actualIndex))
3588-
return false;
3589-
}
3590-
3591-
for (unsigned i = actualIndex + 1, n = Parameters.size(); i != n; ++i) {
3592-
if (oldLabel == Parameters[i].getLabel())
3593-
break;
3594-
3595-
if (!DefaultMap.test(i))
3596-
return false;
3597-
}
3598-
3599-
return true;
3600-
};
3601-
3602-
if (!shouldDiagnoseOoO(first, second)) {
3603-
SmallVector<Identifier, 8> paramLabels;
3604-
llvm::transform(Parameters, std::back_inserter(paramLabels),
3605-
[](const AnyFunctionType::Param &param) {
3606-
return param.getLabel();
3607-
});
3608-
relabelArguments(paramLabels);
3609-
return;
3610-
}
3611-
3612-
// Build a mapping from arguments to parameters.
3613-
SmallVector<unsigned, 4> argBindings(tuple->getNumElements());
3614-
for (unsigned paramIdx = 0; paramIdx != Bindings.size(); ++paramIdx) {
3615-
for (auto argIdx : Bindings[paramIdx])
3616-
argBindings[argIdx] = paramIdx;
3617-
}
3618-
3619-
auto argRange = [&](unsigned argIdx, Identifier label) -> SourceRange {
3620-
auto range = tuple->getElement(argIdx)->getSourceRange();
3621-
if (!label.empty())
3622-
range.Start = tuple->getElementNameLoc(argIdx);
3623-
3624-
unsigned paramIdx = argBindings[argIdx];
3625-
if (Bindings[paramIdx].size() > 1)
3626-
range.End = tuple->getElement(Bindings[paramIdx].back())->getEndLoc();
3627-
3628-
return range;
3629-
};
3630-
3631-
auto firstRange = argRange(argIdx, first);
3632-
auto secondRange = argRange(prevArgIdx, second);
3633-
3634-
SourceLoc diagLoc = firstRange.Start;
3635-
3636-
auto addFixIts = [&](InFlightDiagnostic diag) {
3637-
diag.highlight(firstRange).highlight(secondRange);
3638-
3639-
// Move the misplaced argument by removing it from one location and
3640-
// inserting it in another location. To maintain argument comma
3641-
// separation, since the argument is always moving to an earlier index
3642-
// the preceding comma and whitespace is removed and a new trailing
3643-
// comma and space is inserted with the moved argument.
3644-
auto &SM = TC.Context.SourceMgr;
3645-
auto text = SM.extractText(
3646-
Lexer::getCharSourceRangeFromSourceRange(SM, firstRange));
3647-
3648-
auto removalRange =
3649-
SourceRange(Lexer::getLocForEndOfToken(
3650-
SM, tuple->getElement(argIdx - 1)->getEndLoc()),
3651-
firstRange.End);
3652-
diag.fixItRemove(removalRange);
3653-
diag.fixItInsert(secondRange.Start, text.str() + ", ");
3654-
};
3655-
3656-
// There are 4 diagnostic messages variations depending on
3657-
// labeled/unlabeled arguments.
3658-
if (first.empty() && second.empty()) {
3659-
addFixIts(TC.diagnose(diagLoc,
3660-
diag::argument_out_of_order_unnamed_unnamed,
3661-
argIdx + 1, prevArgIdx + 1));
3662-
} else if (first.empty() && !second.empty()) {
3663-
addFixIts(TC.diagnose(diagLoc, diag::argument_out_of_order_unnamed_named,
3664-
argIdx + 1, second));
3665-
} else if (!first.empty() && second.empty()) {
3666-
addFixIts(TC.diagnose(diagLoc, diag::argument_out_of_order_named_unnamed,
3667-
first, prevArgIdx + 1));
3668-
} else {
3669-
addFixIts(TC.diagnose(diagLoc, diag::argument_out_of_order_named_named,
3670-
first, second));
3671-
}
3672-
3673-
Diagnosed = true;
3559+
bool outOfOrderArgument(unsigned argIdx, unsigned prevArgIdx) override {
3560+
auto &cs = CandidateInfo.CS;
3561+
OutOfOrderArgumentFailure failure(nullptr, cs, argIdx, prevArgIdx, Bindings,
3562+
cs.getConstraintLocator(ArgExpr));
3563+
Diagnosed = failure.diagnoseAsError();
3564+
return true;
36743565
}
36753566

36763567
bool relabelArguments(ArrayRef<Identifier> newNames) override {

‎lib/Sema/CSDiagnostics.cpp‎

Lines changed: 90 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,20 @@ Expr *FailureDiagnostic::findParentExpr(Expr *subExpr) const {
8686
return E ? E->getParentMap()[subExpr] : nullptr;
8787
}
8888

89+
Expr *FailureDiagnostic::getArgumentExprFor(Expr *anchor) const {
90+
if (auto *UDE = dyn_cast<UnresolvedDotExpr>(anchor)) {
91+
if (auto *call = dyn_cast_or_null<CallExpr>(findParentExpr(UDE)))
92+
return call->getArg();
93+
} else if (auto *UME = dyn_cast<UnresolvedMemberExpr>(anchor)) {
94+
return UME->getArgument();
95+
} else if (auto *call = dyn_cast<CallExpr>(anchor)) {
96+
return call->getArg();
97+
} else if (auto *SE = dyn_cast<SubscriptExpr>(anchor)) {
98+
return SE->getIndex();
99+
}
100+
return nullptr;
101+
}
102+
89103
Type RequirementFailure::getOwnerType() const {
90104
return getType(getRawAnchor())
91105
->getInOutObjectType()
@@ -369,18 +383,7 @@ bool LabelingFailure::diagnoseAsError() {
369383
auto &cs = getConstraintSystem();
370384
auto *anchor = getRawAnchor();
371385

372-
Expr *argExpr = nullptr;
373-
if (auto *UDE = dyn_cast<UnresolvedDotExpr>(anchor)) {
374-
if (auto *call = dyn_cast_or_null<CallExpr>(findParentExpr(UDE)))
375-
argExpr = call->getArg();
376-
} else if (auto *UME = dyn_cast<UnresolvedMemberExpr>(anchor)) {
377-
argExpr = UME->getArgument();
378-
} else if (auto *call = dyn_cast<CallExpr>(anchor)) {
379-
argExpr = call->getArg();
380-
} else if (auto *SE = dyn_cast<SubscriptExpr>(anchor)) {
381-
argExpr = SE->getIndex();
382-
}
383-
386+
auto *argExpr = getArgumentExprFor(anchor);
384387
if (!argExpr)
385388
return false;
386389

@@ -2133,3 +2136,78 @@ bool ClosureParamDestructuringFailure::diagnoseAsError() {
21332136
.fixItInsert(bodyLoc, OS.str());
21342137
return true;
21352138
}
2139+
2140+
bool OutOfOrderArgumentFailure::diagnoseAsError() {
2141+
auto *anchor = getRawAnchor();
2142+
auto *argExpr = isa<TupleExpr>(anchor) ? anchor : getArgumentExprFor(anchor);
2143+
if (!argExpr)
2144+
return false;
2145+
2146+
auto *tuple = cast<TupleExpr>(argExpr);
2147+
2148+
Identifier first = tuple->getElementName(ArgIdx);
2149+
Identifier second = tuple->getElementName(PrevArgIdx);
2150+
2151+
// Build a mapping from arguments to parameters.
2152+
SmallVector<unsigned, 4> argBindings(tuple->getNumElements());
2153+
for (unsigned paramIdx = 0; paramIdx != Bindings.size(); ++paramIdx) {
2154+
for (auto argIdx : Bindings[paramIdx])
2155+
argBindings[argIdx] = paramIdx;
2156+
}
2157+
2158+
auto argRange = [&](unsigned argIdx, Identifier label) -> SourceRange {
2159+
auto range = tuple->getElement(argIdx)->getSourceRange();
2160+
if (!label.empty())
2161+
range.Start = tuple->getElementNameLoc(argIdx);
2162+
2163+
unsigned paramIdx = argBindings[argIdx];
2164+
if (Bindings[paramIdx].size() > 1)
2165+
range.End = tuple->getElement(Bindings[paramIdx].back())->getEndLoc();
2166+
2167+
return range;
2168+
};
2169+
2170+
auto firstRange = argRange(ArgIdx, first);
2171+
auto secondRange = argRange(PrevArgIdx, second);
2172+
2173+
SourceLoc diagLoc = firstRange.Start;
2174+
2175+
auto addFixIts = [&](InFlightDiagnostic diag) {
2176+
diag.highlight(firstRange).highlight(secondRange);
2177+
2178+
// Move the misplaced argument by removing it from one location and
2179+
// inserting it in another location. To maintain argument comma
2180+
// separation, since the argument is always moving to an earlier index
2181+
// the preceding comma and whitespace is removed and a new trailing
2182+
// comma and space is inserted with the moved argument.
2183+
auto &SM = getASTContext().SourceMgr;
2184+
auto text = SM.extractText(
2185+
Lexer::getCharSourceRangeFromSourceRange(SM, firstRange));
2186+
2187+
auto removalRange =
2188+
SourceRange(Lexer::getLocForEndOfToken(
2189+
SM, tuple->getElement(ArgIdx - 1)->getEndLoc()),
2190+
firstRange.End);
2191+
diag.fixItRemove(removalRange);
2192+
diag.fixItInsert(secondRange.Start, text.str() + ", ");
2193+
};
2194+
2195+
// There are 4 diagnostic messages variations depending on
2196+
// labeled/unlabeled arguments.
2197+
if (first.empty() && second.empty()) {
2198+
addFixIts(emitDiagnostic(diagLoc,
2199+
diag::argument_out_of_order_unnamed_unnamed,
2200+
ArgIdx + 1, PrevArgIdx + 1));
2201+
} else if (first.empty() && !second.empty()) {
2202+
addFixIts(emitDiagnostic(diagLoc, diag::argument_out_of_order_unnamed_named,
2203+
ArgIdx + 1, second));
2204+
} else if (!first.empty() && second.empty()) {
2205+
addFixIts(emitDiagnostic(diagLoc, diag::argument_out_of_order_named_unnamed,
2206+
first, PrevArgIdx + 1));
2207+
} else {
2208+
addFixIts(emitDiagnostic(diagLoc, diag::argument_out_of_order_named_named,
2209+
first, second));
2210+
}
2211+
2212+
return true;
2213+
}

‎lib/Sema/CSDiagnostics.h‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,10 @@ class FailureDiagnostic {
151151
/// in the root expression or `nullptr` otherwise.
152152
Expr *findParentExpr(Expr *subExpr) const;
153153

154+
/// \returns An argument expression if given anchor is a call, member
155+
/// reference or subscript, nullptr otherwise.
156+
Expr *getArgumentExprFor(Expr *anchor) const;
157+
154158
private:
155159
/// Compute anchor expression associated with current diagnostic.
156160
std::pair<Expr *, bool> computeAnchor() const;
@@ -899,6 +903,26 @@ class MissingArgumentsFailure final : public FailureDiagnostic {
899903
bool diagnoseTrailingClosure(ClosureExpr *closure);
900904
};
901905

906+
class OutOfOrderArgumentFailure final : public FailureDiagnostic {
907+
using ParamBinding = SmallVector<unsigned, 1>;
908+
909+
unsigned ArgIdx;
910+
unsigned PrevArgIdx;
911+
912+
SmallVector<ParamBinding, 4> Bindings;
913+
914+
public:
915+
OutOfOrderArgumentFailure(Expr *root, ConstraintSystem &cs,
916+
unsigned argIdx,
917+
unsigned prevArgIdx,
918+
ArrayRef<ParamBinding> bindings,
919+
ConstraintLocator *locator)
920+
: FailureDiagnostic(root, cs, locator), ArgIdx(argIdx),
921+
PrevArgIdx(prevArgIdx), Bindings(bindings.begin(), bindings.end()) {}
922+
923+
bool diagnoseAsError() override;
924+
};
925+
902926
/// Diagnose an attempt to destructure a single tuple closure parameter
903927
/// into a multiple (possibly anonymous) arguments e.g.
904928
///

‎lib/Sema/CSFix.cpp‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -371,3 +371,16 @@ AddMissingArguments::create(ConstraintSystem &cs, FunctionType *funcType,
371371
void *mem = cs.getAllocator().Allocate(size, alignof(AddMissingArguments));
372372
return new (mem) AddMissingArguments(cs, funcType, synthesizedArgs, locator);
373373
}
374+
375+
bool MoveOutOfOrderArgument::diagnose(Expr *root, bool asNote) const {
376+
OutOfOrderArgumentFailure failure(root, getConstraintSystem(), ArgIdx,
377+
PrevArgIdx, Bindings, getLocator());
378+
return failure.diagnose(asNote);
379+
}
380+
381+
MoveOutOfOrderArgument *MoveOutOfOrderArgument::create(
382+
ConstraintSystem &cs, unsigned argIdx, unsigned prevArgIdx,
383+
ArrayRef<ParamBinding> bindings, ConstraintLocator *locator) {
384+
return new (cs.getAllocator())
385+
MoveOutOfOrderArgument(cs, argIdx, prevArgIdx, bindings, locator);
386+
}

‎lib/Sema/CSFix.h‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,9 @@ enum class FixKind : uint8_t {
129129

130130
/// Allow single tuple closure parameter destructuring into N arguments.
131131
AllowClosureParameterDestructuring,
132+
133+
/// If there is out-of-order argument, let's fix that by re-ordering.
134+
MoveOutOfOrderArgument,
132135
};
133136

134137
class ConstraintFix {
@@ -681,6 +684,35 @@ class AddMissingArguments final
681684
}
682685
};
683686

687+
class MoveOutOfOrderArgument final : public ConstraintFix {
688+
using ParamBinding = SmallVector<unsigned, 1>;
689+
690+
unsigned ArgIdx;
691+
unsigned PrevArgIdx;
692+
693+
SmallVector<ParamBinding, 4> Bindings;
694+
695+
MoveOutOfOrderArgument(ConstraintSystem &cs, unsigned argIdx,
696+
unsigned prevArgIdx, ArrayRef<ParamBinding> bindings,
697+
ConstraintLocator *locator)
698+
: ConstraintFix(cs, FixKind::MoveOutOfOrderArgument, locator),
699+
ArgIdx(argIdx), PrevArgIdx(prevArgIdx),
700+
Bindings(bindings.begin(), bindings.end()) {}
701+
702+
public:
703+
std::string getName() const override {
704+
return "move out-of-order argument to correct position";
705+
}
706+
707+
bool diagnose(Expr *root, bool asNote = false) const override;
708+
709+
static MoveOutOfOrderArgument *create(ConstraintSystem &cs,
710+
unsigned argIdx,
711+
unsigned prevArgIdx,
712+
ArrayRef<ParamBinding> bindings,
713+
ConstraintLocator *locator);
714+
};
715+
684716
} // end namespace constraints
685717
} // end namespace swift
686718

0 commit comments

Comments
 (0)