Skip to content

[Concurrency] Prevent potential for condfail with owned task executor parameter #74929

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 3 commits into from
Jul 3, 2024

Conversation

ktoso
Copy link
Contributor

@ktoso ktoso commented Jul 3, 2024

We noticed this issue while checking #74928 with @tshortli.

While the new parameter is added in a compatible way where code which
does not refer to it will get a defaulted nil value; since we refer to a
new parameter name in source, we need to guard it with a language
feature -- as old compilers will not have this new name available.

Fixes condfails that resulted from these recent PRs: #74000, #74543

Fixes rdar://131064807.

@ktoso
Copy link
Contributor Author

ktoso commented Jul 3, 2024

@swift-ci please smoke test

@ktoso ktoso requested review from DougGregor and tshortli July 3, 2024 07:05
@ktoso
Copy link
Contributor Author

ktoso commented Jul 3, 2024

@swift-ci please smoke test

ktoso and others added 3 commits July 3, 2024 10:36
While the new parameter is added in a compatible way where code which
does not refer to it will get a defaulted nil value; since we refer to a
new parameter name in source, we need to guard it with a language
feature -- as old compilers will not have this new name available.

This should prevent a potential condfail issue.
Supported older compilers don't enable this feature by default, so it can't be
omitted from the `_Concurrency` module's flags (regression from
swiftlang#74543).

Additionally, remove `@_allowFeatureSuppression(IsolatedAny)` from all
declarations. We no longer need to support compilers that don't have the
`IsolatedAny` feature, so the suppression is superfluous and the alternative
branches didn't actually build anyways. _Additionally_, the suppressible
feature logic could not handle suppressing `IsolatedAny` simultaneously with
`SendingArgsAndResults`, resulting in a broken interface because `sending` was
used outside `#if $SendingArgsAndResults` guards.
@tshortli tshortli force-pushed the avoid-condfail-with-owned-taskgroup branch from e3f5ce0 to fb6ab05 Compare July 3, 2024 17:37
@tshortli
Copy link
Contributor

tshortli commented Jul 3, 2024

Additional fixes I added after some testing:

  • Restore -enable-experimental-feature IsolatedAny (older compilers don't enable it by default).
  • Remove @_allowFeatureSuppression(IsolatedAny) attributes. The suppressed alternatives for these methods did not build and additionally they were leaking the sending keyword into scopes that were not protected by $SendingArgsAndResults.

@tshortli
Copy link
Contributor

tshortli commented Jul 3, 2024

@swift-ci please smoke test

@tshortli tshortli requested a review from rjmccall July 3, 2024 17:40
@tshortli tshortli enabled auto-merge July 3, 2024 18:41
@tshortli tshortli merged commit 3626000 into swiftlang:main Jul 3, 2024
3 checks passed
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