Return a more descriptive error message than "|this| used uninitialized in ... constructor"

RESOLVED FIXED in Firefox 58

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: sole, Assigned: mcdonalds.only)

Tracking

(Blocks: 1 bug, {triage-deferred})

Trunk
mozilla58
triage-deferred
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

If you try to use this in a class that inherits from another before calling super, you get a somewhat obscure error: "|this| used uninitialized in [insert class name here] constructor"

This caught me off guard and confused me to no end for a while.

Perhaps we should add something like "Please call super() before accessing |this| on a constructor"?

Examples -- the following causes the error.

```
class A extends B {
  constructor() {
    this.someVariable = 'some value'; // fails
  }
}
```

and the following doesn't.

```
class A extends B {
  constructor() {
    super(); // ☜☜☜ ❗️❗️❗️
    this.someVariable = 'some value'; // works!
  }
}
```
Blocks: 622261
FWIW in Safari I get: ReferenceError: Cannot access uninitialized variable.

And Chrome: ReferenceError: Must call super constructor in derived class before accessing 'this' or returning from derived constructor
Keywords: triage-deferred
Priority: -- → P3
(Assignee)

Comment 2

a year ago
I'm working on this!
(Assignee)

Comment 3

a year ago
When you execute following code, you will get a confusing error message saying "|this| used uninitialized in A class constructor".
In this patch, you will get a clearer one saying "Must call super constructor before using |this| in A class constructor".

> class B {};
> 
> class A extends B {
>   constructor() {
>     // You need to call super(); here.
>     this.someVariable = 'some value'; // fails
>   }
> }
> 
> new A();

Also, when you execute following code, you will get a confusing error message saying "|this| used uninitialized in arrow function in class constructor".
In this patch, you will get a clearer one saying "Must call super constructor before using |this| in arrow function in class constructor".

> class B {};
> 
> class A extends B {
>   constructor() {
>     // You need to call super(); here.
>     (() => {
>         this.someVariable = 'some value'
>     })()
>   }
> }
> 
> new A();
Attachment #8924424 - Flags: review?(arai.unmht)
Assignee: nobody → mcdonalds.only
Status: NEW → ASSIGNED
Comment on attachment 8924424 [details] [diff] [review]
Improve error message for accessing this of uninitialized

Review of attachment 8924424 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for your patch :)

::: js/src/js.msg
@@ +102,4 @@
>  MSG_DEF(JSMSG_INVALID_ARG_TYPE,        3, JSEXN_TYPEERR, "Invalid type: {0} can't be a{1} {2}")
>  MSG_DEF(JSMSG_TERMINATED,              1, JSEXN_ERR, "Script terminated by timeout at:\n{0}")
>  MSG_DEF(JSMSG_CANT_CALL_CLASS_CONSTRUCTOR, 0, JSEXN_TYPEERR, "class constructors must be invoked with |new|")
> +MSG_DEF(JSMSG_UNINITIALIZED_THIS,      1, JSEXN_REFERENCEERR, "Must call super constructor before using |this| in {0} class constructor")

can you start with lowercase "m"?
most other error messages start with lowercase, so it should be better trying to keep consistent
(if we should start with upper case, it might be nice to file a dedicated bug for it)

@@ +102,5 @@
>  MSG_DEF(JSMSG_INVALID_ARG_TYPE,        3, JSEXN_TYPEERR, "Invalid type: {0} can't be a{1} {2}")
>  MSG_DEF(JSMSG_TERMINATED,              1, JSEXN_ERR, "Script terminated by timeout at:\n{0}")
>  MSG_DEF(JSMSG_CANT_CALL_CLASS_CONSTRUCTOR, 0, JSEXN_TYPEERR, "class constructors must be invoked with |new|")
> +MSG_DEF(JSMSG_UNINITIALIZED_THIS,      1, JSEXN_REFERENCEERR, "Must call super constructor before using |this| in {0} class constructor")
> +MSG_DEF(JSMSG_UNINITIALIZED_THIS_ARROW, 0, JSEXN_REFERENCEERR, "Must call super constructor before using |this| in arrow function in class constructor")

"... in derived class constructor", so that it's clear that the arrow function is inside derived class constructor
Attachment #8924424 - Flags: review?(arai.unmht) → feedback+
(Assignee)

Comment 5

a year ago
Thank you for your review!
I modified error messages.
Attachment #8924424 - Attachment is obsolete: true
Attachment #8924432 - Flags: review?(arai.unmht)
Comment on attachment 8924432 [details] [diff] [review]
Improve error message for accessing this of uninitialized

Review of attachment 8924432 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks again!

here's try run.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=46109b6a267136e28e46925dbae4061d3be50acb
Attachment #8924432 - Flags: review?(arai.unmht) → review+
(Assignee)

Comment 7

a year ago
I also modified jit test for error message which is changed in this patch.
Attachment #8924432 - Attachment is obsolete: true
Attachment #8924516 - Flags: review?(arai.unmht)
Comment on attachment 8924516 [details] [diff] [review]
Improve error message for accessing this of uninitialized

Review of attachment 8924516 [details] [diff] [review]:
-----------------------------------------------------------------

thanks!

here's another try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=789e306865f73c75f9df104a499d8e0934a13002
Attachment #8924516 - Flags: review?(arai.unmht) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 9

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdb23b22f19a
Improve error message for accessing this of uninitialized. r=arai
Keywords: checkin-needed

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fdb23b22f19a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.