Skip to content

🍒[5.9][Concurrency] Adopt stable keyword consuming instead of __owned #65575

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 1 commit into from
May 19, 2023

Conversation

ktoso
Copy link
Contributor

@ktoso ktoso commented May 2, 2023

Description:
This change is ABI stable with regards to the mangled name of the enqueue method.

We confirmed this by manually comparing mangled names of a method with __owned and later a consuming parameter -- please double check this though @kavon as you'd know more about this than myself.

This also improves diagnostics around implementing move-only job requirement, since the diagnostics offered by _owned are wrong: they don't show the attribute leading people to add func enqueue( job: ExecutorJob) which is incorrect because they MUST add consuming.

This PR includes tests that simulate an SDK with either the __owned or consuming versions and implementations with the other spelling, to make sure this works as expected.

Risk: Low, the mangling and ABI is the same.
Review by: @kavon @DougGregor
Testing: CI testing; manually verified mangling of the changed method
Original PR: #65571
Radar: rdar://108467550

@ktoso ktoso requested a review from a team as a code owner May 2, 2023 05:31
@ktoso
Copy link
Contributor Author

ktoso commented May 2, 2023

@swift-ci please test

@ktoso ktoso changed the title [Concurrency] Adopt stable keyword consuming instead of __owned 🍒[5.9][Concurrency] Adopt stable keyword consuming instead of __owned May 2, 2023
@ktoso ktoso added concurrency Feature: umbrella label for concurrency language features 🍒 release cherry pick Flag: Release branch cherry picks swift 5.9 labels May 2, 2023
@ktoso
Copy link
Contributor Author

ktoso commented May 2, 2023

Depends on #65455 since that introduces new methods; so we want to do the same to the new ones IF we are to merge these new methods.

add test for warnings emitted when missing consuming attribute

use -emit-sil in mock SDK tests for better coverage
@ktoso ktoso force-pushed the pick-consuming-keyword-job branch from 116d98e to 4cad4f6 Compare May 11, 2023 13:27
@ktoso ktoso requested review from kavon, DougGregor and hborla May 11, 2023 13:27
@ktoso
Copy link
Contributor Author

ktoso commented May 11, 2023

@swift-ci please test

@ktoso
Copy link
Contributor Author

ktoso commented May 11, 2023

Github is having issues; Exception: ['git', 'clone', '--recursive', 'https://github.com/apple/swift-docc.git', 'swift-docc'] checkouts failing

@ktoso
Copy link
Contributor Author

ktoso commented May 11, 2023

@swift-ci please test

@kavon
Copy link
Member

kavon commented May 12, 2023

Just as a note:

This also improves diagnostics around implementing move-only job requirement, since the diagnostics offered by _owned are wrong: they don't show the attribute leading people to add func enqueue( job: ExecutorJob) which is incorrect because they MUST add consuming.

This isn't what the PR is improving. It's just using the consuming attribute rather than __owned, because the latter is confusing for people to see because it's not one of the official ownership specifiers (it's an old name for consuming). So this change is really about changing an API so it uses the better names for this. I don't think we're ever going to suggest people write __owned in a diagnostic.

@ktoso
Copy link
Contributor Author

ktoso commented May 15, 2023

I mean this specifically was causing bad diagnostics telling people to implement "enqueue( job: ExecutorJob)" which then would not compile, since it was missing the __owned; Unless I'm misremembering this change also causes producing the right diagnostic.

@ktoso ktoso merged commit 782990e into swiftlang:release/5.9 May 19, 2023
@ktoso ktoso deleted the pick-consuming-keyword-job branch May 19, 2023 18:00
@ktoso
Copy link
Contributor Author

ktoso commented May 19, 2023

Thanks for review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
concurrency Feature: umbrella label for concurrency language features 🍒 release cherry pick Flag: Release branch cherry picks swift 5.9
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants