From 2467b931a7dc91354ad6ac786aeedc1c4d3f920c Mon Sep 17 00:00:00 2001 From: Allan Shortlidge Date: Wed, 24 Sep 2025 17:27:37 -0700 Subject: [PATCH 1/2] ConstraintSystem: Move key path type utilities to AST. --- include/swift/AST/Types.h | 6 ++++++ include/swift/Sema/ConstraintSystem.h | 4 ---- lib/AST/Type.cpp | 9 +++++++++ lib/IDE/SelectedOverloadInfo.cpp | 2 +- lib/Sema/CSApply.cpp | 2 +- lib/Sema/CSBindings.cpp | 9 +++++---- lib/Sema/CSDiagnostics.cpp | 6 +++--- lib/Sema/CSSimplify.cpp | 2 +- lib/Sema/ConstraintSystem.cpp | 6 ------ lib/Sema/TypeOfReference.cpp | 4 ++-- 10 files changed, 28 insertions(+), 22 deletions(-) diff --git a/include/swift/AST/Types.h b/include/swift/AST/Types.h index c6b12e2e33278..eee1dea674fc4 100644 --- a/include/swift/AST/Types.h +++ b/include/swift/AST/Types.h @@ -1103,6 +1103,12 @@ class alignas(1 << TypeAlignInBits) TypeBase /// Check if this is a std.string type from C++. bool isCxxString(); + /// Check if this type is known to represent key paths. + bool isKnownKeyPathType(); + + /// Check if this type is known to represent immutable key paths. + bool isKnownImmutableKeyPathType(); + /// Check if this is either an Array, Set or Dictionary collection type defined /// at the top level of the Swift module bool isKnownStdlibCollectionType(); diff --git a/include/swift/Sema/ConstraintSystem.h b/include/swift/Sema/ConstraintSystem.h index 1bc1ad5f7f16b..358893257487e 100644 --- a/include/swift/Sema/ConstraintSystem.h +++ b/include/swift/Sema/ConstraintSystem.h @@ -6525,10 +6525,6 @@ class TypeVarRefCollector : public ASTWalker { } }; -/// Determine whether given type is a known one -/// for a key path `{Any, Partial, Writable, ReferenceWritable}KeyPath`. -bool isKnownKeyPathType(Type type); - /// Determine whether the given type is a PartialKeyPath and /// AnyKeyPath or existential type thererof, for example, /// `PartialKeyPath<...> & Sendable`. diff --git a/lib/AST/Type.cpp b/lib/AST/Type.cpp index 65a7620d017ca..ba6a64f87c442 100644 --- a/lib/AST/Type.cpp +++ b/lib/AST/Type.cpp @@ -1309,6 +1309,15 @@ bool TypeBase::isCxxString() { ctx.Id_basic_string.is(clangDecl->getName()); } +bool TypeBase::isKnownKeyPathType() { + return isKeyPath() || isWritableKeyPath() || isReferenceWritableKeyPath() || + isPartialKeyPath() || isAnyKeyPath(); +} + +bool TypeBase::isKnownImmutableKeyPathType() { + return isKeyPath() || isPartialKeyPath() || isAnyKeyPath(); +} + bool TypeBase::isKnownStdlibCollectionType() { if (isArray() || isDictionary() || isSet()) { return true; diff --git a/lib/IDE/SelectedOverloadInfo.cpp b/lib/IDE/SelectedOverloadInfo.cpp index d9a339116fb6c..2fbb1c40c04c9 100644 --- a/lib/IDE/SelectedOverloadInfo.cpp +++ b/lib/IDE/SelectedOverloadInfo.cpp @@ -79,7 +79,7 @@ swift::ide::getSelectedOverloadInfo(const Solution &S, fnType->getParams()[0].getPlainType()->castTo(); auto *keyPathDecl = keyPathTy->getAnyNominal(); - assert(isKnownKeyPathType(keyPathTy) && + assert(keyPathTy->isKnownKeyPathType() && "parameter is supposed to be a keypath"); auto KeyPathDynamicLocator = CS.getConstraintLocator( diff --git a/lib/Sema/CSApply.cpp b/lib/Sema/CSApply.cpp index 79376e002ee95..167f25c810899 100644 --- a/lib/Sema/CSApply.cpp +++ b/lib/Sema/CSApply.cpp @@ -2689,7 +2689,7 @@ namespace { Type keyPathTy = paramType; if (auto *existential = keyPathTy->getAs()) { keyPathTy = existential->getSuperclass(); - assert(isKnownKeyPathType(keyPathTy)); + assert(keyPathTy->isKnownKeyPathType()); } SmallVector components; diff --git a/lib/Sema/CSBindings.cpp b/lib/Sema/CSBindings.cpp index b42bad750a6b7..5c43218521578 100644 --- a/lib/Sema/CSBindings.cpp +++ b/lib/Sema/CSBindings.cpp @@ -513,7 +513,7 @@ void BindingSet::inferTransitiveBindings() { auto bindingTy = binding.BindingType->lookThroughAllOptionalTypes(); Type inferredRootTy; - if (isKnownKeyPathType(bindingTy)) { + if (bindingTy->isKnownKeyPathType()) { // AnyKeyPath doesn't have a root type. if (bindingTy->isAnyKeyPath()) continue; @@ -727,7 +727,8 @@ bool BindingSet::finalize(bool transitive) { for (const auto &binding : Bindings) { auto bindingTy = binding.BindingType->lookThroughAllOptionalTypes(); - assert(isKnownKeyPathType(bindingTy) || bindingTy->is()); + assert(bindingTy->isKnownKeyPathType() || + bindingTy->is()); // Functions don't have capability so we can simply add them. if (auto *fnType = bindingTy->getAs()) { @@ -1694,7 +1695,7 @@ PotentialBindings::inferFromRelational(ConstraintSystem &CS, if (type->isExistentialType()) { auto layout = type->getExistentialLayout(); if (auto superclass = layout.explicitSuperclass) { - if (isKnownKeyPathType(superclass)) { + if (superclass->isKnownKeyPathType()) { type = superclass; objectTy = superclass; } @@ -1702,7 +1703,7 @@ PotentialBindings::inferFromRelational(ConstraintSystem &CS, } } - if (!(isKnownKeyPathType(objectTy) || objectTy->is())) + if (!(objectTy->isKnownKeyPathType() || objectTy->is())) return std::nullopt; } diff --git a/lib/Sema/CSDiagnostics.cpp b/lib/Sema/CSDiagnostics.cpp index c4e76f309168b..ec6ea6d6d5c68 100644 --- a/lib/Sema/CSDiagnostics.cpp +++ b/lib/Sema/CSDiagnostics.cpp @@ -2262,7 +2262,7 @@ bool AssignmentFailure::diagnoseAsError() { auto type = getType(immutableExpr); - if (isKnownKeyPathType(type)) + if (type->isKnownKeyPathType()) message += " is read-only"; else if (VD->isCaptureList()) message += " is an immutable capture"; @@ -7487,7 +7487,7 @@ bool ArgumentMismatchFailure::diagnoseAsError() { // Unresolved key path argument requires a tailored diagnostic // that doesn't mention a fallback type - `KeyPath<_, _>`. - if (argType->isKeyPath() && !isKnownKeyPathType(paramType)) { + if (argType->isKeyPath() && !paramType->isKnownKeyPathType()) { auto keyPathTy = argType->castTo(); auto rootTy = keyPathTy->getGenericArgs()[0]; if (rootTy->is()) { @@ -7852,7 +7852,7 @@ bool ArgumentMismatchFailure::diagnoseKeyPathAsFunctionResultMismatch() const { auto argType = getFromType(); auto paramType = getToType(); - if (!isKnownKeyPathType(argType)) + if (!argType->isKnownKeyPathType()) return false; auto kpType = argType->castTo(); diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp index 0ff46495538d7..fc1bd2a079d8d 100644 --- a/lib/Sema/CSSimplify.cpp +++ b/lib/Sema/CSSimplify.cpp @@ -5598,7 +5598,7 @@ bool ConstraintSystem::repairFailures( // fix-up here unless last component has already a invalid type or // instance fix recorded. if (isExpr(anchor)) { - if (isKnownKeyPathType(lhs) && isKnownKeyPathType(rhs)) { + if (lhs->isKnownKeyPathType() && rhs->isKnownKeyPathType()) { // If we have a conversion happening here, we should let fix happen in // simplifyRestrictedConstraint. if (hasAnyRestriction()) diff --git a/lib/Sema/ConstraintSystem.cpp b/lib/Sema/ConstraintSystem.cpp index e6d462f004f1a..897de2bf547c3 100644 --- a/lib/Sema/ConstraintSystem.cpp +++ b/lib/Sema/ConstraintSystem.cpp @@ -4363,12 +4363,6 @@ Solution::getFunctionArgApplyInfo(ConstraintLocator *locator) const { fnInterfaceType, fnType, callee); } -bool constraints::isKnownKeyPathType(Type type) { - return type->isKeyPath() || type->isWritableKeyPath() || - type->isReferenceWritableKeyPath() || type->isPartialKeyPath() || - type->isAnyKeyPath(); -} - bool constraints::isTypeErasedKeyPathType(Type type) { assert(type); diff --git a/lib/Sema/TypeOfReference.cpp b/lib/Sema/TypeOfReference.cpp index 52679cb498eee..b92a6e8f0cf97 100644 --- a/lib/Sema/TypeOfReference.cpp +++ b/lib/Sema/TypeOfReference.cpp @@ -2392,13 +2392,13 @@ void ConstraintSystem::bindOverloadType(const SelectedOverload &overload, if (auto *existential = paramTy->getAs()) { paramTy = existential->getSuperclass(); - assert(isKnownKeyPathType(paramTy)); + assert(paramTy->isKnownKeyPathType()); } auto keyPathTy = paramTy->castTo(); auto *keyPathDecl = keyPathTy->getAnyNominal(); - assert(isKnownKeyPathType(keyPathTy) && + assert(keyPathTy->isKnownKeyPathType() && "parameter is supposed to be a keypath"); auto *keyPathLoc = getConstraintLocator( From ee988c0da871b68ee9c1a657572572dd5681da82 Mon Sep 17 00:00:00 2001 From: Allan Shortlidge Date: Tue, 23 Sep 2025 18:41:37 -0700 Subject: [PATCH 2/2] Sema: Infer key path immutability during expression availability checking. Accessor availability diagnostics for key path expressions were first introduced by https://github.com/swiftlang/swift/pull/83931. Those changes were insufficient because sometimes key path expressions are generated in the AST with more mutability than is needed by the context and so setter availability could be diagnosed inappropriately. Key path expression binding was refined in https://github.com/swiftlang/swift/pull/84491 to make this less likely to occur. However, there are still some circumstances in which a mutable key path is generated in the AST and then immediately coerced into an immutable key path to satisfy the contextual type. This change infers the immutability of these key path expressions by looking through surrounding conversion expressions. --- lib/Sema/TypeCheckAvailability.cpp | 63 ++++++++++++------- .../Availability/availability_accessors.swift | 48 +++++++++++--- 2 files changed, 82 insertions(+), 29 deletions(-) diff --git a/lib/Sema/TypeCheckAvailability.cpp b/lib/Sema/TypeCheckAvailability.cpp index f46a2c25eb65c..c457b765b022f 100644 --- a/lib/Sema/TypeCheckAvailability.cpp +++ b/lib/Sema/TypeCheckAvailability.cpp @@ -29,6 +29,7 @@ #include "swift/AST/ClangModuleLoader.h" #include "swift/AST/DeclExportabilityVisitor.h" #include "swift/AST/DiagnosticsParse.h" +#include "swift/AST/ExistentialLayout.h" #include "swift/AST/GenericEnvironment.h" #include "swift/AST/Initializer.h" #include "swift/AST/NameLookup.h" @@ -2232,9 +2233,6 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker { /// The context does not involve a key path access. None, - /// The context is an expression that is coerced to a read-only key path. - ReadOnlyCoercion, - /// The context is a key path application (`x[keyPath: \.member]`). Application, }; @@ -2361,15 +2359,6 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker { FCE->getLoc(), Where.getDeclContext()); } - if (auto DTBE = dyn_cast(E)) { - if (auto ty = DTBE->getType()) { - if (ty->isKeyPath()) { - walkInKeyPathAccessContext(DTBE->getSubExpr(), - KeyPathAccessContext::ReadOnlyCoercion); - return Action::SkipChildren(E); - } - } - } if (auto KP = dyn_cast(E)) { maybeDiagKeyPath(KP); } @@ -2586,19 +2575,49 @@ class ExprAvailabilityWalker : public BaseDiagnosticWalker { StorageAccessKind getStorageAccessKindForKeyPath(KeyPathExpr *KP) const { switch (KeyPathAccess) { - case KeyPathAccessContext::None: - // Use the key path's type to determine the access kind. - if (!KP->isObjC()) - if (auto keyPathType = KP->getKeyPathType()) - if (keyPathType->isKeyPath()) + case KeyPathAccessContext::None: { + // Obj-C key paths are always considered mutable. + if (KP->isObjC()) + return StorageAccessKind::GetSet; + + // Check whether key path type indicates immutability, in which case only + // the getter will be used. + if (auto keyPathType = KP->getKeyPathType()) + if (keyPathType->isKnownImmutableKeyPathType()) + return StorageAccessKind::Get; + + // Check if there's a conversion expression containing this one directly + // above it in the stack. If so, and that conversion is to an immutable + // key path type, then we should infer that use of the key path only + // implies use of the getter. + ArrayRef exprs = ExprStack; + // Must be called while visiting the given key path expression. + ASSERT(exprs.back() == KP); + + for (auto *expr : llvm::reverse(exprs.drop_back())) { + // Only traverse through conversions on the stack. + if (!isa(expr) && + !isa(expr)) + break; + + if (auto ty = expr->getType()) { + if (ty->isKnownImmutableKeyPathType()) return StorageAccessKind::Get; - return StorageAccessKind::GetSet; + if (auto existential = dyn_cast(ty)) { + if (auto superclass = + existential->getExistentialLayout().getSuperclass()) { + if (superclass->isKnownImmutableKeyPathType()) + return StorageAccessKind::Get; + } + } + } + } - case KeyPathAccessContext::ReadOnlyCoercion: - // The type of this key path is being coerced to the type KeyPath<_, _> so - // ignore the actual key path type. - return StorageAccessKind::Get; + // We failed to prove that the key path is only used immutably, + // so diagnose both get and set access. + return StorageAccessKind::GetSet; + } case KeyPathAccessContext::Application: // The key path is being applied directly to a base so treat it as if it diff --git a/test/Availability/availability_accessors.swift b/test/Availability/availability_accessors.swift index 578b92de6dacf..37d29f619b544 100644 --- a/test/Availability/availability_accessors.swift +++ b/test/Availability/availability_accessors.swift @@ -56,7 +56,7 @@ struct BaseStruct { var unavailableGetter: T { @available(*, unavailable) - get { fatalError() } // expected-note 69 {{getter for 'unavailableGetter' has been explicitly marked unavailable here}} + get { fatalError() } // expected-note 73 {{getter for 'unavailableGetter' has been explicitly marked unavailable here}} set { } } @@ -68,7 +68,7 @@ struct BaseStruct { var unavailableGetterAndSetter: T { @available(*, unavailable) - get { fatalError() } // expected-note 68 {{getter for 'unavailableGetterAndSetter' has been explicitly marked unavailable here}} + get { fatalError() } // expected-note 71 {{getter for 'unavailableGetterAndSetter' has been explicitly marked unavailable here}} @available(*, unavailable) set { fatalError() } // expected-note 33 {{setter for 'unavailableGetterAndSetter' has been explicitly marked unavailable here}} } @@ -116,11 +116,6 @@ struct SubscriptHelper { return t } -func takesKeyPath(_ t: T, _ keyPath: KeyPath) -> () { } -func takesWritableKeyPath(_ t: inout T, _ keyPath: WritableKeyPath) -> () { - // expected-note@-1 2 {{in call to function 'takesWritableKeyPath'}} -} - func testDiscardedRValueLoads_Struct() { var x = BaseStruct() // expected-warning {{variable 'x' was never mutated; consider changing to 'let' constant}} @@ -548,6 +543,8 @@ func testDiscardedApplyOfFuncWithInOutParam_Class() { func testKeyPathArguments_Struct() { var x = BaseStruct() + func takesKeyPath(_ t: T, _ keyPath: KeyPath) -> () { } + takesKeyPath(x, \.available) takesKeyPath(x, \.unavailableGetter) // expected-error {{getter for 'unavailableGetter' is unavailable}} takesKeyPath(x, \.unavailableSetter) @@ -555,6 +552,35 @@ func testKeyPathArguments_Struct() { takesKeyPath(x, \.deprecatedGetter) // expected-warning {{getter for 'deprecatedGetter' is deprecated: reading not recommended}} takesKeyPath(x, \.deprecatedSetter) + func takesAnyKeyPath(_ keyPath: AnyKeyPath) -> () { } + + takesAnyKeyPath(\BaseStruct.available) + takesAnyKeyPath(\BaseStruct.unavailableGetter) // expected-error {{getter for 'unavailableGetter' is unavailable}} + takesAnyKeyPath(\BaseStruct.unavailableSetter) + takesAnyKeyPath(\BaseStruct.unavailableGetterAndSetter) // expected-error {{getter for 'unavailableGetterAndSetter' is unavailable}} + takesAnyKeyPath(\BaseStruct.deprecatedGetter) // expected-warning {{getter for 'deprecatedGetter' is deprecated: reading not recommended}} + takesAnyKeyPath(\BaseStruct.deprecatedSetter) + + func takesPartialKeyPath(_ t: T, _ keyPath: PartialKeyPath) -> () { } + takesPartialKeyPath(x, \.available) + takesPartialKeyPath(x, \.unavailableGetter) // expected-error {{getter for 'unavailableGetter' is unavailable}} + takesPartialKeyPath(x, \.unavailableSetter) + takesPartialKeyPath(x, \.unavailableGetterAndSetter) // expected-error {{getter for 'unavailableGetterAndSetter' is unavailable}} + takesPartialKeyPath(x, \.deprecatedGetter) // expected-warning {{getter for 'deprecatedGetter' is deprecated: reading not recommended}} + takesPartialKeyPath(x, \.deprecatedSetter) + + func takesSendableKeyPath(_ t: T, _ keyPath: KeyPath & Sendable) -> () { } + + takesSendableKeyPath(x, \.available) + takesSendableKeyPath(x, \.unavailableGetter) // expected-error {{getter for 'unavailableGetter' is unavailable}} + takesSendableKeyPath(x, \.unavailableSetter) + takesSendableKeyPath(x, \.unavailableGetterAndSetter) // expected-error {{getter for 'unavailableGetterAndSetter' is unavailable}} + takesSendableKeyPath(x, \.deprecatedGetter) // expected-warning {{getter for 'deprecatedGetter' is deprecated: reading not recommended}} + takesSendableKeyPath(x, \.deprecatedSetter) + + func takesWritableKeyPath(_ t: inout T, _ keyPath: WritableKeyPath) -> () { } + // expected-note@-1 2 {{in call to function 'takesWritableKeyPath'}} + takesWritableKeyPath(&x, \.available) takesWritableKeyPath(&x, \.unavailableGetter) // expected-error {{getter for 'unavailableGetter' is unavailable}} // FIXME: Ideally we would diagnose the unavailability of the setter instead @@ -566,6 +592,14 @@ func testKeyPathArguments_Struct() { // expected-error@-1 {{generic parameter 'U' could not be inferred}} takesWritableKeyPath(&x, \.deprecatedGetter) // expected-warning {{getter for 'deprecatedGetter' is deprecated: reading not recommended}} takesWritableKeyPath(&x, \.deprecatedSetter) // expected-warning {{setter for 'deprecatedSetter' is deprecated: writing not recommended}} + + func takesUnifiedTypes(_: T, _: T) { } + + takesUnifiedTypes(\BaseStruct.available, \BaseStruct.available) + takesUnifiedTypes(\BaseStruct.available, \BaseStruct.unavailableGetter) // expected-error {{getter for 'unavailableGetter' is unavailable}} + takesUnifiedTypes(\BaseStruct.available, \BaseStruct.unavailableSetter) + takesUnifiedTypes(\BaseStruct.available, \BaseStruct.deprecatedSetter) // expected-warning {{setter for 'deprecatedSetter' is deprecated: writing not recommended}} + takesUnifiedTypes(\BaseStruct.unavailableSetter, \BaseStruct.deprecatedSetter) } var global = BaseStruct()