Skip to content
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

Suggest @preconcurrency on conformances it could help #73741

Merged
merged 3 commits into from
May 22, 2024

Conversation

DougGregor
Copy link
Member

When diagnosing a case where an actor-isolated witness cannot satisfy a non-isolated requirement, also suggest that the conformance could be annotated with @preconcurrency.

@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@DougGregor DougGregor force-pushed the suggest-preconcurrency-conformances branch from ffebd09 to 523e254 Compare May 20, 2024 01:47
@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@DougGregor DougGregor enabled auto-merge May 20, 2024 01:48
@DougGregor DougGregor force-pushed the suggest-preconcurrency-conformances branch from 523e254 to 42d819b Compare May 20, 2024 04:20
@DougGregor
Copy link
Member Author

@swift-ci please smoke test

1 similar comment
@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@@ -2655,6 +2655,8 @@ WARNING(add_predates_concurrency_import,none,
"%select{| as warnings}0", (bool, Identifier))
WARNING(remove_predates_concurrency_import,none,
"'@preconcurrency' attribute on module %0 has no effect", (Identifier))
NOTE(add_preconcurrency_to_conformance,none,
"add '@preconcurrency' to the %0 conformance to suppress isolation-related diagnostics", (DeclName))
Copy link
Member

Choose a reason for hiding this comment

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

Instead of just saying this will suppress diagnostics, should we say that applying @preconcurrency will switch from static isolation diagnostics to dynamic ones? I think this should somehow mention that you're opting into dynamic assertions by doing this.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is a great point! Thank you!

@DougGregor
Copy link
Member Author

@swift-ci please smoke test Linux

@DougGregor
Copy link
Member Author

@swift-ci please clean smoke test Linux

@DougGregor DougGregor force-pushed the suggest-preconcurrency-conformances branch from aa63d7a to 9e26152 Compare May 20, 2024 22:17
@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@DougGregor
Copy link
Member Author

@swift-ci please smoke test

When diagnosing a case where an actor-isolated witness cannot satisfy
a non-isolated requirement, also suggest that the conformance could be
annotated with `@preconcurrency`.
@DougGregor DougGregor force-pushed the suggest-preconcurrency-conformances branch from 8a94990 to 5a0e70a Compare May 21, 2024 05:07
@DougGregor
Copy link
Member Author

@swift-ci please clean smoke test

@DougGregor
Copy link
Member Author

@swift-ci please clean test Linux

@DougGregor
Copy link
Member Author

@swift-ci please clean smoke test Linux

@DougGregor
Copy link
Member Author

@swift-ci please test Linux

@@ -573,6 +573,8 @@ class NormalProtocolConformance : public RootProtocolConformance,
Context(dc) {
assert(!conformingType->hasArchetype() &&
"ProtocolConformances should store interface types");
assert((preconcurrencyLoc.isInvalid() || isPreconcurrency) &&
"Cannot have a @preconcurrency location without isPreconcurrency");
Copy link
Member

Choose a reason for hiding this comment

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

Thank you!

We moved some diagnostics around, which ended up breaking some downstream
tests that we don't want to break. Restore the location.
@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@DougGregor DougGregor merged commit c18d2b1 into apple:main May 22, 2024
3 checks passed
@DougGregor DougGregor deleted the suggest-preconcurrency-conformances branch May 22, 2024 05:56
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.

None yet

3 participants