Revise doc comments for structured concurrency #36905

amartini51 · 2021-04-14T04:53:38Z

Technical and non-clean-up corrections:

Removed the mention of "if no current task is available" from
Task.priority because it's an instance variable. By definition, you
can't access it without some current task.
Changed the abstract for detach(priority:operation:) to note that
it's for non-throwing operations, since it uses Never as its
Task.Handle failure type.
Removed discussion of throwing and rethrowing errors from TaskGroup,
since its child tasks can't be throwing.
Removed explicit see-also cross references to a type that's being used
in the declaration. The rendered documentation already provides an
affordance to navigate to a type in that context.

Style changes, per Apple Style Guide and reference style guidelines:

Used "canceled" and "canceling" throughout. (But note "cancellation".)
Used contractions.
Removed third-level headings. The discussion sections they were in
are short enough that they doesn't need headings.
Removed code voice from abstracts.
Used noun phrase abstracts (not "Returns ...") for properties, instead
of describing them as if they were methods.
Referred to methods by their full names, not just the part before the
parentheses. For example, spawn(overridingPriority:operation:)
rather than just spawn or spawn().

Other changes:

Marked headings in type overviews by underling with a line of equal
signs (=), to match markup elsewhere in the standard library.
Included the type names when referring to methods, to avoid ambiguity
and to make it possible to look up the documentation for that method
if the reader doesn't already know what type it's on.

For rdar://76086293

amartini51 · 2021-04-14T04:58:40Z

Marking as a draft until the placeholders (marked with ◊) get filled in.

ktoso · 2021-04-14T07:10:30Z

stdlib/public/Concurrency/Task.swift

+///
+/// Only code that's running as part of the task can interact with that task,
+/// by invoking the appropriate context-sensitive static functions which operate
+/// on the current task. Because all such functions are asynchronous, they can only
 /// be invoked as part of an existing task, and therefore are guaranteed to be
 /// effective.


Sorry, this is outdated wording here;

We have both static (non async) functions and instance functions now. The wording about async functions and properties is pretty ancient sorry.

I think we can remove the entire sentence from "Because all such functions are asynchronous ... " since those dont exist anymore and they're handled by the static and instance ones.

ktoso · 2021-04-14T07:11:09Z

stdlib/public/Concurrency/Task.swift

 /// be invoked as part of an existing task, and therefore are guaranteed to be
 /// effective.
+/// ◊TR: What do we mean by "effective" above?


I think we can remove the entire sentence so no need to dwell here; but what I meant that "they will work and not crash utterly" :)

ktoso · 2021-04-14T07:13:06Z

stdlib/public/Concurrency/Task.swift

-/// Partial tasks are generally not interacted with by end-users directly,
-/// unless implementing a scheduler.
+/// Unless you're implementing a scheduler,
+/// you don't generally interacted with partial tasks directly.


Suggested change

/// you don't generally interacted with partial tasks directly.

/// you don't generally interact with jobs directly.

I believe John is removing the notion of partial tasks and they'll be named "Jobs": https://github.com/apple/swift/pull/36878/files#diff-92484f8fd5d76f2bd84b8509dc599fde13def3c55f342c8a64ffbba2cc9d7c97L7

ktoso · 2021-04-14T07:14:04Z

stdlib/public/Concurrency/Task.swift

+///
+/// ◊ a task checks its own cancellation status
+/// ◊ you might need to check cancellation repeatedly for long-running work
+/// ◊ the canonical response to cancelation is to throw `Task.CancellationError`


Yes, all these are correct.

The shortcut to "check and throw the cancellation error" is try Task.checkCancellation() but one might implement it just with Task.isCancelled and throwing

ktoso · 2021-04-14T07:17:28Z

stdlib/public/Concurrency/Task.swift

@@ -46,10 +63,9 @@ public struct Task {
 @available(macOS 9999, iOS 9999, watchOS 9999, tvOS 9999, *)
 extension Task {

-  /// Returns 'current' `Task` instance, representing the task from within which
-  /// this function was called.
+  /// Returns the task that this code is being run on.


I feel we should expand here, how about something like:

Suggested change

/// Returns the task that this code is being run on.

/// Returns the task that this code is being run on.

///

/// If invoked from the context of an asynchronous function or closure,

/// this is guaranteed to return a non-nil task. When called from a synchronous

/// context the returned value depends on wether the synchronous operation was

/// itself called from an asynchronous context or not. For example:

///

/// func hello() { assert(Task.current != nil) }

/// func asynchronous() async { hello() }

///

/// is correct and will not trigger the assertion, because an asynchronous function exists in the call-stack leading to the `Task.current` invocation.

or something like that?

ktoso · 2021-04-14T07:18:58Z

stdlib/public/Concurrency/Task.swift

  ///
-  /// If no current `Task` is available, returns `Priority.default`.
+  /// If you access this property outside of any task,
+  /// its value is `Priority.default`.


FIXME: I thought we fixed this -- this should be .unspecified (and in implementation as well)

ktoso · 2021-04-14T07:20:07Z

stdlib/public/Concurrency/Task.swift

  ///
-  /// TODO: Define the details of task priority; It is likely to be a concept
+  /// ◊TODO: Define the details of task priority; It is likely to be a concept
  ///       similar to Darwin Dispatch's QoS; bearing in mind that priority is not as
  ///       much of a thing on other platforms (i.e. server side Linux systems).


for better or worse nothing seems to have been done here and we're likely to stick to the ones we have -- the todo will be likely just removed.

ktoso · 2021-04-14T07:25:38Z

stdlib/public/Concurrency/Task.swift

-  ///   - this DOES affect `Task.currentPriority()`.
+  /// In both cases, priority elevation prevents a low-priority task
+  /// blocking the execution of a high priority task,
+  /// also known as *priority inversion*.


Hmm... I'm not sure about "prevents" it definitely "attempts to lessen the impact of" but we don't really fully prevent it... we still need to complete running the low priority task, even though now it became boosted to high priority... I guess maybe that counts as "prevents"? a bit iffy on the wording hm. @rjmccall maybe has a stronger opinion?

ktoso · 2021-04-14T07:26:51Z

stdlib/public/Concurrency/Task.swift

-  /// i.e. the task will run regardless of the handle still being present or not.
-  /// Dropping a handle however means losing the ability to await on the task's result
-  /// and losing the ability to cancel it.
+  /// It's not a programming error to discard a task's handle without awaiting or canceling the task.


Small side question, just to educate myself; so guidelines are to actually use "it's"? All my life in technical writing i've been avoiding it :) Is it because it's shorter/easier to read?

I'm not sure the exact reason, but yes that's the style used here. For documentation, we follow both Apple Style Guide and style guidance from DevPubs that's specific to reference. ASG's entry for contractions begins:

As part of Apple’s informal voice, contractions are used and recommended throughout most documentation, interface text, and marketing copy. Keep localization in mind when deciding how or whether to use contractions.

ktoso · 2021-04-14T07:27:14Z

stdlib/public/Concurrency/Task.swift

+  /// It's not a programming error to discard a task's handle without awaiting or canceling the task.
+  /// A task runs whether or not you still have its handle stored somewhere.
+  /// However, if you discard a task's handle, you give up the ability
+  /// to wait for that task's result or cancel the task.


ktoso · 2021-04-14T07:29:18Z

stdlib/public/Concurrency/Task.swift

-    /// ### Cancellation
-    /// If the awaited on task gets cancelled externally the `get()` will throw
-    /// a cancellation error.
+    /// If task is canceled, this method throws a cancellation error.


~~we lost the "externally" here.~~

edit, wait a second, this is some ancient semantics that are not actually implemented nor do I think we'd want them (nor are they in the proposal now).

I think we need to change as:

Suggested change

/// If task is canceled, this method throws a cancellation error.

ktoso · 2021-04-14T07:38:12Z

stdlib/public/Concurrency/Task.swift

+    /// If the task is canceled internally ---
+    /// for example, by checking for cancellation
+    /// and throwing a specific error, or by using `checkCancellation` ---
+    /// this method rethrows the error that was thrown by the task.


This wording is slightly off and ancient; most recent source of truth is https://github.com/apple/swift-evolution/blob/main/proposals/0304-structured-concurrency.md

I think the wording is rather:

/// If the task was cancelled it may check for it and throw a `Task.CancellationError`. /// It may also decide to handle cancellation in other ways, including returning a best-effort result or similar.

ktoso · 2021-04-14T07:38:37Z

stdlib/public/Concurrency/Task.swift

+    /// If the task is canceled internally ---
+    /// for example, by checking for cancellation
+    /// and throwing a specific error, or by using `checkCancellation` ---
+    /// this method rethrows the error that was thrown by the task.


same notes as above; this wording is wrong nowadays I believe

ktoso · 2021-04-14T07:39:23Z

stdlib/public/Concurrency/Task.swift

+  /// The task that this handle refers to might check for cancellation ---
+  /// however, since this method is nonthrowing,
+  /// the task can't throw `CancellationError` and needs to use another method
+  /// like returning `nil` instead.


sounds good

ktoso · 2021-04-14T07:39:59Z

stdlib/public/Concurrency/Task.swift

@@ -277,6 +292,8 @@ extension Task {
  /// Flags for schedulable jobs.
  ///
  /// This is a port of the C++ FlagSet.
+  /// ◊TR: What is FlagSet?
+  /// ◊TR: Why does that matter to users that it matches something in C++?


This is an internal type, so we don't need to document it in depth for end users; it only matters to us, implementors of the lib that this must align with the C++ Type of the same values

ktoso · 2021-04-14T07:41:05Z

stdlib/public/Concurrency/Task.swift

+/// Avoid using a detached task unless it isn't possible
+/// to model the operation using structured concurrency features like child tasks.
+/// Child tasks inherit the parent task's priority,
+/// task-local storage, and deadlines,


sadly we're not shipping deadlines this year

Suggested change

/// task-local storage, and deadlines,

/// task-local storage,

they'll come in the future though :)

ktoso · 2021-04-14T07:41:29Z

stdlib/public/Concurrency/Task.swift

+/// Child tasks inherit the parent task's priority,
+/// task-local storage, and deadlines,
+/// and canceling a parent task automatically cancels all of its child tasks.
+/// You need to handle these considerations manually with a detached task.


good to explain what people are losing here! 👍

ktoso · 2021-04-14T07:43:06Z

stdlib/public/Concurrency/Task.swift

+/// A detached task runs to completion
+/// unless it is explicitly canceled by calling the `Task.Handle.cancel()` method.
+/// Specifically, dropping a detached task's handle
+/// doesn't cancel that task.


This wording is tricky; the task always runs to completion, but the cancellation may cause it to return early or throw etc.

But since cancellation is cooperative, this means that if the task never checks cancellation, it'd finish as if nothing ever cancelled it (this is by design).

So we should not say that cancellation prevents running to completion, we can rather say that it impacts the task and how it will return.

Another note: cancellation does not prevent the task from being scheduled either, so it really is just informational to the task

ktoso · 2021-04-14T07:44:49Z

stdlib/public/Concurrency/Task.swift

-/// A detached task always runs to completion unless it is explicitly cancelled.
-/// Specifically, dropping a detached tasks `Task.Handle` does _not_ automatically
-/// cancel given task.
+/// If the operation throws an error, this method rethrows that error.


This is quite not true, detach is not throwing.

What happens is that if the operation throws, then the returned Task.Handle's try await get() will throw. This is "kind of" like rethrowing but I don't think we can use that word here since "rethrows" is a specific thing in Swift.

I'm changing "rethrows" to "propagates", to match what we call this in TSPL where the error makes its way back through a series of calls to throwing functions.

https://docs.swift.org/swift-book/LanguageGuide/ErrorHandling.html#ID510

ktoso · 2021-04-14T07:45:10Z

stdlib/public/Concurrency/Task.swift

+/// A detached task runs to completion
+/// unless it is explicitly canceled by calling the `Task.Handle.cancel()` method.
+/// Specifically, dropping a detached task's handle
+/// doesn't cancel that task.


same concern on wording as above

ktoso · 2021-04-14T07:45:35Z

stdlib/public/Concurrency/Task.swift

-  /// Suspends the current task for _at least_ the given duration
-  /// in nanoseconds.
+  /// Suspends the current task,
+  /// and wait for at least the given duration before resuming it.


tenses are mixed up? Suspends + waits or suspend + wait?

ktoso · 2021-04-14T07:46:13Z

stdlib/public/Concurrency/Task.swift

-  /// Suspends the current task for _at least_ the given duration
-  /// in nanoseconds.
+  /// Suspends the current task,
+  /// and wait for at least the given duration before resuming it.


can we bring back the emphasis on "at least"? It's important to highlight that it's not an exact sleep time guarantee

I need to check editorial guidelines, but I think we avoid emphasis in abstracts because they show up on their own in places like navigation and Xcode quick help.

ktoso · 2021-04-14T07:47:02Z

stdlib/public/Concurrency/Task.swift

+  /// Note that if this task is the highest-priority task in the system,
+  /// the executor immediately resumes execution of the same task.
+  /// As such,
+  /// this method isn't necessarily a way to avoid resource starvation.


(a bit weird formatting in this comment?)

What looks weird to you? If it's just the placement of line breaks, that's a convention we've used elsewhere in documentation to reduce noise in diffs. Instead of breaking at the 80 column mark and rewrapping, we break lines at clause and sentence boundaries, so they don't need to be rewrapped after revising.

I see, that's a new technique to me... A bit weird to be honest since the comments are often read in the source too hm

ktoso · 2021-04-14T07:49:26Z

stdlib/public/Concurrency/Task.swift

+/// the unsafe task handle passed to the closure is always be non-nil
+/// because an asynchronous function always runs in the context of a task.
+/// However if you call this function from the body of a synchronous function,
+/// the unsafe task handle is `nil`


Suggested change

/// the unsafe task handle is `nil`

/// the unsafe task handle may be `nil`

feels more right somehow...? maybe not?

If I call this method from sync code, and my backtrace was all sync back up the call stack, when would this return a non-nil value?

it would be nil then -- if the entire callstack is outside of swift concurrency APIs

Thanks. If it's always nil in that case, I'll avoid writing "may be":

/// However if you call this function from the body of a synchronous function, /// and that function isn't executing in the context of any task, /// the unsafe task handle is `nil`.

ktoso · 2021-04-14T07:50:33Z

stdlib/public/Concurrency/Task.swift

-/// An `UnsafeCurrentTask` should not be stored for "later" access.
+/// To get an instance of `UnsafeCurrentTask` for the current task,
+/// call the `withUnsafeCurrentTask(body:)` method.
+/// Don't try to store an unsafe task handle


maybe just

Suggested change

/// Don't try to store an unsafe task handle

/// Don't store an unsafe task handle

?

(reading a book on writing style recently and it left an impression on me to cut unnecessary words :-))

Maybe this phrasing doesn't work as well in reference here. In TSPL, we use "don't try to do X" to describe something that won't work. Here, you think you can store an unsafe task handle, but you really can't store it. I think the counterpoint to my argument is that you can store it — just that storing the handle doesn't let you do anything useful.

storing it is actively unsafe and would lead to undefined behavior, so the stronger wording would be good hm

Removed "try to" in commit 72322eb.

ktoso · 2021-04-14T07:52:12Z

stdlib/public/Concurrency/Task.swift

 /// generally safe to be invoked from any task/thread.
+/// ◊TR: Which ones, exactly, is that?  "Generally" isn't good enough here.


we can remove the "generally" the trick is "if an API exists on Task, it is also safe on UnsafeCurrentTask from other contexts; If it only exists on UnsafeCurrentTask, it is unsafe to call it from other tasks"

@ktoso

Folding the example @ktoso suggested into the existing code, and removing content that was repeated here from next() but isn't relevant.

amartini51 · 2021-04-30T16:15:58Z

@ktoso I rebased these changes on top of the Swift 5.5 release branch, and changed the PR to target that branch. If you want to avoid re-reading the diff, you can look at the commits from 3c8b4f8deb6 on instead.

This was removed in commit f748e8d. Found this error and verified it's the only change outside of doc comments by running: git diff release/5.5 | grep -v '^+ *///\|- *///' | grep -v '^@@' | grep -v '^ '

ktoso · 2021-05-07T08:02:47Z

Oh sorry I missed the updates! Checking now :) Please ping me on chat if I miss something for a few days :)

For rdar://77330408

amartini51 · 2021-05-13T04:56:54Z

@swift-ci please smoke test

Conflicts in stdlib/public/Concurrency/TaskGroup.swift from async/spawn renaming. Resolved by copying the docs to the new symbols from their old names.

ktoso

Looks good, thanks @amartini51 !

@Availability

Conflicts were just @Availability marker changes.

amartini51 · 2021-05-13T18:46:29Z

@swift-ci please smoke test

amartini51 · 2021-05-13T22:41:43Z

@swift-ci Please test and merge.

amartini51 requested a review from ktoso April 14, 2021 04:53

amartini51 marked this pull request as draft April 14, 2021 04:57

amartini51 force-pushed the task_doc_comments_76086293 branch from dd74560 to e7aff66 Compare April 14, 2021 05:03

ktoso reviewed Apr 14, 2021

View reviewed changes

amartini51 added 7 commits April 29, 2021 13:38

Drop trailing whitespace; add missing period.

bbb22c9

Incorporate tech review from @ktoso.

3c8b4f8

Restore thread-safety note.

f9d5d7a

Current implementation waits, per @ktoso.

f8c3dde

Focus on nextResult() and show an example.

32eac44

Folding the example @ktoso suggested into the existing code, and removing content that was repeated here from next() but isn't relevant.

Show how for-await interacts with errors.

c57dea1

Remove tech review queries that were answered.

d8e5d30

amartini51 force-pushed the task_doc_comments_76086293 branch from 2f12b74 to d8e5d30 Compare April 30, 2021 16:12

amartini51 changed the base branch from main to release/5.5 April 30, 2021 16:13

amartini51 added 2 commits April 30, 2021 09:21

Back out an unintended change from rebasing.

d152153

This was removed in commit f748e8d. Found this error and verified it's the only change outside of doc comments by running: git diff release/5.5 | grep -v '^+ *///\|- *///' | grep -v '^@@' | grep -v '^ '

Remove the second copy of a query.

2af42d7

amartini51 requested a review from natecook1000 April 30, 2021 20:16

amartini51 added 2 commits May 6, 2021 14:40

Track 'release/5.5' to resolve merge conflicts.

f3c073d

Revise doc comments for changes from the release branch.

614c008

Incorporate editorial feedback.

112acee

For rdar://77330408

Track 'release/5.5' to resolve merge conflicts.

b8f4935

Conflicts in stdlib/public/Concurrency/TaskGroup.swift from async/spawn renaming. Resolved by copying the docs to the new symbols from their old names.

amartini51 marked this pull request as ready for review May 13, 2021 05:10

