[borrowing/consuming] Make borrowing and consuming parameters no implicit copy. #66381

gottesmm · 2023-06-06T20:02:38Z

rdar://108383660

… `borrowing`" This reverts commit 8899d3b.

…modifiers" This reverts commit 87f190b.

… functions.

…() instead of getASTType() to prevent us from looking through no implicit copy types while serializing. This change makes sense since, in general, when serializing we do not want to look through no implicit copy types since we are trying to 1-1 preserve the SIL.

…h the AST serialization machinery. This looks like a thinko from the early part of the implementation. Without this, the machinery doesn't recognize the layout and just asserts.

…uming/Borrowing. I also added a static_assert to make sure that we always catch this in the future.

…e_addr. The reason why I am using a different instruction for addresses and objects here is that the object checker doesnt have to deal with things like initialization.

…lywrapper_addr. Just the $*T -> $*@moveOnly T variant for addresses. Unlike the object version this acts like a cast rather than something that provides semantics from the frontend to the optimizer.

…r_to_copyable_addr.

I am going to use this to unwrap ${ @moveOnly T } so that I can pass it to partial_apply that expect a ${ T }

gottesmm · 2023-06-06T20:03:29Z

@swift-ci smoke test

jckarter

Looks good.

gottesmm · 2023-06-06T22:11:08Z

@swift-ci smoke test

…a noimplicitcopy type. rdar://108383660

gottesmm · 2023-06-06T22:12:49Z

@swift-ci smoke test

kavon · 2023-06-06T20:43:12Z

lib/SILOptimizer/Mandatory/MoveOnlyObjectCheckerUtils.h

These are identical except for the isa<...> tests on the instructions. You could define one bool isPartialApplyUser(SILInstruction* user, bool isUser) and replace those tests with isa<PartialApplyInst>(...) == isUser

kavon · 2023-06-06T21:55:24Z

test/SILOptimizer/noimplicitcopy_borrowing_parameters.swift

is this one not suppose to raise any errors since it's not an escaping closure?

Yes. I am going to fix that in a subsequent commit. This is providing the initial support. There will not be a source break when I fix it since we are removing an error.

kavon · 2023-06-06T22:27:18Z

lib/SIL/IR/SILType.cpp

+    // We want to specifically use getASTType() here instead of getRawASTType()
+    // to ensure that we can correctly get our substituted field type. If we
+    // need to rewrap the type layer, we do it below.
    substFieldTy =
        getASTType()->getTypeOfMember(&TC.M, field)->getCanonicalType();


As an aside: I think we ought to do something about this getASTType / getRawASTType thing since it's a bit confusing from a naming perspective alone; the only difference is with removing any move-only wrapper. That wrapper type needs to be more widely supported, since it's now being serialized and carried through the pipeline.

From working with it, I think that the only way to do this is to have a lot of tests that exercise the behavior. If you have a better name, I am more than happy to rename it.

lib/SILGen/SILGenProlog.cpp

lib/Sema/TypeCheckAttr.cpp

kavon · 2023-06-06T22:59:51Z

test/Interpreter/noimplicitcopy_parameters.swift

+    }
+    // CHECK: Doing something MyOtherName
+    f2()
+}


we should test copy x being passed to a consuming parameter here too.

kavon · 2023-06-06T23:07:50Z

test/SILOptimizer/noimplicitcopy_borrowing_parameters.swift

+func testAddressOnlyBorrowingEnum<T>(_ x: borrowing AddressOnlyEnum<T>) {
+    // expected-error @-1 {{'x' is borrowed and cannot be consumed}}
+    switch x { // expected-note {{consumed here}}
+    case let .x(y):
+        _ = y
+        break
+    case let .y(z):
+        _ = z
+        break
+    }
+}


I think we really need to tune these diagnostics to tell users that they can and are required to make an explicit copy.

kavon · 2023-06-06T23:15:32Z

test/SILOptimizer/noimplicitcopy_borrowing_parameters.swift

+func testLoadableBorrowingEnum(_ x: borrowing LoadableEnum) {
+    // expected-error @-1 {{'x' is borrowed and cannot be consumed}}
+    switch x { // expected-note {{consumed here}}
+    case let .x(y):
+        _ = y
+        break
+    case .y:
+        break
+    }
+}
+
+func testLoadableBorrowingCopyOperator(_ x: borrowing NonTrivialStruct) {
+    _ = copy x
+    let _ = copy x
+    consumeVal(copy x)
+}


I think we should mix copy x into more situations to make sure its got coverage, since that's what makes it different from the noncopyable types. For example, we could take this switch test and write a positive / corrected version with switch copy x which I don't think we've tried to ensure it's fine with it.

kavon · 2023-06-06T23:20:04Z

test/SILOptimizer/noimplicitcopy_consuming_parameters.swift

+func testTrivialConsumingConsumeAndUseReinit(_ x: consuming TrivialStruct) {
+    let _ = x
+    x = TrivialStruct()
+    borrowValBorrowing(x)
+}


something I haven't seen yet is that we're still preventing implicit copies on the parameter binding even after you've reassigned it, whether it's a consuming or borrowing parameter. Also makes me wonder what happens when you do x = x... does it require you to write x = copy x ?

Also, a test that entures we still enforce no-implicit-copy after an explicit copy of the param.

gottesmm added 11 commits June 6, 2023 12:37

Revert "temporarily prevent Copyable types from using consuming and…

95cdf2a

… `borrowing`" This reverts commit 8899d3b.

Revert "add and update tests for Copyable types not having ownership …

07677c2

…modifiers" This reverts commit 87f190b.

Restore sema code that bans consuming from being applied to no escape…

cbb9e15

… functions.

[noimplicitcopy] Be sure to register SILMoveOnlyWrappedTypeLayout wit…

508866a

…h the AST serialization machinery. This looks like a thinko from the early part of the implementation. Without this, the machinery doesn't recognize the layout and just asserts.

[move-only] Expand FuncDecl's SelfAccessKind field so it can fit Cons…

76374f8

…uming/Borrowing. I also added a static_assert to make sure that we always catch this in the future.

[borrowing/consuming] Add new instruction: moveonlywrapper_to_copyabl…

43f42e2

…e_addr. The reason why I am using a different instruction for addresses and objects here is that the object checker doesnt have to deal with things like initialization.

[borrowing/consuming] Add a new instruction called copyable_to_moveon…

611e676

…lywrapper_addr. Just the $*T -> $*@moveOnly T variant for addresses. Unlike the object version this acts like a cast rather than something that provides semantics from the frontend to the optimizer.

Add some docs for copyable_to_moveonlywrapper_addr and moveonlywrappe…

68fd68b

…r_to_copyable_addr.

[sil-mode.el] Add syntax highlighting for noimplicit copy instructions.

65a9ba4

[move-only] Add a new instruction: moveonlywrapper_to_copyable_box.

70ab38d

I am going to use this to unwrap ${ @moveOnly T } so that I can pass it to partial_apply that expect a ${ T }

gottesmm requested review from AnthonyLatsis, CodaFi, DougGregor, ahoppen, bnbarham, hborla, ktoso, rintaro, slavapestov and xedin as code owners June 6, 2023 20:02

gottesmm requested a review from jckarter June 6, 2023 20:02

jckarter approved these changes Jun 6, 2023

View reviewed changes

gottesmm force-pushed the noimplicitcopy-borrow-consuming branch from 6b38d73 to 23b439a Compare June 6, 2023 22:11

[borrowing] Add support for borrowing/consuming copyable types to be …

59c8cff

…a noimplicitcopy type. rdar://108383660

gottesmm force-pushed the noimplicitcopy-borrow-consuming branch from 23b439a to 59c8cff Compare June 6, 2023 22:12

gottesmm mentioned this pull request Jun 6, 2023

[5.9][borrowing][consuming] Make borrowing and consuming no implicit copy parameters #66388

Merged

kavon reviewed Jun 6, 2023

View reviewed changes

gottesmm merged commit 29672c5 into swiftlang:main Jun 7, 2023

gottesmm deleted the noimplicitcopy-borrow-consuming branch June 7, 2023 01:41

[borrowing/consuming] Make borrowing and consuming parameters no implicit copy. #66381

[borrowing/consuming] Make borrowing and consuming parameters no implicit copy. #66381

Uh oh!

Conversation

gottesmm commented Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gottesmm commented Jun 6, 2023

Uh oh!

jckarter left a comment

Choose a reason for hiding this comment

Uh oh!

gottesmm commented Jun 6, 2023

Uh oh!

gottesmm commented Jun 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kavon Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gottesmm commented Jun 6, 2023 •

edited

Loading

kavon Jun 6, 2023 •

edited

Loading