Improve error message #1577

bsdelf · 2017-02-10T14:21:34Z

Many error messages of JerryScript only tell there is an error but without further context, which is vague and makes it difficult to debug.

This patch helps the situtation, for errors related to unexisting properties or bad references, it can remind the property/variable names at least.

With this patch, for the following testing code (also works for node):

'use strict';

var print = print || console.log;

var printError = (function (i) {
  return function (head, error) {
    print(i + (i <= 9 ? ' ' : ''), head, Array(30 - head.length).join(' '), error);
      print(Array(100).join('-'));
      ++i;
  };
})(1);

[ 'x = 1;',
  'x += 1;',
  'var y = x;',
  'x.y = 1;',
  'var x; x.y;',
  'var x; x[1];',
  'var x; x.y = 1;',
  'var x; x[0] = 1;',
  'var x = null; x[\'y\'];',
  'null.y = 1;'
].forEach(function (src) {
  try {
    eval(src);
  } catch (e) {
    printError(src, e);
  }
});

The expected output would be:

1  x = 1;                         ReferenceError: x is not defined
---------------------------------------------------------------------------------------------------
2  x += 1;                        ReferenceError: x is not defined
---------------------------------------------------------------------------------------------------
3  var y = x;                     ReferenceError: x is not defined
---------------------------------------------------------------------------------------------------
4  x.y = 1;                       ReferenceError: x is not defined
---------------------------------------------------------------------------------------------------
5  var x; x.y;                    TypeError: Cannot read property 'y' of undefined
---------------------------------------------------------------------------------------------------
6  var x; x[1];                   TypeError: Cannot read property '1' of undefined
---------------------------------------------------------------------------------------------------
7  var x; x.y = 1;                TypeError: Cannot set property 'y' of undefined
---------------------------------------------------------------------------------------------------
8  var x; x[0] = 1;               TypeError: Cannot set property '0' of undefined
---------------------------------------------------------------------------------------------------
9  var x = null; x['y'];          TypeError: Cannot read property 'y' of null
---------------------------------------------------------------------------------------------------
10 null.y = 1;                    TypeError: Cannot set property 'y' of null
---------------------------------------------------------------------------------------------------

zherczeg · 2017-02-10T14:33:37Z

jerry-core/ecma/operations/ecma-exceptions.h

