Fix issue extending a class from a class from a class (fix #1529)

espruino · Feb 22, 2023 · 435b4da · 435b4da
1 parent f1f995e
commit 435b4da
Show file tree

Hide file tree

Showing 5 changed files with 142 additions and 59 deletions.
diff --git a/ChangeLog b/ChangeLog
@@ -44,6 +44,7 @@
             nRF52: Add support for 7 bit UART tx/rx (and parity) and error if a UART setting can't be honoured (fix #2324)
             Bangle.js2: Fix regression in E.showMenu so you can use menu items with ':undefined' (as in the example in the reference)
             Graphics: Ensure that an error is thrown if a palette is used in >8 bit images. It was previously possible to ask for a palette on a 32 bit image, which caused an overflow
+            Fix issue extending a class from a class from a class (fix #1529)
 
      2v16 : JIT functions now execute in their own function scope (allows arguments)
             Move older 'HY' boards to use `g` for the built-in graphics, not `LCD` (and change docs)

diff --git a/src/jsparse.c b/src/jsparse.c
@@ -1209,20 +1209,80 @@ NO_INLINE JsVar *jspeFactorFunctionCall() {
   }
 
   JsVar *parent = 0;
+  JsVar *a = 0;
+
 #ifndef ESPR_NO_CLASSES
-  bool wasSuper = lex->tk==LEX_R_SUPER;
-#endif
-  JsVar *a = jspeFactorMember(jspeFactor(), &parent);
-#ifndef ESPR_NO_CLASSES
-  if (wasSuper) {
-    /* if this was 'super.something' then we need
-     * to overwrite the parent, because it'll be
-     * set to the prototype otherwise.
+  bool hasSetCurrentClassConstructor = false;
+  if (lex->tk==LEX_R_SUPER) {
+    JSP_ASSERT_MATCH(LEX_R_SUPER);
+    /* We would normally handle this in jspeFactor, but we're doing it here
+     as we have access to parent, which we need to be able to set because
+     super() is a bit like calling 'this.super()'. We also need to do some
+     hacky stuff to ensure
+
+     This is kind of nasty, since super appears to do
+     three different things depending on what 'this' is.
+
+      * In the constructor it references the extended class's constructor
+      * in a method it references the constructor's prototype.
+      * in a static method it references the extended class's constructor (but this is different)
      */
-    jsvUnLock(parent);
-    parent = jsvLockAgainSafe(execInfo.thisVar);
-  }
+    if (jsvIsObject(execInfo.thisVar)) {
+      // 'this' is an object - must be calling a normal method
+      JsVar *proto1;
+      if (execInfo.currentClassConstructor && lex->tk=='(') {
+        // If we're doing super() we want the constructor. We have to be careful
+        // we don't keep asking for the same one so don't get it from 'this' which has to be the
+        // same each time https://github.com/espruino/Espruino/issues/1529
+        proto1 = jsvObjectGetChild(execInfo.currentClassConstructor, JSPARSE_PROTOTYPE_VAR, 0);
+      } else {
+        proto1 = jsvObjectGetChild(execInfo.thisVar, JSPARSE_INHERITS_VAR, 0); // if we're in a method, get __proto__ first
+
+      }
+      JsVar *proto2 = jsvIsObject(proto1) ? jsvObjectGetChild(proto1, JSPARSE_INHERITS_VAR, 0) : 0; // still in method, get __proto__.__proto__
+      jsvUnLock(proto1);
+      if (!proto2) {
+        jsExceptionHere(JSET_SYNTAXERROR, "Calling 'super' outside of class");
+        return 0;
+      }
+      // If we're doing super() we want the constructor
+      if (lex->tk=='(') {
+        JsVar *constr = jsvObjectGetChild(proto2, JSPARSE_CONSTRUCTOR_VAR, 0);
+        jsvUnLock(proto2);
+        execInfo.currentClassConstructor = constr;
+        hasSetCurrentClassConstructor = true;
+        a = constr;
+      } else {
+        // But if we're doing something else - eg 'super.' or 'super[' then it needs to reference the prototype
+        a = proto2;
+      }
+    } else if (jsvIsFunction(execInfo.thisVar)) {
+      // 'this' is a function - must be calling a static method
+      JsVar *proto1 = jsvObjectGetChild(execInfo.thisVar, JSPARSE_PROTOTYPE_VAR, 0);
+      JsVar *proto2 = jsvIsObject(proto1) ? jsvObjectGetChild(proto1, JSPARSE_INHERITS_VAR, 0) : 0;
+      jsvUnLock(proto1);
+      if (!proto2) {
+        jsExceptionHere(JSET_SYNTAXERROR, "Calling 'super' outside of class");
+        return 0;
+      }
+      JsVar *constr = jsvObjectGetChild(proto2, JSPARSE_CONSTRUCTOR_VAR, 0);
+      jsvUnLock(proto2);
+      a = constr;
+    } else {
+      jsExceptionHere(JSET_SYNTAXERROR, "Calling 'super' outside of class");
+      return 0;
+    }
+    // search for member accesses eg 'super.xyz'...
+    JsVar *superVar = a;
+    a = jspeFactorMember(a, &parent);
+    // Set the parent to 'this' if it was undefined or equal to the 'super' var
+    if (!parent || parent==superVar) {
+      jsvUnLock(parent);
+      parent = jsvLockAgain(execInfo.thisVar);
+    }
+  } else
 #endif
+  a = jspeFactorMember(jspeFactor(), &parent);
 
   while ((lex->tk=='(' || (isConstructor && JSP_SHOULD_EXECUTE)) && !jspIsInterrupted()) {
     JsVar *funcName = a;
@@ -1237,11 +1297,15 @@ NO_INLINE JsVar *jspeFactorFunctionCall() {
       isConstructor = false; // don't treat subsequent brackets as constructors
     } else
       a = jspeFunctionCall(func, funcName, parent, true, 0, 0);
-
     jsvUnLock3(funcName, func, parent);
     parent=0;
     a = jspeFactorMember(a, &parent);
   }
+#ifndef ESPR_NO_CLASSES
+  if (hasSetCurrentClassConstructor)
+    execInfo.currentClassConstructor = 0;
+#endif
+
 #ifndef ESPR_NO_GET_SET
   /* If we've got something that we care about the parent of (eg. a getter/setter)
    * then we repackage it into a 'NewChild' name that references the parent before
@@ -1612,21 +1676,29 @@ NO_INLINE JsVar *jspeClassDefinition(bool parseNamedClass) {
   }
   if (lex->tk==LEX_R_EXTENDS) {
     JSP_ASSERT_MATCH(LEX_R_EXTENDS);
-    JsVar *extendsFrom = actuallyCreateClass ? jsvSkipNameAndUnLock(jspGetNamedVariable(jslGetTokenValueAsString())) : 0;
+    JsVar *extendsFromName = 0;
+    JsVar *extendsFrom = 0;
+    if (actuallyCreateClass) {
+      extendsFromName = jspGetNamedVariable(jslGetTokenValueAsString());
+      extendsFrom = jsvSkipName(extendsFromName);
+    }
     JSP_MATCH_WITH_CLEANUP_AND_RETURN(LEX_ID,jsvUnLock4(extendsFrom,classFunction,classInternalName,classPrototype),0);
     if (classPrototype) {
       if (jsvIsFunction(extendsFrom)) {
         JsVar *extendsFromProto = jsvObjectGetChild(extendsFrom, JSPARSE_PROTOTYPE_VAR, 0);
         if (extendsFromProto) {
           jsvObjectSetChild(classPrototype, JSPARSE_INHERITS_VAR, extendsFromProto);
           // link in default constructor if ours isn't supplied
-          jsvObjectSetChildAndUnLock(classFunction, JSPARSE_FUNCTION_CODE_NAME, jsvNewFromString("if(this.__proto__.__proto__.constructor)this.__proto__.__proto__.constructor.apply(this,arguments)"));
+          // We must reference it explicitly to ensure that we call the correct one when extending from a class that's already extended
+          // see https://github.com/espruino/Espruino/issues/1529#issuecomment-655217322
+          jsvObjectSetChildAndUnLock(classFunction, JSPARSE_FUNCTION_CODE_NAME,
+              jsvVarPrintf("%v.apply(this,arguments)", extendsFromName));
           jsvUnLock(extendsFromProto);
         }
       } else
-        jsExceptionHere(JSET_SYNTAXERROR, "'extends' argument should be a function, got %t", extendsFrom);
+        jsExceptionHere(JSET_SYNTAXERROR, "'extends' argument %q should be a function, got %t", extendsFromName, extendsFrom);
     }
-    jsvUnLock(extendsFrom);
+    jsvUnLock2(extendsFrom, extendsFromName);
   }
   JSP_MATCH_WITH_CLEANUP_AND_RETURN('{',jsvUnLock3(classFunction,classInternalName,classPrototype),0);
 
@@ -1780,48 +1852,7 @@ NO_INLINE JsVar *jspeFactor() {
     if (!jspCheckStackPosition()) return 0;
     JSP_ASSERT_MATCH(LEX_R_CLASS);
     return jspeClassDefinition(true);
-  } else if (lex->tk==LEX_R_SUPER) {
-    JSP_ASSERT_MATCH(LEX_R_SUPER);
-    /* This is kind of nasty, since super appears to do
-      three different things.
-
-      * In the constructor it references the extended class's constructor
-      * in a method it references the constructor's prototype.
-      * in a static method it references the extended class's constructor (but this is different)
-     */
-
-    if (jsvIsObject(execInfo.thisVar)) {
-      // 'this' is an object - must be calling a normal method
-      JsVar *proto1 = jsvObjectGetChild(execInfo.thisVar, JSPARSE_INHERITS_VAR, 0); // if we're in a method, get __proto__ first
-      JsVar *proto2 = jsvIsObject(proto1) ? jsvObjectGetChild(proto1, JSPARSE_INHERITS_VAR, 0) : 0; // still in method, get __proto__.__proto__
-      jsvUnLock(proto1);
-      if (!proto2) {
-        jsExceptionHere(JSET_SYNTAXERROR, "Calling 'super' outside of class");
-        return 0;
-      }
-      // If we're doing super() we want the constructor
-      if (lex->tk=='(') {
-        JsVar *constr = jsvObjectGetChild(proto2, JSPARSE_CONSTRUCTOR_VAR, 0);
-        jsvUnLock(proto2);
-        return constr;
-      }
-      // But if we're doing something else - eg 'super.' or 'super[' then it needs to reference the prototype
-      return proto2;
-    } else if (jsvIsFunction(execInfo.thisVar)) {
-      // 'this' is a function - must be calling a static method
-      JsVar *proto1 = jsvObjectGetChild(execInfo.thisVar, JSPARSE_PROTOTYPE_VAR, 0);
-      JsVar *proto2 = jsvIsObject(proto1) ? jsvObjectGetChild(proto1, JSPARSE_INHERITS_VAR, 0) : 0;
-      jsvUnLock(proto1);
-      if (!proto2) {
-        jsExceptionHere(JSET_SYNTAXERROR, "Calling 'super' outside of class");
-        return 0;
-      }
-      JsVar *constr = jsvObjectGetChild(proto2, JSPARSE_CONSTRUCTOR_VAR, 0);
-      jsvUnLock(proto2);
-      return constr;
-    }
-    jsExceptionHere(JSET_SYNTAXERROR, "Calling 'super' outside of class");
-    return 0;
+  // LEX_R_SUPER is handled in jspeFactorFunctionCall now
 #endif
   } else if (lex->tk==LEX_R_THIS) {
     JSP_ASSERT_MATCH(LEX_R_THIS);

diff --git a/src/jsparse.h b/src/jsparse.h
@@ -146,6 +146,10 @@ typedef struct {
 #endif
   /// Value of 'this' reserved word
   JsVar *thisVar;
+#ifndef ESPR_NO_CLASSES
+  // Allows 'super' to call 'super' on the correct class on subclasses - see #1529
+  JsVar *currentClassConstructor;
+#endif
 
   volatile JsExecFlags execute; //!< Should we be executing, do we have errors, etc
 } JsExecInfo;

diff --git a/tests/test_class.js b/tests/test_class.js
@@ -41,7 +41,6 @@ class Lion extends Cat {
     return super.speak()+this.name + ' roars.';
   }
   static isReallyADog() {
-    trace(super);
     return super.isDog();
   }
 }

diff --git a/tests/test_class_multiple.js b/tests/test_class_multiple.js
@@ -0,0 +1,48 @@
+// Issue 1529 - part 1
+
+class A {
+    constructor(n) {
+        this.name = n;
+    }
+}
+class B extends A {
+}
+class C extends B {
+}
+new A(); // ok
+new B(); // ok
+var cInst = new C("Test"); // was stack overflow
+
+// Issue 1529 - part 2
+class Being  { 
+    constructor(theLabel) {
+        this.label = theLabel;
+    }
+
+    name() {
+        return this.label;
+    }
+}
+
+class Animal extends Being { 
+    constructor(theLabel) {
+        super(theLabel);
+    }
+}
+
+class Dog extends Animal { 
+    constructor(theLabel) {
+        super(theLabel);
+    }
+}
+
+class Cat extends Animal {
+    constructor(theLabel) {
+        super(theLabel);
+    }
+ }
+
+
+const c = new Cat("Felix");
+const d = new Dog("Fido");
+result = cInst.name=="Test" && c.name()=="Felix" && d.name()=="Fido";