Miscellaneous fixes for Rust type-to-CFITSIO type mappings#442
Miscellaneous fixes for Rust type-to-CFITSIO type mappings#442zec wants to merge 7 commits intosimonrw:mainfrom
Conversation
This change fixes a bug found in use. Our team tried to write
header entries with 64-bit values on a 32-bit ARM system, but found that
the high-order 32 bits were stripped off in the resulting FITS file.
Investigation found that `fitsio`, when using write_key with a 64-bit
{signed, unsigned} integer, was using DataType::{TLONG, TULONG}.
This instructs CFITSIO to treat the passed-in value as a C
{long, unsigned long}.
On some systems, C long is 64 bits long; this is *not* the case
on 32-bit ARM however, where long is 32 bits, causing only 32 bits
of the value to be written.
This commit does not try to match Rust types to C types ABI-by-ABI, but
instead associates each relevant integer and floating-point type
with a value of DataType that is determined at compile-time to be an
equivalent C type.
This was found when running tests.
This lets us keep the MSRV the same, as core::ffi::* only became available in Rust 1.64.0.
This gives them the same ABI robustness as the WritesKey impls now.
📝 WalkthroughWalkthroughThis PR introduces a Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Detailed notes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 12: Replace the generic changelog line "* Miscellaneous ABI-related
fixes" with a specific, user-facing description of the actual fix and its impact
(for example: "Fix: 64-bit header key values no longer truncated on 32-bit
c_long targets, preventing incorrect header lookups and crashes"). Ensure the
new entry mentions the affected behavior and platforms/targets (e.g., 32-bit
`c_long` targets) and the user-visible outcome so readers can assess relevance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d1db00ef-9cb6-41cb-a51c-2d2f4c96ef14
📒 Files selected for processing (7)
CHANGELOG.mdfitsio/src/fitsfile.rsfitsio/src/headers/mod.rsfitsio/src/images.rsfitsio/src/longnam.rsfitsio/src/tables.rsfitsio/src/types.rs
|
I'm looking into why that test failed. Sadly, I have neither |
|
So, I've discovered a few things:
So, now I have a question: what is the proper way forward with this? Update the |
In the future, this should catch an issue like that found in PR simonrw#442 [1] sooner. [1] simonrw#442 (comment)
In the future, this should catch an issue like that found in PR simonrw#442 [1] sooner. [1] simonrw#442 (comment)
In the future, this should catch an issue like that found in PR simonrw#442 [1] sooner. [1] simonrw#442 (comment)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fitsio/src/macros.rs`:
- Around line 22-31: Replace the call to core::matches!(x, $pattern $(if
$guard)?) (which moves x) with a match on x that does not move it: do "match x {
$pattern $(if $guard)? => {}, _ => { panic!(..., core::stringify!($value), &x,
core::stringify!($pattern $(if $guard)?)) } }" so the non-matching arm borrows x
(use &x when formatting) and avoids consuming non-Copy types; update the macro
body to use that match instead of core::matches! while keeping the same panic
message via core::stringify!.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a9b733a9-cb55-45ca-9f37-0ebce8992484
📒 Files selected for processing (3)
CHANGELOG.mdfitsio/src/headers/mod.rsfitsio/src/macros.rs
| ($value:expr, $pattern:pat $(if $guard:expr)? $(,)?) => { | ||
| { | ||
| let x = $value; | ||
| if !core::matches!(x, $pattern $(if $guard)?) { | ||
| panic!( | ||
| "pattern-match assert failed\n value: ({}) == {:?}\n pattern: {}", | ||
| core::stringify!($value), | ||
| x, | ||
| core::stringify!($pattern $(if $guard)?), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In stable Rust, does core::matches!(expr, pattern)moveexpr for non-Copy values? Please cite official docs or macro expansion details.
💡 Result:
Yes, core::matches!(expr, pattern) moves expr for non-Copy values in stable Rust.
Citations:
- 1: http://doc.rust-lang.org/core/macro.matches.html
- 2: https://doc.rust-lang.org/std/macro.matches.html
- 3: https://doc.rust-lang.org/stable/reference/expressions/match-expr.html
- 4: https://docs.rs/assert_matches/latest/src/assert_matches/lib.rs.html
- 5: https://doc.rust-lang.org/1.75.0/std/macro.matches.html
- 6: https://rust.docs.kernel.org/core/macro.matches.html
🏁 Script executed:
cat -n fitsio/src/macros.rs | head -40Repository: simonrw/rust-fitsio
Length of output: 1482
🏁 Script executed:
rg "assert_matches1!" fitsio/Repository: simonrw/rust-fitsio
Length of output: 1896
🏁 Script executed:
rg "fn read_key" fitsio/src/ -A 5Repository: simonrw/rust-fitsio
Length of output: 2909
🏁 Script executed:
cd fitsio && cargo build --lib 2>&1 | head -50Repository: simonrw/rust-fitsio
Length of output: 624
🏁 Script executed:
cat > /tmp/test_copy.rs << 'EOF'
fn main() {
// Check if Result<i32, Error> is Copy-able
let result: Result<i32, String> = Ok(42);
let _ = core::matches!(result, Ok(_));
let _ = result; // Try to use after matches!
}
EOF
rustc --crate-type bin /tmp/test_copy.rs 2>&1Repository: simonrw/rust-fitsio
Length of output: 186
🏁 Script executed:
rg "type Result" fitsio/src/ -B 2 -A 2Repository: simonrw/rust-fitsio
Length of output: 312
🏁 Script executed:
rg "pub enum Error|pub struct Error" fitsio/src/errors.rs -A 10Repository: simonrw/rust-fitsio
Length of output: 280
🏁 Script executed:
cat fitsio/src/errors.rs | grep -A 15 "pub enum Error"Repository: simonrw/rust-fitsio
Length of output: 394
🏁 Script executed:
rg "pub struct FitsError|pub enum FitsError" fitsio/src/errors.rs -A 5Repository: simonrw/rust-fitsio
Length of output: 203
🏁 Script executed:
rg "mod test|#\[test\]|#\[cfg\(test\)\]" fitsio/src/headers/mod.rs | head -20Repository: simonrw/rust-fitsio
Length of output: 184
🏁 Script executed:
python3 << 'EOF'
# Analyze whether Result<String> is used with assert_matches1!
# If it is, the bug would manifest
import re
import subprocess
result = subprocess.run(['rg', 'assert_matches1!', 'fitsio/src/headers/mod.rs', '-A', '1'],
capture_output=True, text=True)
# Look for patterns that might use String types
lines = result.stdout.split('\n')
patterns_used = []
for i, line in enumerate(lines):
if 'assert_matches1!' in line:
patterns_used.append(line.strip())
print("Patterns matched by assert_matches1!:")
for p in patterns_used:
print(f" {p}")
# Check if any use Result<String>
string_results = [p for p in patterns_used if 'String' in p]
print(f"\nUsages with Result<String>: {len(string_results)}")
# The actual values being matched - all appear to be Ok(literal) with numeric types
print("\nAll usages involve Result with numeric (Copy) types.")
print("However, the macro has a latent bug for non-Copy types like Result<String>.")
EOFRepository: simonrw/rust-fitsio
Length of output: 1447
Rewrite macro to avoid moving the matched value before formatting it.
The core::matches! macro moves its argument for non-Copy types. At line 25, this consumes x for Result<i32, Error> (since Error is not Copy); attempting to use x again at line 29 in the panic! macro will fail compilation. Replace with a direct match expression that borrows in the non-matching arm:
Proposed fix
macro_rules! assert_matches1 {
($value:expr, $pattern:pat $(if $guard:expr)? $(,)?) => {
- {
- let x = $value;
- if !core::matches!(x, $pattern $(if $guard)?) {
- panic!(
- "pattern-match assert failed\n value: ({}) == {:?}\n pattern: {}",
- core::stringify!($value),
- x,
- core::stringify!($pattern $(if $guard)?),
- );
- }
- }
+ {
+ match $value {
+ $pattern $(if $guard)? => {}
+ ref x => {
+ panic!(
+ "pattern-match assert failed\n value: ({}) == {:?}\n pattern: {}",
+ core::stringify!($value),
+ x,
+ core::stringify!($pattern $(if $guard)?),
+ );
+ }
+ }
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ($value:expr, $pattern:pat $(if $guard:expr)? $(,)?) => { | |
| { | |
| let x = $value; | |
| if !core::matches!(x, $pattern $(if $guard)?) { | |
| panic!( | |
| "pattern-match assert failed\n value: ({}) == {:?}\n pattern: {}", | |
| core::stringify!($value), | |
| x, | |
| core::stringify!($pattern $(if $guard)?), | |
| ); | |
| ($value:expr, $pattern:pat $(if $guard:expr)? $(,)?) => { | |
| { | |
| match $value { | |
| $pattern $(if $guard)? => {} | |
| ref x => { | |
| panic!( | |
| "pattern-match assert failed\n value: ({}) == {:?}\n pattern: {}", | |
| core::stringify!($value), | |
| x, | |
| core::stringify!($pattern $(if $guard)?), | |
| ); | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fitsio/src/macros.rs` around lines 22 - 31, Replace the call to
core::matches!(x, $pattern $(if $guard)?) (which moves x) with a match on x that
does not move it: do "match x { $pattern $(if $guard)? => {}, _ => { panic!(...,
core::stringify!($value), &x, core::stringify!($pattern $(if $guard)?)) } }" so
the non-matching arm borrows x (use &x when formatting) and avoids consuming
non-Copy types; update the macro body to use that match instead of
core::matches! while keeping the same panic message via core::stringify!.
|
Re CodeRabbit's "Rewrite macro to avoid moving the matched value before formatting it.": CodeRabbit is incorrect. |
|
@simonrw, any updates on this pull request? Anything I should be doing at this point? |
|
Hi @zec thanks for looking into this. I'm pleased your team are using this project. I don't have much time to maintain this codebase any more even though I'd like to. And I have not been working in the field for quite a few years now. How would you feel about becoming a co-maintainer of this project? This would mean you would not be blocked on review from me, and you could implement the changes you need |
Provided the workload isn't too high, I'd be willing do that. I would want at least occasional guidance from you as to code organization, review and QA philosophy, etc. |
3 participants
This pull request fixes a bug found in use, as well as several others that are similar. Our team tried to write header entries with 64-bit values on a 32-bit ARM system, but found that the high-order 32 bits were stripped off in the resulting FITS file.
Investigation found that
fitsio, when using write_key with a 64-bit {signed, unsigned} integer, was usingDataType::{TLONG, TULONG}. This instructs CFITSIO to treat the passed-in value as a C {long,unsigned long}.On some systems, C
longis 64 bits long; this is not the case on 32-bit ARM however, wherelongis 32 bits, causing only 32 bits of the value to be written.This pull request does not try to manually match Rust types to C types ABI-by-ABI, but instead associates each relevant integer and floating-point type with a value of
DataTypethat is determined at compile-time to be an equivalent C type; the newHasFitsDataTypetrait is used to store the results.I do not claim to have found all possible mismatches of this type, but I think I found all involving
DataType.