Make the assert an external method #170

galpeter · 2015-06-10T15:33:34Z

Remove the assert intrinsic from the parser and instead
make it an external method. This allows us to redefine
the method if needed during runtime from JS.

JerryScript-DCO-1.0-Signed-off-by: Peter Gal pgal.u-szeged@partner.samsung.com

galpeter · 2015-06-10T15:34:34Z

Related issue: #28

ruben-ayrapetyan · 2015-06-10T15:36:20Z

@galpeter, would the following constructions work correctly with the patch?

try
{
   ... // should throw TypeError
   assert (false);
}
catch (e)
{
   assert (e instanceof TypeError);
}

ruben-ayrapetyan · 2015-06-10T15:44:03Z

jerry-core/ecma/base/ecma-magic-strings.inc.h

@@ -212,4 +212,6 @@ ECMA_MAGIC_STRING_DEF (ECMA_MAGIC_STRING__EMPTY, "")
 /*
 * Implementation-defined magic strings
 */
+ECMA_MAGIC_STRING_DEF (ECMA_MAGIC_STRING_ASSERT, "assert")
+ECMA_MAGIC_STRING_DEF (ECMA_MAGIC_STRING_ASSERT_FAILED, "Assertion failed")


The following convention is used for magic string identifiers:

value of magic string ECMA_MAGIC_STRING_[identifier][^_CHAR|_U|_UL} is lower-cased "identifier";

value of magic string ECMA_MAGIC_STRING_[identifier]_U is upper-cased "identifier";

value of magic string ECMA_MAGIC_STRING_[identifier]_UL is "identifier" with mixed upper-case and lower-case characters;

value of magic string ECMA_MAGIC_STRING_[identifier]_CHAR is a character;

value of magic string ECMA_MAGIC_STRING__[identifier] is a string described by identifier, but could not be described using other cases.

Seems that we should use either __ case here, or introduce some new descriptor suffix like P for magic strings with phrases.

Okay. Didn't know about these conventions. Maybe we should add it to the start of the file?
Anyways In think this case we should go with the __ version.

I think it is good idea to add comment with the conventions description to the file.

galpeter · 2015-06-10T15:47:28Z

@ruben-ayrapetyan, I think yes as the error thrown by the assert is a common Error and not a TypeError. What I did basically is something like @egavrin described in the #28 issue (2nd comment).

Consider the following code:

try {
    throw new TypeError("MyError");
    assert(false, "message");
} catch (e) {
    print(e.toString());
    assert (e instanceof TypeError);
}

will print out the TypeError: MyError string and exits with an exit code of 0.

ruben-ayrapetyan · 2015-06-10T15:54:26Z

@galpeter, I agree, in the case TypeError is actually thrown, and assertion is successful, the code works as expected.

Otherwise, if TypeError is not thrown, the following would occur:

assert would throw an exception (Error);
the exception would be catched by catch block;
Error: message message would be printed;
the second assert would also fail as type of exception is not TypeError, as expected, and will raise new Error exception;
the exception would be unhandled.

Is the behaviour expected?

galpeter · 2015-06-10T16:16:08Z

@ruben-ayrapetyan, I my eyes yes. One thing is true, it would change the behavior of the assert a bit.

Consider the following code:

try {
    assert(false);
} catch (e) {
    print(e.toString());
    assert (e instanceof TypeError);
}
print("error");

With the patch the output will be

ReferenceError: MyError

with an exit code of 1.

Without the patch the assert exits directly (thus nothing is displayed) and the exit code will be 1.

A few assert implementations of the assert just throw an Error, for example:

but there are other implementations which simply outputs a text to the console and continues the execution.

ruben-ayrapetyan · 2015-06-10T16:45:52Z

@galpeter, thank you for explanation of your view point.

ruben-ayrapetyan · 2015-06-11T20:56:32Z

@galpeter, we've discussed the behaviour a bit.

Please, consider the following hypothetical js code:

// ...
var STATE_CALCULATION = 0, STATE_IO = 1;
state = ...;
// ...

function g () {
  // the function should only be called in IO state
  assert (state == STATE_IO);

    if (!IO.have_data ()) {
      throw new Error ("IO is not ready");
    } else {
      return IO.read (); 
    }
}

