Proposal to add a move(x) function for ending local variable lifetimes #1693

jckarter · 2022-06-23T21:20:09Z

No description provided.

gottesmm

Two small nits.

gottesmm · 2022-06-23T21:40:18Z

proposals/nnnn-move-function.md

+```
+
+Notice how the compiler gives us all of the information that we need to resolve
+this: it tells us where the move was and gives tells us the later uses that


gives tells looks like a thought stream being crossed.

gottesmm · 2022-06-23T21:40:38Z

proposals/nnnn-move-function.md

+}
+```
+
+we get separable nice diagnostics:


I am not sure if nice is needed here. But I am fine with it staying ; ).

hborla · 2022-07-21T03:02:04Z

Hi @gottesmm and @atrick, I'll be managing the review for this proposal! I just finished going through the pitch thread and reading the updated proposal. Here are my notes on reviewers' feedback that I think you should consider addressing in the proposal:

The term "binding" seems to have caused significant confusion in the pitch thread - consider defining it in the proposal.
The proposal is missing a "Motivation" section. The motivation is outlined in the introduction, but please put the motivation under a "Motivation" heading.
The alternative syntax suggestions are already covered in the proposal, but it may be helpful to more directly discuss the mentioned "potential impact to the language syntax" that is minimized by choosing to define move as a function.
- For example, any was recently added as a contextual keyword in expression context, and to avoid stomping on the top-level any function, you cannot parenthesize the constraint operand to a contextual any keyword in expressions.
Clarification on what types of values can be passed to move
- https://forums.swift.org/t/pitch-move-function-use-after-move-diagnostic/53983/33
- https://forums.swift.org/t/pitch-move-function-use-after-move-diagnostic/53983/10
Justification for not annotating move with @discardableResult.
- Michael addressed this in the pitch discussion, but I think it's worth adding to the proposal.
How move fits into the overarching ownership story, perhaps by linking the recent roadmap in a "Future Directions" section.

Let me know if you have any questions! I'll schedule this for review once you've had a chance to consider my notes.

hborla · 2022-07-21T03:10:10Z

proposals/nnnn-move-function.md

+Notice how in the above, we are able to use `x` both in the true block AND the
+continuation block since over all paths, x now has a valid value.
+
+The value based analysis uses Ownership SSA to determine if values are used


Is there a resource that can be linking out to here for folks who are not familiar with Ownership SSA?

I don't think we should be referring to Ownership SSA in a language-level document. We should focus on what the compiler does, not how it does it.

tbkka

On the whole, the examples are good, but there's far too much discussion of how your particular implementation of the proposed feature works.

Here's a way to think about it: Pretend that some other group had implemented a from-scratch implementation of a Swift compiler. That compiler might not use SIL nor OSSA nor any other particular internal technology, but everything in this document should still be relevant to that other implementation.

tbkka · 2022-07-21T17:15:59Z

proposals/nnnn-move-function.md

+useY(y) // do some stuff with local variable y
+useX(x) // error, x's lifetime was ended at [1]
+
+// Ends lifetime of y. Since _ is no-op, we perform an actual release here.


Whether there is a "release" depends on the type of x. Focus on the language implications:

Ends the lifetime of the variable `y`. Assigning to `_` avoids creation of a new lifetime.

I would also through here be careful to distinguish the lifetime of a "variable" from the lifetime of the "value" that may be temporarily stored in that value.

tbkka · 2022-07-21T17:27:07Z

proposals/nnnn-move-function.md

+
+In this document, we propose adding a new function called `move` to the swift
+standard library, which ends the lifetime of a specific local `let`,
+local `var`, or `consuming` parameter. In order to enforce this, the compiler will


Overall suggestion for making markup documents easier to review: Put a line break after every full stop.

I suggest you add a sentence here after "parameter."

This gives developers a way to control retain/release operations and avoid unnecessary copy-on-write behaviors in performance-critical code.

Then this paragraph will provide a good summary: A sentence saying what you propose, a sentence about why, and a sentence summarizing how the compiler will support this operation.

tbkka · 2022-07-21T17:30:23Z

proposals/nnnn-move-function.md

+useY(y) // error, y's lifetime was ended at [2]
+```
+
+This allows the user to influence the uniqueness of COW data structures


I would start the "Motivation" section here, and reword things some to focus
on the problems that you're trying to solve:

Performance-critical code can suffer from unexpected copy-on-write operations and unnecessary retain/release calls. Consider the following array/uniqueness example:

proposals/nnnn-move-function.md

tbkka · 2022-07-21T17:31:31Z

proposals/nnnn-move-function.md

+}
+```
+
+in the example above, without `move`, `y`'s formal lifetime extends to the end of


