Skip to content

Commit af34984

Browse files
xedinxedin
authored
Merge pull request #21682 from xedin/diagnose-subscript-misuse-via-fixes
[Diagnostics] Diagnose subscript operator misuse via fixes
2 parents adfd189 + 91e97a0 commit af34984

File tree

8 files changed

+139
-132
lines changed

8 files changed

+139
-132
lines changed

‎lib/Sema/CSDiag.cpp

Lines changed: 0 additions & 109 deletions
Original file line numberDiff line numberDiff line change
@@ -572,10 +572,6 @@ class FailureDiagnosis :public ASTVisitor<FailureDiagnosis, /*exprresult*/bool>{
572572

573573
bool diagnoseSubscriptErrors(SubscriptExpr *SE, bool performingSet);
574574

575-
/// Diagnose the usage of 'subscript' instead of the operator when calling
576-
/// a subscript and offer a fixit if the inputs are compatible.
577-
bool diagnoseSubscriptMisuse(ApplyExpr *callExpr);
578-
579575
/// Try to diagnose common errors involving implicitly non-escaping parameters
580576
/// of function type, giving more specific and simpler diagnostics, attaching
581577
/// notes on the parameter, and offering fixits to insert @escaping. Returns
@@ -5089,105 +5085,6 @@ bool FailureDiagnosis::diagnoseCallContextualConversionErrors(
50895085
exprType);
50905086
}
50915087