+  do { \
+    const int msg_size = snprintf (NULL, 0, msg_fmt, __VA_ARGS__) + 1; \
+    JMEM_DEFINE_LOCAL_ARRAY (msg_p, msg_size, char); \
+    snprintf (msg_p, (size_t) msg_size, msg_fmt, __VA_ARGS__); \


Does jerry libc has snprintf?

zherczeg · 2017-02-10T14:36:49Z

In general the focus of the JerryScript project is low memory consumption and small binary size, so we don't give too much error context by default. Hence we need a compile time flag to enable this feature.

Additional note: we are working on a debugger in #1557, that will hopefully improves debugging experience.

dbatyai · 2017-02-10T14:43:40Z

jerry-core/vm/vm.c

@@ -96,7 +96,16 @@ vm_op_get_value (ecma_value_t object, /**< base object */

  if (unlikely (ecma_is_value_undefined (object) || ecma_is_value_null (object)))
  {
-    return ecma_raise_type_error (ECMA_ERR_MSG ("Base object cannot be null or undefined."));
+    ecma_string_t *property_name_p = ecma_get_string_from_value (property);
+    ECMA_STRING_TO_UTF8_STRING (property_name_p, property_name_utf8_p, property_name_size);


Personally, I would prefer if the messages were created using the string helper functions found in ecma-helpers-string.c, instead of using intermediate buffers and snprintf, if it's possible. Maybe like defining a few magic strings, and using ecma_concat_ecma_strings to create the final message.

It would be good to use create a helper function than a helper macro.

Indeed, use ecma-helpers-string to handle string is better than use plain C functions, but the code might be tedious to compose messages like "Cannot read property '%s' of %s". Would you have a suggestion?

Yes, please create a new helper function which can do the concatenation.

dbatyai · 2017-02-10T14:48:44Z

The JERRY_ENABLE_ERROR_MESSAGES guard already exists and would be suitable here as well, so no need for a new one.

bsdelf · 2017-02-19T03:32:19Z

Check about the updated patch.

The newly introduced helper function is:

ecma_string_t *ecma_join_ecma_strings (ecma_string_t **string_array, uint32_t n);

I'm not sure whether it is a good idea to let the join function dereference the input ecma-strings.

Otherwise, we might need another helper function:

void ecma_deref_ecma_strings (ecma_string_t **string_array, uint32_t n);

dbatyai · 2017-02-20T09:30:11Z

jerry-core/ecma/operations/ecma-exceptions.h

+  do { \
+    ecma_object_t *error_obj_p = ecma_new_standard_error_with_message (error_type, error_msg_p); \
+    var_name = ecma_make_error_obj_value (error_obj_p); \
+  } while (0)


Could you please remove this macro? It doesn't seem to do much anymore, outside of replacing one line for another.

dbatyai · 2017-02-20T09:35:05Z

In my opinion it's completely fine for ecma_join_ecma_strings to dereference the argument strings.

zherczeg · 2017-02-20T09:43:05Z

I agree, binary size matters.

zherczeg

Some comments.

zherczeg · 2017-02-20T09:43:27Z

jerry-core/ecma/base/ecma-helpers-string.c

@@ -699,6 +699,31 @@ ecma_concat_ecma_strings (ecma_string_t *string1_p, /**< first ecma-string */
 } /* ecma_concat_ecma_strings */

 /**
+ * Join an array of ecma-strings and release them.
+ *
+ * @return a newly joined ecma-string


newly created

zherczeg · 2017-02-20T09:43:49Z

jerry-core/ecma/base/ecma-helpers-string.c

+
+  uint32_t i = 0;
+
+  for (; i < n - 1; ++i)


for (uint32_t i = 0; ...)

zherczeg · 2017-02-20T09:44:32Z

jerry-core/ecma/base/ecma-helpers-string.c

+ecma_join_ecma_strings (ecma_string_t **string_array, uint32_t n)
+{
+  JERRY_ASSERT (string_array != NULL);
+  JERRY_ASSERT (n > 0);


What about n > 1 ?

zherczeg · 2017-02-20T09:45:26Z

jerry-core/ecma/base/ecma-helpers-string.c

+    ecma_string_t *string2_p = string_array[i + 1];
+    string_array[i + 1] = ecma_concat_ecma_strings (string1_p, string2_p);
+    ecma_deref_ecma_string (string1_p);
+    ecma_deref_ecma_string (string2_p);


You don't need to overwrite the string array, just keep the final string in a local variable.

zherczeg · 2017-02-20T09:45:38Z

jerry-core/ecma/base/ecma-helpers-string.c

+ * @return a newly joined ecma-string
+ */
+ecma_string_t *
+ecma_join_ecma_strings (ecma_string_t **string_array, uint32_t n)


Should be const array.

zherczeg · 2017-02-20T09:47:01Z

jerry-core/ecma/operations/ecma-get-put-value.c

+
+    ecma_string_t *msg_str_p = ecma_join_ecma_strings (string_array, JERRY_NUM_OF_ELEMENTS (string_array));
+    ECMA_DEFINE_ERROR_VALUE (error_value, ECMA_ERROR_REFERENCE, msg_str_p);
+    ecma_deref_ecma_string (msg_str_p);


If we use this utility solely for creating errors, we could create the error object in the utility to reduce binary size.

zherczeg · 2017-02-20T09:48:50Z

jerry-core/lit/lit-magic-strings.inc.h

@@ -27,6 +27,7 @@ LIT_MAGIC_STRING_FIRST_STRING_WITH_SIZE (1, LIT_MAGIC_STRING_NEW_LINE_CHAR)
 LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_NEW_LINE_CHAR, "\n")
 LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_SPACE_CHAR, " ")
 LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_DOUBLE_QUOTE_CHAR,"\"")
+LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_SINGLE_QUOTE_CHAR,"'")


I don't think it is a good idea to add magic strings for error handling, but I am curious about others opinion.

What about use JERRY_ENABLE_ERROR_MESSAGES macro to enable specific magic strings at compile time?

My problem is conceptual, no error messages are magic string at this point. Magic strings cover basic JS functionality. @LaszloLango what do you think?

I thinks the concept is that frequently used strings should be magic strings. Increasing the number of magic strings, when the error messages are disabled is not good. I'm OK with using frequent error messages as magic string if JERRY_ENABLE_ERROR_MESSAGES is enabled.

Errors are usually short living objects. Magic strings are designed for property names which likely present for longer times. But having more magic strings does not hurt either.

Ok, we need a system which allows conditional magic string definition. There is no point adding error message strings when they never used. It would just increase binary size.

Errors are usually short living objects.

Good point. Both solutions are good to me (remove or guard).

There is no point adding error message strings when they never used. It would just increase binary size.

I agree.

Turns out introduce a format function to compose error message can make things easier:

ecma_format_ecma_string ("Cannot read property '%' of %", prop_str_p, name_str_p)

No need to involve magic string.

The format string can be easily found with grep.

Error handling in parser_parse_script() can be simplified.

@zherczeg @LaszloLango

Excellent idea.

Sounds great.

bsdelf · 2017-02-27T03:11:13Z

Format function:

ecma_string_t *ecma_format_ecma_strings (const char *format, ...);

Supported placeholders:

%: ecma-string.
%%: single percent sign.

Pitfalls:

ecma_format_ecma_strings ("%%", str_prefix_p, str_p)
ecma_format_ecma_strings ("%%%%%", str_surrounded_by_percent_p)

@zherczeg @LaszloLango

zherczeg

This patch is much better than the previous ones. Thank you for the hard work. A few comments below.

zherczeg · 2017-02-27T09:11:01Z

jerry-core/ecma/base/ecma-helpers-string.c

+  va_end (args);
+
+  return string_p;
+} /* ecma_format_ecma_strings */


We should add a JERRY_ENABLE_ERROR_MESSAGES macro around this function.

zherczeg · 2017-02-27T09:12:14Z

jerry-core/ecma/base/ecma-helpers-string.c

+ */
+ecma_string_t *
+ecma_format_ecma_strings (const char * format, /**< format string */
+                          ...) /**< ecma-strings */


I never particularly like ..., for me it looks like a crude hack. I would prefer a constant array of ecma values. I am curious about others opinion.

I have no problems with it. :)

Variadic function is more readable at caller side, and numbers of placeholders/ arguments can be easily checked.

zherczeg · 2017-02-27T09:16:19Z

jerry-core/ecma/base/ecma-helpers-string.c

+        string_p = ecma_concat_ecma_strings (string1_p, string2_p);
+        ecma_deref_ecma_string (string1_p);
+        ecma_deref_ecma_string (string2_p);
+      }


Do we need to support double % sign? This function is not a general printf, we probably don't need to overcomplicate things.

That would be best.

zherczeg · 2017-02-28T07:54:09Z

jerry-core/ecma/base/ecma-helpers-string.c

+ */
+ecma_string_t *
+ecma_format_ecma_strings (const char *format, /**< format string */
+                          ...) /**< ecma-strings */


ecma values

zherczeg · 2017-02-28T07:54:52Z

jerry-core/ecma/base/ecma-helpers-string.c

+                                                  (lit_utf8_size_t) (end_p - start_p));
+      string_p = ecma_concat_ecma_strings (string1_p, string2_p);
+      ecma_deref_ecma_string (string1_p);
+      ecma_deref_ecma_string (string2_p);


We could skip this if end_p == start_p

zherczeg · 2017-02-28T08:02:24Z