//
// ...
//
// @return amount of data read, or -1, if IO is not ready
function f () {
  try {
    data_read = g ();
    // ...
  } catch (e)
  {
    // the only possible error type that 'g' could throw is the Error
    assert (e instanceof Error);

    return -1;
  }
}

// ...
// call to 'f'
// ...

In this hypothetic example, if the assertion assert (state == STATE_IO) would fail in function g, then function f would be confused about cause of fail and it would return value, meaning that 'IO is not ready', while the real cause of fail is unfulfilled precondition of calling g.

So, it seems that throwing ECMA-defined error type in case of implementation-defined 'assertion failure' behaviour can lead to misunderstanding.

Please, consider checking the following references:

Neither of the implementations (at least of versions that I've checked) throw instance of Error upon assertion failure.

First of them assert throws assert.AssertionError exception.
Second and third just write a log message and continue execution, as you've described in your comment.

Although throwing assert.AssertionError seems to be better than just throwing Error, there are recommendations that all error objects (including implementation-defined) should have Error.prototype in their prototype chain. This would imply that instance of AssertionError should fulfill e instanceof Error condition, so confusion, described above, remains in the case.

From other view point, we should consider use cases of our assert implementation. While the implementations, described above, are providing a way to test external JS applications, our assert seems to be targeted for debugging engine itself. Seems that assert, providing a way to test external JS applications should be implemented externally, through Embedding API.

Considering all of the above, it seems preferable to use implementation that:

prints message to stdout / stderr in debug mode;
sets flag 'assertion failure' that could be either examined with internal interfaces to return 'assertion failure' status code, or examined through an interface of Embedding API.

What do you think about this?

galpeter · 2015-06-12T07:03:25Z

@ruben-ayrapetyan, certainly there are drawbacks if the assert throws an Error.

Considering all of the above, it seems preferable to use implementation that:

prints message to stdout / stderr in debug mode;

sets flag 'assertion failure' that could be either examined with internal interfaces to return 'assertion failure' status code, or examined through an interface of Embedding API.

That works also I think. Upon reading over your message one other solution occurred to me:
What if we remove the assert built-in/opcode and just provide our own assert method externally in the main-linux.cpp file (via jerry_extend_with) ? This way the embedder of JerryScript will have to supply his/her own assert if needed and we can implement the assert however we want without worrying that somebody misuses it when embedding the engine.

ruben-ayrapetyan · 2015-06-13T14:10:21Z

@galpeter,

What if we remove the assert built-in/opcode and just provide our own assert method externally in the main-linux.cpp file (via jerry_extend_with) ? This way the embedder of JerryScript will have to supply his/her own assert if needed and we can implement the assert however we want without worrying that somebody misuses it when embedding the engine.

I think, the idea is great!

Seems that currently it is better to use jerry_api_create_external_function for adding and registering the assert method, as 'plugin' model of engine extension would probably be significantly reworked or, may be, removed.

seanshpark · 2015-06-14T23:43:44Z

What if we remove the assert built-in/opcode and just provide our own assert method externally in the main-linux.cpp file (via jerry_extend_with) ?

Splendid.

galpeter · 2015-06-17T15:47:15Z

@ruben-ayrapetyan, I've updated the PR to provide the assert method via the jerry_api_create_external_function function and for now it throws a string value as an error, which maybe is not the best idea, but for testing the engine I think is enough for now.

ruben-ayrapetyan · 2015-06-17T15:59:24Z

main-linux.cpp

@@ -253,6 +301,9 @@ main (int argc,
        }
      }

+      jerry_api_release_value (&assert_value);
+      jerry_api_release_object (global_obj_p);


These release calls could be moved to line 284.

Okay, will do.

galpeter · 2015-06-17T16:18:14Z

I've update the PR, now it prints out the Script assertion failed message and exits when an failed assert is reached.

ruben-ayrapetyan · 2015-06-17T16:20:49Z

main-linux.cpp

+  {
+    JERRY_ERROR_MSG ("Script assertion failed\n");
+    exit (JERRY_STANDALONE_EXIT_CODE_FAIL);
+    return false;


Seems that return false would never be executed. So, we don't need the line, and also @return section need to be changed to just true.

Ahh.. yeah :) I've updated the PR with the requested changes.

