search

espruino / Espruino

The Espruino JavaScript interpreter - Official Repo
http://www.espruino.com/
Other
2.76k stars 741 forks source link

Merge real_prototype_chain branch #2181

Open gfwilliams opened 2 years ago

gfwilliams commented 2 years ago

Mentioned in:

https://github.com/espruino/Espruino/issues/1348 https://github.com/espruino/Espruino/issues/737

Basically this branch avoids the hacky handling of built-in objects, and implements prototypes properly (rather than as separate to objects). It should also allow much faster lookup of builtins (since the current system uses a large if..then..else)

gfwilliams commented 2 years ago

Branch is: https://github.com/espruino/Espruino/compare/real_prototype_chain?expand=1

I just backed up the 'automatic' changes from scripts/refactor_json_comments.js, since they hide what's going on and will probably be a merge nightmare.

opichals commented 2 years ago

@gfwilliams Yeah, I tried to rebase on top of master over the weekend... it compiles, but looks like I made mistakes as the results are not behaving the same as the old branch. It looks like it doesn't look the 'prototype' correctly for the builtins...

Here https://github.com/opichals/Espruino/tree/real_proto if you would want to have a look.

gfwilliams commented 2 years ago

Thanks! I'm just back from holiday today and still working through messages, so I may not get a chance to look into this for a few days.

Do you have some very simple code that shows the issue?

I found tests/test_array_fill.js which fails, but that maybe seems to be because b = new Array(5); creates an actual instance of the Array object when in that case it should create a jsvNewArray and Espruino should 'fake' the prototype

opichals commented 2 years ago

In the tests/test_pin_pwm.js it seems that the Pin.prototype is always re-created and the modifications (and therefore adding the pwm function doesn't work.

I have also been playing with tests/test_typed_array_sort.js which seems to rather instantiate a generic Object instead of the Uint8Array it should.

opichals commented 2 years ago

Also this https://github.com/opichals/Espruino/commit/4429e29a380af9c48e8718cdc773914958ec3ea5 looks wrong - those are all global/root-visible types and so I should have left the jspNewObject in use for these.

I am getting it correct?

gfwilliams commented 2 years ago

it seems that the Pin.prototype is always re-created

I think normally we'd use jsvCreateNewChild when we create something like Pin.prototype that is new. It's kind of a weak link, so if you're just reading then Pin.prototype never gets created, but if you write to it then it gets added to Pin. That could be missing, which might explain that issue.

However:

tests/test_typed_array_sort.js which seems to rather instantiate a generic Object instead of the Uint8Array it should.

Thanks - that's a nice easy test and it looks like one of the big problems. Just took a look and as far as I can see what's happening is normally jswSymbols_global would have contained a link to gen_jswrap_Uint8Array_Uint8Array which called jswrap_typedarray_constructor. But for some reason build_jswrapper is now ignoring the built-in constructor and is just creating gen_jswrap_global_Uint8Array which is a placeholder which creates an object (rather than the Uint8Array var type we actually want).

I've really got to catch up on all the forum messsages (which is a lot less fun) but I'll try and have a think. Pretty sure the bug is entirely in build_jswrapper though. Looking at the diffs we may also have to be careful - I see FLASH_SECT is missing from at least one decl so this could break the ESP8266 builds too.

Also this https://github.com/opichals/Espruino/commit/4429e29a380af9c48e8718cdc773914958ec3ea5 looks wrong

jspNewObject("LCD", "Graphics"); should create an object called LCD of type Graphics and jspNewHiddenObject wouldn't, so that one needs changing. jspNewHiddenObject mentions:

  /* FIXME: We should try and make sure there aren't duplicates, so if
   * the users adds a function to a prototype on a hidden object, it
   * is available to any instance. But how can this be done? */

I think probably we should actually back out all the jspNewHiddenObject changes and just use jspNewObject for now. It won't be as fast but it won't be any slower than it was before. Then we could look at switching to jspNewHiddenObject later once everything else is working ok and we have a fast solution for handling the different prototype issue.

opichals commented 2 years ago

Thanks for going through the changes. I am sorry I didn't have the capacity to look at this the last week.

I have just dropped the extra jspNewHiddenObject things I added in the branch and dropped the extra console logs I used. And also added back the FLASH_SECT for ESP8266.

gfwilliams commented 2 years ago

I'm just taking a look at this again - it's totally doing my head in.

I'm wondering whether there's a way to split this up into a series of smaller changes that we can make and test one after the other...

Specifically:

opichals commented 2 years ago

The rebase was a bit hairy for myself. I am still getting lost in what JsVar and the JsVar's name meanings, sorry. At this point I am unsure I can really help with this. You as an author could definitely split the work and do a much better job to get it to run at least the same amount of stuff it does in the original branch.

opichals commented 1 year ago

Hey @gfwilliams! :)

