Skip to content

[Diagnostics] Diagnose subscript operator misuse via fixes #21682

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 8, 2019
Merged

[Diagnostics] Diagnose subscript operator misuse via fixes #21682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f032b9c
[CSFix] Attempt to replace `.subscript(...)` with subscript operator
xedin Jan 5, 2019
06a7ad6
[Diagnostics] Diagnose subscript operator misuse via fixes
xedin Jan 6, 2019
91e97a0
[Diagnostics] Diagnose ambiguity resulting from subscript operator fi…
xedin Jan 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 0 additions & 109 deletions lib/Sema/CSDiag.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,10 +572,6 @@ class FailureDiagnosis :public ASTVisitor<FailureDiagnosis, /*exprresult*/bool>{

bool diagnoseSubscriptErrors(SubscriptExpr *SE, bool performingSet);

/// Diagnose the usage of 'subscript' instead of the operator when calling
/// a subscript and offer a fixit if the inputs are compatible.
bool diagnoseSubscriptMisuse(ApplyExpr *callExpr);

/// Try to diagnose common errors involving implicitly non-escaping parameters
/// of function type, giving more specific and simpler diagnostics, attaching
/// notes on the parameter, and offering fixits to insert @escaping. Returns
Expand Down Expand Up @@ -5089,105 +5085,6 @@ bool FailureDiagnosis::diagnoseCallContextualConversionErrors(
exprType);
}

bool FailureDiagnosis::diagnoseSubscriptMisuse(ApplyExpr *callExpr) {
auto UDE = dyn_cast<UnresolvedDotExpr>(callExpr->getFn());
if (!UDE)
return false;

auto baseExpr = UDE->getBase();
if (!baseExpr || UDE->getName().getBaseName() != getTokenText(tok::kw_subscript))
return false;

auto baseType = CS.getType(baseExpr);
// Look up subscript declarations.
auto lookup = CS.lookupMember(baseType->getRValueType(),
DeclName(DeclBaseName::createSubscript()));
auto nonSubscrLookup = CS.lookupMember(baseType->getRValueType(),
UDE->getName());
// Make sure we only found subscript declarations. If not, the problem
// is different - return.
if (lookup.empty() || !nonSubscrLookup.empty())
return false;
// Try to resolve a type of the argument expression.
auto argExpr = typeCheckChildIndependently(callExpr->getArg(),
Type(), CTP_CallArgument);
if (!argExpr)
return CS.TC.Diags.hadAnyError();

SmallVector<Identifier, 2> scratch;
ArrayRef<Identifier> argLabels = callExpr->getArgumentLabels(scratch);
SmallVector<OverloadChoice, 2> choices;

for (auto candidate : lookup)
choices.push_back(OverloadChoice(baseType, candidate.getValueDecl(),
UDE->getFunctionRefKind()));
CalleeCandidateInfo candidateInfo(baseType, choices,
callArgHasTrailingClosure(argExpr),
CS, true);

auto params = decomposeArgType(CS.getType(argExpr), argLabels);
using ClosenessPair = CalleeCandidateInfo::ClosenessResultTy;

candidateInfo.filterList([&](OverloadCandidate cand) -> ClosenessPair {
auto candFuncType = cand.getFunctionType();
if (!candFuncType)
return {CC_GeneralMismatch, {}};

auto candParams = candFuncType->getParams();
if (params.size() != candParams.size())
return {CC_GeneralMismatch, {}};

for (unsigned i = 0, e = params.size(); i < e; i ++) {
if (CS.TC.isConvertibleTo(params[i].getOldType(),
candParams[i].getOldType(), CS.DC) ||
candParams[i].getOldType()->is<GenericTypeParamType>())
continue;
return {CC_GeneralMismatch, {}};
}
return {CC_ExactMatch, {}};
});

auto *locator = CS.getConstraintLocator(UDE, ConstraintLocator::Member);
auto memberRange = baseExpr->getSourceRange();
if (locator)
locator = simplifyLocator(CS, locator, memberRange);
auto nameLoc = DeclNameLoc(memberRange.Start);

auto diag = diagnose(baseExpr->getLoc(),
diag::could_not_find_subscript_member_did_you_mean,
baseType);
diag.highlight(memberRange).highlight(nameLoc.getSourceRange());

auto showNote = [&]() {
diag.flush();
if (candidateInfo.size() == 1)
diagnose(candidateInfo.candidates.front().getDecl(),
diag::kind_declared_here, DescriptiveDeclKind::Subscript);
};
if (candidateInfo.closeness != CC_ExactMatch) {
showNote();
return true;
}
auto toCharSourceRange = Lexer::getCharSourceRangeFromSourceRange;
auto lastArgSymbol = toCharSourceRange(CS.TC.Context.SourceMgr,
argExpr->getEndLoc());

diag.fixItReplace(SourceRange(argExpr->getStartLoc()),
getTokenText(tok::l_square));
diag.fixItRemove(nameLoc.getSourceRange());
diag.fixItRemove(SourceRange(UDE->getDotLoc()));
if (CS.TC.Context.SourceMgr.extractText(lastArgSymbol) ==
getTokenText(tok::r_paren))
diag.fixItReplace(SourceRange(argExpr->getEndLoc()),
getTokenText(tok::r_square));
else
diag.fixItInsertAfter(argExpr->getEndLoc(),
getTokenText(tok::r_square));
showNote();

return true;
}

