Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"super" is null in static method of sub-class #41261

Closed
matthieusieben opened this issue Dec 21, 2021 · 9 comments
Closed

"super" is null in static method of sub-class #41261

matthieusieben opened this issue Dec 21, 2021 · 9 comments
Labels
v8 engine Issues and PRs related to the V8 dependency.

Comments

@matthieusieben
Copy link
Contributor

matthieusieben commented Dec 21, 2021

Version

v17.2.0

Platform

Darwin MBPM.local 21.1.0 Darwin Kernel Version 21.1.0: Wed Oct 13 17:33:23 PDT 2021; root:xnu-8019.41.5~1/RELEASE_X86_64 x86_64

Subsystem

No response

What steps will reproduce the bug?

class A {
  getValue() {
    return 'A'
  }

  static extend() {
    return class extends this {
      getValue() {
        return 'A.extend:' + super.getValue()
      }
    }
  }
}

class B extends A {
  getValue() {
    return 'B:' + super.getValue()
  }

  static extend() {
    return class extends super.extend() {
      getValue() {
        return 'B.extend:' + super.getValue()
      }
    }
  }
}

const C = B.extend()

const c = new C()

console.log(c.getValue()) // B.extend:A.extend:B:A

How often does it reproduce? Is there a required condition?

  • It works on node <= 15
  • If the return class extends super.extend() declares no new method, there is no error.
  • If an intermediary variable is used, then it just works. If instead of this:
return class extends super.extend() {
  // ...
}

We write this:

const Parent = super.extend()
return class extends Parent {
  // ...
}

there will be no error.

What is the expected behavior?

The output should be B.extend:A.extend:B:A

What do you see instead?

/Users/msi/tmp/test.js:21
    return class extends super.extend() {
                               ^

TypeError: Cannot read properties of null (reading 'extend')
    at Function.extend (/Users/msi/tmp/test.js:21:32)
    at Object.<anonymous> (/Users/msi/tmp/test.js:29:13)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:17:47

Additional information

The code works as is in older versions of Node:
Capture d’écran 2021-12-21 à 15 37 02

@aduh95 aduh95 added the v8 engine Issues and PRs related to the V8 dependency. label Dec 22, 2021
@aduh95
Copy link
Contributor

aduh95 commented Dec 22, 2021

Same behavior on Chromium; Firefox and Safari output B.extend:A.extend:B:A. As Chromium, Node.js relies on the V8 JavaScript engine to interpret JS code, this issue should be reported to them instead: https://bugs.chromium.org/p/chromium/issues/entry

@matthieusieben
Copy link
Contributor Author

@dnalborczyk
Copy link
Contributor

dnalborczyk commented Dec 23, 2021

I'm fairly sure you can only extend from a constructor function. this is undefined in your case.

return class extends this {

if you only do:

class A {
	static foo() {
		console.log(this)
	} 
}

A.foo() // undefined

that would be the same as:

class A extends undefined {}
// TypeError: Class extends value undefined is not a constructor or null

if it did work before, and does not anymore, it's likely a bug which has been fixed.

@aduh95
Copy link
Contributor

aduh95 commented Dec 23, 2021

this is undefined in your case.

That's precisely the bug that is being reported, the behavior of V8 is different from other JS engines. If you try your code snippet in Firefox or Safari, you get a different result – and their behavior makes more sense imo, I'd expect A.foo() to be equivalent to Reflect.apply(A.foo, A, []).

@aduh95
Copy link
Contributor

aduh95 commented Dec 23, 2021

@dnalborczyk after checking in Node.js v17.x, I'm not getting this is undefined, it's equal to A:

class A {
	static foo() {
		console.log(this === undefined) // false
                return this;
	} 
}

A.foo() // [class A]

Did you see this result with a recent version of Node.js? If so, we should definitely open a bug for that!

I'm fairly sure you can only extend from a constructor function.

Unrelated, but you can also extend from null:

class A extends null {}

@dnalborczyk
Copy link
Contributor

dnalborczyk commented Dec 23, 2021

@dnalborczyk after checking in Node.js v17.x, I'm not getting this is undefined, it's equal to A:

no, you are absolutely right. I should not comment on any issues late a night. I broke the rule once again 😞 I'm sure console.log(...) returning undefined tripped me off again. actually, it's likely the call to A.foo() displaying undefined in the console which tripped me off.

Unrelated, but you can also extend from null:

class A extends null {}

you are right, you can do that. I think tho the behavior is still not (well) defined. there is a longstanding issue open at TC39: tc39/ecma262#1321

if you instantiate A you get a TypeError Uncaught TypeError: Super constructor null of A is not a constructor

if I remember correctly, some are arguing it should behave similar to Object.create(null), that the extended class would not inherit from the prototype of object.

@matthieusieben
Copy link
Contributor Author

doing class A extends null {} is not what this "issue" is about.

The problem here is that super in super.extend() is (incorrectly) null in a very specific case.

@devsnek
Copy link
Member

devsnek commented Dec 23, 2021

I'm going to close this since it's v8 and there's a bug for it.

@devsnek devsnek closed this as completed Dec 23, 2021
@dnalborczyk
Copy link
Contributor

doing class A extends null {} is not what this "issue" is about.

The problem here is that super in super.extend() is (incorrectly) null in a very specific case.

I think we got that. the conversation just derailed a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

4 participants