fix: remove unsafe code

[vojtechkral]

Change Summary

Going through the new code, found out unsafe is being used directly 😬 This is a proposal to remove the usafe usage in order to provide higher security and fault-tolerance guarantees. I don't believe any performance is meaningfully lost here. I believe the difference amounts to at most one bounds check in a non-hot path.

PR Checklist

• I have read and signed the Contributor License Agreement.

^ I will be sure to sign the CLA if the PR gets reviewed and will be on track for merging...

[RoDmitry]

But overall the unsafe was used correctly. It would panic in tests if used incorrectly. Why are you against it? Any reason for that?

[vojtechkral]

I think the question should be the other way around: Why use unsafe here in the first place? It's very unlikely there is any meaningful perf gain. In some places it even seems like only coding convenience. It's a needless hazard.

Sure maybe tests exercise the code enough today (I've not checked) but that code will be changed as time goes on, someone will add some subtle modification... this is the entire reason Rust exists :)

[RoDmitry]

but that code will be changed as time goes on, someone will add some subtle modification...

According to your line of thought: it won't be tested and it would be better to panic there in release. Not a lot better outcome tbh.

this is the entire reason Rust exists

Not for me. We all have different reasons I guess.

Why use unsafe here in the first place? It's very unlikely there is any meaningful perf gain.

Although true, not a performance bound peace of code. That's just how I code.

[vojtechkral]

and it would be better to panic there in release. Not a lot better outcome tbh.

Generally it is considered significantly better as it doesn't involve UB.

[RoDmitry]

My opinion is that it's just a less optimal code (not important but anyway), and it would be better to just cover it with tests.

Sure maybe tests exercise the code enough today (I've not checked)

Considering you have not even checked for tests, I guess you have made it in principle because you think that Rust should not use unsafe 🤷‍♂️ (which is a huge misbelief).

Generally it is considered significantly better as it doesn't involve UB.

UB is usually just a crash without any info. But panic contains info. That's all the difference. And we don't want any crash to happen. Your solution does not protect the program from crash (because it still can panic). For that we need tests. And that unsafe will panic in tests.
So, please, add tests if you worry about that piece of code.

TLDR: I'm just saying that this PR does not add anything useful, just regression in performance. Because by definition, that code should not crash (UB) or panic. And making it panic instead of crash (when it should not either) is a little bit useless. Just needs more tests.

[vojtechkral]

I guess you have made it in principle because you think that Rust should not use unsafe

Of course not, it wouldn't even be possible to write useful Rust programs without unsafe. And I have written unsafe code myself. However, the general principle is that unsafe should only be used when necessary, which I don't believe is the case here.

UB is usually just a crash without any info. But panic contains info. That's all the difference.

UB is UB, literally anything can happen, including arbitrary code being run if things go wrong enough. Panic is a well-defined sequence of events, and when occuring on a separate thread - such as a worker thread of a networking application - it can also be handled, the application may perform recovery or it may decide to shut down, but still be able to perform cleanups, cache flushes etc.

[vojtechkral]

Overall, I don't mean to argue the merits of unsafe or panics, my main objective here is to see whether this change can be accepted or not and what the overall stance of this project is towards usnafe. My interest are network services with a certain degree of security and fault-tolerance standards (in addition to performance).

Please let me know what the project intends to go for here, so that I can make the appropriate decision regaring usage of this library.

@RoDmitry are you the leader of the project and is this a decision towards keeping the unsafe? If that is the case feel free to close the PR.

Thank you.

[RoDmitry]

the general principle is that unsafe should only be used when necessary, which I don't believe is the case here.

That's wrong. The general principle is that unsafe can be used when there is no UB introduced. And it's allowed to use unsafe in optimizations. And this code contains no UB. If you want future-proof, then tests would protect you a lot more than removing unsafe.

is this a decision towards keeping the unsafe?

No. That's an attempt to make you understand that this PR literally does not change anything. It can be merged, if it makes you feel better. But it does not solve anything.

Here is just two lines of unsafe with no UB. And I guess you become sick from too much unsafe when you see C/C++ code 😅.
I want to write Rust code with the performance of C code. But people like you tell me that they don't want to see any unsafe in a code, even if it makes it work slower. That just makes me sad.

[vojtechkral]

@RoDmitry I have written a fair amount of C/C++ code in my career and I've been fine, thank you for the concern. I disagree tests provide better guarantee, as a lack of unsafe in a piece of Rust code provides a static-proof guarantee of no UB. I did not understand your remark about feeling sad, I don't believe any performance was lost.

Looking into the generated asm of get_next_node(), the bounds check doesn't get optimized out - is that enough to be sad? Don't be sad, there are other panicking checks emitted in that code as well, which I won't name for their protection 🙃

[RoDmitry]

the bounds check doesn't get optimized out - is that enough to be sad?

The overall preference of the imaginary safety over performance makes me sad.

Don't be sad, there are other panicking checks emitted in that code as well, which I won't name for their protection 🙃

But ideally they would also be optimized, which obviously you disagree.

I disagree tests provide better guarantee

Theoretically in the future someone will add some subtle modification... - that's your take, but aren't tests supposed to protect a code from wrong modifications? Why do you disagree about tests? Tell me more about it, I want to understand it better.

And if you give me an example of such a subtle modification which can't be seen in tests, I will not ask any more questions.

[vojtechkral]

The overall preference of the imaginary safety over performance makes me sad.

There is no such tradeoff going on here - there is no real performance to be gained by removing the bounds check. It's like two instructions in a function that doesn't even need optimising much in the first place. It's the performance gain that is imaginary here, not the safety.

And even if there was a perf gain, it would still be a valid question whether it's good enough to sacrifice Rust safety.

And if you give me an example of such a subtle modification which can't be seen in tests

Any example I can think of can be in principle tested by tests. The problem is someone has to write those tests correctly and reviewers need to correctly verify that all possible states are tested etc. In real life this tends to fail. CVE databases are full of memory access errors including from software that is very well tested. This is the reason Rust generally attempts to minimise the space for UB by proof.

This function on its own at this moment is probably fine. But if you feel the urge to even unwrap an Option unsafely then these will soon be all over the codebase and from there it's, statistically, just a matter of time until there's a CVE entry for this lib...

[RoDmitry]

if you feel the urge to even unwrap an Option unsafely

That was unnecessary, I agree 😅.

Any example I can think of can be in principle tested by tests. The problem is someone has to write those tests correctly

Maybe you can write tests then? It would add more protection from panics in release.
Or if you want, we can merge it as is. Is it ready for merge?

[vojtechkral]

Or if you want, we can merge it as is. Is it ready for merge?

Yes, that would be swell 🙏

[RoDmitry]

Omg, I have not published a review, and you did not see my comments 🤦‍♂️. Still getting used to a github review process.

[vojtechkral]

Thanks for review/merging. I hope it doesn't go too badly against your expectations. It makes my work easier 🙏

[RoDmitry]

Not a performance bound peace of code, so it's fine. I think there are other people who don't like unsafe, so they would be more calm using this library. Thank you.

[RoDmitry]

@vojtechkral hey, I've got a question: why hadn't you just use:

[profile.release.package.debug_unsafe]
debug-assertions = true

if you was looking for an extra safety?