ffi: remove function signature property aliases

Signed-off-by: Renegade334 <contact.9a5d6388@renegade334.me.uk>
PR-URL: nodejs#63482
Refs: https://github.com/nodejs/node/pull/62072/changes#r3067834658
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>

Author: Renegade334

doc/api/ffi.md

@@ -124,18 +124,18 @@ such as `0` and `1`; JavaScript `true` and `false` are not accepted.
 
 Functions and callbacks are described with signature objects.
 
-Supported fields:
+Signature objects may contain the following properties, both of which are
+optional:
 
-* `result`, `return`, or `returns` for the return type.
-* `parameters` or `arguments` for the parameter type list.
+* `return` {string} A [type name][type names] specifying the return type of the
+  function or callback. **Default:** `'void'`.
+* `arguments` {string\[]} An array of [type names][] specifying the argument
+  type list of the function or callback. **Default:** `[]`.
 
-Only one return-type field and one parameter-list field may be present in a
-single signature object.
-
-```cjs
+```js
 const signature = {
-  result: 'i32',
-  parameters: ['i32', 'i32'],
+  return: 'i32',
+  arguments: ['i32', 'i32'],
 };
 ```
 
@@ -193,7 +193,7 @@ import { dlopen } from 'node:ffi';
 
 {
   using handle = dlopen('./mylib.so', {
-    add_i32: { parameters: ['i32', 'i32'], result: 'i32' },
+    add_i32: { arguments: ['i32', 'i32'], return: 'i32' },
   });
   console.log(handle.functions.add_i32(20, 22));
 } // handle.lib.close() is invoked automatically here.
@@ -203,8 +203,8 @@ import { dlopen } from 'node:ffi';
 import { dlopen } from 'node:ffi';
 
 const { lib, functions } = dlopen('./mylib.so', {
-  add_i32: { parameters: ['i32', 'i32'], result: 'i32' },
-  string_length: { parameters: ['pointer'], result: 'u64' },
+  add_i32: { arguments: ['i32', 'i32'], return: 'i32' },
+  string_length: { arguments: ['pointer'], return: 'u64' },
 });
 
 console.log(functions.add_i32(20, 22));
@@ -214,8 +214,8 @@ console.log(functions.add_i32(20, 22));
 const { dlopen } = require('node:ffi');
 
 const { lib, functions } = dlopen('./mylib.so', {
-  add_i32: { parameters: ['i32', 'i32'], result: 'i32' },
-  string_length: { parameters: ['pointer'], result: 'u64' },
+  add_i32: { arguments: ['i32', 'i32'], return: 'i32' },
+  string_length: { arguments: ['pointer'], return: 'u64' },
 });
 
 console.log(functions.add_i32(20, 22));
@@ -356,8 +356,8 @@ const { DynamicLibrary } = require('node:ffi');
 
 const lib = new DynamicLibrary('./mylib.so');
 const add = lib.getFunction('add_i32', {
-  parameters: ['i32', 'i32'],
-  result: 'i32',
+  arguments: ['i32', 'i32'],
+  return: 'i32',
 });
 
 console.log(add(20, 22));
@@ -407,7 +407,7 @@ const { DynamicLibrary } = require('node:ffi');
 const lib = new DynamicLibrary('./mylib.so');
 
 const callback = lib.registerCallback(
-  { parameters: ['i32'], result: 'i32' },
+  { arguments: ['i32'], return: 'i32' },
   (value) => value * 2,
 );
 ```
@@ -417,7 +417,7 @@ Callbacks are subject to the following restrictions:
 * They must be invoked on the same system thread where they were created.
 * They must not throw exceptions.
 * They must not return promises.
-* They must return a value compatible with the declared result type.
+* They must return a value compatible with the declared return type.
 * They must not call `library.close()` on their owning library while running.
 * They must not unregister themselves while running.
 
@@ -465,7 +465,7 @@ JavaScript `number` values that match the declared type.
 
 For 64-bit integer types (`i64` and `u64`), pass JavaScript `bigint` values.
 