ruben-ayrapetyan · 2015-06-17T16:27:36Z

main-linux.cpp

+/**
+ * Provide the 'assert' implementation for the engine.
+ *
+ * @return true - if the argument was not a boolean value or it was false.


if the argument is not boolean false, isn't it?
Currently the comment means "true, if assert (false) is called".

ahhh, yeah I wanted to write or it was boolean true

comment updated

Removed the internal assert implementation from the engine and provide externally an assert function via api calls. JerryScript-DCO-1.0-Signed-off-by: Peter Gal pgal.u-szeged@partner.samsung.com

ruben-ayrapetyan · 2015-06-17T16:34:58Z

Looks good to me.

egavrin · 2015-06-19T12:43:39Z

make push

zherczeg · 2015-06-23T12:10:25Z

jerry-core/parser/js/opcodes-dumper.cpp

-    {
-      return true;
-    }
-  }
  return false;
 }


Shouldn't we remove these two functions at all? They seems unnecessary.

Maybe that would be better in a different PR where all intrinsic like things are removed.

zherczeg · 2015-06-23T12:12:19Z

LGTM

galpeter · 2015-06-23T16:25:43Z

Rebased & merged: 77b01a6

galpeter added the ecma builtins Related to ECMA built-in routines label Jun 10, 2015

ruben-ayrapetyan reviewed Jun 10, 2015
View reviewed changes

ruben-ayrapetyan added enhancement An improvement parser Related to the JavaScript parser labels Jun 10, 2015

ruben-ayrapetyan assigned egavrin Jun 10, 2015

galpeter force-pushed the assert_builtin branch 3 times, most recently from 9017696 to c8796ec Compare June 11, 2015 11:19

egavrin added this to the Core ECMA features milestone Jun 15, 2015

egavrin assigned galpeter and unassigned egavrin Jun 15, 2015

galpeter force-pushed the assert_builtin branch 2 times, most recently from 308fc59 to b76bd72 Compare June 17, 2015 15:46

ruben-ayrapetyan reviewed Jun 17, 2015
View reviewed changes

galpeter changed the title ~~Make the assert a built-in method~~ Make the assert an external method Jun 17, 2015

galpeter force-pushed the assert_builtin branch from b76bd72 to 221d2a4 Compare June 17, 2015 16:16

ruben-ayrapetyan reviewed Jun 17, 2015
View reviewed changes

galpeter force-pushed the assert_builtin branch from 221d2a4 to afe3355 Compare June 17, 2015 16:24

ruben-ayrapetyan reviewed Jun 17, 2015
View reviewed changes

Provide assert as an external method.

d1bc950

Removed the internal assert implementation from the engine and provide externally an assert function via api calls. JerryScript-DCO-1.0-Signed-off-by: Peter Gal pgal.u-szeged@partner.samsung.com

galpeter force-pushed the assert_builtin branch from afe3355 to d1bc950 Compare June 17, 2015 16:33

zherczeg reviewed Jun 23, 2015
View reviewed changes

galpeter closed this Jun 23, 2015

This was referenced May 17, 2020

stack-overflow in vm_loop #3750

Closed

stack-overflow in ecma_regexp_match #3753

Closed

Make the assert an external method #170

Make the assert an external method #170

Uh oh!

Conversation

galpeter commented Jun 10, 2015

Uh oh!

galpeter commented Jun 10, 2015

Uh oh!

ruben-ayrapetyan commented Jun 10, 2015

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!

galpeter commented Jun 10, 2015

Uh oh!

ruben-ayrapetyan commented Jun 10, 2015

Uh oh!

galpeter commented Jun 10, 2015

Uh oh!

ruben-ayrapetyan commented Jun 10, 2015

Uh oh!

ruben-ayrapetyan commented Jun 11, 2015

Uh oh!

galpeter commented Jun 12, 2015

Uh oh!

ruben-ayrapetyan commented Jun 13, 2015

Uh oh!

seanshpark commented Jun 14, 2015

Uh oh!

galpeter commented Jun 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galpeter commented Jun 17, 2015

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!

ruben-ayrapetyan commented Jun 17, 2015

Uh oh!

egavrin commented Jun 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Jun 23, 2015

Uh oh!

galpeter commented Jun 23, 2015

Uh oh!

Uh oh!