From 6f7d420f9d23313398b80145bad2f45316d251cf Mon Sep 17 00:00:00 2001 From: Meghana Gupta Date: Tue, 11 Nov 2025 06:13:55 -0800 Subject: [PATCH 1/3] Fix unchecked_conversion insertion for borrow accessors --- lib/SILGen/SILGenApply.cpp | 28 +++++++------ test/SILGen/borrow_accessor.swift | 67 +++++++++++++++++++++++++++++-- 2 files changed, 78 insertions(+), 17 deletions(-) diff --git a/lib/SILGen/SILGenApply.cpp b/lib/SILGen/SILGenApply.cpp index ff81638e3757d..43fd0d35c70b0 100644 --- a/lib/SILGen/SILGenApply.cpp +++ b/lib/SILGen/SILGenApply.cpp @@ -5457,9 +5457,20 @@ ManagedValue CallEmission::applyBorrowMutateAccessor() { // begin_borrow instructions added for move-only self argument. if (selfArgMV.getValue()->getType().isMoveOnly() && selfArgMV.getValue()->getType().isObject()) { - uncurriedArgs[uncurriedArgs.size() - 1] = - ManagedValue::forBorrowedObjectRValue( - lookThroughMoveOnlyCheckerPattern(selfArgMV.getValue())); + uncurriedArgs.back() = ManagedValue::forBorrowedObjectRValue( + lookThroughMoveOnlyCheckerPattern(selfArgMV.getValue())); + } + + if (fnValue.getFunction()->getConventions().hasGuaranteedResult()) { + if (isa(selfArgMV)) { + // unchecked_ownership is used to silence the ownership verifier for + // returning a value produced within a load_borrow scope. SILGenCleanup + // eliminates it and introduces return_borrow appropriately. + uncurriedArgs.back() = + ManagedValue::forForwardedRValue( + SGF, SGF.B.createUncheckedOwnership(uncurriedLoc.value(), + selfArgMV.getValue())); + } } auto value = SGF.applyBorrowMutateAccessor( @@ -5475,22 +5486,13 @@ ManagedValue SILGenFunction::applyBorrowMutateAccessor( ApplyOptions options) { // Emit the call. SmallVector rawResults; + emitRawApply(*this, loc, fn, subs, args, substFnType, options, /*indirect results*/ {}, /*indirect errors*/ {}, rawResults, ExecutorBreadcrumb()); assert(rawResults.size() == 1); auto rawResult = rawResults[0]; - if (fn.getFunction()->getConventions().hasGuaranteedResult()) { - auto selfArg = args.back().getValue(); - if (isa(selfArg)) { - // unchecked_ownership is used to silence the ownership verifier for - // returning a value produced within a load_borrow scope. SILGenCleanup - // eliminates it and introduces return_borrow appropriately. - rawResult = B.createUncheckedOwnership(loc, rawResult); - } - } - if (rawResult->getType().isMoveOnly()) { if (rawResult->getType().isAddress()) { SILFunctionConventions substFnConv(substFnType, SGM.M); diff --git a/test/SILGen/borrow_accessor.swift b/test/SILGen/borrow_accessor.swift index f615f33e49b60..d6d74c9ab07f7 100644 --- a/test/SILGen/borrow_accessor.swift +++ b/test/SILGen/borrow_accessor.swift @@ -1,4 +1,5 @@ // RUN:%target-swift-frontend -emit-silgen %s -enable-experimental-feature BorrowAndMutateAccessors | %FileCheck %s +// RUN:%target-swift-frontend -c %s -enable-experimental-feature BorrowAndMutateAccessors -Xllvm -sil-print-after=SILGenCleanup 2>&1 | %FileCheck %s --check-prefixes=CHECK-SIL // REQUIRES: swift_feature_BorrowAndMutateAccessors @@ -496,6 +497,14 @@ public struct GenNCWrapper : ~Copyable { // CHECK: return [[REG4]] // CHECK: } +// CHECK-SIL: sil hidden [ossa] @$s15borrow_accessor10GenWrapperV1sAA1SVvb : $@convention(method) (@in_guaranteed GenWrapper) -> @guaranteed S { +// CHECK-SIL: bb0([[REG0:%.*]] : $*GenWrapper): +// CHECK-SIL: debug_value [[REG0]], let, name "self", argno 1, expr op_deref +// CHECK-SIL: [[REG2:%.*]] = struct_element_addr [[REG0]], #GenWrapper._s +// CHECK-SIL: [[REG3:%.*]] = load_borrow [[REG2]] +// CHECK-SIL: return_borrow [[REG3]] from_scopes ([[REG3]]) +// CHECK-SIL: } + // CHECK: sil hidden [ossa] @$s15borrow_accessor10GenWrapperV1sAA1SVvz : $@convention(method) (@inout GenWrapper) -> @inout S { // CHECK:bb0([[REG0:%.*]] : $*GenWrapper): // CHECK: debug_value [[REG0]], var, name "self", argno 1, expr op_deref @@ -513,6 +522,14 @@ public struct GenNCWrapper : ~Copyable { // CHECK: return [[REG4]] // CHECK: } +// CHECK-SIL: sil hidden [ossa] @$s15borrow_accessor10GenWrapperV1kAA5KlassCvb : $@convention(method) (@in_guaranteed GenWrapper) -> @guaranteed Klass { +// CHECK-SIL: bb0([[REG0:%.*]] : $*GenWrapper): +// CHECK-SIL: debug_value [[REG0]], let, name "self", argno 1, expr op_deref +// CHECK-SIL: [[REG2:%.*]] = struct_element_addr [[REG0]], #GenWrapper._k +// CHECK-SIL: [[REG3:%.*]] = load_borrow [[REG2]] +// CHECK-SIL: return_borrow [[REG3]] from_scopes ([[REG3]]) +// CHECK-SIL: } + // CHECK: sil hidden [ossa] @$s15borrow_accessor10GenWrapperV1kAA5KlassCvz : $@convention(method) (@inout GenWrapper) -> @inout Klass { // CHECK:bb0([[REG0:%.*]] : $*GenWrapper): // CHECK: debug_value [[REG0]], var, name "self", argno 1, expr op_deref @@ -552,12 +569,22 @@ public struct GenNCWrapper : ~Copyable { // CHECK: [[REG2:%.*]] = struct_element_addr [[REG0]], #GenWrapper._s // CHECK: [[REG3:%.*]] = load_borrow [[REG2]] // CHECK: [[REG4:%.*]] = function_ref @$s15borrow_accessor1SV1kAA5KlassCvb : $@convention(method) (@guaranteed S) -> @guaranteed Klass -// CHECK: [[REG5:%.*]] = apply [[REG4]]([[REG3]]) : $@convention(method) (@guaranteed S) -> @guaranteed Klass -// CHECK: [[REG6:%.*]] = unchecked_ownership [[REG5]] +// CHECK: [[REG5:%.*]] = unchecked_ownership [[REG3]] +// CHECK: [[REG6:%.*]] = apply [[REG4]]([[REG5]]) : $@convention(method) (@guaranteed S) -> @guaranteed Klass // CHECK: end_borrow [[REG3]] // CHECK: return [[REG6]] // CHECK: } +// CHECK-SIL: sil hidden [ossa] @$s15borrow_accessor10GenWrapperV9nested_k1AA5KlassCvb : $@convention(method) (@in_guaranteed GenWrapper) -> @guaranteed Klass { +// CHECK-SIL: bb0([[REG0:%.*]] : $*GenWrapper): +// CHECK-SIL: debug_value [[REG0]], let, name "self", argno 1, expr op_deref +// CHECK-SIL: [[REG2:%.*]] = struct_element_addr [[REG0]], #GenWrapper._s +// CHECK-SIL: [[REG3:%.*]] = load_borrow [[REG2]] +// CHECK-SIL: [[REG4:%.*]] = function_ref @$s15borrow_accessor1SV1kAA5KlassCvb : $@convention(method) (@guaranteed S) -> @guaranteed Klass +// CHECK-SIL: [[REG5:%.*]] = apply [[REG4]]([[REG3]]) : $@convention(method) (@guaranteed S) -> @guaranteed Klass +// CHECK-SIL: return_borrow [[REG5]] from_scopes ([[REG3]]) +// CHECK-SIL: } + // CHECK: sil hidden [ossa] @$s15borrow_accessor10GenWrapperV9nested_k1AA5KlassCvz : $@convention(method) (@inout GenWrapper) -> @inout Klass { // CHECK:bb0([[REG0:%.*]] : $*GenWrapper): // CHECK: debug_value [[REG0]], var, name "self", argno 1, expr op_deref @@ -654,6 +681,15 @@ public struct GenNCWrapper : ~Copyable { // CHECK: return [[REG5]] // CHECK: } +// CHECK-SIL: sil hidden [ossa] @$s15borrow_accessor12GenNCWrapperVAARi_zrlE2ncAA2NCVvb : $@convention(method) (@in_guaranteed GenNCWrapper) -> @guaranteed NC { +// CHECK-SIL: bb0([[REG0:%.*]] : $*GenNCWrapper): +// CHECK-SIL: debug_value [[REG0]], let, name "self", argno 1, expr op_deref +// CHECK-SIL: [[REG2:%.*]] = mark_unresolved_non_copyable_value [no_consume_or_assign] [[REG0]] +// CHECK-SIL: [[REG3:%.*]] = struct_element_addr [[REG2]], #GenNCWrapper._nc +// CHECK-SIL: [[REG4:%.*]] = load_borrow [[REG3]] +// CHECK-SIL: return_borrow [[REG4]] from_scopes ([[REG4]]) +// CHECK-SIL: } + // CHECK: sil hidden [ossa] @$s15borrow_accessor12GenNCWrapperVAARi_zrlE2ncAA2NCVvz : $@convention(method) (@inout GenNCWrapper) -> @inout NC { // CHECK:bb0([[REG0:%.*]] : $*GenNCWrapper): // CHECK: debug_value [[REG0]], var, name "self", argno 1, expr op_deref @@ -673,6 +709,15 @@ public struct GenNCWrapper : ~Copyable { // CHECK: return [[REG5]] // CHECK: } +// CHECK-SIL: sil hidden [ossa] @$s15borrow_accessor12GenNCWrapperVAARi_zrlE3ncwAA0D0Vvb : $@convention(method) (@in_guaranteed GenNCWrapper) -> @guaranteed NCWrapper { +// CHECK-SIL: bb0([[REG0:%.*]] : $*GenNCWrapper): +// CHECK-SIL: debug_value [[REG0]], let, name "self", argno 1, expr op_deref +// CHECK-SIL: [[REG2:%.*]] = mark_unresolved_non_copyable_value [no_consume_or_assign] [[REG0]] +// CHECK-SIL: [[REG3:%.*]] = struct_element_addr [[REG2]], #GenNCWrapper._ncw +// CHECK-SIL: [[REG4:%.*]] = load_borrow [[REG3]] +// CHECK-SIL: return_borrow [[REG4]] from_scopes ([[REG4]]) +// CHECK-SIL: } + // CHECK: sil hidden [ossa] @$s15borrow_accessor12GenNCWrapperVAARi_zrlE3ncwAA0D0Vvz : $@convention(method) (@inout GenNCWrapper) -> @inout NCWrapper { // CHECK:bb0([[REG0:%.*]] : $*GenNCWrapper): // CHECK: debug_value [[REG0]], var, name "self", argno 1, expr op_deref @@ -730,8 +775,8 @@ public struct GenNCWrapper : ~Copyable { // CHECK: [[REG3:%.*]] = struct_element_addr [[REG2]], #GenNCWrapper._ncw // CHECK: [[REG4:%.*]] = load_borrow [[REG3]] // CHECK: [[REG5:%.*]] = function_ref @$s15borrow_accessor9NCWrapperV2ncAA2NCVvb : $@convention(method) (@guaranteed NCWrapper) -> @guaranteed NC -// CHECK: [[REG6:%.*]] = apply [[REG5]]([[REG4]]) : $@convention(method) (@guaranteed NCWrapper) -> @guaranteed NC -// CHECK: [[REG7:%.*]] = unchecked_ownership [[REG6]] +// CHECK: [[REG6:%.*]] = unchecked_ownership [[REG4]] +// CHECK: [[REG7:%.*]] = apply [[REG5]]([[REG6]]) : $@convention(method) (@guaranteed NCWrapper) -> @guaranteed NC // CHECK: [[REG9:%.*]] = copy_value [[REG7]] // CHECK: [[REG10:%.*]] = mark_unresolved_non_copyable_value [no_consume_or_assign] [[REG9]] // CHECK: [[REG11:%.*]] = begin_borrow [[REG10]] @@ -741,6 +786,20 @@ public struct GenNCWrapper : ~Copyable { // CHECK: return [[REG7]] // CHECK: } +// CHECK-SIL: sil hidden [ossa] @$s15borrow_accessor12GenNCWrapperVAARi_zrlE10nested_nc1AA2NCVvb : $@convention(method) (@in_guaranteed GenNCWrapper) -> @guaranteed NC { +// CHECK-SIL: bb0([[REG0:%.*]] : $*GenNCWrapper): +// CHECK-SIL: debug_value [[REG0]], let, name "self", argno 1, expr op_deref +// CHECK-SIL: [[REG2:%.*]] = mark_unresolved_non_copyable_value [no_consume_or_assign] [[REG0]] +// CHECK-SIL: [[REG3:%.*]] = struct_element_addr [[REG2]], #GenNCWrapper._ncw +// CHECK-SIL: [[REG4:%.*]] = load_borrow [[REG3]] +// CHECK-SIL: [[REG5:%.*]] = function_ref @$s15borrow_accessor9NCWrapperV2ncAA2NCVvb : $@convention(method) (@guaranteed NCWrapper) -> @guaranteed NC +// CHECK-SIL: [[REG6:%.*]] = apply [[REG5]]([[REG4]]) : $@convention(method) (@guaranteed NCWrapper) -> @guaranteed NC +// CHECK-SIL: [[REG7:%.*]] = copy_value [[REG6]] +// CHECK-SIL: [[REG8:%.*]] = mark_unresolved_non_copyable_value [no_consume_or_assign] [[REG7]] +// CHECK-SIL: destroy_value [[REG8]] +// CHECK-SIL: return_borrow [[REG6]] from_scopes ([[REG4]]) +// CHECK-SIL: } + // CHECK: sil hidden [ossa] @$s15borrow_accessor12GenNCWrapperVAARi_zrlE10nested_nc1AA2NCVvz : $@convention(method) (@inout GenNCWrapper) -> @inout NC { // CHECK:bb0([[REG0:%.*]] : $*GenNCWrapper): // CHECK: debug_value [[REG0]], var, name "self", argno 1, expr op_deref From 20b23d631c338e49e4bb487a4c5ce14b3d0d238b Mon Sep 17 00:00:00 2001 From: Meghana Gupta Date: Wed, 12 Nov 2025 11:04:30 -0800 Subject: [PATCH 2/3] Diagnose borrow/mutate accessors in enums We don't have support for borrowing switch on Copyable types. It is supported for ~Copyable types, but the return expression emission for borrow accessors is not yet implemented. Diagnose instead of crashing the compiler. --- include/swift/AST/DiagnosticsCommon.def | 4 ++-- lib/Parse/ParseDecl.cpp | 5 ++-- lib/Sema/TypeCheckStorage.cpp | 16 +++++++++---- test/Parse/borrow_and_mutate_accessors.swift | 25 ++++---------------- test/Sema/borrow_and_mutate_accessors.swift | 20 ++++++++++++++-- 5 files changed, 37 insertions(+), 33 deletions(-) diff --git a/include/swift/AST/DiagnosticsCommon.def b/include/swift/AST/DiagnosticsCommon.def index 2bb04b08ed2a7..30907a1c262d1 100644 --- a/include/swift/AST/DiagnosticsCommon.def +++ b/include/swift/AST/DiagnosticsCommon.def @@ -280,8 +280,8 @@ ERROR(lookup_outputs_dont_match,none, // MARK: Accessor diagnostics //------------------------------------------------------------------------------ -ERROR(accessor_not_supported_in_decl,none, - "%0 is supported only on a struct or enum", (StringRef)) +ERROR(borrow_mutate_accessor_not_supported_in_decl, none, + "%0 is supported only on a struct", (StringRef)) //------------------------------------------------------------------------------ // MARK: bridged diagnostics diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp index f80bee63975e0..3a0db65b5193b 100644 --- a/lib/Parse/ParseDecl.cpp +++ b/lib/Parse/ParseDecl.cpp @@ -8131,9 +8131,8 @@ bool Parser::parseAccessorAfterIntroducer( } if (Kind == AccessorKind::Borrow || Kind == AccessorKind::Mutate) { - if (!Flags.contains(PD_InStruct) && !Flags.contains(PD_InEnum) && - !Flags.contains(PD_InExtension)) { - diagnose(Tok, diag::accessor_not_supported_in_decl, + if (!Flags.contains(PD_InStruct) && !Flags.contains(PD_InExtension)) { + diagnose(Tok, diag::borrow_mutate_accessor_not_supported_in_decl, getAccessorNameForDiagnostic(Kind, /*article*/ true, /*underscored*/ false)); } diff --git a/lib/Sema/TypeCheckStorage.cpp b/lib/Sema/TypeCheckStorage.cpp index fb880a9d15971..83013c8207559 100644 --- a/lib/Sema/TypeCheckStorage.cpp +++ b/lib/Sema/TypeCheckStorage.cpp @@ -4324,16 +4324,22 @@ StorageImplInfoRequest::evaluate(Evaluator &evaluator, if (borrow || mutate) { if (auto *extDecl = dyn_cast(DC)) { auto extNominal = extDecl->getExtendedNominal(); - if (!isa(extNominal) && !isa(extNominal)) { + if (!isa(extNominal)) { if (borrow) { storage->getASTContext().Diags.diagnose( - borrow->getLoc(), diag::accessor_not_supported_in_decl, - "a borrow accessor"); + borrow->getLoc(), + diag::borrow_mutate_accessor_not_supported_in_decl, + getAccessorNameForDiagnostic(borrow->getAccessorKind(), + /*article*/ true, + /*underscored*/ false)); } if (mutate) { storage->getASTContext().Diags.diagnose( - mutate->getLoc(), diag::accessor_not_supported_in_decl, - "a mutate accessor"); + mutate->getLoc(), + diag::borrow_mutate_accessor_not_supported_in_decl, + getAccessorNameForDiagnostic(mutate->getAccessorKind(), + /*article*/ true, + /*underscored*/ false)); } } } diff --git a/test/Parse/borrow_and_mutate_accessors.swift b/test/Parse/borrow_and_mutate_accessors.swift index a723fb7f979bc..96c3361340d6f 100644 --- a/test/Parse/borrow_and_mutate_accessors.swift +++ b/test/Parse/borrow_and_mutate_accessors.swift @@ -80,31 +80,14 @@ struct Wrapper { var i: Int var i_accessor: Int { - borrow { // expected-error{{a 'borrow' accessor is supported only on a struct or enum}} + borrow { // expected-error{{a 'borrow' accessor is supported only on a struct}} fatalError() } - mutate { // expected-error{{a 'mutate' accessor is supported only on a struct or enum}} + mutate { // expected-error{{a 'mutate' accessor is supported only on a struct}} return &i // expected-error{{'&' may only be used to pass an argument to inout parameter}} } } -var _count: Int = 0 - -enum Color { - case red - case green - case blue - - var count: Int { - borrow { - return _count - } - mutate { - return &_count - } - } -} - class KlassWrapper { var _k: Klass @@ -113,10 +96,10 @@ class KlassWrapper { } var k: Klass { - borrow {// expected-error{{a 'borrow' accessor is supported only on a struct or enum}} + borrow {// expected-error{{a 'borrow' accessor is supported only on a struct}} return _k } - mutate {// expected-error{{a 'mutate' accessor is supported only on a struct or enum}} + mutate {// expected-error{{a 'mutate' accessor is supported only on a struct}} return &_k } } diff --git a/test/Sema/borrow_and_mutate_accessors.swift b/test/Sema/borrow_and_mutate_accessors.swift index 9cf9bb40f88a3..511f34aa8b0eb 100644 --- a/test/Sema/borrow_and_mutate_accessors.swift +++ b/test/Sema/borrow_and_mutate_accessors.swift @@ -41,10 +41,10 @@ struct Struct { extension Klass { var i: Int { - borrow { // expected-error{{a borrow accessor is supported only on a struct or enum}} + borrow { // expected-error{{a 'borrow' accessor is supported only on a struct}} return 0 } - mutate { // expected-error{{a mutate accessor is supported only on a struct or enum}} + mutate { // expected-error{{a 'mutate' accessor is supported only on a struct}} return &_i } } @@ -67,3 +67,19 @@ protocol P { var phone: String { mutate } // expected-error{{property in protocol must have explicit { get } or { get set } specifier}} // expected-error{{expected get, read, or set in a protocol property}} } +enum OrderStatus: ~Copyable { + case processing(trackingNumber: String) + case cancelled(reason: String) + + var description: String { + borrow { // expected-error{{a 'borrow' accessor is supported only on a struct}} + switch self { + case .processing(let trackingNumber): + return trackingNumber + case .cancelled(let reason): + return reason + } + } + } +} + From 10dd321d38234773a5f1541d972095613f8abdfa Mon Sep 17 00:00:00 2001 From: Meghana Gupta Date: Thu, 11 Dec 2025 21:39:39 -0800 Subject: [PATCH 3/3] Bailout on address apply result while computng argument escaping effects --- .../Sources/Optimizer/Utilities/EscapeUtils.swift | 2 +- test/SILOptimizer/escape_effects.sil | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/SwiftCompilerSources/Sources/Optimizer/Utilities/EscapeUtils.swift b/SwiftCompilerSources/Sources/Optimizer/Utilities/EscapeUtils.swift index 536f93ff3808f..59b56eddaa70e 100644 --- a/SwiftCompilerSources/Sources/Optimizer/Utilities/EscapeUtils.swift +++ b/SwiftCompilerSources/Sources/Optimizer/Utilities/EscapeUtils.swift @@ -725,7 +725,7 @@ fileprivate struct EscapeWalker : ValueDefUseWalker, } case .escapingToReturn(let toPath, let exclusive): if effect.matches(calleeArgIdx, argPath.projectionPath) { - guard let fas = apply as? FullApplySite, let result = fas.singleDirectResult else { + guard let fas = apply as? FullApplySite, let result = fas.singleDirectResult, result.type.isObject else { return isEscaping } diff --git a/test/SILOptimizer/escape_effects.sil b/test/SILOptimizer/escape_effects.sil index 485b0e8ba7b69..41a2ef9e66a35 100644 --- a/test/SILOptimizer/escape_effects.sil +++ b/test/SILOptimizer/escape_effects.sil @@ -414,3 +414,12 @@ bb0(%0 : $*Wrapper): %2 = struct_element_addr %0, #Wrapper._k return %2 } + +sil [ossa] @call_mutate_accessor : $@convention(thin) (@inout Wrapper, @owned Klass) -> () { +bb0(%0 : $*Wrapper, %1 : @owned $Klass): + %func = function_ref @mutate_accessor : $@convention(thin) (@inout Wrapper) -> @inout Klass + %addr = apply %func(%0) : $@convention(thin) (@inout Wrapper) -> @inout Klass + store %1 to [assign] %addr + %r = tuple () + return %r +}