Skip to content

Add Support for Threadsafe and NotThreadSafe annotations #188

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
Nov 26, 2024

Conversation

jrosen081
Copy link
Contributor

Closes #129

This adds support for threadsafe, immutable, and notthreadsafe annotations. I was able to test this locally making jars, which I can attach if needed, but couldn't do any automated testing due to the fact that these types are not built in to Java, so I couldn't use the normal translator tests. I'm open to hearing if there are any good ways of doing automated testing for this (we could possibly build a jar in CI with the given dependencies and use that, but it's probably not great to be doing that during unit tests, especially since I think we will need to download dependencies if we want to use the real annotations in the ticket)

@ktoso
Copy link
Collaborator

ktoso commented Nov 26, 2024

In a Java test modules you could literarily define an annotation with those names - it doesn't have to be the "real jar". Do you want to give that a short or should I try to streamline it a bit more? Idk the exact steps without looking into it, let me know if you'd need a hand here.

Alternatively you could do this on one of the sample apps -- we CI verify them all, so declare the annotation there and see that some type indeed got the Sendable conformance by assigning it to some var: x: Sendable etc?

@jrosen081
Copy link
Contributor Author

In a Java test modules you could literarily define an annotation with those names - it doesn't have to be the "real jar". Do you want to give that a short or should I try to streamline it a bit more? Idk the exact steps without looking into it, let me know if you'd need a hand here.

I'll look into it later this week and let you know if I can figure out the automated test way.

Alternatively you could do this on one of the sample apps -- we CI verify them all, so declare the annotation there and see that some type indeed got the Sendable conformance by assigning it to some var: x: Sendable etc?

Oh didn't know that this was done in CI. If I can't get the first part right, I can do this part

@ktoso
Copy link
Collaborator

ktoso commented Nov 26, 2024

We actually rely on the samples a lot for correctness so yeah, that'd definitely be a viable way to verify things as well :)

@jrosen081 jrosen081 force-pushed the jrosen/swift-thread-safe branch from cfef570 to fe47e58 Compare November 26, 2024 01:59
@@ -0,0 +1,6 @@
package com.example.swift;
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing license header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops updated

Copy link
Collaborator

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for including the snippet

@jrosen081
Copy link
Contributor Author

We actually rely on the samples a lot for correctness so yeah, that'd definitely be a viable way to verify things as well :)

I updated one of the Samples to check for ThreadSafe. This still has the hole where there isn't a way to test something is not Sendable (which NotThreadSafe does). I can look later in the week for other testing of not sendable depending on what you think

@ktoso
Copy link
Collaborator

ktoso commented Nov 26, 2024

That's okey, thanks :)

@ktoso ktoso merged commit c769b1a into swiftlang:main Nov 26, 2024
12 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.

Consider @ThreadSafe when importing Java types
2 participants