Skip to content

ffi: reject arguments after callback in registerCallback - #66997

Open
trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:ffi-register-callback-void
Open

ffi: reject arguments after callback in registerCallback#66997
trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:ffi-register-callback-void

Conversation

@trivikr

@trivikr trivikr commented Oct 25, 2026

Copy link
Copy Markdown
Member

Fixes: #66996

Passing the signature after the callback silently created a void () callback, so native callers got no arguments forwarded and an undefined return value.

Assisted-by: claude:opus-5.5

@trivikr
ffi: reject arguments after callback in registerCallback
73414d1 
Passing the signature after the callback silently created a `void ()`
callback, so native callers got no arguments forwarded and an
undefined return value.

Signed-off-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
Assisted-by: claude:opus-5.5
@nodejs-github-bot

nodejs-github-bot commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/ffi

@nodejs-github-bot nodejs-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 Oct 25, 2026
@trivikr trivikr added ffi Issues and PRs related to experimental Foreign Function Interface support. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Oct 25, 2026
@codecov

codecov Bot commented Oct 25, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.59%. Comparing base (9b5d938) to head (73414d1).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #66997   +/-   ##
=======================================
  Coverage   90.59%   90.59%           
=======================================
  Files         797      797           
  Lines      278367   278372    +5     
  Branches    53531    53534    +3     
=======================================
+ Hits       252174   252203   +29     
+ Misses      16565    16534   -31     
- Partials     9628     9635    +7
Files with missing lines Coverage Δ
src/node_ffi.cc 72.87% <100.00%> (+0.09%) ⬆️

... and 34 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.

mcollina
mcollina approved these changes Oct 25, 2026
View reviewed changes

@mcollina mcollina left a comment

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.

lgtm

@mcollina mcollina added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. and removed request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Oct 25, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 25, 2026
@nodejs-github-bot

nodejs-github-bot commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator

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

@trivikr trivikr added author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue PRs queued for automated landing through the Commit Queue. labels Oct 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue PRs queued for automated landing through the Commit Queue. ffi Issues and PRs related to experimental Foreign Function Interface support. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ffi: registerCallback(callback, signature) silently creates a void () callback

3 participants

@trivikr @nodejs-github-bot @mcollina