Skip to content

[concurrency] implement the 'unsafe' option for @actorIndependent #34260

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
Oct 21, 2020

Conversation

kavon
Copy link
Member

@kavon kavon commented Oct 9, 2020

This PR implements @actorIndependent(unsafe), which disables static safety checks for accesses to actor-isolated storage. It is otherwise semantically the same as @actorIndependent (without the option) that is the 'safe' version.

TODOs:

  • Implement parsing for this attribute.
  • Conditionally disable safety checks in the type checker.
  • Fix missing detection of mutable state access from @actorIndependent computed property, Reported as SR-13735, but already fixed in main.
  • Add @actorIndependent(unsafe) regression tests.
  • Add serialization / deserialization regression tests.

OnClass | OnFunc | OnVar | OnSubscript | ConcurrencyOnly |
ABIStableToAdd | ABIStableToRemove |
APIStableToAdd | APIBreakingToRemove,
104)
Copy link
Contributor

Choose a reason for hiding this comment

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

This all looks right.

@kavon kavon changed the title [concurrency] implement the @actorUnsafe attribute [concurrency] implement the 'unsafe' option for @actorIndependent Oct 13, 2020
Copy link
Contributor

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

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

Looking good

@kavon kavon marked this pull request as ready for review October 14, 2020 21:59
@kavon kavon requested a review from rjmccall October 15, 2020 16:03
Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

This is looking good! Just a few minor requests.

@DougGregor
Copy link
Member

The code's looking good. One last request (sorry): could you do a rebase to clean up the history a little bit?

@kavon kavon marked this pull request as draft October 15, 2020 23:46
@kavon
Copy link
Member Author

kavon commented Oct 16, 2020

Ok I cleaned it up into 3 commits. In future PR's I'll go with a rebasing strategy.

@kavon kavon marked this pull request as ready for review October 16, 2020 01:28
@DougGregor
Copy link
Member

@swift-ci smoke test

kavon added 3 commits October 19, 2020 15:20
[broken] first impl of @actorIndependent in the type checker.

[broken] fixed mistake in my parsing code wrt invalid source range

[broken] found another spot where ActorIndependent needs custom handling

[broken] incomplete set of @actorIndependent(unsafe) tests

updates to ActorIndependentUnsafe

[fixed] add FIXME plus simple handling of IndependentUnsafe context

finished @actorIndependent(unsafe) regression tests

added wip serialization / deserialization test

focus test to just one actor class

round-trip serialize/deserialize test for @actorIndependent

serialize -> deserialize -> serialize -> compare to original

most of doug's comments

addressed robert's comments

fix printing bug; add module printing to regression test

[nfc] update comment for ActorIsolation::IndependentUnsafe
@DougGregor
Copy link
Member

@swift-ci please smoke test

@DougGregor
Copy link
Member

Let's go ahead and merge, and we can tweak later if needed.

@DougGregor DougGregor merged commit ce90767 into swiftlang:main Oct 21, 2020
@kavon kavon deleted the actorUnsafe branch October 27, 2020 22:12
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.

5 participants