Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SE-0377: Document the borrowing and consuming parameter modifiers #173

Merged
merged 13 commits into from
Nov 9, 2023

Conversation

tbkka
Copy link
Contributor

@tbkka tbkka commented Aug 10, 2023

This documents the borrowing and consuming parameter modifiers introduced by SE-0377.

These are comparatively niche, so I'm only documenting them in the "Reference Manual" section for now.

I've structured this by:

  • Creating a new "Parameter Modifiers" section with a brief description of the modifiers concept,
  • Moving the old "In-Out Parameters" description under it, and
  • Adding a new subsection on Borrowing and Consuming Parameters.

I've also adjusted the "In-Out Parameters" introduction so it starts with a code example showing the inout syntax and usage.

Fixes: rdar://116031853

This documents the `borrowing` and `consuming` parameter modifiers
introduced by SE-0377.

These are comparatively niche, so I'm only documenting them in
the "Reference Manual" section for now.

I've structured this by:
* Creating a new "Parameter Modifiers" section with a brief description of the modifiers concept,
* Moving the old "In-Out Parameters" description under it, and
* Adding a new subsection on Borrowing and Consuming Parameters.

I've also adjusted the "In-Out Parameters" introduction so
it starts with a code example showing the `inout` syntax and usage.

SE-0377: https://github.com/apple/swift-evolution/blob/main/proposals/0377-parameter-ownership-modifiers.md
Copy link
Member

@amartini51 amartini51 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Tim!

Structurally, I think this is the right place to add this information, and I think the demotion of the in-out section to make a sibling of the borrowing/consuming section is the right approach.

Some of the content about borrowing and consuming is written more like part of the guide than reference, but I can help you clean that up later. If this is feeling too long for the reference, we can start a section in Memory Safety about ownership.

We probably don't need to add examples of exclusivity violations in the reference, because there's already a section that discusses these situations in more detail in "Memory Safety", which is linked at the end of the section.

