Skip to content

[Concurrency] Reimplement @TaskLocal as a macro #73078

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
May 2, 2024
Merged

[Concurrency] Reimplement @TaskLocal as a macro #73078

merged 7 commits into from
May 2, 2024

Conversation

ktoso
Copy link
Contributor

@ktoso ktoso commented Apr 17, 2024 •
edited
Loading

This reimplements @TaskLocal as a peer + accessor macro.

This is necessary to avoid sendable warnings about task locals having "mutable state" which property wrappers do introduce for the storage.

Resolves rdar://120914014

TODO:

@ktoso
Copy link
Contributor Author

ktoso commented Apr 17, 2024

@swift-ci please smoke test

@ktoso ktoso requested a review from slavapestov as a code owner April 18, 2024 08:52
@ktoso ktoso force-pushed the wip-tasklocal-macro branch from 032f6cd to dd8d8fa Compare April 18, 2024 09:42
ktoso
ktoso commented Apr 18, 2024
View reviewed changes
lib/Sema/TypeCheckConcurrency.cpp
auto &ctx = var->getASTContext();
if (classDecl == ctx.getTaskLocalDecl()) {
return isolation;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

workaround was removed

ktoso
ktoso commented Apr 18, 2024
View reviewed changes
stdlib/public/Concurrency/TaskLocal.swift

/// Macro that introduces a ``TaskLocal-class`` binding.
///
/// - SeeAlso: ``TaskLocal-class``
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to be the way to refer to the same named type

@ktoso ktoso force-pushed the wip-tasklocal-macro branch from dd8d8fa to d9b9139 Compare April 18, 2024 09:43
@ktoso
Copy link
Contributor Author

ktoso commented Apr 18, 2024

@swift-ci please smoke test

@ktoso ktoso mentioned this pull request Apr 18, 2024
Merged
@ktoso ktoso requested a review from ahoppen April 18, 2024 14:15
ahoppen
ahoppen reviewed Apr 18, 2024
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Looks good to me, mostly have small, local comments

@ktoso ktoso force-pushed the wip-tasklocal-macro branch 2 times, most recently from b560210 to 9f327d1 Compare April 19, 2024 04:44
@ktoso
Copy link
Contributor Author

ktoso commented Apr 19, 2024

@swift-ci please smoke test

FranzBusch
FranzBusch reviewed Apr 23, 2024
View reviewed changes
FranzBusch
FranzBusch reviewed Apr 23, 2024
View reviewed changes
@ktoso ktoso force-pushed the wip-tasklocal-macro branch from 9f327d1 to 4b515ed Compare April 30, 2024 04:55
@ktoso
Copy link
Contributor Author

ktoso commented Apr 30, 2024

@swift-ci please smoke test

@ktoso ktoso enabled auto-merge (squash) April 30, 2024 04:55
@ktoso
Copy link
Contributor Author

ktoso commented Apr 30, 2024

@swift-ci please smoke test

@ktoso ktoso force-pushed the wip-tasklocal-macro branch from 29067ea to 6c966c9 Compare April 30, 2024 12:03
@ktoso ktoso force-pushed the wip-tasklocal-macro branch from 6c966c9 to 2ac9f70 Compare April 30, 2024 13:04
@ktoso
Copy link
Contributor Author

ktoso commented Apr 30, 2024

Having a rough time figuring out what mac CI isn't happy about, locally validation test also is happy... Trying again

@ktoso
Copy link
Contributor Author

ktoso commented Apr 30, 2024

@swift-ci please smoke test

@rintaro
Copy link
Member

rintaro commented May 1, 2024

swiftlang/swift-driver#1590
@swift-ci Please smoke test

@rintaro
Copy link
Member

rintaro commented May 1, 2024

swiftlang/sourcekit-lsp#1212
@swift-ci Please smoke test

@rintaro
Copy link
Member

rintaro commented May 1, 2024

swiftlang/swift-driver#1590
@swift-ci Please smoke test

2 similar comments
@rintaro
Copy link
Member

rintaro commented May 1, 2024

swiftlang/swift-driver#1590
@swift-ci Please smoke test

@rintaro
Copy link
Member

rintaro commented May 1, 2024

swiftlang/swift-driver#1590
@swift-ci Please smoke test

@ktoso ktoso disabled auto-merge May 1, 2024 23:59
@ktoso
Copy link
Contributor Author

ktoso commented May 2, 2024 •
edited
Loading

Driver PR was merged, so reenabling automerging this :) Meh, updated branch un-necessarily.

Either way, thank you Rintaro!

@ktoso
Copy link
Contributor Author

ktoso commented May 2, 2024

swiftlang/swift-driver#1590
@swift-ci Please smoke test

@ktoso ktoso enabled auto-merge (squash) May 2, 2024 00:02
@ktoso
Copy link
Contributor Author

ktoso commented May 2, 2024

@swift-ci Please smoke test

@ktoso ktoso merged commit dc5e354 into swiftlang:main May 2, 2024
@ktoso ktoso deleted the wip-tasklocal-macro branch May 2, 2024 05:30
@hyp
Copy link
Contributor

hyp commented May 6, 2024 •
edited
Loading

@ktoso this change broke access control for TaskLocal-wrapped properties. What if you have a public property that's a TaskLocal:

https://github.com/pointfreeco/xctest-dynamic-overlay/blob/698be4131e778c2dd15ea47be0523280328bc2db/Sources/XCTestDynamicOverlay/XCTFail.swift#L4

and then a consumer module is accessing its projected value:

XCTFailContext.$current.withValue(...

After this change this code no longer compiles, as it complains that $current is now internal.
According to the property wrapper rules the $current projected value should be public and accessible. Is that correct, or are TaskLocal properties special?

@hyp
Copy link
Contributor

hyp commented May 6, 2024

this is breaking our builds that depend on this overlay.

@hyp hyp mentioned this pull request May 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FranzBusch FranzBusch FranzBusch left review comments

@ahoppen ahoppen ahoppen left review comments

@xedin xedin Awaiting requested review from xedin xedin is a code owner

@sophiapoirier sophiapoirier Awaiting requested review from sophiapoirier

@angela-laar angela-laar Awaiting requested review from angela-laar

@hborla hborla Awaiting requested review from hborla hborla is a code owner

@slavapestov slavapestov Awaiting requested review from slavapestov slavapestov is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ktoso @rintaro @hyp @FranzBusch @ahoppen