Skip to content

Improve docs, no-panic feature, avoid panic in more functions #16

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 2 commits into from
Dec 24, 2022

Conversation

paulocsanz
Copy link
Owner

To run everything with no-panic use

cargo test --release --features=no-panic

A bunch of stuff will fail. Miri doesn't complain about UB though, which is great. I basically replaced clone_within with the internal implementation that stdlib uses without the assert (as we check them earlier). But replace_range is a super-set of every usage of it, so it would be safer and possibly not even slower to use replace_range for push, insert and remove also.

I also found a bug in try_insert_str where it didn't properly check for the invariants. And I removed utf8 conversion functions as the stdlib implementation always panics (uses Index trait) and we shouldn't copy them as they are super complex. So the caller can do their conversion the way they want.

@paulocsanz
Copy link
Owner Author

@Wicpar It's still ongoing and I may not have time so soon to do it, but I worry that we will conflict and I want us to get rid of most panics (or decide it's impossible and get rid of the constraint and accept that things technically might panic as we can't prove to the compiler nor we want to use unsafe because we might be wrong about the invariants).

@paulocsanz
Copy link
Owner Author

The problem seems that no_panic doesn't really work with doc tests apparently. dtolnay/no-panic#28

The actual way of running with no_panic now is cargo test --lib --tests --release --features=no-panic

The functions are not panicking anymore then. I will work on centralizing the unsafes.

@paulocsanz
Copy link
Owner Author

Ok now almost everything uses replace_range. There are only 2 unsafe usages in src/arraystring.rs, one in drain and the other in replace_range.

Things probably will be slightly slower as truncate functions now use the try_ functions and truncate, but it should be negligible, if we can test it in benchmarks (and decide that's not microbenchmarks being biased) we can change it. I've been thinking about a replace_range_truncate or at least a replace_range_unchecked to be used internally.

But I'm very happy removing most unsafes from the library, and being able to focus on two functions for safety. It's hard to write unsafe code, it's very easy to make it unsound without really understanding why, so I would rather go in the opposite direction from now.

Make everything as safe as possible and if we prove it's a bottleneck and there is a sound way of doing it with less compiler checks (unsafe code) we do it.

I might have undone some of your work, I'm open to changes, wasn't my intention.

The second commit in this PR specifically moves everything to use replace_range. The first one contains a lot of improvements but focusing on not panicking (mostly inlining stdlib functions) instead of being safer.

@paulocsanz
Copy link
Owner Author

I haven't documented the safety reasoning for the unsafes as they are complex and I'm tired, would love if you could do it, if not I can do it next week (although I'm not sure when).

After this, maybe some benchmarks (I removed them from README as they felt untrustworthy but I can add it back), and possibly it's ready? I'm open to any changes you suggest before it though.

I'm planning to setup a machine to run a fuzzer but Idk how long it will take before I learn how to do it and decide to do it.

@paulocsanz
Copy link
Owner Author

I will merge this as is, but you should check it out @Wicpar to see if we conflicted on changes, or if I undid some change that was important to you (for benchmark, safety or any other reasons).

And if there is something else to be done before releasing (I will take some time before, but reiterating, it seems that we are super close to it so it would be great to have your input before it)

@paulocsanz paulocsanz merged commit d73ddfe into main Dec 24, 2022
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.

1 participant