Merge pull request #1134 from svaarala/allow-bound-func-for-thread

Allow bound function as initial Duktape.Thread function

RELEASES.rst

@@ -1905,6 +1905,9 @@ Planned
 * Remove duk_{get,put,has,del}_var() calls from API header; they were not
   fully implemented and not part of the documented public API (GH-762)
 
+* Allow a bound Ecmascript function as an argument to new Duktape.Thread()
+  (GH-1134)
+
 * Minor changes to error messages for errors thrown by Duktape internals
   (GH-827, GH-839, GH-840, GH-1016)
 

doc/testcase-known-issues.yaml

@@ -56,10 +56,6 @@
   test: "test-bug-tonumber-u0000.js"
   knownissue: "'\\u0000' should ToNumber() coerce to NaN, but now coerces to zero like an empty string"
   # Moved
--
-  test: "test-dev-bound-thread-start-func.js"
-  knownissue: "initial function of a new coroutine cannot be bound"
-  # Moved
 -
   test: "test-dev-func-cons-args.js"
   knownissue: "corner cases for 'new Function()' when arguments and code are given as strings"

src-input/duk_api_call.c

@@ -576,3 +576,33 @@ DUK_EXTERNAL void duk_set_magic(duk_context *ctx, duk_idx_t idx, duk_int_t magic
 	DUK_ASSERT(nf != NULL);
 	nf->magic = (duk_int16_t) magic;
 }
+
+/*
+ *  Misc helpers
+ */
+
+DUK_INTERNAL void duk_resolve_nonbound_function(duk_context *ctx) {
+	duk_uint_t sanity;
+	duk_tval *tv;
+
+	sanity = DUK_HOBJECT_BOUND_CHAIN_SANITY;
+	do {
+		tv = DUK_GET_TVAL_NEGIDX(ctx, -1);
+		if (DUK_TVAL_IS_LIGHTFUNC(tv)) {
+			/* Lightweight function: never bound, so terminate. */
+			break;
+		} else if (DUK_TVAL_IS_OBJECT(tv)) {
+			duk_hobject *func;
+
+			func = DUK_TVAL_GET_OBJECT(tv);
+			DUK_ASSERT(func != NULL);
+			if (!DUK_HOBJECT_IS_CALLABLE(func) || !DUK_HOBJECT_HAS_BOUNDFUNC(func)) {
+				break;
+			}
+			duk_get_prop_stridx(ctx, -1, DUK_STRIDX_INT_TARGET);
+			duk_replace(ctx, -2);
+		} else {
+			break;
+		}
+	} while (--sanity > 0);
+}

src-input/duk_api_internal.h

@@ -211,7 +211,10 @@ DUK_INTERNAL_DECL void duk_require_constructor_call(duk_context *ctx);
 
 DUK_INTERNAL_DECL void duk_require_constructable(duk_context *ctx, duk_idx_t idx);
 
+DUK_INTERNAL_DECL void duk_resolve_nonbound_function(duk_context *ctx);
+
 DUK_INTERNAL_DECL duk_idx_t duk_get_top_index_unsafe(duk_context *ctx);
+DUK_INTERNAL_DECL void duk_pop_unsafe(duk_context *ctx);
 
 /* Raw internal valstack access macros: access is unsafe so call site
  * must have a guarantee that the index is valid.  When that is the case,

src-input/duk_api_stack.c

@@ -4503,6 +4503,34 @@ DUK_EXTERNAL void duk_pop(duk_context *ctx) {
 }
 #endif  /* !DUK_USE_PREFER_SIZE */
 
