diff --git a/CHANGELOG.md b/CHANGELOG.md index b3410ae..569b91e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Support returning references to nested objects - Fix [#16](https://github.com/mmomtchev/nobind/issues/16), global and `static` members can be `noexcept` - Fix a minor memory leak when calling a method with incorrect number of arguments +- More meaningful exceptions for constructors to aid debugging ### [1.1.1] 2023-12-01 diff --git a/include/noobject.h b/include/noobject.h index 7f92494..7438464 100644 --- a/include/noobject.h +++ b/include/noobject.h @@ -2,6 +2,7 @@ #include #include #include +#include #include #include @@ -358,7 +359,7 @@ template NoObjectWrap::~NoObjectWrap() { // * From C++ with a Napi::External<> pointer -> it must construct a proxy for this object template NoObjectWrap::NoObjectWrap(const Napi::CallbackInfo &info) : Napi::ObjectWrap>(info) { - // Napi::Env env = info.Env(); + Napi::Env env{info.Env()}; if (info.Length() == 2 && info[0].IsExternal()) { // From C++ owned = info[1].ToBoolean().Value(); @@ -368,16 +369,35 @@ NoObjectWrap::NoObjectWrap(const Napi::CallbackInfo &info) : Napi::Object // From JS owned = true; if (cons.size() > info.Length() && cons[info.Length()].size() > 0) { + std::vector errors; for (auto ctor : cons[info.Length()]) { try { (this->*ctor)(info); return; - } catch (...) { + } catch (const Napi::Error &e) { + // If there is only one constructor for the given number of arguments, + // throw the original construction error, instead of a generic error + // saying that all constructors with X arguments have been tried and none work + if (cons[info.Length()].size() == 1) { + std::rethrow_exception(std::current_exception()); + } + // If not concatenate all the errors + errors.push_back(e.what()); + } catch (const std::exception &e) { + // Same as above + if (cons[info.Length()].size() == 1) { + throw Napi::TypeError::New(env, e.what()); + } + errors.push_back(e.what()); } } + throw Napi::TypeError::New( + env, "All constructors with "s + std::to_string(info.Length()) + " arguments tried: ["s + + std::accumulate(errors.begin() + 1, errors.end(), *errors.begin(), + [](const std::string s1, const std::string s2) { return s1 + ", " + s2; }) + + "]"s); } - throw Napi::TypeError::New(info.Env(), - "No constructor with the given "s + std::to_string(info.Length()) + " arguments found"s); + throw Napi::TypeError::New(env, "No constructor with "s + std::to_string(info.Length()) + " arguments found"); } template diff --git a/test/fixtures/two_cons.cc b/test/fixtures/two_cons.cc index 2b71cd6..bd1b294 100644 --- a/test/fixtures/two_cons.cc +++ b/test/fixtures/two_cons.cc @@ -1,7 +1,10 @@ #include "two_cons.h" +#include TwoCons::TwoCons(int a) { x = a; } TwoCons::TwoCons(const std::string &s) { x = std::stoi(s); } TwoCons::TwoCons() { x = -1; } + +TwoCons::TwoCons(bool) { throw std::logic_error{"wrong constructor"}; } diff --git a/test/fixtures/two_cons.h b/test/fixtures/two_cons.h index 587e9c0..450036b 100644 --- a/test/fixtures/two_cons.h +++ b/test/fixtures/two_cons.h @@ -6,5 +6,6 @@ class TwoCons { TwoCons(int a); TwoCons(const std::string &s); + TwoCons(bool); TwoCons(); }; diff --git a/test/tests/basic_class.js b/test/tests/basic_class.js index 2da0920..ea5248c 100644 --- a/test/tests/basic_class.js +++ b/test/tests/basic_class.js @@ -9,11 +9,11 @@ describe('constructor', () => { it('exception', () => { assert.throws(() => { new dll.Hello; - }, /No constructor with the given 0 arguments found/); + }, /No constructor with 0 arguments found/); assert.throws(() => { new dll.Hello(2); - }, /No constructor with the given 1 arguments found/); + }, /Expected a string/); }); }); diff --git a/test/tests/class_obj.js b/test/tests/class_obj.js index 1838c98..98a426c 100644 --- a/test/tests/class_obj.js +++ b/test/tests/class_obj.js @@ -31,7 +31,7 @@ describe('reference', () => { it('null argument when Expected allowed', () => { assert.throws(() => { new dll.Hello(null); - }, /No constructor with the given 1 arguments found/); + }, /Expected a string/); assert.throws(() => { assert.isNumber(dll.hello_ref(null)); }, /Expected an object/); @@ -56,7 +56,7 @@ describe('reference', () => { it('undefined', () => { assert.throws(() => { new dll.Hello(null); - }, /No constructor with the given 1 arguments found/); + }, /Expected a string/); assert.throws(() => { assert.isNumber(dll.hello_ref(undefined)); }, /Expected an object/); diff --git a/test/tests/cons_overload.cc b/test/tests/cons_overload.cc index 2b604f5..1c69821 100644 --- a/test/tests/cons_overload.cc +++ b/test/tests/cons_overload.cc @@ -3,5 +3,5 @@ #include NOBIND_MODULE(cons_overload, m) { - m.def("TwoCons").cons<>().cons().cons().def<&TwoCons::x>("x"); + m.def("TwoCons").cons<>().cons().cons().cons().def<&TwoCons::x>("x"); } diff --git a/test/tests/cons_overload.js b/test/tests/cons_overload.js index f5adedd..e2e52c6 100644 --- a/test/tests/cons_overload.js +++ b/test/tests/cons_overload.js @@ -15,13 +15,19 @@ describe('overloaded constructor', () => { assert.strictEqual(o.x, 2); }); + it('original C++ exception', () => { + assert.throws(() => { + new dll.TwoCons(false); + }, /wrong constructor/); + }); + it('exception', () => { assert.throws(() => { new dll.TwoCons(1, 2, 3); - }, /No constructor with the given 3 arguments found/); + }, /No constructor with 3 arguments found/); assert.throws(() => { new dll.TwoCons(null); - }, /No constructor with the given 1 arguments found/); + }, /All constructors with 1 arguments tried: \[Expected a string, Expected a number, Expected a boolean\]/); }); });