ktoso approved these changes May 13, 2021

View reviewed changes

amartini51 added 2 commits May 13, 2021 11:15

Make wording a bit stronger.

72322eb

Remove tech review markers.

6167d0d

amartini51 changed the title ~~Initial clean-up pass for structured concurrency doc comments~~ Revise doc comments for structured concurrency May 13, 2021

Track 'release/5.5' to resolve merge conflicts.

290cf5a

Conflicts were just @Availability marker changes.

swift-ci merged commit fe288c2 into swiftlang:release/5.5 May 14, 2021

benrimmington mentioned this pull request May 21, 2021

Structured concurrency revisions for third review swiftlang/swift-evolution#1368

Merged

amartini51 deleted the task_doc_comments_76086293 branch December 8, 2021 17:52

	/// you don't generally interacted with partial tasks directly.
	/// you don't generally interact with jobs directly.

-  /// Returns the task that this code is being run on.
+  /// Returns the task that this code is being run on.
+  ///
+  /// If invoked from the context of an asynchronous function or closure,
+  /// this is guaranteed to return a non-nil task. When called from a synchronous
+  /// context the returned value depends on wether the synchronous operation was
+  /// itself called from an asynchronous context or not. For example:
+  ///
+  ///    func hello() { assert(Task.current != nil) }
+  ///    func asynchronous() async { hello() }
+  ///
+  /// is correct and will not trigger the assertion, because an asynchronous function exists in the call-stack leading to the `Task.current` invocation.

	/// task-local storage, and deadlines,
	/// task-local storage,

	/// the unsafe task handle is `nil`
	/// the unsafe task handle may be `nil`

	/// Don't try to store an unsafe task handle
	/// Don't store an unsafe task handle

		/// generally safe to be invoked from any task/thread.
		/// ◊TR: Which ones, exactly, is that? "Generally" isn't good enough here.

Revise doc comments for structured concurrency #36905

Revise doc comments for structured concurrency #36905

Conversation

amartini51 commented Apr 14, 2021

amartini51 commented Apr 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktoso Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktoso Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktoso May 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amartini51 May 13, 2021 • edited Loading

Choose a reason for hiding this comment

ktoso Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amartini51 commented Apr 30, 2021

ktoso commented May 7, 2021

amartini51 commented May 13, 2021

ktoso left a comment

Choose a reason for hiding this comment

amartini51 commented May 13, 2021

amartini51 commented May 13, 2021

ktoso Apr 14, 2021 •

edited

Loading

ktoso Apr 14, 2021 •

edited

Loading

ktoso May 7, 2021 •

edited

Loading

amartini51 May 13, 2021 •

edited

Loading

ktoso Apr 14, 2021 •

edited

Loading