5092-
bool FailureDiagnosis::diagnoseSubscriptMisuse(ApplyExpr *callExpr) {
5093-
auto UDE = dyn_cast<UnresolvedDotExpr>(callExpr->getFn());
5094-
if (!UDE)
5095-
return false;
5096-
5097-
auto baseExpr = UDE->getBase();
5098-
if (!baseExpr || UDE->getName().getBaseName() != getTokenText(tok::kw_subscript))
5099-
return false;
5100-
5101-
auto baseType = CS.getType(baseExpr);
5102-
// Look up subscript declarations.
5103-
auto lookup = CS.lookupMember(baseType->getRValueType(),
5104-
DeclName(DeclBaseName::createSubscript()));
5105-
auto nonSubscrLookup = CS.lookupMember(baseType->getRValueType(),
5106-
UDE->getName());
5107-
// Make sure we only found subscript declarations. If not, the problem
5108-
// is different - return.
5109-
if (lookup.empty() || !nonSubscrLookup.empty())
5110-
return false;
5111-
// Try to resolve a type of the argument expression.
5112-
auto argExpr = typeCheckChildIndependently(callExpr->getArg(),
5113-
Type(), CTP_CallArgument);
5114-
if (!argExpr)
5115-
return CS.TC.Diags.hadAnyError();
5116-
5117-
SmallVector<Identifier, 2> scratch;
5118-
ArrayRef<Identifier> argLabels = callExpr->getArgumentLabels(scratch);
5119-
SmallVector<OverloadChoice, 2> choices;
5120-
5121-
for (auto candidate : lookup)
5122-
choices.push_back(OverloadChoice(baseType, candidate.getValueDecl(),
5123-
UDE->getFunctionRefKind()));
5124-
CalleeCandidateInfo candidateInfo(baseType, choices,
5125-
callArgHasTrailingClosure(argExpr),
5126-
CS, true);
5127-
5128-
auto params = decomposeArgType(CS.getType(argExpr), argLabels);
5129-
using ClosenessPair = CalleeCandidateInfo::ClosenessResultTy;
5130-
5131-
candidateInfo.filterList([&](OverloadCandidate cand) -> ClosenessPair {
5132-
auto candFuncType = cand.getFunctionType();
5133-
if (!candFuncType)
5134-
return {CC_GeneralMismatch, {}};
5135-
5136-
auto candParams = candFuncType->getParams();
5137-
if (params.size() != candParams.size())
5138-
return {CC_GeneralMismatch, {}};
5139-
5140-
for (unsigned i = 0, e = params.size(); i < e; i ++) {
5141-
if (CS.TC.isConvertibleTo(params[i].getOldType(),
5142-
candParams[i].getOldType(), CS.DC) ||
5143-
candParams[i].getOldType()->is<GenericTypeParamType>())
5144-
continue;
5145-
return {CC_GeneralMismatch, {}};
5146-
}
5147-
return {CC_ExactMatch, {}};
5148-
});
5149-
5150-
auto *locator = CS.getConstraintLocator(UDE, ConstraintLocator::Member);
5151-
auto memberRange = baseExpr->getSourceRange();
5152-
if (locator)
5153-
locator = simplifyLocator(CS, locator, memberRange);
5154-
auto nameLoc = DeclNameLoc(memberRange.Start);
5155-
5156-
auto diag = diagnose(baseExpr->getLoc(),
5157-
diag::could_not_find_subscript_member_did_you_mean,
5158-
baseType);
5159-
diag.highlight(memberRange).highlight(nameLoc.getSourceRange());
5160-
5161-
auto showNote = [&]() {
5162-
diag.flush();
5163-
if (candidateInfo.size() == 1)
5164-
diagnose(candidateInfo.candidates.front().getDecl(),
5165-
diag::kind_declared_here, DescriptiveDeclKind::Subscript);
5166-
};
5167-
if (candidateInfo.closeness != CC_ExactMatch) {
5168-
showNote();
5169-
return true;
5170-
}
5171-
auto toCharSourceRange = Lexer::getCharSourceRangeFromSourceRange;
5172-
auto lastArgSymbol = toCharSourceRange(CS.TC.Context.SourceMgr,
5173-
argExpr->getEndLoc());
5174-
5175-
diag.fixItReplace(SourceRange(argExpr->getStartLoc()),
5176-
getTokenText(tok::l_square));
5177-
diag.fixItRemove(nameLoc.getSourceRange());
5178-
diag.fixItRemove(SourceRange(UDE->getDotLoc()));
5179-
if (CS.TC.Context.SourceMgr.extractText(lastArgSymbol) ==
5180-
getTokenText(tok::r_paren))
5181-
diag.fixItReplace(SourceRange(argExpr->getEndLoc()),
5182-
getTokenText(tok::r_square));
5183-
else
5184-
diag.fixItInsertAfter(argExpr->getEndLoc(),
5185-
getTokenText(tok::r_square));
5186-
showNote();
5187-
5188-
return true;
5189-
}
5190-
51915088
// Check if there is a structural problem in the function expression
51925089
// by performing type checking with the option to allow unresolved
51935090
// type variables. If that is going to produce a function type with
@@ -5262,12 +5159,6 @@ bool FailureDiagnosis::visitApplyExpr(ApplyExpr *callExpr) {
52625159
auto originalFnType = CS.getType(callExpr->getFn());
52635160

52645161
if (shouldTypeCheckFunctionExpr(*this, CS.DC, fnExpr)) {
5265-
5266-
// If we are misusing a subscript, diagnose that and provide a fixit.
5267-
// We diagnose this here to have enough context to offer an appropriate fixit.
5268-
if (diagnoseSubscriptMisuse(callExpr)) {
5269-
return CS.TC.Diags.hadAnyError();
5270-
}
52715162
// Type check the function subexpression to resolve a type for it if
52725163
// possible.
52735164
fnExpr = typeCheckChildIndependently(callExpr->getFn());

‎lib/Sema/CSDiagnostics.cpp

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,10 @@ FailureDiagnostic::emitDiagnostic(ArgTypes &&... Args) const {
7878
return cs.TC.diagnose(std::forward<ArgTypes>(Args)...);
7979
}
8080

81+
Expr *FailureDiagnostic::findParentExpr(Expr *subExpr) const {
82+
return E ? E->getParentMap()[subExpr] : nullptr;
83+
}
84+
8185
Type RequirementFailure::getOwnerType() const {
8286
return getType(getAnchor())->getInOutObjectType()->getMetatypeInstanceType();
8387
}
@@ -1275,3 +1279,56 @@ bool MissingCallFailure::diagnoseAsError() {
12751279
.fixItInsertAfter(insertLoc, "()");
12761280
return true;
12771281
}
1282+
1283+
bool SubscriptMisuseFailure::diagnoseAsError() {
1284+
auto &sourceMgr = getASTContext().SourceMgr;
1285+
1286+
auto *memberExpr = cast<UnresolvedDotExpr>(getRawAnchor());
1287+
auto *baseExpr = getAnchor();
1288+
1289+
auto memberRange = baseExpr->getSourceRange();
1290+
(void)simplifyLocator(getConstraintSystem(), getLocator(), memberRange);
1291+
1292+
auto nameLoc = DeclNameLoc(memberRange.Start);
1293+
1294+
auto diag = emitDiagnostic(baseExpr->getLoc(),
1295+
diag::could_not_find_subscript_member_did_you_mean,
1296+
getType(baseExpr));
1297+
1298+
diag.highlight(memberRange).highlight(nameLoc.getSourceRange());
1299+
1300+
auto *parentExpr = findParentExpr(memberExpr);
1301+
assert(parentExpr && "Couldn't find a parent expression for a member call?!");
1302+
1303+
auto *argExpr = cast<ApplyExpr>(parentExpr)->getArg();
1304+
1305+
auto toCharSourceRange = Lexer::getCharSourceRangeFromSourceRange;
1306+
auto lastArgSymbol = toCharSourceRange(sourceMgr, argExpr->getEndLoc());
1307+
1308+
diag.fixItReplace(SourceRange(argExpr->getStartLoc()),
1309+
getTokenText(tok::l_square));
1310+
diag.fixItRemove(nameLoc.getSourceRange());
1311+
diag.fixItRemove(SourceRange(memberExpr->getDotLoc()));
1312+
1313+
if (sourceMgr.extractText(lastArgSymbol) == getTokenText(tok::r_paren))
1314+
diag.fixItReplace(SourceRange(argExpr->getEndLoc()),
1315+
getTokenText(tok::r_square));
1316+
else
1317+
diag.fixItInsertAfter(argExpr->getEndLoc(), getTokenText(tok::r_square));
1318+
1319+
diag.flush();
1320+
if (auto overload = getOverloadChoiceIfAvailable(getLocator())) {
1321+
emitDiagnostic(overload->choice.getDecl(), diag::kind_declared_here,
1322+
DescriptiveDeclKind::Subscript);
1323+
}
1324+
1325+
return true;
1326+
}
1327+
1328+
bool SubscriptMisuseFailure::diagnoseAsNote() {
1329+
if (auto overload = getOverloadChoiceIfAvailable(getLocator())) {
1330+
emitDiagnostic(overload->choice.getDecl(), diag::found_candidate);
1331+
return true;
1332+
}
1333+
return false;
1334+
}

‎lib/Sema/CSDiagnostics.h

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,10 @@ class FailureDiagnostic {
143143
/// \returns true is locator hasn't been simplified down to expression.
144144
bool hasComplexLocator() const { return HasComplexLocator; }
145145

146+
/// \returns A parent expression if sub-expression is contained anywhere
147+
/// in the root expression or `nullptr` otherwise.
148+
Expr *findParentExpr(Expr *subExpr) const;
149+
146150
private:
147151
/// Compute anchor expression associated with current diagnostic.
148152
std::pair<Expr *, bool> computeAnchor() const;
@@ -182,15 +186,8 @@ class RequirementFailure : public FailureDiagnostic {
182186
if (!expr)
183187
return;
184188

185-
auto *anchor = getAnchor();
186-
expr->forEachChildExpr([&](Expr *subExpr) -> Expr * {
187-
auto *AE = dyn_cast<ApplyExpr>(subExpr);
188-
if (!AE || AE->getFn() != anchor)
189-
return subExpr;
190-
191-
Apply = AE;
192-
return nullptr;
193-
});
189+
if (auto *parentExpr = findParentExpr(getAnchor()))
190+
Apply = dyn_cast<ApplyExpr>(parentExpr);
194191
}
195192

196193
unsigned getRequirementIndex() const {
@@ -639,6 +636,16 @@ class MissingCallFailure final : public FailureDiagnostic {
639636
bool diagnoseAsError() override;
640637
};
641638

639+
class SubscriptMisuseFailure final : public FailureDiagnostic {
640+
public:
641+
SubscriptMisuseFailure(Expr *root, ConstraintSystem &cs,
642+
ConstraintLocator *locator)
643+
: FailureDiagnostic(root, cs, locator) {}
644+
645+
bool diagnoseAsError() override;
646+
bool diagnoseAsNote() override;
647+
};
648+
642649
} // end namespace constraints
643650
} // end namespace swift
644651

‎lib/Sema/CSFix.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,3 +235,13 @@ InsertExplicitCall *InsertExplicitCall::create(ConstraintSystem &cs,
235235
ConstraintLocator *locator) {
236236
return new (cs.getAllocator()) InsertExplicitCall(cs, locator);
237237
}
238+
239+
bool UseSubscriptOperator::diagnose(Expr *root, bool asNote) const {
240+
auto failure = SubscriptMisuseFailure(root, getConstraintSystem(), getLocator());
241+
return failure.diagnose(asNote);
242+
}
243+
244+
UseSubscriptOperator *UseSubscriptOperator::create(ConstraintSystem &cs,
245+
ConstraintLocator *locator) {
246+
return new (cs.getAllocator()) UseSubscriptOperator(cs, locator);
247+
}

‎lib/Sema/CSFix.h

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,9 @@ enum class FixKind : uint8_t {
9696

9797
/// Add explicit `()` at the end of function or member to call it.
9898
InsertCall,
99+
100+
/// Instead of spelling out `subscript` directly, use subscript operator.
101+
UseSubscriptOperator,
99102
};
100103

101104
class ConstraintFix {
@@ -447,6 +450,21 @@ class InsertExplicitCall final : public ConstraintFix {
447450
ConstraintLocator *locator);
448451
};
449452

453+
class UseSubscriptOperator final : public ConstraintFix {
454+
public:
455+
UseSubscriptOperator(ConstraintSystem &cs, ConstraintLocator *locator)
456+
: ConstraintFix(cs, FixKind::UseSubscriptOperator, locator) {}
457+
458+
std::string getName() const override {
459+
return "replace '.subscript(...)' with subscript operator";
460+
}
461+
462+
bool diagnose(Expr *root, bool asNote = false) const override;
463+
464+
static UseSubscriptOperator *create(ConstraintSystem &cs,
465+
ConstraintLocator *locator);
466+
};
467+
450468
} // end namespace constraints
451469
} // end namespace swift
452470