+/* Unsafe internal variant which assumes there are enough values on the value
+ * stack so that a top check can be skipped safely.
+ */
+#if defined(DUK_USE_PREFER_SIZE)
+DUK_INTERNAL void duk_pop_unsafe(duk_context *ctx) {
+	DUK_ASSERT_CTX_VALID(ctx);
+	duk_pop_n(ctx, 1);
+}
+#else
+DUK_INTERNAL void duk_pop_unsafe(duk_context *ctx) {
+	duk_hthread *thr = (duk_hthread *) ctx;
+	duk_tval *tv;
+	DUK_ASSERT_CTX_VALID(ctx);
+
+	DUK_ASSERT(thr->valstack_top >= thr->valstack_bottom);
+	DUK_ASSERT(thr->valstack_top != thr->valstack_bottom);
+
+	tv = --thr->valstack_top;  /* tv points to element just below prev top */
+	DUK_ASSERT(tv >= thr->valstack_bottom);
+#ifdef DUK_USE_REFERENCE_COUNTING
+	DUK_TVAL_SET_UNDEFINED_UPDREF(thr, tv);  /* side effects */
+#else
+	DUK_TVAL_SET_UNDEFINED(tv);
+#endif
+	DUK_ASSERT(thr->valstack_top >= thr->valstack_bottom);
+}
+#endif  /* !DUK_USE_PREFER_SIZE */
+
 DUK_EXTERNAL void duk_pop_2(duk_context *ctx) {
 	DUK_ASSERT_CTX_VALID(ctx);
 	duk_pop_n(ctx, 2);

src-input/duk_bi_thread.c

@@ -13,6 +13,10 @@ DUK_INTERNAL duk_ret_t duk_bi_thread_constructor(duk_context *ctx) {
 	duk_hthread *new_thr;
 	duk_hobject *func;
 
+	/* Check that the argument is callable; this is not 100% because we
+	 * don't allow native functions to be a thread's initial function.
+	 * Resume will reject such functions in any case.
+	 */
 	/* XXX: need a duk_require_func_promote_lfunc() */
 	func = duk_require_hobject_promote_lfunc(ctx, 0);
 	DUK_ASSERT(func != NULL);
@@ -50,8 +54,6 @@ DUK_INTERNAL duk_ret_t duk_bi_thread_constructor(duk_context *ctx) {
 DUK_INTERNAL duk_ret_t duk_bi_thread_resume(duk_context *ctx) {
 	duk_hthread *thr = (duk_hthread *) ctx;
 	duk_hthread *thr_resume;
-	duk_tval *tv;
-	duk_hobject *func;
 	duk_hobject *caller_func;
 	duk_small_int_t is_error;
 
@@ -107,26 +109,27 @@ DUK_INTERNAL duk_ret_t duk_bi_thread_resume(duk_context *ctx) {
 		 * tip-top shape (longjmp handler will assert for these).
 		 */
 	} else {
+		duk_hobject *h_fun;
+
 		DUK_ASSERT(thr_resume->state == DUK_HTHREAD_STATE_INACTIVE);
 
+		/* The initial function must be an Ecmascript function (but
+		 * can be bound).  We must make sure of that before we longjmp
+		 * because an error in the RESUME handler call processing will
+		 * not be handled very cleanly.
+		 */
 		if ((thr_resume->callstack_top != 0) ||
 		    (thr_resume->valstack_top - thr_resume->valstack != 1)) {
-			goto state_invalid_initial;
-		}
-		tv = &thr_resume->valstack_top[-1];
-		DUK_ASSERT(tv >= thr_resume->valstack && tv < thr_resume->valstack_top);
-		if (!DUK_TVAL_IS_OBJECT(tv)) {
-			goto state_invalid_initial;
-		}
-		func = DUK_TVAL_GET_OBJECT(tv);
-		DUK_ASSERT(func != NULL);
-		if (!DUK_HOBJECT_IS_COMPFUNC(func)) {
-			/* Note: cannot be a bound function either right now,
-			 * this would be easy to relax though.
-			 */
-			goto state_invalid_initial;
+			goto state_error;
 		}
 
+		duk_push_tval(ctx, DUK_GET_TVAL_NEGIDX((duk_context *) thr_resume, -1));
+		duk_resolve_nonbound_function(ctx);
+		h_fun = duk_require_hobject(ctx, -1);  /* reject lightfuncs on purpose */
+		if (!DUK_HOBJECT_IS_CALLABLE(h_fun) || !DUK_HOBJECT_IS_COMPFUNC(h_fun)) {
+			goto state_error;
+		}
+		duk_pop(ctx);
 	}
 
 	/*
@@ -177,13 +180,8 @@ DUK_INTERNAL duk_ret_t duk_bi_thread_resume(duk_context *ctx) {
 	duk_err_longjmp(thr);  /* execution resumes in bytecode executor */
 	return 0;  /* never here */
 
- state_invalid_initial:
-	DUK_ERROR_TYPE(thr, "invalid initial thread state/stack");
-	return 0;  /* never here */
-
  state_error:
-	DUK_ERROR_TYPE(thr, "invalid state");
-	return 0;  /* never here */
+	DUK_DCERROR_TYPE_INVALID_STATE(thr);
 }
 #endif
 
@@ -297,8 +295,7 @@ DUK_INTERNAL duk_ret_t duk_bi_thread_yield(duk_context *ctx) {
 	return 0;  /* never here */
 
  state_error:
-	DUK_ERROR_TYPE(thr, "invalid state");
-	return 0;  /* never here */
+	DUK_DCERROR_TYPE_INVALID_STATE(thr);
 }
 #endif
 

src-input/duk_error.h

@@ -222,6 +222,13 @@
 		DUK_ERROR_TYPE_INVALID_ARGS((thr)); \
 		return 0; \
 	} while (0)
+#define DUK_ERROR_TYPE_INVALID_STATE(thr) do { \
+		duk_err_type_invalid_state((thr), DUK_FILE_MACRO, (duk_int_t) DUK_LINE_MACRO); \
+	} while (0)
+#define DUK_DCERROR_TYPE_INVALID_STATE(thr) do { \
+		DUK_ERROR_TYPE_INVALID_STATE((thr)); \
+		return 0; \
+	} while (0)
 #define DUK_ERROR_TYPE_INVALID_TRAP_RESULT(thr) do { \
 		duk_err_type_invalid_trap_result((thr), DUK_FILE_MACRO, (duk_int_t) DUK_LINE_MACRO); \
 	} while (0)
