Skip to content

[Concurrency][SE-review update] Task names update #80984

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 4 commits into from
May 27, 2025

Conversation

ktoso
Copy link
Contributor

@ktoso ktoso commented Apr 22, 2025

This lowers the availability of the new name: Task initializers to "as low as possible" as requested by the SE review acceptance.

Specifically, if a method gets the TaskExecutor the lowest availability we can do is 6.0. Otherwise we try 5.1. Discarding task group forces us to be 5.9.

While doing this, we move all these methods, as well as legacy APIs, into gyb generation. So we save significant repetition and finally have one place to edit documentation for the many overloads.

This also CHANGES the always-emit-into-client functions, instead of the alternative of 50 overloads we'd have to do otherwise. So we're keeping the number of overloads managable, at the risk of a rare source break if someone used Task.init as function -- this was discussed and considered an acceptable risk.

@ktoso
Copy link
Contributor Author

ktoso commented Apr 22, 2025

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented Apr 22, 2025

@swift-ci please test

@ktoso
Copy link
Contributor Author

ktoso commented Apr 22, 2025

@swift-ci please test source compatibility

@ktoso ktoso changed the title task names review update [Concurrency][SE-review update] Task names update Apr 22, 2025
@ktoso
Copy link
Contributor Author

ktoso commented Apr 22, 2025

Windows too hm


TimeZone_ICU.swift.obj : error LNK2001: unresolved external symbol __imp_$sSS17_StringProcessing14RegexComponent0C7BuilderMc

bin\FoundationInternationalization.dll : fatal error LNK1120: 2 unresolved externals

@ktoso
Copy link
Contributor Author

ktoso commented Apr 22, 2025

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented Apr 22, 2025

@swift-ci please test source compatibility

@ktoso
Copy link
Contributor Author

ktoso commented Apr 22, 2025

@swift-ci please test source compatibility

@ktoso
Copy link
Contributor Author

ktoso commented Apr 22, 2025

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented Apr 23, 2025

UPDATE: We decided to change the always-emit-into-client APIs, rather than keep adding exponentially more overloads.

--

Well, this turned out quite more complex:

  • 16 declarations for all the task groups task groups
    • some of them, 5.1
    • some of them 5.9 (because DiscardingTaskGroup)
    • some of them 6.0 (because any API accepting TaskExecutor)
  • 24 declarations of Task.init because of init/detached/throwing/non-throwing/startSynchronously and the various "not available in embedded" etc...
    • any Task.init with a TaskExecutor is 6.0
    • baseline are 5.1
    • 🤔 There's a silly 3x here thanks to $Embedded and SWIFT_STDLIB_TASK_TO_THREAD_MODEL_CONCURRENCY, maybe we could simplify the number of redeclarations here a bit... at least so that it's a 2x and not a 3x heh. I'll think more about it.
  • 10 declarations for startSynchronously
    • 8 for the various task groups
    • 2 for Task.init -- one throwing and one not throwing

That's, 50 declarations in total, and I just noticed we didn't do startSynchronously + detached, so that's 2 more.

@ktoso ktoso force-pushed the task-names-update branch from 7c7607b to 16119e5 Compare April 23, 2025 06:20
ktoso added a commit to ktoso/swift-evolution that referenced this pull request Apr 24, 2025
Implemented, only pending a slight availability change to be done in: swiftlang/swift#80984
@ktoso ktoso force-pushed the task-names-update branch from ca9ba77 to 32e1b67 Compare April 24, 2025 13:01
@ktoso
Copy link
Contributor Author

ktoso commented Apr 24, 2025

@swift-ci please smoke test

@ktoso ktoso requested a review from al45tair April 24, 2025 13:05
@ktoso
Copy link
Contributor Author

ktoso commented Apr 25, 2025

Blocked on @kubamracek's idea to fix the embedded build.

@kubamracek
Copy link
Contributor

#81107 is now merged. I think you should be able to drop the SIA_ARCH changes.

@ktoso
Copy link
Contributor Author

ktoso commented Apr 28, 2025

Great, thank you!

@ktoso ktoso force-pushed the task-names-update branch from 32e1b67 to 009af88 Compare April 28, 2025 06:41
@ktoso
Copy link
Contributor Author

ktoso commented Apr 28, 2025

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented May 6, 2025

@swift-ci please test

@ktoso
Copy link
Contributor Author

ktoso commented May 7, 2025

@swift-ci please smoke test macOS

@ktoso
Copy link
Contributor Author

ktoso commented May 7, 2025

Cannot reproduce the linux failure on Linux either... very confused what's going on here - need to keep digging. A fresh linux build works fine, so I'm not sure why CI would report compile errors :\

@ktoso ktoso force-pushed the task-names-update branch from 6fae193 to a512765 Compare May 7, 2025 08:10
@ktoso
Copy link
Contributor Author

ktoso commented May 7, 2025

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented May 23, 2025

@swift-ci please smoke test

@ktoso ktoso force-pushed the task-names-update branch from 74cd4d5 to b925431 Compare May 23, 2025 05:32
@ktoso
Copy link
Contributor Author

ktoso commented May 23, 2025

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented May 23, 2025

@swift-ci please smoke test

@ktoso ktoso marked this pull request as draft May 23, 2025 16:31
@ktoso ktoso force-pushed the task-names-update branch from e0cb825 to 313551e Compare May 23, 2025 16:55
ktoso added 3 commits May 23, 2025 10:30
…EIC funcs

[Concurrency] Lower availability for task names + addTask

WIP on changing the always emit into client funcs
…utor

[Concurrency] Adjust tests for renamed initializer

[Embedded][Concurrency] Workaround for Gyb+concurrency module+embedded

Add Task+init gyb to embedded; Remove TaskGroup+Embedded.swift workaround

[Concurrency] Undo build workaround
@ktoso ktoso force-pushed the task-names-update branch from 313551e to ee7c32b Compare May 23, 2025 17:30
@ktoso
Copy link
Contributor Author

ktoso commented May 23, 2025

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented May 23, 2025

@swift-ci please test source compatibility

@ktoso ktoso marked this pull request as ready for review May 23, 2025 17:40
@ktoso
Copy link
Contributor Author

ktoso commented May 23, 2025

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented May 23, 2025

@swift-ci please test source compatibility

@ktoso ktoso merged commit 31b6ae2 into swiftlang:main May 27, 2025
5 checks passed
ktoso added a commit that referenced this pull request May 28, 2025
**Description**: This adds "task name" parameter to all task creating
functions.

This is done in a few ways, e.g. we can backdeploy this to 5.1 in APIs
which do not accept the `TaskExecutor` but it they do we provide a
version for 6.0+ etc. This was requested in the SE acceptable of this
proposal [Acceptance post
SE-0469](https://forums.swift.org/t/accepted-with-modifications-se-0469-task-naming/79438).

This moves all these declarations to gyb since going through them one by
one has become unmaintainable otherwise.

**Scope/Impact**: All task creation APIs now gain a new task name
parameter.
**Risk:** Medium, changes existing APIs rather than adding "even more
overloads" though this risk was discussed in the team and accepted. This
has a potential to be source breaking it someone used Task.init and
friends as function.
**Testing**: CI testing, source compatibility suite testing
**Reviewed by**: 

**Original PR:** 
- #81107 build changes required
for this
- #80984


**Radar:**

---------

Co-authored-by: Kuba Mracek <[email protected]>
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.

3 participants