-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
node-api: clarify enum value ABI stability #59085
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
1ad13ea
to
4d200da
Compare
4d200da
to
bb4d552
Compare
bb4d552
to
898fccc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments tryong to improve the doc.
documented, an enum type should be considered to be extensible. | ||
|
||
For an enum type returned from an Node-API function, or as an out parameter of | ||
an Node-API function, the value is an integer value and an addon should |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a Node-API
work with bit operators like bit-OR (`|`) as a bit value. Unless otherwise | ||
documented, an enum type should be considered to be extensible. | ||
|
||
For an enum type returned from an Node-API function, or as an out parameter of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a Node
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or provided as an out ...
handle unknown values. For example, an addon should have a `default` branch | ||
when checking `napi_status` in switch statements as new status codes can be | ||
introduced in newer Node.js versions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, when checking napi_status
in switch statements, an addon should include a default
branch, as new status codes may be introduced in newer Node.js versions
For an enum type used in an in-parameter, extended values are guarded with | ||
Node-API version of which version they are introduced. The result of passing | ||
an unknown integer value to Node-API functions is undefined unless otherwise | ||
documented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For an enum type used in an in-parameter, extended values are version-guarded based on the Node-API version in which they were introduced. The result of passing an unknown integer value to Node-API functions is undefined unless otherwise documented.
### Enum values in ABI stability | ||
|
||
All enum data types defined in Node-API should be considered as a fixed size | ||
integer value. Bit flag enum types should be explicitly documented, and they |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we be more specific and say that we expect enums to be int32_t
values?
integer value. Bit flag enum types should be explicitly documented, and they | |
`int32_t` value. Bit flag enum types should be explicitly documented, and they |
an unknown integer value to Node-API functions is undefined unless otherwise | ||
documented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid the undefined bahaviour and rather say that it is an error.
an unknown integer value to Node-API functions is undefined unless otherwise | |
documented. | |
an unknown integer value to Node-API functions is an error. The Node-API | |
functions are expected to return napi_invalid_arg in such cases. |
@@ -121,6 +121,24 @@ must use Node-API exclusively by restricting itself to using | |||
and by checking, for all external libraries that it uses, that the external | |||
library makes ABI stability guarantees similar to Node-API. | |||
|
|||
### Enum values in ABI stability |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also augment the https://github.com/nodejs/node/blob/main/doc/contributing/adding-new-napi-api.md document along with it?
handle unknown values. For example, an addon should have a `default` branch | ||
when checking `napi_status` in switch statements as new status codes can be | ||
introduced in newer Node.js versions. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I see the justification for the napi_status
value, I would like to better understand the application for the out-
parameters. Are there examples in some system APIs where developers were adding new values to the output enum values. Are there any such examples in GCC libraries or any other system that cares about the ABI stability?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is rare because usually kernel APIs do not return a value like type enum
. But there are still cases like CLD_CONTINUED
used in siginfo_t::si_code
, as an out-param in waitid
, can be added over time. It is more common to return an int flag and new bit flags are added over time, e.g. IFF_LOWER_UP
in ioctl
.
It is more like a design choice that the kernel does not return types/flags information after the user code handed over them, avoiding compatibility issue.
However, Node-API needs to incorporate new ECMAScript features, and we must expose new (rare) JavaScript types (symbol as an example, retrospectively). Even napi_valuetype
can not be determined to be frozen and napi_typeof
must work with new types. As JavaScript land can pass new value types to a Node-API function freely (without respective Node-API versions), a Node-API function must defensively reject unknown types, in accordance to the ABI guarantee.
integer value. Bit flag enum types should be explicitly documented, and they | ||
work with bit operators like bit-OR (`|`) as a bit value. Unless otherwise | ||
documented, an enum type should be considered to be extensible. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mention that we must never remove or reuse previous enum values. Instead, we must always only add new enum values with the new integer values.
To me creating such rule today makes it ABI incompatible with the previous code compiled against Node-API. E.g. previously we added the new status such as I believe we must carefully discuss such new rule about freely adding new enum values to output and return values. IMHO, it may break the ABI compatibility. E.g. see this StackOverflow discussion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am concerned about freely adding new output and return enum values. Let's discuss it first.
For an enum type used in an in-parameter, extended values are guarded with | ||
Node-API version of which version they are introduced. The result of passing | ||
an unknown integer value to Node-API functions is undefined unless otherwise | ||
documented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a side note: these constraints generally look good to me but they could likely benefit from some concrete examples illustrating the correct structure.
For reference, #45181 added |
@legendecas , let's discuss it in more details tomorrow in Node-API meeting. This way we could add new features to the Node-API while allowing different implementations to say if they are available or not.
I believe that time we had to do something quick to address a serious issue. Due to V8 changes some of Node-API functions simply stopped to work. It would be nice to also think through the scenarios where the underlying JS engine not only adds features, but also removes or disables them. We must keep the ABI stability, but it seems like we do not have the full control over the overall API behavior. It maybe also worth to document it. |
Codify the ABI stability guarantees on Node-API enum types.
In summary, we should distinguish enums used as an in-parameter, and used as a return value or an out-parameter:
napi_invalid_arg
in general practice). New values will be introduced with an NAPI_VERSION guard.Unless explicitly documented, an enum type does not work with bit operators.
Refs: #58879