search

parallelsystems / bit-struct

ergonomic bitfields in Rust
12 stars 3 forks source link

Remove transmute copy failures #9

Closed JarredAllen closed 1 year ago

JarredAllen commented 1 year ago

Background

4 outlines a potentially problematic use of core::mem::transmute_copy, violating the preconditions. In Rust 1.64.0, they added a check to the function that the preconditions are met, panicking if they aren't. This causes the code to panic whenever a bit struct is created, when compiled in Rust 1.64.0 (and, as I understand the code, any bit struct with multiple fields is guaranteed to panic).

Description

This PR removes those calls which violate the preconditions and panic on Rust 1.64.0. This required a fair amount of refactoring to the original codebase, hence the large number of lines changed (note that ~800 lines belong to a large, repetitive macro implementing a trait for all pairs of numeric types, so this isn't adding as much code as the line counts indicate).

While I was in here, I also added documentation on items which were missing it and did some other refactoring work, including moving some things around to be more clearly structured (which is why the number of lines changed by this PR is so high) and satisfying some clippy lints.

Also, this crate was using the derive feature on serde without including it, which causes problems when imported by a crate which isn't using the derive feature, so I specified it in the Cargo.toml. Note that this changes the compiler output for the integration tests that are expected to fail, as they are no longer complaining about serde not having derive macros.

Some of the changes I had to make were technically backwards-incompatible, so I bumped the minor version number. However, any sane use of this code should still work (and my codebase, which uses this crate fairly extensively and was experiencing the above-described error did not require any changes to be compatible with the changes to this crate), so this should be a simple upgrade. I also increased the minimum supported rust version to 1.62.1, because doing so was necessary to let some functions be const that could be const and bumping it while making a breaking change seemed like the right time to do so.

This closes #4. I did leave an invocation of transmute_copy that satisfies the preconditions in the code, but the unsafe invocations have been removed.

Verification

All of the tests still pass when I run locally on my machine (Rust 1.64). Also, I tried on Rust 1.62.1 (the new MSRV), and rust changed its compiler error messages so those tests don't match exactly, but the other tests all pass on the older version. Also, the formatting matches that produced by cargo fmt, and no errors are produced by clippy, doc, or udeps.

andrewgazelka commented 1 year ago

Do you think trying to have more DRY code here makes sense?

https://github.com/parallelsystems/bit-struct/pull/9/files#diff-ed12f5ea605f23bb4d26cc65778e3daff9db3eec40e2abfc82beafca130a99a0R741-R1596

afaik we shouldn't need to use proc macros for this, but I didn't look at your additions carefully, so maybe not.

andrewgazelka commented 1 year ago

Also thanks for fixing that issue! I do not know why it was fixed earlier. 😅

JarredAllen commented 1 year ago

Do you think trying to have more DRY code here makes sense?

https://github.com/parallelsystems/bit-struct/pull/9/files#diff-ed12f5ea605f23bb4d26cc65778e3daff9db3eec40e2abfc82beafca130a99a0R741-R1596

afaik we shouldn't need to use proc macros for this, but I didn't look at your additions carefully, so maybe not.

The link doesn't work for me (it just points to the entire diff), but if you're referring to the massive 800 line macro invocation at the end of src/types.rs, I think I could make it about half as long (still big, though) by making the macro implementation much more complex, but I didn't think it was worth the trade-off since the code is big, but pretty easy to understand what it does imo. I don't think declarative macros are powerful enough to provide anything smaller than that, though.

JarredAllen commented 1 year ago

Do you think trying to have more DRY code here makes sense? https://github.com/parallelsystems/bit-struct/pull/9/files#diff-ed12f5ea605f23bb4d26cc65778e3daff9db3eec40e2abfc82beafca130a99a0R741-R1596 afaik we shouldn't need to use proc macros for this, but I didn't look at your additions carefully, so maybe not.

The link doesn't work for me (it just points to the entire diff), but if you're referring to the massive 800 line macro invocation at the end of src/types.rs, I think I could make it about half as long (still big, though) by making the macro implementation much more complex, but I didn't think it was worth the trade-off since the code is big, but pretty easy to understand what it does imo. I don't think declarative macros are powerful enough to provide anything smaller than that, though.

Nevermind, I was cleverer this morning and cut it down to 64 lines without making the macro that much harder to understand.

andrewgazelka commented 1 year ago

Do you think trying to have more DRY code here makes sense? https://github.com/parallelsystems/bit-struct/pull/9/files#diff-ed12f5ea605f23bb4d26cc65778e3daff9db3eec40e2abfc82beafca130a99a0R741-R1596 afaik we shouldn't need to use proc macros for this, but I didn't look at your additions carefully, so maybe not.

The link doesn't work for me (it just points to the entire diff), but if you're referring to the massive 800 line macro invocation at the end of src/types.rs, I think I could make it about half as long (still big, though) by making the macro implementation much more complex, but I didn't think it was worth the trade-off since the code is big, but pretty easy to understand what it does imo. I don't think declarative macros are powerful enough to provide anything smaller than that, though.

Nevermind, I was cleverer this morning and cut it down to 64 lines without making the macro that much harder to understand.

Looks good! :)