// Check if there is a structural problem in the function expression
// by performing type checking with the option to allow unresolved
// type variables. If that is going to produce a function type with
Expand Down Expand Up @@ -5262,12 +5159,6 @@ bool FailureDiagnosis::visitApplyExpr(ApplyExpr *callExpr) {
auto originalFnType = CS.getType(callExpr->getFn());

if (shouldTypeCheckFunctionExpr(*this, CS.DC, fnExpr)) {

// If we are misusing a subscript, diagnose that and provide a fixit.
// We diagnose this here to have enough context to offer an appropriate fixit.
if (diagnoseSubscriptMisuse(callExpr)) {
return CS.TC.Diags.hadAnyError();
}
// Type check the function subexpression to resolve a type for it if
// possible.
fnExpr = typeCheckChildIndependently(callExpr->getFn());
Expand Down
57 changes: 57 additions & 0 deletions lib/Sema/CSDiagnostics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ FailureDiagnostic::emitDiagnostic(ArgTypes &&... Args) const {
return cs.TC.diagnose(std::forward<ArgTypes>(Args)...);
}

Expr *FailureDiagnostic::findParentExpr(Expr *subExpr) const {
return E ? E->getParentMap()[subExpr] : nullptr;
}

Type RequirementFailure::getOwnerType() const {
return getType(getAnchor())->getInOutObjectType()->getMetatypeInstanceType();
}
Expand Down Expand Up @@ -1275,3 +1279,56 @@ bool MissingCallFailure::diagnoseAsError() {
.fixItInsertAfter(insertLoc, "()");
return true;
}

bool SubscriptMisuseFailure::diagnoseAsError() {
auto &sourceMgr = getASTContext().SourceMgr;

auto *memberExpr = cast<UnresolvedDotExpr>(getRawAnchor());
auto *baseExpr = getAnchor();

auto memberRange = baseExpr->getSourceRange();
(void)simplifyLocator(getConstraintSystem(), getLocator(), memberRange);

auto nameLoc = DeclNameLoc(memberRange.Start);

auto diag = emitDiagnostic(baseExpr->getLoc(),
diag::could_not_find_subscript_member_did_you_mean,
getType(baseExpr));

diag.highlight(memberRange).highlight(nameLoc.getSourceRange());

auto *parentExpr = findParentExpr(memberExpr);
assert(parentExpr && "Couldn't find a parent expression for a member call?!");

auto *argExpr = cast<ApplyExpr>(parentExpr)->getArg();

auto toCharSourceRange = Lexer::getCharSourceRangeFromSourceRange;
auto lastArgSymbol = toCharSourceRange(sourceMgr, argExpr->getEndLoc());

diag.fixItReplace(SourceRange(argExpr->getStartLoc()),
getTokenText(tok::l_square));
diag.fixItRemove(nameLoc.getSourceRange());
diag.fixItRemove(SourceRange(memberExpr->getDotLoc()));

if (sourceMgr.extractText(lastArgSymbol) == getTokenText(tok::r_paren))
diag.fixItReplace(SourceRange(argExpr->getEndLoc()),
getTokenText(tok::r_square));
else
diag.fixItInsertAfter(argExpr->getEndLoc(), getTokenText(tok::r_square));

diag.flush();
if (auto overload = getOverloadChoiceIfAvailable(getLocator())) {
emitDiagnostic(overload->choice.getDecl(), diag::kind_declared_here,
DescriptiveDeclKind::Subscript);
}

return true;
}

bool SubscriptMisuseFailure::diagnoseAsNote() {
if (auto overload = getOverloadChoiceIfAvailable(getLocator())) {
emitDiagnostic(overload->choice.getDecl(), diag::found_candidate);
return true;
}
return false;
}
25 changes: 16 additions & 9 deletions lib/Sema/CSDiagnostics.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,10 @@ class FailureDiagnostic {
/// \returns true is locator hasn't been simplified down to expression.
bool hasComplexLocator() const { return HasComplexLocator; }

/// \returns A parent expression if sub-expression is contained anywhere
/// in the root expression or `nullptr` otherwise.
Expr *findParentExpr(Expr *subExpr) const;

private:
/// Compute anchor expression associated with current diagnostic.
std::pair<Expr *, bool> computeAnchor() const;
Expand Down Expand Up @@ -182,15 +186,8 @@ class RequirementFailure : public FailureDiagnostic {
if (!expr)
return;

auto *anchor = getAnchor();
expr->forEachChildExpr([&](Expr *subExpr) -> Expr * {
auto *AE = dyn_cast<ApplyExpr>(subExpr);
if (!AE || AE->getFn() != anchor)
return subExpr;

Apply = AE;
return nullptr;
});
if (auto *parentExpr = findParentExpr(getAnchor()))
Apply = dyn_cast<ApplyExpr>(parentExpr);
}

unsigned getRequirementIndex() const {
Expand Down Expand Up @@ -639,6 +636,16 @@ class MissingCallFailure final : public FailureDiagnostic {
bool diagnoseAsError() override;
};

class SubscriptMisuseFailure final : public FailureDiagnostic {
public:
SubscriptMisuseFailure(Expr *root, ConstraintSystem &cs,
ConstraintLocator *locator)
: FailureDiagnostic(root, cs, locator) {}

bool diagnoseAsError() override;
bool diagnoseAsNote() override;
};

} // end namespace constraints
} // end namespace swift

Expand Down
10 changes: 10 additions & 0 deletions lib/Sema/CSFix.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,3 +235,13 @@ InsertExplicitCall *InsertExplicitCall::create(ConstraintSystem &cs,
ConstraintLocator *locator) {
return new (cs.getAllocator()) InsertExplicitCall(cs, locator);
}

bool UseSubscriptOperator::diagnose(Expr *root, bool asNote) const {
auto failure = SubscriptMisuseFailure(root, getConstraintSystem(), getLocator());
return failure.diagnose(asNote);
}

UseSubscriptOperator *UseSubscriptOperator::create(ConstraintSystem &cs,
ConstraintLocator *locator) {
return new (cs.getAllocator()) UseSubscriptOperator(cs, locator);
}
18 changes: 18 additions & 0 deletions lib/Sema/CSFix.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ enum class FixKind : uint8_t {

/// Add explicit `()` at the end of function or member to call it.
InsertCall,

/// Instead of spelling out `subscript` directly, use subscript operator.
UseSubscriptOperator,
};

class ConstraintFix {
Expand Down Expand Up @@ -447,6 +450,21 @@ class InsertExplicitCall final : public ConstraintFix {
ConstraintLocator *locator);
};

class UseSubscriptOperator final : public ConstraintFix {
public:
UseSubscriptOperator(ConstraintSystem &cs, ConstraintLocator *locator)
: ConstraintFix(cs, FixKind::UseSubscriptOperator, locator) {}

std::string getName() const override {
return "replace '.subscript(...)' with subscript operator";
}

bool diagnose(Expr *root, bool asNote = false) const override;

static UseSubscriptOperator *create(ConstraintSystem &cs,
ConstraintLocator *locator);
};

} // end namespace constraints
} // end namespace swift

