Skip to content

ffi: remove function signature property aliases - #63482

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
Renegade334:ffi-dealias-signatures
May 24, 2026
Merged

ffi: remove function signature property aliases#63482
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
Renegade334:ffi-dealias-signatures

Conversation

@Renegade334

@Renegade334Renegade334 commented May 21, 2026•
edited
Loading

Copy link
Copy Markdown
Member

Refs: https://github.com/nodejs/node/pull/62072/changes#r3067834658

Having multiple property name aliases for function signature objects doesn't add utility, but from a TS perspective, it blocks being able to infer the resultant function type from the shape of the signature object. With all else being equal, it'd be useful to get rid of them.

@nodejs-github-bot

nodejs-github-bot commented May 21, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/ffi

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels May 21, 2026
@Renegade334Renegade334 added the ffi Issues and PRs related to experimental Foreign Function Interface support. label May 21, 2026
@Renegade334
ffi: remove function signature property aliases
52cedc7
Signed-off-by: Renegade334 <contact.9a5d6388@renegade334.me.uk>
@Renegade334
Renegade334force-pushed the ffi-dealias-signatures branch from 35c2eee to 52cedc7CompareMay 21, 2026 23:15
@codecov

codecovBot commented May 22, 2026•
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.13%. Comparing base (2f56cd2) to head (fc33b1a).
⚠️ Report is 19 commits behind head on main.

Files with missing linesPatch %Lines
lib/internal/ffi-shared-buffer.js93.02%3 Missing ⚠️
src/ffi/types.cc60.00%0 Missing and 2 partials ⚠️
src/node_ffi.cc91.66%0 Missing and 1 partial ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #63482 +/- ##
==========================================
- Coverage 90.17% 90.13% -0.05% 
==========================================
Files 718 718 Lines 227731 228342 +611 Branches 42768 42919 +151 ==========================================
+ Hits 205365 205807 +442 - Misses 14145 14274 +129 - Partials 8221 8261 +40
Files with missing linesCoverage Δ
src/node_ffi.cc70.05% <91.66%> (+0.04%)⬆️
src/ffi/types.cc46.91% <60.00%> (-2.01%)⬇️
lib/internal/ffi-shared-buffer.js98.83% <93.02%> (-0.22%)⬇️

... and 64 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

addaleax
addaleax approved these changes May 22, 2026
View reviewed changes
Comment threaddoc/api/ffi.md
* `result`, `return`, or `returns` for the return type.
* `parameters` or `arguments` for the parameter type list.
* `result` {string} A [type name][type names] specifying the return type of the
function or callback. **Default:** `'void'`.

@addaleaxaddaleaxMay 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I can cast a vote for keeping return instead of result, I'd like to do so – that's just generally more in line with what the standardized terminology around function signatures is

@ShogunPandaShogunPandaMay 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if I have to pick one I'd go for return and arguments.
@Renegade334 Are you fine with doing so?

@Renegade334Renegade334May 22, 2026

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me.

@ShogunPandaShogunPandaMay 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, if @addaleax concurs I think you can continue with the changes.

@Renegade334
Renegade334force-pushed the ffi-dealias-signatures branch 2 times, most recently from d2f2716 to dc9a62bCompareMay 23, 2026 16:40
@Renegade334
Renegade334force-pushed the ffi-dealias-signatures branch from dc9a62b to fc33b1aCompareMay 23, 2026 16:41
Renegade334
Renegade334 commented May 23, 2026
View reviewed changes
Comment threadlib/internal/ffi-shared-buffer.js
ERR_INTERNAL_ASSERTION,
},
} = require('internal/errors');
const assert = require('internal/assert');

@Renegade334Renegade334May 23, 2026

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of the assertions had to be modified anyway with this changeset, so as a driveby I've changed them all to the canonical internal assertion pattern.

@ShogunPandaShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2026
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2026
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented May 24, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/73655/

@ShogunPandaShogunPanda added the commit-queue Add this label to land a pull request using GitHub Actions. label May 24, 2026
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 24, 2026
@nodejs-github-bot
nodejs-github-bot merged commit df09b2a into nodejs:mainMay 24, 2026
82 of 84 checks passed
@nodejs-github-bot

nodejs-github-bot commented May 24, 2026

Copy link
Copy Markdown
Collaborator

Landed in df09b2a

aduh95 pushed a commit that referenced this pull request May 27, 2026
@Renegade334@aduh95
ffi: remove function signature property aliases
972cd22
Signed-off-by: Renegade334 <contact.9a5d6388@renegade334.me.uk>
PR-URL: #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>
@github-actionsgithub-actionsBot mentioned this pull request May 30, 2026
Merged
addaleax added a commit to addaleax/node that referenced this pull request May 30, 2026
@addaleax
benchmark: remove old alias usage in ffi benchmarks
2a4bb29
Follow-up to df09b2a.
Refs: nodejs#63482
Signed-off-by: Anna Henningsen <anna@addaleax.net>
@addaleaxaddaleax mentioned this pull request May 30, 2026
Merged
@Renegade334
Renegade334 deleted the ffi-dealias-signatures branch June 1, 2026 16:56
nodejs-github-bot pushed a commit that referenced this pull request Jun 1, 2026
@addaleax
benchmark: remove old alias usage in ffi benchmarks
7af433b
Follow-up to df09b2a.
Refs: #63482
Signed-off-by: Anna Henningsen <anna@addaleax.net>
PR-URL: #63666
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
aduh95 pushed a commit that referenced this pull request Jun 18, 2026
@addaleax@aduh95
benchmark: remove old alias usage in ffi benchmarks
066fff1
Follow-up to df09b2a.
Refs: #63482
Signed-off-by: Anna Henningsen <anna@addaleax.net>
PR-URL: #63666
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ShogunPandaShogunPandaShogunPanda approved these changes
@addaleaxaddaleaxaddaleax approved these changes

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.ffiIssues and PRs related to experimental Foreign Function Interface support.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Renegade334@nodejs-github-bot@ShogunPanda@addaleax