Cleaner API

[amodm]

This PR introduces a cleaner memory-safe API via the following changes:

• Swift side function signature does not need UnsafePointer anymore for its arguments. So now it's func getGreeting(name: SRString)
• Rust side now has a .to_swift() function enabled on SR* types, which returns a SwiftRef<Type> (e.g. SwiftRef<SRString>. So long as we pass this from the Rust side, we don't need UnsafePointer on the Swift side. So now, the Rust side looks like unsafe { get_greeting(srstring.to_swift()) }
• Swift side now has a .toRust() function on NSObject via an extension, so that we can do a return myString.toRust() instead of return toRust(myString).

The earlier approach still exists, for those who prefer that:

• Rust side function signature: unsafe fn get_greeting(name: &SRString)
• Swift side function signature: func getGreeting(name: UnsafePointer<SRString>)

This PR also fixes a bunch of fmt and clippy errors. IMHO, it would be a good practice to make sure that anything going into the main branch is naturally clean of these.

[Brendonovich]

I love the toRust change, but I've refactored how SwiftRef works so that there's no transmute_copy or unsafe. From what I understand, your goal with SwiftRef was to have a copy of the NonNull held by each SRObject. Turns out that NonNull is Copy, which allows for SRObject to be copied and held by SwiftRef, preserving memory layout since it's repr(transparent) all the way down. I've also moved to_swift (now swift_ref) to the types themselves so that a trait doesn't have to be imported.

[amodm]

Please hold on merging this change. I'll need to verify the memory safety of this. I know the tests have passed, but it shouldn't, with these changes. I'll revert shortly.

[amodm]

I've fixed the tests (there was a bug in them), and now they fail.

The reason is that you're using .clone(), which creates a completely new SRObject, leading to its own drop() whenever a SwiftRef expires. But the underlying object behind all of those clones is the same, so this leads to a memory imbalance, and crash.

That's the reason why I had to use transmute, so that no objects get created. That's also the reason why I deliberately marked to_swift() as unsafe, because the caller needs to be aware that through this function, they can technically retain a copy of references to swift object, even when it has been dropped. Rust demands that such functions be marked unsafe.

[amodm]

You can also work around this issue by taking a corresponding retainedValue on the swift side, for every rust side .clone(), but the question is, would it be worth it?

[Brendonovich]

Huh, that's a really good catch. Can't it be fixed by making SwiftRef only hold a NonNull rather than the whole SRObject like you did before? The tests pass fine on my end with that change.

[amodm]

Yes, that should work as semantically that's identical to the original PR. You'll still need to mark all SR**.swift_ref() functions as unsafe, as technically they can be used to retain references to invalid objects.

Personally, I feel that the above change will take away from the core of what you were trying to achieve (no-unsafe code), with more number of unsafe than before, and more LoC. But still, your discretion on whether to stick with original PR, or make the changes above.

Separately, please do also consider that to_swift() and toRust() have a stronger correspondence (naming convention wise), than .swift_ref() and .toRust()

[Brendonovich]

I'd rather use swift_ref than to_swift since swift_ref directly produces a SwiftRef, whereas toRust just returns the regular data with a side effect. It's not a huge deal anyway, since I'm going to make a macro that conceals the need to use SwiftRef and swift_ref anyway.

Seeing as accessing data via a NonNull is already unsafe, is it really worth making swift_ref unsafe? Especially when it doesn't actually involve any unsafe code.

[amodm]

AFAICT, because swift_ref allows one to retain copies independently of the lifecycle of the object, technically Rust's safety rules require you to expose it as unsafe, even if hidden behind a macro.

Alternatively, you'll need to bind SwiftRef<'a> to the lifetime of the underlying object. In that case, you can avoid the unsafe. I'm guessing that should work from a memory-safety pov.

[Brendonovich]

Seeing as I'm going to macro this all anyway, I'm gonna do the SwiftRef<'a> approach which will require unsafe. Doesn't really matter if the generics are complex if the end user doesn't need to use them.

[Brendonovich]

Hmm, I think this has been flawed from the start - specifically the use case with test_reflection. Both dd9eaf6 with the $op(); addition and the most recent commit fail to run sometimes because of that test. I'm not quite sure how we should handle it or how the reference counting operates in that case.

[amodm]

Reflection isn't difficult to fix here. Defining the signature something along the lines of

extern "C" {
    fn reflect_string(string: &SRString) -> &SRString;
}

should do it, along with any related ARC issues and with the appropriate changes on the rust side.

But the amount of changes you've made to this PR is already overwhelming, with very little of the original left. I'm spending far more time assisting you with your changes, than the original PR, and that's neither a strong motivation to me, nor IMHO a fair burden to place on a contributor.

It would've been simply easier to just reject the PR, and do the changes you wanted in your own branch. Which is why I'm closing this PR. I'll retain your changes on my fork for another 24 hrs, just in case you don't have a local copy of it for any reason.

[amodm]

There were other issues in this repo I would've liked to contribute to, but clearly you have strong opinions (rightly so, as the owner) about how the code should look like, and I feel it would be easier for those ideas to show up in code before contributors like me barge in in with their own implementations and then face the burden of having to retro-fit the ideas you have for the repo.

A Contribution Guide section in README might be helpful to articulate what you're looking for as well.

[amodm]

As I won't be contributing anymore to this repo, just leaving my findings about the build being stuck on the Post Checkout step. It seems to be a bug in Runners code. The leaks command, when run with sudo fixes the issue on github. I've verified that it works, and should help the build complete, instead of being stuck.

Hopefully all of the above helps you make this a better repo. My best wishes 👍🏼

[Brendonovich]

Damn, guess I fucked this one up. I'm really sorry I made you feel that way, I was under the impression we were making good progress but now it's evident that you didn't like this back-and-forth.

Your help over the past couple of weeks has been extremely valuable, and I'm not sure swift-rs would have ever been more developed without your knowledge of Swift. I wish you'd have let me know this was overwhelming earlier so that I could have changed my actions.

If I wanted to reject the PR then I would have, but you proposed some valuable ideas that I wanted you to get the attribution for, instead of me just putting them on my own branch. I probably could have added you as a co-author on my own commits, but that didn't cross my mind.

I'd be very grateful if you changed your mind, but if not then I'll take your changes to a new branch and co-author you, seeing as you don't want this PR merged. I'll add a Contribution Guide as you suggested - I usually require people to open an issue before submitting a PR, but this repo was created before I learned how to properly handle open source and I never got around to updating it.

I hope we can come to an understanding, but regardless I wish you well.

[amodm]

Your clarification is deeply appreciated, thanks. It isn't the back & forth that was the problem, but the scope of the PR. I was specifically targeting doing away with UnsafePointer, since my memory-safety PR had introduced it earlier, and I felt a sense of responsibility to try to minimise its side-effect.

Your changes OTOH were coming from a pov of the full-fledged API you want to see, which is fine by itself, but is of a larger scope, which I unwittingly got pulled into, and ended up spending more time than I wanted, on reviewing the safety aspects of a very different codebase. Something that should've been additive, became substitutive.

I'm sincere in my suggestion that it'd be easier on contributors once you've gotten the skeletal structure in place, as that would relieve them of the burden of retro-fitting new ideas in their PRs at a late stage.

I'll be sticking with my decision for the time being, but I greatly appreciate your closing words here. I'm not bummed about our interaction, just need to stay focussed on my project.