Skip to content

Commit ff0bf39

Browse files
xedinxedin
committed
[ConstraintSystem] Detect and diagnoses use of members with mutating getters in key path
Such use is currently not allowed so variables or subscripts with mutating getters should be rejected and diagnosed. ```swift struct S { var foo: Int { mutating get { return 42 } } subscript(_: Int) -> Bool { mutating get { return false } } } _ = \S.foo _ = \S.[0] ``` Resolves: rdar://problem/49413561 (PR swiftlang#24097)
1 parent e0f1994 commit ff0bf39

File tree

7 files changed

+164
-45
lines changed

7 files changed

+164
-45
lines changed

‎lib/Sema/CSApply.cpp

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4588,11 +4588,7 @@ namespace {
45884588
} else {
45894589
// Key paths don't work with mutating-get properties.
45904590
auto varDecl = cast<VarDecl>(property);
4591-
if (varDecl->isGetterMutating()) {
4592-
cs.TC.diagnose(componentLoc, diag::expr_keypath_mutating_getter,
4593-
property->getFullName());
4594-
}
4595-
4591+
assert(!varDecl->isGetterMutating());
45964592
// Key paths don't currently support static members.
45974593
// There is a fix which diagnoses such situation already.
45984594
assert(!varDecl->isStatic());
@@ -4627,10 +4623,7 @@ namespace {
46274623
SelectedOverload &overload, SourceLoc componentLoc, Expr *indexExpr,
46284624
ArrayRef<Identifier> labels, ConstraintLocator *locator) {
46294625
auto subscript = cast<SubscriptDecl>(overload.choice.getDecl());
4630-
if (subscript->isGetterMutating()) {
4631-
cs.TC.diagnose(componentLoc, diag::expr_keypath_mutating_getter,
4632-
subscript->getFullName());
4633-
}
4626+
assert(!subscript->isGetterMutating());
46344627

46354628
cs.TC.requestMemberLayout(subscript);
46364629

‎lib/Sema/CSDiagnostics.cpp

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2449,9 +2449,8 @@ bool KeyPathSubscriptIndexHashableFailure::diagnoseAsError() {
24492449
return true;
24502450
}
24512451

2452-
bool InvalidStaticMemberRefInKeyPath::diagnoseAsError() {
2452+
SourceLoc InvalidMemberRefInKeyPath::getLoc() const {
24532453
auto *anchor = getRawAnchor();
2454-
auto loc = anchor->getLoc();
24552454

24562455
if (auto *KPE = dyn_cast<KeyPathExpr>(anchor)) {
24572456
auto *locator = getLocator();
@@ -2461,9 +2460,18 @@ bool InvalidStaticMemberRefInKeyPath::diagnoseAsError() {
24612460
});
24622461

24632462
assert(component != locator->getPath().end());
2464-
loc = KPE->getComponents()[component->getValue()].getLoc();
2463+
return KPE->getComponents()[component->getValue()].getLoc();
24652464
}
24662465

2467-
emitDiagnostic(loc, diag::expr_keypath_static_member, Member->getBaseName());
2466+
return anchor->getLoc();
2467+
}
2468+
2469+
bool InvalidStaticMemberRefInKeyPath::diagnoseAsError() {
2470+
emitDiagnostic(getLoc(), diag::expr_keypath_static_member, getName());
2471+
return true;
2472+
}
2473+
2474+
bool InvalidMemberWithMutatingGetterInKeyPath::diagnoseAsError() {
2475+
emitDiagnostic(getLoc(), diag::expr_keypath_mutating_getter, getName());
24682476
return true;
24692477
}

‎lib/Sema/CSDiagnostics.h

Lines changed: 50 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1031,6 +1031,26 @@ class KeyPathSubscriptIndexHashableFailure final : public FailureDiagnostic {
10311031
bool diagnoseAsError() override;
10321032
};
10331033

1034+
class InvalidMemberRefInKeyPath : public FailureDiagnostic {
1035+
ValueDecl *Member;
1036+
1037+
public:
1038+
InvalidMemberRefInKeyPath(Expr *root, ConstraintSystem &cs, ValueDecl *member,
1039+
ConstraintLocator *locator)
1040+
: FailureDiagnostic(root, cs, locator), Member(member) {
1041+
assert(member->hasName());
1042+
assert(locator->isForKeyPathComponent());
1043+
}
1044+
1045+
DeclName getName() const { return Member->getFullName(); }
1046+
1047+
bool diagnoseAsError() override = 0;
1048+
1049+
protected:
1050+
/// Compute location of the failure for diagnostic.
1051+
SourceLoc getLoc() const;
1052+
};
1053+
10341054
/// Diagnose an attempt to reference a static member as a key path component
10351055
/// e.g.
10361056
///
@@ -1041,16 +1061,39 @@ class KeyPathSubscriptIndexHashableFailure final : public FailureDiagnostic {
10411061
///
10421062
/// _ = \S.Type.foo
10431063
/// ```
1044-
class InvalidStaticMemberRefInKeyPath final : public FailureDiagnostic {
1045-
ValueDecl *Member;
1046-
1064+
class InvalidStaticMemberRefInKeyPath final : public InvalidMemberRefInKeyPath {
10471065
public:
10481066
InvalidStaticMemberRefInKeyPath(Expr *root, ConstraintSystem &cs,
10491067
ValueDecl *member, ConstraintLocator *locator)
1050-
: FailureDiagnostic(root, cs, locator), Member(member) {
1051-
assert(member->hasName());
1052-
assert(locator->isForKeyPathComponent());
1053-
}
1068+
: InvalidMemberRefInKeyPath(root, cs, member, locator) {}
1069+
1070+
bool diagnoseAsError() override;
1071+
};
1072+
1073+
/// Diagnose an attempt to reference a member which has a mutating getter as a
1074+
/// key path component e.g.
1075+
///
1076+
/// ```swift
1077+
/// struct S {
1078+
/// var foo: Int {
1079+
/// mutating get { return 42 }
1080+
/// }
1081+
///
1082+
/// subscript(_: Int) -> Bool {
1083+
/// mutating get { return false }
1084+
/// }
1085+
/// }
1086+
///
1087+
/// _ = \S.foo
1088+
/// _ = \S.[42]
1089+
/// ```
1090+
class InvalidMemberWithMutatingGetterInKeyPath final
1091+
: public InvalidMemberRefInKeyPath {
1092+
public:
1093+
InvalidMemberWithMutatingGetterInKeyPath(Expr *root, ConstraintSystem &cs,
1094+
ValueDecl *member,
1095+
ConstraintLocator *locator)
1096+
: InvalidMemberRefInKeyPath(root, cs, member, locator) {}
10541097

10551098
bool diagnoseAsError() override;
10561099
};

‎lib/Sema/CSFix.cpp

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -423,15 +423,26 @@ TreatKeyPathSubscriptIndexAsHashable::create(ConstraintSystem &cs, Type type,
423423
TreatKeyPathSubscriptIndexAsHashable(cs, type, locator);
424424
}
425425

426-
bool AllowStaticMemberRefInKeyPath::diagnose(Expr *root, bool asNote) const {
427-
InvalidStaticMemberRefInKeyPath failure(root, getConstraintSystem(), Member,
428-
getLocator());
429-
return failure.diagnose(asNote);
426+
bool AllowInvalidRefInKeyPath::diagnose(Expr *root, bool asNote) const {
427+
switch (Kind) {
428+
case RefKind::StaticMember: {
429+
InvalidStaticMemberRefInKeyPath failure(root, getConstraintSystem(), Member,
430+
getLocator());
431+
return failure.diagnose(asNote);
432+
}
433+
434+
case RefKind::MutatingGetter: {
435+
InvalidMemberWithMutatingGetterInKeyPath failure(
436+
root, getConstraintSystem(), Member, getLocator());
437+
return failure.diagnose(asNote);
438+
}
439+
}
430440
}
431441

432-
AllowStaticMemberRefInKeyPath *
433-
AllowStaticMemberRefInKeyPath::create(ConstraintSystem &cs, ValueDecl *member,
434-
ConstraintLocator *locator) {
442+
AllowInvalidRefInKeyPath *
443+
AllowInvalidRefInKeyPath::create(ConstraintSystem &cs, RefKind kind,
444+
ValueDecl *member,
445+
ConstraintLocator *locator) {
435446
return new (cs.getAllocator())
436-
AllowStaticMemberRefInKeyPath(cs, member, locator);
447+
AllowInvalidRefInKeyPath(cs, kind, member, locator);
437448
}

‎lib/Sema/CSFix.h

Lines changed: 39 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -144,8 +144,9 @@ enum class FixKind : uint8_t {
144144
/// of the index arguments to be Hashable.
145145
TreatKeyPathSubscriptIndexAsHashable,
146146

147-
/// Allow a reference to a static member as a key path component.
148-
AllowStaticMemberRefInKeyPath,
147+
/// Allow an invalid reference to a member declaration as part
148+
/// of a key path component.
149+
AllowInvalidRefInKeyPath,
149150
};
150151

151152
class ConstraintFix {
@@ -783,23 +784,51 @@ class TreatKeyPathSubscriptIndexAsHashable final : public ConstraintFix {
783784
create(ConstraintSystem &cs, Type type, ConstraintLocator *locator);
784785
};
785786

786-
class AllowStaticMemberRefInKeyPath final : public ConstraintFix {
787+
class AllowInvalidRefInKeyPath final : public ConstraintFix {
788+
enum RefKind {
789+
// Allow a reference to a static member as a key path component.
790+
StaticMember,
791+
// Allow a reference to a declaration with mutating getter as
792+
// a key path component.
793+
MutatingGetter,
794+
} Kind;
795+
787796
ValueDecl *Member;
788797

789-
AllowStaticMemberRefInKeyPath(ConstraintSystem &cs, ValueDecl *member,
790-
ConstraintLocator *locator)
791-
: ConstraintFix(cs, FixKind::AllowStaticMemberRefInKeyPath, locator),
792-
Member(member) {}
798+
AllowInvalidRefInKeyPath(ConstraintSystem &cs, RefKind kind,
799+
ValueDecl *member, ConstraintLocator *locator)
800+
: ConstraintFix(cs, FixKind::AllowInvalidRefInKeyPath, locator),
801+
Kind(kind), Member(member) {}
793802

794803
public:
795804
std::string getName() const override {
796-
return "allow reference to a static member as a key path component";
805+
switch (Kind) {
806+
case RefKind::StaticMember:
807+
return "allow reference to a static member as a key path component";
808+
case RefKind::MutatingGetter:
809+
return "allow reference to a member with mutating getter as a key "
810+
"path component";
811+
}
797812
}
798813

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

801-
static AllowStaticMemberRefInKeyPath *
802-
create(ConstraintSystem &cs, ValueDecl *member, ConstraintLocator *locator);
816+
static AllowInvalidRefInKeyPath *forStaticMember(ConstraintSystem &cs,
817+
ValueDecl *member,
818+
ConstraintLocator *locator) {
819+
return create(cs, RefKind::StaticMember, member, locator);
820+
}
821+
822+
static AllowInvalidRefInKeyPath *
823+
forMutatingGetter(ConstraintSystem &cs, ValueDecl *member,
824+
ConstraintLocator *locator) {
825+
return create(cs, RefKind::MutatingGetter, member, locator);
826+
}
827+
828+
private:
829+
static AllowInvalidRefInKeyPath *create(ConstraintSystem &cs, RefKind kind,
830+
ValueDecl *member,
831+
ConstraintLocator *locator);
803832
};
804833

805834
} // end namespace constraints

‎lib/Sema/CSSimplify.cpp

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5023,14 +5023,21 @@ ConstraintSystem::simplifyKeyPathConstraint(Type keyPathTy,
50235023
}
50245024

50255025
// Referencing static members in key path is not currently allowed.
5026-
if (storage->isStatic()) {
5026+
if (storage->isStatic() || storage->isGetterMutating()) {
50275027
if (!shouldAttemptFixes())
50285028
return SolutionKind::Error;
50295029

5030-
auto componentLoc =
5031-
locator.withPathElement(LocatorPathElt::getKeyPathComponent(i));
5032-
auto *fix = AllowStaticMemberRefInKeyPath::create(
5033-
*this, choices[i].getDecl(), getConstraintLocator(componentLoc));
5030+
auto *componentLoc = getConstraintLocator(
5031+
locator.withPathElement(LocatorPathElt::getKeyPathComponent(i)));
5032+
5033+
ConstraintFix *fix = nullptr;
5034+
if (storage->isStatic()) {
5035+
fix = AllowInvalidRefInKeyPath::forStaticMember(
5036+
*this, choices[i].getDecl(), componentLoc);
5037+
} else {
5038+
fix = AllowInvalidRefInKeyPath::forMutatingGetter(
5039+
*this, choices[i].getDecl(), componentLoc);
5040+
}
50345041

50355042
if (recordFix(fix))
50365043
return SolutionKind::Error;
@@ -6330,7 +6337,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
63306337
case FixKind::AllowInaccessibleMember:
63316338
case FixKind::AllowAnyObjectKeyPathRoot:
63326339
case FixKind::TreatKeyPathSubscriptIndexAsHashable:
6333-
case FixKind::AllowStaticMemberRefInKeyPath:
6340+
case FixKind::AllowInvalidRefInKeyPath:
63346341
llvm_unreachable("handled elsewhere");
63356342
}
63366343

‎test/expr/unary/keypath/keypath.swift

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -732,7 +732,7 @@ func test_keypath_with_static_members(_ p: P_With_Static_Members) {
732732
static var baz: Int = 42
733733
}
734734

735-
func foo(_ s: S) {
735+
func foo(_ s: S) {
736736
let _ = \S.Type.foo
737737
// expected-error@-1 {{key path cannot refer to static member 'foo'}}
738738
let _ = s[keyPath: \.foo]
@@ -748,6 +748,34 @@ func foo(_ s: S) {
748748
}
749749
}
750750

751+
func test_keypath_with_mutating_getter() {
752+
struct S {
753+
var foo: Int {
754+
mutating get { return 42 }
755+
}
756+
757+
subscript(_: Int) -> [Int] {
758+
mutating get { return [] }
759+
}
760+
}
761+
762+
_ = \S.foo
763+
// expected-error@-1 {{key path cannot refer to 'foo', which has a mutating getter}}
764+
let _: KeyPath<S, Int> = \.foo
765+
// expected-error@-1 {{key path cannot refer to 'foo', which has a mutating getter}}
766+
_ = \S.[0]
767+
// expected-error@-1 {{key path cannot refer to 'subscript(_:)', which has a mutating getter}}
768+
_ = \S.[0].count
769+
// expected-error@-1 {{key path cannot refer to 'subscript(_:)', which has a mutating getter}}
770+
771+
func test_via_subscript(_ s: S) {
772+
_ = s[keyPath: \.foo]
773+
// expected-error@-1 {{key path cannot refer to 'foo', which has a mutating getter}}
774+
_ = s[keyPath: \.[0].count]
775+
// expected-error@-1 {{key path cannot refer to 'subscript(_:)', which has a mutating getter}}
776+
}
777+
}
778+
751779
func testSyntaxErrors() { // expected-note{{}}
752780
_ = \. ; // expected-error{{expected member name following '.'}}
753781
_ = \.a ;

0 commit comments

Comments
 (0)