(do not merge) comptime fn name guarantees for napi bindings#198
(do not merge) comptime fn name guarantees for napi bindings#198spiral-ladder wants to merge 36 commits intonapifrom
Conversation
Summary of ChangesHello @spiral-ladder, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust compile-time mechanism for NAPI bindings in Zig, ensuring that exposed JavaScript properties correctly map to underlying Zig functions. Beyond this core improvement, it substantially broadens the NAPI interface for Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
| makeNapiPropertyDescriptor(0, "slot"), | ||
| .{ .utf8name = "slott", .getter = napi.wrapCallback(0, BeaconStateView_slot) }, |
There was a problem hiding this comment.
this is the key benefit, if we misspell slott in line 735 this compiles fine and we'd probably only catch this at runtime. with a 2-line helper fn we use at 734 we can guarantee at compile time this doesn't happen.
if we do makeNapiPropertyDescriptor(0, "slot") we run into:
➜ zig build build-lib:bindings
build-lib:bindings
└─ install bindings
└─ zig build-lib bindings Debug native 1 errors
bindings/napi/BeaconStateView.zig:724:75: error: root source file struct 'BeaconStateView' has no member named 'BeaconStateView_slott'
return .{ .utf8name = name.ptr, .getter = napi.wrapCallback(argc_cap, @field(@This(), func_name)) };
^~~~~~~~~~~~~~~~~~~~~~~~~~![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a makeNapiPropertyDescriptor helper function to provide compile-time guarantees for N-API bindings, which is a valuable improvement for ensuring the correctness of the bindings. The changes also include a significant amount of refactoring and the addition of many new features to the BeaconStateView bindings. My review includes suggestions to improve the new helper function, fix a typo, and align with the repository's coding style guide.
| fn makeNapiPropertyDescriptor( | ||
| comptime argc_cap: usize, | ||
| comptime name: []const u8, | ||
| ) napi.c.napi_property_descriptor { | ||
| const func_name = @typeName(BeaconStateView) ++ "_" ++ name; | ||
|
|
||
| return .{ .utf8name = name.ptr, .getter = napi.wrapCallback(argc_cap, @field(@This(), func_name)) }; | ||
| } |
There was a problem hiding this comment.
This helper function is a great idea for compile-time safety. It could be extended to support methods as well, which would allow it to be used for more of the properties in the register function.
Here's a suggested implementation that supports both getters and methods:
fn makeNapiPropertyDescriptor(
comptime prop_type: enum { getter, method },
comptime argc_cap: usize,
comptime name: []const u8,
) napi.c.napi_property_descriptor {
const func_name = @typeName(BeaconStateView) ++ "_" ++ name;
const func = @field(@This(), func_name);
const callback = napi.wrapCallback(argc_cap, func);
return switch (prop_type) {
.getter => .{ .utf8name = name.ptr, .getter = callback },
.method => .{ .utf8name = name.ptr, .method = callback },
};
}
There was a problem hiding this comment.
yes we can extend this helper fn
| const shuffle = @import("./shuffle.zig"); | ||
| const proposer_index = @import("./proposer_index.zig"); | ||
| const beaconStateView = @import("./beacon_state_view.zig"); | ||
| const BeaconStateView = @import("./BeaconStateView.zig"); |
There was a problem hiding this comment.
The filename BeaconStateView.zig does not follow the snake_case convention for filenames specified in the style guide. Please rename it to beacon_state_view.zig. The imported module variable should also be camelCase (beaconStateView) as per the style guide. This will require updating its usage on line 31 as well.
const beaconStateView = @import("./beacon_state_view.zig");
References
- Style guide line 212 states: "Use
snake_casefor file names. ... We don't use Zig'sCamelCase.zigstyle for 'struct' files to keep the convention simple and consistent." It also states to usecamelCasefor variable names. (link)
There was a problem hiding this comment.
let's not follow tigerstyle for this rule
| var validators = try state.validators(); | ||
| const validators_len = try validators.length(); | ||
| if (voluntary_exit.validator_index >= validators_len) { | ||
| return false; | ||
| return .inactive; | ||
| } | ||
|
|
||
| var validator = try validators.get(@intCast(voluntary_exit.validator_index)); | ||
| const current_epoch = epoch_cache.epoch; | ||
|
|
||
| const activation_epoch = try validator.get("activation_epoch"); | ||
| // verify the validator is active | ||
| if (!try isActiveValidatorView(&validator, current_epoch)) { | ||
| return .inactive; | ||
| } |
There was a problem hiding this comment.
The logic for checking validator active status is duplicated. The check for voluntary_exit.validator_index >= validators_len on line 41 is effectively checking if the validator exists. If it doesn't, isActiveValidatorView will also return false (as it will be checking a default/zeroed validator struct if validators.get returns one for an out-of-bounds index, which won't be active). You can simplify this by removing the initial check.
| comptime argc_cap: usize, | ||
| comptime name: []const u8, | ||
| ) napi.c.napi_property_descriptor { | ||
| const func_name = @typeName(BeaconStateView) ++ "_" ++ name; |
There was a problem hiding this comment.
if we omit the BeaconStateView_ prefix (since we converted this into a top-level struct) we can just do @field(@This(), name) directly which is even nicer. cc @wemeetagain we spoke about naming with the prefix in our review call
2 participants
Introduces
makeNapiPropertyDescriptorthat guarantees, at compile-time, that a given function exists within our bindings.built on top of @guha-rahul 's PR #189
See relevant commits for a quick demo
f119052
13fedac