‎lib/Sema/CSSimplify.cpp

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3765,10 +3765,6 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(
37653765
// function takes no arguments, otherwise it
37663766
// would make sense to report this a missing member.
37673767
if (funcType->getNumParams() == 0) {
3768-
auto *fix = InsertExplicitCall::create(*this, locator);
3769-
if (recordFix(fix))
3770-
return SolutionKind::Error;
3771-
37723768
auto result = simplifyMemberConstraint(
37733769
kind, funcType->getResult(), member, memberTy, useDC,
37743770
functionRefKind, outerAlternatives, flags, locatorB);
@@ -3777,9 +3773,24 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(
37773773
// let's return, otherwise let's fall-through and report
37783774
// this problem as a missing member.
37793775
if (result == SolutionKind::Solved)
3780-
return result;
3776+
return recordFix(InsertExplicitCall::create(*this, locator))
3777+
? SolutionKind::Error
3778+
: SolutionKind::Solved;
37813779
}
37823780
}
3781+
3782+
// Instead of using subscript operator spelled out `subscript` directly.
3783+
if (member.getBaseName() == getTokenText(tok::kw_subscript)) {
3784+
auto result = simplifyMemberConstraint(
3785+
kind, baseTy, DeclBaseName::createSubscript(), memberTy, useDC,
3786+
functionRefKind, {}, flags, locatorB);
3787+
3788+
// Looks like it was indeed meant to be a subscript operator.
3789+
if (result == SolutionKind::Solved)
3790+
return recordFix(UseSubscriptOperator::create(*this, locator))
3791+
? SolutionKind::Error
3792+
: SolutionKind::Solved;
3793+
}
37833794
}
37843795
return SolutionKind::Error;
37853796
}
@@ -5442,6 +5453,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
54425453
return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;
54435454
}
54445455