@@ -288,13 +295,20 @@
 #define DUK_ERROR_TYPE_INVALID_ARGS(thr) do { \
 		duk_err_type((thr)); \
 	} while (0)
-#define DUK_ERROR_TYPE_INVALID_TRAP_RESULT(thr) do { \
+#define DUK_DCERROR_TYPE_INVALID_ARGS(thr) do { \
+		DUK_UNREF((thr)); \
+		return DUK_RET_TYPE_ERROR; \
+	} while (0)
+#define DUK_ERROR_TYPE_INVALID_STATE(thr) do { \
 		duk_err_type((thr)); \
 	} while (0)
-#define DUK_DCERROR_TYPE_INVALID_ARGS(thr) do { \
+#define DUK_DCERROR_TYPE_INVALID_STATE(thr) do { \
 		DUK_UNREF((thr)); \
 		return DUK_RET_TYPE_ERROR; \
 	} while (0)
+#define DUK_ERROR_TYPE_INVALID_TRAP_RESULT(thr) do { \
+		duk_err_type((thr)); \
+	} while (0)
 #define DUK_ERROR_TYPE(thr,msg) do { \
 		duk_err_type((thr)); \
 	} while (0)
@@ -436,6 +450,7 @@ DUK_NORETURN(DUK_INTERNAL_DECL void duk_err_range_index(duk_hthread *thr, const
 DUK_NORETURN(DUK_INTERNAL_DECL void duk_err_range_push_beyond(duk_hthread *thr, const char *filename, duk_int_t linenumber));
 DUK_NORETURN(DUK_INTERNAL_DECL void duk_err_range(duk_hthread *thr, const char *filename, duk_int_t linenumber, const char *message));
 DUK_NORETURN(DUK_INTERNAL_DECL void duk_err_type_invalid_args(duk_hthread *thr, const char *filename, duk_int_t linenumber));
+DUK_NORETURN(DUK_INTERNAL_DECL void duk_err_type_invalid_state(duk_hthread *thr, const char *filename, duk_int_t linenumber));
 DUK_NORETURN(DUK_INTERNAL_DECL void duk_err_type_invalid_trap_result(duk_hthread *thr, const char *filename, duk_int_t linenumber));
 #else  /* DUK_VERBOSE_ERRORS */
 DUK_NORETURN(DUK_INTERNAL_DECL void duk_err_error(duk_hthread *thr));

src-input/duk_error_macros.c

@@ -67,6 +67,9 @@ DUK_INTERNAL void duk_err_range_push_beyond(duk_hthread *thr, const char *filena
 DUK_INTERNAL void duk_err_type_invalid_args(duk_hthread *thr, const char *filename, duk_int_t linenumber) {
 	DUK_ERROR_RAW(thr, filename, linenumber, DUK_ERR_TYPE_ERROR, DUK_STR_INVALID_ARGS);
 }
+DUK_INTERNAL void duk_err_type_invalid_state(duk_hthread *thr, const char *filename, duk_int_t linenumber) {
+	DUK_ERROR_RAW(thr, filename, linenumber, DUK_ERR_TYPE_ERROR, DUK_STR_INVALID_STATE);
+}
 DUK_INTERNAL void duk_err_type_invalid_trap_result(duk_hthread *thr, const char *filename, duk_int_t linenumber) {
 	DUK_ERROR_RAW(thr, filename, linenumber, DUK_ERR_TYPE_ERROR, DUK_STR_INVALID_TRAP_RESULT);
 }

src-input/duk_js_executor.c

@@ -1095,8 +1095,6 @@ duk_small_uint_t duk__handle_longjmp(duk_hthread *thr,
 		DUK_ASSERT(DUK_ACT_GET_FUNC(thr->callstack + thr->callstack_top - 1) != NULL &&
 		           DUK_HOBJECT_IS_NATFUNC(DUK_ACT_GET_FUNC(thr->callstack + thr->callstack_top - 1)) &&
 		           ((duk_hnatfunc *) DUK_ACT_GET_FUNC(thr->callstack + thr->callstack_top - 1))->func == duk_bi_thread_resume);
-		DUK_ASSERT(DUK_ACT_GET_FUNC(thr->callstack + thr->callstack_top - 2) != NULL &&
-		           DUK_HOBJECT_IS_COMPFUNC(DUK_ACT_GET_FUNC(thr->callstack + thr->callstack_top - 2)));      /* an Ecmascript function */
 		DUK_ASSERT_DISABLE((thr->callstack + thr->callstack_top - 2)->idx_retval >= 0);                              /* unsigned */
 
 		tv = &thr->heap->lj.value2;  /* resumee */
@@ -1115,9 +1113,6 @@ duk_small_uint_t duk__handle_longjmp(duk_hthread *thr,
 		           (DUK_ACT_GET_FUNC(resumee->callstack + resumee->callstack_top - 1) != NULL &&
 		            DUK_HOBJECT_IS_NATFUNC(DUK_ACT_GET_FUNC(resumee->callstack + resumee->callstack_top - 1)) &&
 		            ((duk_hnatfunc *) DUK_ACT_GET_FUNC(resumee->callstack + resumee->callstack_top - 1))->func == duk_bi_thread_yield));
-		DUK_ASSERT(resumee->state != DUK_HTHREAD_STATE_YIELDED ||
-		           (DUK_ACT_GET_FUNC(resumee->callstack + resumee->callstack_top - 2) != NULL &&
-		            DUK_HOBJECT_IS_COMPFUNC(DUK_ACT_GET_FUNC(resumee->callstack + resumee->callstack_top - 2))));      /* an Ecmascript function */
 		DUK_ASSERT_DISABLE(resumee->state != DUK_HTHREAD_STATE_YIELDED ||
 		           (resumee->callstack + resumee->callstack_top - 2)->idx_retval >= 0);                              /* idx_retval unsigned */
 		DUK_ASSERT(resumee->state != DUK_HTHREAD_STATE_INACTIVE ||
@@ -1196,7 +1191,11 @@ duk_small_uint_t duk__handle_longjmp(duk_hthread *thr,
 			                                      1,              /* num_stack_args */
 			                                      call_flags);    /* call_flags */
 			if (setup_rc == 0) {
-				/* Shouldn't happen but check anyway. */
+				/* This shouldn't happen; Duktape.Thread.resume()
+				 * should make sure of that.  If it does happen
+				 * this internal error will propagate out of the
+				 * executor which can be quite misleading.
+				 */
 				DUK_ERROR_INTERNAL(thr);
 			}
 

src-input/duk_strings.h

@@ -26,6 +26,7 @@
 #define DUK_STR_UNSUPPORTED                      "unsupported"
 #define DUK_STR_INVALID_COUNT                    "invalid count"
 #define DUK_STR_INVALID_ARGS                     "invalid args"
+#define DUK_STR_INVALID_STATE                    "invalid state"
 #define DUK_STR_INVALID_INPUT                    "invalid input"
 #define DUK_STR_INVALID_LENGTH                   "invalid length"
 #define DUK_STR_NOT_CONSTRUCTABLE                "not constructable"

tests/ecmascript/test-dev-coroutine-bound-func.js

@@ -0,0 +1,41 @@
+/*
+ *  Coroutine with initial bound function.
+ */
+
+/*===
+mythread starting
+object mythis
+["FOO","BAR","foo"]
+1
+2
+3
+undefined
+===*/
+
+function test() {
+    function mythread(a,b,c) {
+        print('mythread starting');
+        print(typeof this, this);
+        print(JSON.stringify([a,b,c]));
+        Duktape.Thread.yield(1);
+        Duktape.Thread.yield(2);
+        Duktape.Thread.yield(3);
+    }
+
+    var T = new Duktape.Thread(mythread.bind('mythis', 'FOO', 'BAR'));
+
+    // Only one argument can be given for the initial call, but the bound
+    // arguments are still prepended as normal.
+    print(Duktape.Thread.resume(T, 'foo'));
+
+    // No difference in further resumes.
+    print(Duktape.Thread.resume(T, 'foo'));
+    print(Duktape.Thread.resume(T, 'foo'));
+    print(Duktape.Thread.resume(T, 'foo'));
+}
+
+try {
+    test();
+} catch (e) {
+    print(e.stack || e);
+}

tests/ecmascript/test-dev-coroutine-native-func.js

@@ -0,0 +1,27 @@
+/*
+ *  Native function not allowed as an initial function.
+ */
+
+/*===
+caught error
+TypeError
+===*/
+
+function test() {
+    var T = new Duktape.Thread(Math.cos);
+
+    print(Duktape.Thread.resume(T, 'foo'));
+    print(Duktape.Thread.resume(T, 'foo'));
+    print(Duktape.Thread.resume(T, 'foo'));
+    print(Duktape.Thread.resume(T, 'foo'));
+}
+
+try {
+    test();
+} catch (e) {
+    // Print when we get here: if the executor RESUME longjmp handler, we
+    // *won't* come here at all.  Rather, the error is propagated out of the
+    // executor (as an "internal error").
+    print('caught error');
+    print(e.name);
+}

website/guide/duktapebuiltins.html

@@ -562,10 +562,11 @@ ordinary function and as a constructor.  The behavior is the same in both
 cases:</p>
 <ul>
 <li>The first argument is checked to be a function (if not, a <code>TypeError</code>
-    is thrown).  The return value is a new thread whose initial function is
-    recorded to be the argument function (this function will start executing
-    when the new thread is first resumed).  The internal prototype of the
-    newly created Thread will be the <code>Duktape.Thread.prototype</code> object.</li>
+    is thrown).  The function must be an Ecmascript function (bound or non-bound).
+    The return value is a new thread whose initial function is recorded to be the
+    argument function (this function will start executing when the new thread is
+    first resumed).  The internal prototype of the newly created Thread will be the
+    <code>Duktape.Thread.prototype</code> object.</li>
 </ul>
 
 <h2>Duktape.Thread.prototype</h2>

website/guide/functionobjects.html

@@ -16,7 +16,7 @@ afterwards):</p>
 <tr><td class="propname">prototype</td><td>standard</td><td>Prototype used for new objects when called as a constructor.  Present for most constructable Function objects, not copied to bound functions.</td></tr>
 <tr><td class="propname">caller</td><td>standard</td><td>Accessor which throws an error.  Present for strict functions and bound functions.  Not copied to bound functions.  (If <code>DUK_USE_NONSTD_FUNC_CALLER_PROPERTY</code> is given, non-strict functions will get a non-standard <code>caller</code> property.)</td></tr>
 <tr><td class="propname">arguments</td><td>standard</td><td>Accessor which throws an error.  Present for strict functions and bound functions.  Not copied to bound functions.</td></tr>
-<tr><td class="propname">name</td><td>Duktape</td><td>Function name, see below.  Copied to bound function from target function.</td></tr>
+<tr><td class="propname">name</td><td>Duktape</td><td>Function name, see below.  Bound function name is based on this property, with a <code>"bound "</code> prefix (standard ES6 behavior).</td></tr>
 <tr><td class="propname">fileName</td><td>Duktape</td><td>Filename or context where function was declared (same name as in error tracebacks).  Copied to bound function from target function. </td></tr>
 <tr><td class="propname">callee</td><td>n/a</td><td>Never assigned by default (listed here to clarify relationship to "caller" property).</td></tr>
 </tbody>