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);