jerry-core/vm/vm.c

-    return ecma_raise_type_error (ECMA_ERR_MSG ("Base object cannot be null or undefined."));
+#ifdef JERRY_ENABLE_ERROR_MESSAGES
+    ecma_value_t obj_to_string_result = ecma_op_to_string (object);
+    ecma_string_t *obj_name_p = ecma_get_string_from_value (obj_to_string_result);


The ecma_op_to_string can have side effects (can run a JS code) and might not be successful. The side effects worries me most, since we might break ES5 compatibility. What about returning with the class of an object? That is always successful.

zherczeg · 2017-02-28T08:02:54Z

jerry-core/vm/vm.c

+    ecma_value_t obj_to_string_result = ecma_op_to_string (object);
+    ecma_string_t *obj_name_p = ecma_get_string_from_value (obj_to_string_result);
+
+    ecma_value_t prop_to_string_result = ecma_op_to_string (property);


Again, if property is an object, we should limit this conversion.

zherczeg · 2017-02-28T08:05:46Z

Perhaps we could just pass all ecma values to the utility function and let it do the string conversion if needed. Objects could simply be represented with their class.

bsdelf · 2017-03-06T02:03:05Z

Patch updated. @zherczeg @LaszloLango

LaszloLango · 2017-03-06T06:44:17Z

jerry-core/ecma/base/ecma-helpers.h

