fix: pr comments

README.md

@@ -160,29 +160,17 @@ header shape from the library's ABI format. It carries two honest limits today:
 - **Events are CBOR-only.** Applying `abi = c` to an `{.ffiEvent.}` proc is a
   hard compile error; declare events with `abi = cbor` (they ride CBOR
   internally regardless of the library default).
-- **All-scalar `abi = c` procs are dropped from the foreign bindings.** A
-  `{.ffi: "abi = c".}` method whose every param and return is a plain scalar
-  takes the CBOR-free scalar fast path at runtime, but the foreign codegen for
-  that inline-args shape is a follow-up (tracked in #120) — such procs are
-  omitted from the generated `.h`. Give a proc at least one non-scalar
-  (struct / `seq` / `Option`) param or return, or use `abi = cbor`, if you need
-  it in the bindings.
-
-An `abi = c` proc whose whole signature is scalar — fixed-width integer, float,
-or bool params (a `string` return is fine, a `string` param is not) and no
-structs, handles, or pointers — dispatches through a CBOR-free scalar fast path.
-The `abi = c` C header (an `abi = c` library generated with `-d:targetLang=c`)
-emits real bindings for that shape: the wrapper passes the scalar args inline
-(no request struct) and adapts the raw-bytes reply into the same typed callback
-surface the flat-struct methods use. The CBOR-speaking outputs — the CBOR C
-header and the `cpp`, `rust`, `cddl` targets — have no scalar codegen, so under
-`-d:ffiGenBindings` they would omit such a proc from the generated bindings —
-and `genBindings()` fails with an error naming the affected procs. Resolve it by
-making the whole library `abi = c` and generating C bindings, switching the proc
-to `abi = cbor`, adding a non-scalar param so it takes the CBOR wire shape, or
-passing `-d:ffiAllowScalarSkip` to accept the omission (the proc still works
-over the scalar fast path; it's just absent from the generated foreign
-bindings).
+- **All-scalar `abi = c` procs bind only in the `abi = c` C header.** A
+  `{.ffi: "abi = c".}` method whose params and return are all scalars — ints,
+  floats, bools; a `string` return is fine, a `string` param is not — takes a
+  CBOR-free fast path, and its C wrapper passes the args inline instead of
+  packing a request struct. Only the `abi = c` C header emits that shape. The
+  CBOR C header and the `cpp`, `rust` and `cddl` targets have no scalar codegen
+  and would silently omit the proc, so `genBindings()` fails and names it. Fix
+  it by generating C bindings from an `abi = c` library, switching the proc to
+  `abi = cbor`, giving it a non-scalar param, or passing
+  `-d:ffiAllowScalarSkip` to accept the omission — the proc still works over the
+  fast path, it's just absent from the bindings.
 
 ## Placement of `genBindings()`
 

examples/echo/c_abi_bindings/echo.h

@@ -37,14 +37,13 @@ typedef void (*EchoShoutReplyFn)(int err_code, const ShoutResponse* reply, const
 typedef void (*EchoVersionReplyFn)(int err_code, const char* reply, const char* err_msg, void* user_data);
 
 typedef void (*EchoCreateRawFn)(int err_code, const char* ctx_addr, const char* err_msg, void* user_data);
-/* Raw reply of a scalar-fast-path export: `msg`/`len` are raw bytes (a
-   string return's UTF-8, or the 8-byte native-endian scalar image), not
+/* Raw reply of a scalar-fast-path export: `msg`/`len` are bytes (a string
+   return's UTF-8, or the 8-byte native-endian scalar image), not
    NUL-terminated and valid only for the duration of the call. */
 typedef void (*EchoScalarRawFn)(int caller_ret, char* msg, size_t len, void* user_data);
 #ifndef NIMFFI_ABI_DUP_CSTR_N
 #define NIMFFI_ABI_DUP_CSTR_N
-/* NUL-terminated copy of a length-delimited (not NUL-terminated) byte run;
-   NULL on allocation failure or a length that would overflow `n + 1`. */
+/* NUL-terminated copy of a length-delimited byte run; NULL if it can't. */
 static inline char* nimffi_abi_dup_cstr_n(const char* s, size_t n) {
     if (n == SIZE_MAX) return NULL;
     char* p = (char*)malloc(n + 1);

examples/echo/c_bindings/nim_ffi_cbor.h

@@ -271,8 +271,8 @@ static inline char* nimffi_dup_cstr(const char* s) {
 }
 
 /* NUL-terminated copy of a length-delimited (not NUL-terminated) byte run,
- * for turning the FFICallback's raw error `msg`/`len` into a C string. Returns
- * NULL on allocation failure or a length that would overflow `n + 1`. */
+ * for turning the FFICallback's raw error `msg`/`len` into a C string; NULL if
+ * it can't. */
 static inline char* nimffi_dup_cstr_n(const char* s, size_t n) {
     if (n == SIZE_MAX) {
         return NULL;

examples/timer/c_bindings/nim_ffi_cbor.h

@@ -271,8 +271,8 @@ static inline char* nimffi_dup_cstr(const char* s) {
 }
 
 /* NUL-terminated copy of a length-delimited (not NUL-terminated) byte run,
- * for turning the FFICallback's raw error `msg`/`len` into a C string. Returns
- * NULL on allocation failure or a length that would overflow `n + 1`. */
+ * for turning the FFICallback's raw error `msg`/`len` into a C string; NULL if
+ * it can't. */
 static inline char* nimffi_dup_cstr_n(const char* s, size_t n) {
     if (n == SIZE_MAX) {
         return NULL;

ffi/codegen/c.nim

@@ -946,8 +946,6 @@ proc newAbiReg(types: seq[FFITypeMeta], procs: seq[FFIProcMeta]): AbiReg =
   for t in types:
     reg.typeTable[t.name] = t
   for p in procs:
-    # A scalar-fast-path proc has no Req envelope: its export takes the scalar
-    # args inline (see emitAbiScalarMethod).
     if p.kind != FFIKind.DTOR and not p.scalarFastPath:
       let rt = reqTypeMeta(p)
       reg.typeTable[rt.name] = rt
@@ -1013,24 +1011,18 @@ proc emitAbiReplyTypedefs(
     )
 
 func abiScalarRawFnName(libType: string): string =
-  ## The `FFICallBack`-shaped raw callback typedef a scalar-fast-path export
-  ## takes: the dylib replies with raw bytes (no CBOR, no flat struct) that the
-  ## per-method trampoline converts into the typed reply.
+  ## Raw-bytes callback typedef a scalar-fast-path export takes.
   return libType & "ScalarRawFn"
 
 const abiScalarDupCStr = "nimffi_abi_dup_cstr_n"
 
 func abiScalarDupHelper(): seq[string] =
-  ## CBOR-free twin of `nimffi_dup_cstr_n` for the scalar trampolines: a
-  ## NUL-terminated copy of a length-delimited (not NUL-terminated) byte run,
-  ## returning NULL on allocation failure or a length that would overflow the
-  ## `n + 1` size passed to `malloc`.
-  # Guarded so co-including two `abi = c` headers in one TU doesn't redefine it.
+  ## CBOR-free twin of `nimffi_dup_cstr_n`, include-guarded so two `abi = c`
+  ## headers can co-exist in one TU.
   return @[
     "#ifndef NIMFFI_ABI_DUP_CSTR_N",
     "#define NIMFFI_ABI_DUP_CSTR_N",
-    "/* NUL-terminated copy of a length-delimited (not NUL-terminated) byte run;",
-    "   NULL on allocation failure or a length that would overflow `n + 1`. */",
+    "/* NUL-terminated copy of a length-delimited byte run; NULL if it can't. */",
     "static inline char* " & abiScalarDupCStr & "(const char* s, size_t n) {",
     "    if (n == SIZE_MAX) return NULL;",
     "    char* p = (char*)malloc(n + 1);",
@@ -1044,8 +1036,6 @@ func abiScalarDupHelper(): seq[string] =
   ]
 
 func abiScalarArgParams(m: FFIProcMeta): seq[string] =
-  ## C parameters for a scalar method's args — passed inline by value, in both
-  ## the raw export and the high-level wrapper (no Req struct).
   var params: seq[string] = @[]
   for ep in m.extraParams:
     params.add(abiLeafCType(ep.typeName.strip()).cType & " " & ep.name)
@@ -1070,10 +1060,10 @@ proc emitAbiExternDecls(
         ")(int err_code, const char* ctx_addr, const char* err_msg, void* user_data);"
     )
   if haveScalar:
-    lines.add("/* Raw reply of a scalar-fast-path export: `msg`/`len` are raw bytes (a")
     lines.add(
-      "   string return's UTF-8, or the 8-byte native-endian scalar image), not"
+      "/* Raw reply of a scalar-fast-path export: `msg`/`len` are bytes (a string"
     )
+    lines.add("   return's UTF-8, or the 8-byte native-endian scalar image), not")
     lines.add("   NUL-terminated and valid only for the duration of the call. */")
     lines.add(
       "typedef void (*" & abiScalarRawFnName(libType) &
@@ -1240,11 +1230,9 @@ proc emitAbiMethod(
   lines.add("")
 
 func abiScalarOkLines(m: FFIProcMeta, fnType: string): seq[string] =
-  ## Trampoline RET_OK branch: convert the raw reply bytes into the typed
-  ## reply. A string return rides as its own UTF-8 bytes (copied and
-  ## NUL-terminated here); every other scalar is the 8-byte native-endian image
-  ## of the Nim-side pack (signed ints sign-extended to 64 bits, floats widened
-  ## to double, bool as 0/1 — see `ffiScalarRetBytes`).
+  ## Trampoline RET_OK branch. A string return rides as its own UTF-8; every
+  ## other scalar is the 8-byte image `ffiScalarRetBytes` packs (ints
+  ## sign-extended, floats widened to double, bool as 0/1).
   let rt = m.returnTypeName.strip()
   if rt == "string" or rt == "cstring":
     return @[
@@ -1294,13 +1282,9 @@ proc emitAbiScalarMethod(
     ctxType, libName, libType: string,
     m: FFIProcMeta,
 ) =
-  ## A scalar-fast-path method: no Req struct crosses — the wrapper hands the
-  ## scalar args straight to the raw export and a per-method trampoline adapts
-  ## the raw-bytes reply into the same typed `ReplyFn` surface the flat-struct
-  ## methods use. The heap box carrying the caller's callback is freed by the
-  ## trampoline, which the dylib invokes exactly once on every path (the ctx
-  ## guard and enqueue failures reply synchronously; success replies from the
-  ## FFI thread).
+  ## Args go inline to the raw export and a trampoline adapts the raw-bytes
+  ## reply into the typed `ReplyFn` surface. The trampoline frees the callback
+  ## box, relying on the dylib invoking it exactly once on every path.
   let stripped = stripLibPrefix(m.procName, m.libName)
   let pascal = snakeToPascalCase(stripped)
   let info = abiMethodReplyInfo(reg, libType, m)

ffi/codegen/meta.nim

@@ -32,11 +32,8 @@ type
     returnIsHandle*: bool
     abiFormat*: ABIFormat
     scalarFastPath*: bool
-      ## True for an `abi = c` proc whose whole signature is scalar (see
-      ## `isScalarOnly`): dispatches through the CBOR-free scalar fast path.
-      ## Only the `abi = c` C header emits a foreign binding for it (inline
-      ## scalar args + raw-bytes reply trampoline); every other target — and
-      ## the CBOR C header — drops it (see `bindableProcs`).
+      ## `abi = c` proc with an all-scalar signature: uses the CBOR-free fast
+      ## path, and binds only in the `abi = c` C header (see `bindableProcs`).
 
   FFIFieldMeta* = object
     name*: string
@@ -138,8 +135,5 @@ const ffiOutputDir* {.strdefine.} = ""
 # Nim src path override relative to outputDir (-d:ffiSrcPath); empty derives it.
 const ffiSrcPath* {.strdefine.} = ""
 
-# When set, scalar-only `abi = c` procs are silently omitted from bindings
-# generated by targets without scalar codegen (every target except the
-# `abi = c` C header) instead of failing the build. Off by default so the drop
-# is loud; see genBindings().
+# When true, targets without scalar codegen silently omit scalar-only `abi = c` procs rather than failing the build. Off by default so the drop is loud; see genBindings().
 const ffiAllowScalarSkip* {.booldefine.} = false

ffi/codegen/templates/c/cbor_helpers.h.tpl

@@ -271,8 +271,8 @@ static inline char* nimffi_dup_cstr(const char* s) {
 }
 
 /* NUL-terminated copy of a length-delimited (not NUL-terminated) byte run,
- * for turning the FFICallback's raw error `msg`/`len` into a C string. Returns
- * NULL on allocation failure or a length that would overflow `n + 1`. */
+ * for turning the FFICallback's raw error `msg`/`len` into a C string; NULL if
+ * it can't. */
 static inline char* nimffi_dup_cstr_n(const char* s, size_t n) {
     if (n == SIZE_MAX) {
         return NULL;

ffi/internal/ffi_macro.nim

@@ -1519,10 +1519,8 @@ macro ffiEvent*(args: varargs[untyped]): untyped =
   return generated
 
 proc reportScalarFastPathDrops(procs: seq[FFIProcMeta]) {.compileTime.} =
-  ## Only the `abi = c` C header emits foreign bindings for scalar-fast-path
-  ## procs; every other target (and the CBOR C header) drops them. Fail loudly,
-  ## naming them, unless `-d:ffiAllowScalarSkip` opts into the silent omission
-  ## (then just hint).
+  ## Fail loudly on scalar-fast-path procs a target can't bind, unless
+  ## `-d:ffiAllowScalarSkip` downgrades it to a hint.
   var skipped: seq[string] = @[]
   for p in procs:
     if p.scalarFastPath:
@@ -1537,15 +1535,17 @@ proc reportScalarFastPathDrops(procs: seq[FFIProcMeta]) {.compileTime.} =
       )
     return
   error(
-    "genBindings: this target has no foreign-binding codegen for " &
-      "scalar-fast-path `abi = c` procs, so these would be silently omitted " &
-      "from the generated bindings: " & skipped.join(", ") &
-      ".\nThey are emitted only into the `abi = c` C header (an `abi = c` " &
-      "library generated with -d:targetLang=c).\n" & "Fix by one of:\n" &
-      "  - make the library `abi = c` (declareLibrary(..., \"c\")) and generate " &
-      "C bindings, or\n" & "  - switch the proc to `abi = cbor`, or\n" &
-      "  - add a non-scalar param (e.g. a struct or handle) so it takes the " &
-      "CBOR wire shape, or\n" & "  - pass -d:ffiAllowScalarSkip to accept the omission."
+    """genBindings: this target has no foreign-binding codegen for scalar-fast-path
+`abi = c` procs, so these would be silently omitted from the generated bindings:
+$1
+They are emitted only into the `abi = c` C header (an `abi = c` library generated
+with -d:targetLang=c).
+Fix by one of:
+  - make the library `abi = c` (declareLibrary(..., "c")) and generate C bindings, or
+  - switch the proc to `abi = cbor`, or
+  - add a non-scalar param (e.g. a struct or handle) so it takes the CBOR wire shape, or
+  - pass -d:ffiAllowScalarSkip to accept the omission.""" %
+      [skipped.join(", ")]
   )
 
 proc bindingsOutputDir(lang, explicit: string): string {.compileTime.} =
@@ -1604,9 +1604,7 @@ macro genBindings*(
       let lang = string_helpers.toLower(rawLang.strip())
       if lang.len == 0:
         continue
-      # The `abi = c` C header is the one output with scalar-fast-path codegen,
-      # so it binds the full registry. Every other target — and the CBOR C
-      # header — drops scalar procs (loudly, unless skipped).
+      # The `abi = c` C header is the only output with scalar-fast-path codegen.
       let emitsScalars = lang == "c" and currentDefaultABIFormat == ABIFormat.C
       let genProcs =
         if emitsScalars:

ffi/internal/ffi_scalar.nim

@@ -32,11 +32,9 @@ func isScalarOnly*(p: FFIProcMeta): bool =
   true
 
 func bindableProcs*(procs: seq[FFIProcMeta]): seq[FFIProcMeta] =
-  ## The procs the CBOR-speaking foreign-binding generators emit for.
-  ## Scalar-fast-path procs are dropped: their C export takes inline scalar
-  ## args, not the CBOR `(reqCbor, reqCborLen)` shape those backends assume, so
-  ## emitting a CBOR caller for them would be wrong. Only the `abi = c` C header
-  ## has scalar codegen and binds the full registry (see genBindings()).
+  ## Procs the CBOR-speaking generators emit for; scalar-fast-path procs are
+  ## dropped (their inline-scalar export doesn't match the CBOR codegen shape).
+  ## The `abi = c` C header binds the full registry instead.
   var kept: seq[FFIProcMeta] = @[]
   for p in procs:
     if not p.scalarFastPath:

tests/e2e/c_abi/test_echo_c_abi.c

@@ -1,12 +1,9 @@
-/* End-to-end test for the CBOR-free `abi = c` echo bindings. Unlike the CBOR C
- * backend, this header links no TinyCBOR: the flat structs in echo.h are the C
- * ABI, strings are plain borrowed `const char*`. The test drives the same
- * async, callback-per-call surface — constructor, an object-returning method,
- * a scalar-fast-path method and teardown — copying out what each callback
- * delivers (owned by the binding, valid only for the call) and polling a
- * `done` flag to sequence the async calls. echoVersion rides the CBOR-free
- * scalar fast path: its binding passes no struct and adapts the raw-bytes
- * reply into the same typed callback shape as the flat `_CWire` methods. */
+/* End-to-end test for the CBOR-free `abi = c` echo bindings: the `_CWire`
+ * structs in echo.h are the C ABI, strings are borrowed `const char*`, no
+ * TinyCBOR. Drives the async callback-per-call surface (ctor, object-returning
+ * method, teardown). echoVersion rides the scalar fast path, whose binding
+ * passes no struct and adapts the raw-bytes reply into the same typed callback
+ * shape as the flat `_CWire` methods. */
 #include "echo.h"
 #include <assert.h>
 #include <stdatomic.h>

tests/unit/fixtures/scalar_skip_fixture.nim

@@ -1,8 +1,7 @@
 ## Compile fixture for the scalar-fast-path drop error (see
-## tests/unit/test_scalar_skip_gen.nim). This is a CBOR-default library with one
-## stray scalar `abi = c` proc, so no target can emit a binding for it: under
-## `-d:ffiGenBindings` genBindings() must fail — unless `-d:ffiAllowScalarSkip`
-## is passed, which downgrades the drop to a hint.
+## tests/unit/test_scalar_skip_gen.nim): a CBOR-default library, so no target
+## binds its scalar `abi = c` proc and genBindings() fails unless
+## -d:ffiAllowScalarSkip.
 
 import ffi, chronos
 
@@ -20,8 +19,8 @@ proc scalarskip_create*(cfg: SkipConfig): Future[Result[SkipLib, string]] {.ffiC
 proc scalarskip_add*(
     lib: SkipLib, a: int, b: int
 ): Future[Result[int, string]] {.ffi: "abi = c".} =
-  ## All-scalar signature: dispatches through the CBOR-free fast path. In this
-  ## CBOR-default library no target can emit a foreign binding for it.
+  ## All-scalar signature: CBOR-free fast path, unbindable outside an `abi = c`
+  ## library.
   return ok(lib.base + a + b)
 
 genBindings()

tests/unit/test_c_abi_codegen.nim

@@ -161,11 +161,8 @@ suite "generateCAbiLibHeader":
     check "return timer_add(ctx->ptr, timer_add_scalar_reply, box, a, b);" in header
 
   test "scalar returns unpack the 8-byte image; strings copy through the dup helper":
-    # int return: the slot is the sign-extended int64 image.
     check "memcpy(&reply, &slot, sizeof(reply));" in header
-    # float32 return: packed as a widened double, narrowed back here.
     check "float reply = (float)wide;" in header
-    # string return: copied via the overflow-guarded, NUL-terminating dup helper.
     check "char* reply = nimffi_abi_dup_cstr_n(msg ? msg : \"\", msg ? len : 0);" in
       header
     check "if (n == SIZE_MAX) return NULL;" in header

tests/unit/test_scalar_skip_gen.nim

@@ -1,12 +1,7 @@
-## Drives the scalar-fast-path drop error end to end: compiles
-## `fixtures/scalar_skip_fixture.nim` (a CBOR-default library with a stray
-## all-scalar `abi = c` proc) with `-d:ffiGenBindings` and asserts genBindings()
-## fails loudly, and that `-d:ffiAllowScalarSkip` downgrades the drop to a clean
-## build. The positive path — the `abi = c` C header emitting a real scalar
-## binding — is covered by test_c_abi_codegen and the echo c_abi e2e.
+## Asserts genBindings() fails loudly on a scalar `abi = c` proc no target can
+## bind, and that -d:ffiAllowScalarSkip downgrades it to a clean build.
 ##
-## The fixture is compiled in a child `nim check` (search paths and compiler
-## captured at compile time) so its expected failure is observed as a test
+## The fixture compiles in a child `nim check` so its expected failure is a test
 ## assertion, not this file's own compile error.
 
 import std/[os, osproc, strutils, compilesettings]