Skip to content

[Educational Notes] Start adding educational notes for data-race safety. #79509

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

Merged
merged 6 commits into from
Mar 4, 2025

Conversation

hborla
Copy link
Member

@hborla hborla commented Feb 20, 2025

Putting this PR up now because I'd love some feedback on the depth of explanation and the writing style of the educational note. I'm working on more, but am still trying to get a feel for how best to write these notes.

Note that the suggested fix for the region isolation error assumes the existence of @execution(caller) attribute because it does exist right now on main. However, the proposal that adds this feature is under review now, and this note will be updated along with any changes to the design as a result of the review discussion.

I also removed complex-closure-inference.md because it's just out of date. Multi-statement closure inference was added in SE-0362.

Resolves #79640

This educational note was obsoleted by SE-0362.
@hborla hborla changed the title [Educational Notes] Start updating educational notes for data-race safety. [Educational Notes] Start adding educational notes for data-race safety. Feb 20, 2025
Copy link

@leberwurstsaft leberwurstsaft left a comment

Choose a reason for hiding this comment

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

Yay education 🎉
Thanks for adding this!

Copy link
Member

@heckj heckj left a comment

Choose a reason for hiding this comment

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

In the set of changes I tried to make the wording a bit more direct, assuming the reading is a developer who's seeing these messages "in flight" and may or may not know about concurrency at any level of detail. So in particular, I tried to match the phrasing in the Swift 6 migration guide for terms, tried to make the discussion a bit more direct in guidance, and knocked out some of the passive verbs to make it more clear what was doing the guaranteeing or allowing when it comes to the code.

I'm hoping it's helpful, but please disregard if it isn't - or if I'm inaccurate in anything. I tried hard not to introduce any inaccuracies, but I don't know this topic to the depth y'all do.

@@ -0,0 +1,52 @@
# Sending value risks causing data races

If a type does not conform to `Sendable` the compiler will enforce that each instance of that type is only accessed by one concurrency domain at a time. The `sending 'x' risks causing data races` error indicates that your code can access a non-`Sendable` value from multiple concurrency domains at once.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If a type does not conform to `Sendable` the compiler will enforce that each instance of that type is only accessed by one concurrency domain at a time. The `sending 'x' risks causing data races` error indicates that your code can access a non-`Sendable` value from multiple concurrency domains at once.
If a type does not conform to `Sendable` the compiler will enforce that each instance of that type is only accessed by one isolation domain at a time. The `sending 'x' risks causing data races` error indicates that your code can access a non-`Sendable` value from multiple isolation domains at once.

Copy link
Member

Choose a reason for hiding this comment

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

I think by my reading that "concurreny domain" and "isolation domain" are equivalent, and existing docs seem to lean into the term "isolation domain" over the other.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, these terms are equivalent. But I also tend to agree that it may be confusing for programmers who've seen "isolation domain" before in Swift's documentation. It sounds like "concurrency domain" is a more approachable/readable term for this, so this is why it appears in these educational notes. That being said, I agree that we should understand when each of these terms are most appropriate to use, or whether they should be used interchangeably to avoid possible confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

I go back and forth on which term is better. The current documentation is already fragmented - TSPL uses "concurrency domain" and the migration guide uses "isolation domain". I think we should settle which term to standardize on in swiftlang/swift-migration-guide#130 (or perhaps in a forum discussion) and update everything at once. I'm going to continue using "concurrency domain" for now, but I will update these notes and TSPL, or the migration guide depending on what we decide.


If a type does not conform to `Sendable` the compiler will enforce that each instance of that type is only accessed by one concurrency domain at a time. The `sending 'x' risks causing data races` error indicates that your code can access a non-`Sendable` value from multiple concurrency domains at once.

For example, if a value can be accessed from the main actor, it's invalid to send the same instance to another concurrency domain while the main actor can still access it. This mistake is common when calling an `async` function on a class from the main actor:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For example, if a value can be accessed from the main actor, it's invalid to send the same instance to another concurrency domain while the main actor can still access it. This mistake is common when calling an `async` function on a class from the main actor:
For example, if a value can be accessed from the main actor, it's invalid to send the same instance to another isolation domain while the main actor can still access it. This mistake is common when calling an `async` function on a class from the main actor:

Copy link
Member

@simanerush simanerush left a comment

Choose a reason for hiding this comment

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

Thank you for writing these, so cool to see progress in concurrency documentation!🎉

@@ -0,0 +1,52 @@
# Sending value risks causing data races

If a type does not conform to `Sendable` the compiler will enforce that each instance of that type is only accessed by one concurrency domain at a time. The `sending 'x' risks causing data races` error indicates that your code can access a non-`Sendable` value from multiple concurrency domains at once.
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, these terms are equivalent. But I also tend to agree that it may be confusing for programmers who've seen "isolation domain" before in Swift's documentation. It sounds like "concurrency domain" is a more approachable/readable term for this, so this is why it appears in these educational notes. That being said, I agree that we should understand when each of these terms are most appropriate to use, or whether they should be used interchangeably to avoid possible confusion.

@hborla hborla force-pushed the educational-notes branch 3 times, most recently from 8fda891 to 28666ae Compare March 4, 2025 05:36
@hborla hborla force-pushed the educational-notes branch from 28666ae to 36d1d34 Compare March 4, 2025 05:41
@hborla hborla force-pushed the educational-notes branch from 36d1d34 to fdd7402 Compare March 4, 2025 05:44
@hborla
Copy link
Member Author

hborla commented Mar 4, 2025

@swift-ci please smoke test

@hborla
Copy link
Member Author

hborla commented Mar 4, 2025

Thanks everyone for the feedback so far! There's more I want to do to improve these notes, but I think those changes will be easier to review as follow-up PRs. I'd also like to experiment with incorporating some more structure into the notes, e.g. with headings so they're easier to scan.

@hborla
Copy link
Member Author

hborla commented Mar 4, 2025

swiftlang/swift-installer-scripts#397

@swift-ci please test Windows

@hborla hborla merged commit 3a744d2 into swiftlang:main Mar 4, 2025
3 checks passed
@hborla hborla deleted the educational-notes branch March 4, 2025 19:22
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.

Add initial educational notes for common data-race safety errors
4 participants