Skip to content

Commit 4fac0b3

Browse files
slsl
authored
Merge pull request #24450 from sl/sl/sr-10202
Add useful diagnostic when trying to write through key path on private(set) var
2 parents f5ba89d + 501cd91 commit 4fac0b3

File tree

3 files changed

+195
-125
lines changed

3 files changed

+195
-125
lines changed

‎lib/Sema/CSDiagnostics.cpp

Lines changed: 173 additions & 120 deletions
Original file line numberDiff line numberDiff line change
@@ -1107,101 +1107,134 @@ bool AssignmentFailure::diagnoseAsError() {
11071107
// we find a node in the lvalue path that is problematic, this returns it.
11081108
auto immInfo = resolveImmutableBase(destExpr);
11091109

1110-
// Otherwise, we cannot resolve this because the available setter candidates
1111-
// are all mutating and the base must be mutating. If we dug out a
1112-
// problematic decl, we can produce a nice tailored diagnostic.
1113-
if (auto *VD = dyn_cast_or_null<VarDecl>(immInfo.second)) {
1114-
std::string message = "'";
1115-
message += VD->getName().str().str();
1116-
message += "'";
1117-
1118-
auto type = getType(immInfo.first);
1119-
auto bgt = type ? type->getAs<BoundGenericType>() : nullptr;
1120-
1121-
if (bgt && bgt->getDecl() == getASTContext().getKeyPathDecl())
1122-
message += " is a read-only key path";
1123-
else if (VD->isCaptureList())
1124-
message += " is an immutable capture";
1125-
else if (VD->isImplicit())
1126-
message += " is immutable";
1127-
else if (VD->isLet())
1128-
message += " is a 'let' constant";
1129-
else if (!VD->isSettable(DC))
1130-
message += " is a get-only property";
1131-
else if (!VD->isSetterAccessibleFrom(DC))
1132-
message += " setter is inaccessible";
1133-
else {
1134-
message += " is immutable";
1135-
}
1136-
1137-
emitDiagnostic(Loc, DeclDiagnostic, message)
1138-
.highlight(immInfo.first->getSourceRange());
1139-
1140-
// If there is a masked instance variable of the same type, emit a
1141-
// note to fixit prepend a 'self.'.
1142-
if (auto typeContext = DC->getInnermostTypeContext()) {
1143-
UnqualifiedLookup lookup(VD->getFullName(), typeContext,
1144-
getASTContext().getLazyResolver());
1145-
for (auto &result : lookup.Results) {
1146-
const VarDecl *typeVar = dyn_cast<VarDecl>(result.getValueDecl());
1147-
if (typeVar && typeVar != VD && typeVar->isSettable(DC) &&
1148-
typeVar->isSetterAccessibleFrom(DC) &&
1149-
typeVar->getType()->isEqual(VD->getType())) {
1150-
// But not in its own accessor.
1151-
auto AD =
1152-
dyn_cast_or_null<AccessorDecl>(DC->getInnermostMethodContext());
1153-
if (!AD || AD->getStorage() != typeVar) {
1154-
emitDiagnostic(Loc, diag::masked_instance_variable,
1155-
typeContext->getSelfTypeInContext())
1156-
.fixItInsert(Loc, "self.");
1110+
Optional<OverloadChoice> choice = immInfo.second;
1111+
1112+
// Attempt diagnostics based on the overload choice.
1113+
if (choice.hasValue()) {
1114+
1115+
auto getKeyPathArgument = [](SubscriptExpr *expr) {
1116+
auto *TE = dyn_cast<TupleExpr>(expr->getIndex());
1117+
assert(TE->getNumElements() == 1);
1118+
assert(TE->getElementName(0).str() == "keyPath");
1119+
return TE->getElement(0);
1120+
};
1121+
1122+
if (!choice->isDecl()) {
1123+
if (choice->getKind() == OverloadChoiceKind::KeyPathApplication &&
1124+
!isa<ApplyExpr>(immInfo.first)) {
1125+
std::string message = "key path is read-only";
1126+
if (auto *SE = dyn_cast<SubscriptExpr>(immInfo.first)) {
1127+
if (auto *DRE = dyn_cast<DeclRefExpr>(getKeyPathArgument(SE))) {
1128+
auto identifier = DRE->getDecl()->getBaseName().getIdentifier();
1129+
message =
1130+
"'" + identifier.str().str() + "' is a read-only key path";
11571131
}
11581132
}
1133+
emitDiagnostic(Loc, DeclDiagnostic, message)
1134+
.highlight(immInfo.first->getSourceRange());
1135+
return true;
11591136
}
1137+
return false;
11601138
}
11611139

1162-
// If this is a simple variable marked with a 'let', emit a note to fixit
1163-
// hint it to 'var'.
1164-
VD->emitLetToVarNoteIfSimple(DC);
1165-
return true;
1166-
}
1140+
// Otherwise, we cannot resolve this because the available setter candidates
1141+
// are all mutating and the base must be mutating. If we dug out a
1142+
// problematic decl, we can produce a nice tailored diagnostic.
1143+
if (auto *VD = dyn_cast<VarDecl>(choice->getDecl())) {
1144+
std::string message = "'";
1145+
message += VD->getName().str().str();
1146+
message += "'";
1147+
1148+
auto type = getType(immInfo.first);
1149+
1150+
if (isKnownKeyPathType(type))
1151+
message += " is read-only";
1152+
else if (VD->isCaptureList())
1153+
message += " is an immutable capture";
1154+
else if (VD->isImplicit())
1155+
message += " is immutable";
1156+
else if (VD->isLet())
1157+
message += " is a 'let' constant";
1158+
else if (!VD->isSettable(DC))
1159+
message += " is a get-only property";
1160+
else if (!VD->isSetterAccessibleFrom(DC))
1161+
message += " setter is inaccessible";
1162+
else {
1163+
message += " is immutable";
1164+
}
11671165

1168-
// If the underlying expression was a read-only subscript, diagnose that.
1169-
if (auto *SD = dyn_cast_or_null<SubscriptDecl>(immInfo.second)) {
1170-
StringRef message;
1171-
if (!SD->isSettable())
1172-
message = "subscript is get-only";
1173-
else if (!SD->isSetterAccessibleFrom(DC))
1174-
message = "subscript setter is inaccessible";
1175-
else
1176-
message = "subscript is immutable";
1166+
emitDiagnostic(Loc, DeclDiagnostic, message)
1167+
.highlight(immInfo.first->getSourceRange());
1168+
1169+
// If there is a masked instance variable of the same type, emit a
1170+
// note to fixit prepend a 'self.'.
1171+
if (auto typeContext = DC->getInnermostTypeContext()) {
1172+
UnqualifiedLookup lookup(VD->getFullName(), typeContext,
1173+
getASTContext().getLazyResolver());
1174+
for (auto &result : lookup.Results) {
1175+
const VarDecl *typeVar = dyn_cast<VarDecl>(result.getValueDecl());
1176+
if (typeVar && typeVar != VD && typeVar->isSettable(DC) &&
1177+
typeVar->isSetterAccessibleFrom(DC) &&
1178+
typeVar->getType()->isEqual(VD->getType())) {
1179+
// But not in its own accessor.
1180+
auto AD =
1181+
dyn_cast_or_null<AccessorDecl>(DC->getInnermostMethodContext());
1182+
if (!AD || AD->getStorage() != typeVar) {
1183+
emitDiagnostic(Loc, diag::masked_instance_variable,
1184+
typeContext->getSelfTypeInContext())
1185+
.fixItInsert(Loc, "self.");
1186+
}
1187+
}
1188+
}
1189+
}
11771190

1178-
emitDiagnostic(Loc, DeclDiagnostic, message)
1179-
.highlight(immInfo.first->getSourceRange());
1180-
return true;
1181-
}
1191+
// If this is a simple variable marked with a 'let', emit a note to fixit
1192+
// hint it to 'var'.
1193+
VD->emitLetToVarNoteIfSimple(DC);
1194+
return true;
1195+
}
11821196

1183-
// If we're trying to set an unapplied method, say that.
1184-
if (auto *VD = immInfo.second) {
1185-
std::string message = "'";
1186-
message += VD->getBaseName().getIdentifier().str();
1187-
message += "'";
1197+
// If the underlying expression was a read-only subscript, diagnose that.
1198+
if (auto *SD = dyn_cast_or_null<SubscriptDecl>(choice->getDecl())) {
1199+
StringRef message;
1200+
if (!SD->isSettable())
1201+
message = "subscript is get-only";
1202+
else if (!SD->isSetterAccessibleFrom(DC))
1203+
message = "subscript setter is inaccessible";
1204+
else
1205+
message = "subscript is immutable";
11881206

1189-
auto diagID = DeclDiagnostic;
1190-
if (auto *AFD = dyn_cast<AbstractFunctionDecl>(VD)) {
1191-
if (AFD->hasImplicitSelfDecl()) {
1192-
message += " is a method";
1193-
diagID = diag::assignment_lhs_is_immutable_variable;
1194-
} else {
1195-
message += " is a function";
1196-
}
1197-
} else
1198-
message += " is not settable";
1207+
emitDiagnostic(Loc, DeclDiagnostic, message)
1208+
.highlight(immInfo.first->getSourceRange());
1209+
return true;
1210+
}
11991211

1200-
emitDiagnostic(Loc, diagID, message)
1201-
.highlight(immInfo.first->getSourceRange());
1202-
return true;
1212+
// If we're trying to set an unapplied method, say that.
1213+
if (auto *VD = choice->getDecl()) {
1214+
std::string message = "'";
1215+
message += VD->getBaseName().getIdentifier().str();
1216+
message += "'";
1217+
1218+
auto diagID = DeclDiagnostic;
1219+
if (auto *AFD = dyn_cast<AbstractFunctionDecl>(VD)) {
1220+
if (AFD->hasImplicitSelfDecl()) {
1221+
message += " is a method";
1222+
diagID = diag::assignment_lhs_is_immutable_variable;
1223+
} else {
1224+
message += " is a function";
1225+
}
1226+
} else
1227+
message += " is not settable";
1228+
1229+
emitDiagnostic(Loc, diagID, message)
1230+
.highlight(immInfo.first->getSourceRange());
1231+
return true;
1232+
}
12031233
}
12041234

1235+
// Fall back to producing diagnostics based on the expression since we
1236+
// couldn't determine anything from the OverloadChoice.
1237+
12051238
// If a keypath was the problem but wasn't resolved into a vardecl
12061239
// it is ambiguous or unable to be used for setting.
12071240
if (auto *KPE = dyn_cast_or_null<KeyPathExpr>(immInfo.first)) {
@@ -1353,40 +1386,60 @@ void AssignmentFailure::fixItChangeInoutArgType(const Expr *arg,
13531386
.fixItReplaceChars(startLoc, endLoc, scratch);
13541387
}
13551388

1356-
std::pair<Expr *, ValueDecl *>
1389+
std::pair<Expr *, Optional<OverloadChoice>>
13571390
AssignmentFailure::resolveImmutableBase(Expr *expr) const {
13581391
auto &cs = getConstraintSystem();
13591392
auto *DC = getDC();
13601393
expr = expr->getValueProvidingExpr();
13611394

1395+
auto isImmutable = [&DC](ValueDecl *decl) {
1396+
if (auto *storage = dyn_cast<AbstractStorageDecl>(decl))
1397+
return !storage->isSettable(nullptr) ||
1398+
!storage->isSetterAccessibleFrom(DC);
1399+
1400+
return false;
1401+
};
1402+
13621403
// Provide specific diagnostics for assignment to subscripts whose base expr
13631404
// is known to be an rvalue.
13641405
if (auto *SE = dyn_cast<SubscriptExpr>(expr)) {
13651406
// If we found a decl for the subscript, check to see if it is a set-only
13661407
// subscript decl.
1367-
SubscriptDecl *member = nullptr;
1368-
if (SE->hasDecl())
1369-
member = dyn_cast_or_null<SubscriptDecl>(SE->getDecl().getDecl());
1370-
1371-
if (!member) {
1372-
auto loc =
1373-
cs.getConstraintLocator(SE, ConstraintLocator::SubscriptMember);
1374-
member = dyn_cast_or_null<SubscriptDecl>(getMemberRef(loc));
1408+
if (SE->hasDecl()) {
1409+
const auto &declRef = SE->getDecl();
1410+
if (auto *subscript =
1411+
dyn_cast_or_null<SubscriptDecl>(declRef.getDecl())) {
1412+
if (isImmutable(subscript))
1413+
return {expr, OverloadChoice(getType(SE->getBase()), subscript,
1414+
FunctionRefKind::DoubleApply)};
1415+
}
13751416
}
13761417

1418+
Optional<OverloadChoice> member = getMemberRef(
1419+
cs.getConstraintLocator(SE, ConstraintLocator::SubscriptMember));
1420+
13771421
// If it isn't settable, return it.
13781422
if (member) {
1379-
if (!member->isSettable() || !member->isSetterAccessibleFrom(DC))
1423+
if (member->isDecl() && isImmutable(member->getDecl()))
13801424
return {expr, member};
1381-
}
13821425

1383-
if (auto tupleExpr = dyn_cast<TupleExpr>(SE->getIndex())) {
1384-
if (tupleExpr->getNumElements() == 1 &&
1385-
tupleExpr->getElementName(0).str() == "keyPath") {
1386-
auto indexType = getType(tupleExpr->getElement(0));
1426+
// We still have a choice, the choice is not a decl
1427+
if (!member->isDecl()) {
1428+
// This must be a keypath application
1429+
assert(member->getKind() == OverloadChoiceKind::KeyPathApplication);
1430+
1431+
auto *argType = getType(SE->getIndex())->castTo<TupleType>();
1432+
assert(argType->getNumElements() == 1);
1433+
1434+
auto indexType = resolveType(argType->getElementType(0));
1435+
13871436
if (auto bgt = indexType->getAs<BoundGenericType>()) {
1388-
if (bgt->getDecl() == getASTContext().getKeyPathDecl())
1389-
return resolveImmutableBase(tupleExpr->getElement(0));
1437+
// In Swift versions lower than 5, this check will fail as read only
1438+
// key paths can masquerade as writable for compatibilty reasons.
1439+
// This is fine as in this case we just fall back on old diagnostics.
1440+
if (bgt->getDecl() == getASTContext().getKeyPathDecl()) {
1441+
return {expr, member};
1442+
}
13901443
}
13911444
}
13921445
}
@@ -1400,18 +1453,12 @@ AssignmentFailure::resolveImmutableBase(Expr *expr) const {
14001453
// If we found a decl for the UDE, check it.
14011454
auto loc = cs.getConstraintLocator(UDE, ConstraintLocator::Member);
14021455

1403-
auto *member = getMemberRef(loc);
1456+
auto member = getMemberRef(loc);
1457+
14041458
// If we can resolve a member, we can determine whether it is settable in
14051459
// this context.
1406-
if (member) {
1407-
auto *memberVD = dyn_cast<VarDecl>(member);
1408-
1409-
// If the member isn't a vardecl (e.g. its a funcdecl), or it isn't
1410-
// settable, then it is the problem: return it.
1411-
if (!memberVD || !member->isSettable(nullptr) ||
1412-
!memberVD->isSetterAccessibleFrom(DC))
1413-
return {expr, member};
1414-
}
1460+
if (member && member->isDecl() && isImmutable(member->getDecl()))
1461+
return {expr, member};
14151462

14161463
// If we weren't able to resolve a member or if it is mutable, then the
14171464
// problem must be with the base, recurse.
@@ -1421,16 +1468,18 @@ AssignmentFailure::resolveImmutableBase(Expr *expr) const {
14211468
if (auto *MRE = dyn_cast<MemberRefExpr>(expr)) {
14221469
// If the member isn't settable, then it is the problem: return it.
14231470
if (auto member = dyn_cast<AbstractStorageDecl>(MRE->getMember().getDecl()))
1424-
if (!member->isSettable(nullptr) || !member->isSetterAccessibleFrom(DC))
1425-
return {expr, member};
1471+
if (isImmutable(member))
1472+
return {expr, OverloadChoice(getType(MRE->getBase()), member,
1473+
FunctionRefKind::SingleApply)};
14261474

14271475
// If we weren't able to resolve a member or if it is mutable, then the
14281476
// problem must be with the base, recurse.
14291477
return resolveImmutableBase(MRE->getBase());
14301478
}
14311479

14321480
if (auto *DRE = dyn_cast<DeclRefExpr>(expr))
1433-
return {expr, DRE->getDecl()};
1481+
return {expr,
1482+
OverloadChoice(Type(), DRE->getDecl(), FunctionRefKind::Unapplied)};
14341483

14351484
// Look through x!
14361485
if (auto *FVE = dyn_cast<ForceValueExpr>(expr))
@@ -1448,13 +1497,17 @@ AssignmentFailure::resolveImmutableBase(Expr *expr) const {
14481497
if (auto *SAE = dyn_cast<SelfApplyExpr>(expr))
14491498
return resolveImmutableBase(SAE->getFn());
14501499

1451-
return {expr, nullptr};
1500+
return {expr, None};
14521501
}
14531502

1454-
ValueDecl *AssignmentFailure::getMemberRef(ConstraintLocator *locator) const {
1503+
Optional<OverloadChoice>
1504+
AssignmentFailure::getMemberRef(ConstraintLocator *locator) const {
14551505
auto member = getOverloadChoiceIfAvailable(locator);
1456-
if (!member || !member->choice.isDecl())
1457-
return nullptr;
1506+
if (!member)
1507+
return None;
1508+
1509+
if (!member->choice.isDecl())
1510+
return member->choice;
14581511

14591512
auto *DC = getDC();
14601513
auto &TC = getTypeChecker();
@@ -1478,14 +1531,14 @@ ValueDecl *AssignmentFailure::getMemberRef(ConstraintLocator *locator) const {
14781531
locator, LocatorPathElt::getKeyPathDynamicMember(keyPath));
14791532

14801533
auto memberRef = getOverloadChoiceIfAvailable(memberLoc);
1481-
return memberRef ? memberRef->choice.getDecl() : nullptr;
1534+
return memberRef ? Optional<OverloadChoice>(memberRef->choice) : None;
14821535
}
14831536

14841537
// If this is a string based dynamic lookup, there is no member declaration.
1485-
return nullptr;
1538+
return None;
14861539
}
14871540

1488-
return decl;
1541+
return member->choice;
14891542
}
14901543

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

0 commit comments

Comments
 (0)