Skip to content

Commit 42aed47

Browse files
digama0claude
andcommitted
mm0b_parser: pin the fixture's index format, and say why AlignFile is align(8)
The two fixes these tests were written alongside are already here: `e820dc3f` made `AlignFile` `align(8)` and regenerated `test_resources/peano.mmb` as part of the CI work, and the fixture bytes are identical to the ones regenerated on `mmb-explorer`. What did not come with them is the coverage and the reasoning, picked up here from that branch (`65933edd`, `00d19428`). `peano_index` parses the fixture *with* its index and checks that names resolve. `peano0` parses it as a `BareMmbFile`, which ignores the index entirely, so it kept passing while the fixture's index went a format version behind — which is how the fixture came to be stale unnoticed. A test that never looks at the index cannot report one that has rotted. The `AlignFile` doc comment records why it must be `align(8)` and not `align(1)`: `repr(align(N))` only ever raises a type's alignment, so `align(1)` on a `[u8; N]` is a no-op and the array lands wherever the compiler puts a 1-aligned local, making the parse fail with `Unaligned` depending on nothing but stack layout. `MmbFile::parse` now states the requirement from the other side, since it is the caller who has to satisfy it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 5c2af3e commit 42aed47

2 files changed

Lines changed: 42 additions & 4 deletions

File tree

mm0-rs/components/mm0b_parser/src/parser.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -882,6 +882,11 @@ impl<'a, X: MmbIndexBuilder<'a>> MmbFile<'a, X> {
882882
/// Parse a [`MmbFile`] from a file, provided as a byte slice.
883883
/// This does the minimum checking to construct the parsed object,
884884
/// it is not a verifier.
885+
///
886+
/// `buf` must be **8-byte aligned**, since [`Header`] is `align(8)`; otherwise
887+
/// this returns [`ParseError::Unaligned`]. A `Vec<u8>` from `fs::read` is
888+
/// suitably aligned in practice, but a subslice at an arbitrary offset, or a
889+
/// short array literal, may not be — wrap such data in an `align(8)` struct.
885890
pub fn parse(buf: &'a [u8]) -> Result<Self, ParseError> {
886891
use ParseError::{BadIndexParse, BadSorts, BadTerms, BadThms};
887892
let (zc_header, sorts) =

mm0-rs/components/mm0b_parser/tests/basic.rs

Lines changed: 37 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,14 @@
1-
use mm0b_parser::{BareMmbFile, ParseError};
1+
use mm0b_parser::{BareMmbFile, BasicMmbFile, NumdStmtCmd, ParseError};
22
use std::fs::OpenOptions;
33
use std::io::Read;
44
use std::path::PathBuf;
55

6+
/// Force the test data to the 8-byte alignment [`BareMmbFile::parse`] requires.
7+
///
8+
/// This must be `align(8)`, not `align(1)`: `repr(align(N))` can only *raise* a
9+
/// type's alignment, so `align(1)` is a no-op on a `[u8; N]` and leaves the array
10+
/// wherever the compiler happens to put a 1-aligned local — making the parse fail
11+
/// with [`ParseError::Unaligned`] depending only on stack layout.
612
#[repr(align(8))]
713
struct AlignFile<T>(T);
814

@@ -16,8 +22,7 @@ fn try_next_decl_infinite_loop() {
1622
assert!(matches!(iter.next().unwrap().unwrap_err(), ParseError::BadProofLen(40)));
1723
}
1824

19-
#[test]
20-
fn peano0() {
25+
fn peano_bytes() -> Vec<u8> {
2126
let mut mmb_bytes = Vec::new();
2227
let mut mmb_file = OpenOptions::new()
2328
.read(true)
@@ -26,5 +31,33 @@ fn peano0() {
2631
.unwrap();
2732
mmb_file.read_to_end(&mut mmb_bytes).unwrap();
2833
assert!(!mmb_bytes.is_empty());
29-
assert!(BareMmbFile::parse(mmb_bytes.as_slice()).is_ok());
34+
mmb_bytes
35+
}
36+
37+
#[test]
38+
fn peano0() {
39+
assert!(BareMmbFile::parse(peano_bytes().as_slice()).is_ok());
40+
}
41+
42+
/// Parse the fixture *with* its index, and check that names resolve.
43+
///
44+
/// `peano0` only parses as a [`BareMmbFile`], which ignores the index entirely,
45+
/// so it keeps passing even if the fixture's index is stale or malformed — which
46+
/// is exactly how the fixture came to be a format version behind. This test pins
47+
/// the index format down.
48+
#[test]
49+
fn peano_index() {
50+
let bytes = peano_bytes();
51+
let file = BasicMmbFile::parse(bytes.as_slice()).expect("fixture index should parse");
52+
let names = file
53+
.proof()
54+
.take(4)
55+
.map(|decl| match decl.unwrap().0 {
56+
NumdStmtCmd::Sort { sort_id } => file.sort_name(sort_id).into_owned(),
57+
NumdStmtCmd::TermDef { term_id, .. } => file.term_name(term_id).into_owned(),
58+
NumdStmtCmd::Axiom { thm_id } | NumdStmtCmd::Thm { thm_id, .. } =>
59+
file.thm_name(thm_id).into_owned(),
60+
})
61+
.collect::<Vec<_>>();
62+
assert_eq!(names, ["wff", "im", "not", "ax_1"]);
3063
}

0 commit comments

Comments
 (0)