Skip to content

Commit 1d22e98

Browse files
slsl
committed
Sema: Add a diagnostic for assigning using a subscript with a read-only keypath
1 parent 1192b91 commit 1d22e98

File tree

2 files changed

+177
-124
lines changed

2 files changed

+177
-124
lines changed

‎lib/Sema/CSDiagnostics.cpp

Lines changed: 172 additions & 121 deletions
Original file line numberDiff line numberDiff line change
@@ -981,100 +981,128 @@ bool AssignmentFailure::diagnoseAsError() {
981981
// we find a node in the lvalue path that is problematic, this returns it.
982982
auto immInfo = resolveImmutableBase(destExpr);
983983

984-
// Otherwise, we cannot resolve this because the available setter candidates
985-
// are all mutating and the base must be mutating. If we dug out a
986-
// problematic decl, we can produce a nice tailored diagnostic.
987-
if (auto *VD = dyn_cast_or_null<VarDecl>(immInfo.second)) {
988-
std::string message = "'";
989-
message += VD->getName().str().str();
990-
message += "'";
991-
992-
auto type = getType(immInfo.first);
993-
auto bgt = type ? type->getAs<BoundGenericType>() : nullptr;
994-
995-
if (bgt && bgt->getDecl() == getASTContext().getKeyPathDecl())
996-
message += " is a read-only key path";
997-
else if (VD->isCaptureList())
998-
message += " is an immutable capture";
999-
else if (VD->isImplicit())
1000-
message += " is immutable";
1001-
else if (VD->isLet())
1002-
message += " is a 'let' constant";
1003-
else if (!VD->isSettable(DC))
1004-
message += " is a get-only property";
1005-
else if (!VD->isSetterAccessibleFrom(DC))
1006-
message += " setter is inaccessible";
1007-
else {
1008-
message += " is immutable";
1009-
}
1010-
1011-
emitDiagnostic(Loc, DeclDiagnostic, message)
1012-
.highlight(immInfo.first->getSourceRange());
1013-
1014-
// If there is a masked instance variable of the same type, emit a
1015-
// note to fixit prepend a 'self.'.
1016-
if (auto typeContext = DC->getInnermostTypeContext()) {
1017-
UnqualifiedLookup lookup(VD->getFullName(), typeContext,
1018-
getASTContext().getLazyResolver());
1019-
for (auto &result : lookup.Results) {
1020-
const VarDecl *typeVar = dyn_cast<VarDecl>(result.getValueDecl());
1021-
if (typeVar && typeVar != VD && typeVar->isSettable(DC) &&
1022-
typeVar->isSetterAccessibleFrom(DC) &&
1023-
typeVar->getType()->isEqual(VD->getType())) {
1024-
// But not in its own accessor.
1025-
auto AD =
1026-
dyn_cast_or_null<AccessorDecl>(DC->getInnermostMethodContext());
1027-
if (!AD || AD->getStorage() != typeVar) {
1028-
emitDiagnostic(Loc, diag::masked_instance_variable,
1029-
typeContext->getSelfTypeInContext())
1030-
.fixItInsert(Loc, "self.");
984+
Optional<OverloadChoice> choice = immInfo.second;
985+
986+
// Attempt diagnostics based on the overload choice.
987+
if (choice.hasValue()) {
988+
989+
if (!choice->isDecl()) {
990+
if (choice->getKind() == OverloadChoiceKind::KeyPathApplication &&
991+
!isa<ApplyExpr>(immInfo.first)) {
992+
std::string message = "the key path";
993+
if (auto *SE = dyn_cast<SubscriptExpr>(immInfo.first)) {
994+
if (auto *tupleExpr = dyn_cast<TupleExpr>(SE->getIndex())) {
995+
if (auto *DRE = dyn_cast<DeclRefExpr>(tupleExpr->getElement(0))) {
996+
auto identifier = DRE->getDecl()->getBaseName().getIdentifier();
997+
message = "'" + identifier.str().str() + "'";
998+
}
1031999
}
10321000
}
1001+
emitDiagnostic(Loc, DeclDiagnostic, message + " is a read-only key path")
1002+
.highlight(immInfo.first->getSourceRange());
1003+
return true;
1004+
}
1005+
return false;
1006+
}
1007+
1008+
// Otherwise, we cannot resolve this because the available setter candidates
1009+
// are all mutating and the base must be mutating. If we dug out a
1010+
// problematic decl, we can produce a nice tailored diagnostic.
1011+
if (auto *VD = dyn_cast_or_null<VarDecl>(choice->getDecl())) {
1012+
std::string message = "'";
1013+
message += VD->getName().str().str();
1014+
message += "'";
1015+
1016+
auto type = getType(immInfo.first);
1017+
1018+
if (isKnownKeyPathType(type))
1019+
message += " is a read-only key path";
1020+
else if (VD->isCaptureList())
1021+
message += " is an immutable capture";
1022+
else if (VD->isImplicit())
1023+
message += " is immutable";
1024+
else if (VD->isLet())
1025+
message += " is a 'let' constant";
1026+
else if (!VD->isSettable(DC))
1027+
message += " is a get-only property";
1028+
else if (!VD->isSetterAccessibleFrom(DC))
1029+
message += " setter is inaccessible";
1030+
else {
1031+
message += " is immutable";
10331032
}
1034-
}
10351033

1036-
// If this is a simple variable marked with a 'let', emit a note to fixit
1037-
// hint it to 'var'.
1038-
VD->emitLetToVarNoteIfSimple(DC);
1039-
return true;
1040-
}
1034+
emitDiagnostic(Loc, DeclDiagnostic, message)
1035+
.highlight(immInfo.first->getSourceRange());
1036+
1037+
// If there is a masked instance variable of the same type, emit a
1038+
// note to fixit prepend a 'self.'.
1039+
if (auto typeContext = DC->getInnermostTypeContext()) {
1040+
UnqualifiedLookup lookup(VD->getFullName(), typeContext,
1041+
getASTContext().getLazyResolver());
1042+
for (auto &result : lookup.Results) {
1043+
const VarDecl *typeVar = dyn_cast<VarDecl>(result.getValueDecl());
1044+
if (typeVar && typeVar != VD && typeVar->isSettable(DC) &&
1045+
typeVar->isSetterAccessibleFrom(DC) &&
1046+
typeVar->getType()->isEqual(VD->getType())) {
1047+
// But not in its own accessor.
1048+
auto AD =
1049+
dyn_cast_or_null<AccessorDecl>(DC->getInnermostMethodContext());
1050+
if (!AD || AD->getStorage() != typeVar) {
1051+
emitDiagnostic(Loc, diag::masked_instance_variable,
1052+
typeContext->getSelfTypeInContext())
1053+
.fixItInsert(Loc, "self.");
1054+
}
1055+
}
1056+
}
1057+
}
10411058

1042-
// If the underlying expression was a read-only subscript, diagnose that.
1043-
if (auto *SD = dyn_cast_or_null<SubscriptDecl>(immInfo.second)) {
1044-
StringRef message;
1045-
if (!SD->isSettable())
1046-
message = "subscript is get-only";
1047-
else if (!SD->isSetterAccessibleFrom(DC))
1048-
message = "subscript setter is inaccessible";
1049-
else
1050-
message = "subscript is immutable";
1059+
// If this is a simple variable marked with a 'let', emit a note to fixit
1060+
// hint it to 'var'.
1061+
VD->emitLetToVarNoteIfSimple(DC);
1062+
return true;
1063+
}
10511064

1052-
emitDiagnostic(Loc, DeclDiagnostic, message)
1053-
.highlight(immInfo.first->getSourceRange());
1054-
return true;
1055-
}
1065+
// If the underlying expression was a read-only subscript, diagnose that.
1066+
if (auto *SD = dyn_cast_or_null<SubscriptDecl>(choice->getDecl())) {
1067+
StringRef message;
1068+
if (!SD->isSettable())
1069+
message = "subscript is get-only";
1070+
else if (!SD->isSetterAccessibleFrom(DC))
1071+
message = "subscript setter is inaccessible";
1072+
else
1073+
message = "subscript is immutable";
10561074

1057-
// If we're trying to set an unapplied method, say that.
1058-
if (auto *VD = immInfo.second) {
1059-
std::string message = "'";
1060-
message += VD->getBaseName().getIdentifier().str();
1061-
message += "'";
1075+
emitDiagnostic(Loc, DeclDiagnostic, message)
1076+
.highlight(immInfo.first->getSourceRange());
1077+
return true;
1078+
}
10621079

1063-
auto diagID = DeclDiagnostic;
1064-
if (auto *AFD = dyn_cast<AbstractFunctionDecl>(VD)) {
1065-
if (AFD->hasImplicitSelfDecl()) {
1066-
message += " is a method";
1067-
diagID = diag::assignment_lhs_is_immutable_variable;
1068-
} else {
1069-
message += " is a function";
1070-
}
1071-
} else
1072-
message += " is not settable";
1080+
// If we're trying to set an unapplied method, say that.
1081+
if (auto *VD = choice->getDecl()) {
1082+
std::string message = "'";
1083+
message += VD->getBaseName().getIdentifier().str();
1084+
message += "'";
1085+
1086+
auto diagID = DeclDiagnostic;
1087+
if (auto *AFD = dyn_cast<AbstractFunctionDecl>(VD)) {
1088+
if (AFD->hasImplicitSelfDecl()) {
1089+
message += " is a method";
1090+
diagID = diag::assignment_lhs_is_immutable_variable;
1091+
} else {
1092+
message += " is a function";
1093+
}
1094+
} else
1095+
message += " is not settable";
10731096

1074-
emitDiagnostic(Loc, diagID, message)
1075-
.highlight(immInfo.first->getSourceRange());
1076-
return true;
1097+
emitDiagnostic(Loc, diagID, message)
1098+
.highlight(immInfo.first->getSourceRange());
1099+
return true;
1100+
}
1101+
10771102
}
1103+
1104+
// Fall back to producing diagnostics based on the expression since we
1105+
// couldn't determine anything from the OverloadChoice.
10781106

10791107
// If a keypath was the problem but wasn't resolved into a vardecl
10801108
// it is ambiguous or unable to be used for setting.
@@ -1227,40 +1255,63 @@ void AssignmentFailure::fixItChangeInoutArgType(const Expr *arg,
12271255
.fixItReplaceChars(startLoc, endLoc, scratch);
12281256
}
12291257

1230-
std::pair<Expr *, ValueDecl *>
1258+
std::pair<Expr *, Optional<OverloadChoice>>
12311259
AssignmentFailure::resolveImmutableBase(Expr *expr) const {
12321260
auto &cs = getConstraintSystem();
12331261
auto *DC = getDC();
12341262
expr = expr->getValueProvidingExpr();
12351263

1264+
auto isImmutable = [&](ValueDecl *decl) {
1265+
if (auto *storage = dyn_cast<AbstractStorageDecl>(decl))
1266+
return !storage->isSettable(nullptr) ||
1267+
!storage->isSetterAccessibleFrom(DC);
1268+
1269+
return false;
1270+
};
1271+
12361272
// Provide specific diagnostics for assignment to subscripts whose base expr
12371273
// is known to be an rvalue.
12381274
if (auto *SE = dyn_cast<SubscriptExpr>(expr)) {
1275+
12391276
// If we found a decl for the subscript, check to see if it is a set-only
12401277
// subscript decl.
1241-
SubscriptDecl *member = nullptr;
1242-
if (SE->hasDecl())
1243-
member = dyn_cast_or_null<SubscriptDecl>(SE->getDecl().getDecl());
1244-
1245-
if (!member) {
1246-
auto loc =
1247-
cs.getConstraintLocator(SE, ConstraintLocator::SubscriptMember);
1248-
member = dyn_cast_or_null<SubscriptDecl>(getMemberRef(loc));
1278+
if (SE->hasDecl()) {
1279+
const auto &declRef = SE->getDecl();
1280+
if (auto *subscript =
1281+
dyn_cast_or_null<SubscriptDecl>(declRef.getDecl())) {
1282+
if (isImmutable(subscript))
1283+
return {expr, OverloadChoice(getType(SE->getBase()), subscript,
1284+
FunctionRefKind::DoubleApply)};
1285+
}
12491286
}
12501287

1288+
Optional<OverloadChoice> member;
1289+
1290+
auto loc = cs.getConstraintLocator(SE, ConstraintLocator::SubscriptMember);
1291+
member = getMemberRef(loc);
1292+
12511293
// If it isn't settable, return it.
12521294
if (member) {
1253-
if (!member->isSettable() || !member->isSetterAccessibleFrom(DC))
1295+
if (member->isDecl() && isImmutable(member->getDecl()))
12541296
return {expr, member};
1255-
}
12561297

1257-
if (auto tupleExpr = dyn_cast<TupleExpr>(SE->getIndex())) {
1258-
if (tupleExpr->getNumElements() == 1 &&
1259-
tupleExpr->getElementName(0).str() == "keyPath") {
1260-
auto indexType = getType(tupleExpr->getElement(0));
1298+
// We still have a choice, the choice is not a decl
1299+
if (!member->isDecl()) {
1300+
// This must be a keypath application
1301+
assert(member->getKind() == OverloadChoiceKind::KeyPathApplication);
1302+
1303+
auto *argType = getType(SE->getIndex())->castTo<TupleType>();
1304+
assert(argType->getNumElements() == 1);
1305+
1306+
auto indexType = resolveType(argType->getElementType(0));
1307+
12611308
if (auto bgt = indexType->getAs<BoundGenericType>()) {
1262-
if (bgt->getDecl() == getASTContext().getKeyPathDecl())
1263-
return resolveImmutableBase(tupleExpr->getElement(0));
1309+
// In Swift versions lower than 5, this check will fail as read only
1310+
// key paths can masquerade as writable for compatibilty reasons.
1311+
// This is fine as in this case we just fall back on old diagnostics.
1312+
if (bgt->getDecl() == getASTContext().getKeyPathDecl()) {
1313+
return {expr, member};
1314+
}
12641315
}
12651316
}
12661317
}
@@ -1274,18 +1325,12 @@ AssignmentFailure::resolveImmutableBase(Expr *expr) const {
12741325
// If we found a decl for the UDE, check it.
12751326
auto loc = cs.getConstraintLocator(UDE, ConstraintLocator::Member);
12761327

1277-
auto *member = getMemberRef(loc);
1328+
auto member = getMemberRef(loc);
1329+
12781330
// If we can resolve a member, we can determine whether it is settable in
12791331
// this context.
1280-
if (member) {
1281-
auto *memberVD = dyn_cast<VarDecl>(member);
1282-
1283-
// If the member isn't a vardecl (e.g. its a funcdecl), or it isn't
1284-
// settable, then it is the problem: return it.
1285-
if (!memberVD || !member->isSettable(nullptr) ||
1286-
!memberVD->isSetterAccessibleFrom(DC))
1287-
return {expr, member};
1288-
}
1332+
if (member && member->isDecl() && isImmutable(member->getDecl()))
1333+
return {expr, member};
12891334

12901335
// If we weren't able to resolve a member or if it is mutable, then the
12911336
// problem must be with the base, recurse.
@@ -1295,16 +1340,18 @@ AssignmentFailure::resolveImmutableBase(Expr *expr) const {
12951340
if (auto *MRE = dyn_cast<MemberRefExpr>(expr)) {
12961341
// If the member isn't settable, then it is the problem: return it.
12971342
if (auto member = dyn_cast<AbstractStorageDecl>(MRE->getMember().getDecl()))
1298-
if (!member->isSettable(nullptr) || !member->isSetterAccessibleFrom(DC))
1299-
return {expr, member};
1343+
if (isImmutable(member))
1344+
return {expr, OverloadChoice(getType(MRE->getBase()), member,
1345+
FunctionRefKind::SingleApply)};
13001346

13011347
// If we weren't able to resolve a member or if it is mutable, then the
13021348
// problem must be with the base, recurse.
13031349
return resolveImmutableBase(MRE->getBase());
13041350
}
13051351

13061352
if (auto *DRE = dyn_cast<DeclRefExpr>(expr))
1307-
return {expr, DRE->getDecl()};
1353+
return {expr,
1354+
OverloadChoice(Type(), DRE->getDecl(), FunctionRefKind::Unapplied)};
13081355

13091356
// Look through x!
13101357
if (auto *FVE = dyn_cast<ForceValueExpr>(expr))
@@ -1322,13 +1369,17 @@ AssignmentFailure::resolveImmutableBase(Expr *expr) const {
13221369
if (auto *SAE = dyn_cast<SelfApplyExpr>(expr))
13231370
return resolveImmutableBase(SAE->getFn());
13241371

1325-
return {expr, nullptr};
1372+
return {expr, None};
13261373
}
13271374

1328-
ValueDecl *AssignmentFailure::getMemberRef(ConstraintLocator *locator) const {
1375+
Optional<OverloadChoice>
1376+
AssignmentFailure::getMemberRef(ConstraintLocator *locator) const {
13291377
auto member = getOverloadChoiceIfAvailable(locator);
1330-
if (!member || !member->choice.isDecl())
1331-
return nullptr;
1378+
if (!member)
1379+
return None;
1380+
1381+
if (!member->choice.isDecl())
1382+
return member->choice;
13321383

13331384
auto *DC = getDC();
13341385
auto &TC = getTypeChecker();
@@ -1352,14 +1403,14 @@ ValueDecl *AssignmentFailure::getMemberRef(ConstraintLocator *locator) const {
13521403
locator, LocatorPathElt::getKeyPathDynamicMember(keyPath));
13531404

13541405
auto memberRef = getOverloadChoiceIfAvailable(memberLoc);
1355-
return memberRef ? memberRef->choice.getDecl() : nullptr;
1406+
return memberRef ? Optional<OverloadChoice>(memberRef->choice) : None;
13561407
}
13571408

13581409
// If this is a string based dynamic lookup, there is no member declaration.
1359-
return nullptr;
1410+
return None;
13601411
}
13611412

1362-
return decl;
1413+
return member->choice;
13631414
}
13641415

13651416
Diag<StringRef> AssignmentFailure::findDeclDiagonstic(ASTContext &ctx,

0 commit comments

Comments
 (0)