I have rebased the https://github.com/opichals/Espruino/tree/real_proto on top of current master.

I was also able to trace the root cause of the tests/test_typed_array_sort.js not using the Uint8Array instance. The constructor was not called because of the patch below was missing.

diff --git a/src/jsparse.c b/src/jsparse.c
index 1e6379990..91f2dd84e 100644
--- a/src/jsparse.c
+++ b/src/jsparse.c
@@ -497,7 +497,7 @@ NO_INLINE JsVar *jspeFunctionCall(JsVar *function, JsVar *functionName, JsVar *t
      *   a) args were pre-parsed and we have to populate the function
      *   b) we parse our own args, which is possibly better
      */
-    if (jsvIsNativeFunction(function)) { // ------------------------------------- NATIVE
+    if (jsvIsNativeFunction(function) || jsvIsNativeObject(function)) { // ------------------------------------- NATIVE
       unsigned int argPtrSize = 0;
       int boundArgs = 0;
gfwilliams commented 1 year ago

Brilliant, thanks for revisiting this! I just went to build it with DEBUG=1 make and I get a bunch of errors. Am I missing something, or were you building for a very specific device?

What's the current state of this? It now runs and passes all the same tests the current build does?

opichals commented 1 year ago

To build the branch I do:

make clean
node ./scripts/refactor_json_comments.js
DEBUG=1 make

The tests results are currently the following:

./bin/espruino --test-all
...
espruino:  322 of 377 tests passed
espruino: FAILS:
espruino: tests/test_new_string_from_constructor.js
espruino: tests/test_promise7.js
espruino: tests/test_array_length.js
espruino: tests/test_ledflash.js
espruino: tests/test_mandelbrot.js
espruino: tests/test_long_string_to_name.js
espruino: tests/test_promise3.js
espruino: tests/test_new_nested2_FAIL.js
espruino: tests/test_array_remove.js
espruino: tests/test_arrays_treated_as_references_in_functions.js
espruino: tests/test_getter.js
espruino: tests/test_exception_8.js
espruino: tests/test_promise2.js
espruino: tests/test_setter.js
espruino: tests/test_dgram_recvBufferSize.js
espruino: tests/test_object_hasOwnProperty.js
espruino: tests/test_this_1.js
espruino: tests/test_crypto.js
espruino: tests/test_promise9.js
espruino: tests/test_builtin_prototype_expansion.js
espruino: tests/test_array_vars_are_references.js
espruino: tests/test_promise_all.js
espruino: tests/test_switch_case_construct.js
espruino: tests/test_promise.js
espruino: tests/test_number_tostring.js
espruino: tests/test_graphics_wrapString.js
espruino: tests/test_graphics_fonts.js
espruino: tests/test_arrays_with_non_array_indices.js
espruino: tests/test_array_reduce.js
espruino: tests/test_uncaughtException.js
espruino: tests/test_string_array_indices.js
espruino: tests/test_crypt_md5.js
espruino: tests/test_array_for_in_loop_iteration_order.js
espruino: tests/manual/bangle2_fill.js
espruino: tests/test_object_proto.js
espruino: tests/test_262_FAIL.js
espruino: tests/test_object_constructor_jgalaor2.js
espruino: tests/test_object_keys_as_vars.js
espruino: tests/test_pin_type.js
espruino: tests/test_array_splice.js
espruino: tests/test_spi_prototype_shared.js
espruino: tests/test_arrow_functions_this.js
espruino: tests/test_array_reverse.js
espruino: tests/test_graphics_drawStringImage.js
espruino: tests/test_arraybuffer_inheritance.js
espruino: tests/test_promise_exception.js
espruino: tests/test_array_big_indices.js
espruino: tests/test_delete.js
espruino: tests/test_object_getOwnPropertyNames.js
espruino: tests/test_number_constructor_FAIL.js
espruino: tests/test_E_getSizeOf.js
espruino: tests/test_isarray.js
espruino: tests/test_object_this.js
espruino: tests/test_object_zero_in_keys.js
espruino: tests/test_tensorflow.js
opichals commented 1 year ago

From what I can see this is still not the same as your original real_prototype_chain problem count :(

Here the Array index assignments are not working properly:

tests/test_array_length.js
tests/test_array_reverse.js

Than there is something with the Object/Array instance prototype access where the tests/test_array_remove.js is signalling:

Uncaught Error: Function "remove" not found!
 at line 14 col 3
a.remove(2);

and e.g. tests/test_this_1.js responding with:

Uncaught Error: Function "foo" not found!
 at line 12 col 9
"Hello".foo(100);
gfwilliams commented 1 year ago

Ok, thanks! I'll try and take a look at this tomorrow - it could be maybe only one or two things that are broken.

gfwilliams commented 1 year ago

Ahh - got it! I had some un-committed files which were causing node ./scripts/refactor_json_comments.js to fail, and then it broke when running it a second time because it doesn't like parsing files it's already worked on :)

Just a note for later - I think jswrap_banglejs_gps_character might be broken now

I did see /home/gw/workspace/Espruino/tests/test_this_2.js failing with an assert because the this var was a name though, so I'm not sure if we have exactly the same code still?

I've spent ages on this so far and it's a bit of a can of worms. But as far as I can see one big issue was if you do: Array.x = 5 then Array.x gets set, but Array is immediately thrown away because it's not referenced. Next time you ask for Array.x it's not found.

As I understand it, there might have been some code to work around this in jspeFactorMember by ensuring that it stored a chain of 'NewChild' vars so that when the value was finally written with jsvReplaceWith everything was remembered - but jsvReplaceWith was never modified to cope with that...

And those changes caused a bunch of other trouble with Functions getting called with this set to a Name rather than the actual object they belonged to.

Before I figured all that out I tried to undo those changes and got a much higher pass rate, but at the end of the day I think they were needed, and had to undo that work. The other option would have been to detect in jsvReplaceWith if the destination was a NativeObject, find out its name, and then add it - but that in itself is tricky and wouldn't fix it at a deeper level like String.fromCharCode.x = 5

Anyway my diffs are below if it helps - I don't seem able to attach them:

diff --git a/src/jsparse.c b/src/jsparse.c
index f4f95c6f7..57bcda697 100644
--- a/src/jsparse.c
+++ b/src/jsparse.c
@@ -1079,16 +1078,15 @@ NO_INLINE JsVar *jspeFactorMember(JsVar *a, JsVar **parentResult) {
               // if no child found, create a pointer to where it could be
               // as we don't want to allocate it until it's written
               JsVar *nameVar = jsvNewNameFromString(jslGetTokenValueAsString());
-              child = jsvCreateNewChild(aVar, nameVar, 0);
+              child = jsvCreateNewChild(a, nameVar, 0);
               jsvUnLock(nameVar);
             } else {
               // could have been a string...
               jsExceptionHere(JSET_ERROR, "Cannot read property '%s' of %s", name, jsvIsUndefined(aVar) ? "undefined" : "null");
             }
           }
-          jsvUnLock(parent);
-          parent = aVar;
-          jsvUnLock(a);
+          jsvUnLock2(parent, aVar);
+          parent = a;
           a = child;
         }
         // skip over current token (we checked above that it was an ID or reserved word)