@@ -172,6 +172,7 @@ ecma_string_t *ecma_new_ecma_string_from_magic_string_id (lit_magic_string_id_t
 ecma_string_t *ecma_new_ecma_string_from_magic_string_ex_id (lit_magic_string_ex_id_t id);
 ecma_string_t *ecma_new_ecma_length_string (void);
 ecma_string_t *ecma_concat_ecma_strings (ecma_string_t *string1_p, ecma_string_t *string2_p);
+ecma_string_t *ecma_format_ecma_values (const char *format, ...);


#ifdef JERRY_ENABLE_ERROR_MESSAGES ecma_string_t *ecma_format_ecma_values (const char *format, ...); #endif /* JERRY_ENABLE_ERROR_MESSAGES */

LaszloLango · 2017-03-06T06:48:49Z

jerry-core/ecma/base/ecma-helpers-string.c

+        if (ecma_is_value_string (arg_val))
+        {
+          arg_string_p = ecma_get_string_from_value (arg_val);
+          ecma_ref_ecma_string (arg_string_p);


Why do we need to increase the reference here?

Because ecma_get_magic_string will increase the reference count, while ecma_get_string_from_value won't. To make algorithm consistent, we increase reference count for the latter case, and dereference them after concatation.

I see, thanks for the explanation.

zherczeg

This patch is getting quite good now. Just a few more minor touches.

zherczeg · 2017-03-06T07:31:16Z

jerry-core/ecma/base/ecma-helpers-string.c

+        ecma_deref_ecma_string (string2_p);
+      }
+
+      /* Convert an argument to string without side effect. */


zherczeg · 2017-03-06T07:32:56Z

jerry-core/ecma/base/ecma-helpers-string.c

+      }
+      else
+      {
+        if (ecma_is_value_string (arg_val))


I think we can simply use the to_string in all other cases since non-object types can be converted to string without side effects.

zherczeg · 2017-03-06T07:38:42Z

jerry-core/vm/vm.c

+      ecma_string_t *error_msg_p = ecma_format_ecma_values ("Cannot set property '%' of %", property, object);
+      ecma_object_t *error_obj_p = ecma_new_standard_error_with_message (ECMA_ERROR_TYPE, error_msg_p);
+      ecma_value_t error_value = ecma_make_error_obj_value (error_obj_p);
+      ecma_deref_ecma_string (error_msg_p);


These four lines are repeated all the time. I would suggest to add the error type as a first argument for ecma_format_ecma_values and create the error there to reduce the binary size. Or introduce another helper. Perhaps the name could be ecma_new_standard_error_with_format.

I prefer the latter option.

However, if we are going to add a helper function in "ecma-exceptions.c" to raise ecma-error, why not merge ecma_format_ecma_values into that function too? After all, ecma_format_ecma_values is guarded by JERRY_ENABLE_ERROR_MESSAGES, it is only used to compose error message.

This was option 1, or I misunderstand something.

bsdelf · 2017-03-08T01:36:28Z

Patch updated. @zherczeg @LaszloLango

zherczeg

Very good patch.
LGTM after the minor code revert.

zherczeg · 2017-03-08T05:11:53Z

jerry-core/vm/vm.c

      ecma_free_value (property);
-      return to_object;
+
+      return error_value;


I see the point of recreating the error when messages are enabled, but not when they are disabled. Please update this ifdef to not change the original behavior.

JerryScript-DCO-1.0-Signed-off-by: Yanhui Shen [email protected]

bsdelf · 2017-03-08T09:22:07Z

Partially reverted. @zherczeg @LaszloLango

LaszloLango

Thanks for your hard work. LGTM

zherczeg reviewed Feb 10, 2017

View reviewed changes

bsdelf force-pushed the improve-error-message branch from 582854e to dad2c22 Compare February 10, 2017 14:34

dbatyai reviewed Feb 10, 2017

View reviewed changes

bsdelf force-pushed the improve-error-message branch 4 times, most recently from 1cea6e8 to 7076e72 Compare February 18, 2017 13:21

dbatyai reviewed Feb 20, 2017

View reviewed changes

zherczeg requested changes Feb 20, 2017

View reviewed changes

bsdelf force-pushed the improve-error-message branch 2 times, most recently from cf97620 to 1221039 Compare February 25, 2017 17:04

zherczeg requested changes Feb 27, 2017

View reviewed changes

bsdelf force-pushed the improve-error-message branch 2 times, most recently from 060a8e5 to 0e3791a Compare February 28, 2017 07:13

zherczeg requested changes Feb 28, 2017

View reviewed changes

LaszloLango mentioned this pull request Mar 2, 2017

Runtime error messages contain no source information #1635

Closed

bsdelf force-pushed the improve-error-message branch from 0e3791a to 5d8fdca Compare March 5, 2017 16:03

LaszloLango requested changes Mar 6, 2017

View reviewed changes

zherczeg requested changes Mar 6, 2017

View reviewed changes

bsdelf force-pushed the improve-error-message branch from 5d8fdca to 577bdc8 Compare March 7, 2017 14:02

zherczeg approved these changes Mar 8, 2017

View reviewed changes

Improve error message

919db82

JerryScript-DCO-1.0-Signed-off-by: Yanhui Shen [email protected]

bsdelf force-pushed the improve-error-message branch from 577bdc8 to 919db82 Compare March 8, 2017 08:20

LaszloLango approved these changes Mar 8, 2017

View reviewed changes

zherczeg merged commit a2a160d into jerryscript-project:master Mar 8, 2017

bsdelf deleted the improve-error-message branch March 8, 2017 12:14

Improve error message #1577

Improve error message #1577

Uh oh!

Conversation

bsdelf commented Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Feb 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg Feb 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsdelf Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbatyai commented Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsdelf commented Feb 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbatyai commented Feb 20, 2017

Uh oh!

zherczeg commented Feb 20, 2017

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsdelf Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsdelf commented Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsdelf commented Feb 10, 2017 •

edited

Loading

zherczeg Feb 13, 2017 •

edited

Loading

bsdelf Feb 14, 2017 •

edited

Loading

dbatyai commented Feb 10, 2017 •

edited

Loading

bsdelf commented Feb 19, 2017 •

edited

Loading

bsdelf Feb 24, 2017 •

edited

Loading

bsdelf commented Feb 27, 2017 •

edited

Loading

bsdelf Feb 28, 2017 •

edited

Loading

bsdelf commented Mar 6, 2017 •

edited

Loading

bsdelf Mar 6, 2017 •

edited

Loading