From 1a1b5106370a7b99414a83043cc9e3da9817606a Mon Sep 17 00:00:00 2001 From: alandefreitas Date: Wed, 29 Nov 2023 19:36:14 -0300 Subject: [PATCH] feat: complete JavaScript helpers support This commit uses proxy objects to offer complete support for JavaScript helpers that return reference types. Previously, JavaScript reference types returned by these functions were deep-copied or not handled at all. Now, proxy objects in both directions are used while helpers create a scope that's kept alive as long as necessary by the Handlebars engine. As the objects don't need to be deep copied, this change improves performance and allows objects with circular references, which are common in MrDocs. Additionally, JavaScript helpers receive a proxy object equivalent to the handlebars `options` object, and helper function registration was also simplified and improved to remove redundant code. This commit provides new test cases to validate the current code without counting on MrDocs. --- CMakeUserPresets.json.example | 9 + include/mrdocs/Support/JavaScript.hpp | 53 ++++ include/mrdocs/Support/Path.hpp | 82 ++++-- .../addons/generator/html/helpers/add.js | 3 + src/lib/Gen/adoc/Builder.cpp | 66 +---- src/lib/Gen/html/Builder.cpp | 75 +---- src/lib/Support/JavaScript.cpp | 258 +++++++++++++++++- src/test/Support/JavaScript.cpp | 90 ++++++ 8 files changed, 493 insertions(+), 143 deletions(-) create mode 100644 share/mrdocs/addons/generator/html/helpers/add.js diff --git a/CMakeUserPresets.json.example b/CMakeUserPresets.json.example index 4167f0e30..36c8a3f78 100644 --- a/CMakeUserPresets.json.example +++ b/CMakeUserPresets.json.example @@ -60,6 +60,15 @@ "Clang_ROOT": "C:\\Users\\$env{USERNAME}\\Libraries\\llvm-project\\llvm\\install\\MSVC\\RelWithDebInfo" } }, + { + "name": "DebWithOpt-ClangCL", + "inherits": "DebWithOpt-MSVC", + "binaryDir": "${sourceDir}/build/${presetName}", + "cacheVariables": { + "CMAKE_C_COMPILER": "clang-cl.exe", + "CMAKE_CXX_COMPILER": "clang-cl.exe" + } + }, { "name": "Debug-GCC", "inherits": "debug", diff --git a/include/mrdocs/Support/JavaScript.hpp b/include/mrdocs/Support/JavaScript.hpp index 702d89c20..27d6ae6e6 100644 --- a/include/mrdocs/Support/JavaScript.hpp +++ b/include/mrdocs/Support/JavaScript.hpp @@ -20,6 +20,9 @@ namespace clang { namespace mrdocs { + +class Handlebars; + namespace js { struct Access; @@ -214,6 +217,42 @@ class Scope MRDOCS_DECL ~Scope(); + /** Push an integer to the stack + */ + MRDOCS_DECL + Value + pushInteger(std::int64_t value); + + /** Push a double to the stack + */ + MRDOCS_DECL + Value + pushDouble(double value); + + /** Push a boolean to the stack + */ + MRDOCS_DECL + Value + pushBoolean(bool value); + + /** Push a string to the stack + */ + MRDOCS_DECL + Value + pushString(std::string_view value); + + /** Push a new object to the stack + */ + MRDOCS_DECL + Value + pushObject(); + + /** Push a new array to the stack + */ + MRDOCS_DECL + Value + pushArray(); + /** Compile and run a script. This function compiles and executes @@ -973,6 +1012,20 @@ isFunction() const noexcept return type() == Type::function; } +/** Register a JavaScript helper function + + This function registers a JavaScript function + as a helper function that can be called from + Handlebars templates. + */ +MRDOCS_DECL +Expected +registerHelper( + clang::mrdocs::Handlebars& hbs, + std::string_view name, + Context& ctx, + std::string_view script); + } // js } // mrdocs } // clang diff --git a/include/mrdocs/Support/Path.hpp b/include/mrdocs/Support/Path.hpp index e9520fc1d..6690272f9 100644 --- a/include/mrdocs/Support/Path.hpp +++ b/include/mrdocs/Support/Path.hpp @@ -44,32 +44,80 @@ forEachFile( bool recursive, AnyFileVisitor& visitor); -/** Visit each file in a directory. -*/ -template -Error -forEachFile( - std::string_view dirPath, - bool recursive, - Visitor&& visitor) +namespace detail { +template +struct FileVisitor : AnyFileVisitor { - struct FileVisitor : AnyFileVisitor + Visitor& visitor_; + + explicit FileVisitor(Visitor& v) + : visitor_(v) { - Visitor& visitor_; + } - explicit FileVisitor(Visitor& v) - : visitor_(v) + Error + visitFile(std::string_view fileName) override + { + using R = std::invoke_result_t; + if (std::same_as) + { + visitor_(fileName); + return Error::success(); + } + else { + return toError(visitor_(fileName)); } + } + + static + Error + toError(Expected const& e) + { + return e ? Error::success() : e.error(); + } + + template + static + Error + toError(Expected const& e) + { + return e ? toError(e.value()) : e.error(); + } - Error - visitFile(std::string_view fileName) override + template + static + Error + toError(T const& e) + { + if constexpr (std::same_as) + { + return e; + } + else if constexpr (std::convertible_to) + { + if (e) + return Error::success(); + return Error("visitor returned falsy"); + } + else { - return visitor_(fileName); + return Error::success(); } - }; + } +}; +} - FileVisitor v{visitor}; +/** Visit each file in a directory. +*/ +template +Error +forEachFile( + std::string_view dirPath, + bool recursive, + Visitor&& visitor) +{ + detail::FileVisitor v{visitor}; return forEachFile(dirPath, recursive, static_cast(v)); } diff --git a/share/mrdocs/addons/generator/html/helpers/add.js b/share/mrdocs/addons/generator/html/helpers/add.js new file mode 100644 index 000000000..85f080e7b --- /dev/null +++ b/share/mrdocs/addons/generator/html/helpers/add.js @@ -0,0 +1,3 @@ +function add(a, b) { + return a + b; +} \ No newline at end of file diff --git a/src/lib/Gen/adoc/Builder.cpp b/src/lib/Gen/adoc/Builder.cpp index 6d97a2787..a4d17798f 100644 --- a/src/lib/Gen/adoc/Builder.cpp +++ b/src/lib/Gen/adoc/Builder.cpp @@ -57,75 +57,28 @@ Builder( return Error::success(); }).maybeThrow(); - // load helpers - js::Scope scope(ctx_); + // Load JavaScript helpers std::string helpersPath = files::appendPath( config->addonsDir, "generator", "asciidoc", "helpers"); forEachFile(helpersPath, true, - [&](std::string_view pathName) + [&](std::string_view pathName)-> Expected { // Register JS helper function in the global object constexpr std::string_view ext = ".js"; - if (!pathName.ends_with(ext)) - { - return Error::success(); - } + if (!pathName.ends_with(ext)) return {}; auto name = files::getFileName(pathName); name.remove_suffix(ext.size()); - auto text = files::getFileText(pathName); - if (!text) - { - return text.error(); - } - auto JSFn = scope.compile_function(*text); - if (!JSFn) - { - return JSFn.error(); - } - scope.getGlobalObject().set(name, *JSFn); - - // Register C++ helper that retrieves the helper - // from the global object, converts the arguments, - // and invokes the JS function. - hbs_.registerHelper(name, dom::makeVariadicInvocable( - [this, name=std::string(name)]( - dom::Array const& args) -> Expected - { - // Get function from global scope - js::Scope scope(ctx_); - js::Value fn = scope.getGlobalObject().get(name); - if (fn.isUndefined()) - { - return Unexpected(Error("helper not found")); - } - if (!fn.isFunction()) - { - return Unexpected(Error("helper is not a function")); - } - - // Call function - std::vector arg_span; - arg_span.reserve(args.size()); - for (auto const& arg : args) - { - arg_span.push_back(arg); - } - auto result = fn.apply(arg_span); - if (!result) - { - return dom::Kind::Undefined; - } - - // Convert result to dom::Value - return result->getDom(); - })); - return Error::success(); + MRDOCS_TRY(auto script, files::getFileText(pathName)); + MRDOCS_TRY(js::registerHelper(hbs_, name, ctx_, script)); + return {}; }).maybeThrow(); + hbs_.registerHelper( "is_multipage", dom::makeInvocable([res = config->multiPage]() -> Expected { return res; })); + hbs_.registerHelper("primary_location", dom::makeInvocable([](dom::Value const& v) -> dom::Value @@ -162,6 +115,7 @@ Builder( } return first; })); + helpers::registerStringHelpers(hbs_); helpers::registerAntoraHelpers(hbs_); helpers::registerContainerHelpers(hbs_); @@ -183,8 +137,6 @@ callTemplate( MRDOCS_TRY(auto fileText, files::getFileText(pathName)); HandlebarsOptions options; options.noEscape = true; - // options.compat = true; - Expected exp = hbs_.try_render(fileText, context, options); if (!exp) diff --git a/src/lib/Gen/html/Builder.cpp b/src/lib/Gen/html/Builder.cpp index 9473012d0..c0ff86754 100644 --- a/src/lib/Gen/html/Builder.cpp +++ b/src/lib/Gen/html/Builder.cpp @@ -60,70 +60,23 @@ Builder( return Error::success(); }).maybeThrow(); - // load helpers - js::Scope scope(ctx_); + // Load JavaScript helpers std::string helpersPath = files::appendPath( - config->addonsDir, "generator", "asciidoc", "helpers"); + config->addonsDir, "generator", "html", "helpers"); forEachFile(helpersPath, true, - [&](std::string_view pathName) - { - // Register JS helper function in the global object - constexpr std::string_view ext = ".js"; - if (!pathName.ends_with(ext)) - { - return Error::success(); - } - auto name = files::getFileName(pathName); - name.remove_suffix(ext.size()); - auto text = files::getFileText(pathName); - if (!text) + [&](std::string_view pathName)-> Expected { - return text.error(); - } - auto JSFn = scope.compile_function(*text); - if (!JSFn) - { - return JSFn.error(); - } - scope.getGlobalObject().set(name, *JSFn); - - // Register C++ helper that retrieves the helper - // from the global object, converts the arguments, - // and invokes the JS function. - hbs_.registerHelper(name, dom::makeVariadicInvocable( - [this, name=std::string(name)]( - dom::Array const& args) -> Expected - { - // Get function from global scope - js::Scope scope(ctx_); - js::Value fn = scope.getGlobalObject().get(name); - if (fn.isUndefined()) - { - return Unexpected(Error("helper not found")); - } - if (!fn.isFunction()) - { - return Unexpected(Error("helper is not a function")); - } - - // Call function - std::vector arg_span; - arg_span.reserve(args.size()); - for (auto const& arg : args) - { - arg_span.push_back(arg); - } - auto result = fn.apply(arg_span); - if (!result) - { - return dom::Kind::Undefined; - } - - // Convert result to dom::Value - return result->getDom(); - })); - return Error::success(); - }).maybeThrow(); + // Register JS helper function in the global object + constexpr std::string_view ext = ".js"; + if (!pathName.ends_with(ext)) + return {}; + auto name = files::getFileName(pathName); + name.remove_suffix(ext.size()); + MRDOCS_TRY(auto script, files::getFileText(pathName)); + MRDOCS_TRY(js::registerHelper(hbs_, name, ctx_, script)); + return {}; + }).maybeThrow(); + hbs_.registerHelper( "is_multipage", dom::makeInvocable([res = config->multiPage]() -> Expected { diff --git a/src/lib/Support/JavaScript.cpp b/src/lib/Support/JavaScript.cpp index d8ad0789d..baa80c0b2 100644 --- a/src/lib/Support/JavaScript.cpp +++ b/src/lib/Support/JavaScript.cpp @@ -12,6 +12,7 @@ #include "lib/Support/Error.hpp" #include #include +#include #include #include #include @@ -59,42 +60,61 @@ Context( ++impl_->refs; } +/* Access to the underlying duktape context in + Context and Scope. + */ struct Access { duk_context* ctx_ = nullptr; - Context::Impl* impl_ = nullptr;; + Context::Impl* impl_ = nullptr; + // Access from an original duktape context explicit Access(duk_context* ctx) noexcept : ctx_(ctx) { } + // Access from a Context explicit Access(Context const& ctx) noexcept : ctx_(ctx.impl_->ctx) , impl_(ctx.impl_) { } + // Access from a Scope explicit Access(Scope const& scope) noexcept : Access(scope.ctx_) { } + // Implicit conversion to a duktape context + // for use with the duktape C API operator duk_context*() const noexcept { return ctx_; } + // Access to a value idx in its Scope static duk_idx_t idx(Value const& value) noexcept { return value.idx_; } + // Mark a scope as referenced by another + // scope or Value + // This is used to keep the scope alive + // while it is being used and to + // destroy it when it is no longer needed static void addref(Scope& scope) noexcept { ++scope.refs_; } + // Mark a scope as referenced by one less + // scope or Value + // This is used to keep the scope alive + // while it is being used and to + // destroy it when it is no longer needed static void release(Scope& scope) noexcept { if(--scope.refs_ != 0) @@ -306,6 +326,60 @@ setGlobal( this->getGlobalObject().set(name, value); } +Value +Scope:: +pushInteger(std::int64_t value) +{ + Access A(*this); + duk_push_int(A, value); + return Access::construct(-1, *this); +} + +Value +Scope:: +pushDouble(double value) +{ + Access A(*this); + duk_push_number(A, value); + return Access::construct(-1, *this); +} + +Value +Scope:: +pushBoolean(bool value) +{ + Access A(*this); + duk_push_boolean(A, value); + return Access::construct(-1, *this); +} + +Value +Scope:: +pushString(std::string_view value) +{ + Access A(*this); + duk_push_lstring(A, value.data(), value.size()); + return Access::construct(-1, *this); +} + +Value +Scope:: +pushObject() +{ + Access A(*this); + duk_push_object(A); + return Access::construct(-1, *this); +} + +Value +Scope:: +pushArray() +{ + Access A(*this); + duk_push_array(A); + return Access::construct(-1, *this); +} + //------------------------------------------------ // // JS -> C++ dom::Value bindings @@ -318,9 +392,16 @@ class JSObjectImpl : public dom::ObjectImpl { Access A_; duk_idx_t idx_; + std::shared_ptr scope_; public: - ~JSObjectImpl() override = default; + ~JSObjectImpl() override + { + if (scope_) + { + Access::release(*scope_); + } + } JSObjectImpl( Scope& scope, duk_idx_t idx) noexcept @@ -369,15 +450,33 @@ class JSObjectImpl : public dom::ObjectImpl { return idx_; } + + // Set a shared pointer to the Scope so that it + // can temporarily outlive the variable + void + setScope(std::shared_ptr scope) noexcept + { + MRDOCS_ASSERT(scope); + MRDOCS_ASSERT(Access(*scope.get()).ctx_ == A_.ctx_); + scope_ = std::move(scope); + Access::addref(*scope_); + } }; class JSArrayImpl : public dom::ArrayImpl { Access A_; duk_idx_t idx_; + std::shared_ptr scope_; public: - ~JSArrayImpl() override = default; + ~JSArrayImpl() override + { + if (scope_) + { + Access::release(*scope_); + } + } JSArrayImpl( Scope& scope, duk_idx_t idx) noexcept @@ -423,6 +522,17 @@ class JSArrayImpl : public dom::ArrayImpl { return idx_; } + + // Set a shared pointer to the Scope so that it + // can temporarily outlive the variable + void + setScope(std::shared_ptr scope) noexcept + { + MRDOCS_ASSERT(scope); + MRDOCS_ASSERT(Access(*scope.get()).ctx_ == A_.ctx_); + scope_ = std::move(scope); + Access::addref(*scope_); + } }; // A JavaScript function defined in the scope as a dom::Function @@ -430,8 +540,16 @@ class JSFunctionImpl : public dom::FunctionImpl { Access A_; duk_idx_t idx_; + std::shared_ptr scope_; + public: - ~JSFunctionImpl() override = default; + ~JSFunctionImpl() override + { + if (scope_) + { + Access::release(*scope_); + } + } JSFunctionImpl( Scope& scope, duk_idx_t idx) noexcept @@ -467,6 +585,17 @@ class JSFunctionImpl : public dom::FunctionImpl { return idx_; } + + // Set a shared pointer to the Scope so that it + // can temporarily outlive the variable + void + setScope(std::shared_ptr scope) noexcept + { + MRDOCS_ASSERT(scope); + MRDOCS_ASSERT(Access(*scope.get()).ctx_ == A_.ctx_); + scope_ = std::move(scope); + Access::addref(*scope_); + } }; } // (anon) @@ -992,15 +1121,18 @@ domValue_get( { if (duk_is_array(A, idx)) { - return {dom::newArray(A, idx)}; + duk_dup(A, idx); + return {dom::newArray(A, duk_get_top_index(A))}; } if (duk_is_function(A, idx)) { - return {dom::newFunction(A, idx)}; + duk_dup(A, idx); + return {dom::newFunction(A, duk_get_top_index(A))}; } if (duk_is_object(A, idx)) { - return {dom::newObject(A, idx)}; + duk_dup(A, idx); + return {dom::newObject(A, duk_get_top_index(A))}; } return nullptr; } @@ -1179,6 +1311,13 @@ JSArrayImpl:: size() const { Access A(A_); + auto t = duk_get_type(A, idx_); + if (t != DUK_TYPE_OBJECT && scope_) + { + auto top = duk_get_top(A); + MRDOCS_ASSERT(top > 0); + } + MRDOCS_ASSERT(t == DUK_TYPE_OBJECT); MRDOCS_ASSERT(duk_is_array(A, idx_)); return duk_get_length(A, idx_); } @@ -1221,7 +1360,7 @@ Value:: if( ! scope_) return; Access A(*scope_); - if(idx_ == duk_get_top(A) - 1) + if (idx_ == duk_get_top(A) - 1) duk_pop(A); Access::release(*scope_); } @@ -1741,6 +1880,109 @@ operator&&(Value const& lhs, Value const& rhs) return rhs; } +Expected +registerHelper( + clang::mrdocs::Handlebars& hbs, + std::string_view name, + Context& ctx, + std::string_view script) +{ + // Register the compiled helper function in the global scope + constexpr auto global_helpers_key = DUK_HIDDEN_SYMBOL("MrDocsHelpers"); + { + Scope s(ctx); + Value g = s.getGlobalObject(); + MRDOCS_ASSERT(g.isObject()); + if (!g.exists(global_helpers_key)) + { + Value obj = s.pushObject(); + MRDOCS_ASSERT(obj.isObject()); + g.set(global_helpers_key, obj); + } + Value helpers = g.get(global_helpers_key); + MRDOCS_ASSERT(helpers.isObject()); + MRDOCS_TRY(Value JSFn, s.compile_function(script)); + if (!JSFn.isFunction()) + { + return Unexpected(Error(fmt::format( + "helper \"{}\" is not a function", name))); + } + helpers.set(name, JSFn); + } + + // Register C++ helper that retrieves the helper + // from the global object, converts the arguments, + // and invokes the JS function. + hbs.registerHelper(name, dom::makeVariadicInvocable( + [&ctx, global_helpers_key, name=std::string(name)]( + dom::Array const& args) -> Expected + { + // Get function from global scope + auto s = std::make_shared(ctx); + Value g = s->getGlobalObject(); + MRDOCS_ASSERT(g.isObject()); + MRDOCS_ASSERT(g.exists(global_helpers_key)); + Value helpers = g.get(global_helpers_key); + MRDOCS_ASSERT(helpers.isObject()); + Value fn = helpers.get(name); + MRDOCS_ASSERT(fn.isFunction()); + + // Call function + std::vector arg_span; + arg_span.reserve(args.size()); + for (auto const& arg : args) + { + arg_span.push_back(arg); + } + auto JSResult = fn.apply(arg_span); + if (!JSResult) + { + return dom::Kind::Undefined; + } + + // Convert result to dom::Value + dom::Value result = JSResult->getDom(); + const bool isPrimitive = + !result.isObject() && + !result.isArray() && + !result.isFunction(); + if (isPrimitive) + { + return result; + } + + // Non-primitive values need to keep the + // JS scope alive until the value is used + // by the Handlebars engine. + auto setScope = [&s](auto& result, auto TI) + { + using T = typename std::decay_t::type; + auto* impl = dynamic_cast(result.impl().get()); + MRDOCS_ASSERT(impl); + impl->setScope(s); + }; + if (result.isObject()) + { + setScope( + result.getObject(), + std::type_identity{}); + } + else if (result.isArray()) + { + setScope( + result.getArray(), + std::type_identity{}); + } + else if (result.isFunction()) + { + setScope( + result.getFunction(), + std::type_identity{}); + } + return result; + })); + return {}; +} } // js } // mrdocs diff --git a/src/test/Support/JavaScript.cpp b/src/test/Support/JavaScript.cpp index 77338913a..e8e7ec9e3 100644 --- a/src/test/Support/JavaScript.cpp +++ b/src/test/Support/JavaScript.cpp @@ -9,6 +9,7 @@ // #include +#include #include #include @@ -37,6 +38,41 @@ struct JavaScript_test Scope scope(ctx); } + // Push values directly + { + Scope scope(ctx); + + // pushInteger(std::int64_t value); + Value a = scope.pushInteger(1); + BOOST_TEST(a.isInteger()); + BOOST_TEST(a.getDom() == 1); + + // pushDouble(double value); + Value b = scope.pushDouble(1.5); + BOOST_TEST(b.isDouble()); + BOOST_TEST(b.getDom() == 1.5); + + // pushBoolean(bool value); + Value c = scope.pushBoolean(true); + BOOST_TEST(c.isBoolean()); + BOOST_TEST(c.getDom() == true); + + // pushString(std::string_view value); + Value e = scope.pushString("hello world"); + BOOST_TEST(e.isString()); + BOOST_TEST(e.getDom() == "hello world"); + + // pushObject(); + Value f = scope.pushObject(); + BOOST_TEST(f.isObject()); + BOOST_TEST(f.getDom().isObject()); + + // pushArray(); + Value g = scope.pushArray(); + BOOST_TEST(g.isArray()); + BOOST_TEST(g.getDom().isArray()); + } + // script { Scope scope(ctx); @@ -1199,6 +1235,59 @@ struct JavaScript_test } } + void + test_hbs_helpers() + { + Handlebars hbs; + js::Context ctx; + + // Primitive types + { + // Number + js::registerHelper(hbs, "add", ctx, "function(a, b) { return a + b; }"); + BOOST_TEST(hbs.render("{{add 1 2}}") == "3"); + js::registerHelper(hbs, "sub", ctx, "function(a, b) { return a - b; }"); + BOOST_TEST(hbs.render("{{sub 3 2}}") == "1"); + + // String + js::registerHelper(hbs, "concat", ctx, "function(a, b) { return a + b; }"); + BOOST_TEST(hbs.render("{{concat 'a' 'b'}}") == "ab"); + + // Boolean + js::registerHelper(hbs, "and", ctx, "function(a, b) { return a && b; }"); + BOOST_TEST(hbs.render("{{and true true}}") == "true"); + + // Undefined + js::registerHelper(hbs, "undef", ctx, "function() { return undefined; }"); + BOOST_TEST(hbs.render("{{undef}}") == ""); + + // Null + js::registerHelper(hbs, "null", ctx, "function() { return null; }"); + BOOST_TEST(hbs.render("{{null}}") == ""); + } + + // Reference types + { + // Object + js::registerHelper(hbs, "obj", ctx, "function() { return { a: 1 }; }"); + BOOST_TEST(hbs.render("{{obj}}") == "[object Object]"); + + // Array + js::registerHelper(hbs, "arr", ctx, "function() { return [1, 2, 3]; }"); + BOOST_TEST(hbs.render("{{arr}}") == "[1,2,3]"); + + // Function + js::registerHelper(hbs, "fn", ctx, "function() { return function() {}; }"); + BOOST_TEST(hbs.render("{{fn}}") == ""); + } + + // Access helper options from JavaScript + { + js::registerHelper(hbs, "opt", ctx, "function(options) { return options.hash.a; }"); + BOOST_TEST(hbs.render("{{opt a=1}}") == "1"); + } + } + void run() { test_context(); @@ -1207,6 +1296,7 @@ struct JavaScript_test test_cpp_function(); test_cpp_object(); test_cpp_array(); + test_hbs_helpers(); } };