Expand Down
22 changes: 17 additions & 5 deletions lib/Sema/CSSimplify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3765,10 +3765,6 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(
// function takes no arguments, otherwise it
// would make sense to report this a missing member.
if (funcType->getNumParams() == 0) {
auto *fix = InsertExplicitCall::create(*this, locator);
if (recordFix(fix))
return SolutionKind::Error;

auto result = simplifyMemberConstraint(
kind, funcType->getResult(), member, memberTy, useDC,
functionRefKind, outerAlternatives, flags, locatorB);
Expand All @@ -3777,9 +3773,24 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(
// let's return, otherwise let's fall-through and report
// this problem as a missing member.
if (result == SolutionKind::Solved)
return result;
return recordFix(InsertExplicitCall::create(*this, locator))
? SolutionKind::Error
: SolutionKind::Solved;
}
}

// Instead of using subscript operator spelled out `subscript` directly.
if (member.getBaseName() == getTokenText(tok::kw_subscript)) {
auto result = simplifyMemberConstraint(
kind, baseTy, DeclBaseName::createSubscript(), memberTy, useDC,
functionRefKind, {}, flags, locatorB);

// Looks like it was indeed meant to be a subscript operator.
if (result == SolutionKind::Solved)
return recordFix(UseSubscriptOperator::create(*this, locator))
? SolutionKind::Error
: SolutionKind::Solved;
}
}
return SolutionKind::Error;
}
Expand Down Expand Up @@ -5442,6 +5453,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;
}

