From 1fb208a9913a196c94095b660c86237e2bde798d Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sat, 12 Sep 2026 15:59:35 +0700 Subject: [PATCH] The header is checked at build time, not only by a tool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reading the header produced declarations and nothing else, so the gap the whole thing exists to close — that nothing verifies a declaration against the library — was closed by a command somebody could run rather than by a property the build had. Now `import` runs both comparisons whenever a header resolves. Build-stopping, not a note. The package named the header, so the header is the package's own claim about what it binds; a defstruct that disagrees lays fields out in the wrong order and reads as five plausible numbers rather than as a link error. Continuing past a known-wrong layout to produce a program that will read garbage is the shape the house rule against swallowing things exists to prevent. Both messages point at the line in raylib.flan, not at the header. Verified by breaking it on purpose: a permuted Texture2D stops the build naming the field that moved, and `f64` where raylib says `float` stops it naming the parameter — which is the hazard BUILT.md calls out by name and says only a test can catch. A set-but-wrong FLAN_RAYLIB_H used to be indistinguishable from not opting in: the line was skipped and nothing was said. Unset still means off and silent; a path that is not there is now an error naming it. That is the difference between an opt-in and a trap. test/headers/sample.h is one function per decision the importer makes. The raylib case needs raylib installed, at the right version, with a variable set, so it would skip everywhere and cover nothing; this one does not move. It also found a bug, fixed next. Reach still prunes with 256 extra declarations in play: a wasm32-wasi build of a program that imports raylib and calls none of it links without libraylib, which is the case Reach.link exists for. --- lib/cimport.ml | 6 ++-- lib/load.ml | 82 +++++++++++++++++++++++++++++++++++++++++-- test/headers/sample.h | 48 +++++++++++++++++++++++++ 3 files changed, 132 insertions(+), 4 deletions(-) create mode 100644 test/headers/sample.h diff --git a/lib/cimport.ml b/lib/cimport.ml index a330cf2..058b77a 100644 --- a/lib/cimport.ml +++ b/lib/cimport.ml @@ -613,7 +613,7 @@ let check_structs ~env ~(structs : (string * Ast.field list) list) (d : dump) = if kebab cn <> f.Ast.fname then Some (Printf.sprintf - "the defstruct has %s where %s has %s — the field order disagrees" + "the defstruct has %s where %s has %s, so the field order disagrees" f.Ast.fname r.rname (kebab cn)) else match (try Some (value_ty env ct) with Refused _ -> None) with @@ -662,7 +662,9 @@ let dump_of_clang ~loc ~header ~flags = mtime, and the full flag list, because a flag changes what clang sees; plus a format version, because the cached value is a marshalled OCaml value and a compiler whose [dump] type has changed must not read one written by the old - one. *) + one. [Marshal] does not check that for you and a mismatch is a segfault + rather than an exception, so the discipline is: **change the [dump] type, + bump [cache_format] in the same commit.** Nothing enforces it. *) let cache_format = 1 diff --git a/lib/load.ml b/lib/load.ml index f92872f..fbb044c 100644 --- a/lib/load.ml +++ b/lib/load.ml @@ -566,7 +566,17 @@ let header_specs ~loc dir = let h = if Filename.is_relative h then Filename.concat dir h else h in - if optional && not (Sys.file_exists h) then None + (* An optional line that expanded to nothing at all is the + line being switched off, which is the whole point of the + marker. An optional line that expanded to a *path* is somebody + opting in, and a path that is not there is their typo — told + about by name, rather than silently behaving as though they had + not opted in at all. Those two are the difference between an + opt-in and a trap. *) + if optional && String.trim expanded = "" then None + else if not (Sys.file_exists h) then + fail loc + "%s names the header %s, and there is no such file" path h else Some (h, flags))) (read_lines path) @@ -679,10 +689,78 @@ let rec import ~seen ~loc alias dir = | _ -> None) ds in - let r, _, _ = + let r, dump, env = Cimport.header ~loc ~header:h ~flags ~known_structs ~known_enums ~taken ~bound_syms in + (* The point of reading the header, and the reason it is not + enough to generate declarations out of it. + + Everything the generator produces agrees with itself by + construction — the typedef and the Flan struct come from one + [defstruct], the prototype and the wrapper from one + declaration — so the only thing that can disagree is the + *library*, and until a header was read nothing here had a + second opinion to disagree with. Now it does, so it says so. + + Build-stopping, not a note. The package named this header, so + the header is the package's own claim about what it binds; a + [defstruct] that disagrees with it lays fields out in the + wrong order and reads as five plausible numbers rather than as + a link error, which is the failure BUILT.md says only a test + can catch. Continuing past a known-wrong layout to produce a + program that will read garbage is the shape the house rule + against swallowing things exists to prevent. + + A structure the header does not describe at all is not + checked and not complained about: a package may legitimately + describe something the header does not name. *) + let structs = + List.filter_map + (fun (d : Ast.decl) -> + match d.Ast.d with + | Ast.Defstruct (n, fs) -> Some (n, fs, d.Ast.dloc) + | _ -> None) + ds + in + List.iter + (fun (n, why) -> + let at = + List.find_map + (fun (m, _, l) -> if String.equal m n then Some l else None) + structs + in + fail (Option.value ~default:loc at) + "the defstruct %s disagrees with %s: %s" n h why) + (Cimport.check_structs ~env + ~structs:(List.map (fun (n, fs, _) -> (n, fs)) structs) dump); + (* And the hand-written bindings, against the header's own + signatures. These are the lines the importer deliberately + leaves alone, which is exactly why they are the ones nothing + else can check: a wrong declare-c is wrong in the generated + prototype too, so the two halves agree with each other and + only the library knows better. *) + let bound = + List.filter_map + (fun (d : Ast.decl) -> + match d.Ast.d with + | Ast.DeclareC (fn, sym) -> Some (fn, sym) + | _ -> None) + ds + in + List.iter + (fun (x : Cimport.sig_diff) -> + let at = + List.find_map + (fun ((fn : Ast.fn), sym) -> + if String.equal sym x.Cimport.dsym then Some fn.Ast.nloc + else None) + bound + in + fail (Option.value ~default:loc at) + "the declare-c of %s disagrees with %s: %s" + x.Cimport.dflan h x.Cimport.dwhy) + (Cimport.diff_bound ~env ~bound dump); r) (header_specs ~loc dir) in diff --git a/test/headers/sample.h b/test/headers/sample.h new file mode 100644 index 0000000..de56002 --- /dev/null +++ b/test/headers/sample.h @@ -0,0 +1,48 @@ +/* A small C header, for testing the importer against something that does not + * move. The raylib case needs raylib installed, needs the right version of it, + * and needs an environment variable set, so it is the wrong thing to hang the + * refusal catalogue on: it would skip everywhere and cover nothing. This + * header has one function per decision Cimport makes, and the test asserts on + * the reasons rather than on the count. + * + * Deliberately includes nothing. A header that pulls in stdio would make the + * dump thirty times larger and would put libc's declarations in the way of + * reading the test's. */ + +typedef struct Pair { float x; float y; } Pair; +typedef struct Shade { unsigned char r, g, b, a; } Shade; +typedef struct Undescribed { int a; int b; } Undescribed; + +/* A second typedef name for a record the package already describes under + * another one. raylib does this: struct Texture is Texture2D and also + * TextureCubemap. Both have to resolve to the one defstruct. */ +typedef struct Pair Point; + +typedef enum Mood { MOOD_CALM = 0, MOOD_CROSS = 1 } Mood; + +typedef void (*Notify)(void *user, unsigned int n); + +/* --- accepted --- */ +void set_seed(unsigned int seed); +int add_ints(int a, int b); +Pair make_pair(float x, float y); /* aggregate out, by out-pointer */ +float pair_len(Pair p); /* aggregate in, by pointer */ +Shade tint(Shade base, Shade over); +int name_length(const char *text); /* const char * is a string in */ +int count_at(const int *values, int n); /* T * is (Ptr T) */ +Pair point_of(Point p); /* the second typedef name */ +int mood_value(Mood m); /* a C enum is an int */ +void take_nothing(void); + +/* --- refused, one per reason --- */ +const char *name_of(int which); /* returns char * */ +void fill_buffer(char *out, int cap); /* non-const char *: C writes it */ +int printf_like(const char *fmt, ...); /* variadic */ +void on_event(Notify cb); /* a callback */ +long file_time(const char *path); /* long varies across our targets */ +Undescribed make_undescribed(void); /* no defstruct for it */ + +/* Two names that kebab to one, so the collision is refused by name rather than + * arriving at the checker as a duplicate declaration nobody wrote. */ +int Spin2D(int n); +int spin2d(int n);