-For pointer-like parameters:
+For pointer-like arguments:
 
 * `null` and `undefined` are passed as null pointers.
 * `string` values are copied to temporary NUL-terminated UTF-8 strings for the
@@ -727,3 +727,4 @@ and keep callback and pointer lifetimes explicit on the native side.
 [`--allow-ffi`]: cli.md#--allow-ffi
 [`ffi.toBuffer(pointer, length, copy)`]: #ffitobufferpointer-length-copy
 [`using`]: https://tc39.es/proposal-explicit-resource-management/#sec-using-declarations
+[type names]: #type-names

lib/internal/ffi-shared-buffer.js

@@ -31,18 +31,14 @@ const {
   TypeError,
 } = primordials;
 
-const {
-  codes: {
-    ERR_INTERNAL_ASSERTION,
-  },
-} = require('internal/errors');
+const assert = require('internal/assert');
 
 const {
   DynamicLibrary,
   charIsSigned,
+  kSbArguments,
   kSbInvokeSlow,
-  kSbParams,
-  kSbResult,
+  kSbReturn,
   kSbSharedBuffer,
   uintptrMax,
 } = internalBinding('ffi');
@@ -159,11 +155,9 @@ function writeNumericArg(view, info, offset, arg, index) {
     return;
   }
 
-  /* c8 ignore start */
   // Unreachable: caller filters out non-numeric kinds.
-  throw new ERR_INTERNAL_ASSERTION(
-    `FFI: writeNumericArg reached with unexpected kind="${kind}"`);
-  /* c8 ignore stop */
+  /* c8 ignore next */
+  assert.fail(`FFI: writeNumericArg reached with unexpected kind="${kind}"`);
 }
 
 // Returns true on fast-path success, false when the caller must fall back