@@ -1267,8 +1265,10 @@ NO_INLINE JsVar *jspeFactorFunctionCall() {
       bool parseArgs = lex->tk=='(';
       a = jspeConstruct(func, funcName, parseArgs);
       isConstructor = false; // don't treat subsequent brackets as constructors
-    } else
+    } else {
+      parent = jsvSkipNameAndUnLock(parent); // 'parent' could be a NewChild now
       a = jspeFunctionCall(func, funcName, parent, true, 0, 0);
+    }
     jsvUnLock3(funcName, func, parent);
     parent=0;
     a = jspeFactorMember(a, &parent);
@@ -1464,7 +1464,7 @@ NO_INLINE JsVar *jspeFactorTypeOf() {
 NO_INLINE JsVar *jspeFactorDelete() {
   JSP_ASSERT_MATCH(LEX_R_DELETE);
   JsVar *parent = 0;
-  JsVar *a = jsvSkipNameAndUnLock(jspeFactorMember(jspeFactor(), &parent));
+  JsVar *a = jspeFactorMember(jspeFactor(), &parent);
   JsVar *result = 0;
   if (JSP_SHOULD_EXECUTE) {
     bool ok = false;
diff --git a/src/jsvar.c b/src/jsvar.c
index 19e824c93..882b68ec7 100644
--- a/src/jsvar.c
+++ b/src/jsvar.c
@@ -2220,6 +2220,22 @@ void jsvReplaceWith(JsVar *dst, JsVar *src) {
   if (jsvIsNewChild(dst)) {
     // Get what it should have been a child of
     JsVar *parent = jsvLock(jsvGetNextSibling(dst));
+    if (jsvIsNewChild(parent)) { // check if this NewChild links to another NewChild (eg Array.x=1 where Array was never used before)
+      // FIXME: what about great grandparents?
+      JsVar *grandparent = jsvLock(jsvGetNextSibling(parent));
+      // Remove the 'new child' flagging
+      jsvUnRef(grandparent);
+      jsvSetNextSibling(parent, 0);
+      jsvUnRef(grandparent);
+      jsvSetPrevSibling(parent, 0);
+      // Add to the grandparent
+      jsvAddName(grandparent, parent);
+      jsvUnLock(grandparent);
+      parent = jsvSkipNameAndUnLock(parent);
+      jsvRef(parent); // no idea why we need this
+      jsvRef(parent); // no idea why we need this
+    }
+    //jsiConsolePrintf("PARENT ");jsvTrace(parent,0);
     if (!jsvIsString(parent)) {
       // if we can't find a char in a string we still return a NewChild,
       // but we can't add character back in
@@ -2378,8 +2394,10 @@ JsVar *jsvSkipNameWithParent(JsVar *a, bool repeat, JsVar *parent) {
   }
 #ifndef SAVE_ON_FLASH
   if (jsvIsGetterOrSetter(pa)) {
-    JsVar *getterParent = jsvIsNewChild(a)?jsvLock(jsvGetNextSibling(a)):0;
-    JsVar *v = jsvExecuteGetter(getterParent?getterParent:parent, pa);
+    // FIXME tests/test_getter.js is now broken - might be related
+    JsVar *getterParent = jsvIsNewChild(a)?jsvSkipNameAndUnLock(jsvLock(jsvGetNextSibling(a))):0;
+    if (!getterParent) getterParent = jsvSkipName(parent);
+    JsVar *v = jsvExecuteGetter(getterParent, pa);
     jsvUnLock2(getterParent,pa);
     pa = v;
   }
@@ -3827,10 +3844,14 @@ void _jsvTrace(JsVar *var, int indent, JsVar *baseVar, int level) {
   }

   if (jsvIsNewChild(var)) {
-    jsiConsolePrint("NewChild PARENT:");
+    jsiConsolePrint("NewChild\n");
+    for (i=0;i<indent;i++) jsiConsolePrint(" ");
+    jsiConsolePrint(" PARENT: ");
     JsVar *parent = jsvGetAddressOf(jsvGetNextSibling(var));
     _jsvTrace(parent, indent+2, baseVar, level+1);
-    jsiConsolePrint("CHILD: ");
+    jsiConsolePrint("\n");
+    for (i=0;i<indent;i++) jsiConsolePrint(" ");
+    jsiConsolePrint(" CHILD: ");
   } else if (jsvIsName(var)) jsiConsolePrint("Name ");

   char endBracket = ' ';
diff --git a/src/jswrap_object.c b/src/jswrap_object.c
index a05914d13..1f098a6ec 100644
--- a/src/jswrap_object.c
+++ b/src/jswrap_object.c
@@ -24,25 +24,33 @@
 #endif//__MINGW32__

 /*JSON{
-  "type" : "class",
-  "class" : "Object",
+  "type" : "variable",
+  "name" : "global",
+  "memberOf" : "global",
+  "generate_full" : "jsvLockAgain(execInfo.root)",
+  "return" : ["JsVar","An Object"]
+}
+This references the global scope. For instance `global.print == print`
+*/
+/*JSON{
+  "type" : "object",
+  "name" : "Object",
   "memberOf" : "global",
   "check" : "jsvIsObject(var)"
 }
 This is the built-in class for Objects

With that I'm still only seeing 308 passes.

gfwilliams commented 1 year ago

I realised that in normal Espruino we just add any object that is a child of the global object automatically and there is no need for the stuff in ReplaceWith- so actually my original hunch was probably right. On Monday I'll have a go at making it more like the original code - which should increase the pass rate significantly

gfwilliams commented 1 year ago

Getting there now! 368 tests passing - only a few that aren't.

gfwilliams commented 1 year ago

I have my changes on a branch: https://github.com/espruino/Espruino/compare/master...gfwilliams:Espruino:real_proto_opichals

Note that I have one commit with the result of refactor_json_comments in just so I can avoid diffs all over the place - when merging I'll remove that and then do one big commit based on the most recent changes.

As of now, the only test fail that doesn't fail on the main repo is:

... so looking quite good. As mentioned in https://github.com/espruino/Espruino/issues/2181#issuecomment-1548146740 there are still a few outstanding things though