Skip to content

Commit 1e51d30

Browse files
tshortlitshortli
authored
Merge pull request swiftlang#83931 from tshortli/key-path-availability
Sema: Diagnose key path setter availability
2 parents 5eb727b + 2d5824e commit 1e51d30

File tree

3 files changed

+173
-50
lines changed

3 files changed

+173
-50
lines changed

‎lib/Sema/TypeCheckAvailability.cpp‎

Lines changed: 124 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -2204,7 +2204,7 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker {
22042204
/// Models how member references will translate to accessor usage. This is
22052205
/// used to diagnose the availability of individual accessors that may be
22062206
/// called by the expression being checked.
2207-
enum class MemberAccessContext : unsigned {
2207+
enum class MemberAccessContext : uint8_t {
22082208
/// The starting access context for the root of any expression tree. In this
22092209
/// context, a member access will call the get accessor only.
22102210
Default,
@@ -2225,11 +2225,48 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker {
22252225
Writeback
22262226
};
22272227

2228+
/// Models how key path references will translate to accessor usage. This is
2229+
/// used to diagnose the availability of individual accessors that may be
2230+
/// called by the expression being checked.
2231+
enum class KeyPathAccessContext : uint8_t {
2232+
/// The context does not involve a key path access.
2233+
None,
2234+
2235+
/// The context is an expression that is coerced to a read-only key path.
2236+
ReadOnlyCoercion,
2237+
2238+
/// The context is a key path application (`x[keyPath: \.member]`).
2239+
Application,
2240+
};
2241+
22282242
ASTContext &Context;
2229-
MemberAccessContext AccessContext = MemberAccessContext::Default;
22302243
SmallVector<const Expr *, 16> ExprStack;
22312244
SmallVector<bool, 4> PreconcurrencyCalleeStack;
22322245
const ExportContext &Where;
2246+
MemberAccessContext MemberAccess = MemberAccessContext::Default;
2247+
KeyPathAccessContext KeyPathAccess = KeyPathAccessContext::None;
2248+
bool InInOutExpr = false;
2249+
2250+
/// A categorization of which accessors are used by a given storage access.
2251+
enum class StorageAccessKind {
2252+
Get,
2253+
Set,
2254+
GetSet,
2255+
};
2256+
2257+
/// Returns the storage access kind as indicated by the current member access
2258+
/// context.
2259+
StorageAccessKind getMemberStorageAccessKind() const {
2260+
switch (MemberAccess) {
2261+
case MemberAccessContext::Default:
2262+
case MemberAccessContext::Load:
2263+
return StorageAccessKind::Get;
2264+
case MemberAccessContext::Assignment:
2265+
return StorageAccessKind::Set;
2266+
case MemberAccessContext::Writeback:
2267+
return StorageAccessKind::GetSet;
2268+
}
2269+
}
22332270

22342271
public:
22352272
explicit ExprAvailabilityWalker(const ExportContext &Where)
@@ -2238,7 +2275,7 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker {
22382275
PreWalkAction walkToArgumentPre(const Argument &Arg) override {
22392276
// Arguments should be walked in their own member access context which
22402277
// starts out read-only by default.
2241-
walkInContext(Arg.getExpr(), MemberAccessContext::Default);
2278+
walkInMemberAccessContext(Arg.getExpr(), MemberAccessContext::Default);
22422279
return Action::SkipChildren();
22432280
}
22442281

@@ -2255,7 +2292,8 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker {
22552292
if (auto DR = dyn_cast<DeclRefExpr>(E)) {
22562293
diagnoseDeclRefAvailability(DR->getDeclRef(), DR->getSourceRange(),
22572294
getEnclosingApplyExpr(), std::nullopt);
2258-
maybeDiagStorageAccess(DR->getDecl(), DR->getSourceRange(), DC);
2295+
maybeDiagStorageAccess(DR->getDecl(), getMemberStorageAccessKind(),
2296+
DR->getSourceRange(), DC);
22592297
}
22602298
if (auto MR = dyn_cast<MemberRefExpr>(E)) {
22612299
walkMemberRef(MR);
@@ -2274,7 +2312,9 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker {
22742312
if (auto S = dyn_cast<SubscriptExpr>(E)) {
22752313
if (S->hasDecl()) {
22762314
diagnoseDeclRefAvailability(S->getDecl(), S->getSourceRange(), S);
2277-
maybeDiagStorageAccess(S->getDecl().getDecl(), S->getSourceRange(), DC);
2315+
maybeDiagStorageAccess(S->getDecl().getDecl(),
2316+
getMemberStorageAccessKind(),
2317+
S->getSourceRange(), DC);
22782318
PreconcurrencyCalleeStack.push_back(
22792319
hasReferenceToPreconcurrencyDecl(S));
22802320
}
@@ -2321,9 +2361,24 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker {
23212361
FCE->getLoc(),
23222362
Where.getDeclContext());
23232363
}
2364+
if (auto DTBE = dyn_cast<DerivedToBaseExpr>(E)) {
2365+
if (auto ty = DTBE->getType()) {
2366+
if (ty->isKeyPath()) {
2367+
walkInKeyPathAccessContext(DTBE->getSubExpr(),
2368+
KeyPathAccessContext::ReadOnlyCoercion);
2369+
return Action::SkipChildren(E);
2370+
}
2371+
}
2372+
}
23242373
if (auto KP = dyn_cast<KeyPathExpr>(E)) {
23252374
maybeDiagKeyPath(KP);
23262375
}
2376+
if (auto KPAE = dyn_cast<KeyPathApplicationExpr>(E)) {
2377+
KPAE->getBase()->walk(*this);
2378+
walkInKeyPathAccessContext(KPAE->getKeyPath(),
2379+
KeyPathAccessContext::Application);
2380+
return Action::SkipChildren(E);
2381+
}
23272382
if (auto A = dyn_cast<AssignExpr>(E)) {
23282383
// Attempting to assign to a @preconcurrency declaration should
23292384
// downgrade Sendable conformance mismatches to warnings.
@@ -2402,7 +2457,7 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker {
24022457
}
24032458

24042459
if (auto LE = dyn_cast<LoadExpr>(E)) {
2405-
walkLoadExpr(LE);
2460+
walkInMemberAccessContext(LE->getSubExpr(), MemberAccessContext::Load);
24062461
return Action::SkipChildren(E);
24072462
}
24082463

@@ -2489,19 +2544,14 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker {
24892544
// encountered walking (pre-order) is the Dest is the destination of the
24902545
// write. For the moment this is fine -- but future syntax might violate
24912546
// this assumption.
2492-
walkInContext(Dest, MemberAccessContext::Assignment);
2547+
walkInMemberAccessContext(Dest, MemberAccessContext::Assignment);
24932548

24942549
// Check RHS in getter context
24952550
Expr *Source = E->getSrc();
24962551
if (!Source) {
24972552
return;
24982553
}
2499-
walkInContext(Source, MemberAccessContext::Default);
2500-
}
2501-
2502-
/// Walk a load expression, checking for availability.
2503-
void walkLoadExpr(LoadExpr *E) {
2504-
walkInContext(E->getSubExpr(), MemberAccessContext::Load);
2554+
walkInMemberAccessContext(Source, MemberAccessContext::Default);
25052555
}
25062556

25072557
/// Walk a member reference expression, checking for availability.
@@ -2517,10 +2567,10 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker {
25172567
// ╰─── MemberAccessContext::Writeback
25182568
//
25192569
MemberAccessContext accessContext =
2520-
(AccessContext == MemberAccessContext::Assignment)
2570+
(MemberAccess == MemberAccessContext::Assignment)
25212571
? MemberAccessContext::Writeback
2522-
: AccessContext;
2523-
walkInContext(E->getBase(), accessContext);
2572+
: MemberAccess;
2573+
walkInMemberAccessContext(E->getBase(), accessContext);
25242574

25252575
ConcreteDeclRef DR = E->getMember();
25262576
// Diagnose for the member declaration itself.
@@ -2530,7 +2580,32 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker {
25302580

25312581
// Diagnose for appropriate accessors, given the access context.
25322582
auto *DC = Where.getDeclContext();
2533-
maybeDiagStorageAccess(DR.getDecl(), E->getSourceRange(), DC);
2583+
maybeDiagStorageAccess(DR.getDecl(), getMemberStorageAccessKind(),
2584+
E->getSourceRange(), DC);
2585+
}
2586+
2587+
StorageAccessKind getStorageAccessKindForKeyPath(KeyPathExpr *KP) const {
2588+
switch (KeyPathAccess) {
2589+
case KeyPathAccessContext::None:
2590+
// Use the key path's type to determine the access kind.
2591+
if (!KP->isObjC())
2592+
if (auto keyPathType = KP->getKeyPathType())
2593+
if (keyPathType->isKeyPath())
2594+
return StorageAccessKind::Get;
2595+
2596+
return StorageAccessKind::GetSet;
2597+
2598+
case KeyPathAccessContext::ReadOnlyCoercion:
2599+
// The type of this key path is being coerced to the type KeyPath<_, _> so
2600+
// ignore the actual key path type.
2601+
return StorageAccessKind::Get;
2602+
2603+
case KeyPathAccessContext::Application:
2604+
// The key path is being applied directly to a base so treat it as if it
2605+
// were a direct access to the member in the current member access
2606+
// context.
2607+
return getMemberStorageAccessKind();
2608+
}
25342609
}
25352610

25362611
/// Walk a keypath expression, checking all of its components for
@@ -2541,6 +2616,8 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker {
25412616
if (KP->isObjC())
25422617
flags = DeclAvailabilityFlag::ForObjCKeyPath;
25432618

2619+
auto accessKind = getStorageAccessKindForKeyPath(KP);
2620+
25442621
for (auto &component : KP->getComponents()) {
25452622
switch (component.getKind()) {
25462623
case KeyPathExpr::Component::Kind::Member:
@@ -2550,7 +2627,7 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker {
25502627
auto range = component.getSourceRange();
25512628
if (diagnoseDeclRefAvailability(decl, loc, nullptr, flags))
25522629
break;
2553-
maybeDiagStorageAccess(decl.getDecl(), range, declContext);
2630+
maybeDiagStorageAccess(decl.getDecl(), accessKind, range, declContext);
25542631
break;
25552632
}
25562633

@@ -2575,13 +2652,15 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker {
25752652

25762653
/// Walk an inout expression, checking for availability.
25772654
void walkInOutExpr(InOutExpr *E) {
2655+
llvm::SaveAndRestore<bool> S(this->InInOutExpr, true);
2656+
25782657
// Typically an InOutExpr should begin a `Writeback` context. However,
25792658
// inside a LoadExpr this transition is suppressed since the entire
25802659
// expression is being coerced to an r-value.
2581-
auto accessContext = AccessContext != MemberAccessContext::Load
2660+
auto accessContext = MemberAccess != MemberAccessContext::Load
25822661
? MemberAccessContext::Writeback
2583-
: AccessContext;
2584-
walkInContext(E->getSubExpr(), accessContext);
2662+
: MemberAccess;
2663+
walkInMemberAccessContext(E->getSubExpr(), accessContext);
25852664
}
25862665

25872666
/// Walk an abstract closure expression, checking for availability
@@ -2598,16 +2677,23 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker {
25982677
return;
25992678
}
26002679

2601-
/// Walk the given expression in the member access context.
2602-
void walkInContext(Expr *E, MemberAccessContext AccessContext) {
2603-
llvm::SaveAndRestore<MemberAccessContext>
2604-
C(this->AccessContext, AccessContext);
2680+
/// Walk the given expression in a specific member access context.
2681+
void walkInMemberAccessContext(Expr *E, MemberAccessContext AccessContext) {
2682+
llvm::SaveAndRestore<MemberAccessContext> C(this->MemberAccess,
2683+
AccessContext);
2684+
E->walk(*this);
2685+
}
2686+
2687+
/// Walk the given expression in a specific key path access context.
2688+
void walkInKeyPathAccessContext(Expr *E, KeyPathAccessContext AccessContext) {
2689+
llvm::SaveAndRestore<KeyPathAccessContext> C(this->KeyPathAccess,
2690+
AccessContext);
26052691
E->walk(*this);
26062692
}
26072693

26082694
/// Emit diagnostics, if necessary, for accesses to storage where
26092695
/// the accessor for the AccessContext is not available.
2610-
void maybeDiagStorageAccess(const ValueDecl *VD,
2696+
void maybeDiagStorageAccess(const ValueDecl *VD, StorageAccessKind accessKind,
26112697
SourceRange ReferenceRange,
26122698
const DeclContext *ReferenceDC) const {
26132699
if (Context.LangOpts.DisableAvailabilityChecking)
@@ -2621,30 +2707,28 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker {
26212707
return;
26222708
}
26232709

2710+
DeclAvailabilityFlags flags;
2711+
if (InInOutExpr)
2712+
flags |= DeclAvailabilityFlag::ForInout;
2713+
26242714
// Check availability of accessor functions.
26252715
// TODO: if we're talking about an inlineable storage declaration,
26262716
// this probably needs to be refined to not assume that the accesses are
26272717
// specifically using the getter/setter.
2628-
switch (AccessContext) {
2629-
case MemberAccessContext::Default:
2630-
case MemberAccessContext::Load:
2718+
switch (accessKind) {
2719+
case StorageAccessKind::Get:
26312720
diagAccessorAvailability(D->getOpaqueAccessor(AccessorKind::Get),
2632-
ReferenceRange, ReferenceDC, std::nullopt);
2721+
ReferenceRange, ReferenceDC, flags);
26332722
break;
2634-
2635-
case MemberAccessContext::Assignment:
2723+
case StorageAccessKind::Set:
26362724
diagAccessorAvailability(D->getOpaqueAccessor(AccessorKind::Set),
2637-
ReferenceRange, ReferenceDC, std::nullopt);
2725+
ReferenceRange, ReferenceDC, flags);
26382726
break;
2639-
2640-
case MemberAccessContext::Writeback:
2727+
case StorageAccessKind::GetSet:
26412728
diagAccessorAvailability(D->getOpaqueAccessor(AccessorKind::Get),
2642-
ReferenceRange, ReferenceDC,
2643-
DeclAvailabilityFlag::ForInout);
2644-
2729+
ReferenceRange, ReferenceDC, flags);
26452730
diagAccessorAvailability(D->getOpaqueAccessor(AccessorKind::Set),
2646-
ReferenceRange, ReferenceDC,
2647-
DeclAvailabilityFlag::ForInout);
2731+
ReferenceRange, ReferenceDC, flags);
26482732
break;
26492733
}
26502734
}

0 commit comments

Comments
 (0)