@@ -208,51 +202,46 @@ function inheritMetadata(wrapper, rawFn, nargs) {
 // arguments out of it into invocation-local storage before `ffi_call` and
 // reads the return value back only after, so nested/reentrant calls into
 // the same function are safe.
-function wrapWithSharedBuffer(rawFn, parameters, resultType) {
-  if (rawFn === undefined || rawFn === null) return rawFn;
+function wrapWithSharedBuffer(rawFn, signature) {
+  if (rawFn == null) return rawFn;
   const buffer = rawFn[kSbSharedBuffer];
   if (buffer === undefined) return rawFn;
 
   // Callers without explicit signature info (the `functions` accessor
-  // patch below) rely on the `kSbParams` / `kSbResult` metadata attached
+  // patch below) rely on the `kSbArguments` / `kSbReturn` metadata attached
   // by the native `CreateFunction`.
-  if (parameters === undefined) parameters = rawFn[kSbParams];
-  if (resultType === undefined) resultType = rawFn[kSbResult];
-  // `CreateFunction` always attaches these for SB-eligible functions.
-  // Missing here means the native side and this wrapper are out of sync.
-  /* c8 ignore start */
-  if (parameters === undefined || resultType === undefined) {
-    throw new ERR_INTERNAL_ASSERTION(
-      'FFI: shared-buffer raw function is missing kSbParams or kSbResult');
+  let argumentTypes, returnType;
+  if (signature === undefined) {
+    argumentTypes = rawFn[kSbArguments];
+    returnType = rawFn[kSbReturn];
+
+    // `CreateFunction` always attaches these for SB-eligible functions.
+    // Missing here means the native side and this wrapper are out of sync.
+    assert(argumentTypes !== undefined && returnType !== undefined,
+           'FFI: shared-buffer raw function is missing kSbArguments or kSbReturn');
+  } else {
+    argumentTypes = signature.arguments ?? [];
+    returnType = signature.return ?? 'void';
   }
-  /* c8 ignore stop */
 
   const slowInvoke = rawFn[kSbInvokeSlow];
   const view = new DataView(buffer);
   let retGetter = null;
-  if (resultType !== 'void') {
-    const retInfo = sbTypeInfo[resultType];
-    /* c8 ignore start */
-    if (retInfo === undefined) {
-      throw new ERR_INTERNAL_ASSERTION(
-        `FFI: shared-buffer type table missing entry for result type "${resultType}"`);
-    }
-    /* c8 ignore stop */
+  if (returnType !== 'void') {
+    const retInfo = sbTypeInfo[returnType];
+    assert(retInfo !== undefined,
+           `FFI: shared-buffer type table missing entry for return type "${returnType}"`);
     retGetter = retInfo.get;
   }
 
-  const nargs = parameters.length;
+  const nargs = argumentTypes.length;
   const argInfos = [];
   const argOffsets = [];
   let anyPointer = false;
   for (let i = 0; i < nargs; i++) {
-    const info = sbTypeInfo[parameters[i]];
-    /* c8 ignore start */
-    if (info === undefined) {
-      throw new ERR_INTERNAL_ASSERTION(
-        `FFI: shared-buffer type table missing entry for parameter type "${parameters[i]}"`);
-    }
-    /* c8 ignore stop */
+    const info = sbTypeInfo[argumentTypes[i]];
+    assert(info !== undefined,
+           `FFI: shared-buffer type table missing entry for argument type "${argumentTypes[i]}"`);
     // Push the `sbTypeInfo` entry directly (entries with the same `kind`
     // share a shape, keeping `writeNumericArg`'s call sites
     // low-polymorphism) and store offsets in a parallel array to avoid
@@ -267,13 +256,8 @@ function wrapWithSharedBuffer(rawFn, parameters, resultType) {
     // Pointer signatures need a per-arg runtime type check and fall back
     // to the native slow-path invoker for non-BigInt pointer arguments,
     // so arity specialization wouldn't buy much here.
-    /* c8 ignore start */
-    if (slowInvoke === undefined) {
-      throw new ERR_INTERNAL_ASSERTION(
-        'FFI: shared-buffer raw function with pointer arguments is ' +
-        'missing kSbInvokeSlow');
-    }
-    /* c8 ignore stop */
+    assert(slowInvoke !== undefined,
+           'FFI: shared-buffer raw function with pointer arguments is missing kSbInvokeSlow');
     wrapper = function(...args) {
       if (args.length !== nargs) {
         throwFFIArgCountError(nargs, args.length);
@@ -542,19 +526,6 @@ function buildNumericWrapper(
   };
 }
 
-// Accept-set mirrors the native `ParseFunctionSignature` in
-// `src/ffi/types.cc`. `ParseFunctionSignature` additionally throws when
-// multiple aliases are set at once. The wrapper runs before the native
-// call, so those conflicts still surface from the native side regardless
-// of which alias we happen to read here.
-function sigParams(sig) {
-  return sig.parameters ?? sig.arguments ?? [];
-}
-
-function sigResult(sig) {
-  return sig.result ?? sig.return ?? sig.returns ?? 'void';
-}
-
 // The native invoker for SB-eligible symbols is `InvokeFunctionSB`, which
 // reads arguments from the shared buffer populated by
 // `wrapWithSharedBuffer`. These patches make sure every path that surfaces
@@ -563,11 +534,11 @@ function sigResult(sig) {
 const rawGetFunction = DynamicLibrary.prototype.getFunction;
 const rawGetFunctions = DynamicLibrary.prototype.getFunctions;
 
-DynamicLibrary.prototype.getFunction = function getFunction(name, sig) {
-  // Native `DynamicLibrary::GetFunction` validates `sig`, so by the time
-  // we have `raw` we know `sig` is a valid object.
-  const raw = FunctionPrototypeCall(rawGetFunction, this, name, sig);
-  return wrapWithSharedBuffer(raw, sigParams(sig), sigResult(sig));
+DynamicLibrary.prototype.getFunction = function getFunction(name, signature) {
+  // Native `DynamicLibrary::GetFunction` validates `signature`, so by the time
+  // we have `raw` we know `signature` is a valid object.
+  const raw = FunctionPrototypeCall(rawGetFunction, this, name, signature);
+  return wrapWithSharedBuffer(raw, signature);
 };
 
 DynamicLibrary.prototype.getFunctions = function getFunctions(definitions) {
@@ -583,13 +554,12 @@ DynamicLibrary.prototype.getFunctions = function getFunctions(definitions) {
   for (let i = 0; i < keys.length; i++) {
     const name = keys[i];
     // No `definitions`: native side returned every cached function, so we
-    // wrap using each function's own `kSbParams` / `kSbResult` metadata
+    // wrap using each function's own `kSbArguments` / `kSbReturn` metadata
     // (same fallback as the `functions` accessor).
     if (definitions === undefined) {
       out[name] = wrapWithSharedBuffer(raw[name]);
     } else {
-      const sig = definitions[name];
-      out[name] = wrapWithSharedBuffer(raw[name], sigParams(sig), sigResult(sig));
+      out[name] = wrapWithSharedBuffer(raw[name], definitions[name]);
     }
   }
   return out;
@@ -602,16 +572,12 @@ DynamicLibrary.prototype.getFunctions = function getFunctions(definitions) {
   // uninitialized buffer.
   const functionsDescriptor =
     ObjectGetOwnPropertyDescriptor(DynamicLibrary.prototype, 'functions');
-  /* c8 ignore start */
-  if (functionsDescriptor === undefined || !functionsDescriptor.get) {
-    // Missing getter means the native and JS sides are out of sync; silently
-    // skipping the patch would expose the fast-path-against-uninitialized-buffer
-    // footgun this whole block exists to prevent.
-    throw new ERR_INTERNAL_ASSERTION(
-      'FFI: DynamicLibrary.prototype.functions accessor not found or has no getter');
-  }
-  /* c8 ignore stop */
-  const origGetter = functionsDescriptor.get;
+  const origGetter = functionsDescriptor?.get;
+  // Missing getter means the native and JS sides are out of sync; silently
+  // skipping the patch would expose the fast-path-against-uninitialized-buffer
+  // footgun this whole block exists to prevent.
+  assert(origGetter !== undefined,
+         'FFI: DynamicLibrary.prototype.functions accessor not found or has no getter');
   ObjectDefineProperty(DynamicLibrary.prototype, 'functions', {
     __proto__: null,
     configurable: true,

src/env_properties.h

@@ -48,8 +48,8 @@
   V(async_id_symbol, "async_id_symbol")                                        \
   V(ffi_sb_shared_buffer_symbol, "ffi_sb_shared_buffer_symbol")                \
   V(ffi_sb_invoke_slow_symbol, "ffi_sb_invoke_slow_symbol")                    \
-  V(ffi_sb_params_symbol, "ffi_sb_params_symbol")                              \
-  V(ffi_sb_result_symbol, "ffi_sb_result_symbol")                              \
+  V(ffi_sb_arguments_symbol, "ffi_sb_arguments_symbol")                        \
+  V(ffi_sb_return_symbol, "ffi_sb_return_symbol")                              \
   V(constructor_key_symbol, "constructor_key_symbol")                          \
   V(handle_onclose_symbol, "handle_onclose")                                   \
   V(no_message_symbol, "no_message_symbol")                                    \
@@ -293,7 +293,6 @@
   V(password_string, "password")                                               \
   V(path_string, "path")                                                       \
   V(pathname_string, "pathname")                                               \
-  V(parameters_string, "parameters")                                           \
   V(pending_handle_string, "pendingHandle")                                    \
   V(permission_string, "permission")                                           \
   V(phase_string, "phase")                                                     \
@@ -329,9 +328,8 @@
   V(require_string, "require")                                                 \
   V(resource_string, "resource")                                               \
   V(result_string, "result")                                                   \
-  V(return_string, "return")                                                   \
-  V(returns_string, "returns")                                                 \
   V(return_arrays_string, "returnArrays")                                      \
+  V(return_string, "return")                                                   \
   V(salt_length_string, "saltLength")                                          \
   V(search_string, "search")                                                   \
   V(servername_string, "servername")                                           \