Capitalize "In"

tbkka · 2022-07-21T17:50:17Z

proposals/nnnn-move-function.md

+```
+
+
+In the future, we may add support for globals and stored properties, although


Again, you're talking about the implementation. You need to explain why you do not think the feature requires this support today.

Hmmm.... This paragraph might be better off moved to "alternatives considered".

tbkka · 2022-07-21T17:53:50Z

proposals/nnnn-move-function.md

+}
+```
+
+Builtin.move is a hook in the compiler to force emission of special SIL "move"


I think you should delete the rest of this section from this point.

At most, I would like to see a brief paragraph that says in essence that "Builtin.move() causes the compiler to perform additional data flow analysis to verify that the argument is not in fact used after this point." The details of how that is implemented in this particular compiler is not relevant; a different compiler may implement it in very different ways.

tbkka · 2022-07-21T17:54:40Z

proposals/nnnn-move-function.md

+an SSA based analysis that determines if any uses of an address are reachable
+from a move. All of these are already in tree and can be used today by invoking
+the stdlib non-API function `_move` on a local let or move. *NOTE* This function
+is always emit into client and transparent so there isn't an ABI impact so it is


This NOTE is useful to keep, since it explains the library implementation shown at the top of this section.

tbkka · 2022-07-21T17:55:37Z

proposals/nnnn-move-function.md

+
+None, this is additive.
+
+## Alternatives considered


This is a reasonable place to explain why you don't think global support is necessary and could be considered separately.

tbkka · 2022-07-21T17:56:32Z

proposals/nnnn-move-function.md

+term that has already been used in other Swift standard library APIs such as
+the `UnsafeMutablePointer.move*` family of methods that move a value out of
+memory referenced by a pointer. Declaring it as a function also minimizes the
+potential impact to the language syntax. We are however open to discussing


Strike the sentence beginning "We are however open ..."

jckarter · 2022-07-21T19:24:42Z

I'll go ahead and start working on the requested revisions. Thanks @hborla and @tbkka!

jckarter · 2022-07-21T23:27:06Z

@hborla @tbkka How's this look now?

tbkka · 2022-07-21T23:41:19Z

proposals/nnnn-move-function.md

+In this case, we get the following output from the compiler as expected:
+
+```swift
+test.swift:10:7: error: 'y' used after being moved


I would drop this entire example and just put the error message directly into the example above:

useYAgain(y) // Error: 'y' used after being moved

tbkka · 2022-07-21T23:45:00Z

Definitely improved. Still some work to do, but it's improving nicely.

hborla

Thank you!

tbkka · 2022-07-22T16:08:37Z

proposals/0366-move-function.md

+
+If the binding is a `var`, the analysis additionally allows for code to
+conditionally reinitialize the var and thus be able to use it in positions
+that are dominated by the reinitialization.  continuation path. Consider the


Extra words?

Feedback has been addressed

gottesmm reviewed Jun 23, 2022

View reviewed changes

Proposal to add a move(x) function for ending local variable lifetimes

b5511be

jckarter force-pushed the move-function branch from 753a947 to b5511be Compare June 23, 2022 23:23

hborla self-assigned this Jul 19, 2022

hborla reviewed Jul 21, 2022

View reviewed changes

tbkka previously requested changes Jul 21, 2022

View reviewed changes

Editorial feedback

5a4187d

tbkka reviewed Jul 21, 2022

View reviewed changes

Editorial feedback ][

2e33507

hborla approved these changes Jul 22, 2022

View reviewed changes

Assign SE-0366 to move and schedule for review.

5147cf9

tbkka reviewed Jul 22, 2022

View reviewed changes

Typo fix and more editorial changes

11ae518

hborla merged commit e655bc1 into swiftlang:main Jul 22, 2022

		```


		In the future, we may add support for globals and stored properties, although

Uh oh!

Proposal to add a move(x) function for ending local variable lifetimes #1693

Proposal to add a move(x) function for ending local variable lifetimes #1693

Uh oh!

Conversation

jckarter commented Jun 23, 2022

Uh oh!

gottesmm left a comment

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!

hborla commented Jul 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbkka left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

jckarter commented Jul 21, 2022

Uh oh!

jckarter commented Jul 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbkka commented Jul 21, 2022

Uh oh!

hborla left a comment

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!

4 participants