5456+
case FixKind::UseSubscriptOperator:
54455457
case FixKind::InsertCall:
54465458
case FixKind::ExplicitlyEscaping:
54475459
case FixKind::CoerceToCheckedCast:

‎lib/Sema/ConstraintSystem.cpp

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2205,8 +2205,16 @@ bool ConstraintSystem::diagnoseAmbiguityWithFixes(
22052205
{
22062206
DiagnosticTransaction transaction(TC.Diags);
22072207

2208-
TC.diagnose(commonAnchor->getLoc(), diag::ambiguous_reference_to_decl,
2209-
decl->getDescriptiveKind(), decl->getFullName());
2208+
const auto *fix = viableSolutions.front().second;
2209+
if (fix->getKind() == FixKind::UseSubscriptOperator) {
2210+
auto *UDE = cast<UnresolvedDotExpr>(commonAnchor);
2211+
TC.diagnose(commonAnchor->getLoc(),
2212+
diag::could_not_find_subscript_member_did_you_mean,
2213+
getType(UDE->getBase()));
2214+
} else {
2215+
TC.diagnose(commonAnchor->getLoc(), diag::ambiguous_reference_to_decl,
2216+
decl->getDescriptiveKind(), decl->getFullName());
2217+
}
22102218

22112219
for (const auto &viable : viableSolutions) {
22122220
// Create scope so each applied solution is rolled back.

‎test/decl/subscript/subscripting.swift

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -265,8 +265,10 @@ func testGenSubscriptFixit(_ s0: GenSubscriptFixitTest) {
265265
}
266266

267267
struct SubscriptTest1 {
268-
subscript(keyword:String) -> Bool { return true } // expected-note 2 {{found this candidate}}
269-
subscript(keyword:String) -> String? {return nil } // expected-note 2 {{found this candidate}}
268+
subscript(keyword:String) -> Bool { return true }
269+
// expected-note@-1 4 {{found this candidate}}
270+
subscript(keyword:String) -> String? {return nil }
271+
// expected-note@-1 4 {{found this candidate}}
270272

271273
subscript(arg: SubClass) -> Bool { return true } // expected-note {{declared here}}
272274
subscript(arg: Protocol) -> Bool { return true } // expected-note 2 {{declared here}}
@@ -285,21 +287,23 @@ func testSubscript1(_ s1 : SubscriptTest1) {
285287
_ = s1.subscript((true, (5, baz: SubSubClass())), "hello")
286288
// expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}} {{9-10=}} {{10-19=}} {{19-20=[}} {{60-61=]}}
287289
_ = s1.subscript((fo: true, (5, baz: SubClass())), "hello")
288-
// expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}}
290+
// expected-error@-1 {{cannot convert value of type '(fo: Bool, (Int, baz: SubClass))' to expected argument type '(foo: Bool, bar: (Int, baz: SubClass))'}}
289291
_ = s1.subscript(SubSubClass())
290292
// expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}} {{9-10=}} {{10-19=}} {{19-20=[}} {{33-34=]}}
291293
_ = s1.subscript(ClassConformingToProtocol())
292294
// expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}} {{9-10=}} {{10-19=}} {{19-20=[}} {{47-48=]}}
293295
_ = s1.subscript(ClassConformingToRefinedProtocol())
294296
// expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}} {{9-10=}} {{10-19=}} {{19-20=[}} {{54-55=]}}
295297
_ = s1.subscript(true)
296-
// expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}}
298+
// expected-error@-1 {{cannot invoke 'subscript' with an argument list of type '(Bool)'}}
299+
// expected-note@-2 {{overloads for 'subscript' exist with these partially matching parameter lists: (Protocol), (String), (SubClass)}}
297300
_ = s1.subscript(SuperClass())
298-
// expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}}
301+
// expected-error@-1 {{cannot invoke 'subscript' with an argument list of type '(SuperClass)'}}
302+
// expected-note@-2 {{overloads for 'subscript' exist with these partially matching parameter lists: (Protocol), (String), (SubClass)}}
299303
_ = s1.subscript("hello")
300-
// expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}} {{9-10=}} {{10-19=}} {{19-20=[}} {{27-28=]}}
304+
// expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}}
301305
_ = s1.subscript("hello"
302-
// expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}} {{9-10=}} {{10-19=}} {{19-20=[}} {{27-27=]}}
306+
// expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}}
303307
// expected-note@-2 {{to match this opening '('}}
304308

305309
let _ = s1["hello"]

0 commit comments

Comments
 (0)