From e8d9326d5f8ade96713f676c6a8de096d653affe Mon Sep 17 00:00:00 2001
From: Dmitry Vasilev <dmitry.vasil@gmail.com>
Date: Thu, 26 Oct 2023 12:25:35 +0800
Subject: [PATCH] fix error origin

---
 src/ast_utils.js | 44 +++++++++++++++++++-------------------------
 src/color.js     | 14 +++++---------
 src/eval.js      | 22 ++++++++++++++--------
 test/test.js     | 12 ++++++------
 4 files changed, 44 insertions(+), 48 deletions(-)

diff --git a/src/ast_utils.js b/src/ast_utils.js
index 0588d9a..953da50 100644
--- a/src/ast_utils.js
+++ b/src/ast_utils.js
@@ -145,33 +145,27 @@ export const find_node = (node, pred) => {
     )
 }
 
-// TODO refactor, at eval.js we have explicit information if node is error
-// origin, without guessing. See also color.js
 // TODO result is ok, but value is rejected promise
-// TODO check if return result is null and throw early
-export const find_error_origin_node = node =>
-  find_node(
-    // TODO do not go inside function_expr
-    node, 
-    n => n.result != null && !n.result.ok && (
-      n.result.error != null
-      ||
-      // node has no error, but its children also have no error, so this node
-      // is error origin
-      n.children.find(c => find_error_origin_node(c) != null) == null
-      &&
-      (
-        // In case if throw null or throw undefined
-        n.type == 'throw'
-        ||
-        // await can also throw null
-        n.type == 'unary' && n.operator == 'await'
-        // or function call throwing null or undefined
-        || 
-        n.type == 'function_call'
-      )
+export const find_error_origin_node = (node, is_root = true) => {
+  if(node.result == null) {
+    return null
+  }
+  if(node.type == 'function_expr' && !is_root) {
+    return null
+  }
+  if(node.result.is_error_origin) {
+    return node
+  }
+  if(node.children == null) {
+    return null
+  }
+  return node
+    .children
+    .reduce(
+      (result, c) => result ?? find_error_origin_node(c, false),
+      null
     )
-  )
+}
 
 /* Maps tree nodes, discarding mapped children, so maps only node contents, not
  * allowing to modify structure */
diff --git a/src/color.js b/src/color.js
index e49d75b..283c8eb 100644
--- a/src/color.js
+++ b/src/color.js
@@ -20,7 +20,7 @@ const is_result_eq = (a,b) => a.result == null
   ? b.result == null
   : b.result != null 
       && a.result.ok == b.result.ok 
-      && a.result.error_origin == b.result.error_origin
+      && !!a.result.is_error_origin == !!b.result.is_error_origin
 
 const node_to_color = node => ({
   index: node.index, 
@@ -31,11 +31,7 @@ const node_to_color = node => ({
       ? null
       : node.result.ok
         ? {ok: true}
-        // node.result.error may be null, for example if throw null
-        // See find_error_origin_node
-        : node.result.error == null
-          ? {ok: false, error_origin: false}
-          : {ok: false, error_origin: true}
+        : {ok: false, is_error_origin: !!node.result.is_error_origin}
 })
 
 const is_short_circuit = node =>
@@ -175,7 +171,7 @@ const do_color = (node, is_root = false) => {
     return [node_to_color(node)]
   } 
 
-  if(node.result?.error != null) {
+  if(node.result?.is_error_origin) {
     const color = node_to_color(node)
     const exprs = collect_function_exprs(node)
     if(exprs.length == 0) {
@@ -208,7 +204,7 @@ const do_color = (node, is_root = false) => {
   const result = color_children(node, is_root)
   return node.result != null && !node.result.ok
     ? result.map(c => c.result == null
-        ? {...c, result: {ok: false, error_origin: false}}
+        ? {...c, result: {ok: false, is_error_origin: false}}
         : c
       )
     : result
@@ -222,7 +218,7 @@ export const color = frame => {
       c.result != null
       &&
       // Parts that were not error origins
-      (c.result.ok || c.result.error_origin)
+      (c.result.ok || c.result.is_error_origin)
     )
 
   // Sanity-check result
diff --git a/src/eval.js b/src/eval.js
index 3be111c..1740076 100644
--- a/src/eval.js
+++ b/src/eval.js
@@ -452,7 +452,7 @@ export const eval_codestring = (codestring, scope) =>
       try {
         return {ok: true, value: eval('with({...scope}){' + codestring + '}')}
       } catch(error) {
-        return {ok: false, error}
+        return {ok: false, error, is_error_origin: true}
       }
     `
   ))(codestring, scope)
@@ -481,7 +481,7 @@ const get_args_scope = (fn_node, args, closure) => {
 
   if(!ok) {
     // TODO show exact destructuring error
-    return {ok, error}
+    return {ok, error, is_error_origin: true}
   } else {
     return {
       ok, 
@@ -534,6 +534,7 @@ const do_eval_frame_expr = (node, scope, callsleft, context) => {
         children: result.children,
         calls: result.calls,
         error: new TypeError(child.string + ' is not iterable'),
+        is_error_origin: true,
       }
     }
   } else if([
@@ -598,6 +599,7 @@ const do_eval_frame_expr = (node, scope, callsleft, context) => {
         return {
           ok: false,
           error: context.calltree_node.error,
+          is_error_origin: true,
           children,
           calls,
         }
@@ -611,6 +613,7 @@ const do_eval_frame_expr = (node, scope, callsleft, context) => {
         call: c,
         value: c.value, 
         error: c.error, 
+        is_error_origin: !c.ok,
         children,
         calls: calls.slice(1)
       }
@@ -687,7 +690,7 @@ const do_eval_frame_expr = (node, scope, callsleft, context) => {
       return {ok: false, children, calls}
     } else {
       const expr = children[0]
-      let ok, value, error
+      let ok, value, error, is_error_origin
       if(node.operator == '!') {
         ok = true
         value = !expr.result.value
@@ -707,6 +710,7 @@ const do_eval_frame_expr = (node, scope, callsleft, context) => {
             ok = status.ok
             error = status.error
             value = status.value
+            is_error_origin = !ok
           }
         } else {
           ok = true
@@ -715,7 +719,7 @@ const do_eval_frame_expr = (node, scope, callsleft, context) => {
       } else {
         throw new Error('unknown op')
       }
-      return {ok, children, calls, value, error}
+      return {ok, children, calls, value, error, is_error_origin}
     }
   } else if(node.type == 'binary' && !['&&', '||', '??'].includes(node.operator)){
 
@@ -783,7 +787,7 @@ const eval_children = (node, scope, calls, context) => {
 }
 
 const eval_frame_expr = (node, scope, callsleft, context) => {
-  const {ok, error, value, call, children, calls} 
+  const {ok, error, is_error_origin, value, call, children, calls} 
     = do_eval_frame_expr(node, scope, callsleft, context)
   if(callsleft != null && calls == null) {
     // TODO remove it, just for debug
@@ -795,7 +799,7 @@ const eval_frame_expr = (node, scope, callsleft, context) => {
       ...node, 
       children, 
       // Add `call` for step_into
-      result: {ok, error, value, call}
+      result: {ok, error, value, call, is_error_origin}
     },
     calls,
   }
@@ -974,7 +978,7 @@ const eval_statement = (s, scope, calls, context) => {
       return {
         ok: false,
         // TODO assign error to node where destructuring failed, not to every node
-        node: {...s_evaled, result: {ok, error}},
+        node: {...s_evaled, result: {ok, error, is_error_origin: true}},
         scope,
         calls,
       }
@@ -1135,6 +1139,7 @@ const eval_statement = (s, scope, calls, context) => {
         children: [node], 
         result: {
           ok: false, 
+          is_error_origin: node.result.ok,
           error: node.result.ok ? node.result.value : null,
         }
       },
@@ -1188,7 +1193,8 @@ export const eval_frame = (calltree_node, modules) => {
             a => ({...a, 
               result: {
                 ok: args_scope_result.ok,
-                error:  args_scope_result.ok ? null : args_scope_result.error,
+                error: args_scope_result.ok ? null : args_scope_result.error,
+                is_error_origin: !args_scope_result.ok,
                 value: !args_scope_result.ok ? null : args_scope_result.value[a.value],
               }
             })
diff --git a/test/test.js b/test/test.js
index 41e5b64..4613a20 100644
--- a/test/test.js
+++ b/test/test.js
@@ -1504,7 +1504,7 @@ export const tests = [
         {
           index: code.indexOf('x()'), 
           length: 'x()'.length, 
-          result: { ok: false, error_origin: true } 
+          result: { ok: false, is_error_origin: true } 
         }
       ]
     )
@@ -1527,12 +1527,12 @@ export const tests = [
         { 
           index: code.indexOf('throw'), 
           length: 'throw new Error()'.length, 
-          result: { ok: false, error_origin: true } 
+          result: { ok: false, is_error_origin: true } 
         },
         { 
           index: code.indexOf('x()'), 
           length: "x()".length, 
-          result: { ok: false, error_origin: true } 
+          result: { ok: false, is_error_origin: true } 
         }
       ]
     )
@@ -1544,7 +1544,7 @@ export const tests = [
     // Color only index access, not grouping braces
     assert_equal(
       color_file(initial, ''),
-      [ { index: 1, length: 7, result: { ok: false, error_origin: true } } ],
+      [ { index: 1, length: 7, result: { ok: false, is_error_origin: true } } ],
     )
   }),
 
@@ -1603,7 +1603,7 @@ export const tests = [
         { 
           index: 0, 
           length: code.length, 
-          result: { ok: false, error_origin: true } 
+          result: { ok: false, is_error_origin: true } 
         } 
       ]
     )
@@ -1667,7 +1667,7 @@ const y = x()`
     const i = test_initial_state(code)
     const coloring = color_file(i, '')
 
-    const result = {ok: false, error_origin: true}
+    const result = {ok: false, is_error_origin: true}
     assert_equal(
       coloring,
       [