src: return Maybe<> on pending exception when cpp exception disabled by legendecas · Pull Request #927 · nodejs/node-addon-api

legendecas · 2021-03-09T13:15:56Z

Fixes: #690

This PR introduced a Maybe type to enforce value existence check when CPP exceptions disabled and there is a pending JavaScript exception so that the operation can not be performed.

The Maybe returning behavior requires a predefined macro NODE_ADDON_API_ENABLE_MAYBE to enable.

make exiting tests compatible with two flavors (with or without Maybes)

legendecas · 2021-05-23T16:04:05Z

@nodejs/node-api hi, team, I believe this PR is ready and may take some reviews. Thanks a lot!

mhdawson · 2021-06-10T15:43:16Z

A few questions that we might discuss more in the nest team meeting

This seems to change whether some functions are const or not ?
Changes Number::FloatValue - to add a round trip instead of cast ?
There are still a number of TODOs
- //TODO: find a way to define NAPI_HAS_ATTRIBUTE_WARN_UNUSED_RESULT
- Many of -> ///TODO(legendecas): find a way to boxing with MaybeOrValue
We might need more explanation of what additional we need to do when writing tests to make sure the
Maybe case is properly covered.

mhdawson · 2021-06-10T19:00:50Z

+
+Value fn(const CallbackInfo& info) {
+ Object obj = info[0].As<Object>();
+ Value value = MaybeUnwrap(obj->Get("foobar")); // <- `obj->Get` may throws


Should this be in a return or is it that we need to use the helpers for more than returns as well?

mhdawson · 2021-06-10T19:00:59Z

+are defined as `Napi::MaybeOrValue<>` to prevent from duplicating most of the
+code base.
+
+To properly test these build flavors, we should take care of the return value


Maybe this should be something like.
To properly test these build flavors, all values returned by a function defined/used within the
node-addon-api test suite, should use one of helpers to ....

mhdawson · 2021-06-11T15:41:05Z

 napi_status status = napi_coerce_to_number(_env, _value, &result);
-NAPI_THROW_IF_FAILED(_env, status, Number());
-return Number(_env, result);
+NAPI_MAYBE_RETURN_OR_THROW_IF_FAILED(


I think maybe this should just be called:
NAPI_RETURN_OR_THROW_IF_FAILED

Sounds much simpler. Renamed.

gabrielschulhof · 2021-06-11T15:52:30Z

 }

-inline bool Object::InstanceOf(const Function& constructor) const {
+inline MaybeOrValue<bool> Object::InstanceOf(const Function& constructor) {


This should remain const based on our discussion about #992.

@gabrielschulhof thanks for catching that, not sure why I did not find it on my second look.

Thanks for catching it, fixed.

mhdawson · 2021-06-16T17:55:57Z

@legendecas looks like this needs to be rebased.

# Conflicts: # napi-inl.h # test/binding.gyp

mhdawson · 2021-06-17T21:34:16Z

+```cpp
+#include "napi.h"
+#include "test_helper.h"
+
+using namespace Napi;
+
+void fn(const CallbackInfo& info) {
+ Object obj = info[0].As<Object>();
+ Value value = MaybeUnwrap(obj->Get("foobar")); // <- `obj->Get` is calling
+ // into JavaScript and may throw JavaScript Exceptions. Here we just assert
+ // getting the parameter must not fail for convenience.
+
+ // ... do works with the value.
+}
+```


I think this might be better after the three examples or maybe it could just be removed if we had examples in each of the sections for the helpers?

mhdawson

LGTM

legendecas · 2021-06-23T17:11:41Z

@gabrielschulhof 👋 may I have your review on this again? thanks a lot!

gabrielschulhof · 2021-07-09T17:05:44Z

 Napi::Symbol kTestMethodTInternal = Napi::Symbol::New(env, "kTestMethodTInternal");
 Napi::Symbol kTestVoidMethodTInternal = Napi::Symbol::New(env, "kTestVoidMethodTInternal");

- exports.Set("Test", DefineClass(env, "Test", {


This one and many lines below look like pure whitespace changes. Are they necessary?

There are two new elements at the bottom of the block. git-clang-format will check with blocks as minimum units.

legendecas · 2021-07-20T14:39:24Z

@gabrielschulhof hi, PR updated with suggestion applied. PTAL, thanks a lot!

mhdawson · 2021-07-27T15:15:56Z

@gabrielschulhof I'll plan to land end of this week unless you have objections/additional comments before then.

legendecas · 2021-08-03T06:25:47Z

@mhdawson @gabrielschulhof 👋 should we land this PR now?

mhdawson · 2021-08-03T14:26:12Z

@legendecas could you squash into 1 commit? I tried landing and I ended up with conflicts.

legendecas · 2021-08-03T15:04:33Z

@mhdawson used the GitHub interface to squash merging. It seems no conflicts at all..

PR-URL: nodejs#927 Reviewed-By: Michael Dawson <midawson@redhat.com>

PR-URL: nodejs/node-addon-api#927 Reviewed-By: Michael Dawson <midawson@redhat.com>

JoseExposito mentioned this pull request Mar 13, 2021
Add Object missing tests #923
Closed

legendecas force-pushed the maybe branch 2 times, most recently from 019adad to 2b40b49Compare March 26, 2021 08:44

This comment has been minimized.
Sign in to view

legendecas force-pushed the maybe branch from 2b40b49 to 1277c72Compare March 26, 2021 09:53

legendecas commented Mar 26, 2021
View reviewed changes

Comment threadnapi-inl.h Outdated

legendecas force-pushed the maybe branch 5 times, most recently from 96baadc to 7d933d0Compare April 6, 2021 15:57

legendecas marked this pull request as ready for review April 6, 2021 16:02

This comment has been minimized.
Sign in to view

legendecas force-pushed the maybe branch 2 times, most recently from d75115a to e42a1b9Compare April 15, 2021 18:29

legendecas force-pushed the maybe branch from e42a1b9 to efa8a2cCompare April 24, 2021 16:06

legendecas force-pushed the maybe branch 4 times, most recently from eb8980e to 9684123Compare May 23, 2021 16:01

legendecas commented May 23, 2021
View reviewed changes

Comment threaddoc/maybe.md

legendecas added 2 commits June 4, 2021 00:42

src: return Maybe<> on pending exception when cpp exception disabled
451f27f

fixup! remove the newly added napi_pending_exception checks
0f95980

legendecas force-pushed the maybe branch from 9684123 to 0f95980Compare June 3, 2021 16:42

legendecas requested a review from a teamJune 9, 2021 15:18

fixup! FunctionReference::New return type
676798c

legendecas added 2 commits June 11, 2021 00:26

fixup!
6d16dda

fixup! doc: test docs
c1385ba

mhdawson reviewed Jun 10, 2021
View reviewed changes

mhdawson reviewed Jun 11, 2021
View reviewed changes

gabrielschulhof reviewed Jun 11, 2021
View reviewed changes

fixup! review suggestions
fec9561

Merge branch 'main' into maybe
6012787
# Conflicts: # napi-inl.h # test/binding.gyp

mhdawson reviewed Jun 17, 2021
View reviewed changes

Comment threadtest/functionreference.cc Outdated

fixup! remove unnecessary MaybeUnwrapOr
baf6ab8

legendecas force-pushed the maybe branch from c6469c5 to baf6ab8Compare June 18, 2021 14:16

mhdawson approved these changes Jun 18, 2021
View reviewed changes

gabrielschulhof reviewed Jul 9, 2021
View reviewed changes

legendecas added 3 commits July 15, 2021 00:38

fixup! apply review suggestions
43e14e5

Merge remote-tracking branch 'upstream/main' into maybe
b16a549

fixup! update Symbol::For
68facc7

legendecas merged commit 3615041 into nodejs:mainAug 3, 2021

legendecas deleted the maybe branch August 3, 2021 15:04

deepakrkris pushed a commit to deepakrkris/node-addon-api that referenced this pull request Sep 23, 2021
src: return Maybe on pending exception when cpp exception disabled
6f79043
PR-URL: nodejs#927 Reviewed-By: Michael Dawson <midawson@redhat.com>

deepakrkris pushed a commit to deepakrkris/node-addon-api that referenced this pull request Oct 15, 2021
src: return Maybe on pending exception when cpp exception disabled
dcdd2ea
PR-URL: nodejs#927 Reviewed-By: Michael Dawson <midawson@redhat.com>

kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
src: return Maybe on pending exception when cpp exception disabled
12dec47
PR-URL: nodejs/node-addon-api#927 Reviewed-By: Michael Dawson <midawson@redhat.com>

Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
src: return Maybe on pending exception when cpp exception disabled
c9629da
PR-URL: nodejs/node-addon-api#927 Reviewed-By: Michael Dawson <midawson@redhat.com>

wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
src: return Maybe on pending exception when cpp exception disabled
1020609
PR-URL: nodejs/node-addon-api#927 Reviewed-By: Michael Dawson <midawson@redhat.com>

johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
src: return Maybe on pending exception when cpp exception disabled
101a5b9
PR-URL: nodejs/node-addon-api#927 Reviewed-By: Michael Dawson <midawson@redhat.com>

Uh oh!

Uh oh!

Conversation

legendecas commented Mar 9, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

This comment has been minimized.

legendecas commented May 23, 2021

Uh oh!

Uh oh!

mhdawson commented Jun 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhdawson commented Jun 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

legendecas commented Jun 23, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

legendecas commented Jul 20, 2021

Uh oh!

mhdawson commented Jul 27, 2021

Uh oh!

legendecas commented Aug 3, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson commented Aug 3, 2021

Uh oh!

legendecas commented Aug 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

legendecas commented Mar 9, 2021•
edited
Loading

legendecas commented Aug 3, 2021•
edited
Loading