The formal grammar needs to move at the end of sections, and needs to start with a "Grammar of" line. (When we get grammar we're happy with, we have to update the summary of the grammar manually too.) The new parameter-modifier needs to be added to one of the other productions -- currently, nothing in the grammar refers to this production, so there's no way to "get here" so to speak. I think it could be added to the definition of parameter, although I also see inout is already part of type-annotation, so that might be the place to add borrowing and consuming.

A minor point: How did you decide where to break lines? It looks like you might be hard wrapping at ~60 characters? The existing content breaks lines at clause boundaries, with the idea that each line is a "chunk" of prose that stands on its own to some extent. Most of the existing notes in the TSPL style guide about this tell you why to use semantic line breaks, but maybe more detail there is needed about exactly where to break lines.

TSPL.docc/ReferenceManual/Declarations.md Outdated Show resolved Hide resolved
TSPL.docc/ReferenceManual/Declarations.md Outdated Show resolved Hide resolved
TSPL.docc/ReferenceManual/Declarations.md Outdated Show resolved Hide resolved
TSPL.docc/ReferenceManual/Declarations.md Outdated Show resolved Hide resolved
@tbkka
Copy link
Contributor Author

tbkka commented Aug 14, 2023

I also see inout is already part of type-annotation, so that might be the place to add borrowing and consuming.

Umm.... Is that right? In particular, I don't think we currently allow var foo: inout Int (which is suggested by the grammar here).

@tbkka
Copy link
Contributor Author

tbkka commented Aug 15, 2023

The new parameter-modifier needs to be added to one of the other productions ...

I was a little confused about that, because the earlier sections in this particular file do not provide grammar productions for function declarations, just template examples.

@tbkka
Copy link
Contributor Author

tbkka commented Aug 15, 2023

The existing content breaks lines at clause boundaries, ...

I believe I've done that. If you see any place I've missed, please let me know (I'm certainly familiar with the old nroff/manpage conventions and do try to follow them strictly. But yes, I do tend to prefer short lines.)

@amartini51
Copy link
Member

amartini51 commented Sep 22, 2023

@tbkka

I also see inout is already part of type-annotation, so that might be the place to add borrowing and consuming.

Umm.... Is that right? In particular, I don't think we currently allow var foo: inout Int (which is suggested by the grammar here).

Looking back through the commit history, that part of the grammar is from commit ba96767, and came from the grammar in SE-0031. Prior to that commit, inout used to be part of the grammar for 'parameter'. Should we include those proposal authors as reviewers, once you and I have a PR that's ready for final technical and editorial review, or include them in the discussion about this grammar?

The new parameter-modifier needs to be added to one of the other productions ...

I was a little confused about that, because the earlier sections in this particular file do not provide grammar productions for function declarations, just template examples.

Maybe it's hard to follow while editing, but the formal grammar appears at the end of each section. So subsections don't contain their own grammar. For example, the Variable Declaration section has several subsections, and then one "Grammar of a variable declaration" box at the end.

Each section ends with a grammar-of box — in the case of Function Declaration, there are nine subsections, so you have to scroll a fair bit. The new grammar productions in all nine of those subsections are collected in the "Grammar of a function declaration" box at the end of the section.

Elsewhere in the reference, some subsections have their own grammar-of boxes. For function declarations, I think they're all in one box mostly because the subsections don't introduce much grammar — most of the grammar is the same for regular functions as it is for functions that are throwing, async, nonreturning, and so on. But maybe the Functions section is getting too long, and we should divide up its grammar to make it easier to navigate.

The existing content breaks lines at clause boundaries, ...

I believe I've done that. If you see any place I've missed, please let me know (I'm certainly familiar with the old nroff/manpage conventions and do try to follow them strictly. But yes, I do tend to prefer short lines.)

You're exactly correct; the convention here is essentially the same as nroff and man pages. Different writers have different styles for where to break lines and how long a line should be, which is fine. (I can often guess who wrote something in TSPL, based on formatting preferences.)

To help keep the content readable in a monospace font, let's prefer --- rather than an em-dash. I see one or two places elsewhere in the book where those are used, which I'm fixing. (EDIT: On more careful inspection, those were hyphens at the start of a line, which makes them look like list items, and renders as hyphens. Switched to ASCII em-dash markup at the end of the previous line.)

I'll take a light revision pass for style and, and then push to this branch.

- Introduce in prose 'copy' before showing it in code.
- Various style changes per Apple and DevPubs style: not using "since"
  to mean "because", placing "only" directly before the word it
  modifies, avoiding bare "this", preferring present rather than future
  tense, and naming of the & operator.
- Match typical TSPL style for wording at the start of a reference
  section and "prints" comments.
- Added XXX markers for issues that should block merging this branch.
@amartini51
Copy link
Member

@tbkka Please take a look at my revised version. Most of the changes are Apple or DevPubs style, with some my personal wording preference, and a couple minor changes to order — introducing things before showing them. I left XXX markers for things I want to resolve before merging this branch because the build script for TSPL treats them as a hard error, ensuring none get left behind.

I see you have some TODO markers near the end. Were those things you'd like to include in this PR, or possibilities for the future?

I'm also planning to re-read the SE proposal again while looking at the reference, checking for additional information we should include.

@amartini51
Copy link
Member

(Ping for @tbkka)

The grammar used to allow `inout` on any type annotation, following the
grammar changes described in SE-0031.  However, that isn't correct --
parameter modifiers like `inout` are allowed only in a function type or
a function declaration, as part of a parameter's type.  Separating the
productions for type-annotation and parameter-type-annotation adds a
small amount of duplication to the grammar, but it removes the
overproduction.
@tbkka
Copy link
Contributor Author

tbkka commented Oct 7, 2023

Your changes look good!

amartini51 and others added 2 commits October 6, 2023 17:17
Co-authored-by: Tim Kientzle <tkientzle@apple.com>
We do actually use "lifetime" already, so no issue with that here.

Avoiding "retain" which could be confused for manual reference counting,
and "alive" which requires extending the "object lifetime" metaphor.
@amartini51 amartini51 self-requested a review October 9, 2023 19:23
Copy link
Member

@amartini51 amartini51 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before I merge, I want to have someone double check the grammar changes with me. Otherwise I think this is ready now.

The new to-do markers that this PR adds are both things we can add later:

  • borrowing and consuming keywords with noncopyable argument types. We don't discuss noncopyable types yet in TSPL (rdar://110848216).

  • Any change of parameter modifier is ABI-breaking. TSPL doesn't discuss ABI stability other than@frozen.

amartini51 and others added 2 commits October 19, 2023 14:38
Per TR from Tim Kientzle, this shouldn't be documented here.  TSPL
doesn't really have any place where we document the function-calling
conventions (that's really more ABI than language).  The fact that we
currently always copy, even though it might get optimized out, is an
implementation detail.
Co-authored-by: Tim Kientzle <tkientzle@apple.com>
Co-authored-by: Rex <rex.fenley@gmail.com>
Copy link
Contributor

@chuckdude chuckdude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor changes throughout; approved with changes.

Please let me know if there's anything here that you'd like to discuss.

TSPL.docc/ReferenceManual/Declarations.md Outdated Show resolved Hide resolved
TSPL.docc/ReferenceManual/Declarations.md Outdated Show resolved Hide resolved
TSPL.docc/ReferenceManual/Declarations.md Outdated Show resolved Hide resolved
TSPL.docc/ReferenceManual/Declarations.md Outdated Show resolved Hide resolved
TSPL.docc/ReferenceManual/Declarations.md Outdated Show resolved Hide resolved
TSPL.docc/ReferenceManual/Declarations.md Outdated Show resolved Hide resolved
TSPL.docc/ReferenceManual/Declarations.md Outdated Show resolved Hide resolved
TSPL.docc/ReferenceManual/Declarations.md Outdated Show resolved Hide resolved
TSPL.docc/ReferenceManual/Declarations.md Show resolved Hide resolved
TSPL.docc/ReferenceManual/Declarations.md Outdated Show resolved Hide resolved
Edits from rdar://116694613

Co-authored-by: Chuck Toporek <chuck_toporek@apple.com>
Co-authored-by: Chuck Toporek <76215+chuckdude@users.noreply.github.com>
@amartini51 amartini51 merged commit c08214e into swiftlang:main Nov 9, 2023
amartini51 added a commit to amartini51/swift-book that referenced this pull request May 1, 2024
This got missed in PR swiftlang#173 when adding them.
amartini51 added a commit that referenced this pull request May 8, 2024
This list got missed in PR #173 when adding them to the reference.

Fixes: #300
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants