From 4ff3e9a9223a77342d771b49bffde3985b15c60e Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sun, 13 Sep 2026 14:16:57 +0700 Subject: [PATCH] A finding about the bindings file is not a reason to stop a build check_constants makes two kinds of finding and they were treated alike. A value that does not match, or a C name the header does not have, is the library contradicting the package and stops a build the way a permuted defstruct does. An enum nobody mapped and a rule that reaches nothing are about the package's own bindings file -- real, and worth fixing, but telling a lane that added a defenum to go and edit a config in a message shaped like "your layout is wrong" is the wrong thing to fail a build with. Those gate generate-c, where that file is edited. Also: a const prefix now counts as reaching a name before an explicit constant line is consulted, so a rule whose every match is also spelled out by hand is not reported as matching nothing. --- BUILT.md | 9 +++++++ bin/main.ml | 24 ++++++++++++++----- lib/cimport.ml | 60 +++++++++++++++++++++++++++++++++-------------- lib/load.ml | 15 +++++++++--- test/test_flan.ml | 20 +++++++++++++++- 5 files changed, 100 insertions(+), 28 deletions(-) diff --git a/BUILT.md b/BUILT.md index 1b98fb9..347b1c2 100644 --- a/BUILT.md +++ b/BUILT.md @@ -392,6 +392,15 @@ next lane adds an enum, adds no line, and nothing notices. `enum Foo -` is how a package says out loud that the header has nothing to check `Foo` against — a sentence somebody wrote rather than a line nobody did. +**The two kinds of finding have different dispositions, which is the one thing +worth getting right here.** A value that does not match, or a C name the header +does not have, is the *library* contradicting the package — the same kind of +thing a permuted `defstruct` is, and an ordinary build stops on it. An enum +nobody mapped and a rule that reaches nothing are about the package's own +`bindings` file: real, worth fixing, and not a reason to fail somebody's build +with a message shaped like "your layout is wrong". Those gate `flan generate-c` +instead, which is where that file is edited and where the author is standing. + `defconst` is deliberately not held to that. A package's constants are mostly its own — raylib's 26 colours, an example's screen size — and demanding a line for each would be noise with no second author behind it. `gesture-all` is the diff --git a/bin/main.ml b/bin/main.ml index 25578fa..11496ac 100644 --- a/bin/main.ml +++ b/bin/main.ml @@ -299,7 +299,10 @@ let () = Printf.printf ";; every defenum member agrees with the header\n" | bad -> List.iter - (fun (n, why) -> Printf.printf ";; DISAGREES %s: %s\n" n why) + (fun (x : Flan.Cimport.const_diff) -> + Printf.printf ";; %s %s: %s\n" + (if x.Flan.Cimport.cmapping then "UNMAPPED" else "DISAGREES") + x.Flan.Cimport.cname x.Flan.Cimport.cwhy) bad)) (* Regeneration. [import-c] prints what it would produce; this writes it, and @@ -356,7 +359,10 @@ let () = x.Flan.Cimport.dflan x.Flan.Cimport.dsym x.Flan.Cimport.dwhy) r.Flan.Cimport.gsigs; List.iter - (fun (n, why) -> Printf.eprintf "DISAGREES %s: %s\n" n why) + (fun (x : Flan.Cimport.const_diff) -> + Printf.eprintf "%s %s: %s\n" + (if x.Flan.Cimport.cmapping then "UNMAPPED" else "DISAGREES") + x.Flan.Cimport.cname x.Flan.Cimport.cwhy) r.Flan.Cimport.gconsts; if r.Flan.Cimport.gwrote then Printf.printf @@ -367,14 +373,20 @@ let () = (List.length r.Flan.Cimport.ghidden) r.Flan.Cimport.gfns h else begin Printf.eprintf - "flan generate-c: %s disagrees with %s — %d struct layouts, %d \ - hand-written signatures and %d constants. Nothing was written: a \ + "flan generate-c: %s does not agree with %s — %d struct layouts, \ + %d hand-written signatures and %d constants, of which %d are a \ + mapping the package has not declared. Nothing was written: a \ generated file made against a header the library does not match \ - is the silent failure this check exists to prevent.\n" + is the silent failure this check exists to prevent, and a \ + constant nothing is mapped to is one nothing checks.\n" dir h (List.length r.Flan.Cimport.gstructs) (List.length r.Flan.Cimport.gsigs) - (List.length r.Flan.Cimport.gconsts); + (List.length r.Flan.Cimport.gconsts) + (List.length + (List.filter + (fun (x : Flan.Cimport.const_diff) -> x.Flan.Cimport.cmapping) + r.Flan.Cimport.gconsts)); exit 1 end) diff --git a/lib/cimport.ml b/lib/cimport.ml index 080340b..faee6fd 100644 --- a/lib/cimport.ml +++ b/lib/cimport.ml @@ -955,13 +955,33 @@ let check_structs ~env ~(structs : (string * Ast.field list) list) (d : dump) = [gesture-all] is the honest example of one that is not: 1023 is the OR of ten members and no C enumerator has that value to compare against. *) +(* Two kinds of finding, and they have different dispositions. + + [cmapping = false] is a disagreement with the *library*: a value that does + not match, or a C name the header does not have. That is the same kind of + thing a permuted [defstruct] is, and an ordinary build stops on it. + + [cmapping = true] is about the package's own [bindings] file — an enum + nobody mapped, a rule that reaches nothing. Real, and worth fixing, but it + is not the library contradicting anybody, and stopping a build over it + would tell a lane that added a [defenum] to go and edit a config file in a + message shaped like "your layout is wrong". Those gate [generate-c], which + is where the config is being edited and where the author is standing. *) +type const_diff = { cname : string; cwhy : string; cmapping : bool } + let check_constants ~config ~(enums : (string * (string * int64) list) list) - ~(consts : (string * Ast.expr) list) (d : dump) : (string * string) list = + ~(consts : (string * Ast.expr) list) (d : dump) : const_diff list = let found = Hashtbl.create 512 in List.iter (fun (n, v) -> Hashtbl.replace found n v) d.consts; let out = ref [] in - let say name fmt = Printf.ksprintf (fun m -> out := (name, m) :: !out) fmt in + let emit mapping name fmt = + Printf.ksprintf + (fun m -> out := { cname = name; cwhy = m; cmapping = mapping } :: !out) + fmt + in + let say name fmt = emit false name fmt in + let mapping_say name fmt = emit true name fmt in (* One Flan name, its value, and the C name it claims to be. *) let compare_one flan v cname = match Hashtbl.find_opt found cname with @@ -981,7 +1001,7 @@ let check_constants ~config (fun (ename, members) -> match List.assoc_opt ename config.enum_prefixes with | None -> - say ename + mapping_say ename "the defenum %s has no `enum` line in the package's `bindings`, so \ nothing checks its members against the header — add `enum %s \ `, or `enum %s -` to say the header has nothing to \ @@ -1007,18 +1027,22 @@ let check_constants ~config in List.iter (fun (n, e) -> + (* The prefix is marked as reaching something *before* an explicit + [constant] line is consulted, so a rule whose every match is also + spelled out by hand is not reported as reaching nothing. A false + finding in a check whose whole value is that a finding is real. *) + let by_rule = + List.find_map + (fun (fp, cp) -> + match strip_prefix fp n with + | Some rest -> + Hashtbl.replace used ("const:" ^ fp) (); + Some (cp ^ screaming rest) + | None -> None) + config.const_prefixes + in let cname = - match List.assoc_opt n explicit with - | Some c -> Some c - | None -> - List.find_map - (fun (fp, cp) -> - match strip_prefix fp n with - | Some rest -> - Hashtbl.replace used ("const:" ^ fp) (); - Some (cp ^ screaming rest) - | None -> None) - config.const_prefixes + match List.assoc_opt n explicit with Some c -> Some c | None -> by_rule in match cname with | None -> () @@ -1037,14 +1061,14 @@ let check_constants ~config List.iter (fun (ename, _) -> if not (Hashtbl.mem used ("enum:" ^ ename)) then - say ename + mapping_say ename "`enum %s` in the package's `bindings` names no defenum the \ package declares" ename) config.enum_prefixes; List.iter (fun (fp, _) -> if not (Hashtbl.mem used ("const:" ^ fp)) then - say fp + mapping_say fp "`const %s` in the package's `bindings` matches no defconst the \ package declares" fp) config.const_prefixes; @@ -1060,7 +1084,7 @@ let check_constants ~config enums in if not known then - say flan + mapping_say flan "`constant %s` in the package's `bindings` names no defconst and no \ enum member the package declares" flan) explicit; @@ -1391,7 +1415,7 @@ type regen = { ghidden : (string * string) list; gstructs : (string * string) list; gsigs : sig_diff list; - gconsts : (string * string) list; + gconsts : const_diff list; } let banner h = diff --git a/lib/load.ml b/lib/load.ml index 96af62c..3e110d5 100644 --- a/lib/load.ml +++ b/lib/load.ml @@ -876,9 +876,18 @@ let rec import ~seen ~open_ ~loc alias dir = ds in List.iter - (fun (n, why) -> - fail (Option.value ~default:loc (cloc n)) - "the package disagrees with %s: %s" h why) + (fun (x : Cimport.const_diff) -> + (* Only the findings that are the *library* contradicting the + package stop a build. A [defenum] nobody mapped and a rule + that reaches nothing are about the package's own + `bindings` file, and they gate `flan generate-c`, which is + where that file is edited — telling a lane that added an + enum to go and fix a config, in a message shaped like "the + layout is wrong", is the wrong thing to stop a build + with. *) + if not x.Cimport.cmapping then + fail (Option.value ~default:loc (cloc x.Cimport.cname)) + "the package disagrees with %s: %s" h x.Cimport.cwhy) (Cimport.check_constants ~config ~enums ~consts:pconsts dump); r) (header_specs ~loc dir) diff --git a/test/test_flan.ml b/test/test_flan.ml index 96a5b50..1cff6c9 100644 --- a/test/test_flan.ml +++ b/test/test_flan.ml @@ -1724,10 +1724,15 @@ let () = prefix rule cannot reach it. The narrow exception, said once. *) constants = [ ("Shading/half-dark", "SHADE_HALFDARK") ] } in - let constants ?(config = mapping) src = + let raw_constants ?(config = mapping) src = Cimport.check_constants ~config ~enums:(enums_of (program src)) ~consts:(pconsts_of (program src)) dump in + let constants ?(config = mapping) src = + List.map + (fun (x : Cimport.const_diff) -> (x.Cimport.cname, x.Cimport.cwhy)) + (raw_constants ~config src) + in check "constants that agree with the header are not reported" (constants (const_fixture ()) = []); (* The value is compared, which is the whole point: 340 is KEY_LEFT_SHIFT @@ -1822,6 +1827,19 @@ let () = | [ ("Mood/nope", why) ] -> contains why "names no defconst" | _ -> false); + (* The two kinds of finding are told apart, because they have different + dispositions: a value that disagrees with the library stops an ordinary + build, and an enum nobody wrote a line for is about the package's own + config and gates `generate-c` instead. *) + check "a value disagreement is not a mapping finding" + (match raw_constants (const_fixture ~fancy:"8" ()) with + | [ x ] -> not x.Cimport.cmapping + | _ -> false); + check "an unmapped defenum is a mapping finding" + (match raw_constants (const_fixture ~extra:"(defenum Nobody [a 0])\n" ()) with + | [ x ] -> x.Cimport.cmapping + | _ -> false); + (* The name rule, which is not an inverse of kebab and does not need to be: a constant has no declaration to store its C spelling in. *) List.iter