[MOD-10008] Reduce the amount of unsafe code by using more idiomatic Rust definitions by LukeMathWalker · Pull Request #6257 · RediSearch/RediSearch

LukeMathWalker · 2025-06-04T15:03:25Z

Describe the changes in the pull request

Correctness is our paramount concern when manipulating C types in Rust.
When defining idiomatic Rust wrappers, we are caught between a rock and a hard place:

If we create thin new types around the generated bindings (in the ffi crate) we get layout correctness for free. If the C code changes, the bindings will change and any discrepancy will cause a compilation error in Rust. At the same time, we're working with raw pointers everywhere, thus forcing us to use a lot of unsafe code where mistakes can be made more easily.
If we use a "Rust-native" definition, we reduce the amount of unsafe code but we risk layout drift.

This PR tries to strike a balance for the buffer.* module.
We stick to wrapping the ffi type for Buffer, but we use more idiomatic Rust types for reader and writer. We add compile-time assertions to guard against layout drift.

Mark if applicable

This PR introduces API changes
This PR introduces serialization changes

zeenix

it would have been better to do the splitting & merging of files in separate commit(s) so it's easier to see the main change for reviewers.

LGTM otherwise.

zeenix · 2025-06-04T15:14:46Z

src/redisearch_rs/buffer/src/reader.rs

+}
+
+// Check, at compile-time, that `BufferReader` and `ffi::BufferReader` have the same representation.
+const _: () = {


why not an ignored test instead?

I didn't think I could have a const fn as a test, but today I learned it's possible.
Done in afe41ac

That's not what I meant. :) I didn't know that either.

I was talking about:

#[test] #[ignore]

which tests the code to compile but not actually run.

While that would be technically correct (i.e. the current test desugars to a no-op at runtime), I think it'd be confusing to see that #[ignore] for folks who are not too well-versed into the semantics of const evaluation.
Since the penalty is negligible, I prefer to leave it as is.

I'm just wondering if declaring the test fn as const is doing what you are thinking. Did you find any docs on this. I couldn't. I'd be fine with just turning it back to const _: () = that you originally did.

It seems to, but then it only checks it if tests are built. So yeah, probably better to revert back. Done in 5e61a40

chesedo

nit: small comment improvement

src/redisearch_rs/buffer/src/writer.rs

src/redisearch_rs/buffer/src/reader.rs

codecov · 2025-06-04T15:58:34Z

Codecov Report

Attention: Patch coverage is 89.55224% with 7 lines in your changes missing coverage. Please review.

Project coverage is 88.79%. Comparing base (242a33d) to head (c9d1afa).
Report is 22 commits behind head on master.

Files with missing lines	Patch %	Lines
src/redisearch_rs/buffer/src/reader.rs	84.00%	3 Missing and 1 partial ⚠️
src/redisearch_rs/buffer/src/writer.rs	90.32%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6257      +/-   ##
==========================================
- Coverage   88.87%   88.79%   -0.08%     
==========================================
  Files         239      241       +2     
  Lines       40258    40623     +365     
  Branches     3165     3265     +100     
==========================================
+ Hits        35780    36073     +293     
- Misses       4443     4513      +70     
- Partials       35       37       +2

Flag	Coverage Δ
flow	`82.16% <ø> (-0.71%)`	⬇️
unit	`46.12% <89.55%> (+0.32%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

kei-nan · 2025-06-05T12:17:08Z

src/redisearch_rs/buffer/src/reader.rs

+    const POSITION_OFFSET_MATCHES: bool =
+        offset_of!(BufferReader<'static>, position) == offset_of!(ffi::BufferReader, pos);
+
+    // Conditional compilation failure on mismatch


What should be done if the panic occurs?
What steps should we do?
Revert this PR?

Maybe add a comment with some directions.

Done in 1eab83f

…ions in buffer

Co-authored-by: Pieter <[email protected]>

LukeMathWalker · 2025-06-16T10:33:07Z

I've rebased on the latest master. Can you give it a final look @kei-nan?

github-actions bot added the size:L label Jun 4, 2025

LukeMathWalker force-pushed the buffer-writer-bytemuck branch 3 times, most recently from ebebf31 to 9f863c4 Compare June 4, 2025 15:06

LukeMathWalker requested a review from zeenix June 4, 2025 15:06

LukeMathWalker force-pushed the buffer-writer-bytemuck branch from 9f863c4 to 46cab95 Compare June 4, 2025 15:07

LukeMathWalker requested a review from chesedo June 4, 2025 15:07

LukeMathWalker force-pushed the buffer-writer-bytemuck branch from 46cab95 to e7033d4 Compare June 4, 2025 15:15

zeenix previously approved these changes Jun 4, 2025

View reviewed changes

chesedo reviewed Jun 4, 2025

View reviewed changes

src/redisearch_rs/buffer/src/writer.rs Outdated Show resolved Hide resolved

src/redisearch_rs/buffer/src/reader.rs Outdated Show resolved Hide resolved

LukeMathWalker dismissed zeenix’s stale review via afe41ac June 4, 2025 15:28

chesedo previously approved these changes Jun 5, 2025

View reviewed changes

LukeMathWalker dismissed chesedo’s stale review via 66a3a53 June 5, 2025 07:42

LukeMathWalker changed the title ~~Reduce the amount of unsafe code by using more idiomatic Rust definitions~~ [MOD-10008] Reduce the amount of unsafe code by using more idiomatic Rust definitions Jun 5, 2025

LukeMathWalker requested a review from kei-nan June 5, 2025 11:45

kei-nan reviewed Jun 5, 2025

View reviewed changes

LukeMathWalker requested a review from kei-nan June 16, 2025 08:05

LukeMathWalker and others added 8 commits June 16, 2025 12:32

Reduce the amount of unsafe code by using more idiomatic Rust definit…

af5332b

…ions in buffer

Use a test rather than a const block

ab626ac

Update src/redisearch_rs/buffer/src/writer.rs

1a8a34b

Co-authored-by: Pieter <[email protected]>

Update src/redisearch_rs/buffer/src/reader.rs

b38ab72

Co-authored-by: Pieter <[email protected]>

Improve test coverage

7bdb6cd

Ignore miri on panicking tests

41143df

Back to const blocks

05cd6cb

Explain what the check is meant to be used for

c9d1afa

LukeMathWalker force-pushed the buffer-writer-bytemuck branch from 1eab83f to c9d1afa Compare June 16, 2025 10:32

kei-nan approved these changes Jun 16, 2025

View reviewed changes

LukeMathWalker added this pull request to the merge queue Jun 16, 2025

Merged via the queue into master with commit 0cc4d7b Jun 16, 2025
15 checks passed

LukeMathWalker deleted the buffer-writer-bytemuck branch June 16, 2025 14:57

LukeMathWalker restored the buffer-writer-bytemuck branch February 6, 2026 09:22

Conversation

LukeMathWalker commented Jun 4, 2025

Describe the changes in the pull request

Mark if applicable

Uh oh!

zeenix left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chesedo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LukeMathWalker commented Jun 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Jun 4, 2025 •

edited

Loading