Refactor ARGV handling and add system.programArgs.ewlsh/refactor-argv

(Changes from Philip folded in: use native function with reserved slot to
store built argv array)

10 files changed, 122 insertions, 10 deletions

diff --git a/gjs/atoms.h b/gjs/atoms.h
index 424f6ea2..25c9904d 100644
--- a/gjs/atoms.h
+++ b/gjs/atoms.h
@@ -52,6 +52,7 @@ class JSTracer;
     macro(overrides, "overrides") \
     macro(param_spec, "ParamSpec") \
     macro(parent_module, "__parentModule__") \
+    macro(program_args, "programArgs") \
     macro(program_invocation_name, "programInvocationName") \
     macro(program_path, "programPath") \
     macro(prototype, "prototype") \
diff --git a/gjs/console.cpp b/gjs/console.cpp
index 7c39f39e..49c82299 100644
--- a/gjs/console.cpp
+++ b/gjs/console.cpp
@@ -165,16 +165,9 @@ check_script_args_for_stray_gjs_args(int           argc,
 int define_argv_and_eval_script(GjsContext* js_context, int argc,
                                 char* const* argv, const char* script,
                                 size_t len, const char* filename) {
-    GError* error = nullptr;
-
-    /* prepare command line arguments */
-    if (!gjs_context_define_string_array(
-            js_context, "ARGV", argc, const_cast<const char**>(argv), &error)) {
-        g_critical("Failed to define ARGV: %s", error->message);
-        g_clear_error(&error);
-        return 1;
-    }
+    gjs_context_set_argv(js_context, argc, const_cast<const char**>(argv));
 
+    GError* error = nullptr;
     /* evaluate the script */
     int code = 0;
     if (exec_as_module) {
diff --git a/gjs/context-private.h b/gjs/context-private.h
index 6f5cba47..b0746192 100644
--- a/gjs/context-private.h
+++ b/gjs/context-private.h
@@ -10,8 +10,10 @@
 #include <stdint.h>
 #include <sys/types.h>  // for ssize_t
 
+#include <string>
 #include <type_traits>  // for is_same
 #include <unordered_map>
+#include <vector>
 
 #include <glib-object.h>
 #include <glib.h>
@@ -83,6 +85,8 @@ class GjsContextPrivate : public JS::JobQueue {
 
     GjsAtoms* m_atoms;
 
+    std::vector<std::string> m_args;
+
     JobQueueStorage m_job_queue;
     unsigned m_idle_drain_handler;
 
@@ -189,6 +193,8 @@ class GjsContextPrivate : public JS::JobQueue {
     void set_should_listen_sigusr2(bool value) {
         m_should_listen_sigusr2 = value;
     }
+    void set_args(std::vector<std::string>&& args);
+    GJS_JSAPI_RETURN_CONVENTION JSObject* build_args_array();
     [[nodiscard]] bool is_owner_thread() const {
         return m_owner_thread == g_thread_self();
     }
diff --git a/gjs/context.cpp b/gjs/context.cpp
index 59310a69..d54ab6ef 100644
--- a/gjs/context.cpp
+++ b/gjs/context.cpp
@@ -599,6 +599,14 @@ GjsContextPrivate::GjsContextPrivate(JSContext* cx, GjsContext* public_context)
     }
 }
 
+void GjsContextPrivate::set_args(std::vector<std::string>&& args) {
+    m_args = args;
+}
+
+JSObject* GjsContextPrivate::build_args_array() {
+    return gjs_build_string_array(m_cx, m_args);
+}
+
 static void
 gjs_context_get_property (GObject     *object,
                           guint        prop_id,
@@ -1337,6 +1345,13 @@ gjs_context_define_string_array(GjsContext  *js_context,
         strings = {array_values, array_values + array_length};
     }
 
+    // ARGV is a special case to preserve backwards compatibility.
+    if (strcmp(array_name, "ARGV") == 0) {
+        gjs->set_args(std::move(strings));
+
+        return true;
+    }
+
     JS::RootedObject global_root(gjs->context(), gjs->global());
     if (!gjs_define_string_array(gjs->context(), global_root, array_name,
                                  strings, JSPROP_READONLY | JSPROP_PERMANENT)) {
@@ -1351,6 +1366,14 @@ gjs_context_define_string_array(GjsContext  *js_context,
     return true;
 }
 
+void gjs_context_set_argv(GjsContext* js_context, ssize_t array_length,
+                          const char** array_values) {
+    g_return_if_fail(GJS_IS_CONTEXT(js_context));
+    GjsContextPrivate* gjs = GjsContextPrivate::from_object(js_context);
+    std::vector<std::string> args(array_values, array_values + array_length);
+    gjs->set_args(std::move(args));
+}
+
 static GjsContext *current_context;
 
 GjsContext *
diff --git a/gjs/context.h b/gjs/context.h
index 380b98b2..d2ed9eb4 100644
--- a/gjs/context.h
+++ b/gjs/context.h
@@ -13,6 +13,7 @@
 
 #include <stdbool.h>    /* IWYU pragma: keep */
 #include <stdint.h>
+#include <sys/types.h> /* for ssize_t */
 
 #ifndef _WIN32
 #    include <signal.h> /* for siginfo_t */
@@ -65,6 +66,10 @@ GJS_EXPORT GJS_USE bool gjs_context_define_string_array(
     GjsContext* js_context, const char* array_name, gssize array_length,
     const char** array_values, GError** error);
 
+GJS_EXPORT void gjs_context_set_argv(GjsContext* js_context,
+                                     ssize_t array_length,
+                                     const char** array_values);
+
 GJS_EXPORT GJS_USE GList* gjs_context_get_all(void);
 
 GJS_EXPORT GJS_USE GjsContext* gjs_context_get_current(void);
diff --git a/installed-tests/js/testSystem.js b/installed-tests/js/testSystem.js
index e805f7bb..ce4963db 100644
--- a/installed-tests/js/testSystem.js
+++ b/installed-tests/js/testSystem.js
@@ -61,3 +61,20 @@ describe('System.programPath', function () {
         expect(System.programPath).toBe(null);
     });
 });
+
+describe('System.programArgs', function () {
+    it('System.programArgs is an array', function () {
+        expect(Array.isArray(System.programArgs)).toBeTruthy();
+    });
+
+    it('modifications persist', function () {
+        System.programArgs.push('--foo');
+        expect(System.programArgs.pop()).toBe('--foo');
+    });
+
+    it('System.programArgs is equal to ARGV', function () {
+        expect(System.programArgs).toEqual(ARGV);
+        ARGV.push('--foo');
+        expect(System.programArgs.pop()).toBe('--foo');
+    });
+});
diff --git a/modules/esm/system.js b/modules/esm/system.js
index c12d8de5..7143e8a3 100644
--- a/modules/esm/system.js
+++ b/modules/esm/system.js
@@ -9,6 +9,7 @@ export let {
     clearDateCaches,
     exit,
     gc,
+    programArgs,
     programInvocationName,
     programPath,
     refcount,
@@ -21,6 +22,7 @@ export default {
     clearDateCaches,
     exit,
     gc,
+    programArgs,
     programInvocationName,
     programPath,
     refcount,
diff --git a/modules/script/_bootstrap/default.js b/modules/script/_bootstrap/default.js
index e31d80cb..952d7fe3 100644
--- a/modules/script/_bootstrap/default.js
+++ b/modules/script/_bootstrap/default.js
@@ -8,6 +8,14 @@
     const {print, printerr, log, logError} = imports._print;
 
     Object.defineProperties(exports, {
+        ARGV: {
+            configurable: false,
+            enumerable: true,
+            get() {
+                // Wait until after bootstrap or programArgs won't be set.
+                return imports.system.programArgs;
+            },
+        },
         print: {
             configurable: false,
             enumerable: true,
diff --git a/modules/system.cpp b/modules/system.cpp
index 936584c2..8c321d45 100644
--- a/modules/system.cpp
+++ b/modules/system.cpp
@@ -193,6 +193,30 @@ static JSFunctionSpec module_funcs[] = {
     JS_FN("clearDateCaches", gjs_clear_date_caches, 0, GJS_MODULE_PROP_FLAGS),
     JS_FS_END};
 
+static bool get_program_args(JSContext* cx, unsigned argc, JS::Value* vp) {
+    static const size_t SLOT_ARGV = 0;
+
+    JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
+    GjsContextPrivate* priv = GjsContextPrivate::from_cx(cx);
+
+    JS::RootedValue v_argv(
+        cx, js::GetFunctionNativeReserved(&args.callee(), SLOT_ARGV));
+
+    if (v_argv.isUndefined()) {
+        // First time this property is accessed, build the array
+        JS::RootedObject argv(cx, priv->build_args_array());
+        if (!argv)
+            return false;
+        js::SetFunctionNativeReserved(&args.callee(), SLOT_ARGV,
+                                      JS::ObjectValue(*argv));
+        args.rval().setObject(*argv);
+    } else {
+        args.rval().set(v_argv);
+    }
+
+    return true;
+}
+
 bool
 gjs_js_define_system_stuff(JSContext              *context,
                            JS::MutableHandleObject module)
@@ -213,7 +237,13 @@ gjs_js_define_system_stuff(JSContext              *context,
             return false;
     }
 
-    return gjs_string_from_utf8(context, program_name,
+    JS::RootedObject program_args_getter(
+        context,
+        JS_GetFunctionObject(js::NewFunctionByIdWithReserved(
+            context, get_program_args, 0, 0, gjs->atoms().program_args())));
+
+    return program_args_getter &&
+           gjs_string_from_utf8(context, program_name,
                                 &v_program_invocation_name) &&
            /* The name is modeled after program_invocation_name, part of glibc
             */
@@ -224,6 +254,9 @@ gjs_js_define_system_stuff(JSContext              *context,
            JS_DefinePropertyById(context, module, gjs->atoms().program_path(),
                                  v_program_path,
                                  GJS_MODULE_PROP_FLAGS | JSPROP_READONLY) &&
+           JS_DefinePropertyById(context, module, gjs->atoms().program_args(),
+                                 program_args_getter, nullptr,
+                                 GJS_MODULE_PROP_FLAGS | JSPROP_GETTER) &&
            JS_DefinePropertyById(context, module, gjs->atoms().version(),
                                  GJS_VERSION,
                                  GJS_MODULE_PROP_FLAGS | JSPROP_READONLY);
diff --git a/test/gjs-tests.cpp b/test/gjs-tests.cpp
index d8a32102..e63fc430 100644
--- a/test/gjs-tests.cpp
+++ b/test/gjs-tests.cpp
@@ -811,6 +811,28 @@ static void gjstest_test_args_rounded_values() {
     assert_equal(gjs_arg_get_maybe_rounded<uint64_t>(&arg), 0.0);
 }
 
+static void gjstest_test_func_gjs_context_argv_array() {
+    GjsAutoUnref<GjsContext> gjs = gjs_context_new();
+    GError* error = NULL;
+    int status;
+
+    const char* argv[1] = {"test"};
+    bool ok = gjs_context_define_string_array(gjs, "ARGV", 1, argv, &error);
+
+    g_assert_no_error(error);
+    g_clear_error(&error);
+    g_assert_true(ok);
+
+    ok = gjs_context_eval(gjs, R"js(
+        imports.system.exit(ARGV[0] === "test" ? 0 : 1)
+    )js",
+                          -1, "<main>", &status, &error);
+
+    g_assert_cmpint(status, ==, 0);
+    g_assert_error(error, GJS_ERROR, GJS_ERROR_SYSTEM_EXIT);
+    g_assert_false(ok);
+}
+
 int
 main(int    argc,
      char **argv)
@@ -840,6 +862,8 @@ main(int    argc,
 
     g_test_add_func("/gjs/context/construct/destroy", gjstest_test_func_gjs_context_construct_destroy);
     g_test_add_func("/gjs/context/construct/eval", gjstest_test_func_gjs_context_construct_eval);
+    g_test_add_func("/gjs/context/argv",
+                    gjstest_test_func_gjs_context_argv_array);
     g_test_add_func("/gjs/context/eval/dynamic-import",
                     gjstest_test_func_gjs_context_eval_dynamic_import);
     g_test_add_func("/gjs/context/eval/dynamic-import/relative",