case FixKind::UseSubscriptOperator:
case FixKind::InsertCall:
case FixKind::ExplicitlyEscaping:
case FixKind::CoerceToCheckedCast:
Expand Down
12 changes: 10 additions & 2 deletions lib/Sema/ConstraintSystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2205,8 +2205,16 @@ bool ConstraintSystem::diagnoseAmbiguityWithFixes(
{
DiagnosticTransaction transaction(TC.Diags);

TC.diagnose(commonAnchor->getLoc(), diag::ambiguous_reference_to_decl,
decl->getDescriptiveKind(), decl->getFullName());
const auto *fix = viableSolutions.front().second;
if (fix->getKind() == FixKind::UseSubscriptOperator) {
auto *UDE = cast<UnresolvedDotExpr>(commonAnchor);
TC.diagnose(commonAnchor->getLoc(),
diag::could_not_find_subscript_member_did_you_mean,
getType(UDE->getBase()));
} else {
TC.diagnose(commonAnchor->getLoc(), diag::ambiguous_reference_to_decl,
decl->getDescriptiveKind(), decl->getFullName());
}

for (const auto &viable : viableSolutions) {
// Create scope so each applied solution is rolled back.
Expand Down
18 changes: 11 additions & 7 deletions test/decl/subscript/subscripting.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,8 +265,10 @@ func testGenSubscriptFixit(_ s0: GenSubscriptFixitTest) {
}

struct SubscriptTest1 {
subscript(keyword:String) -> Bool { return true } // expected-note 2 {{found this candidate}}
subscript(keyword:String) -> String? {return nil } // expected-note 2 {{found this candidate}}
subscript(keyword:String) -> Bool { return true }
// expected-note@-1 4 {{found this candidate}}
subscript(keyword:String) -> String? {return nil }
// expected-note@-1 4 {{found this candidate}}

subscript(arg: SubClass) -> Bool { return true } // expected-note {{declared here}}
subscript(arg: Protocol) -> Bool { return true } // expected-note 2 {{declared here}}
Expand All @@ -285,21 +287,23 @@ func testSubscript1(_ s1 : SubscriptTest1) {
_ = s1.subscript((true, (5, baz: SubSubClass())), "hello")
// 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=]}}
_ = s1.subscript((fo: true, (5, baz: SubClass())), "hello")
// expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}}
// expected-error@-1 {{cannot convert value of type '(fo: Bool, (Int, baz: SubClass))' to expected argument type '(foo: Bool, bar: (Int, baz: SubClass))'}}
_ = s1.subscript(SubSubClass())
// 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=]}}
_ = s1.subscript(ClassConformingToProtocol())
// 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=]}}
_ = s1.subscript(ClassConformingToRefinedProtocol())
// 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=]}}
_ = s1.subscript(true)
// expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}}
// expected-error@-1 {{cannot invoke 'subscript' with an argument list of type '(Bool)'}}
Copy link
Contributor

@jrose-apple jrose-apple Jan 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you disagree with me on this? This is one of the ones that's changed for the worse (IMO). Can you get it to use your new wording?

Copy link
Contributor Author

@xedin xedin Jan 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't really disagree, I'm just going to make it work with re-labeling/re-typing fix. It's going to have both diagnostics for incorrect argument to fix-it about subscript. It's on my radar, I just need to gradually fix CSDiag mess, because that's what is the biggest barrier here is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll stop bugging you about this. :-) I definitely think you've been doing great chipping away at CSDiag!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's okay, no problem! :) Once I convert most of the stuff to use fixes we wouldn't have this ordering problem anymore, but for a bit some of the diagnostics might be in reverse order to what they used to be before, because it would fall into CSDiag first and then diagnose with a fix...

// expected-note@-2 {{overloads for 'subscript' exist with these partially matching parameter lists: (Protocol), (String), (SubClass)}}
_ = s1.subscript(SuperClass())
// expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}}
// expected-error@-1 {{cannot invoke 'subscript' with an argument list of type '(SuperClass)'}}
// expected-note@-2 {{overloads for 'subscript' exist with these partially matching parameter lists: (Protocol), (String), (SubClass)}}
_ = s1.subscript("hello")
// 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=]}}
// expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we lose the fix-its here?

_ = s1.subscript("hello"
// 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=]}}
// expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}}
// expected-note@-2 {{to match this opening '('}}

let _ = s1["hello"]
Expand Down